[PATCH v3 1/6] libcamera: software_isp: Track frames and requests

[Barnabás Pőcze]

2025. január 27., hétfő 19:35 keltezéssel, Kieran Bingham <kieran.bingham at ideasonboard.com> írta:

> Quoting Laurent Pinchart (2025-01-27 06:59:36)
> > Hi Milan,
> >
> > Thank you for the patch.
> >
> > On Mon, Jan 13, 2025 at 02:34:00PM +0100, Milan Zamazal wrote:
> > > Hardware pipelines track requests and other information related to
> > > particular frames.  This hasn't been needed in software ISP so far.  But
> > > in order to be able to track metadata corresponding to a given frame,
> > > frame-request tracking mechanism starts being useful.  This patch
> > > introduces the basic tracking structure, actual metadata handling is
> > > added in the following patch.
> > >
> 
> I'm also weary that Barnabás made some fixes to other IPA 'frame
> handlers' recently.

The changes in the proposed https://patchwork.libcamera.org/patch/22610/ patch
have already been incorporated into this patch set.


> 
> I think somehow this concept needs to be templated to make sure we don't
> have different bugs and different implementation in each IPA...
> 
> Not a fault of 'this' patch, but I'm really weary about seeing multiple
> implementations of the same object.

I agree.


Regards,
Barnabás Pőcze


> 
> > > Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> > > ---
> > >  src/libcamera/pipeline/simple/simple.cpp | 81 ++++++++++++++++++++++--
> > >  1 file changed, 76 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > index 8ac24e6e..123179b6 100644
> > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > @@ -181,6 +181,64 @@ LOG_DEFINE_CATEGORY(SimplePipeline)
> > >
> > >  class SimplePipelineHandler;
> > >
> > > +struct SimpleFrameInfo {
> > > +     uint32_t frame;
> > > +     Request *request;
> > > +};
> > > +
> > > +class SimpleFrames
> > > +{
> > > +public:
> > > +     void create(Request *request);
> > > +     void destroy(uint32_t frame);
> > > +     void clear();
> > > +
> > > +     SimpleFrameInfo *find(uint32_t frame);
> > > +     SimpleFrameInfo *find(Request *request);
> > > +
> > > +private:
> > > +     std::map<uint32_t, SimpleFrameInfo> frameInfo_;
> > > +};
> > > +
> > > +void SimpleFrames::create(Request *request)
> > > +{
> > > +     uint32_t frame = request->sequence();
> > > +     auto [it, inserted] = frameInfo_.try_emplace(request->sequence());
> > > +     ASSERT(inserted);
> > > +     it->second.frame = frame;
> > > +     it->second.request = request;
> > > +}
> > > +
> > > +void SimpleFrames::destroy(uint32_t frame)
> > > +{
> > > +     frameInfo_.erase(frame);
> > > +}
> > > +
> > > +void SimpleFrames::clear()
> > > +{
> > > +     frameInfo_.clear();
> > > +}
> > > +
> > > +SimpleFrameInfo *SimpleFrames::find(uint32_t frame)
> > > +{
> > > +     auto info = frameInfo_.find(frame);
> > > +     if (info == frameInfo_.end())
> > > +             return nullptr;
> > > +     return &info->second;
> > > +}
> > > +
> > > +SimpleFrameInfo *SimpleFrames::find(Request *request)
> > > +{
> > > +     for (auto &itInfo : frameInfo_) {
> > > +             SimpleFrameInfo *info = &itInfo.second;
> > > +
> > > +             if (info->request == request)
> > > +                     return info;
> > > +     }
> > > +
> > > +     return nullptr;
> > > +}
> > > +
> > >  struct SimplePipelineInfo {
> > >       const char *driver;
> > >       /*
> > > @@ -293,11 +351,13 @@ public:
> > >
> > >       std::unique_ptr<Converter> converter_;
> > >       std::unique_ptr<SoftwareIsp> swIsp_;
> > > +     SimpleFrames frameInfo_;
> > >
> > >  private:
> > >       void tryPipeline(unsigned int code, const Size &size);
> > >       static std::vector<const MediaPad *> routedSourcePads(MediaPad *sink);
> > >
> > > +     void completeRequest(Request *request);
> > >       void conversionInputDone(FrameBuffer *buffer);
> > >       void conversionOutputDone(FrameBuffer *buffer);
> > >
> > > @@ -799,7 +859,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
> > >                       /* No conversion, just complete the request. */
> > >                       Request *request = buffer->request();
> > >                       pipe->completeBuffer(request, buffer);
> > > -                     pipe->completeRequest(request);
> > > +                     completeRequest(request);
> > >                       return;
> > >               }
> > >
> > > @@ -817,7 +877,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
> > >               const RequestOutputs &outputs = conversionQueue_.front();
> > >               for (auto &[stream, buf] : outputs.outputs)
> > >                       pipe->completeBuffer(outputs.request, buf);
> > > -             pipe->completeRequest(outputs.request);
> > > +             completeRequest(outputs.request);
> > >               conversionQueue_.pop();
> > >
> > >               return;
> > > @@ -875,7 +935,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
> > >
> > >       /* Otherwise simply complete the request. */
> > >       pipe->completeBuffer(request, buffer);
> > > -     pipe->completeRequest(request);
> > > +     completeRequest(request);
> > >  }
> > >
> > >  void SimpleCameraData::clearIncompleteRequests()
> > > @@ -886,6 +946,14 @@ void SimpleCameraData::clearIncompleteRequests()
> > >       }
> > >  }
> > >
> > > +void SimpleCameraData::completeRequest(Request *request)
> > > +{
> > > +     SimpleFrameInfo *info = frameInfo_.find(request);
> > > +     if (info)
> >
> > If there's no frame info something is really wrong. I don't think
> > completing the request makes much sense in that case.
> >
> > > +             frameInfo_.destroy(info->frame);
> > > +     pipe()->completeRequest(request);
> > > +}
> > > +
> > >  void SimpleCameraData::conversionInputDone(FrameBuffer *buffer)
> > >  {
> > >       /* Queue the input buffer back for capture. */
> > > @@ -899,7 +967,7 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)
> > >       /* Complete the buffer and the request. */
> > >       Request *request = buffer->request();
> > >       if (pipe->completeBuffer(request, buffer))
> > > -             pipe->completeRequest(request);
> > > +             completeRequest(request);
> > >  }
> > >
> > >  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
> > > @@ -1412,6 +1480,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
> > >
> > >       video->bufferReady.disconnect(data, &SimpleCameraData::imageBufferReady);
> > >
> > > +     data->frameInfo_.clear();
> > >       data->clearIncompleteRequests();
> > >       data->conversionBuffers_.clear();
> > >
> > > @@ -1442,8 +1511,10 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
> > >
> > >       if (data->useConversion_) {
> > >               data->conversionQueue_.push({ request, std::move(buffers) });
> > > -             if (data->swIsp_)
> > > +             if (data->swIsp_) {
> > > +                     data->frameInfo_.create(request);
> > >                       data->swIsp_->queueRequest(request->sequence(), request->controls());
> > > +             }
> > >       }
> > >
> > >       return 0;
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>