[{"id":13542,"web_url":"https://patchwork.libcamera.org/comment/13542/","msgid":"<20201028235434.GC10030@pendragon.ideasonboard.com>","date":"2020-10-28T23:54:34","subject":"Re: [libcamera-devel] [PATCH v2 4/6] pipeline: raspberrypi: Add IPA\n\tcall tracepoints","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\nOn Wed, Oct 28, 2020 at 07:31:49PM +0900, Paul Elder wrote:\n\nThank you for the patch.\n\n> Emit tracepoints for IPA calls in the raspberrypi pipeline handler, to\n> allow profiling of IPA calls.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> New in v2\n> \n> I was getting compiler complaints when I passed direct strings to the\n> tracepoint without casting... I wasn't getting it in tests...\n\nCan you share the compiler errors ? You cast away the const qualifier,\nwhich should be done using a const_cast<> instead of a C-style cast, but\nwhich should more importantly not be done at all :-) Let's try to fix\nthis.\n\n> ---\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 9 +++++++++\n> 1 file changed, 9 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 0a5a7288..2de60d3d 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -31,6 +31,7 @@\n> #include \"libcamera/internal/ipa_manager.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> +#include \"libcamera/internal/tracepoints.h\"\n> #include \"libcamera/internal/utils.h\"\n> #include \"libcamera/internal/v4l2_controls.h\"\n> #include \"libcamera/internal/v4l2_videodevice.h\"\n> @@ -1240,6 +1241,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \n> void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)\n> {\n> +\tLIBCAMERA_TRACEPOINT(ipa_call_finish, (char *)\"rpi\", (char *)\"signalStatReady\");\n> +\n\nI have a few comments, but I think they can be addressed on top. Only\nthe const cast is something I'd like to see fixed. Of course if some of\nmy crazy ideas make sense to you and require little effort, feel free to\naddress them in v3 :-)\n\nFirst of all, I wonder if we should create additional macros, for\ninstance\n\n\tLIBCAMERA_TRACEPOINT_IPA_END(rpi, signalStatReady);\n\nand with a corresponding LIBCAMERA_TRACEPOINT_IPA_START.\n\nThen, it would be really neat if all this could be handled completely\ntransparently in the generated code. We could have trace points for all\ncalls, in both directions. What we are missing today for this to work is\nknowledge, in the code generator, that RPiIPA::runIsp() is the finish\npoint corresponding to RPiIPA::signalIspPrepare(). We could possibly\nextend the IDL to record such information, but that's likely a lot of\nwork for little gain. I wouldn't if it wouldn't be best to record the\nrelationships between the calls in a custom format, that would then be\nused as an input to an analyzer script to calculate the right timings.\nWe could store the information in a comment block in the mojom file, or\nin an external file.\n\nThis is a bit of a rabbit hole, not something we need to address now,\nbut I'd like to hear opinions about the idea.\n\n> \tif (state_ == State::Stopped)\n> \t\thandleState();\n> \n> @@ -1273,6 +1276,8 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n> \n> void RPiCameraData::runIsp(uint32_t bufferId)\n> {\n> +\tLIBCAMERA_TRACEPOINT(ipa_call_finish, (char *)\"rpi\", (char *)\"signalIspPrepare\");\n> +\n> \tif (state_ == State::Stopped)\n> \t\thandleState();\n> \n> @@ -1406,6 +1411,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> \t * application until after the IPA signals so.\n> \t */\n> \tif (stream == &isp_[Isp::Stats]) {\n> +\t\tLIBCAMERA_TRACEPOINT(ipa_call_start, (char *)\"rpi\", (char *)\"signalStatReady\");\n> \t\tipa_->signalStatReady(RPi::BufferMask::STATS | static_cast(index));\n> \t} else {\n> \t\t/* Any other ISP output can be handed back to the application now. */\n> @@ -1658,7 +1664,9 @@ void RPiCameraData::tryRunPipeline()\n> \t * queue the ISP output buffer listed in the request to start the HW\n> \t * pipeline.\n> \t */\n> +\tLIBCAMERA_TRACEPOINT(ipa_call_start, (char *)\"rpi\", (char *)\"signalQueueRequest\");\n> \tipa_->signalQueueRequest(request->controls());\n> +\tLIBCAMERA_TRACEPOINT(ipa_call_finish, (char *)\"rpi\", (char *)\"signalQueueRequest\");\n> \n> \t/* Ready to use the buffers, pop them off the queue. */\n> \tbayerQueue_.pop();\n> @@ -1677,6 +1685,7 @@ void RPiCameraData::tryRunPipeline()\n> \tipa::rpi::IspPreparePayload ispPrepare;\n> \tispPrepare.embeddedbufferId_ = RPi::BufferMask::EMBEDDED_DATA | embeddedId;\n> \tispPrepare.bayerbufferId_ = RPi::BufferMask::BAYER_DATA | bayerId;\n> +\tLIBCAMERA_TRACEPOINT(ipa_call_start, (char *)\"rpi\", (char *)\"signalIspPrepare\");\n> \tipa_->signalIspPrepare(ispPrepare);\n> }\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BA667BDB1E\n\tfor ;\n\tWed, 28 Oct 2020 23:55:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2BACE62825;\n\tThu, 29 Oct 2020 00:55:25 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 93383627A2\n\tfor ;\n\tThu, 29 Oct 2020 00:55:23 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 002F8576;\n\tThu, 29 Oct 2020 00:55:22 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"J937uTna\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603929323;\n\tbh=Jf2Jp0lkaaGjT6rrYjKnLI+HKeub5qNcRV+3JVhH4sA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=J937uTnaX/jU3j+IJbocvHRAu7knilRFZOubQRYmnblCzZv9AQeVUKAnh23Kx8aFg\n\tgqV2KPbXcCI1BazN6xxJ0s9bAa2qZ2O5d2hb4lDjSic6O5hDCC8+fWqhdqyWd4/KBN\n\thpHbf8cqWpa80XPtfVRByFcRcJS4IfPy5pNvAZAA=","Date":"Thu, 29 Oct 2020 01:54:34 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20201028235434.GC10030@pendragon.ideasonboard.com>","References":"<20201028103151.34575-1-paul.elder@ideasonboard.com>\n\t<20201028103151.34575-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201028103151.34575-4-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 4/6] pipeline: raspberrypi: Add IPA\n\tcall tracepoints","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]