[libcamera-devel] [PATCH v4 3/3] libcamera: pipeline: ipu3: Apply the requested test pattern mode
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Nov 9 12:54:09 CET 2021
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 ...
> > > + 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
> > >
More information about the libcamera-devel
mailing list