<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thanks for the review.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 28 Jul 2021 at 07:54, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Wed, Jul 21, 2021 at 10:28:00AM +0100, Naushir Patuck wrote:<br>
> When RPiCameraData::clearIncompleteRequests() clears out the request queue<br>
> during a stop condition, it unconditionally calls completeBuffer() on all<br>
> buffers in each request.  This is wrong, as a buffer could have already been<br>
> completed as part of the current request, but the request itself may not yet<br>
> have completed.<br>
> <br>
> Fix this by checking if the buffers in the request have been completed before<br>
> cancelling them.<br>
<br>
This probably calls for improving the PipelineHandler, FrameBuffer and<br>
Request classes to simplify completion handling. For the time being,<br>
this patch looks good.<br></blockquote><div><br></div><div>Agree.  I think perhaps a Request::Cancel() that pipeline handlers could call</div><div>might be a neat solution.  I'd imagine the implementation would look pretty</div><div>similar to the code in this patch.</div><div><br></div><div>Regards,</div><div>Naush </div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> Fixes: d372aaa10ddb ("pipeline: raspberrypi: Simplify RPiCameraData::clearIncompleteRequests()")<br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++++--<br>
>  1 file changed, 8 insertions(+), 2 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index f821d8fe1b6c..0bab3bedd402 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1520,8 +1520,14 @@ void RPiCameraData::clearIncompleteRequests()<br>
>  <br>
>               for (auto &b : request->buffers()) {<br>
>                       FrameBuffer *buffer = b.second;<br>
> -                     buffer->cancel();<br>
> -                     pipe_->completeBuffer(request, buffer);<br>
> +                     /*<br>
> +                      * Has the buffer already been handed back to the<br>
> +                      * request? If not, do so now.<br>
> +                      */<br>
> +                     if (buffer->request()) {<br>
> +                             buffer->cancel();<br>
> +                             pipe_->completeBuffer(request, buffer);<br>
> +                     }<br>
>               }<br>
>  <br>
>               pipe_->completeRequest(request);<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>