[libcamera-devel] [PATCH 09/10] libcamera: pipeline_handler: Handle fences

[Kieran Bingham]

Quoting Jacopo Mondi (2021-10-28 12:15:19)
> Before queueing a request to the device, any synchronization fence from
> the Request framebuffers has to be waited on.
> 
> Start by moving the fences to the Request, in order to reset the ones
> in the FrameBuffers and then enable all of them one by one.


Aha, ok so now I'm starting to understand the lifetime of the fence.

If a Buffer is added with a fence to protect it until it's ready, do we
introduce any kind of race when enabling if the buffer fence has
completed /before/ we enable it?

I.e. could we end up sitting on a fence waiting for a completion that
has already occured, and then timeout - and cause the request to be
incorrectly cancelled?


> When a fence completes, make sure all fences in the Request have been
> waited on or have expired and only after that actually queue the Request
> to the device.
> 
> If any fence in the Request has expired cancel the request and do not
> queue it to the device.

Sounds good...

> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/internal/pipeline_handler.h |  2 +
>  src/libcamera/pipeline_handler.cpp            | 78 ++++++++++++++++++-
>  2 files changed, 77 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 5c3c0bc5a8b3..5b98011ac4f6 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -29,6 +29,7 @@ class CameraConfiguration;
>  class CameraManager;
>  class DeviceEnumerator;
>  class DeviceMatch;
> +class Fence;
>  class FrameBuffer;
>  class MediaDevice;
>  class PipelineHandler;
> @@ -79,6 +80,7 @@ private:
>         void mediaDeviceDisconnected(MediaDevice *media);
>         virtual void disconnect();
>  
> +       void fenceCompleted(Request *request, FrameBuffer *buffer, Fence *fence);
>         void doQueueRequest();
>  
>         std::vector<std::shared_ptr<MediaDevice>> mediaDevices_;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 0f9fec1b618f..39cb680e5386 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -18,6 +18,7 @@
>  #include <libcamera/framebuffer.h>
>  
>  #include "libcamera/internal/camera.h"
> +#include "libcamera/internal/framebuffer.h"
>  #include "libcamera/internal/device_enumerator.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/request.h"
> @@ -336,11 +337,70 @@ void PipelineHandler::queueRequest(Request *request)
>  {
>         LIBCAMERA_TRACEPOINT(request_queue, request);
>  
> +       Request::Private *data = request->_d();
> +
>         {
>                 MutexLocker lock(waitingRequestsMutex_);
>                 waitingRequests_.push_back(request);
>         }
>  
> +       for (FrameBuffer *buffer : request->pending_) {
> +               if (buffer->fence() == -1)
> +                       continue;
> +
> +               /*
> +                * Move the Fence into the Request::Private list of
> +                * fences. This resets the file descriptor fence in the
> +                * FrameBuffer to -1.
> +                */
> +               data->addFence(std::move(buffer->_d()->fence()));
> +
> +               Fence *fence = &data->fences().back();
> +               fence->complete.connect(this,
> +                                       [this, request, buffer, fence]() {
> +                                               fenceCompleted(request, buffer, fence);
> +                                       });
> +       }

I can't help but wonder if this should be handled in the Request, and
not by moving the fences to the camera data....

The request could signal that it is ready for consumption, without
having to move the ownership of the fence from the buffer to the
camera...

Then the PipelineHandler wouldn't have to be digging around in the
internals of the Request buffers.

Of course it already has access to those, but I'm not 100% convinced,
this is a good reason to dig in there, and not expose an "I am ready"
signal from the request. But maybe that adds more layering on the requst
that is undesirable?


That's a fair bit of redesign though - so if you say "No, it's better
this way" I'll just move on ;-)



> +
> +       /* If no fences to wait on, we can queue the request immediately. */
> +       if (!data->pendingFences()) {
> +               doQueueRequest();
> +
> +               return;
> +       }
> +
> +       /*
> +        * Now that we have added all fences, enable them one by one.
> +        * Enabling fences while adding them to the Request would race on the
> +        * number of pending fences.
> +        */
> +       for (Fence &fence : data->fences())
> +               fence.enable();
> +}
> +
> +void PipelineHandler::fenceCompleted(Request *request, FrameBuffer *buffer,
> +                                    Fence *fence)
> +{
> +       Request::Private *data = request->_d();
> +
> +       if (fence->expired()) {
> +               /*
> +                * Move the fence back to the framebuffer, so that it doesn't
> +                * get closed and the file descriptor number is made
> +                * available again to applications.
> +                */
> +               FrameBuffer::Private *bufferData = buffer->_d();
> +               bufferData->fence() = std::move(*fence);
> +
> +               data->fenceExpired();
> +       } else {
> +               data->fenceCompleted();
> +       }
> +
> +       if (data->pendingFences())
> +               return;
> +
> +       data->clearFences();
>         doQueueRequest();
>  }
>  
> @@ -357,23 +417,35 @@ void PipelineHandler::doQueueRequest()
>                         return;
>  
>                 request = waitingRequests_.front();
> +               if (request->_d()->pendingFences())
> +                       return;
> +
>                 waitingRequests_.pop_front();
>         }
>  
> -       /* Queue Request to the pipeline handler. */
>         Camera *camera = request->_d()->camera();
>         Camera::Private *camData = camera->_d();
>  
>         request->sequence_ = camData->requestSequence_++;
> -       camData->queuedRequests_.push_back(request);
>  
> +       /* Cancel the request if one of the fences has failed. */
> +       if (request->_d()->expiredFences()) {
> +               request->cancel();
> +               completeRequest(request);
> +
> +               doQueueRequest();

This makes me really think this would be better handled in a loop rather
than recursion... :-S

> +
> +               return;
> +       }
> +
> +       /* Queue Request to the pipeline handler. */
> +       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();
>  }
>  
> -- 
> 2.33.1
>