[libcamera-devel] [RFC PATCH v2] libcamera: pipeline: ipu3: Apply the requested test pattern mode

[Jean-Michel Hautbois]

Hi Hiro,

Thanks for the patch !

On 05/10/2021 07:44, Hirokazu Honda wrote:
> This enables ipu3 pipeline handler to apply the test pattern mode
> per frame.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 92e86925..07fda65f 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -69,6 +69,7 @@ public:
>  	void statBufferReady(FrameBuffer *buffer);
>  	void queuePendingRequests();
>  	void cancelPendingRequests();
> +	void frameStart(uint32_t sequence);
>  
>  	CIO2Device cio2_;
>  	ImgUDevice *imgu_;
> @@ -88,6 +89,7 @@ public:
>  	std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_;
>  
>  	std::queue<Request *> pendingRequests_;
> +	std::queue<Request *> processingRequests_;
>  
>  	ControlInfoMap ipaControls_;
>  
> @@ -568,6 +570,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  	cio2->sensor()->sensorInfo(&sensorInfo);
>  	data->cropRegion_ = sensorInfo.analogCrop;
>  
> +	cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff);

There is a dependency on "[PATCH] libcamera: camera_sensor: Enable to
set a test pattern mode" (20211005043723.568685-1-hiroh at chromium.org), 
right ?
Could you make it a series please :-) ?

> +
>  	/*
>  	 * Configure the H/V flip controls based on the combination of
>  	 * the sensor and user transform.
> @@ -801,6 +805,9 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>  
>  	ret |= data->imgu_->stop();
>  	ret |= data->cio2_.stop();
> +
> +	data->processingRequests_ = {};

Shouldn't we do that in cancelPendingRequests() as a mirror of the push 
which is done in queuePendingRequests() ?

I am a bit worry that it is done after cio2_->stop()...

> +
>  	if (ret)
>  		LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();
>  
> @@ -851,6 +858,8 @@ void IPU3CameraData::queuePendingRequests()
>  
>  		info->rawBuffer = rawBuffer;
>  
> +		processingRequests_.push(request);
> +
>  		ipa::ipu3::IPU3Event ev;
>  		ev.op = ipa::ipu3::EventProcessControls;
>  		ev.frame = info->id;
> @@ -1107,8 +1116,8 @@ int PipelineHandlerIPU3::registerCameras()
>  		data->delayedCtrls_ =
>  			std::make_unique<DelayedControls>(cio2->sensor()->device(),
>  							  params);
> -		data->cio2_.frameStart().connect(data->delayedCtrls_.get(),
> -						 &DelayedControls::applyControls);
> +		data->cio2_.frameStart().connect(data.get(),
> +						 &IPU3CameraData::frameStart);

The IPA will receive the ipa::ipu3::EventProcessControls immediately, so 
it could be notified to stop trying to apply the algorithm for instance 
(to be discussed). I think this is good :-).

>  
>  		/* Convert the sensor rotation to a transformation */
>  		int32_t rotation = 0;
> @@ -1399,6 +1408,31 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>  	ipa_->processEvent(ev);
>  }
>  
> +void IPU3CameraData::frameStart(uint32_t sequence)
> +{
> +	if (!processingRequests_.empty()) {

Add a LOG(IPU3, Debug) here ?

> +		/* Assumes applying the test pattern mode affects immediately. */
> +		Request *request = processingRequests_.front();
> +		processingRequests_.pop();
> +
> +		if (request->controls().contains(controls::draft::TestPatternMode)) {

Same here, to see which test pattern is applied ?

> +			const int32_t testPatternMode = request->controls().get(
> +				controls::draft::TestPatternMode);
> +
> +			int ret = cio2_.sensor()->setTestPatternMode(testPatternMode);
> +			if (ret) {
> +				LOG(IPU3, Error) << "Failed to set test pattern mode: "
> +						 << ret;
> +			} else {
> +				request->metadata().set(controls::draft::TestPatternMode,
> +							testPatternMode);
> +			}
> +		}
> +	}
> +

And a small comment for this line ?

> +	delayedCtrls_->applyControls(sequence);
> +}
> +
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)
>  
>  } /* namespace libcamera */
> 

Now, this patch makes the per-frame control on the IPA point of view 
more important... How will we handle those ? This is still an open 
question...

Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>