[PATCH 1/5] libcamera: rkisp1: Make tryCompleteRequest() params agnostic

[Kieran Bingham]

Quoting Jacopo Mondi (2024-02-13 11:10:37)
> Hi Dan
> 
> On Mon, Feb 12, 2024 at 03:35:28PM +0000, Daniel Scally wrote:
> > tryCompleteRequest() doesn't actually need to know the status of the
> > parameters buffer to decide whether it's ok to complete the Request
> > or not - remove the check. Since setting the paramsDequeued flag is
> > all that the paramsReady slot actually does, that can be removed too.
> 
> When PipelineHandlerRkISP1::tryCompleteRequest() is called
> succesfully, it destroies the RkISP1Frames associated with the
> completed request.
> 
> int RkISP1Frames::destroy(unsigned int frame)
> {
>         RkISP1FrameInfo *info = find(frame);
>         if (!info)
>                 return -ENOENT;
> 
>         pipe_->availableParamBuffers_.push(info->paramBuffer);
>         pipe_->availableStatBuffers_.push(info->statBuffer);
> 
>         frameInfo_.erase(info->frame);
> 
>         delete info;
> 
>         return 0;
> }
> 
> 
> This
>         pipe_->availableParamBuffers_.push(info->paramBuffer);
> 
> returns the parameter buffer to the list of available ones, but to do
> so, shouldn't we make sure it has been de-queued from the video output
> device first ?

That's probably something that should happen in the param buffer ready
slot...

> 
> >
> > Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 20 --------------------
> >  1 file changed, 20 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 586b46d6..5460dc11 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -60,7 +60,6 @@ struct RkISP1FrameInfo {
> >       FrameBuffer *mainPathBuffer;
> >       FrameBuffer *selfPathBuffer;
> >
> > -     bool paramDequeued;
> >       bool metadataProcessed;
> >  };
> >
> > @@ -177,7 +176,6 @@ private:
> >       int createCamera(MediaEntity *sensor);
> >       void tryCompleteRequest(RkISP1FrameInfo *info);
> >       void bufferReady(FrameBuffer *buffer);
> > -     void paramReady(FrameBuffer *buffer);
> >       void statReady(FrameBuffer *buffer);
> >       void frameStart(uint32_t sequence);
> >
> > @@ -248,7 +246,6 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
> >       info->mainPathBuffer = mainPathBuffer;
> >       info->selfPathBuffer = selfPathBuffer;
> >       info->statBuffer = statBuffer;
> > -     info->paramDequeued = false;
> >       info->metadataProcessed = false;
> >
> >       frameInfo_[frame] = info;
> > @@ -1213,7 +1210,6 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> >       if (hasSelfPath_)
> >               selfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
> >       stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
> > -     param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);
> >
> >       /*
> >        * Enumerate all sensors connected to the ISP and create one
> > @@ -1243,9 +1239,6 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)
> >       if (!info->metadataProcessed)
> >               return;
> >
> > -     if (!isRaw_ && !info->paramDequeued)
> > -             return;
> > -
> >       data->frameInfo_.destroy(info->frame);
> >
> >       completeRequest(request);
> > @@ -1287,19 +1280,6 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
> >       tryCompleteRequest(info);
> >  }
> >
> > -void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)
> > -{
> > -     ASSERT(activeCamera_);
> > -     RkISP1CameraData *data = cameraData(activeCamera_);
> > -
> > -     RkISP1FrameInfo *info = data->frameInfo_.find(buffer);
> > -     if (!info)
> > -             return;
> > -
> > -     info->paramDequeued = true;
> > -     tryCompleteRequest(info);

I agree that this doesn't need to be so closely associated with the
request, but we should track that now the paramBuffer is 'free' and
ready to be reused, so I'd at least expect that in here we add it to a
free-list, which is then consumed when asking the IPA to populate the
next param buffer.


> > -}
> > -
> >  void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
> >  {
> >       ASSERT(activeCamera_);
> > --
> > 2.34.1
> >