[libcamera-devel] [PATCH v6 7/7] libcamera: buffer: Store Request reference in Buffer

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2019-04-16 15:42:10 +0200, Jacopo Mondi wrote:
> Add to the Buffer class methods to set and retrieve a reference to the
> Request instance this buffer is part of.
> 
> As Buffers might outlive the Request they are associated with, the
> reference is only temporary valid during the buffer completion interval
> (since 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             | 34 +++++++++++++++++++++++++++-
>  src/libcamera/pipeline/ipu3/ipu3.cpp |  2 +-
>  src/libcamera/request.cpp            |  4 ++++
>  4 files changed, 44 insertions(+), 2 deletions(-)
> 
> 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..550091c998a6 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,25 @@ 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

s/callers/caller/

> + * needs to associated a buffer to the request it has been queued to.

s/it has been queued to/belongs to/

> + *
> + * Buffers are associated to requests at Request::prepare() time and said
> + * association is valid until the buffer does not complete at
> + * Request::completeBuffer() time.

A Buffer is associated to a request by Request::prepare() and the 
association is valid until the buffer completes.

> + * Before and after the buffer completion
> + * interval (the time between when the request is queued to the Camera, and
> + * the buffer is marked as 'complete' by pipeline handlers) the reference to
> + * the request is set to nullptr.

I would drop this.

> + *
> + * \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 +278,19 @@ void Buffer::cancel()
>  	status_ = BufferCancelled;
>  }
>  
> +/**
> + * \fn Buffer::setRequest()
> + * \brief Set the request this buffer belongs to
> + *
> + * Buffers are associated to Streams in a Request, which is then sent to the
> + * Camera for processing. This method stores in the Buffer a pointer to the
> + * Request this Buffer is part of, for later retrieval through the
> + * Buffer::request() method.

I would drop this. Or if you really want rewrite it to drop the talk of 
streams, it's only confusing. All setRequest() does is associate a 
buffer to a request. How it all fits together should be described at a 
higher level and not in a function description.

> + *
> + * The intended callers are the Request::prepare() and Request::completeBuffer()
> + * methods.

s/the//
s/methods//

> + */
> +
>  /**
>   * \class BufferPool
>   * \brief A pool of buffers
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index f96e8763bce9..6f5747fb1505 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -633,7 +633,7 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)
>   */
>  void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
>  {
> -	Request *request = queuedRequests_.front();
> +	Request *request = buffer->request();
>  
>  	pipe_->completeBuffer(camera_, request, buffer);
>  	pipe_->completeRequest(camera_, request);
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 1946845b91f4..9ae62f632308 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -144,6 +144,8 @@ int Request::prepare()
>  
>  	for (auto const &pair : bufferMap_) {
>  		Buffer *buffer = pair.second;
> +
> +		buffer->setRequest(this);
>  		pending_.insert(buffer);
>  	}
>  
> @@ -180,6 +182,8 @@ bool Request::completeBuffer(Buffer *buffer)
>  	int ret = pending_.erase(buffer);
>  	ASSERT(ret == 1);
>  
> +	buffer->setRequest(nullptr);
> +
>  	return !hasPendingBuffers();
>  }
>  
> -- 
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund