| Message ID | 20211130233634.34173-11-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Wed, Dec 01, 2021 at 12:36:33AM +0100, Jacopo Mondi wrote: > Before queueing a request to the device, any synchronization fence from > the Request' framebuffers has to be waited on. > > Connect the Request::Private::prepared signal to the function that > queues requests to the hardware and call Request::Private::prepare(). > > When the waiting request queue is inspected, verify if has completed its s/if has/if it has/ > preparation phase and queue it to the device. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline_handler.cpp | 48 ++++++++++++++++++++++-------- > 1 file changed, 36 insertions(+), 12 deletions(-) > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 8f1f20e896b0..0a6a2e6ee983 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -7,6 +7,7 @@ > > #include "libcamera/internal/pipeline_handler.h" > > +#include <chrono> > #include <sys/sysmacros.h> > > #include <libcamera/base/log.h> > @@ -17,6 +18,7 @@ > #include <libcamera/framebuffer.h> > > #include "libcamera/internal/camera.h" > +#include "libcamera/internal/framebuffer.h" Still no warning about the alphabetical order ? > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/request.h" > @@ -36,6 +38,8 @@ > * the REGISTER_PIPELINE_HANDLER() macro. > */ > > +using namespace std::chrono_literals; > + > namespace libcamera { > > LOG_DEFINE_CATEGORY(Pipeline) > @@ -323,10 +327,16 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const > * \param[in] request The request to queue > * > * This function 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 cancelled. > + * processing. The request is first added to the internal list of waiting > + * requests which have to be prepared to make sure they are ready for being > + * queued to the pipeline handler. > + * > + * The queue of waiting requests is iterated and all prepared requests are > + * passed to the pipeline handler in the same order they have been queued by > + * calling this function. > + * > + * If a Request fails during the preparation phase or if the pipeline handler > + * fails in queuing the request to the hardware the request is cancelled. > * > * Keeping track of queued requests ensures automatic completion of all requests > * when the pipeline handler is stopped with stop(). Request completion shall be > @@ -339,11 +349,20 @@ void PipelineHandler::queueRequest(Request *request) > LIBCAMERA_TRACEPOINT(request_queue, request); > > waitingRequests_.push(request); > - doQueueRequests(); > + > + request->_d()->prepared.connect(this, [this]() { > + doQueueRequests(); > + }); > + request->_d()->prepare(300ms); > } > > /** > * \brief Queue one requests to the device > + * > + * If a Request has failed during the preparation phase it is cancelled. > + * > + * If queuing a Request to the pipeline handler fails, the Request is cancelled > + * as well. > */ > void PipelineHandler::doQueueRequest(Request *request) > { > @@ -355,6 +374,12 @@ void PipelineHandler::doQueueRequest(Request *request) > > request->_d()->sequence_ = data->requestSequence_++; > > + if (request->_d()->cancelled_) { > + request->_d()->cancel(); Not that it's incorrect, but this looks really weird and calls for some refactoring on top of this series. Could you add a \todo comment ? > + completeRequest(request); > + return; > + } > + > int ret = queueRequestDevice(camera, request); > if (ret) { > request->_d()->cancel(); > @@ -365,19 +390,18 @@ void PipelineHandler::doQueueRequest(Request *request) > /** > * \brief Queue requests to the device Maybe * \brief Queue prepared requests to the device ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > * > - * Iterate the lst of waiting requests and queue them to the hardware one > - * by one. > + * Iterate the queue of waiting requests and queue them in order the hardware if > + * they have completed their preparation phase. > */ > void PipelineHandler::doQueueRequests() > { > - while (true) { > - if (waitingRequests_.empty()) > - return; > - > + while (!waitingRequests_.empty()) { > Request *request = waitingRequests_.front(); > - waitingRequests_.pop(); > + if (!request->_d()->prepared_) > + break; > > doQueueRequest(request); > + waitingRequests_.pop(); > } > } >
Hi Laurent, On Wed, Dec 01, 2021 at 04:20:35AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Dec 01, 2021 at 12:36:33AM +0100, Jacopo Mondi wrote: > > Before queueing a request to the device, any synchronization fence from > > the Request' framebuffers has to be waited on. > > > > Connect the Request::Private::prepared signal to the function that > > queues requests to the hardware and call Request::Private::prepare(). > > > > When the waiting request queue is inspected, verify if has completed its > > s/if has/if it has/ > > > preparation phase and queue it to the device. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline_handler.cpp | 48 ++++++++++++++++++++++-------- > > 1 file changed, 36 insertions(+), 12 deletions(-) > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index 8f1f20e896b0..0a6a2e6ee983 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -7,6 +7,7 @@ > > > > #include "libcamera/internal/pipeline_handler.h" > > > > +#include <chrono> > > #include <sys/sysmacros.h> > > > > #include <libcamera/base/log.h> > > @@ -17,6 +18,7 @@ > > #include <libcamera/framebuffer.h> > > > > #include "libcamera/internal/camera.h" > > +#include "libcamera/internal/framebuffer.h" > > Still no warning about the alphabetical order ? > ouch no. It's also weird that I get this on a previous patch -------------------------------------------------------------------------------------------- 3d2e23d0730b257931bc101aafac08c15f0789e6 libcamera: pipeline_handler: Split request queueing -------------------------------------------------------------------------------------------- --- include/libcamera/internal/pipeline_handler.h +++ include/libcamera/internal/pipeline_handler.h @@ -8,7 +8,6 @@ #pragma once #include <memory> -#include <queue> #include <set> #include <string> #include <sys/types.h> --- 1 potential issue detected, please review > > #include "libcamera/internal/device_enumerator.h" > > #include "libcamera/internal/media_device.h" > > #include "libcamera/internal/request.h" > > @@ -36,6 +38,8 @@ > > * the REGISTER_PIPELINE_HANDLER() macro. > > */ > > > > +using namespace std::chrono_literals; > > + > > namespace libcamera { > > > > LOG_DEFINE_CATEGORY(Pipeline) > > @@ -323,10 +327,16 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const > > * \param[in] request The request to queue > > * > > * This function 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 cancelled. > > + * processing. The request is first added to the internal list of waiting > > + * requests which have to be prepared to make sure they are ready for being > > + * queued to the pipeline handler. > > + * > > + * The queue of waiting requests is iterated and all prepared requests are > > + * passed to the pipeline handler in the same order they have been queued by > > + * calling this function. > > + * > > + * If a Request fails during the preparation phase or if the pipeline handler > > + * fails in queuing the request to the hardware the request is cancelled. > > * > > * Keeping track of queued requests ensures automatic completion of all requests > > * when the pipeline handler is stopped with stop(). Request completion shall be > > @@ -339,11 +349,20 @@ void PipelineHandler::queueRequest(Request *request) > > LIBCAMERA_TRACEPOINT(request_queue, request); > > > > waitingRequests_.push(request); > > - doQueueRequests(); > > + > > + request->_d()->prepared.connect(this, [this]() { > > + doQueueRequests(); > > + }); > > + request->_d()->prepare(300ms); > > } > > > > /** > > * \brief Queue one requests to the device > > + * > > + * If a Request has failed during the preparation phase it is cancelled. > > + * > > + * If queuing a Request to the pipeline handler fails, the Request is cancelled > > + * as well. > > */ > > void PipelineHandler::doQueueRequest(Request *request) > > { > > @@ -355,6 +374,12 @@ void PipelineHandler::doQueueRequest(Request *request) > > > > request->_d()->sequence_ = data->requestSequence_++; > > > > + if (request->_d()->cancelled_) { > > + request->_d()->cancel(); > > Not that it's incorrect, but this looks really weird and calls for some > refactoring on top of this series. Could you add a \todo comment ? Yes, it feels weird. But either I add new flags specific to the prepare phase result or I do this. I thought I can't call Request::Private::cancel() directly from the timeout handler as it would complete the request out of order, but it actually only notifies buffers completion not the request. I can try looking into that. > > > + completeRequest(request); > > + return; > > + } > > + > > int ret = queueRequestDevice(camera, request); > > if (ret) { > > request->_d()->cancel(); > > @@ -365,19 +390,18 @@ void PipelineHandler::doQueueRequest(Request *request) > > /** > > * \brief Queue requests to the device > > Maybe > > * \brief Queue prepared requests to the device > > ? > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > * > > - * Iterate the lst of waiting requests and queue them to the hardware one > > - * by one. > > + * Iterate the queue of waiting requests and queue them in order the hardware if > > + * they have completed their preparation phase. > > */ > > void PipelineHandler::doQueueRequests() > > { > > - while (true) { > > - if (waitingRequests_.empty()) > > - return; > > - > > + while (!waitingRequests_.empty()) { > > Request *request = waitingRequests_.front(); > > - waitingRequests_.pop(); > > + if (!request->_d()->prepared_) > > + break; > > > > doQueueRequest(request); > > + waitingRequests_.pop(); > > } > > } > > > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Wed, Dec 01, 2021 at 11:35:24AM +0100, Jacopo Mondi wrote: > On Wed, Dec 01, 2021 at 04:20:35AM +0200, Laurent Pinchart wrote: > > On Wed, Dec 01, 2021 at 12:36:33AM +0100, Jacopo Mondi wrote: > > > Before queueing a request to the device, any synchronization fence from > > > the Request' framebuffers has to be waited on. > > > > > > Connect the Request::Private::prepared signal to the function that > > > queues requests to the hardware and call Request::Private::prepare(). > > > > > > When the waiting request queue is inspected, verify if has completed its > > > > s/if has/if it has/ > > > > > preparation phase and queue it to the device. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/pipeline_handler.cpp | 48 ++++++++++++++++++++++-------- > > > 1 file changed, 36 insertions(+), 12 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > index 8f1f20e896b0..0a6a2e6ee983 100644 > > > --- a/src/libcamera/pipeline_handler.cpp > > > +++ b/src/libcamera/pipeline_handler.cpp > > > @@ -7,6 +7,7 @@ > > > > > > #include "libcamera/internal/pipeline_handler.h" > > > > > > +#include <chrono> > > > #include <sys/sysmacros.h> > > > > > > #include <libcamera/base/log.h> > > > @@ -17,6 +18,7 @@ > > > #include <libcamera/framebuffer.h> > > > > > > #include "libcamera/internal/camera.h" > > > +#include "libcamera/internal/framebuffer.h" > > > > Still no warning about the alphabetical order ? > > ouch no. > > It's also weird that I get this on a previous patch > > -------------------------------------------------------------------------------------------- > 3d2e23d0730b257931bc101aafac08c15f0789e6 libcamera: pipeline_handler: Split request queueing > -------------------------------------------------------------------------------------------- > --- include/libcamera/internal/pipeline_handler.h > +++ include/libcamera/internal/pipeline_handler.h > @@ -8,7 +8,6 @@ > #pragma once > > #include <memory> > -#include <queue> > #include <set> > #include <string> > #include <sys/types.h> > --- > 1 potential issue detected, please review There's probably a few things to fix in checkstyle.py :-S > > > #include "libcamera/internal/device_enumerator.h" > > > #include "libcamera/internal/media_device.h" > > > #include "libcamera/internal/request.h" > > > @@ -36,6 +38,8 @@ > > > * the REGISTER_PIPELINE_HANDLER() macro. > > > */ > > > > > > +using namespace std::chrono_literals; > > > + > > > namespace libcamera { > > > > > > LOG_DEFINE_CATEGORY(Pipeline) > > > @@ -323,10 +327,16 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const > > > * \param[in] request The request to queue > > > * > > > * This function 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 cancelled. > > > + * processing. The request is first added to the internal list of waiting > > > + * requests which have to be prepared to make sure they are ready for being > > > + * queued to the pipeline handler. > > > + * > > > + * The queue of waiting requests is iterated and all prepared requests are > > > + * passed to the pipeline handler in the same order they have been queued by > > > + * calling this function. > > > + * > > > + * If a Request fails during the preparation phase or if the pipeline handler > > > + * fails in queuing the request to the hardware the request is cancelled. > > > * > > > * Keeping track of queued requests ensures automatic completion of all requests > > > * when the pipeline handler is stopped with stop(). Request completion shall be > > > @@ -339,11 +349,20 @@ void PipelineHandler::queueRequest(Request *request) > > > LIBCAMERA_TRACEPOINT(request_queue, request); > > > > > > waitingRequests_.push(request); > > > - doQueueRequests(); > > > + > > > + request->_d()->prepared.connect(this, [this]() { > > > + doQueueRequests(); > > > + }); > > > + request->_d()->prepare(300ms); > > > } > > > > > > /** > > > * \brief Queue one requests to the device > > > + * > > > + * If a Request has failed during the preparation phase it is cancelled. > > > + * > > > + * If queuing a Request to the pipeline handler fails, the Request is cancelled > > > + * as well. > > > */ > > > void PipelineHandler::doQueueRequest(Request *request) > > > { > > > @@ -355,6 +374,12 @@ void PipelineHandler::doQueueRequest(Request *request) > > > > > > request->_d()->sequence_ = data->requestSequence_++; > > > > > > + if (request->_d()->cancelled_) { > > > + request->_d()->cancel(); > > > > Not that it's incorrect, but this looks really weird and calls for some > > refactoring on top of this series. Could you add a \todo comment ? > > Yes, it feels weird. But either I add new flags specific to the > prepare phase result or I do this. > > I thought I can't call Request::Private::cancel() directly from the timeout > handler as it would complete the request out of order, but it actually > only notifies buffers completion not the request. I can try looking > into that. The whole cancellation API is a bit dirty in my opinion. We can address it on top of this. > > > + completeRequest(request); > > > + return; > > > + } > > > + > > > int ret = queueRequestDevice(camera, request); > > > if (ret) { > > > request->_d()->cancel(); > > > @@ -365,19 +390,18 @@ void PipelineHandler::doQueueRequest(Request *request) > > > /** > > > * \brief Queue requests to the device > > > > Maybe > > > > * \brief Queue prepared requests to the device > > > > ? > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > * > > > - * Iterate the lst of waiting requests and queue them to the hardware one > > > - * by one. > > > + * Iterate the queue of waiting requests and queue them in order the hardware if > > > + * they have completed their preparation phase. > > > */ > > > void PipelineHandler::doQueueRequests() > > > { > > > - while (true) { > > > - if (waitingRequests_.empty()) > > > - return; > > > - > > > + while (!waitingRequests_.empty()) { > > > Request *request = waitingRequests_.front(); > > > - waitingRequests_.pop(); > > > + if (!request->_d()->prepared_) > > > + break; > > > > > > doQueueRequest(request); > > > + waitingRequests_.pop(); > > > } > > > } > > >
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 8f1f20e896b0..0a6a2e6ee983 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -7,6 +7,7 @@ #include "libcamera/internal/pipeline_handler.h" +#include <chrono> #include <sys/sysmacros.h> #include <libcamera/base/log.h> @@ -17,6 +18,7 @@ #include <libcamera/framebuffer.h> #include "libcamera/internal/camera.h" +#include "libcamera/internal/framebuffer.h" #include "libcamera/internal/device_enumerator.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/request.h" @@ -36,6 +38,8 @@ * the REGISTER_PIPELINE_HANDLER() macro. */ +using namespace std::chrono_literals; + namespace libcamera { LOG_DEFINE_CATEGORY(Pipeline) @@ -323,10 +327,16 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const * \param[in] request The request to queue * * This function 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 cancelled. + * processing. The request is first added to the internal list of waiting + * requests which have to be prepared to make sure they are ready for being + * queued to the pipeline handler. + * + * The queue of waiting requests is iterated and all prepared requests are + * passed to the pipeline handler in the same order they have been queued by + * calling this function. + * + * If a Request fails during the preparation phase or if the pipeline handler + * fails in queuing the request to the hardware the request is cancelled. * * Keeping track of queued requests ensures automatic completion of all requests * when the pipeline handler is stopped with stop(). Request completion shall be @@ -339,11 +349,20 @@ void PipelineHandler::queueRequest(Request *request) LIBCAMERA_TRACEPOINT(request_queue, request); waitingRequests_.push(request); - doQueueRequests(); + + request->_d()->prepared.connect(this, [this]() { + doQueueRequests(); + }); + request->_d()->prepare(300ms); } /** * \brief Queue one requests to the device + * + * If a Request has failed during the preparation phase it is cancelled. + * + * If queuing a Request to the pipeline handler fails, the Request is cancelled + * as well. */ void PipelineHandler::doQueueRequest(Request *request) { @@ -355,6 +374,12 @@ void PipelineHandler::doQueueRequest(Request *request) request->_d()->sequence_ = data->requestSequence_++; + if (request->_d()->cancelled_) { + request->_d()->cancel(); + completeRequest(request); + return; + } + int ret = queueRequestDevice(camera, request); if (ret) { request->_d()->cancel(); @@ -365,19 +390,18 @@ void PipelineHandler::doQueueRequest(Request *request) /** * \brief Queue requests to the device * - * Iterate the lst of waiting requests and queue them to the hardware one - * by one. + * Iterate the queue of waiting requests and queue them in order the hardware if + * they have completed their preparation phase. */ void PipelineHandler::doQueueRequests() { - while (true) { - if (waitingRequests_.empty()) - return; - + while (!waitingRequests_.empty()) { Request *request = waitingRequests_.front(); - waitingRequests_.pop(); + if (!request->_d()->prepared_) + break; doQueueRequest(request); + waitingRequests_.pop(); } }
Before queueing a request to the device, any synchronization fence from the Request' framebuffers has to be waited on. Connect the Request::Private::prepared signal to the function that queues requests to the hardware and call Request::Private::prepare(). When the waiting request queue is inspected, verify if has completed its preparation phase and queue it to the device. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline_handler.cpp | 48 ++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 12 deletions(-)