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

[Jacopo Mondi]

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 ?

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