| Message ID | 20190416134210.21097-8-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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(); > > } > >
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 + * needs to associated a buffer to the request it has been queued 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. 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. + * + * \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. + * + * The intended callers are the Request::prepare() and Request::completeBuffer() + * 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(); }
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@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(-)