[1/5] libcamera: rkisp1: Make tryCompleteRequest() params agnostic
diff mbox series

Message ID 20240212153532.179283-2-dan.scally@ideasonboard.com
State Rejected, archived
Headers show
Series
  • Remove RkISP1FrameInfo class
Related show

Commit Message

Dan Scally Feb. 12, 2024, 3:35 p.m. UTC 
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.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 20 --------------------
 1 file changed, 20 deletions(-)

Comments

Jacopo Mondi Feb. 13, 2024, 11:10 a.m. UTC | #1 
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 ?

>
> Signed-off-by: Daniel Scally <dan.scally@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);
> -}
> -
>  void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
>  {
>  	ASSERT(activeCamera_);
> --
> 2.34.1
>
Kieran Bingham Feb. 13, 2024, 1:26 p.m. UTC | #2 
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@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
> >
Dan Scally Feb. 15, 2024, 12:51 p.m. UTC | #3 
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@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
>>>

Patch
diff mbox series 

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);
-}
-
 void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
 {
 	ASSERT(activeCamera_);