[libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler: Split request queueing

[Umang Jain]

Hi Jacopo,

Thank you for the patch. One question below

On 10/28/21 4:45 PM, Jacopo Mondi wrote:
> In order to prepare to handle synchronization fences at Request
> queueing time, split the PipelineHandler::queueRequest() function in
> two, by creating a list of waiting requests and introducing a new
> doQueueDevice() function that queues Requests to the device in the
> order the pipeline has received them.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>   include/libcamera/internal/pipeline_handler.h |  7 ++++
>   src/libcamera/pipeline_handler.cpp            | 35 +++++++++++++++++--
>   2 files changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 41cba44d990f..afb7990ea21b 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -7,7 +7,9 @@
>   #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__
>   #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__
>   
> +#include <list>
>   #include <memory>
> +#include <mutex>
>   #include <set>
>   #include <string>
>   #include <sys/types.h>
> @@ -76,9 +78,14 @@ private:
>   	void mediaDeviceDisconnected(MediaDevice *media);
>   	virtual void disconnect();
>   
> +	void doQueueRequest();
> +
>   	std::vector<std::shared_ptr<MediaDevice>> mediaDevices_;
>   	std::vector<std::weak_ptr<Camera>> cameras_;
>   
> +	std::mutex waitingRequestsMutex_;
> +	std::list<Request *> waitingRequests_;
> +
>   	const char *name_;
>   
>   	friend class PipelineHandlerFactory;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index cada864687ff..38edd00cebad 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -10,6 +10,7 @@
>   #include <sys/sysmacros.h>
>   
>   #include <libcamera/base/log.h>
> +#include <libcamera/base/thread.h>
>   #include <libcamera/base/utils.h>
>   
>   #include <libcamera/camera.h>
> @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request)
>   {
>   	LIBCAMERA_TRACEPOINT(request_queue, request);
>   
> +	{
> +		MutexLocker lock(waitingRequestsMutex_);
> +		waitingRequests_.push_back(request);


Why do we need to lock this? Are there multiple threads accessing 
PipelineHandler::queueRequest()

The documentation of the function states

\context This function is called from the CameraManager thread.

which I believe is all synchronous at this point.

> +	}
> +
> +	doQueueRequest();
> +}
> +
> +/**
> + * \brief Queue a request to the device
> + */
> +void PipelineHandler::doQueueRequest()
> +{
> +	Request *request = nullptr;
> +	{
> +		MutexLocker lock(waitingRequestsMutex_);
> +
> +		if (!waitingRequests_.size())
> +			return;
> +
> +		request = waitingRequests_.front();
> +		waitingRequests_.pop_front();
> +	}
> +
> +	/* Queue Request to the pipeline handler. */
>   	Camera *camera = request->_d()->camera();
> -	Camera::Private *data = camera->_d();
> -	data->queuedRequests_.push_back(request);
> +	Camera::Private *camData = camera->_d();
>   
> -	request->sequence_ = data->requestSequence_++;
> +	request->sequence_ = camData->requestSequence_++;
> +	camData->queuedRequests_.push_back(request);
>   
>   	int ret = queueRequestDevice(camera, request);
>   	if (ret) {
>   		request->cancel();
>   		completeRequest(request);
>   	}
> +
> +	/* Try to queue the next Request in the queue, if ready. */
> +	doQueueRequest();
>   }
>   
>   /**