[libcamera-devel,v2,04/12] libcamera: request: Add Fence to Request::addBuffer()

14672 diff mbox series

Add an overloaded version of Request::addBuffer() that allows
application to specify a Fence instance to be associated with the
Framebuffer.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/request.h |  5 ++++
 src/libcamera/request.cpp   | 58 ++++++++++++++++++++++++++++---------
 2 files changed, 50 insertions(+), 13 deletions(-)

Hi Jacopo,

Thank you for the patch.

On Sat, Nov 20, 2021 at 12:13:05PM +0100, Jacopo Mondi wrote:
> Add an overloaded version of Request::addBuffer() that allows
> application to specify a Fence instance to be associated with the
> Framebuffer.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/request.h |  5 ++++
>  src/libcamera/request.cpp   | 58 ++++++++++++++++++++++++++++---------
>  2 files changed, 50 insertions(+), 13 deletions(-)
> 
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index f0c5163d987e..37aebc41834e 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -22,6 +22,7 @@ namespace libcamera {
>  
>  class Camera;
>  class CameraControlValidator;
> +class Fence;
>  class FrameBuffer;
>  class Stream;
>  
> @@ -52,6 +53,8 @@ public:
>  	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);
>  	FrameBuffer *findBuffer(const Stream *stream) const;
>  
>  	uint32_t sequence() const;
> @@ -65,6 +68,8 @@ public:
>  private:
>  	LIBCAMERA_DISABLE_COPY(Request)
>  
> +	int _addBuffer(const Stream *stream, FrameBuffer *buffer);
> +
>  	ControlList *controls_;
>  	ControlList *metadata_;
>  	BufferMap bufferMap_;
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 90c436648405..1d47698a6263 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -19,6 +19,7 @@
>  
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/camera_controls.h"
> +#include "libcamera/internal/fence.h"
>  #include "libcamera/internal/framebuffer.h"
>  #include "libcamera/internal/tracepoints.h"
>  
> @@ -316,6 +317,26 @@ void Request::reuse(ReuseFlag flags)
>   * \return The map of Stream to FrameBuffer
>   */
>  
> +int Request::_addBuffer(const Stream *stream, FrameBuffer *buffer)
> +{
> +	if (!stream) {
> +		LOG(Request, Error) << "Invalid stream reference";
> +		return -EINVAL;
> +	}
> +
> +	auto it = bufferMap_.find(stream);
> +	if (it != bufferMap_.end()) {
> +		LOG(Request, Error) << "FrameBuffer already set for stream";
> +		return -EEXIST;
> +	}
> +
> +	buffer->_d()->setRequest(this);
> +	_d()->pending_.insert(buffer);
> +	bufferMap_[stream] = buffer;
> +
> +	return 0;
> +}
> +
>  /**
>   * \brief Add a FrameBuffer with its associated Stream to the Request
>   * \param[in] stream The stream the buffer belongs to
> @@ -334,22 +355,33 @@ void Request::reuse(ReuseFlag flags)
>   */
>  int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
>  {
> -	if (!stream) {
> -		LOG(Request, Error) << "Invalid stream reference";
> -		return -EINVAL;
> -	}
> +	int ret = _addBuffer(stream, buffer);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Drop the buffer Fence, if any.
> +	 *
> +	 * Buffers can be re-used and might still have a Fence associated from
> +	 * a previous run if the Fence has failed. Drop it here.
> +	 */
> +	buffer->_d()->closeFence();

I think it would be better to ASSERT(!buffer->fence()) instead.
Applications must handle fence timeouts by extracting the fence from the
FrameBuffer at request completion time. If we silently ignore the
failures to do so, it will cause hard to debug problem.

We could alternatively print an Error message and return an error, to
avoid crashes. That would probably be better, as wait timeouts should be
rare, so they may not be caught early during development.

With such a change, we could probably merge the two addBuffer()
functions in one, with a default value for the fence argument.

>  
> -	auto it = bufferMap_.find(stream);
> -	if (it != bufferMap_.end()) {
> -		LOG(Request, Error) << "FrameBuffer already set for stream";
> -		return -EEXIST;
> -	}
> +	return 0;
> +}
>  
> -	buffer->_d()->setRequest(this);
> -	_d()->pending_.insert(buffer);
> -	bufferMap_[stream] = buffer;
> +/**
> + * \brief Add a FrameBuffer with its associated Stream and Fence
> + * \copydoc Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
> + * \param[in] fence The synchronization fence associated with \a buffer
> + */
> +int Request::addBuffer(const Stream *stream, FrameBuffer *buffer,
> +		       std::unique_ptr<Fence> &&fence)
> +{
> +	if (fence->isValid())
> +		buffer->_d()->setFence(std::move(fence));
>  
> -	return 0;
> +	return _addBuffer(stream, buffer);
>  }
>  
>  /**
> -- 
> 2.33.1
>