[PATCH 1/3] ipa: simple: softisp: Extend to pass metadata

[Kieran Bingham]

Quoting Laurent Pinchart (2024-06-18 00:37:14)
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Tue, Jun 18, 2024 at 12:25:23AM +0100, Kieran Bingham wrote:
> > Extend the Simple IPA IPC to support returning a metadata controllist
> 
> s/controllist/ControlList/
> 
> > when the process call has completed.
> > 
> > For efficiency, use the existing signal setIspParams() directly
> > to avoid having an extra async callback to return the metadata.
> > 
> > Merge the metadata reported by the ISP into any completing
> > request to provide to the application.
> > 
> > This should be further extended or improved to make use of the frame
> > context structures so that the metadata is tied to a specific frame and
> > request completion.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  include/libcamera/internal/software_isp/software_isp.h | 5 ++++-
> >  include/libcamera/ipa/soft.mojom                       | 2 +-
> >  src/ipa/simple/soft_simple.cpp                         | 4 +++-
> >  src/libcamera/pipeline/simple/simple.cpp               | 7 +++++--
> >  src/libcamera/software_isp/software_isp.cpp            | 6 ++++--
> >  5 files changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> > index c5338c05b99b..2631f40ef22a 100644
> > --- a/include/libcamera/internal/software_isp/software_isp.h
> > +++ b/include/libcamera/internal/software_isp/software_isp.h
> > @@ -80,8 +80,10 @@ public:
> >       Signal<> ispStatsReady;
> >       Signal<const ControlList &> setSensorControls;
> >  
> > +     const ControlList &metadata() { return metadata_; }
> > +
> >  private:
> > -     void saveIspParams();
> > +     void saveIspParams(const ControlList &metadata);
> >       void setSensorCtrls(const ControlList &sensorControls);
> >       void statsReady();
> >       void inputReady(FrameBuffer *input);
> > @@ -92,6 +94,7 @@ private:
> >       SharedMemObject<DebayerParams> sharedParams_;
> >       DebayerParams debayerParams_;
> >       DmaBufAllocator dmaHeap_;
> > +     ControlList metadata_;
> >  
> >       std::unique_ptr<ipa::soft::IPAProxySoft> ipa_;
> >  };
> > diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> > index 3aa2066ebaa0..1a0221df7660 100644
> > --- a/include/libcamera/ipa/soft.mojom
> > +++ b/include/libcamera/ipa/soft.mojom
> > @@ -24,5 +24,5 @@ interface IPASoftInterface {
> >  
> >  interface IPASoftEventInterface {
> >       setSensorControls(libcamera.ControlList sensorControls);
> > -     setIspParams();
> > +     setIspParams(libcamera.ControlList metadata);
> >  };
> > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> > index b7746ce09206..09c7b575301e 100644
> > --- a/src/ipa/simple/soft_simple.cpp
> > +++ b/src/ipa/simple/soft_simple.cpp
> > @@ -250,6 +250,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
> >       SwIspStats::Histogram histogram = stats_->yHistogram;
> >       if (ignoreUpdates_ > 0)
> >               blackLevel_.update(histogram);
> > +
> > +     ControlList metadata(controls::controls);
> >       const uint8_t blackLevel = blackLevel_.get();
> >  
> >       /*
> > @@ -303,7 +305,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
> >               params_->blue[i] = gammaTable_[idx];
> >       }
> >  
> > -     setIspParams.emit();
> > +     setIspParams.emit(metadata);
> >  
> >       /* \todo Switch to the libipa/algorithm.h API someday. */
> >  
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index eb36578e320e..accdb744d61b 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -860,10 +860,13 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
> >                       return;
> >               }
> >  
> > -             if (converter_)
> > +             if (converter_) {
> >                       converter_->queueBuffers(buffer, conversionQueue_.front());
> > -             else
> > +             } else {
> >                       swIsp_->queueBuffers(buffer, conversionQueue_.front());
> > +                     if (request)
> > +                             request->metadata().merge(swIsp_->metadata());
> 
> You're shoving metadata in a random request here, that really makes me
> cringe. It's not just that there's no frame context tracking on the IPA
> side, the pipeline handler side is wrong too.

Oh, yes - I'm aware ;-)

There's lots more to do on the pipeline handler to fix up managing
controls and synchronisation.

I know Milan has a branch with some work that gets closer to this on
the IPA side already too, so I'm looking forward to that getting out.

As I said to Robert on IRC when I sent these, it was a case of 'no point
them sitting on my PC when they could be a starting point/reference'.

What I want to make obvious or easier - is that we can put more data in
here such as timing or measurements of the internal performances to be
able to give constant monitoring of the 'cost' of the SoftISP.



> > +             }
> >  
> >               conversionQueue_.pop();
> >               return;
> > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> > index 20fb6f48fdf9..efbbea2d2279 100644
> > --- a/src/libcamera/software_isp/software_isp.cpp
> > +++ b/src/libcamera/software_isp/software_isp.cpp
> > @@ -68,7 +68,8 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)
> >  SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)
> >       : dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |
> >                  DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap |
> > -                DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)
> > +                DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf),
> > +       metadata_(controls::controls)
> >  {
> >       /*
> >        * debayerParams_ must be initialized because the initial value is used for
> > @@ -349,9 +350,10 @@ void SoftwareIsp::process(FrameBuffer *input, FrameBuffer *output)
> >                              ConnectionTypeQueued, input, output, debayerParams_);
> >  }
> >  
> > -void SoftwareIsp::saveIspParams()
> > +void SoftwareIsp::saveIspParams(const ControlList &metadata)
> >  {
> >       debayerParams_ = *sharedParams_;
> > +     metadata_.merge(metadata, ControlList::MergePolicy::OverwriteExisting);
> >  }
> >  
> >  void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls)
> 
> -- 
> Regards,
> 
> Laurent Pinchart