[libcamera-devel] [PATCH 09/10] libcamera: pipeline_handler: Handle fences
Jacopo Mondi
jacopo at jmondi.org
Tue Nov 9 18:57:24 CET 2021
Hi Kieran
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
>
> > 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.
Thanks
j
> > +
> > + 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
> >
More information about the libcamera-devel
mailing list