[libcamera-devel] [PATCH v3 02/14] ipu3: ipa: Report effective sensor controls with statistics to IPA

[Kieran Bingham]

Quoting Jean-Michel Hautbois (2021-11-11 11:05:53)
> From: Han-Lin Chen <hanlinchen at chromium.org>
> 
> The Intel close sourced IPA requires the effective controls applied to
> the sensor when the statistics are generated. Report effective sensor controls
> with the statistics to IPA.
> 
> Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> [Jean-Michel: Reword s/stastistics/statistics and move reset after IPA
> start]
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/libcamera/pipeline/ipu3/frames.h | 4 ++++
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h
> index 3ef7e445..614701e2 100644
> --- a/src/libcamera/pipeline/ipu3/frames.h
> +++ b/src/libcamera/pipeline/ipu3/frames.h
> @@ -14,6 +14,8 @@
>  
>  #include <libcamera/base/signal.h>
>  
> +#include <libcamera/controls.h>
> +
>  namespace libcamera {
>  
>  class FrameBuffer;
> @@ -34,6 +36,8 @@ public:
>                 FrameBuffer *paramBuffer;
>                 FrameBuffer *statBuffer;
>  
> +               ControlList effectiveSensorControls;
> +
>                 bool paramDequeued;
>                 bool metadataProcessed;
>         };
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 8816efc5..3fcfa777 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -767,6 +767,12 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis
>         if (ret)
>                 goto error;
>  
> +       /*
> +        * Reset the delayed controls with the gain and exposure values set by
> +        * the IPA.

I still think this comment is saying something is happening which isn't.

Just because this comment is in the RPi pipeline handler isn't an
excuse/reason to keep it here if it's not true.

RPi calls the following in PipelineHandlerRPi::start()
	/* Start the IPA. */
	ipa::RPi::StartConfig startConfig;
	data->ipa_->start(controls ? *controls : ControlList{ controls::controls },
			  &startConfig);

	/* Apply any gain/exposure settings that the IPA may have passed back. */
	if (!startConfig.controls.empty())
		data->setSensorControls(startConfig.controls);

before calling delayed controls reset.

If we aren't doing that, then the comment simply doesn't make sense
here.

Removed or fixed...

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +        */
> +       data->delayedCtrls_->reset();
> +
>         /*
>          * Start the ImgU video devices, buffers will be queued to the
>          * ImgU output and viewfinder when requests will be queued.
> @@ -1363,6 +1369,8 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
>         request->metadata().set(controls::SensorTimestamp,
>                                 buffer->metadata().timestamp);
>  
> +       info->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence);
> +
>         if (request->findBuffer(&rawStream_))
>                 pipe()->completeBuffer(request, buffer);
>  
> @@ -1419,6 +1427,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>         ev.frame = info->id;
>         ev.bufferId = info->statBuffer->cookie();
>         ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);
> +       ev.sensorControls = info->effectiveSensorControls;
>         ipa_->processEvent(ev);
>  }
>  
> -- 
> 2.32.0
>