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

[Hirokazu Honda]

Hi Kieran,

On Tue, Nov 9, 2021 at 8:54 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting Hirokazu Honda (2021-11-09 06:39:43)
> > Hi Kieran,
> >
> > On Tue, Nov 9, 2021 at 7:16 AM Kieran Bingham
> > <kieran.bingham at ideasonboard.com> wrote:
> > >
> > > 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.
> > >
> >
> > Sorry, I don't get this comment. Could you elaborate?
>
> I meant that I think this is good ;-)
>
> It makes an explicit call on the IPU3CameraData when there is a
> frameStart event, rather than the short cut to delayed controls..
>
> > > >
> > > >                 /* 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).
> > >
> >
> > That's good point.. It is difficult to set control in a proper time as
> > we're not sure when the proper time is.
> > We also need to think about the effect to the proceeding frames that
> > are being processed.
> >
> > Thanks,
> > -Hiro
> > > 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);
>
> Talking with Jacopo, he really dislikes passing the request to the
> sensor ;-( - While I dislike having to filter out the test pattern
> control (or CameraSensor specific controls) in the frameStart event for
> each pipeline handler.
>
> Not sure what the middle ground is yet. But it might just be renaming
> applyRequestControls() to applyTestPatternControls() or something that
> can't be suggested that it might be used by other controls. Perhaps
> that's closer to your original implementation anyway ...
>

I will ask Laurent about this today.

-Hiro
>
> > > > +               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
> > > >