[libcamera-devel] [PATCH 7/8] libcamera: pipeline: ipu3: frames: Associate buffers with the reqeust

[Kieran Bingham]

Hi Laurent,

On 14/03/2021 02:09, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> s/reqeust/request/ in the subject line
> 
> On Fri, Mar 12, 2021 at 06:11:30AM +0000, Kieran Bingham wrote:
>> Ensure that the buffers are associated with the request even if they are
> 
> s/the request/a request/ (or their request) ?
> 
>> used internally to be able to correctly map back to the resources they
>> are being used to fulfil.
> 
> Sounds a bit weird, but I get what you mean :-)
> 
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> I was a bit worried we could expand the usage of the request_ pointer in
> the FrameBuffer class in the future to handle more operations
> automatically, leading to issues if we associate internal buffers with
> requests, but thinking about it, your proposal makes the most sense. We
> even state that this is the expected use case in the
> FrameBuffer::setRequest() documentation :-)
> 

Yes, this patch is a bit of a wild card.

But I believe (at least while debugging) we should be able to identify
all resources, internal or not, against the request that has caused that
resource to be handled.

Niklas has suggested that sometimes there won't be a request, so that
might be a corner case to consider, but I think I've seen that you
replied that would be an invalid use case currently.

The FrameInfos class currently looks up which FrameInfo instance is
associated with a buffer by doing a lookup. I'd be tempted to have an
internal cookie for requests to allow a pipeline handler to obtain any
internal state associated with a request without performing a lookup.
And then a buffer could map to it's associated context without needing
to do a search in a vector or list.




> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> While at it, should we also apply the following ?
> 
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 0ef3bc04659b..3cd777d1b742 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -278,10 +278,9 @@ FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
> 
>  		buffer = availableBuffers_.front();
>  		availableBuffers_.pop();
> +		buffer->setRequest(request);
>  	}
> 
> -	buffer->setRequest(request);
> -

Err ... Doesn't this stop setting the Request when a raw buffer is
provided by the application?


>  	int ret = output_->queueBuffer(buffer);
>  	if (ret)
>  		return nullptr;
> 
>> ---
>>  src/libcamera/pipeline/ipu3/frames.cpp | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp
>> index 7a7c5643df43..2a0590258d03 100644
>> --- a/src/libcamera/pipeline/ipu3/frames.cpp
>> +++ b/src/libcamera/pipeline/ipu3/frames.cpp
>> @@ -56,6 +56,9 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)
>>  	FrameBuffer *paramBuffer = availableParamBuffers_.front();
>>  	FrameBuffer *statBuffer = availableStatBuffers_.front();
>>  
>> +	paramBuffer->setRequest(request);
>> +	statBuffer->setRequest(request);
>> +
>>  	availableParamBuffers_.pop();
>>  	availableStatBuffers_.pop();
>>  
> 

-- 
Regards
--
Kieran