[libcamera-devel] [PATCH v2 09/12] libcamera: pipeline_handler: Prepare Request

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sat, Nov 20, 2021 at 12:13:10PM +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 the Request::Private
> status to discern if a Request has failed preparing or it can be queued
> to the hardware.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline_handler.cpp | 32 +++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 3b9145992c8f..82ef214d16f2 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"

checkstyle.py didn't warn 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)
> @@ -338,7 +342,14 @@ void PipelineHandler::queueRequest(Request *request)
>  	LIBCAMERA_TRACEPOINT(request_queue, request);
>  
>  	waitingRequests_.push_back(request);
> -	doQueueRequests();
> +
> +	request->_d()->prepared.connect(this, [this]() {
> +						doQueueRequests();
> +					});
> +
> +	Request::Private::Status status = request->_d()->prepare(300ms);
> +	if (status == Request::Private::Status::Ready)
> +		doQueueRequests();
>  }
>  
>  /**
> @@ -354,6 +365,12 @@ void PipelineHandler::doQueueRequest(Request *request)
>  
>  	request->_d()->sequence_ = data->requestSequence_++;
>  
> +	if (request->_d()->privateStatus() == Request::Private::Status::Failed) {
> +		request->_d()->cancel();
> +		completeRequest(request);
> +		return;
> +	}
> +
>  	int ret = queueRequestDevice(camera, request);
>  	if (ret) {
>  		request->_d()->cancel();
> @@ -371,9 +388,18 @@ void PipelineHandler::doQueueRequests()
>  			return;
>  
>  		Request *request = waitingRequests_.front();
> -		waitingRequests_.pop_front();
> +		Request::Private::Status status = request->_d()->privateStatus();
> +
> +		switch (status) {
> +		case Request::Private::Status::Pending:
> +			return;
>  
> -		doQueueRequest(request);
> +		case Request::Private::Status::Failed:
> +		case Request::Private::Status::Ready:
> +			waitingRequests_.pop_front();
> +			doQueueRequest(request);
> +			break;
> +		}
>  	}

The patch looks fine overall, but will likely change with the removal
(or refactoring) of privateStatus().

>  }
>  

-- 
Regards,

Laurent Pinchart