[libcamera-devel] [PATCH v3 1/2] libcamera: pipeline_handler: Register requests

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Jan 21, 2022 at 02:24:19PM +0000, Kieran Bingham wrote:
> Provide a call allowing requests to be registered and associated with
> the pipeline handler after being constructed by the camera.
> 
> This provides an opportunity for the Pipeline Handler to connect any

s/Pipeline Handler/pipeline handler/ (or PipelineHandler)

> signals it may be interested in receiving for the request such as
> getting notifications when the request is ready for processing when
> using a fence.
> 
> While here, update the existing usage of the d pointer in
> Camera::createRequest() to match the style of other functions.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> 
> v3:
>  - Remove unneeded conditional on construction of the request.
> 
> v2:
>  - Rename function to 'registerRequest' rather than 'prepareRequest'.
> 
>  include/libcamera/internal/pipeline_handler.h |  1 +
>  src/libcamera/camera.cpp                      | 13 ++++++++++---
>  src/libcamera/pipeline_handler.cpp            | 19 ++++++++++++++++---
>  3 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index e5b8ffb4db3d..c3e4c2587ecd 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -59,6 +59,7 @@ public:
>  	void stop(Camera *camera);
>  	bool hasPendingRequests(const Camera *camera) const;
>  
> +	void registerRequest(Request *request);
>  	void queueRequest(Request *request);
>  
>  	bool completeBuffer(Request *request, FrameBuffer *buffer);
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 86d84ac0a77d..bb856d606f4a 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -1074,12 +1074,19 @@ int Camera::configure(CameraConfiguration *config)
>   */
>  std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)
>  {
> -	int ret = _d()->isAccessAllowed(Private::CameraConfigured,
> -					Private::CameraRunning);
> +	Private *const d = _d();
> +
> +	int ret = d->isAccessAllowed(Private::CameraConfigured,
> +				     Private::CameraRunning);
>  	if (ret < 0)
>  		return nullptr;
>  
> -	return std::make_unique<Request>(this, cookie);
> +	std::unique_ptr<Request> request = std::make_unique<Request>(this, cookie);
> +
> +	/* Associate the request with the pipeline handler. */
> +	d->pipe_->registerRequest(request.get());
> +
> +	return request;
>  }
>  
>  /**
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 03e4b9e66aa6..e27dc182c128 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -337,6 +337,22 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const
>  	return !camera->_d()->queuedRequests_.empty();
>  }
>  
> +/**
> + * \fn PipelineHandler::registerRequest()
> + * \brief Register a request for use by the Pipeline Handler

s/Pipeline Handler/pipeline handler/

> + * \param[in] request The request to register

Maybe we could add

 * This function is called when the request is created, and allows the pipeline
 * handler to perform any one-time initialization it requires for the request.

> + */
> +void PipelineHandler::registerRequest(Request *request)
> +{
> +	/*
> +	 * Connect the request prepared signal to notify the pipeline handler
> +	 * when a request is ready to be processed.
> +	 */
> +	request->_d()->prepared.connect(this, [this]() {
> +						doQueueRequests();
> +					});

While at it, this be simplified to

	request->_d()->prepared.connect(this, &PipelineHandler::doQueueRequests);

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +}
> +
>  /**
>   * \fn PipelineHandler::queueRequest()
>   * \brief Queue a request
> @@ -366,9 +382,6 @@ void PipelineHandler::queueRequest(Request *request)
>  
>  	waitingRequests_.push(request);
>  
> -	request->_d()->prepared.connect(this, [this]() {
> -						doQueueRequests();
> -					});
>  	request->_d()->prepare(300ms);
>  }
>  

-- 
Regards,

Laurent Pinchart