[libcamera-devel] [PATCH 06/10] libcamera: framebuffer: Add synchronization Fence

[Jacopo Mondi]

Hi Kieran

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

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

Thanks
  j

> >
> >         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?
>
>
> > + *
> > + * 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 ...
>
> 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
> > --
> > 2.33.1
> >