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

[Jacopo Mondi]

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 ?

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