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

[Laurent Pinchart]

Hi Kieran,

On Mon, Mar 15, 2021 at 11:38:24AM +0000, Kieran Bingham wrote:
> 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.

It's a good idea. If you implement this, can you make Request Extensible
first, and store this in the Private class ?

> > 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?

Isn't it set by Request::addBuffer() ?

> >  	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,

Laurent Pinchart