[RFC PATCH 5/7] pipeline: rkisp1: Apply initial controls

[Kieran Bingham]

Quoting Laurent Pinchart (2025-01-12 22:00:40)
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Fri, Dec 20, 2024 at 05:26:51PM +0100, Stefan Klug wrote:
> > Controls passed to Camera::start() are not handled at the moment.
> > Implement that by issuing a dummy queueRequest() to initialize the frame
> > context and then returning the controls synchronously to the caller. In
> > the pipeline the controls get passed to the sensor before the call to
> > stremOn() to ensure the controls are applied right away. Passing the
> 
> s/stremOn/streamOn/
> 
> > controls to the pipeline using the setSensorControls signal is not
> > appropriate because it is asynchronous and would reach the sensor too
> > late (initial controls need to be applied before the sensor starts and
> > before delayed controls initializes it's start condition.)
> > 
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> >  include/libcamera/ipa/rkisp1.mojom       |  7 ++++++-
> >  src/ipa/rkisp1/rkisp1.cpp                | 15 ++++++++++-----
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 11 +++++++++--
> >  3 files changed, 25 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > index 043ad27ea199..86eae55cd530 100644
> > --- a/include/libcamera/ipa/rkisp1.mojom
> > +++ b/include/libcamera/ipa/rkisp1.mojom
> > @@ -14,13 +14,18 @@ struct IPAConfigInfo {
> >       uint32 paramFormat;
> >  };
> >  
> > +struct StartResult {
> > +     libcamera.ControlList controls;
> > +     int32 code;
> > +};
> > +
> >  interface IPARkISP1Interface {
> >       init(libcamera.IPASettings settings,
> >            uint32 hwRevision,
> >            libcamera.IPACameraSensorInfo sensorInfo,
> >            libcamera.ControlInfoMap sensorControls)
> >               => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > -     start() => (int32 ret);
> > +     start(libcamera.ControlList controls) => (StartResult result);
> >       stop();
> >  
> >       configure(IPAConfigInfo configInfo,
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 45a539a61fb1..799fe0846635 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -55,7 +55,7 @@ public:
> >                const IPACameraSensorInfo &sensorInfo,
> >                const ControlInfoMap &sensorControls,
> >                ControlInfoMap *ipaControls) override;
> > -     int start() override;
> > +     void start(const ControlList &controls, StartResult *result) override;
> >       void stop() override;
> >  
> >       int configure(const IPAConfigInfo &ipaConfig,
> > @@ -209,12 +209,17 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> >       return 0;
> >  }
> >  
> > -int IPARkISP1::start()
> > +void IPARkISP1::start(const ControlList &controls, StartResult *result)
> >  {
> > -     ControlList ctrls = getSensorControls(0);
> > -     setSensorControls.emit(0, ctrls);
> > +     /*
> > +      * \todo: This feels a bit counter intuitive, as the queueRequest() for frame
> > +      * 0 will be called again when the actual request for frame 0 get's
> > +      * queued.
> > +      */
> 
> s/todo:/todo/
> 
> It's more than counterintuitive, I think it's an important design
> problem. I'm OK with this dirty hack for now, but I think it should be
> addressed sooner than later. Can you word the \todo comment a bit
> stronger ?
> 
> > +     queueRequest(0, controls);

This means we call IPARkISP1::queueRequest(0, ...) twice,
which means we call 
	IPAFrameContext &frameContext = context_.frameContexts.alloc(0);

twice. Does that result in a double allocation? Or is it ok ?

I followed it through myself, and it seems it's fine, as it only obtains
the 0'th element from the circular queue, but it will call
init(frameContext, frame) and re-zero that framecontext, erasing any
controls you might have put in there from this start list.

I think that's ok - but best to be aware of it. Possibly even with a
comment saying that the controls will not be maintained in the frame
context queue...



> >  
> > -     return 0;
> > +     result->controls = getSensorControls(0);
> > +     result->code = 0;
> >  }
> >  
> >  void IPARkISP1::stop()
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 71a07c6027b9..ee6ed1b9c84a 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -1086,13 +1086,20 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
> >               return ret;
> >       actions += [&]() { freeBuffers(camera); };
> >  
> > -     ret = data->ipa_->start();
> > -     if (ret) {
> > +     ControlList ctrls;
> > +     if (!!controls)
> > +             ctrls = *controls;
> > +
> > +     ipa::rkisp1::StartResult res;
> > +     data->ipa_->start(ctrls, &res);
> > +     if (res.code) {
> >               LOG(RkISP1, Error)
> >                       << "Failed to start IPA " << camera->id();
> >               return ret;
> >       }
> >       actions += [&]() { data->ipa_->stop(); };
> > +     data->sensor_->setControls(&res.controls);
> > +     data->delayedCtrls_->reset();
> 
> Resetting the delayed controls seems to be an important bug fix. It
> should be mentioned in the commit message, or better split to a separate
> commit.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> >  
> >       data->frame_ = 0;
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart