| Message ID | 20210312061131.854849-8-kieran.bingham@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
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@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 :-) Reviewed-by: Laurent Pinchart <laurent.pinchart@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); - 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(); >
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@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@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(); >> >
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@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@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(); > >>
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();
Ensure that the buffers are associated with the request even if they are used internally to be able to correctly map back to the resources they are being used to fulfil. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/pipeline/ipu3/frames.cpp | 3 +++ 1 file changed, 3 insertions(+)