[libcamera-devel] [PATCH v5 7/7] libcamera: buffer: Store Request reference in Buffer
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 16 13:35:59 CEST 2019
Hi Jacopo,
Thank you for the patch.
On Tue, Apr 16, 2019 at 02:16:29AM +0200, Niklas Söderlund wrote:
> On 2019-04-16 01:18:59 +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).
> >
> > To show this change purpose, use the new Buffer::request() method
> > in IPU3 pipeline handler, as it will soon be moved to support multiple
> > streams where buffers might complete in an order different from the request
> > queuing one.
>
> You know it, this should IMHO be done in a separate patch or merged with
> some other patch adding multiple stream support to the IPU3 ;-)
>
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > include/libcamera/buffer.h | 6 ++++
> > src/libcamera/buffer.cpp | 45 +++++++++++++++++++++++++++-
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
> > src/libcamera/request.cpp | 4 +++
> > 4 files changed, 55 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> > index 0c844d126a27..a2daaaf346dc 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 *req) { request_ = req; }
s/req/request/
I wonder if we need this method, or if the Request class could access
the request_ field directly as it's a friend. I suppose than an accessor
is cleaner.
> > +
> > 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..790c6dcffe3a 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,26 @@ 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 slots
> > + * implemented in CameraData sub-classes which needs to associated a request
> > + * to the just completed buffer, before calling Request::completeBuffer().
>
> The intended caller of this method is buffer completion handlers. Buffer
"handlers" is plural, so "The intended callers of this method are".
> completion handlers often need to associate a buffer to the request it
> is associated with.
s/is associated with/has been queued to/ to avoid repeating "associate"?
> > + *
> > + * It is up to the caller of this function to deal with the case the buffer has
> > + * been already marked as complete, and the reference to the Request has been
> > + * invalidated and set to nullptr.
>
> I would drop this.
I agree, but I would also add
* The returned request pointer is guaranteed to be valid from the time the
* request is prepared with Request::prepare() to the time the buffer is marked
* as complete with Request::completeBuffer().
Maybe with an additional "See the documentation of those two methods for
more information".
> > + *
> > + * See also Buffer::setRequest() for a more detailed explanation of the
> > + * validity interval of the Request reference contained in a Buffer.
Ah, the information is in setRequest() :-) I would move it here, as
setRequest() has a single intended caller, while request() is called by
pipeline handlers, so it's easier for pipeline handlers developers to
not have to read the setRequest() documentation.
> See Buffer::setRequest() for a detailed explanation of the lifetime
> cycle for the buffer to request association.
Doxygen has a \sa tag for the "See also".
> > + *
> > + * \return The Request this Buffer belongs to or nullptr if the Buffer has
> > + * not been queued to the Camera for processing yet or it has completed already.
>
> The Request the Buffer belongs to, or nullptr if the buffer is either
> completed or not associate with a request
s/associate/associated/
> > + * \sa Buffer::setRequest()
> > + */
> > +
> > /**
> > * \brief Mark a buffer as cancel by setting its status to BufferCancelled
> > */
> > @@ -259,6 +279,29 @@ 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.
> > + *
> > + * Buffers are associated to requests at Request::prepare() time and said
> > + * association is valid until the buffer does not complete at
> > + * Request::completeBuffer() time. 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
> > + * Request is set to nullptr.
>
> IMHO most of this should be rephrases and moved to the documentation for
> Request::prepare(). What should be documented here is how this call
> modifies the state of the Buffer object.
>
> > + *
> > + * The intended caller of this methods is the Request class at
> > + * Request::prepare() time, when it stores a reference to itself through a
> > + * call to this method, and at Request::completeBuffer() time, where it
> > + * invalidates the request_ reference by calling this method with a nullptr as
> > + * argument.
> > + */
>
> I'm not saying it's wrong, but is it strictly necessary to document
> intended callers for an internal API? I agree this is most needed for
> interfaces deigned to be called at specific points in either
> applications or pipeline handles (and later 3A).
It's not strictly necessary, but it can avoid abuses for methods that
are meant to be called from a very specific place. I would however
simplify this paragraph by just stating
* The intended callers are the Request::prepare() and Request::completeBuffer()
* methods.
With the life cycle explanation moved to the Request documentation, as
proposed above.
> > +
> > /**
> > * \class BufferPool
> > * \brief A pool of buffers
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 7d865fa329ea..ce680835cec2 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -630,7 +630,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 7fa034e6c747..6b175e125596 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -130,6 +130,8 @@ int Request::prepare()
> > {
> > for (auto const &pair : bufferMap_) {
> > Buffer *buffer = pair.second;
> > +
> > + buffer->setRequest(this);
> > pending_.insert(buffer);
> > }
> >
> > @@ -166,6 +168,8 @@ bool Request::completeBuffer(Buffer *buffer)
> > int ret = pending_.erase(buffer);
> > ASSERT(ret == 1);
> >
> > + buffer->setRequest(nullptr);
> > +
> > return empty();
> > }
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list