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

[Laurent Pinchart]

On Wed, Jun 19, 2024 at 11:10:44PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-06-18 00:37:14)
> > 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.

I like that :-)

> > > +             }
> > >  
> > >               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