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

[Dan Scally]

Hi Kieran, Jacopo

On 13/02/2024 13:26, Kieran Bingham wrote:
> 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.


Yeah this was really stupid...it of course needs to be returned to the availableParamBuffers_ list 
here and that's how it was working until about 20 minutes before posting the series, but when I was 
rebased to tidy up this patch I thought "Why do we even need this" and deleted it - so that was dumb 
and I obviously didn't test it properly after that rebase.

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