[libcamera-devel] [PATCH v2 4/6] pipeline: raspberrypi: Add IPA call tracepoints

[Laurent Pinchart]

Hi Paul,
On Wed, Oct 28, 2020 at 07:31:49PM +0900, Paul Elder wrote:

Thank you for the patch.

> Emit tracepoints for IPA calls in the raspberrypi pipeline handler, to
> allow profiling of IPA calls.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> New in v2
> 
> I was getting compiler complaints when I passed direct strings to the
> tracepoint without casting... I wasn't getting it in tests...

Can you share the compiler errors ? You cast away the const qualifier,
which should be done using a const_cast<> instead of a C-style cast, but
which should more importantly not be done at all :-) Let's try to fix
this.

> ---
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 0a5a7288..2de60d3d 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -31,6 +31,7 @@
>  #include "libcamera/internal/ipa_manager.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/pipeline_handler.h"
> +#include "libcamera/internal/tracepoints.h"
>  #include "libcamera/internal/utils.h"
>  #include "libcamera/internal/v4l2_controls.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
> @@ -1240,6 +1241,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  
>  void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)
>  {
> +	LIBCAMERA_TRACEPOINT(ipa_call_finish, (char *)"rpi", (char *)"signalStatReady");
> +

I have a few comments, but I think they can be addressed on top. Only
the const cast is something I'd like to see fixed. Of course if some of
my crazy ideas make sense to you and require little effort, feel free to
address them in v3 :-)

First of all, I wonder if we should create additional macros, for
instance

	LIBCAMERA_TRACEPOINT_IPA_END(rpi, signalStatReady);

and with a corresponding LIBCAMERA_TRACEPOINT_IPA_START.

Then, it would be really neat if all this could be handled completely
transparently in the generated code. We could have trace points for all
calls, in both directions. What we are missing today for this to work is
knowledge, in the code generator, that RPiIPA::runIsp() is the finish
point corresponding to RPiIPA::signalIspPrepare(). We could possibly
extend the IDL to record such information, but that's likely a lot of
work for little gain. I wouldn't if it wouldn't be best to record the
relationships between the calls in a custom format, that would then be
used as an input to an analyzer script to calculate the right timings.
We could store the information in a comment block in the mojom file, or
in an external file.

This is a bit of a rabbit hole, not something we need to address now,
but I'd like to hear opinions about the idea.

>  	if (state_ == State::Stopped)
>  		handleState();
>  
> @@ -1273,6 +1276,8 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
>  
>  void RPiCameraData::runIsp(uint32_t bufferId)
>  {
> +	LIBCAMERA_TRACEPOINT(ipa_call_finish, (char *)"rpi", (char *)"signalIspPrepare");
> +
>  	if (state_ == State::Stopped)
>  		handleState();
>  
> @@ -1406,6 +1411,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
>  	 * application until after the IPA signals so.
>  	 */
>  	if (stream == &isp_[Isp::Stats]) {
> +		LIBCAMERA_TRACEPOINT(ipa_call_start, (char *)"rpi", (char *)"signalStatReady");
>  		ipa_->signalStatReady(RPi::BufferMask::STATS | static_cast<unsigned int>(index));
>  	} else {
>  		/* Any other ISP output can be handed back to the application now. */
> @@ -1658,7 +1664,9 @@ void RPiCameraData::tryRunPipeline()
>  	 * queue the ISP output buffer listed in the request to start the HW
>  	 * pipeline.
>  	 */
> +	LIBCAMERA_TRACEPOINT(ipa_call_start, (char *)"rpi", (char *)"signalQueueRequest");
>  	ipa_->signalQueueRequest(request->controls());
> +	LIBCAMERA_TRACEPOINT(ipa_call_finish, (char *)"rpi", (char *)"signalQueueRequest");
>  
>  	/* Ready to use the buffers, pop them off the queue. */
>  	bayerQueue_.pop();
> @@ -1677,6 +1685,7 @@ void RPiCameraData::tryRunPipeline()
>  	ipa::rpi::IspPreparePayload ispPrepare;
>  	ispPrepare.embeddedbufferId_ = RPi::BufferMask::EMBEDDED_DATA | embeddedId;
>  	ispPrepare.bayerbufferId_ = RPi::BufferMask::BAYER_DATA | bayerId;
> +	LIBCAMERA_TRACEPOINT(ipa_call_start, (char *)"rpi", (char *)"signalIspPrepare");
>  	ipa_->signalIspPrepare(ispPrepare);
>  }
>  

-- 
Regards,

Laurent Pinchart