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

[Kieran Bingham]

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?

>  
> +       {
> +               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.


>  
> -       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 ...

>         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?


>  }
>  
>  /**
> -- 
> 2.33.1
>