[libcamera-devel] [PATCH v7 3/3] libcamera: pipeline: ipu3: Apply a requested test pattern mode

[Jacopo Mondi]

Hi Hiro,

On Wed, Nov 24, 2021 at 04:08:12AM +0900, Hirokazu Honda wrote:
> This enables ipu3 pipeline handler to apply a test pattern mode
> per frame.

Yes, and this also  introduces a a way to set controls
immediately, the first (and only one for now) is the test pattern
mode.

Do you think it would be mentioned ?

>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 45 ++++++++++++++++++++++++++--
>  1 file changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 25490dcf..a15c6466 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -59,6 +59,7 @@ public:
>  	void statBufferReady(FrameBuffer *buffer);
>  	void queuePendingRequests();
>  	void cancelPendingRequests();
> +	void frameStart(uint32_t sequence);
>
>  	CIO2Device cio2_;
>  	ImgUDevice *imgu_;
> @@ -76,7 +77,10 @@ public:
>
>  	std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_;
>
> +	/* Requests before queueing cio2 device. */
>  	std::queue<Request *> pendingRequests_;
> +	/* Requests queued in cio2 device and before passing imgu device. */
> +	std::queue<Request *> processingRequests_;
>
>  	ControlInfoMap ipaControls_;
>
> @@ -803,6 +807,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>
>  	ret |= data->imgu_->stop();
>  	ret |= data->cio2_.stop();
> +

Intentional ?

>  	if (ret)
>  		LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();
>
> @@ -823,6 +828,8 @@ void IPU3CameraData::cancelPendingRequests()
>  		pipe()->completeRequest(request);
>  		pendingRequests_.pop();
>  	}
> +
> +	processingRequests_ = {};

Should processingRequests_ be completed one by one as the pending ones
instead of being dropped ?

>  }
>
>  void IPU3CameraData::queuePendingRequests()
> @@ -853,6 +860,8 @@ void IPU3CameraData::queuePendingRequests()
>
>  		info->rawBuffer = rawBuffer;
>
> +		processingRequests_.push(request);
> +
>  		ipa::ipu3::IPU3Event ev;
>  		ev.op = ipa::ipu3::EventProcessControls;
>  		ev.frame = info->id;
> @@ -1121,8 +1130,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);
>
>  		/* Convert the sensor rotation to a transformation */
>  		int32_t rotation = 0;
> @@ -1414,6 +1423,38 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>  	ipa_->processEvent(ev);
>  }
>
> +void IPU3CameraData::frameStart(uint32_t sequence)

Maybe a small comment block on the function

/*
 * Handle the start of frame exposure signal.
 *
 * Inspect the list of pending requests waiting for a RAW frame to be
 * produced and apply controls for the 'next' one.
 *
 * Some controls need to be applied immediately, such as the
 * TestPatternMode one. Other controls are handled through the delayed
 * controls class.
 */

This shows that this functions does two things:
- Apply some libcamera::controls from the Request immediately
- Inform the DelayedControl class that a new frame has started to
  advance its internal frame-counting

> +{
> +	if (!processingRequests_.empty()) {
> +		/* Handle controls which are to be set ready for the next frame to start. */

                /* Apply controls that have to take effect immediately. */

I now wonder how did we get to the notion that some controls take
effect 'immediately'. Did we get here simply because we had to way to
apply libcamera::controls to CameraSensor without going through the
IPA ? I then wonder if talking about 'immediate' and 'next frame' is
not misleading. Aren't we just applying some controls which the
pipelinehandler is in charge of bypassing the IPA ?

do we need a notion of immediate controls ?

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

		if (!request->controls().contains(controls::draft::TestPatternMode))
                        break;

And reduce the indendation below

> +			const int32_t testPatternMode = request->controls().get(
> +				controls::draft::TestPatternMode);
> +
> +			LOG(IPU3, Debug) << "Apply test pattern mode: "
> +					 << testPatternMode;
> +
> +			int ret = cio2_.sensor()->setTestPatternMode(
> +				static_cast<controls::draft::TestPatternModeEnum>(
> +					testPatternMode));
> +			if (ret) {
> +				LOG(IPU3, Error) << "Failed to set test pattern mode: "
> +						 << ret;
                                break;
And remove the else {} clause

> +			} else {
> +				request->metadata().set(controls::draft::TestPatternMode,
> +							testPatternMode);
> +			}
> +		}
> +	}
> +
> +	/* Controls that don't affect immediately are applied in delayedCtrls. */

I would drop this with the block at the beginning of the function

> +	delayedCtrls_->applyControls(sequence);

Minors apart

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
   j

> +}
> +
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)
>
>  } /* namespace libcamera */
> --
> 2.34.0.rc2.393.gf8c9666880-goog
>