[libcamera-devel,v5,08/11] libcamera: request: Add Request::Private::prepare()
diff mbox series

Message ID 20211210205239.354901-9-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: Add fence
Related show

Commit Message

Jacopo Mondi Dec. 10, 2021, 8:52 p.m. UTC 
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@jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/internal/request.h |  16 ++++
 src/libcamera/request.cpp            | 133 +++++++++++++++++++++++++++
 2 files changed, 149 insertions(+)

Comments

Laurent Pinchart Dec. 10, 2021, 9:27 p.m. UTC | #1 
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@jmondi.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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.

> +		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();
>  }
>  
>  /**
Jacopo Mondi Dec. 11, 2021, 3:01 p.m. UTC | #2 
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@jmondi.org>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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

Patch
diff mbox series 

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