[libcamera-devel] [PATCH 05/10] libcamera: request: Add support for fences

[Laurent Pinchart]

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 at 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 at 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();
> > > >  }
> > > >
> > > >  /**

-- 
Regards,

Laurent Pinchart