[libcamera-devel] [PATCH v7 3/3] libcamera: pipeline: ipu3: Apply a requested test pattern mode
Jacopo Mondi
jacopo at jmondi.org
Fri Nov 26 09:27:04 CET 2021
Hi Hiro,
On Fri, Nov 26, 2021 at 01:42:05PM +0900, Hirokazu Honda wrote:
> Hi Jacopo, thank you for reviewing.
>
> On Thu, Nov 25, 2021 at 9:00 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > 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 ?
> >
>
> Hmm, this is a good question.
> I think the metadata for aborted capture doesn't have to be applied to a camera.
> So I just discard pending requests.
>
So we'll lose requests when the Camera is stopped. Please have a look
at IPU3CameraData::cancelPendingRequests(). I think we'll have to do
the same for processingRequests_
> > > }
> > >
> > > 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 ?
>
> It effects immediately that a control is applied to a capture which
> associates the control.
> I think the time of applying the control is dependent on camera hardware.
>
>
> >
> > > + 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
> >
>
> hmm, break cannot be used because this is neither for nor while.
Ah ups
> I think the later delayedCtrls can be invoked in the begging of the
> function and then can use "return"?
That would be nice, thanks
>
> -Hiro
> > > + } 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
> > >
More information about the libcamera-devel
mailing list