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

[Laurent Pinchart]

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.

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

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

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

-- 
Regards,

Laurent Pinchart