[PATCH 3/5] libcamera: rkisp1: Switch IPA slots to use bufferId not frame

[Kieran Bingham]

Quoting Jacopo Mondi (2024-02-13 12:46:36)
> Hi Dan
> 
> On Mon, Feb 12, 2024 at 03:35:30PM +0000, Daniel Scally wrote:
> > In preparation for removing the RkISP1FrameInfo class we need the
> > RkISP1 IPA to emit bufferIds rather than frame numbers for its slots
> > so that the pipeline handler can find the correct buffer properly.
> >
> > Switch the slots; reconfigure the pipeline handler to obtain the
> > correct buffer first before passing it to RkISP1FrameInfo::find()
> > as before; one of its overloads already works with FrameBuffer *.
> >
> > Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> > ---
> >  include/libcamera/ipa/rkisp1.mojom       |  4 +-
> >  src/ipa/rkisp1/rkisp1.cpp                |  4 +-
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 59 ++++++++++++++++--------
> >  3 files changed, 44 insertions(+), 23 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > index 1009e970..4c097622 100644
> > --- a/include/libcamera/ipa/rkisp1.mojom
> > +++ b/include/libcamera/ipa/rkisp1.mojom
> > @@ -36,7 +36,7 @@ interface IPARkISP1Interface {
> >  };
> >
> >  interface IPARkISP1EventInterface {
> > -     paramsBufferReady(uint32 frame);
> > +     paramsBufferReady(uint32 bufferId);
> >       setSensorControls(uint32 frame, libcamera.ControlList sensorControls);
> > -     metadataReady(uint32 frame, libcamera.ControlList metadata);
> > +     metadataReady(uint32 bufferId, libcamera.ControlList metadata);
> >  };
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 6544c925..7e01a04d 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -337,7 +337,7 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
> 
> I guess this should be ok. The parameters and stats buffers are
> initialized with a cookie
> 
>         for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_) {
>                 buffer->setCookie(ipaBufferId++);
> 
> which is used to identify them on the IPA buffer map
> 
>         rkisp1_params_cfg *params =
>                 reinterpret_cast<rkisp1_params_cfg *>(
>                         mappedBuffers_.at(bufferId).planes()[0].data());
> 
> There shouldn't be any risk of the ids colliding as we're constantly
> cycling on the same set of buffers and the ownership of the frame
> buffer doesn't change compared to the previous version if I'm not
> mistaken
> 
> >       for (auto const &algo : algorithms())
> >               algo->prepare(context_, frame, frameContext, params);
> >
> > -     paramsBufferReady.emit(frame);
> > +     paramsBufferReady.emit(bufferId);
> >  }
> >
> >  void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId,
> > @@ -370,7 +370,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> >
> >       setControls(frame);
> >
> > -     metadataReady.emit(frame, metadata);
> > +     metadataReady.emit(bufferId, metadata);
> >  }
> >
> >  void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 22e553fe..d4ed38a4 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -376,23 +376,34 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> >       return 0;
> >  }
> >
> > -void RkISP1CameraData::paramFilled(unsigned int frame)
> > +void RkISP1CameraData::paramFilled(unsigned int bufferId)
> >  {
> >       PipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();
> > -     RkISP1FrameInfo *info = frameInfo_.find(frame);
> > -     if (!info)
> > -             return;
> >
> > -     info->paramBuffer->_d()->metadata().planes()[0].bytesused =
> > -             sizeof(struct rkisp1_params_cfg);
> > -     pipe->param_->queueBuffer(info->paramBuffer);
> > -     pipe->stat_->queueBuffer(info->statBuffer);
> > +     for (std::unique_ptr<FrameBuffer> &paramBuffer : pipe->paramBuffers_) {
> > +             if (paramBuffer->cookie() != bufferId)
> > +                     continue;
> > +
> > +             RkISP1FrameInfo *info = frameInfo_.find(paramBuffer.get());
> > +             if (!info)
> > +                     return;
> > +
> > +             info->paramBuffer->_d()->metadata().planes()[0].bytesused =
> > +                     sizeof(struct rkisp1_params_cfg);
> > +             pipe->param_->queueBuffer(info->paramBuffer);
> > +             pipe->stat_->queueBuffer(info->statBuffer);
> >
> > -     if (info->mainPathBuffer)
> > -             mainPath_->queueBuffer(info->mainPathBuffer);
> > +             if (info->mainPathBuffer)
> > +                     mainPath_->queueBuffer(info->mainPathBuffer);
> > +
> > +             if (selfPath_ && info->selfPathBuffer)
> > +                     selfPath_->queueBuffer(info->selfPathBuffer);
> > +
> > +             return;
> > +     }
> >
> > -     if (selfPath_ && info->selfPathBuffer)
> > -             selfPath_->queueBuffer(info->selfPathBuffer);
> > +     LOG(RkISP1, Fatal) << "Can't locate buffer from bufferId";

These Fatal's in the param/stats handling worry me a little though. Can
we be absolutely sure that there should be no reason we can get here?

I.e. Can this happen if say we drop some frames at runtime?



> > +     return;
> 
> No need for return
> 
> >  }
> >
> >  void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
> > @@ -401,16 +412,26 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
> >       delayedCtrls_->push(sensorControls);
> >  }
> >
> > -void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)
> > +void RkISP1CameraData::metadataReady(unsigned int bufferId,
> > +                                  const ControlList &metadata)
> >  {
> > -     RkISP1FrameInfo *info = frameInfo_.find(frame);
> > -     if (!info)
> > -             return;
> > +     for (std::unique_ptr<FrameBuffer> &statBuffer : pipe()->statBuffers_) {
> > +             if (statBuffer->cookie() != bufferId)
> > +                     continue;
> > +
> > +             RkISP1FrameInfo *info = frameInfo_.find(statBuffer.get());
> > +             if (!info)
> > +                     return;
> >
> > -     info->request->metadata().merge(metadata);
> > -     info->metadataProcessed = true;
> > +             info->request->metadata().merge(metadata);
> > +             info->metadataProcessed = true;
> > +
> > +             pipe()->tryCompleteRequest(info);
> > +             return;
> > +     }
> >
> > -     pipe()->tryCompleteRequest(info);
> > +     LOG(RkISP1, Fatal) << "Can't locate buffer from bufferId";
> > +     return;
> 
> same here
> 
> >  }
> >
> >  /* -----------------------------------------------------------------------------
> > --
> > 2.34.1
> >