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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Dec 10, 2021 at 09:52:33PM +0100, Jacopo Mondi wrote:
> Add an optional fence parameter to Request::addBuffer() to allow
> associating a Fence with a FrameBuffer.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/request.h |  4 +++-
>  src/libcamera/request.cpp   | 35 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 37 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..016db9d62340 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 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,30 @@ 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 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
> + * 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.

I think you forgot to drop this paragraph when adding the next one.

> + *
> + * 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::releaseFence()
> + *
>   * \return 0 on success or a negative error code otherwise
>   * \retval -EEXIST The request already contains a buffer for the stream

 * \retval -EEXIST The request already contains a buffer for the stream,
 * or the buffer still references a fence

>   * \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 +344,18 @@ 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.
> +	 */
> +	if (buffer->_d()->fence()) {
> +		LOG(Request, Error) << "Failed to add buffer with a valid fence";

Maybe

		LOG(Request, Error) << "Can't add buffer that still references a fence";

to match the documentation ?


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

-- 
Regards,

Laurent Pinchart