[libcamera-devel] [PATCH v7 8/8] libcamera: buffer: Store Request reference in Buffer

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Apr 17, 2019 at 03:58:58PM +0200, Jacopo Mondi wrote:
> Add to the Buffer class methods to set and retrieve a reference to the
> Request instance the buffer is part of.
> 
> As buffers might outlive the Request they are associated with, the

s/might //

> reference is only temporary valid during the buffer completion interval
> (from when the buffer gets queued to Camera for processing, until it
> gets marked as completed).
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/buffer.h |  6 ++++++
>  src/libcamera/buffer.cpp   | 25 ++++++++++++++++++++++++-
>  src/libcamera/request.cpp  |  3 +++
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 0c844d126a27..8f9b42e39339 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -13,6 +13,7 @@
>  namespace libcamera {
>  
>  class BufferPool;
> +class Request;
>  
>  class Plane final
>  {
> @@ -52,14 +53,18 @@ public:
>  	unsigned int sequence() const { return sequence_; }
>  	Status status() const { return status_; }
>  	std::vector<Plane> &planes() { return planes_; }
> +	Request *request() const { return request_; }
>  
>  private:
>  	friend class BufferPool;
>  	friend class PipelineHandler;
> +	friend class Request;
>  	friend class V4L2Device;
>  
>  	void cancel();
>  
> +	void setRequest(Request *request) { request_ = request; }
> +
>  	unsigned int index_;
>  	unsigned int bytesused_;
>  	uint64_t timestamp_;
> @@ -67,6 +72,7 @@ private:
>  	Status status_;
>  
>  	std::vector<Plane> planes_;
> +	Request *request_;
>  };
>  
>  class BufferPool final
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index e2d1cf04411e..8261ca41d62e 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -196,7 +196,7 @@ void *Plane::mem()
>   */
>  
>  Buffer::Buffer()
> -	: index_(-1)
> +	: index_(-1), request_(nullptr)
>  {
>  }
>  
> @@ -248,6 +248,22 @@ Buffer::Buffer()
>   * \return The buffer status
>   */
>  
> +/**
> + * \fn Buffer::request()
> + * \brief Retrieve the request this buffer belongs to
> + *
> + * The intended callers of this method are buffer completion handlers that
> + * needs to associate a buffer to the request it belongs to.

s/needs/need/

With this fixed,

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

> + *
> + * A Buffer is associated to a request by Request::prepare() and the
> + * association is valid until the buffer completes. The returned request
> + * pointer is valid only during that interval.
> + *
> + * \return The Request the Buffer belongs to, or nullptr if the buffer is
> + * either completed or not associated with a request
> + * \sa Buffer::setRequest()
> + */
> +
>  /**
>   * \brief Mark a buffer as cancel by setting its status to BufferCancelled
>   */
> @@ -259,6 +275,13 @@ void Buffer::cancel()
>  	status_ = BufferCancelled;
>  }
>  
> +/**
> + * \fn Buffer::setRequest()
> + * \brief Set the request this buffer belongs to
> + *
> + * The intended callers are Request::prepare() and Request::completeBuffer().
> + */
> +
>  /**
>   * \class BufferPool
>   * \brief A pool of buffers
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index b215366baa7c..290175f876b4 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -141,6 +141,7 @@ int Request::prepare()
>  
>  	for (auto const &pair : bufferMap_) {
>  		Buffer *buffer = pair.second;
> +		buffer->setRequest(this);
>  		pending_.insert(buffer);
>  	}
>  
> @@ -177,6 +178,8 @@ bool Request::completeBuffer(Buffer *buffer)
>  	int ret = pending_.erase(buffer);
>  	ASSERT(ret == 1);
>  
> +	buffer->setRequest(nullptr);
> +
>  	return !hasPendingBuffers();
>  }
>  

-- 
Regards,

Laurent Pinchart