[libcamera-devel] [PATCH 09/10] libcamera: pipeline_handler: Handle fences
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Nov 12 17:02:57 CET 2021
Hi Jacopo,
Thank you for the patch.
On Tue, Nov 09, 2021 at 06:57:24PM +0100, Jacopo Mondi wrote:
> On Tue, Nov 09, 2021 at 04:07:43PM +0000, Kieran Bingham wrote:
> > 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?
>
> depends :) For sofware fences I think if a fence gets signalled before
> we get to poll() on it, I think poll() would immediately return
> signalling the fence is read.
>
> I assume hw fences should work the same, but I can't tell for sure
That's how the fence API is meant to behave, yes.
> > > 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....
>
> They get moved to Request::Private
>
> Request::Private *data = request->_d();
>
> for (buffer : request->pending)
> data->addFence(std::move(buffer->_d()->fence()));
>
> > 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...
>
> However I got your point, the Request could be auto-counting, ie we
> connect the Fence::complete signal to a slot in Request::Private and
> basically move the fence counting implemented in
> PipelineHandler::fenceCompleted() there. I initially wanted this but I
> got discouraged by the need to have the Request then queue itself when
> it's ready. Now that I look at it again, we could add to
> Request::Private a 'done' (or 'ready' or whatever) signal to which we
> connect a slot in PipelineHandler. This would also simplify the
> Request::Private::fence*() interface that can be made private to
> Request::Private.
>
> > Then the PipelineHandler wouldn't have to be digging around in the
> > internals of the Request buffers.
>
> I'll experiment with that, but please note that fences will anyway
> have to be moved to Request::Private here anyhow, as doing so at
> Request::addBuffer() time would invalidate the fence as seen from
> application much earlier than Camera::queueRequest() time, something I
> would rather not do.
>
> > 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?
>
> Let's see how it looks like. I didn't want the request to call
> PipelineHandler::doQueueRequest() when it's ready, but maybe with a
> Signal it gets nicer.
>
> > That's a fair bit of redesign though - so if you say "No, it's better
> > this way" I'll just move on ;-)
>
> Ouch, I already acknoleded you comment. Can I take it back and say
> "No, it's better this way" ? :D
>
> > > +
> > > + /* 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
>
> Still not convinced, mostly for a style point of view of having one
> more indentation layer.
You can split the function in two, doQueueRequest() and
doQueueRequests(), with the latter implementing the loop and calling the
former.
> > > +
> > > + 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();
> > > }
> > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list