[PATCH v6 11/18] libcamera: software_isp: Call Algorithm::prepare

[Milan Zamazal]

Hi Kieran,

thank you for review.

Kieran Bingham <kieran.bingham at ideasonboard.com> writes:

> Quoting Kieran Bingham (2024-09-09 21:09:20)
>> Quoting Dan Scally (2024-09-09 09:21:45)
>> > Hi Milan
>
>> > 
>> > On 06/09/2024 13:09, Milan Zamazal wrote:
>> > > This patch adds Algorithm::prepare call for the defined algorithms.
>> > > This is preparation only since there are currently no Algorithm based
>> > > algorithms defined.
>> > >
>> > > Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> > > Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
>> > > ---
>> > 
>> > Reviewed-by: Daniel Scally <dan.scally at ideasonboard.com>
>> > 
>> > >   include/libcamera/ipa/soft.mojom            | 1 +
>> > >   src/ipa/simple/soft_simple.cpp              | 8 ++++++++
>> > >   src/libcamera/software_isp/software_isp.cpp | 1 +
>> > >   3 files changed, 10 insertions(+)
>> > >
>> > > diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
>> > > index ddccd154..347fd69b 100644
>> > > --- a/include/libcamera/ipa/soft.mojom
>> > > +++ b/include/libcamera/ipa/soft.mojom
>> > > @@ -24,6 +24,7 @@ interface IPASoftInterface {
>> > >               => (int32 ret);
>> > >   
>> > >       [async] queueRequest(uint32 frame, libcamera.ControlList sensorControls);
>> > > +     [async] fillParamsBuffer(uint32 frame);
>> > >       [async] processStats(uint32 frame,
>> > >                            uint32 bufferId,
>> > >                            libcamera.ControlList sensorControls);
>> > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> > > index eb3bbd92..3a0cb6e0 100644
>> > > --- a/src/ipa/simple/soft_simple.cpp
>> > > +++ b/src/ipa/simple/soft_simple.cpp
>> > > @@ -79,6 +79,7 @@ public:
>> > >       void stop() override;
>> > >   
>> > >       void queueRequest(const uint32_t frame, const ControlList &controls) override;
>> > > +     void fillParamsBuffer(const uint32_t frame) override;
>> > >       void processStats(const uint32_t frame, const uint32_t bufferId,
>> > >                         const ControlList &sensorControls) override;
>> > >   
>> > > @@ -279,6 +280,13 @@ void IPASoftSimple::queueRequest(const uint32_t frame, const ControlList &contro
>> > >               algo->queueRequest(context_, frame, frameContext, controls);
>> > >   }
>> > >   
>> > > +void IPASoftSimple::fillParamsBuffer(const uint32_t frame)
>> > > +{
>> > > +     IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>> > > +     for (auto const &algo : algorithms())
>> > > +             algo->prepare(context_, frame, frameContext, params_);
>> 
>> This doesn't fill a parameter buffer. Do we anticipate having a
>> structured buffer to pass to the SoftISP components?
>> 
>> Otherwise perhaps we should just call this
>>         'IPASoftSimple::prepare(const uint32_t frame)'

It has been renamed from IPASoftSimple::prepare to
IPASoftSimple::fillParamsBuffer on Laurent's request, for consistency
with the other pipelines.  params_ is a mmaped structure, a single slot
currently but it will be selected from a ring in a followup patch
series; so I think "buffer" in the name is not completely off.

>> > > +}
>> > > +
>> > >   void IPASoftSimple::processStats([[maybe_unused]] const uint32_t frame,
>> > >                                [[maybe_unused]] const uint32_t bufferId,
>> > >                                const ControlList &sensorControls)
>> > > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
>> > > index eda18947..564e44e8 100644
>> > > --- a/src/libcamera/software_isp/software_isp.cpp
>> > > +++ b/src/libcamera/software_isp/software_isp.cpp
>> > > @@ -352,6 +352,7 @@ void SoftwareIsp::stop()
>> > >    */
>> > >   void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output)
>> > >   {
>> > > +     ipa_->fillParamsBuffer(frame);
>> 
>> Oh... are we mixing prepare and process calls?
>> 
>> I'll have to take a look at the bigger picture ....
>
> Aha - no - this does make sense because this is the 'process' stages of
> the SoftISP which is the 'running' process.

Yes.

> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>> 
>> > >       debayer_->invokeMethod(&DebayerCpu::process,
>> > >                              ConnectionTypeQueued, frame, input, output, debayerParams_);
>> > >   }