| Message ID | 20210324150125.1318325-6-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Wed, Mar 24, 2021 at 03:01:24PM +0000, Kieran Bingham wrote: > When the imguOutputBufferReady() detects a cancelled frame, it is > reporting that the metadata has been processed in order to be able to > complete the cancelled request. > > This causes the FrameInfo to be completed and deleted early, but then an > active buffer on the IMGU can complete and be unable to find the > FrameInfo for it to complete correctly. This is because IPU3Frames::tryComplete() will erroneously consider that the frame is complete, right ? I'm a bit puzzled, that function checks if there's any pending buffer, wouldn't that condition be true until all the buffers are returned ? What am I missing ? > Do not mark metadataProcessed early on the event that a single buffer is > detected as cancelled. The stopping of the V4L2 devices will ensure > that all queued buffers are returned to us and we can follow the normal > and expected shutdown sequence. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 34ee600340b1..a8edf906220b 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -1232,9 +1232,6 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > cropRegion_ = request->controls().get(controls::ScalerCrop); > request->metadata().set(controls::ScalerCrop, cropRegion_); > > - if (buffer->metadata().status == FrameMetadata::FrameCancelled) > - info->metadataProcessed = true; > - > if (frameInfos_.tryComplete(info)) > pipe_->completeRequest(request); > }
Hi Laurent, On 24/03/2021 15:42, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Wed, Mar 24, 2021 at 03:01:24PM +0000, Kieran Bingham wrote: >> When the imguOutputBufferReady() detects a cancelled frame, it is >> reporting that the metadata has been processed in order to be able to >> complete the cancelled request. >> >> This causes the FrameInfo to be completed and deleted early, but then an >> active buffer on the IMGU can complete and be unable to find the >> FrameInfo for it to complete correctly. > > This is because IPU3Frames::tryComplete() will erroneously consider that > the frame is complete, right ? I'm a bit puzzled, that function checks > if there's any pending buffer, wouldn't that condition be true until all > the buffers are returned ? What am I missing ? It waits until all application provided buffers are returned. It doesn't track internal buffers. (assuming you mean the request->hasPendingBuffers check) This is why I previously raised the idea that internal buffers should also be tracked against a request directly, but that should be handled through a private implementation, as we can't associate them with streams that the application may interrogate. tryComplete requires 3 things. 1) !request->hasPendingBuffers() 2) info->metadataProcessed 3) info->paramDequeued info->metadataProcessed is set: A) IPU3CameraData::statBufferReady() If the stat buffer is cancelled, and not sent to the IPA. B) when ActionMetadataReady is received from the IPA I.e., after successfully parsing the stats in IPA. C) here in IPU3CameraData::imguOutputBufferReady() This is incorrect, as it's essentially completing the internal stats buffer while it can be potentially still on a V4L2 device, and just about to complete all by itself. imguOutputBufferReady completes the user facing buffer, and that's all it should do. A, and B are perfectly valid times to complete the stats buffer. C (fixed by this patch) is not. >> Do not mark metadataProcessed early on the event that a single buffer is >> detected as cancelled. The stopping of the V4L2 devices will ensure >> that all queued buffers are returned to us and we can follow the normal >> and expected shutdown sequence. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >> index 34ee600340b1..a8edf906220b 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -1232,9 +1232,6 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) >> cropRegion_ = request->controls().get(controls::ScalerCrop); >> request->metadata().set(controls::ScalerCrop, cropRegion_); >> >> - if (buffer->metadata().status == FrameMetadata::FrameCancelled) >> - info->metadataProcessed = true; >> - >> if (frameInfos_.tryComplete(info)) >> pipe_->completeRequest(request); >> } >
Hi Kieran, On Wed, Mar 24, 2021 at 04:09:35PM +0000, Kieran Bingham wrote: > On 24/03/2021 15:42, Laurent Pinchart wrote: > > On Wed, Mar 24, 2021 at 03:01:24PM +0000, Kieran Bingham wrote: > >> When the imguOutputBufferReady() detects a cancelled frame, it is > >> reporting that the metadata has been processed in order to be able to > >> complete the cancelled request. > >> > >> This causes the FrameInfo to be completed and deleted early, but then an > >> active buffer on the IMGU can complete and be unable to find the > >> FrameInfo for it to complete correctly. > > > > This is because IPU3Frames::tryComplete() will erroneously consider that > > the frame is complete, right ? I'm a bit puzzled, that function checks > > if there's any pending buffer, wouldn't that condition be true until all > > the buffers are returned ? What am I missing ? > > It waits until all application provided buffers are returned. It doesn't > track internal buffers. (assuming you mean the > request->hasPendingBuffers check) Ah of course. That's what I was missing. Thanks. > This is why I previously raised the idea that internal buffers should > also be tracked against a request directly, but that should be handled > through a private implementation, as we can't associate them with > streams that the application may interrogate. > > > tryComplete requires 3 things. > > 1) !request->hasPendingBuffers() > 2) info->metadataProcessed > 3) info->paramDequeued > > > info->metadataProcessed is set: > > A) IPU3CameraData::statBufferReady() > If the stat buffer is cancelled, and not sent to the IPA. > > B) when ActionMetadataReady is received from the IPA > I.e., after successfully parsing the stats in IPA. > > C) here in IPU3CameraData::imguOutputBufferReady() > This is incorrect, as it's essentially completing the internal stats > buffer while it can be potentially still on a V4L2 device, and just > about to complete all by itself. > imguOutputBufferReady completes the user facing buffer, and that's > all it should do. > > A, and B are perfectly valid times to complete the stats buffer. C > (fixed by this patch) is not. Thanks for putting up with me :-) Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Do not mark metadataProcessed early on the event that a single buffer is > >> detected as cancelled. The stopping of the V4L2 devices will ensure > >> that all queued buffers are returned to us and we can follow the normal > >> and expected shutdown sequence. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 3 --- > >> 1 file changed, 3 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index 34ee600340b1..a8edf906220b 100644 > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> @@ -1232,9 +1232,6 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > >> cropRegion_ = request->controls().get(controls::ScalerCrop); > >> request->metadata().set(controls::ScalerCrop, cropRegion_); > >> > >> - if (buffer->metadata().status == FrameMetadata::FrameCancelled) > >> - info->metadataProcessed = true; > >> - > >> if (frameInfos_.tryComplete(info)) > >> pipe_->completeRequest(request); > >> }
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 34ee600340b1..a8edf906220b 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -1232,9 +1232,6 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) cropRegion_ = request->controls().get(controls::ScalerCrop); request->metadata().set(controls::ScalerCrop, cropRegion_); - if (buffer->metadata().status == FrameMetadata::FrameCancelled) - info->metadataProcessed = true; - if (frameInfos_.tryComplete(info)) pipe_->completeRequest(request); }
When the imguOutputBufferReady() detects a cancelled frame, it is reporting that the metadata has been processed in order to be able to complete the cancelled request. This causes the FrameInfo to be completed and deleted early, but then an active buffer on the IMGU can complete and be unable to find the FrameInfo for it to complete correctly. Do not mark metadataProcessed early on the event that a single buffer is detected as cancelled. The stopping of the V4L2 devices will ensure that all queued buffers are returned to us and we can follow the normal and expected shutdown sequence. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 3 --- 1 file changed, 3 deletions(-)