[libcamera-devel] [PATCH v5 7/7] libcamera: buffer: Store Request reference in Buffer
Jacopo Mondi
jacopo at jmondi.org
Tue Apr 16 15:24:17 CEST 2019
Hi Laurent,
On Tue, Apr 16, 2019 at 02:35:59PM +0300, Laurent Pinchart wrote:
> 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.
>
True indeed, but I would prefer going through an accessor method if
possible...
> > > +
> > > 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.
I don't like the "often need"
>
> s/is associated with/has been queued to/ to avoid repeating "associate"?
+ * The intended callers of this method are buffer completion
+ * handlers that needs to associated a buffer to the request it has
+ * been queued to.
I find the original version more informative, but ok...
>
> > > + *
> > > + * 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".
This I agree.
>
> > > + *
> > > + * 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.
>
Right, I'll move the lifecycle description here
> > 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/
Ack, I'll take it in.
>
> > > + * \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.
>
Ack!
Thanks
j
> > > +
> > > /**
> > > * \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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190416/0dd33664/attachment.sig>
More information about the libcamera-devel
mailing list