[libcamera-devel] [PATCH v3 10/11] libcamera: pipeline_handler: Prepare Request

[Jacopo Mondi]

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 at 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 at 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