[libcamera-devel] [PATCH v4 05/11] libcamera: request: Add Fence to Request::addBuffer()

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Dec 01, 2021 at 03:29:30PM +0100, Jacopo Mondi wrote:
> Add an optional synchronization fence to Request::addBuffer() to allow

s/synchronization fence/fence/ (same in the patch)

> associating a Fence with a FrameBuffer part of a Request.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/request.h |  4 +++-
>  src/libcamera/request.cpp   | 26 +++++++++++++++++++++++++-
>  2 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 8c78970d88ab..1eb537e9b09b 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -17,6 +17,7 @@
>  #include <libcamera/base/signal.h>
>  
>  #include <libcamera/controls.h>
> +#include <libcamera/fence.h>
>  
>  namespace libcamera {
>  
> @@ -51,7 +52,8 @@ public:
>  	ControlList &controls() { return *controls_; }
>  	ControlList &metadata() { return *metadata_; }
>  	const BufferMap &buffers() const { return bufferMap_; }
> -	int addBuffer(const Stream *stream, FrameBuffer *buffer);
> +	int addBuffer(const Stream *stream, FrameBuffer *buffer,
> +		      std::unique_ptr<Fence> fence = nullptr);
>  	FrameBuffer *findBuffer(const Stream *stream) const;
>  
>  	uint32_t sequence() const;
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 692202473360..5ddb4b0592b3 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -14,6 +14,7 @@
>  
>  #include <libcamera/camera.h>
>  #include <libcamera/control_ids.h>
> +#include <libcamera/fence.h>
>  #include <libcamera/framebuffer.h>
>  #include <libcamera/stream.h>
>  
> @@ -294,6 +295,7 @@ void Request::reuse(ReuseFlag flags)
>   * \brief Add a FrameBuffer with its associated Stream to the Request
>   * \param[in] stream The stream the buffer belongs to
>   * \param[in] buffer The FrameBuffer to add to the request
> + * \param[in] fence The optional synchronization Fence
>   *
>   * A reference to the buffer is stored in the request. The caller is responsible
>   * for ensuring that the buffer will remain valid until the request complete
> @@ -302,11 +304,24 @@ void Request::reuse(ReuseFlag flags)
>   * A request can only contain one buffer per stream. If a buffer has already
>   * been added to the request for the same stream, this function returns -EEXIST.
>   *
> + * A synchronization Fence can be optionally associated with the \a buffer.
> + *
> + * When a valid fence is provided to this function, \a fence is moved to \a
> + * buffer and this Request will only be queued to the device once the
> + * synchronization fences of all its buffers have been correctly signalled.
> + *
> + * If the \a fence associated with \a buffer fails, the application is
> + * responsible for resetting it before associating this buffer with a new
> + * Request by calling this function again.

"fails" is a bit vague. How about the following ?

 * If the \a fence associated with \a buffer isn't signalled, the request will
 * fail after a timeout. The buffer will still contain the fence, which
 * applications must retrieve with FrameBuffer::releaseFence() before the buffer
 * can be reused in another request. Attempting to add a buffer that still
 * contains a fence to a request will result in this function returning -EEXIST.

> + *
> + * \sa FrameBuffer::resetFence()
> + *
>   * \return 0 on success or a negative error code otherwise
>   * \retval -EEXIST The request already contains a buffer for the stream
>   * \retval -EINVAL The buffer does not reference a valid Stream
>   */
> -int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
> +int Request::addBuffer(const Stream *stream, FrameBuffer *buffer,
> +		       std::unique_ptr<Fence> fence)
>  {
>  	if (!stream) {
>  		LOG(Request, Error) << "Invalid stream reference";
> @@ -323,6 +338,15 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
>  	_d()->pending_.insert(buffer);
>  	bufferMap_[stream] = buffer;
>  
> +	/*
> +	 * Make sure the fence has been extracted from the buffer
> +	 * to avoid waiting on a stale fence.
> +	 */
> +	ASSERT(!buffer->_d()->fence());

I wonder if that's not a bit too harsh. Maybe we should log an Error
message and return an error (something different than EEXIST and EINVAL
could be nice, although this really means that there's an issue on the
application side, so we could possibly reuse EEXIST) ?

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

> +
> +	if (fence && fence->isValid())
> +		buffer->_d()->setFence(std::move(fence));
> +
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart