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

[Kieran Bingham]

Quoting Hirokazu Honda (2021-11-04 06:42:52)
> This enables ipu3 pipeline handler to apply the test pattern mode
> per frame.
> 
> 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 | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 63cb7f11..cb141ba9 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_;
> @@ -78,6 +79,7 @@ public:
>         std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_;
>  
>         std::queue<Request *> pendingRequests_;
> +       std::queue<Request *> processingRequests_;

I'm a bit weary of having requests on multiple queues like this.
It might be better if we could add some documentation describing the
purpose of each queue, and what lifetime a request has on each one.


Perhaps a banner comment before processingRequests_ to describe briefly
that the requests will sit on this queue to synchronise setting controls
at frameStart time? I'm not sure what level will help - but it seems a
bit vague for 'processingRequests' and 'pendingRequests' otherwise.



>  
>         ControlInfoMap ipaControls_;
>  
> @@ -569,6 +571,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>         cio2->sensor()->sensorInfo(&sensorInfo);
>         data->cropRegion_ = sensorInfo.analogCrop;
>  
> +       cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff);
> +

This looks like something the CameraSensor should do at the end of initialisation. (CameraSensor::init())


>         /*
>          * Configure the H/V flip controls based on the combination of
>          * the sensor and user transform.
> @@ -797,11 +801,13 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>         int ret = 0;
>  
>         data->cancelPendingRequests();
> +       data->processingRequests_ = {};

We add requests to the processingRequests_ during
queuePendingRequests(), so maybe we should clear them during
cancelPendingRequests()?

>         data->ipa_->stop();
>  
>         ret |= data->imgu_->stop();
>         ret |= data->cio2_.stop();
> +
>         if (ret)
>                 LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();
>  
> @@ -852,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;
> @@ -1131,8 +1139,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);

I like having an explicit/clear frameStart event function here.

>  
>                 /* Convert the sensor rotation to a transformation */
>                 int32_t rotation = 0;
> @@ -1423,6 +1431,22 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>         ipa_->processEvent(ev);
>  }
>  
> +void IPU3CameraData::frameStart(uint32_t sequence)
> +{
> +       if (!processingRequests_.empty()) {
> +               /* Assumes applying the test pattern mode affects immediately. */

That comment seems misplaced now.
Perhaps you could add:

		  /*
		   * Handle controls which are to be set ready for the
		   * next frame to start.
		   */

But I'm not sure of the race/sequencing here. I presume at the point we
have a frameStart event, any controls we apply will take effect for the
/next/ frame ? This frame has already started...

Or do we expect to set a control here and have it take effect on this
frame? (That sounds way too racy to be possible).

We're going to need to be able to set other controls at this point,
perhaps from the IPA too, so I expect we'll see some further rework here 
later.

> +               Request *request = processingRequests_.front();
> +               processingRequests_.pop();
> +
> +               int ret = cio2_.sensor()->applyRequestControls(request);
> +               if (ret)
> +                       LOG(IPU3, Error) << "Failed applying controls: " << ret;
> +       }
> +
> +       /* Controls that don't affect immediately are applied in delayedCtrls. */
> +       delayedCtrls_->applyControls(sequence);
> +}
> +
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)
>  
>  } /* namespace libcamera */
> -- 
> 2.33.1.1089.g2158813163f-goog
>