| Message ID | 20211028111520.256612-7-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 >
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@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 > >
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@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
Hi Laurent On Fri, Nov 12, 2021 at 05:38:07PM +0200, Laurent Pinchart wrote: > 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@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. Yes, and that's why I insisted on Fence having a move only semantic. The key idea (which could be achieved in different ways than moving fences, by storing them as unique ptrs in example as you suggested) was indeed to guarantee that: - Fences (and their underlying file descriptor) are only owned by a single component - Framebuffer creation -> frame buffer internal - Request::addBuffer() -> frame buffer internal - Camera::queueRequest() -> request internal, read as -1 from frame buffer - Request::complete() - either deleted if fence has been signaled or moved back to the frame buffer if timedout All of this can be read as: if a completed buffer has a fence, then it is reponsibility of the app to handle it. Otherwise it is guaranteed the core closed it. I thought it was a clear interaction pattern for apps, and I had integers as representation towards apps as it read more natural "if (buffer.fence != -1)" but indeed it will require to rethink the interface completely when we'll support other types of fences. I'll see if a similar semantic can be implemented by using unique_ptr<> only, but it felt so natural to me to use move semantic to -move back- a fence to a Framebuffer if it had timedout to give it back to applications. Probably we can keep the fence owned by the framebuffer for the whole Request lifecycle, and simply .release() it if it gets signalled before timeout. > > > > 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
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); 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. + * + * 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. + */ +int FrameBuffer::fence() const +{ + const Fence &fence = _d()->fence(); + return fence.fd(); +} + /** * \fn FrameBuffer::cancel() * \brief Marks the buffer as cancelled
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@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(-)