[libcamera-devel] [PATCH 06/10] libcamera: framebuffer: Add synchronization Fence
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Nov 12 16:38:07 CET 2021
Hi Jacopo,
Thank you for the patch.
On Tue, Nov 09, 2021 at 06:36:37PM +0100, Jacopo Mondi wrote:
> On Tue, Nov 09, 2021 at 01:53:11PM +0000, Kieran Bingham wrote:
> > Quoting Jacopo Mondi (2021-10-28 12:15:16)
> > > Add an optional synchronization file descriptor to the FrameBuffer constuctor.
s/constuctor/constructor/
> > >
> > > The fence is handled by the FrameBuffer::Private class by constructing
> > > a Fence class instance to wrap the file descriptor. Once the Request the
> > > FrameBuffer is part of has completed, the fence file descriptor will read
> > > as -1 if successfully handled by the library; otherwise the file
> > > descriptor value is kept and applications are responsible for closing
> > > it.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > include/libcamera/framebuffer.h | 5 ++-
> > > include/libcamera/internal/framebuffer.h | 7 +++-
> > > src/libcamera/framebuffer.cpp | 46 +++++++++++++++++++++---
> > > 3 files changed, 52 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> > > index 7f2f176af691..ac96790b76c0 100644
> > > --- a/include/libcamera/framebuffer.h
> > > +++ b/include/libcamera/framebuffer.h
> > > @@ -57,7 +57,8 @@ public:
> > > unsigned int length;
> > > };
> > >
> > > - FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0);
> > > + FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0,
> > > + int fence = -1);
> >
> > Hrm, -1 seems a bit odd here, Perhaps we should be passing in a
> > FileDescriptor.
>
> That would also make clear that apps are not meant to touch the Fence
> once it has been added to the FrameBuffer
Or even passing a Fence ? We'd need a default Fence constructor for an
invalid fence.
I think the fence should be associated with the frame buffer when adding
it to a request. In the use case where FrameBuffer instances are created
at init time and reused, we'll have a new fence every time a frame
buffer is queued in a request.
We need to figure out what to do with fences that fail time out, we need
a way to extract them from the frame buffer when the request completes
(in order to pass them back to the Android camera service), without
closing the fd at that point, but we also need to make sure that the fd
will get closed automatically if the application doesn't handle the
fence at completion time to avoid leaks.
> > does this impact on the recent work to have an external
> > FrameBufferAllocator at all?
>
> Not sure. I guess its interface should be expanded to accept an
> optional fence. Being it Android-only, the FrameBufferAllocator can
> accept a raw int and wrap it in a FileDescriptor before creating the
> FrameBuffer.
>
> > >
> > > const std::vector<Plane> &planes() const { return planes_; }
> > > Request *request() const;
> > > @@ -66,6 +67,8 @@ public:
> > > unsigned int cookie() const { return cookie_; }
> > > void setCookie(unsigned int cookie) { cookie_ = cookie; }
> > >
> > > + int fence() const;
> > > +
> > > void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }
> > >
> > > private:
> > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
> > > index cd33c295466e..db1a4bd72354 100644
> > > --- a/include/libcamera/internal/framebuffer.h
> > > +++ b/include/libcamera/internal/framebuffer.h
> > > @@ -11,6 +11,8 @@
> > >
> > > #include <libcamera/framebuffer.h>
> > >
> > > +#include <libcamera/internal/fence.h>
> > > +
> > > namespace libcamera {
> > >
> > > class FrameBuffer::Private : public Extensible::Private
> > > @@ -18,14 +20,17 @@ class FrameBuffer::Private : public Extensible::Private
> > > LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
> > >
> > > public:
> > > - Private();
> > > + Private(int fence);
> > >
> > > void setRequest(Request *request) { request_ = request; }
> > > bool isContiguous() const { return isContiguous_; }
> > > + const Fence &fence() const { return fence_; }
> > > + Fence &fence() { return fence_; }
> > >
> > > private:
> > > Request *request_;
> > > bool isContiguous_;
> > > + Fence fence_;
> > > };
> > >
> > > } /* namespace libcamera */
> > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > > index d44a98babd05..c3e03896b184 100644
> > > --- a/src/libcamera/framebuffer.cpp
> > > +++ b/src/libcamera/framebuffer.cpp
> > > @@ -111,8 +111,12 @@ LOG_DEFINE_CATEGORY(Buffer)
> > > * pipeline handlers.
> > > */
> > >
> > > -FrameBuffer::Private::Private()
> > > - : request_(nullptr), isContiguous_(true)
> > > +/**
> > > + * \brief Construct a FrameBuffer::Private instance
> > > + * \param[in] fence The synchronization fence file descriptor
> > > + */
> > > +FrameBuffer::Private::Private(int fence)
> > > + : request_(nullptr), isContiguous_(true), fence_(fence)
> > > {
> > > }
> > >
> > > @@ -137,6 +141,18 @@ FrameBuffer::Private::Private()
> > > * \return True if the planes are stored contiguously in memory, false otherwise
> > > */
> > >
> > > +/**
> > > + * \fn const FrameBuffer::Private::fence() const
> > > + * \brief Return a const reference to the Fence
> > > + * \return A const reference to the frame buffer fence
> > > + */
> > > +
> > > +/**
> > > + * \fn FrameBuffer::Private::fence()
> > > + * \brief Return a reference to the Fence
> > > + * \return A reference to the frame buffer fence
> > > + */
> > > +
> > > /**
> > > * \class FrameBuffer
> > > * \brief Frame buffer data and its associated dynamic metadata
> > > @@ -211,9 +227,11 @@ FrameBuffer::Private::Private()
> > > * \brief Construct a FrameBuffer with an array of planes
> > > * \param[in] planes The frame memory planes
> > > * \param[in] cookie Cookie
> > > + * \param[in] fence Synchronization fence
> > > */
> > > -FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
> > > - : Extensible(std::make_unique<Private>()), planes_(planes),
> > > +FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie,
> > > + int fence)
> > > + : Extensible(std::make_unique<Private>(fence)), planes_(planes),
> > > cookie_(cookie)
> > > {
> > > metadata_.planes_.resize(planes_.size());
> > > @@ -305,6 +323,26 @@ Request *FrameBuffer::request() const
> > > * libcamera core never modifies the buffer cookie.
> > > */
> > >
> > > +/**
> > > + * \fn FrameBuffer::fence()
> > > + * \brief Return the synchronization fence file descriptor
> > > + *
> > > + * The fence file descriptor is set by applications at construction time.
> > > + *
> > > + * Once the Request is queued to the Camera the fence is handled by the
> > > + * library and if successfully notified the fence will read as -1 after the
> > > + * Request has completed.
> >
> > Are fences always kept with the FrameBuffer allocations? Or is it a new
> > fence for everytime the FrameBuffer is queued?
See above ;-)
> > > + *
> > > + * If waiting for fence fails, the fence is instead kept in the FrameBuffer
> > > + * after the Request has completed and it is responsibility of the
> > > + * application to correctly close it.
> >
> > By simply closing the fd? Would this be automatic if it was constructed
> > into a FileDescriptor?
> >
> > I don't think I've understood the underlying lifetime of the fence
> > mechanisms enough yet ...
A bit more documentation would indeed be useful.
> > I'll keep going and see if I get enlightenment from the following
> > patches.
> >
> > > + */
> > > +int FrameBuffer::fence() const
> > > +{
> > > + const Fence &fence = _d()->fence();
> > > + return fence.fd();
> > > +}
> > > +
> > > /**
> > > * \fn FrameBuffer::cancel()
> > > * \brief Marks the buffer as cancelled
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list