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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Dec 01, 2021 at 03:29:34PM +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>
> ---
>  include/libcamera/internal/request.h |  15 +++
>  src/libcamera/request.cpp            | 132 ++++++++++++++++++++++++++-
>  2 files changed, 146 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
> index 1340ffa2a683..740ab21ac7e0 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,24 @@ public:
>  	void cancel();
>  	void reuse();
>  
> +	void prepare(std::chrono::milliseconds timeout = 0ms);
> +	Signal<> prepared;
> +
>  private:
>  	friend class PipelineHandler;
>  
>  	void doCancelRequest();
> +	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 5ddb4b0592b3..59fb02b4a0b7 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();
>  }
>  
>  /**
> @@ -163,6 +165,134 @@ void Request::Private::reuse()
>  	sequence_ = 0;
>  	cancelled_ = false;
>  	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 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)
> +{
> +	prepared_ = false;

Should this be moved to Request::Private::reuse() ?

> +
> +	/* Create and connect one notifier for each synchronization fence. */
> +	for (FrameBuffer *buffer : pending_) {
> +		const Fence *fence = buffer->_d()->fence();
> +

You can drop the blank line.

> +		if (!fence)
> +			continue;
> +
> +		notifiers_[buffer] = std::make_unique<EventNotifier>(fence->fd().get(),
> +								     EventNotifier::Read);
> +	}
> +
> +	if (notifiers_.empty()) {
> +		prepared_ = true;
> +		prepared.emit();
> +		return;
> +	}
> +
> +	for (auto &it : notifiers_) {
> +		FrameBuffer *buffer = it.first;
> +		std::unique_ptr<EventNotifier> &notifier = it.second;
> +
> +		notifier->activated.connect(this, [this, buffer] {
> +							notifierActivated(buffer);
> +					        });

I think this can be moved to the loop above, possibly with something
like

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


> +	}
> +
> +	/*
> +	 * 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) {

Maybe

	if (timeout) {

> +		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 (prepared_
> + * == true) and is ready for being queued. The Request might complete with

s/for being/to be/

> + * 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();
> +	prepared_ = true;
> +	prepared.emit();
> +}
> +
> +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();
> +
> +	prepared_ = true;
> +	prepared.emit();
>  }
>  
>  /**
> @@ -314,7 +444,7 @@ void Request::reuse(ReuseFlag flags)
>   * responsible for resetting it before associating this buffer with a new
>   * Request by calling this function again.
>   *
> - * \sa FrameBuffer::resetFence()
> + * \sa FrameBuffer::releaseFence()

This seems to belong to another patch.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>   *
>   * \return 0 on success or a negative error code otherwise
>   * \retval -EEXIST The request already contains a buffer for the stream

-- 
Regards,

Laurent Pinchart