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

[Hirokazu Honda]

Hi Jean-Michel,

On Mon, Oct 25, 2021 at 3:30 PM Jean-Michel Hautbois
<jeanmichel.hautbois at ideasonboard.com> wrote:
>
> 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 ?
>

I would not do it because processingReqeusts_ is often not empty and
therefore adding LOG here is verbose.

Thanks,
-Hiro
> > +             /* 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>