<div dir="ltr"><div dir="ltr">Hi Jacopo, thank you for the patch.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 11, 2021 at 5:28 AM Niklas Söderlund <<a href="mailto:niklas.soderlund@ragnatech.se" target="_blank">niklas.soderlund@ragnatech.se</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 Jacopo,<br>
<br>
Thanks for your patch.<br>
<br>
On 2021-05-10 12:52:30 +0200, Jacopo Mondi wrote:<br>
> Capture requests are queued by the PipelineHandler base class to each<br>
> pipeline handler implementation using the virtual queueRequestDevice()<br>
> function.<br>
> <br>
> However, if the pipeline handler fails to queue the request to the<br>
> hardware, the request gets silently deleted from the list of queued<br>
> ones, without notifying application of the error.<br>
> <br>
> Allowing applications to know if a Request has failed to queue<br>
> by emitting the Camera::bufferCompleted and Camera::requestCompleted<br>
> signals allows them to maintain a state consistent with the one<br>
> internal to the library.<br>
> <br>
> To do so, if queuing a request to the device fails, cancel the request<br>
> and emit the completion signal to report the failure to applications.<br>
> <br>
> Signed-off-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> ---<br>
>  src/libcamera/pipeline_handler.cpp | 15 ++++++++++++++-<br>
>  1 file changed, 14 insertions(+), 1 deletion(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp<br>
> index f41b7a7b3308..260a52018a4d 100644<br>
> --- a/src/libcamera/pipeline_handler.cpp<br>
> +++ b/src/libcamera/pipeline_handler.cpp<br>
> @@ -391,6 +391,8 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const<br>
>   * This method queues a capture request to the pipeline handler for processing.<br>
>   * The request is first added to the internal list of queued requests, and<br>
>   * then passed to the pipeline handler with a call to queueRequestDevice().<br>
> + * If the pipeline handler fails in queuing the request to the hardware the<br>
> + * request is immediately completed with error.<br>
>   *<br>
>   * Keeping track of queued requests ensures automatic completion of all requests<br>
>   * when the pipeline handler is stopped with stop(). Request completion shall be<br>
> @@ -409,8 +411,19 @@ void PipelineHandler::queueRequest(Request *request)<br>
>       request->sequence_ = data->requestSequence_++;<br>
>  <br>
>       int ret = queueRequestDevice(camera, request);<br>
> -     if (ret)<br>
> +     if (ret) {<br>
> +             /*<br>
> +              * Cancel the request and notify completion of its buffers in<br>
> +              * error state. Then complete the cancelled request and remove<br>
> +              * it from the queued requests list.<br>
> +              */<br>
> +             request->cancel();<br>
> +             for (auto bufferMap : request->buffers())<br>
> +                     camera->bufferCompleted.emit(request, bufferMap.second);<br>
<br>
I wonder if the emitting of bufferCompleted should be moved to <br>
Camera::requestComplete()? This would be a common thing for all requests <br>
that completes in the cancel state right?<br>
<br></blockquote><div><br></div><div>I think this should be done in Request::cancel().</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>
> +             camera->requestComplete(request);<br>
>               data->queuedRequests_.remove(request);<br></blockquote><div><br></div><div>I wonder if we should call completeRequest() to keep the request return order same as the request queue order.</div><div><br></div><div>By the way, a discussion is needed about how to report the error code. cf.) <a href="https://patchwork.libcamera.org/patch/11764/" target="_blank">https://patchwork.libcamera.org/patch/11764/</a>.</div><div>But this patch can go without it.</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>
>  }<br>
>  <br>
>  /**<br>
> -- <br>
> 2.31.1<br>
> <br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
<br>
-- <br>
Regards,<br>
Niklas Söderlund<br>
_______________________________________________<br>
libcamera-devel mailing list<br>
<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
<a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>