| Message ID | 20210721092800.1735805-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thank you very much for fixing this! On Wed, 21 Jul 2021 at 10:28, Naushir Patuck <naush@raspberrypi.com> wrote: > > When RPiCameraData::clearIncompleteRequests() clears out the request queue > during a stop condition, it unconditionally calls completeBuffer() on all > buffers in each request. This is wrong, as a buffer could have already been > completed as part of the current request, but the request itself may not yet > have completed. > > Fix this by checking if the buffers in the request have been completed before > cancelling them. > > Fixes: d372aaa10ddb ("pipeline: raspberrypi: Simplify RPiCameraData::clearIncompleteRequests()") > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> I've been trying this patch out and it has indeed fixed the crash I was experiencing. Tested-by: David Plowman <david.plowman@raspberrypi.com> Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index f821d8fe1b6c..0bab3bedd402 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1520,8 +1520,14 @@ void RPiCameraData::clearIncompleteRequests() > > for (auto &b : request->buffers()) { > FrameBuffer *buffer = b.second; > - buffer->cancel(); > - pipe_->completeBuffer(request, buffer); > + /* > + * Has the buffer already been handed back to the > + * request? If not, do so now. > + */ > + if (buffer->request()) { > + buffer->cancel(); > + pipe_->completeBuffer(request, buffer); > + } > } > > pipe_->completeRequest(request); > -- > 2.25.1 >
Hi, Another ping for a review please :-) Regards, Naush On Wed, 21 Jul 2021 at 10:28, Naushir Patuck <naush@raspberrypi.com> wrote: > When RPiCameraData::clearIncompleteRequests() clears out the request queue > during a stop condition, it unconditionally calls completeBuffer() on all > buffers in each request. This is wrong, as a buffer could have already > been > completed as part of the current request, but the request itself may not > yet > have completed. > > Fix this by checking if the buffers in the request have been completed > before > cancelling them. > > Fixes: d372aaa10ddb ("pipeline: raspberrypi: Simplify > RPiCameraData::clearIncompleteRequests()") > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index f821d8fe1b6c..0bab3bedd402 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1520,8 +1520,14 @@ void RPiCameraData::clearIncompleteRequests() > > for (auto &b : request->buffers()) { > FrameBuffer *buffer = b.second; > - buffer->cancel(); > - pipe_->completeBuffer(request, buffer); > + /* > + * Has the buffer already been handed back to the > + * request? If not, do so now. > + */ > + if (buffer->request()) { > + buffer->cancel(); > + pipe_->completeBuffer(request, buffer); > + } > } > > pipe_->completeRequest(request); > -- > 2.25.1 > >
Hi Naush, Thank you for the patch. On Wed, Jul 21, 2021 at 10:28:00AM +0100, Naushir Patuck wrote: > When RPiCameraData::clearIncompleteRequests() clears out the request queue > during a stop condition, it unconditionally calls completeBuffer() on all > buffers in each request. This is wrong, as a buffer could have already been > completed as part of the current request, but the request itself may not yet > have completed. > > Fix this by checking if the buffers in the request have been completed before > cancelling them. This probably calls for improving the PipelineHandler, FrameBuffer and Request classes to simplify completion handling. For the time being, this patch looks good. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Fixes: d372aaa10ddb ("pipeline: raspberrypi: Simplify RPiCameraData::clearIncompleteRequests()") > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index f821d8fe1b6c..0bab3bedd402 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1520,8 +1520,14 @@ void RPiCameraData::clearIncompleteRequests() > > for (auto &b : request->buffers()) { > FrameBuffer *buffer = b.second; > - buffer->cancel(); > - pipe_->completeBuffer(request, buffer); > + /* > + * Has the buffer already been handed back to the > + * request? If not, do so now. > + */ > + if (buffer->request()) { > + buffer->cancel(); > + pipe_->completeBuffer(request, buffer); > + } > } > > pipe_->completeRequest(request);
Hi Laurent, Thanks for the review. On Wed, 28 Jul 2021 at 07:54, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Wed, Jul 21, 2021 at 10:28:00AM +0100, Naushir Patuck wrote: > > When RPiCameraData::clearIncompleteRequests() clears out the request > queue > > during a stop condition, it unconditionally calls completeBuffer() on all > > buffers in each request. This is wrong, as a buffer could have already > been > > completed as part of the current request, but the request itself may not > yet > > have completed. > > > > Fix this by checking if the buffers in the request have been completed > before > > cancelling them. > > This probably calls for improving the PipelineHandler, FrameBuffer and > Request classes to simplify completion handling. For the time being, > this patch looks good. > Agree. I think perhaps a Request::Cancel() that pipeline handlers could call might be a neat solution. I'd imagine the implementation would look pretty similar to the code in this patch. Regards, Naush > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Fixes: d372aaa10ddb ("pipeline: raspberrypi: Simplify > RPiCameraData::clearIncompleteRequests()") > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index f821d8fe1b6c..0bab3bedd402 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -1520,8 +1520,14 @@ void RPiCameraData::clearIncompleteRequests() > > > > for (auto &b : request->buffers()) { > > FrameBuffer *buffer = b.second; > > - buffer->cancel(); > > - pipe_->completeBuffer(request, buffer); > > + /* > > + * Has the buffer already been handed back to the > > + * request? If not, do so now. > > + */ > > + if (buffer->request()) { > > + buffer->cancel(); > > + pipe_->completeBuffer(request, buffer); > > + } > > } > > > > pipe_->completeRequest(request); > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index f821d8fe1b6c..0bab3bedd402 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1520,8 +1520,14 @@ void RPiCameraData::clearIncompleteRequests() for (auto &b : request->buffers()) { FrameBuffer *buffer = b.second; - buffer->cancel(); - pipe_->completeBuffer(request, buffer); + /* + * Has the buffer already been handed back to the + * request? If not, do so now. + */ + if (buffer->request()) { + buffer->cancel(); + pipe_->completeBuffer(request, buffer); + } } pipe_->completeRequest(request);
When RPiCameraData::clearIncompleteRequests() clears out the request queue during a stop condition, it unconditionally calls completeBuffer() on all buffers in each request. This is wrong, as a buffer could have already been completed as part of the current request, but the request itself may not yet have completed. Fix this by checking if the buffers in the request have been completed before cancelling them. Fixes: d372aaa10ddb ("pipeline: raspberrypi: Simplify RPiCameraData::clearIncompleteRequests()") Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)