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

[Jacopo Mondi]

Hi Laurent

On Sun, Nov 21, 2021 at 11:39:22PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Sat, Nov 20, 2021 at 12:13:09PM +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 notifies the Request as Ready to be queued
> > once all the fences have been signalled.
> >
> > An optional timeout allows to interrupt blocked waits and notify the
> > Request as failed.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/internal/request.h |  21 ++++
> >  src/libcamera/request.cpp            | 150 +++++++++++++++++++++++++++
> >  2 files changed, 171 insertions(+)
> >
> > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
> > index 59bddde3a090..26b25fb12261 100644
> > --- a/include/libcamera/internal/request.h
> > +++ b/include/libcamera/internal/request.h
> > @@ -7,10 +7,16 @@
> >  #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__
> >  #define __LIBCAMERA_INTERNAL_REQUEST_H__
> >
> > +#include <chrono>
> >  #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;
> > @@ -21,6 +27,12 @@ class Request::Private : public Extensible::Private
> >  	LIBCAMERA_DECLARE_PUBLIC(Request)
> >
> >  public:
> > +	enum class Status {
> > +		Pending,
> > +		Ready,
> > +		Failed
> > +	};
> > +
> >  	Private(Camera *camera);
> >  	~Private();
> >
> > @@ -29,21 +41,30 @@ public:
> >
> >  	uint64_t cookie() const;
> >  	Request::Status status() const;
> > +	Status privateStatus() const { return status_; }
>
> As mentioned in the cover letter, status and privateStatus isn't nice
> indeed. I think one option to remove the privateStatus() could be to
> call cancel() in Request::Private::timeout(), and then use
> Request::status() in PipelineHandler::doQueueRequests(). There could be
> other options too.

I didn't do so as I thought it could have caused out-of-order
completions but I was probably wrong as just buffes are completed in
error state by Request::cancel() not the request!

Anyway, I need somehow to keep the state of when a request has been
prepared (ie or fences signalled or timeout) to pass it to
queueRequestDevice.

Probably a single boolean prepared_ flag in Private would do but I
really dislike flags like this as keeping track of them is hell when
something goes wrong..

>
> >
> >  	bool completeBuffer(FrameBuffer *buffer);
> >  	void complete();
> >  	void cancel();
> >  	void reuse();
> >
> > +	Status prepare(std::chrono::milliseconds timeout = 0ms);
> > +	Signal<> prepared;
> > +
> >  	uint32_t sequence_ = 0;
> >
> >  private:
> >  	void _cancel();
> > +	void notifierActivated(const std::unique_ptr<EventNotifier> &notifier);
> > +	void timeout();
> >
> >  	Camera *camera_;
> >  	bool cancelled_;
> > +	Status status_ = Status::Pending;
> >
> >  	std::unordered_set<FrameBuffer *> pending_;
> > +	std::unordered_set<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 1d47698a6263..98f9719e5cf2 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -43,6 +43,22 @@ LOG_DEFINE_CATEGORY(Request)
> >   * subclasses).
> >   */
> >
> > +/**
> > + * \enum Request::Private::Status
> > + * \brief Request private status
> > + *
> > + * The Request private status describes the lifecycle of the Request between
> > + * the time it is queued to the Camera and the time it is queued to the device.
> > + *
> > + * A Request is created in Status::Pending state. Before actually queueing the
> > + * Request the PipelineHandler base class prepare() the Request, whose status
> > + * transitions to either Status::Ready or Status::Failed.
> > + *
> > + * \var Request::Private::Status::Pending
> > + * \var Request::Private::Status::Ready
> > + * \var Request::Private::Status::Failed
> > + */
> > +
> >  /**
> >   * \brief Create a Request::Private
> >   * \param camera The Camera that creates the request
> > @@ -95,6 +111,17 @@ Request::Status Request::Private::status() const
> >  	return _o<Request>()->status();
> >  }
> >
> > +/**
> > + * \fn Request::Private::privateStatus()
> > + * \brief Retrieve the Request private status
> > + *
> > + * The private status, as described by the Request::Private:Status enumeration,
> > + * describes the Request status between the time it is queued to the Camera and
> > + * the time the Request is applied to the hardware.
> > + *
> > + * \return The Request private state
> > + */
> > +
> >  /**
> >   * \brief Complete a buffer for the request
> >   * \param[in] buffer The buffer that has completed
> > @@ -155,6 +182,8 @@ void Request::Private::_cancel()
> >
> >  	cancelled_ = true;
> >  	pending_.clear();
> > +	notifiers_.clear();
> > +	timer_.reset();
> >  }
> >
> >  /**
> > @@ -182,8 +211,84 @@ void Request::Private::reuse()
> >  {
> >  	sequence_ = 0;
> >  	cancelled_ = false;
> > +	status_ = Status::Pending;
> >  	pending_.clear();
> > +	notifiers_.clear();
> > +	timer_.reset();
> > +}
> > +
> > +/**
> > + * \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 it by ensuring it is
>
> s/it by/by/
>
> > + * 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 marked as Failed, otherwise it is set to the Ready state.
> > + *
> > + * \sa Request::Private::Status
> > + *
> > + * The function returns Status::Ready if all the prepare operations have been
> > + * completed synchronously. If Status::Ready is returned the Request can be
> > + * queued immediately and the prepared signal is not emitted. If instead the
> > + * prepare operation requires to wait the completion of asynchronous events,
> > + * such as fence notifications or timer expiration this function returns
> > + * Status::Pending and the asynchronous event completion is notified by emitting
> > + * the prepared signal.
> > + *
> > + * As we currently only handle fences, the function return Status::Ready if
> > + * there are no fences to wait on. Status::Prepared is otherwise returned and
> > + * the prepared signal is emitted when all fences have been signalled or the
> > + * optional timeout has expired.
> > + *
> > + * The intended user of this function is the PipelineHandler base class, which
> > + * 'prepares' a Request before queuing it to the hardware device.
> > + *
> > + * \return The Request status
> > + */
> > +Request::Private::Status Request::Private::prepare(std::chrono::milliseconds timeout)
> > +{
> > +	FrameBuffer::Private *bufferData;
> > +
> > +	/* Create and connect one notifier for each synchronization fence. */
> > +	for (FrameBuffer *buffer : pending_) {
> > +		bufferData = buffer->_d();
> > +
> > +		if (!bufferData->fence())
> > +			continue;
> > +
> > +		int fenceFd = bufferData->fence()->fd().fd();
> > +		notifiers_.emplace(new EventNotifier(fenceFd, EventNotifier::Read));
>
> There's no bug in your code, but usage of new() indicates potential
> memory leaks, so it's best to minimize its usage. I'd write
>
> 		notifiers_.insert(std::make_unique<EventNotifier>(fenceFd, EventNotifier::Read));
>
> > +	}
> > +
> > +	if (notifiers_.empty()) {
> > +		status_ = Status::Ready;
> > +		return status_;
> > +	}
> > +
> > +	for (const std::unique_ptr<EventNotifier> &notifier : notifiers_)
> > +		notifier->activated.connect(this, [this, &notifier] {
> > +							notifierActivated(notifier);
> > +					        });
> > +
> > +	/* In case a timeout is specified, create a timer and set it up. */
>
> Let's add
>
> 	 * 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) {
> > +		timer_ = std::make_unique<Timer>();
> > +		timer_->timeout.connect(this, &Request::Private::timeout);
> > +		timer_->start(timeout);
> > +	}
> > +
> > +	return Status::Pending;
> >  }
> > +
> > +/**
> > + * \var Request::Private::prepared
> > + * \brief Request preparation completed Signal
> > + */
> > +
> >  /**
> >   * \var Request::Private::sequence_
> >   * \brief The request sequence number
> > @@ -191,6 +296,51 @@ void Request::Private::reuse()
> >   * \copydoc Request::sequence()
> >   */
> >
> > +void Request::Private::notifierActivated(const std::unique_ptr<EventNotifier> &notifier)
> > +{
> > +	auto it = notifiers_.find(notifier);
> > +	ASSERT(it != notifiers_.end());
> > +
> > +	/* We need to close the fence if successfully signalled. */
> > +	int fd = notifier->fd();
> > +	bool found = false;
> > +	for (FrameBuffer *buffer : pending_) {
> > +		FrameBuffer::Private *bufferData = buffer->_d();
> > +
> > +		if (!bufferData->fence())
> > +			continue;
> > +
> > +		if (bufferData->fence()->fd().fd() != fd)
> > +			continue;
>
> If you turned notifiers_ into a map indexed by FrameBuffer *, you could
> pass the FrameBuffer pointer instead of the notifier to the
> notifierActivated() function, and avoid this loop.

That's a very nice idea!

>
> > +
> > +		bufferData->closeFence();
> > +		found = true;
> > +		break;
> > +	}
> > +	ASSERT(found);
> > +
> > +	notifiers_.erase(it);
> > +	if (!notifiers_.empty())
> > +		return;
> > +
> > +	/* All fences completed, delete the timer and move to state Ready. */
> > +	timer_.reset();
> > +	status_ = Status::Ready;
> > +	prepared.emit();
> > +}
> > +
> > +void Request::Private::timeout()
> > +{
> > +	/* A timeout can only happen if there are fences not yet signalled. */
> > +	ASSERT(!notifiers_.empty());
> > +	notifiers_.clear();
> > +
> > +	LOG(Request, Error) << "Request prepare timeout";
>
> Could you print the cookie to ease debugging ?
>

Indeed

> > +
> > +	status_ = Status::Failed;
> > +	prepared.emit();
> > +}
> > +
> >  /**
> >   * \enum Request::Status
> >   * Request completion status
>
> --
> Regards,
>
> Laurent Pinchart