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

[Umang Jain]

Hi Jacopo

On 11/10/21 6:38 PM, Jacopo Mondi wrote:
> Hi Umang
>
> On Wed, Nov 10, 2021 at 06:20:01PM +0530, Umang Jain wrote:
>> 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.
> The waitingRequests_ queue is accessed by the doQueueRequest()
> function which might get called in response to fence completion event.
>
> But now that I've said so, the EventDispatcher that sends fence
> completions is the one running in the current thread, which is the
> same as the CameraManager one, so it might be safe to remove the
> locking maybe ?


That's what I am contemplating too, the slot when triggered will run in 
the same thread (which is the camera-manager one). I see no harm in 
removing the lock but I maybe wrong too. Uptill now, in my limited 
reading around queueRequest and the PH class, I didn't see any separate 
thread angle coming up so... I'll keep an lookout.

>
> Thanks
>    j
>
>>> +	}
>>> +
>>> +	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();
>>>    }
>>>    /**