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

[Kieran Bingham]

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.


does this impact on the recent work to have an external
FrameBufferAllocator at all?

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