[libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler: Split request queueing
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Nov 12 16:39:48 CET 2021
Hello,
On Wed, Nov 10, 2021 at 06:47:01PM +0530, Umang Jain wrote:
> On 11/10/21 6:38 PM, Jacopo Mondi wrote:
> > On Wed, Nov 10, 2021 at 06:20:01PM +0530, Umang Jain wrote:
> >> 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.
As mentioned in a separate e-mail in this thread, pipeline handlers are
single-threaded, so there's no need for a lock.
> >>> + }
> >>> +
> >>> + 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();
> >>> }
> >>> /**
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list