| Message ID | 20211028111520.256612-6-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jacopo Mondi (2021-10-28 12:15:15) > Prepare the Request::Private class to handle fences by providing a > storage vector and interface functions to handle the number of pending > and expired fences. > > The intended user of the interface is the PipelineHandler class > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/internal/request.h | 21 +++++++ > src/libcamera/request.cpp | 89 +++++++++++++++++++++++++++- > 2 files changed, 109 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h > index df0cc014067e..2be4874756de 100644 > --- a/include/libcamera/internal/request.h > +++ b/include/libcamera/internal/request.h > @@ -7,8 +7,12 @@ > #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__ > #define __LIBCAMERA_INTERNAL_REQUEST_H__ > > +#include <vector> > + > #include <libcamera/request.h> > > +#include <libcamera/internal/fence.h> > + > namespace libcamera { > > class Camera; > @@ -24,9 +28,26 @@ public: > > Camera *camera() const { return camera_; } > > + unsigned int pendingFences() const { return pendingFences_; } > + unsigned int expiredFences() const { return expiredFences_; } > + > + void reuse(); > + > + std::vector<Fence> &fences() { return fences_; } > + void addFence(Fence &&fence); > + void clearFences(); > + > + void fenceExpired(); > + void fenceCompleted(); > + > private: > Camera *camera_; > bool cancelled_; > + > + unsigned int pendingFences_ = 0; > + unsigned int expiredFences_ = 0; > + > + std::vector<Fence> fences_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index 33fee1ac05ba..e88eee1fac36 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -63,6 +63,92 @@ Request::Private::~Private() > * request hasn't been queued > */ > > +/** > + * \fn Request::Private::pendingFences() > + * \brief Retrieve the number of pending fences > + * > + * A Fence is pending if has not yet been signalled or it has not expired yet. > + * > + * \return The number of pending fences > + */ > + > +/** > + * \fn Request::Private::expiredFences() > + * \brief Retrieve the number of expired fences > + * \return The number of expired fences > + */ > + > +/** > + * \brief Reset the request for reuse > + */ > +void Request::Private::reuse() > +{ > + cancelled_ = false; > + > + fences_.clear(); > + pendingFences_ = 0; > + expiredFences_ = 0; > +} > + > +/** > + * \fn Request::Private::fences() > + * \brief Retrieve a reference to the vector of pending fences > + * \return A reference to the vector of pending fences > + */ > + > +/** > + * \brief Move a Fence into the Request > + * > + * Move a Fence into the Request and increase the pending fences > + * counter. The Fence is now stored into the Request and the original > + * one passed in as parameter is reset. > + * > + * Once the Fence completes, either because it is signalled or because > + * it has expired, the caller shall notify the Request about this by > + * calling fenceCompleted() or fenceExpired(); > + */ > +void Request::Private::addFence(Fence &&fence) > +{ > + fences_.push_back(std::move(fence)); > + pendingFences_++; Hrm ... I was expecting signals to be connected here ... but I guess that happens elsewhere... > +} > + > +/** > + * \brief Release all Fences stored in the request > + * > + * Clear the vector of fences in the Request by releasing all of them. > + * All Fences are closed and their file descriptors reset to 0. > + */ > +void Request::Private::clearFences() > +{ > + ASSERT(!pendingFences_); > + fences_.clear(); > +} > + > +/** > + * \brief Notify that a Fence has been signalled > + * > + * Notify to the Request that a Fence has completed. This function decrease the > + * number of pending Fences in the request. > + */ > +void Request::Private::fenceCompleted() > +{ > + pendingFences_--; > +} > + > +/** > + * \brief Notify that a Fence has expired > + * > + * Notify to the Request that a Fence has expired. This function decrease the > + * number of pending Fences in the request and increase the number of expired > + * ones. > + */ > +void Request::Private::fenceExpired() > +{ > + expiredFences_++; > + pendingFences_--; Are all these event driven counters thread safe? They may be, just seems a potential risk area... > +} > + > /** > * \enum Request::Status > * Request completion status > @@ -158,10 +244,11 @@ void Request::reuse(ReuseFlag flags) > > sequence_ = 0; > status_ = RequestPending; > - _d()->cancelled_ = false; > > controls_->clear(); > metadata_->clear(); > + > + _d()->reuse(); > } > > /** > -- > 2.33.1 >
Hi Kieran, On Tue, Nov 09, 2021 at 01:23:32PM +0000, Kieran Bingham wrote: > Quoting Jacopo Mondi (2021-10-28 12:15:15) > > Prepare the Request::Private class to handle fences by providing a > > storage vector and interface functions to handle the number of pending > > and expired fences. > > > > The intended user of the interface is the PipelineHandler class > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/internal/request.h | 21 +++++++ > > src/libcamera/request.cpp | 89 +++++++++++++++++++++++++++- > > 2 files changed, 109 insertions(+), 1 deletion(-) > > > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h > > index df0cc014067e..2be4874756de 100644 > > --- a/include/libcamera/internal/request.h > > +++ b/include/libcamera/internal/request.h > > @@ -7,8 +7,12 @@ > > #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__ > > #define __LIBCAMERA_INTERNAL_REQUEST_H__ > > > > +#include <vector> > > + > > #include <libcamera/request.h> > > > > +#include <libcamera/internal/fence.h> > > + > > namespace libcamera { > > > > class Camera; > > @@ -24,9 +28,26 @@ public: > > > > Camera *camera() const { return camera_; } > > > > + unsigned int pendingFences() const { return pendingFences_; } > > + unsigned int expiredFences() const { return expiredFences_; } > > + > > + void reuse(); > > + > > + std::vector<Fence> &fences() { return fences_; } > > + void addFence(Fence &&fence); > > + void clearFences(); > > + > > + void fenceExpired(); > > + void fenceCompleted(); > > + > > private: > > Camera *camera_; > > bool cancelled_; > > + > > + unsigned int pendingFences_ = 0; > > + unsigned int expiredFences_ = 0; > > + > > + std::vector<Fence> fences_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > index 33fee1ac05ba..e88eee1fac36 100644 > > --- a/src/libcamera/request.cpp > > +++ b/src/libcamera/request.cpp > > @@ -63,6 +63,92 @@ Request::Private::~Private() > > * request hasn't been queued > > */ > > > > +/** > > + * \fn Request::Private::pendingFences() > > + * \brief Retrieve the number of pending fences > > + * > > + * A Fence is pending if has not yet been signalled or it has not expired yet. > > + * > > + * \return The number of pending fences > > + */ > > + > > +/** > > + * \fn Request::Private::expiredFences() > > + * \brief Retrieve the number of expired fences > > + * \return The number of expired fences > > + */ > > + > > +/** > > + * \brief Reset the request for reuse > > + */ > > +void Request::Private::reuse() > > +{ > > + cancelled_ = false; > > + > > + fences_.clear(); > > + pendingFences_ = 0; > > + expiredFences_ = 0; > > +} > > + > > +/** > > + * \fn Request::Private::fences() > > + * \brief Retrieve a reference to the vector of pending fences > > + * \return A reference to the vector of pending fences > > + */ > > + > > +/** > > + * \brief Move a Fence into the Request > > + * > > + * Move a Fence into the Request and increase the pending fences > > + * counter. The Fence is now stored into the Request and the original > > + * one passed in as parameter is reset. > > + * > > + * Once the Fence completes, either because it is signalled or because > > + * it has expired, the caller shall notify the Request about this by > > + * calling fenceCompleted() or fenceExpired(); > > + */ > > +void Request::Private::addFence(Fence &&fence) > > +{ > > + fences_.push_back(std::move(fence)); > > + pendingFences_++; > > Hrm ... I was expecting signals to be connected here ... but I guess > that happens elsewhere... > > > > +} > > + > > +/** > > + * \brief Release all Fences stored in the request > > + * > > + * Clear the vector of fences in the Request by releasing all of them. > > + * All Fences are closed and their file descriptors reset to 0. > > + */ > > +void Request::Private::clearFences() > > +{ > > + ASSERT(!pendingFences_); > > + fences_.clear(); > > +} > > + > > +/** > > + * \brief Notify that a Fence has been signalled > > + * > > + * Notify to the Request that a Fence has completed. This function decrease the > > + * number of pending Fences in the request. > > + */ > > +void Request::Private::fenceCompleted() > > +{ > > + pendingFences_--; > > +} > > + > > +/** > > + * \brief Notify that a Fence has expired > > + * > > + * Notify to the Request that a Fence has expired. This function decrease the > > + * number of pending Fences in the request and increase the number of expired > > + * ones. > > + */ > > +void Request::Private::fenceExpired() > > +{ > > + expiredFences_++; > > + pendingFences_--; > > Are all these event driven counters thread safe? They may be, just seems > a potential risk area... My understanding is that slots are synchronous unless otherwise specified. As the Request::Private::fence*() functions are called from the PipelineHandler::fenceCompleted() slot connected to Fence::complete signal what happens is that - Fence::activated() stops the timer - Fence::timedout() disables the event notifier so a single Fence::complete is emitted per Fence. From there we call the PipelineHandler::fenceCompleted() slot which is run synchronously, hence I think we're safe. Does this seems safe to you as well ? Thanks j > > > > +} > > + > > /** > > * \enum Request::Status > > * Request completion status > > @@ -158,10 +244,11 @@ void Request::reuse(ReuseFlag flags) > > > > sequence_ = 0; > > status_ = RequestPending; > > - _d()->cancelled_ = false; > > > > controls_->clear(); > > metadata_->clear(); > > + > > + _d()->reuse(); > > } > > > > /** > > -- > > 2.33.1 > >
Hi Jacopo, On 10/28/21 4:45 PM, Jacopo Mondi wrote: > Prepare the Request::Private class to handle fences by providing a > storage vector and interface functions to handle the number of pending > and expired fences. > > The intended user of the interface is the PipelineHandler class > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/internal/request.h | 21 +++++++ > src/libcamera/request.cpp | 89 +++++++++++++++++++++++++++- > 2 files changed, 109 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h > index df0cc014067e..2be4874756de 100644 > --- a/include/libcamera/internal/request.h > +++ b/include/libcamera/internal/request.h > @@ -7,8 +7,12 @@ > #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__ > #define __LIBCAMERA_INTERNAL_REQUEST_H__ > > +#include <vector> > + > #include <libcamera/request.h> > > +#include <libcamera/internal/fence.h> > + > namespace libcamera { > > class Camera; > @@ -24,9 +28,26 @@ public: > > Camera *camera() const { return camera_; } > > + unsigned int pendingFences() const { return pendingFences_; } > + unsigned int expiredFences() const { return expiredFences_; } > + > + void reuse(); > + > + std::vector<Fence> &fences() { return fences_; } > + void addFence(Fence &&fence); > + void clearFences(); > + > + void fenceExpired(); > + void fenceCompleted(); > + > private: > Camera *camera_; > bool cancelled_; > + > + unsigned int pendingFences_ = 0; > + unsigned int expiredFences_ = 0; > + > + std::vector<Fence> fences_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index 33fee1ac05ba..e88eee1fac36 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -63,6 +63,92 @@ Request::Private::~Private() > * request hasn't been queued > */ > > +/** > + * \fn Request::Private::pendingFences() > + * \brief Retrieve the number of pending fences > + * > + * A Fence is pending if has not yet been signalled or it has not expired yet. > + * > + * \return The number of pending fences > + */ > + > +/** > + * \fn Request::Private::expiredFences() > + * \brief Retrieve the number of expired fences > + * \return The number of expired fences > + */ > + > +/** > + * \brief Reset the request for reuse > + */ > +void Request::Private::reuse() > +{ > + cancelled_ = false; > + > + fences_.clear(); > + pendingFences_ = 0; > + expiredFences_ = 0; > +} > + > +/** > + * \fn Request::Private::fences() > + * \brief Retrieve a reference to the vector of pending fences > + * \return A reference to the vector of pending fences > + */ > + > +/** > + * \brief Move a Fence into the Request > + * > + * Move a Fence into the Request and increase the pending fences > + * counter. The Fence is now stored into the Request and the original > + * one passed in as parameter is reset. > + * > + * Once the Fence completes, either because it is signalled or because > + * it has expired, the caller shall notify the Request about this by > + * calling fenceCompleted() or fenceExpired(); > + */ > +void Request::Private::addFence(Fence &&fence) > +{ > + fences_.push_back(std::move(fence)); Drive-by comment, please let me know if I am wrong: As per my understanding and your clarification, the fence is moved to a new Fence and pushed into fences_. I believe this new Fence will be disabled (by-default behaviour as stated in Fence class). So I don't think any of fenceCompleted() or fenceExpired() getting called, which seems problematic. > + pendingFences_++; > +} > + > +/** > + * \brief Release all Fences stored in the request > + * > + * Clear the vector of fences in the Request by releasing all of them. > + * All Fences are closed and their file descriptors reset to 0. > + */ > +void Request::Private::clearFences() > +{ > + ASSERT(!pendingFences_); > + fences_.clear(); > +} > + > +/** > + * \brief Notify that a Fence has been signalled > + * > + * Notify to the Request that a Fence has completed. This function decrease the > + * number of pending Fences in the request. > + */ > +void Request::Private::fenceCompleted() > +{ > + pendingFences_--; > +} > + > +/** > + * \brief Notify that a Fence has expired > + * > + * Notify to the Request that a Fence has expired. This function decrease the > + * number of pending Fences in the request and increase the number of expired > + * ones. > + */ > +void Request::Private::fenceExpired() > +{ > + expiredFences_++; > + pendingFences_--; > +} > + > /** > * \enum Request::Status > * Request completion status > @@ -158,10 +244,11 @@ void Request::reuse(ReuseFlag flags) > > sequence_ = 0; > status_ = RequestPending; > - _d()->cancelled_ = false; > > controls_->clear(); > metadata_->clear(); > + > + _d()->reuse(); > } > > /**
Hi Umang, On Wed, Nov 10, 2021 at 02:51:44PM +0530, Umang Jain wrote: > Hi Jacopo, > > On 10/28/21 4:45 PM, Jacopo Mondi wrote: > > Prepare the Request::Private class to handle fences by providing a > > storage vector and interface functions to handle the number of pending > > and expired fences. > > > > The intended user of the interface is the PipelineHandler class > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/internal/request.h | 21 +++++++ > > src/libcamera/request.cpp | 89 +++++++++++++++++++++++++++- > > 2 files changed, 109 insertions(+), 1 deletion(-) > > > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h > > index df0cc014067e..2be4874756de 100644 > > --- a/include/libcamera/internal/request.h > > +++ b/include/libcamera/internal/request.h > > @@ -7,8 +7,12 @@ > > #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__ > > #define __LIBCAMERA_INTERNAL_REQUEST_H__ > > +#include <vector> > > + > > #include <libcamera/request.h> > > +#include <libcamera/internal/fence.h> > > + > > namespace libcamera { > > class Camera; > > @@ -24,9 +28,26 @@ public: > > Camera *camera() const { return camera_; } > > + unsigned int pendingFences() const { return pendingFences_; } > > + unsigned int expiredFences() const { return expiredFences_; } > > + > > + void reuse(); > > + > > + std::vector<Fence> &fences() { return fences_; } > > + void addFence(Fence &&fence); > > + void clearFences(); > > + > > + void fenceExpired(); > > + void fenceCompleted(); > > + > > private: > > Camera *camera_; > > bool cancelled_; > > + > > + unsigned int pendingFences_ = 0; > > + unsigned int expiredFences_ = 0; > > + > > + std::vector<Fence> fences_; > > }; > > } /* namespace libcamera */ > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > index 33fee1ac05ba..e88eee1fac36 100644 > > --- a/src/libcamera/request.cpp > > +++ b/src/libcamera/request.cpp > > @@ -63,6 +63,92 @@ Request::Private::~Private() > > * request hasn't been queued > > */ > > +/** > > + * \fn Request::Private::pendingFences() > > + * \brief Retrieve the number of pending fences > > + * > > + * A Fence is pending if has not yet been signalled or it has not expired yet. > > + * > > + * \return The number of pending fences > > + */ > > + > > +/** > > + * \fn Request::Private::expiredFences() > > + * \brief Retrieve the number of expired fences > > + * \return The number of expired fences > > + */ > > + > > +/** > > + * \brief Reset the request for reuse > > + */ > > +void Request::Private::reuse() > > +{ > > + cancelled_ = false; > > + > > + fences_.clear(); > > + pendingFences_ = 0; > > + expiredFences_ = 0; > > +} > > + > > +/** > > + * \fn Request::Private::fences() > > + * \brief Retrieve a reference to the vector of pending fences > > + * \return A reference to the vector of pending fences > > + */ > > + > > +/** > > + * \brief Move a Fence into the Request > > + * > > + * Move a Fence into the Request and increase the pending fences > > + * counter. The Fence is now stored into the Request and the original > > + * one passed in as parameter is reset. > > + * > > + * Once the Fence completes, either because it is signalled or because > > + * it has expired, the caller shall notify the Request about this by > > + * calling fenceCompleted() or fenceExpired(); > > + */ > > +void Request::Private::addFence(Fence &&fence) > > +{ > > + fences_.push_back(std::move(fence)); > > Drive-by comment, please let me know if I am wrong: > > As per my understanding and your clarification, the fence is moved to a new > Fence and pushed into fences_. I believe this new Fence will be disabled > (by-default behaviour as stated in Fence class). So I don't think any of > fenceCompleted() or fenceExpired() getting called, which seems problematic. > No, that's by design :) Fences will be activated only when the request is queued to the camera. > > + pendingFences_++; > > +} > > + > > +/** > > + * \brief Release all Fences stored in the request > > + * > > + * Clear the vector of fences in the Request by releasing all of them. > > + * All Fences are closed and their file descriptors reset to 0. > > + */ > > +void Request::Private::clearFences() > > +{ > > + ASSERT(!pendingFences_); > > + fences_.clear(); > > +} > > + > > +/** > > + * \brief Notify that a Fence has been signalled > > + * > > + * Notify to the Request that a Fence has completed. This function decrease the > > + * number of pending Fences in the request. > > + */ > > +void Request::Private::fenceCompleted() > > +{ > > + pendingFences_--; > > +} > > + > > +/** > > + * \brief Notify that a Fence has expired > > + * > > + * Notify to the Request that a Fence has expired. This function decrease the > > + * number of pending Fences in the request and increase the number of expired > > + * ones. > > + */ > > +void Request::Private::fenceExpired() > > +{ > > + expiredFences_++; > > + pendingFences_--; > > +} > > + > > /** > > * \enum Request::Status > > * Request completion status > > @@ -158,10 +244,11 @@ void Request::reuse(ReuseFlag flags) > > sequence_ = 0; > > status_ = RequestPending; > > - _d()->cancelled_ = false; > > controls_->clear(); > > metadata_->clear(); > > + > > + _d()->reuse(); > > } > > /**
Quoting Jacopo Mondi (2021-11-09 17:34:46) > Hi Kieran, > > On Tue, Nov 09, 2021 at 01:23:32PM +0000, Kieran Bingham wrote: > > Quoting Jacopo Mondi (2021-10-28 12:15:15) > > > Prepare the Request::Private class to handle fences by providing a > > > storage vector and interface functions to handle the number of pending > > > and expired fences. > > > > > > The intended user of the interface is the PipelineHandler class > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > include/libcamera/internal/request.h | 21 +++++++ > > > src/libcamera/request.cpp | 89 +++++++++++++++++++++++++++- > > > 2 files changed, 109 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h > > > index df0cc014067e..2be4874756de 100644 > > > --- a/include/libcamera/internal/request.h > > > +++ b/include/libcamera/internal/request.h > > > @@ -7,8 +7,12 @@ > > > #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__ > > > #define __LIBCAMERA_INTERNAL_REQUEST_H__ > > > > > > +#include <vector> > > > + > > > #include <libcamera/request.h> > > > > > > +#include <libcamera/internal/fence.h> > > > + > > > namespace libcamera { > > > > > > class Camera; > > > @@ -24,9 +28,26 @@ public: > > > > > > Camera *camera() const { return camera_; } > > > > > > + unsigned int pendingFences() const { return pendingFences_; } > > > + unsigned int expiredFences() const { return expiredFences_; } > > > + > > > + void reuse(); > > > + > > > + std::vector<Fence> &fences() { return fences_; } > > > + void addFence(Fence &&fence); > > > + void clearFences(); > > > + > > > + void fenceExpired(); > > > + void fenceCompleted(); > > > + > > > private: > > > Camera *camera_; > > > bool cancelled_; > > > + > > > + unsigned int pendingFences_ = 0; > > > + unsigned int expiredFences_ = 0; > > > + > > > + std::vector<Fence> fences_; > > > }; > > > > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > > index 33fee1ac05ba..e88eee1fac36 100644 > > > --- a/src/libcamera/request.cpp > > > +++ b/src/libcamera/request.cpp > > > @@ -63,6 +63,92 @@ Request::Private::~Private() > > > * request hasn't been queued > > > */ > > > > > > +/** > > > + * \fn Request::Private::pendingFences() > > > + * \brief Retrieve the number of pending fences > > > + * > > > + * A Fence is pending if has not yet been signalled or it has not expired yet. > > > + * > > > + * \return The number of pending fences > > > + */ > > > + > > > +/** > > > + * \fn Request::Private::expiredFences() > > > + * \brief Retrieve the number of expired fences > > > + * \return The number of expired fences > > > + */ > > > + > > > +/** > > > + * \brief Reset the request for reuse > > > + */ > > > +void Request::Private::reuse() > > > +{ > > > + cancelled_ = false; > > > + > > > + fences_.clear(); > > > + pendingFences_ = 0; > > > + expiredFences_ = 0; > > > +} > > > + > > > +/** > > > + * \fn Request::Private::fences() > > > + * \brief Retrieve a reference to the vector of pending fences > > > + * \return A reference to the vector of pending fences > > > + */ > > > + > > > +/** > > > + * \brief Move a Fence into the Request > > > + * > > > + * Move a Fence into the Request and increase the pending fences > > > + * counter. The Fence is now stored into the Request and the original > > > + * one passed in as parameter is reset. > > > + * > > > + * Once the Fence completes, either because it is signalled or because > > > + * it has expired, the caller shall notify the Request about this by > > > + * calling fenceCompleted() or fenceExpired(); > > > + */ > > > +void Request::Private::addFence(Fence &&fence) > > > +{ > > > + fences_.push_back(std::move(fence)); > > > + pendingFences_++; > > > > Hrm ... I was expecting signals to be connected here ... but I guess > > that happens elsewhere... > > > > > > > +} > > > + > > > +/** > > > + * \brief Release all Fences stored in the request > > > + * > > > + * Clear the vector of fences in the Request by releasing all of them. > > > + * All Fences are closed and their file descriptors reset to 0. > > > + */ > > > +void Request::Private::clearFences() > > > +{ > > > + ASSERT(!pendingFences_); > > > + fences_.clear(); > > > +} > > > + > > > +/** > > > + * \brief Notify that a Fence has been signalled > > > + * > > > + * Notify to the Request that a Fence has completed. This function decrease the > > > + * number of pending Fences in the request. > > > + */ > > > +void Request::Private::fenceCompleted() > > > +{ > > > + pendingFences_--; > > > +} > > > + > > > +/** > > > + * \brief Notify that a Fence has expired > > > + * > > > + * Notify to the Request that a Fence has expired. This function decrease the > > > + * number of pending Fences in the request and increase the number of expired > > > + * ones. > > > + */ > > > +void Request::Private::fenceExpired() > > > +{ > > > + expiredFences_++; > > > + pendingFences_--; > > > > Are all these event driven counters thread safe? They may be, just seems > > a potential risk area... > > My understanding is that slots are synchronous unless otherwise > specified. > > As the Request::Private::fence*() functions are called from the > PipelineHandler::fenceCompleted() slot connected to Fence::complete > signal what happens is that > - Fence::activated() stops the timer > - Fence::timedout() disables the event notifier > > so a single Fence::complete is emitted per Fence. > > From there we call the PipelineHandler::fenceCompleted() slot which is > run synchronously, hence I think we're safe. > > Does this seems safe to you as well ? Can multiple Fences complete at the same time? or are they 'safe' because they would all be handled/monitored from the same event loop, thus only one can happen... I think it's fine. They all generate events from the EventNotifier, and I think I expect them to all be in the same thread.... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Thanks > j > > > > > > > > +} > > > + > > > /** > > > * \enum Request::Status > > > * Request completion status > > > @@ -158,10 +244,11 @@ void Request::reuse(ReuseFlag flags) > > > > > > sequence_ = 0; > > > status_ = RequestPending; > > > - _d()->cancelled_ = false; > > > > > > controls_->clear(); > > > metadata_->clear(); > > > + > > > + _d()->reuse(); > > > } > > > > > > /** > > > -- > > > 2.33.1 > > >
Hi Jacopo, On 11/10/21 2:58 PM, Jacopo Mondi wrote: > Hi Umang, > > On Wed, Nov 10, 2021 at 02:51:44PM +0530, Umang Jain wrote: >> Hi Jacopo, >> >> On 10/28/21 4:45 PM, Jacopo Mondi wrote: >>> Prepare the Request::Private class to handle fences by providing a >>> storage vector and interface functions to handle the number of pending >>> and expired fences. >>> >>> The intended user of the interface is the PipelineHandler class >>> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>> --- >>> include/libcamera/internal/request.h | 21 +++++++ >>> src/libcamera/request.cpp | 89 +++++++++++++++++++++++++++- >>> 2 files changed, 109 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h >>> index df0cc014067e..2be4874756de 100644 >>> --- a/include/libcamera/internal/request.h >>> +++ b/include/libcamera/internal/request.h >>> @@ -7,8 +7,12 @@ >>> #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__ >>> #define __LIBCAMERA_INTERNAL_REQUEST_H__ >>> +#include <vector> >>> + >>> #include <libcamera/request.h> >>> +#include <libcamera/internal/fence.h> >>> + >>> namespace libcamera { >>> class Camera; >>> @@ -24,9 +28,26 @@ public: >>> Camera *camera() const { return camera_; } >>> + unsigned int pendingFences() const { return pendingFences_; } >>> + unsigned int expiredFences() const { return expiredFences_; } >>> + >>> + void reuse(); >>> + >>> + std::vector<Fence> &fences() { return fences_; } >>> + void addFence(Fence &&fence); >>> + void clearFences(); >>> + >>> + void fenceExpired(); >>> + void fenceCompleted(); >>> + >>> private: >>> Camera *camera_; >>> bool cancelled_; >>> + >>> + unsigned int pendingFences_ = 0; >>> + unsigned int expiredFences_ = 0; >>> + >>> + std::vector<Fence> fences_; >>> }; >>> } /* namespace libcamera */ >>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp >>> index 33fee1ac05ba..e88eee1fac36 100644 >>> --- a/src/libcamera/request.cpp >>> +++ b/src/libcamera/request.cpp >>> @@ -63,6 +63,92 @@ Request::Private::~Private() >>> * request hasn't been queued >>> */ >>> +/** >>> + * \fn Request::Private::pendingFences() >>> + * \brief Retrieve the number of pending fences >>> + * >>> + * A Fence is pending if has not yet been signalled or it has not expired yet. >>> + * >>> + * \return The number of pending fences >>> + */ >>> + >>> +/** >>> + * \fn Request::Private::expiredFences() >>> + * \brief Retrieve the number of expired fences >>> + * \return The number of expired fences >>> + */ >>> + >>> +/** >>> + * \brief Reset the request for reuse >>> + */ >>> +void Request::Private::reuse() >>> +{ >>> + cancelled_ = false; >>> + >>> + fences_.clear(); >>> + pendingFences_ = 0; >>> + expiredFences_ = 0; >>> +} >>> + >>> +/** >>> + * \fn Request::Private::fences() >>> + * \brief Retrieve a reference to the vector of pending fences >>> + * \return A reference to the vector of pending fences >>> + */ >>> + >>> +/** >>> + * \brief Move a Fence into the Request >>> + * >>> + * Move a Fence into the Request and increase the pending fences >>> + * counter. The Fence is now stored into the Request and the original >>> + * one passed in as parameter is reset. >>> + * >>> + * Once the Fence completes, either because it is signalled or because >>> + * it has expired, the caller shall notify the Request about this by >>> + * calling fenceCompleted() or fenceExpired(); >>> + */ >>> +void Request::Private::addFence(Fence &&fence) >>> +{ >>> + fences_.push_back(std::move(fence)); >> Drive-by comment, please let me know if I am wrong: >> >> As per my understanding and your clarification, the fence is moved to a new >> Fence and pushed into fences_. I believe this new Fence will be disabled >> (by-default behaviour as stated in Fence class). So I don't think any of >> fenceCompleted() or fenceExpired() getting called, which seems problematic. >> > No, that's by design :) > > Fences will be activated only when the request is queued to the camera. Ok, I see in the below patch that you enable one-by-one. I think I got confused by your reply and the statement point from the cover letter : ``` - Fences are attacched to a FrameBuffer and their value is valid until the Request is queued to the Camera ``` > >>> + pendingFences_++; >>> +} >>> + >>> +/** >>> + * \brief Release all Fences stored in the request >>> + * >>> + * Clear the vector of fences in the Request by releasing all of them. >>> + * All Fences are closed and their file descriptors reset to 0. >>> + */ >>> +void Request::Private::clearFences() >>> +{ >>> + ASSERT(!pendingFences_); >>> + fences_.clear(); >>> +} >>> + >>> +/** >>> + * \brief Notify that a Fence has been signalled >>> + * >>> + * Notify to the Request that a Fence has completed. This function decrease the >>> + * number of pending Fences in the request. >>> + */ >>> +void Request::Private::fenceCompleted() >>> +{ >>> + pendingFences_--; >>> +} >>> + >>> +/** >>> + * \brief Notify that a Fence has expired >>> + * >>> + * Notify to the Request that a Fence has expired. This function decrease the >>> + * number of pending Fences in the request and increase the number of expired >>> + * ones. >>> + */ >>> +void Request::Private::fenceExpired() >>> +{ >>> + expiredFences_++; >>> + pendingFences_--; >>> +} >>> + >>> /** >>> * \enum Request::Status >>> * Request completion status >>> @@ -158,10 +244,11 @@ void Request::reuse(ReuseFlag flags) >>> sequence_ = 0; >>> status_ = RequestPending; >>> - _d()->cancelled_ = false; >>> controls_->clear(); >>> metadata_->clear(); >>> + >>> + _d()->reuse(); >>> } >>> /**
Hello, On Wed, Nov 10, 2021 at 10:57:26AM +0000, Kieran Bingham wrote: > Quoting Jacopo Mondi (2021-11-09 17:34:46) > > On Tue, Nov 09, 2021 at 01:23:32PM +0000, Kieran Bingham wrote: > > > Quoting Jacopo Mondi (2021-10-28 12:15:15) > > > > Prepare the Request::Private class to handle fences by providing a > > > > storage vector and interface functions to handle the number of pending > > > > and expired fences. > > > > > > > > The intended user of the interface is the PipelineHandler class > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > include/libcamera/internal/request.h | 21 +++++++ > > > > src/libcamera/request.cpp | 89 +++++++++++++++++++++++++++- > > > > 2 files changed, 109 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h > > > > index df0cc014067e..2be4874756de 100644 > > > > --- a/include/libcamera/internal/request.h > > > > +++ b/include/libcamera/internal/request.h > > > > @@ -7,8 +7,12 @@ > > > > #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__ > > > > #define __LIBCAMERA_INTERNAL_REQUEST_H__ > > > > > > > > +#include <vector> > > > > + > > > > #include <libcamera/request.h> > > > > > > > > +#include <libcamera/internal/fence.h> > > > > + > > > > namespace libcamera { > > > > > > > > class Camera; > > > > @@ -24,9 +28,26 @@ public: > > > > > > > > Camera *camera() const { return camera_; } > > > > > > > > + unsigned int pendingFences() const { return pendingFences_; } > > > > + unsigned int expiredFences() const { return expiredFences_; } > > > > + > > > > + void reuse(); > > > > + > > > > + std::vector<Fence> &fences() { return fences_; } > > > > + void addFence(Fence &&fence); > > > > + void clearFences(); > > > > + > > > > + void fenceExpired(); > > > > + void fenceCompleted(); > > > > + > > > > private: > > > > Camera *camera_; > > > > bool cancelled_; > > > > + > > > > + unsigned int pendingFences_ = 0; > > > > + unsigned int expiredFences_ = 0; By moving timeout handling to the request or pipeline handler, you could drop the expiredFences_ counter and checked the pendingFences_ counter when the global timer expires. > > > > + > > > > + std::vector<Fence> fences_; Do we need to move the fences to the request ? It seems that fences_ isn't used internally, and fences() is only used in PipelineHandler::queueRequest() where we could have a vector of Fence pointers instead as a local variable (or even enable the fences in the first loop in that function). That would simplify the API. > > > > }; > > > > > > > > } /* namespace libcamera */ > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > > > index 33fee1ac05ba..e88eee1fac36 100644 > > > > --- a/src/libcamera/request.cpp > > > > +++ b/src/libcamera/request.cpp > > > > @@ -63,6 +63,92 @@ Request::Private::~Private() > > > > * request hasn't been queued > > > > */ > > > > > > > > +/** > > > > + * \fn Request::Private::pendingFences() > > > > + * \brief Retrieve the number of pending fences > > > > + * > > > > + * A Fence is pending if has not yet been signalled or it has not expired yet. > > > > + * > > > > + * \return The number of pending fences > > > > + */ > > > > + > > > > +/** > > > > + * \fn Request::Private::expiredFences() > > > > + * \brief Retrieve the number of expired fences > > > > + * \return The number of expired fences > > > > + */ > > > > + > > > > +/** > > > > + * \brief Reset the request for reuse > > > > + */ > > > > +void Request::Private::reuse() > > > > +{ > > > > + cancelled_ = false; > > > > + > > > > + fences_.clear(); > > > > + pendingFences_ = 0; > > > > + expiredFences_ = 0; > > > > +} > > > > + > > > > +/** > > > > + * \fn Request::Private::fences() > > > > + * \brief Retrieve a reference to the vector of pending fences > > > > + * \return A reference to the vector of pending fences > > > > + */ > > > > + > > > > +/** > > > > + * \brief Move a Fence into the Request > > > > + * > > > > + * Move a Fence into the Request and increase the pending fences > > > > + * counter. The Fence is now stored into the Request and the original > > > > + * one passed in as parameter is reset. > > > > + * > > > > + * Once the Fence completes, either because it is signalled or because > > > > + * it has expired, the caller shall notify the Request about this by > > > > + * calling fenceCompleted() or fenceExpired(); > > > > + */ > > > > +void Request::Private::addFence(Fence &&fence) > > > > +{ > > > > + fences_.push_back(std::move(fence)); > > > > + pendingFences_++; > > > > > > Hrm ... I was expecting signals to be connected here ... but I guess > > > that happens elsewhere... > > > > > > > +} > > > > + > > > > +/** > > > > + * \brief Release all Fences stored in the request > > > > + * > > > > + * Clear the vector of fences in the Request by releasing all of them. > > > > + * All Fences are closed and their file descriptors reset to 0. > > > > + */ > > > > +void Request::Private::clearFences() > > > > +{ > > > > + ASSERT(!pendingFences_); > > > > + fences_.clear(); > > > > +} > > > > + > > > > +/** > > > > + * \brief Notify that a Fence has been signalled > > > > + * > > > > + * Notify to the Request that a Fence has completed. This function decrease the > > > > + * number of pending Fences in the request. > > > > + */ > > > > +void Request::Private::fenceCompleted() > > > > +{ > > > > + pendingFences_--; > > > > +} > > > > + > > > > +/** > > > > + * \brief Notify that a Fence has expired > > > > + * > > > > + * Notify to the Request that a Fence has expired. This function decrease the > > > > + * number of pending Fences in the request and increase the number of expired > > > > + * ones. > > > > + */ > > > > +void Request::Private::fenceExpired() > > > > +{ > > > > + expiredFences_++; > > > > + pendingFences_--; > > > > > > Are all these event driven counters thread safe? They may be, just seems > > > a potential risk area... > > > > My understanding is that slots are synchronous unless otherwise > > specified. > > > > As the Request::Private::fence*() functions are called from the > > PipelineHandler::fenceCompleted() slot connected to Fence::complete > > signal what happens is that > > - Fence::activated() stops the timer > > - Fence::timedout() disables the event notifier > > > > so a single Fence::complete is emitted per Fence. > > > > From there we call the PipelineHandler::fenceCompleted() slot which is > > run synchronously, hence I think we're safe. > > > > Does this seems safe to you as well ? > > Can multiple Fences complete at the same time? or are they 'safe' > because they would all be handled/monitored from the same event loop, > thus only one can happen... Everything is running in a single thread on the pipeline handler side, with a single event loop, so it should be safe (there's a single thread covering all pipeline handlers at the moment, which may be turned into a per-pipeline thread in the future, but that won't make any difference here). > I think it's fine. They all generate events from the EventNotifier, and > I think I expect them to all be in the same thread.... > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > +} > > > > + > > > > /** > > > > * \enum Request::Status > > > > * Request completion status > > > > @@ -158,10 +244,11 @@ void Request::reuse(ReuseFlag flags) > > > > > > > > sequence_ = 0; > > > > status_ = RequestPending; > > > > - _d()->cancelled_ = false; > > > > > > > > controls_->clear(); > > > > metadata_->clear(); > > > > + > > > > + _d()->reuse(); > > > > } > > > > > > > > /**
diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h index df0cc014067e..2be4874756de 100644 --- a/include/libcamera/internal/request.h +++ b/include/libcamera/internal/request.h @@ -7,8 +7,12 @@ #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__ #define __LIBCAMERA_INTERNAL_REQUEST_H__ +#include <vector> + #include <libcamera/request.h> +#include <libcamera/internal/fence.h> + namespace libcamera { class Camera; @@ -24,9 +28,26 @@ public: Camera *camera() const { return camera_; } + unsigned int pendingFences() const { return pendingFences_; } + unsigned int expiredFences() const { return expiredFences_; } + + void reuse(); + + std::vector<Fence> &fences() { return fences_; } + void addFence(Fence &&fence); + void clearFences(); + + void fenceExpired(); + void fenceCompleted(); + private: Camera *camera_; bool cancelled_; + + unsigned int pendingFences_ = 0; + unsigned int expiredFences_ = 0; + + std::vector<Fence> fences_; }; } /* namespace libcamera */ diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index 33fee1ac05ba..e88eee1fac36 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -63,6 +63,92 @@ Request::Private::~Private() * request hasn't been queued */ +/** + * \fn Request::Private::pendingFences() + * \brief Retrieve the number of pending fences + * + * A Fence is pending if has not yet been signalled or it has not expired yet. + * + * \return The number of pending fences + */ + +/** + * \fn Request::Private::expiredFences() + * \brief Retrieve the number of expired fences + * \return The number of expired fences + */ + +/** + * \brief Reset the request for reuse + */ +void Request::Private::reuse() +{ + cancelled_ = false; + + fences_.clear(); + pendingFences_ = 0; + expiredFences_ = 0; +} + +/** + * \fn Request::Private::fences() + * \brief Retrieve a reference to the vector of pending fences + * \return A reference to the vector of pending fences + */ + +/** + * \brief Move a Fence into the Request + * + * Move a Fence into the Request and increase the pending fences + * counter. The Fence is now stored into the Request and the original + * one passed in as parameter is reset. + * + * Once the Fence completes, either because it is signalled or because + * it has expired, the caller shall notify the Request about this by + * calling fenceCompleted() or fenceExpired(); + */ +void Request::Private::addFence(Fence &&fence) +{ + fences_.push_back(std::move(fence)); + pendingFences_++; +} + +/** + * \brief Release all Fences stored in the request + * + * Clear the vector of fences in the Request by releasing all of them. + * All Fences are closed and their file descriptors reset to 0. + */ +void Request::Private::clearFences() +{ + ASSERT(!pendingFences_); + fences_.clear(); +} + +/** + * \brief Notify that a Fence has been signalled + * + * Notify to the Request that a Fence has completed. This function decrease the + * number of pending Fences in the request. + */ +void Request::Private::fenceCompleted() +{ + pendingFences_--; +} + +/** + * \brief Notify that a Fence has expired + * + * Notify to the Request that a Fence has expired. This function decrease the + * number of pending Fences in the request and increase the number of expired + * ones. + */ +void Request::Private::fenceExpired() +{ + expiredFences_++; + pendingFences_--; +} + /** * \enum Request::Status * Request completion status @@ -158,10 +244,11 @@ void Request::reuse(ReuseFlag flags) sequence_ = 0; status_ = RequestPending; - _d()->cancelled_ = false; controls_->clear(); metadata_->clear(); + + _d()->reuse(); } /**
Prepare the Request::Private class to handle fences by providing a storage vector and interface functions to handle the number of pending and expired fences. The intended user of the interface is the PipelineHandler class Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/internal/request.h | 21 +++++++ src/libcamera/request.cpp | 89 +++++++++++++++++++++++++++- 2 files changed, 109 insertions(+), 1 deletion(-)