[libcamera-devel] [PATCH v5 7/7] libcamera: buffer: Store Request reference in Buffer
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Apr 16 02:16:29 CEST 2019
Hi Jacopo,
Thanks for your work.
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; }
> +
> 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
completion handlers often need to associate a buffer to the request it
is associated with.
> + *
> + * 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.
> + *
> + * See also Buffer::setRequest() for a more detailed explanation of the
> + * validity interval of the Request reference contained in a Buffer.
See Buffer::setRequest() for a detailed explanation of the lifetime
cycle for the buffer to request association.
> + *
> + * \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
> + * \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).
> +
> /**
> * \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();
> }
>
> --
> 2.21.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list