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

[Laurent Pinchart]

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 ?

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