| Message ID | 20190630181049.9548-7-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2019-06-30 20:10:47 +0200, Jacopo Mondi wrote: > Add and operation to map external buffers provided by applications in > a Request to the Stream's internal buffers used by the pipeline handlers > to interact with the video device. > > For streams using internal memory allocation, the two buffers are the > same as applications effectively use Buffers from the Stream's pool > where the video device memory has been exported to. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/buffer.h | 1 + > include/libcamera/stream.h | 5 +++ > src/libcamera/stream.cpp | 66 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 72 insertions(+) > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > index 260a62e9e77e..d5d3dc90a096 100644 > --- a/include/libcamera/buffer.h > +++ b/include/libcamera/buffer.h > @@ -59,6 +59,7 @@ private: > friend class BufferPool; > friend class PipelineHandler; > friend class Request; > + friend class Stream; > friend class V4L2VideoDevice; > > void cancel(); > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > index 796f1aff2602..4c034c113ddb 100644 > --- a/include/libcamera/stream.h > +++ b/include/libcamera/stream.h > @@ -85,6 +85,8 @@ public: > const StreamConfiguration &configuration() const { return configuration_; } > MemoryType memoryType() const { return memoryType_; } > > + Buffer *mapBuffer(Buffer *requestBuffer); > + > protected: > friend class Camera; > > @@ -94,6 +96,9 @@ protected: > BufferPool bufferPool_; > StreamConfiguration configuration_; > MemoryType memoryType_; > + > + std::vector<Buffer *> mappableBuffers_; > + std::map<unsigned int, Buffer *> bufferMaps_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > index b6292427d3a2..f36336857ad6 100644 > --- a/src/libcamera/stream.cpp > +++ b/src/libcamera/stream.cpp > @@ -13,6 +13,8 @@ > #include <iomanip> > #include <sstream> > > +#include <libcamera/request.h> > + > #include "log.h" > > /** > @@ -445,6 +447,26 @@ void Stream::createBuffers(unsigned int count) > return; > > bufferPool_.createBuffers(count); > + > + /* Streams with internal memory usage do not need buffer mapping. */ > + if (memoryType_ == InternalMemory) > + return; > + > + /* > + * Prepare for buffer mapping by queuing all buffers from the internal > + * pool. Each external buffer presented by application will be mapped > + * on an internal one. > + */ > + mappableBuffers_.clear(); > + for (Buffer &buffer : bufferPool_.buffers()) { > + /* Reserve all planes to support mapping multiplanar buffers. */ > + buffer.planes().clear(); > + /* \todo: I would use VIDEO_MAX_PLANES but that's V4L2 stuff.. */ > + for (unsigned int i = 0; i < 3; ++i) > + buffer.planes().emplace_back(); > + > + mappableBuffers_.push_back(&buffer); > + } > } > > /** > @@ -457,6 +479,50 @@ void Stream::destroyBuffers() > createBuffers(0); > } > > +/** > + * \brief Map the buffer an application has associated with a Request to an > + * internl one > + * > + * \todo Rewrite documentation > + * If the Stream uses external memory, we need to map the externally > + * provided buffer to an internal one, trying to keep a best effort > + * association based on the Buffer's last usage time. I would not use the word last here as it (at lest to me) implies that the buffer selected is the most recently used buffer and not the oldest buffer. > + * External and internal buffers are associated by using the dmabuf > + * fds as key. > + */ > +Buffer *Stream::mapBuffer(Buffer *requestBuffer) > +{ > + /* > + * \todo Multiplane APIs have one fd per plane, the key should be > + * hashed using all the planes fds. > + */ > + unsigned int key = requestBuffer->planes()[0].dmabuf(); > + > + /* If the buffer has already been mapped, just return it. */ > + auto mapped = bufferMaps_.find(key); > + if (mapped != bufferMaps_.end()) > + return mapped->second; On a hit should you not update the buffers position in mappableBuffers_ to increase the hit rate and try to keep the cache as hot as possible? One idea is to make mappableBuffers_ a std::set and index it on a timestmap property, that way you could just update the timestamp when a buffer is resued and when you need to evict a buffer from the list the oldes buffer will always be at the front (or end) of the set. > + > + /* > + * Remove the last recently used buffer from the circular list and > + * use it for mapping. > + */ > + auto mappable = mappableBuffers_.begin(); > + Buffer *buffer = *mappable; > + mappableBuffers_.erase(mappable); > + mappableBuffers_.push_back(buffer); It's simpler to rotate the vector, std::rotate(mappableBuffers_.begin(), mappableBuffers_.begin() + 1, mappableBuffers_.end()); > + > + /* \todo: Support multiplanar external buffers. */ > + buffer->planes()[0].setDmabuf(key, 0); > + > + /* Pipeline handlers use request_ at buffer completion time. */ > + buffer->request_ = requestBuffer->request(); > + > + bufferMaps_[key] = buffer; I'm no expert on our buffer handling, but when you evict a buffer from mappableBuffers_ should it not also be removed from bufferMaps_? Else if every buffer imported is a new dmabuf we will consume a lot of memory. > + > + return buffer; > +} > + > /** > * \var Stream::bufferPool_ > * \brief The pool of buffers associated with the stream > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Tue, Jul 02, 2019 at 01:07:30AM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > On 2019-06-30 20:10:47 +0200, Jacopo Mondi wrote: > > Add and operation to map external buffers provided by applications in > > a Request to the Stream's internal buffers used by the pipeline handlers > > to interact with the video device. > > > > For streams using internal memory allocation, the two buffers are the > > same as applications effectively use Buffers from the Stream's pool > > where the video device memory has been exported to. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/buffer.h | 1 + > > include/libcamera/stream.h | 5 +++ > > src/libcamera/stream.cpp | 66 ++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 72 insertions(+) > > > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > > index 260a62e9e77e..d5d3dc90a096 100644 > > --- a/include/libcamera/buffer.h > > +++ b/include/libcamera/buffer.h > > @@ -59,6 +59,7 @@ private: > > friend class BufferPool; > > friend class PipelineHandler; > > friend class Request; > > + friend class Stream; > > friend class V4L2VideoDevice; > > > > void cancel(); > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > index 796f1aff2602..4c034c113ddb 100644 > > --- a/include/libcamera/stream.h > > +++ b/include/libcamera/stream.h > > @@ -85,6 +85,8 @@ public: > > const StreamConfiguration &configuration() const { return configuration_; } > > MemoryType memoryType() const { return memoryType_; } > > > > + Buffer *mapBuffer(Buffer *requestBuffer); > > + > > protected: > > friend class Camera; > > > > @@ -94,6 +96,9 @@ protected: > > BufferPool bufferPool_; > > StreamConfiguration configuration_; > > MemoryType memoryType_; > > + > > + std::vector<Buffer *> mappableBuffers_; > > + std::map<unsigned int, Buffer *> bufferMaps_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > > index b6292427d3a2..f36336857ad6 100644 > > --- a/src/libcamera/stream.cpp > > +++ b/src/libcamera/stream.cpp > > @@ -13,6 +13,8 @@ > > #include <iomanip> > > #include <sstream> > > > > +#include <libcamera/request.h> > > + > > #include "log.h" > > > > /** > > @@ -445,6 +447,26 @@ void Stream::createBuffers(unsigned int count) > > return; > > > > bufferPool_.createBuffers(count); > > + > > + /* Streams with internal memory usage do not need buffer mapping. */ > > + if (memoryType_ == InternalMemory) > > + return; > > + > > + /* > > + * Prepare for buffer mapping by queuing all buffers from the internal > > + * pool. Each external buffer presented by application will be mapped > > + * on an internal one. > > + */ > > + mappableBuffers_.clear(); > > + for (Buffer &buffer : bufferPool_.buffers()) { > > + /* Reserve all planes to support mapping multiplanar buffers. */ > > + buffer.planes().clear(); > > + /* \todo: I would use VIDEO_MAX_PLANES but that's V4L2 stuff.. */ > > + for (unsigned int i = 0; i < 3; ++i) > > + buffer.planes().emplace_back(); > > + > > + mappableBuffers_.push_back(&buffer); > > + } > > } > > > > /** > > @@ -457,6 +479,50 @@ void Stream::destroyBuffers() > > createBuffers(0); > > } > > > > +/** > > + * \brief Map the buffer an application has associated with a Request to an > > + * internl one > > + * > > + * \todo Rewrite documentation > > + * If the Stream uses external memory, we need to map the externally > > + * provided buffer to an internal one, trying to keep a best effort > > + * association based on the Buffer's last usage time. > > I would not use the word last here as it (at lest to me) implies that > the buffer selected is the most recently used buffer and not the oldest > buffer. > > > + * External and internal buffers are associated by using the dmabuf > > + * fds as key. > > + */ > > +Buffer *Stream::mapBuffer(Buffer *requestBuffer) > > +{ > > + /* > > + * \todo Multiplane APIs have one fd per plane, the key should be > > + * hashed using all the planes fds. > > + */ > > + unsigned int key = requestBuffer->planes()[0].dmabuf(); > > + > > + /* If the buffer has already been mapped, just return it. */ > > + auto mapped = bufferMaps_.find(key); > > + if (mapped != bufferMaps_.end()) > > + return mapped->second; > > On a hit should you not update the buffers position in mappableBuffers_ > to increase the hit rate and try to keep the cache as hot as possible? You are right! > > One idea is to make mappableBuffers_ a std::set and index it on a > timestmap property, that way you could just update the timestamp when a > buffer is resued and when you need to evict a buffer from the list the > oldes buffer will always be at the front (or end) of the set. Or I could remove the buffer from the vector of used buffers and push it back again. Your one is probably more elegant, but I do expect a very limited number of buffers to cycle. > > > + > > + /* > > + * Remove the last recently used buffer from the circular list and > > + * use it for mapping. > > + */ > > + auto mappable = mappableBuffers_.begin(); > > + Buffer *buffer = *mappable; > > + mappableBuffers_.erase(mappable); > > + mappableBuffers_.push_back(buffer); > > It's simpler to rotate the vector, > > std::rotate(mappableBuffers_.begin(), mappableBuffers_.begin() + 1, mappableBuffers_.end()); > Thanks! Also, if you have better names to suggest in place of mappableBuffers_ I would be happy to hear that. > > + > > + /* \todo: Support multiplanar external buffers. */ > > + buffer->planes()[0].setDmabuf(key, 0); > > + > > + /* Pipeline handlers use request_ at buffer completion time. */ > > + buffer->request_ = requestBuffer->request(); > > + > > + bufferMaps_[key] = buffer; > > I'm no expert on our buffer handling, but when you evict a buffer from > mappableBuffers_ should it not also be removed from bufferMaps_? Else if > every buffer imported is a new dmabuf we will consume a lot of memory. > True, the map could grow if we keep receiving new buffers. Again, I expect application to cycle on a limited number of buffers so I was not too concerned, but yes, when a a buffer mapping is removed, the entry on the map corresponding to that buffer key should be erased. Thanks for spotting these! > > + > > + return buffer; > > +} > > + > > /** > > * \var Stream::bufferPool_ > > * \brief The pool of buffers associated with the stream > > -- > > 2.21.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
Hi Jacopo, On 30/06/2019 19:10, Jacopo Mondi wrote: > Add and operation to map external buffers provided by applications in > a Request to the Stream's internal buffers used by the pipeline handlers > to interact with the video device. > > For streams using internal memory allocation, the two buffers are the > same as applications effectively use Buffers from the Stream's pool > where the video device memory has been exported to. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/buffer.h | 1 + > include/libcamera/stream.h | 5 +++ > src/libcamera/stream.cpp | 66 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 72 insertions(+) > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > index 260a62e9e77e..d5d3dc90a096 100644 > --- a/include/libcamera/buffer.h > +++ b/include/libcamera/buffer.h > @@ -59,6 +59,7 @@ private: > friend class BufferPool; > friend class PipelineHandler; > friend class Request; > + friend class Stream; > friend class V4L2VideoDevice; > > void cancel(); > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > index 796f1aff2602..4c034c113ddb 100644 > --- a/include/libcamera/stream.h > +++ b/include/libcamera/stream.h > @@ -85,6 +85,8 @@ public: > const StreamConfiguration &configuration() const { return configuration_; } > MemoryType memoryType() const { return memoryType_; } > > + Buffer *mapBuffer(Buffer *requestBuffer); > + > protected: > friend class Camera; > > @@ -94,6 +96,9 @@ protected: > BufferPool bufferPool_; > StreamConfiguration configuration_; > MemoryType memoryType_; > + > + std::vector<Buffer *> mappableBuffers_; > + std::map<unsigned int, Buffer *> bufferMaps_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > index b6292427d3a2..f36336857ad6 100644 > --- a/src/libcamera/stream.cpp > +++ b/src/libcamera/stream.cpp > @@ -13,6 +13,8 @@ > #include <iomanip> > #include <sstream> > > +#include <libcamera/request.h> > + > #include "log.h" > > /** > @@ -445,6 +447,26 @@ void Stream::createBuffers(unsigned int count) > return; > > bufferPool_.createBuffers(count); > + > + /* Streams with internal memory usage do not need buffer mapping. */ > + if (memoryType_ == InternalMemory) > + return; > + > + /* > + * Prepare for buffer mapping by queuing all buffers from the internal > + * pool. Each external buffer presented by application will be mapped > + * on an internal one. > + */ > + mappableBuffers_.clear(); > + for (Buffer &buffer : bufferPool_.buffers()) { > + /* Reserve all planes to support mapping multiplanar buffers. */ > + buffer.planes().clear(); > + /* \todo: I would use VIDEO_MAX_PLANES but that's V4L2 stuff.. */ > + for (unsigned int i = 0; i < 3; ++i) > + buffer.planes().emplace_back(); Just a small one: Could buffer.planes().resize(3) be better here? > + > + mappableBuffers_.push_back(&buffer); > + } > } > > /** > @@ -457,6 +479,50 @@ void Stream::destroyBuffers() > createBuffers(0); > } > > +/** > + * \brief Map the buffer an application has associated with a Request to an > + * internl one > + * > + * \todo Rewrite documentation > + * If the Stream uses external memory, we need to map the externally > + * provided buffer to an internal one, trying to keep a best effort > + * association based on the Buffer's last usage time. > + * External and internal buffers are associated by using the dmabuf > + * fds as key. > + */ > +Buffer *Stream::mapBuffer(Buffer *requestBuffer) > +{ > + /* > + * \todo Multiplane APIs have one fd per plane, the key should be > + * hashed using all the planes fds. > + */ > + unsigned int key = requestBuffer->planes()[0].dmabuf(); > + > + /* If the buffer has already been mapped, just return it. */ > + auto mapped = bufferMaps_.find(key); > + if (mapped != bufferMaps_.end()) > + return mapped->second; > + > + /* > + * Remove the last recently used buffer from the circular list and > + * use it for mapping. > + */ > + auto mappable = mappableBuffers_.begin(); > + Buffer *buffer = *mappable; > + mappableBuffers_.erase(mappable); > + mappableBuffers_.push_back(buffer); > + > + /* \todo: Support multiplanar external buffers. */ > + buffer->planes()[0].setDmabuf(key, 0); > + > + /* Pipeline handlers use request_ at buffer completion time. */ > + buffer->request_ = requestBuffer->request(); > + > + bufferMaps_[key] = buffer; > + > + return buffer; > +} > + > /** > * \var Stream::bufferPool_ > * \brief The pool of buffers associated with the stream >
diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h index 260a62e9e77e..d5d3dc90a096 100644 --- a/include/libcamera/buffer.h +++ b/include/libcamera/buffer.h @@ -59,6 +59,7 @@ private: friend class BufferPool; friend class PipelineHandler; friend class Request; + friend class Stream; friend class V4L2VideoDevice; void cancel(); diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h index 796f1aff2602..4c034c113ddb 100644 --- a/include/libcamera/stream.h +++ b/include/libcamera/stream.h @@ -85,6 +85,8 @@ public: const StreamConfiguration &configuration() const { return configuration_; } MemoryType memoryType() const { return memoryType_; } + Buffer *mapBuffer(Buffer *requestBuffer); + protected: friend class Camera; @@ -94,6 +96,9 @@ protected: BufferPool bufferPool_; StreamConfiguration configuration_; MemoryType memoryType_; + + std::vector<Buffer *> mappableBuffers_; + std::map<unsigned int, Buffer *> bufferMaps_; }; } /* namespace libcamera */ diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp index b6292427d3a2..f36336857ad6 100644 --- a/src/libcamera/stream.cpp +++ b/src/libcamera/stream.cpp @@ -13,6 +13,8 @@ #include <iomanip> #include <sstream> +#include <libcamera/request.h> + #include "log.h" /** @@ -445,6 +447,26 @@ void Stream::createBuffers(unsigned int count) return; bufferPool_.createBuffers(count); + + /* Streams with internal memory usage do not need buffer mapping. */ + if (memoryType_ == InternalMemory) + return; + + /* + * Prepare for buffer mapping by queuing all buffers from the internal + * pool. Each external buffer presented by application will be mapped + * on an internal one. + */ + mappableBuffers_.clear(); + for (Buffer &buffer : bufferPool_.buffers()) { + /* Reserve all planes to support mapping multiplanar buffers. */ + buffer.planes().clear(); + /* \todo: I would use VIDEO_MAX_PLANES but that's V4L2 stuff.. */ + for (unsigned int i = 0; i < 3; ++i) + buffer.planes().emplace_back(); + + mappableBuffers_.push_back(&buffer); + } } /** @@ -457,6 +479,50 @@ void Stream::destroyBuffers() createBuffers(0); } +/** + * \brief Map the buffer an application has associated with a Request to an + * internl one + * + * \todo Rewrite documentation + * If the Stream uses external memory, we need to map the externally + * provided buffer to an internal one, trying to keep a best effort + * association based on the Buffer's last usage time. + * External and internal buffers are associated by using the dmabuf + * fds as key. + */ +Buffer *Stream::mapBuffer(Buffer *requestBuffer) +{ + /* + * \todo Multiplane APIs have one fd per plane, the key should be + * hashed using all the planes fds. + */ + unsigned int key = requestBuffer->planes()[0].dmabuf(); + + /* If the buffer has already been mapped, just return it. */ + auto mapped = bufferMaps_.find(key); + if (mapped != bufferMaps_.end()) + return mapped->second; + + /* + * Remove the last recently used buffer from the circular list and + * use it for mapping. + */ + auto mappable = mappableBuffers_.begin(); + Buffer *buffer = *mappable; + mappableBuffers_.erase(mappable); + mappableBuffers_.push_back(buffer); + + /* \todo: Support multiplanar external buffers. */ + buffer->planes()[0].setDmabuf(key, 0); + + /* Pipeline handlers use request_ at buffer completion time. */ + buffer->request_ = requestBuffer->request(); + + bufferMaps_[key] = buffer; + + return buffer; +} + /** * \var Stream::bufferPool_ * \brief The pool of buffers associated with the stream
Add and operation to map external buffers provided by applications in a Request to the Stream's internal buffers used by the pipeline handlers to interact with the video device. For streams using internal memory allocation, the two buffers are the same as applications effectively use Buffers from the Stream's pool where the video device memory has been exported to. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/buffer.h | 1 + include/libcamera/stream.h | 5 +++ src/libcamera/stream.cpp | 66 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+)