[libcamera-devel] [PATCH v5 08/11] libcamera: request: Add Request::Private::prepare()

[Jacopo Mondi]

Hi Laurent

On Fri, Dec 10, 2021 at 11:27:32PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Dec 10, 2021 at 09:52:36PM +0100, Jacopo Mondi wrote:
> > Add a prepare() function to the Private Request representation.
> >
> > The prepare() function is used by the PipelineHandler class to
> > prepare a Request to be queued to the hardware.
> >
> > The current implementation of prepare() handles the fences associated
> > with the Framebuffers part of a Request. The function starts an event
> > notifier for each of those and emits the Request::prepared signal when
> > all fences have been signalled or an optional timeout has expired.
> >
> > The optional timeout allows to interrupt blocked waits and notify the
> > Request as failed so that it can be cancelled.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/internal/request.h |  16 ++++
> >  src/libcamera/request.cpp            | 133 +++++++++++++++++++++++++++
> >  2 files changed, 149 insertions(+)
> >
> > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
> > index 1340ffa2a683..1f2499896e23 100644
> > --- a/include/libcamera/internal/request.h
> > +++ b/include/libcamera/internal/request.h
> > @@ -7,10 +7,17 @@
> >  #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__
> >  #define __LIBCAMERA_INTERNAL_REQUEST_H__
> >
> > +#include <chrono>
> > +#include <map>
> >  #include <memory>
> >
> > +#include <libcamera/base/event_notifier.h>
> > +#include <libcamera/base/timer.h>
> > +
> >  #include <libcamera/request.h>
> >
> > +using namespace std::chrono_literals;
> > +
> >  namespace libcamera {
> >
> >  class Camera;
> > @@ -32,16 +39,25 @@ public:
> >  	void cancel();
> >  	void reuse();
> >
> > +	void prepare(std::chrono::milliseconds timeout = 0ms);
> > +	Signal<> prepared;
> > +
> >  private:
> >  	friend class PipelineHandler;
> >
> >  	void doCancelRequest();
> > +	void emitPrepareCompleted();
> > +	void notifierActivated(FrameBuffer *buffer);
> > +	void timeout();
> >
> >  	Camera *camera_;
> >  	bool cancelled_;
> >  	uint32_t sequence_ = 0;
> > +	bool prepared_ = false;
> >
> >  	std::unordered_set<FrameBuffer *> pending_;
> > +	std::map<FrameBuffer *, std::unique_ptr<EventNotifier>> notifiers_;
> > +	std::unique_ptr<Timer> timer_;
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index 016db9d62340..7e9238dc1eb4 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -135,6 +135,8 @@ void Request::Private::doCancelRequest()
> >
> >  	cancelled_ = true;
> >  	pending_.clear();
> > +	notifiers_.clear();
> > +	timer_.reset();
> >  }
> >
> >  /**
> > @@ -162,7 +164,138 @@ void Request::Private::reuse()
> >  {
> >  	sequence_ = 0;
> >  	cancelled_ = false;
> > +	prepared_ = false;
> >  	pending_.clear();
> > +	notifiers_.clear();
> > +	timer_.reset();
> > +}
> > +
> > +/*
> > + * Helper function to save some lines of code and make sure prepared_ is set
> > + * to true before emitting the signal.
> > + */
> > +void Request::Private::emitPrepareCompleted()
> > +{
> > +	prepared_ = true;
> > +	prepared.emit();
> > +}
> > +
> > +/**
> > + * \brief Prepare the Request to be queued to the device
> > + * \param[in] timeout Optional expiration timeout
> > + *
> > + * Prepare a Request to be queued to the hardware device by ensuring it is
> > + * ready for the incoming memory transfers.
> > + *
> > + * This currently means waiting on each frame buffer acquire fence to be
> > + * signalled. An optional expiration timeout can be specified. If not all the
> > + * fences have been signalled correctly before the timeout expires the Request
> > + * is cancelled.
> > + *
> > + * The function immediately emits the prepared signal if all the prepare
> > + * operations have been completed synchronously. If instead the prepare
> > + * operations require to wait the completion of asynchronous events, such as
> > + * fences notifications or timer expiration, the prepared signal is emitted upon
> > + * the asynchronous event completion.
> > + *
> > + * As we currently only handle fences, the function emits the prepared signal
> > + * immediately if there are no fences to wait on. Otherwise the prepared signal
> > + * is emitted when all fences have been signalled or the optional timeout has
> > + * expired.
> > + *
> > + * If not all the fences have been correctly signalled or the optional timeout
> > + * has expired the Request will be cancelled and the Request::prepared signal
> > + * emitted.
> > + *
> > + * The intended user of this function is the PipelineHandler base class, which
> > + * 'prepares' a Request before queuing it to the hardware device.
> > + */
> > +void Request::Private::prepare(std::chrono::milliseconds timeout)
> > +{
> > +	/* Create and connect one notifier for each synchronization fence. */
> > +	for (FrameBuffer *buffer : pending_) {
> > +		const Fence *fence = buffer->_d()->fence();
> > +		if (!fence)
> > +			continue;
> > +
> > +		std::unique_ptr<EventNotifier> notifier =
> > +			std::make_unique<EventNotifier>(fence->fd().get(),
> > +							EventNotifier::Read);
> > +
> > +		notifier->activated.connect(this, [this, buffer] {
> > +							notifierActivated(buffer);
> > +					    });
> > +
> > +		notifiers_[buffer] = std::move(notifier);
> > +	}
> > +
> > +	if (notifiers_.empty()) {
> > +		emitPrepareCompleted();
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * In case a timeout is specified, create a timer and set it up.
> > +	 *
> > +	 * The timer must be created here instead of in the Request constructor,
> > +	 * in order to be bound to the pipeline handler thread.
> > +	 */
> > +	if (timeout == 0ms) {
>
> This was
>
> 	if (timeout != 0ms) {
>
> in the previous version.
>
> 	if (timeout) {
>
> would work too, but
>
> 	if (timeout == 0ms) {
>
> is probably not what you want.
>
> By the way, a timer with a 0ms timeout is a simple way to get a function
> called as soon as control goes back to the event loop, if you ever find
> yourself in the need of that.

Ouch, again weird that it doesn't fail.
Anyway, no, if (timeout) doesn't work

../src/libcamera/request.cpp:243:13: error: could not convert ‘timeout’ from ‘std::chrono::milliseconds’ {aka ‘std::chrono::duration<long int, std::ratio<1, 1000> >’} to ‘bool

>
> > +		timer_ = std::make_unique<Timer>();
> > +		timer_->timeout.connect(this, &Request::Private::timeout);
> > +		timer_->start(timeout);
> > +	}
> > +}
> > +
> > +/**
> > + * \var Request::Private::prepared
> > + * \brief Request preparation completed Signal
> > + *
> > + * The signal is emitted once the request preparation has completed and is ready
> > + * to be queued. The Request might complete with errors in which case it is
> > + * cancelled.
> > + *
> > + * The intended slot for this signal is the PipelineHandler::doQueueRequests()
> > + * function which queues Request after they have been prepared or cancel them
> > + * if they have failed preparing.
> > + */
> > +
> > +void Request::Private::notifierActivated(FrameBuffer *buffer)
> > +{
> > +	/* Close the fence if successfully signalled. */
> > +	ASSERT(buffer);
> > +	buffer->releaseFence();
> > +
> > +	/* Remove the entry from the map and check if other fences are pending. */
> > +	auto it = notifiers_.find(buffer);
> > +	ASSERT(it != notifiers_.end());
> > +	notifiers_.erase(it);
> > +
> > +	Request *request = _o<Request>();
> > +	LOG(Request, Debug)
> > +		<< "Request " << request->cookie() << " buffer " << buffer
> > +		<< " fence signalled";
> > +
> > +	if (!notifiers_.empty())
> > +		return;
> > +
> > +	/* All fences completed, delete the timer and move to state Ready. */
> > +	timer_.reset();
> > +	emitPrepareCompleted();
> > +}
> > +
> > +void Request::Private::timeout()
> > +{
> > +	/* A timeout can only happen if there are fences not yet signalled. */
> > +	ASSERT(!notifiers_.empty());
> > +	notifiers_.clear();
> > +
> > +	Request *request = _o<Request>();
> > +	LOG(Request, Debug) << "Request prepare timeout: " << request->cookie();
> > +
> > +	cancel();
> > +
> > +	emitPrepareCompleted();
> >  }
> >
> >  /**
>
> --
> Regards,
>
> Laurent Pinchart