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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Apr 16, 2019 at 10:14:34PM +0200, Niklas Söderlund wrote:
> 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.

s/this buffer/the buffer/

> > 
> > As Buffers might outlive the Request they are associated with, the

s/Buffers/buffers/ (let's keep capitalized class names invariable)

> > reference is only temporary valid during the buffer completion interval
> > (since when the buffer gets queued to Camera for processing, until it

s/since/from/

> > 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/

"callers ... are" or "caller ... is". As there are multiple completion
handlers, I think the plural is the correct form.

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

s/needs to associated/need to associate/

> s/it has been queued to/belongs to/

s/belongs/it belongs/ :-)

> > + *
> > + * 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.

I would replace it with "The returned request pointer is valid only
during that interval." as I think it's important to document the
lifetime of the pointer.

> > + *
> > + * \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;
> > +

You can remove this blank line.

> > +		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();
> >  }
> >  

-- 
Regards,

Laurent Pinchart