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

[Jacopo Mondi]

Hi Kieran

On Tue, Nov 09, 2021 at 02:39:31PM +0000, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2021-10-28 12:15:17)
> > 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);
>
> Should we add a new TRACEPOINT for when the request gets queued to the
> device now?
>

Dunno, this seems internal stuff, while I assumed tracepoints are
meant to track public API interactions ? Am I mistaken ?

> >
> > +       {
> > +               MutexLocker lock(waitingRequestsMutex_);
> > +               waitingRequests_.push_back(request);
> > +       }
> > +
> > +       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();
>
> Is this rename data -> camData required? it would simplify the diff if
> it was done separately. Otherwise - perhaps mention in the
> commit-message.

It's probably a leftover, I'll keep using 'data'.

>
>
> >
> > -       request->sequence_ = data->requestSequence_++;
> > +       request->sequence_ = camData->requestSequence_++;
> > +       camData->queuedRequests_.push_back(request);
> >
> >         int ret = queueRequestDevice(camera, request);
>
> So we have a queue before we queue now... ;-)
>
> We probably need to update/create some new block diagrams for the
> Pipeline Handler documentation to show how the internal queuing works ...
>
> So I think this is now
> 	queueRequest();  // Add to the queue
> 	-> doQueueRequest(); // See if we're allowed to queue
> 	  -> queueRequestDevice(); // Add to the kernel/hw queue ...
>

it's more like

         queueRequest();
         if (ready)
                doQueueRequest()
                        queueRequestDevice()
         else
                waitOnFences()
                        if (fencesDone)
                                doQueueRequest()
                                        queueRequestDevice()


> >         if (ret) {
> >                 request->cancel();
> >                 completeRequest(request);
> >         }
> > +
> > +       /* Try to queue the next Request in the queue, if ready. */
> > +       doQueueRequest();
>
> Hrm... I get a little scared/pessimistic when I see recursion. Is this
> better handled as a loop some how?
>

Why so ? It's one more level of indendation and I'm not sure what
benefits it would bring :)
>
> >  }
> >
> >  /**
> > --
> > 2.33.1
> >