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

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

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