| Message ID | 20210513092246.42847-4-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Thu, May 13, 2021 at 11:22:41AM +0200, Jacopo Mondi wrote: > Capture requests are queued by the PipelineHandler base class to each > pipeline handler implementation using the virtual queueRequestDevice() > function. > > However, if the pipeline handler fails to queue the request to the > hardware, the request gets silently deleted from the list of queued > ones, without notifying application of the error. > > Allowing applications to know if a Request has failed to queue > by emitting the Camera::bufferCompleted and Camera::requestCompleted > signals allows them to maintain a state consistent with the one > internal to the library. > > To do so, if queuing a request to the device fails, cancel the request > and emit the completion signal to report the failure to applications. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline_handler.cpp | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index f41b7a7b3308..260a52018a4d 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -391,6 +391,8 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const > * This method queues a capture request to the pipeline handler for processing. > * The request is first added to the internal list of queued requests, and > * then passed to the pipeline handler with a call to queueRequestDevice(). > + * If the pipeline handler fails in queuing the request to the hardware the > + * request is immediately completed with error. > * > * Keeping track of queued requests ensures automatic completion of all requests > * when the pipeline handler is stopped with stop(). Request completion shall be > @@ -409,8 +411,19 @@ void PipelineHandler::queueRequest(Request *request) > request->sequence_ = data->requestSequence_++; > > int ret = queueRequestDevice(camera, request); > - if (ret) > + if (ret) { > + /* > + * Cancel the request and notify completion of its buffers in > + * error state. Then complete the cancelled request and remove > + * it from the queued requests list. > + */ > + request->cancel(); > + for (auto bufferMap : request->buffers()) > + camera->bufferCompleted.emit(request, bufferMap.second); > + > + camera->requestComplete(request); This breaks the request completion ordering. I think you could just call PipelineHandler::completeRequest() instead. > data->queuedRequests_.remove(request); > + } > } > > /**
Hi Jacopo and Laurent, On Mon, May 17, 2021 at 11:45 AM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, May 13, 2021 at 11:22:41AM +0200, Jacopo Mondi wrote: > > Capture requests are queued by the PipelineHandler base class to each > > pipeline handler implementation using the virtual queueRequestDevice() > > function. > > > > However, if the pipeline handler fails to queue the request to the > > hardware, the request gets silently deleted from the list of queued > > ones, without notifying application of the error. > > > > Allowing applications to know if a Request has failed to queue > > by emitting the Camera::bufferCompleted and Camera::requestCompleted > > signals allows them to maintain a state consistent with the one > > internal to the library. > > > > To do so, if queuing a request to the device fails, cancel the request > > and emit the completion signal to report the failure to applications. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline_handler.cpp | 15 ++++++++++++++- > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline_handler.cpp > b/src/libcamera/pipeline_handler.cpp > > index f41b7a7b3308..260a52018a4d 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -391,6 +391,8 @@ bool PipelineHandler::hasPendingRequests(const > Camera *camera) const > > * This method queues a capture request to the pipeline handler for > processing. > > * The request is first added to the internal list of queued requests, > and > > * then passed to the pipeline handler with a call to > queueRequestDevice(). > > + * If the pipeline handler fails in queuing the request to the hardware > the > > + * request is immediately completed with error. > > * > > * Keeping track of queued requests ensures automatic completion of all > requests > > * when the pipeline handler is stopped with stop(). Request completion > shall be > > @@ -409,8 +411,19 @@ void PipelineHandler::queueRequest(Request *request) > > request->sequence_ = data->requestSequence_++; > > > > int ret = queueRequestDevice(camera, request); > > - if (ret) > > + if (ret) { > > + /* > > + * Cancel the request and notify completion of its buffers > in > > + * error state. Then complete the cancelled request and > remove > > + * it from the queued requests list. > > + */ > > + request->cancel(); > > + for (auto bufferMap : request->buffers()) > > + camera->bufferCompleted.emit(request, > bufferMap.second); > > + > > + camera->requestComplete(request); > > This breaks the request completion ordering. I think you could just call > PipelineHandler::completeRequest() instead. > I had the same comment in the previous version. https://patchwork.libcamera.org/patch/12243/ We were waiting for your reply. Alright we got the reply now. > > data->queuedRequests_.remove(request); > > + } > > } > > > > /** > > -- > Regards, > > Laurent Pinchart >
Hello, On Mon, May 17, 2021 at 01:48:59PM +0900, Hirokazu Honda wrote: > Hi Jacopo and Laurent, > > On Mon, May 17, 2021 at 11:45 AM Laurent Pinchart < > laurent.pinchart@ideasonboard.com> wrote: > > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Thu, May 13, 2021 at 11:22:41AM +0200, Jacopo Mondi wrote: > > > Capture requests are queued by the PipelineHandler base class to each > > > pipeline handler implementation using the virtual queueRequestDevice() > > > function. > > > > > > However, if the pipeline handler fails to queue the request to the > > > hardware, the request gets silently deleted from the list of queued > > > ones, without notifying application of the error. > > > > > > Allowing applications to know if a Request has failed to queue > > > by emitting the Camera::bufferCompleted and Camera::requestCompleted > > > signals allows them to maintain a state consistent with the one > > > internal to the library. > > > > > > To do so, if queuing a request to the device fails, cancel the request > > > and emit the completion signal to report the failure to applications. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/pipeline_handler.cpp | 15 ++++++++++++++- > > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/pipeline_handler.cpp > > b/src/libcamera/pipeline_handler.cpp > > > index f41b7a7b3308..260a52018a4d 100644 > > > --- a/src/libcamera/pipeline_handler.cpp > > > +++ b/src/libcamera/pipeline_handler.cpp > > > @@ -391,6 +391,8 @@ bool PipelineHandler::hasPendingRequests(const > > Camera *camera) const > > > * This method queues a capture request to the pipeline handler for > > processing. > > > * The request is first added to the internal list of queued requests, > > and > > > * then passed to the pipeline handler with a call to > > queueRequestDevice(). > > > + * If the pipeline handler fails in queuing the request to the hardware > > the > > > + * request is immediately completed with error. > > > * > > > * Keeping track of queued requests ensures automatic completion of all > > requests > > > * when the pipeline handler is stopped with stop(). Request completion > > shall be > > > @@ -409,8 +411,19 @@ void PipelineHandler::queueRequest(Request *request) > > > request->sequence_ = data->requestSequence_++; > > > > > > int ret = queueRequestDevice(camera, request); > > > - if (ret) > > > + if (ret) { > > > + /* > > > + * Cancel the request and notify completion of its buffers > > in > > > + * error state. Then complete the cancelled request and > > remove > > > + * it from the queued requests list. > > > + */ > > > + request->cancel(); > > > + for (auto bufferMap : request->buffers()) > > > + camera->bufferCompleted.emit(request, > > bufferMap.second); > > > + > > > + camera->requestComplete(request); > > > > This breaks the request completion ordering. I think you could just call > > PipelineHandler::completeRequest() instead. > > > > I had the same comment in the previous version. > https://patchwork.libcamera.org/patch/12243/ > We were waiting for your reply. Alright we got the reply now. > Yes, I've cc-ed Laurent in that patch to know his opinion. Laurent, could you please read my reply and let me know if it add anything to the discussion ? Thanks j > > > > > data->queuedRequests_.remove(request); > > > + } > > > } > > > > > > /** > > > > -- > > Regards, > > > > Laurent Pinchart > >
Hi Jacopo, On Mon, May 17, 2021 at 09:17:32AM +0200, Jacopo Mondi wrote: > On Mon, May 17, 2021 at 01:48:59PM +0900, Hirokazu Honda wrote: > > On Mon, May 17, 2021 at 11:45 AM Laurent Pinchart wrote: > > > On Thu, May 13, 2021 at 11:22:41AM +0200, Jacopo Mondi wrote: > > > > Capture requests are queued by the PipelineHandler base class to each > > > > pipeline handler implementation using the virtual queueRequestDevice() > > > > function. > > > > > > > > However, if the pipeline handler fails to queue the request to the > > > > hardware, the request gets silently deleted from the list of queued > > > > ones, without notifying application of the error. > > > > > > > > Allowing applications to know if a Request has failed to queue > > > > by emitting the Camera::bufferCompleted and Camera::requestCompleted > > > > signals allows them to maintain a state consistent with the one > > > > internal to the library. > > > > > > > > To do so, if queuing a request to the device fails, cancel the request > > > > and emit the completion signal to report the failure to applications. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > src/libcamera/pipeline_handler.cpp | 15 ++++++++++++++- > > > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > > index f41b7a7b3308..260a52018a4d 100644 > > > > --- a/src/libcamera/pipeline_handler.cpp > > > > +++ b/src/libcamera/pipeline_handler.cpp > > > > @@ -391,6 +391,8 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const > > > > * This method queues a capture request to the pipeline handler for processing. > > > > * The request is first added to the internal list of queued requests, and > > > > * then passed to the pipeline handler with a call to queueRequestDevice(). > > > > + * If the pipeline handler fails in queuing the request to the hardware the > > > > + * request is immediately completed with error. > > > > * > > > > * Keeping track of queued requests ensures automatic completion of all requests > > > > * when the pipeline handler is stopped with stop(). Request completion shall be > > > > @@ -409,8 +411,19 @@ void PipelineHandler::queueRequest(Request *request) > > > > request->sequence_ = data->requestSequence_++; > > > > > > > > int ret = queueRequestDevice(camera, request); > > > > - if (ret) > > > > + if (ret) { > > > > + /* > > > > + * Cancel the request and notify completion of its buffers in > > > > + * error state. Then complete the cancelled request and remove > > > > + * it from the queued requests list. > > > > + */ > > > > + request->cancel(); > > > > + for (auto bufferMap : request->buffers()) > > > > + camera->bufferCompleted.emit(request, bufferMap.second); > > > > + > > > > + camera->requestComplete(request); > > > > > > This breaks the request completion ordering. I think you could just call > > > PipelineHandler::completeRequest() instead. > > > > > > > I had the same comment in the previous version. > > https://patchwork.libcamera.org/patch/12243/ > > We were waiting for your reply. Alright we got the reply now. > > > > Yes, I've cc-ed Laurent in that patch to know his opinion. > > Laurent, could you please read my reply and let me know if it add > anything to the discussion ? Done. Sorry for missing it in the first place. > > > > data->queuedRequests_.remove(request); > > > > + } > > > > } > > > > > > > > /**
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index f41b7a7b3308..260a52018a4d 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -391,6 +391,8 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const * This method queues a capture request to the pipeline handler for processing. * The request is first added to the internal list of queued requests, and * then passed to the pipeline handler with a call to queueRequestDevice(). + * If the pipeline handler fails in queuing the request to the hardware the + * request is immediately completed with error. * * Keeping track of queued requests ensures automatic completion of all requests * when the pipeline handler is stopped with stop(). Request completion shall be @@ -409,8 +411,19 @@ void PipelineHandler::queueRequest(Request *request) request->sequence_ = data->requestSequence_++; int ret = queueRequestDevice(camera, request); - if (ret) + if (ret) { + /* + * Cancel the request and notify completion of its buffers in + * error state. Then complete the cancelled request and remove + * it from the queued requests list. + */ + request->cancel(); + for (auto bufferMap : request->buffers()) + camera->bufferCompleted.emit(request, bufferMap.second); + + camera->requestComplete(request); data->queuedRequests_.remove(request); + } } /**
Capture requests are queued by the PipelineHandler base class to each pipeline handler implementation using the virtual queueRequestDevice() function. However, if the pipeline handler fails to queue the request to the hardware, the request gets silently deleted from the list of queued ones, without notifying application of the error. Allowing applications to know if a Request has failed to queue by emitting the Camera::bufferCompleted and Camera::requestCompleted signals allows them to maintain a state consistent with the one internal to the library. To do so, if queuing a request to the device fails, cancel the request and emit the completion signal to report the failure to applications. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline_handler.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)