[libcamera-devel] [PATCH 03/10] libcamera: Introduce Fence class

[Kieran Bingham]

Quoting Jacopo Mondi (2021-11-09 17:24:42)
> Hi Kieran
> 
> On Tue, Nov 09, 2021 at 01:39:58PM +0000, Kieran Bingham wrote:
> > Quoting Kieran Bingham (2021-11-09 13:00:43)
> > > Quoting Jacopo Mondi (2021-10-28 12:15:13)
> 
> [snip]
> 
> > > > +void Fence::moveFence(Fence &other)
> > >
> > > I see now that the text above really did mean moving a fence invalidates
> > > it, but I'm not sure how that use case comes about.
> > >
> > > Could this funtion perhaps document /why/ moving a fence necessitates
> > > invalidating it, and why it moves the internal fd to preserve it's
> > > lifetime - but - that the fence can no longer be used... (or something
> > > like that if I've inferred correctly).
> > >
> > >
> > > > +{
> > > > +       fd_ = std::move(other.fd_);
> > > > +
> > >
> > > There might be code connected to the complete signal of the 'other'
> > > fence. Should they be moved over to this? or should we explictly state
> > > that listeners to that completion event are 'disconnected' (somewhat
> > > implicitly).
> > >
> > > Perhaps we should explictily either disconnect or move them in the
> > > code...?
> > >
> > > > +       other.disable();
> > > > +       if (other.timer_.isRunning())
> > > > +               other.timer_.stop();
> >
> > also - in this case here, the other timer is still running, so there is
> > perhaps expected to be someone listing waiting for either a completed
> > fence notification or a timeout. Does calling .stop() - activate the
> > signal to signal a timeout? If not - perhaps we should explicitly /
> > manually do so, and generate a timeout on it?
> >
> 
> afaict Timer::stop() does not generate a timeout and I'm not sure
> generating an event on the moved Fence is the right thing to do. How
> is it the slot supposed to know if a real timeout has occurred ?

I'm weary that I may be into hypotheticals of situations that won't
occur, but I'm trying to look at it from how the class is designed and
therefore /could/ be used.

My concern is that you are indicating in the code that we are moving a
Fence which has a /running/ timer set on it.

We are 'sweeping' the rug out from under the listener, whomever that may
be. They are expecting either a fence completion event, or a timeout -
but the move operation is cancelling both of those (and we're about to
disconnect the signal entirely too I expect).

So I think the listener should be told there is a timeout at least.

If this is unexpected behaviour, and moving a Fence which has an active
running timer shouldn't happen - then we either need to ASSERT() or
LOG(Error) on it here in Fence::moveFence() and make sure that expected
behaviour is documented.


> > > Should you move the timeout_ value over to this one too?
> > >
> > > > +       other.timeout_ = 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Move-construct a Fence
> > > > + * \param[in] other The other fence to move
> > >
> > > Ok things are becoming clearer. Perhaps:
> > >
> > >  *
> > >  * The other fence is disabled, and invalidated while the underlying
> > >  * FileDescriptor is moved to this Fence. The newly moved fence can be
> > >  * re-used when required.
> > >
> > > > + */
> > > > +Fence::Fence(Fence &&other)
> > > > +       : notifier_(other.fd(), EventNotifier::Read, nullptr, false)
> > > > +{
> > > > +       moveFence(other);
> > > > +
> > > > +       notifier_.activated.connect(this, &Fence::activated);
> > > > +       timer_.timeout.connect(this, &Fence::timedout);
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Move-assign the value of the \a other fence to this
> > > > + * \param[in] other The other fence to move
> > > > + * \return This fence
> > >
> > > Something similar to the above addition maybe...
> > >
> > >
> > > > + */
> > > > +Fence &Fence::operator=(Fence &&other)
> > > > +{
> > > > +       moveFence(other);
> > > > +
> > > > +       notifier_ = EventNotifier(fd_.fd(), EventNotifier::Read, nullptr, false);
> > > > +
> > > > +       return *this;
> > > > +}
> > > > +
> > > > +/**
> > > > + * \fn Fence::fd() const
> > > > + * \brief Return the synchronization fence file descriptor
> > > > + * \return The synchronization fence file descriptor
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn Fence::timeout()
> > > > + * \brief Retrieve the fence timeout
> > > > + * \return The fence timeout in milliseconds
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn Fence::completed()
> > > > + * \brief Check if the fence has completed before timing out
> > > > + * \return True if the fence has completed
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn Fence::expired()
> > > > + * \brief Check if the fence has expired before completing
> > > > + * \return True if the fence has expired
> > > > + */
> > > > +
> > > > +/**
> > > > + * \brief Enable a fence by enabling events notifications
> > > > + */
> > > > +void Fence::enable()
> > > > +{
> > > > +       MutexLocker lock(mutex_);
> > > > +
> > > > +       expired_ = false;
> > > > +       completed_ = false;
> > > > +
> > > > +       notifier_.setEnabled(true);
> > > > +       if (timeout_)
> > > > +               timer_.start(timeout_);
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Disable a fence by disabling events notifications
> > > > + */
> > > > +void Fence::disable()
> > > > +{
> > > > +       notifier_.setEnabled(false);
> > > > +}
> > > > +
> > > > +/**
> > > > + * \var Fence::complete
> > > > + * \brief The fence completion signal
> > > > + *
> > > > + * A Fence is completed if signalled or timeout.
> > >
> > > A Fence is completed when signalled or if a timeout occurs.
> > >
> > > > + */
> > > > +
> > > > +void Fence::activated()
> > > > +{
> > > > +       MutexLocker lock(mutex_);
> > > > +
> > > > +       if (timer_.isRunning())
> > > > +               timer_.stop();
> > > > +
> > > > +       completed_ = true;
> > > > +       expired_ = false;
> > > > +
> > > > +       complete.emit();
> > >
> > > Passing the expiration state as a boolean here might help the receiver
> > > know why it completed.
> > >
> > > But maybe that's optional - or better handled as you have it anyway.
> > >
> > > > +}
> > > > +
> > > > +void Fence::timedout()
> > > > +{
> > > > +       MutexLocker lock(mutex_);
> > > > +
> > > > +       expired_ = true;
> > > > +       completed_ = false;
> > > > +
> > > > +       /* Disable events notification after timeout. */
> > > > +       disable();
> > > > +
> > > > +       complete.emit();
> > > > +}
> > > > +
> > > > +} /* namespace libcamera */
> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > index 6727a777d804..6fb0d5f49b63 100644
> > > > --- a/src/libcamera/meson.build
> > > > +++ b/src/libcamera/meson.build
> > > > @@ -14,6 +14,7 @@ libcamera_sources = files([
> > > >      'delayed_controls.cpp',
> > > >      'device_enumerator.cpp',
> > > >      'device_enumerator_sysfs.cpp',
> > > > +    'fence.cpp',
> > > >      'file_descriptor.cpp',
> > > >      'formats.cpp',
> > > >      'framebuffer.cpp',
> > > > --
> > > > 2.33.1
> > > >