| Message ID | 20210826112539.170694-8-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. I think the subject line is a bit long ;-) I'll review this as part of the series right after "[PATCH v3 0/3] Improve CameraBuffer implementation". On Thu, Aug 26, 2021 at 08:25:37PM +0900, Hirokazu Honda wrote: > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > include/libcamera/internal/v4l2_videodevice.h | 10 +- > src/libcamera/v4l2_videodevice.cpp | 141 ++++++++++++++---- > test/v4l2_videodevice/buffer_cache.cpp | 6 +- > 3 files changed, 123 insertions(+), 34 deletions(-) > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > index e767ec84..bfda6726 100644 > --- a/include/libcamera/internal/v4l2_videodevice.h > +++ b/include/libcamera/internal/v4l2_videodevice.h > @@ -114,8 +114,9 @@ struct V4L2Capability final : v4l2_capability { > class V4L2BufferCache > { > public: > - V4L2BufferCache(unsigned int numEntries); > - V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers); > + V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes); > + V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers, > + unsigned int numPlanes); > ~V4L2BufferCache(); > > int get(const FrameBuffer &buffer); > @@ -126,7 +127,8 @@ private: > { > public: > Entry(); > - Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer); > + Entry(bool free, uint64_t lastUsed, unsigned int numPlanes, > + const FrameBuffer &buffer); > > bool operator==(const FrameBuffer &buffer) const; > > @@ -149,6 +151,7 @@ private: > > std::atomic<uint64_t> lastUsedCounter_; > std::vector<Entry> cache_; > + unsigned int numPlanes_; > /* \todo Expose the miss counter through an instrumentation API. */ > unsigned int missCounter_; > }; > @@ -242,6 +245,7 @@ private: > FrameBuffer *dequeueBuffer(); > > V4L2Capability caps_; > + V4L2DeviceFormat format_; > > enum v4l2_buf_type bufferType_; > enum v4l2_memory memoryType_; > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 2ff25af2..f5f8741a 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -25,6 +25,7 @@ > > #include <libcamera/file_descriptor.h> > > +#include "libcamera/internal/formats.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/media_object.h" > > @@ -157,13 +158,15 @@ LOG_DECLARE_CATEGORY(V4L2) > /** > * \brief Create an empty cache with \a numEntries entries > * \param[in] numEntries Number of entries to reserve in the cache > + * \param[in] numPlanes Number of V4L2 buffer planes > * > - * Create a cache with \a numEntries entries all marked as unused. The entries > - * will be populated as the cache is used. This is typically used to implement > - * buffer import, with buffers added to the cache as they are queued. > + * Create a cache with \a numEntries entries all marked as unused. The entry is > + * for \a numPlanes planes V4L2 buffer. The entries will be populated as the > + * cache is used. This is typically used to implement buffer import, with > + * buffers added to the cache as they are queued. > */ > -V4L2BufferCache::V4L2BufferCache(unsigned int numEntries) > - : lastUsedCounter_(1), missCounter_(0) > +V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes) > + : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0) > { > cache_.resize(numEntries); > } > @@ -171,17 +174,20 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries) > /** > * \brief Create a pre-populated cache > * \param[in] buffers Array of buffers to pre-populated with > + * \param[in] numPlanes Number of V4L2 buffer planes > * > - * Create a cache pre-populated with \a buffers. This is typically used to > - * implement buffer export, with all buffers added to the cache when they are > - * allocated. > + * Create a cache pre-populated with \a buffers. The entry is for \a numPlanes > + * planes V4L2 buffer. This is typically used to implement buffer export, with > + * all buffers added to the cache when they are allocated. > */ > -V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers) > - : lastUsedCounter_(1), missCounter_(0) > +V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers, > + unsigned int numPlanes) > + : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0) > { > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) > cache_.emplace_back(true, > lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel), > + numPlanes_, > *buffer.get()); > } > > @@ -237,6 +243,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer) > > cache_[use] = Entry(false, > lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel), > + numPlanes_, > buffer); > > return use; > @@ -257,24 +264,53 @@ V4L2BufferCache::Entry::Entry() > { > } > > -V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer) > +V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, > + unsigned int numPlanes, const FrameBuffer &buffer) > : free_(free), lastUsed_(lastUsed) > { > - for (const FrameBuffer::Plane &plane : buffer.planes()) > - planes_.emplace_back(plane); > + ASSERT(numPlanes <= buffer.planes().size()); > + unsigned int length = 0; > + for (const FrameBuffer::Plane &plane : buffer.planes()) { > + ASSERT(plane.offset == length); > + length += plane.length; > + > + if (planes_.size() < numPlanes) > + planes_.emplace_back(plane); > + } > + > + if (numPlanes == 1) > + planes_[0].length = length; > } > > bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > { > const std::vector<FrameBuffer::Plane> &planes = buffer.planes(); > > - if (planes_.size() != planes.size()) > + if (planes_.size() != planes.size() || planes_.size() == 1) > return false; > > - for (unsigned int i = 0; i < planes.size(); i++) > - if (planes_[i].fd != planes[i].fd.fd() || > - planes_[i].length != planes[i].length) > + if (planes_.size() == 1) { > + /* > + * planes_ is V4L2 single-planar format buffer. fds of > + * FrameBuffer::Plane must be identical and the sum of plane > + * size is the V4L2 buffer length. > + */ > + unsigned int length = 0; > + for (unsigned int i = 0; i < planes.size(); i++) { > + if (planes_[0].fd != planes[i].fd.fd()) > + return false; > + length += planes[i].length; > + } > + if (length != planes_[0].length) > return false; > + } else { > + /* planes_ is V4L2 multi-planar format buffer. */ > + for (unsigned int i = 0; i < planes.size(); i++) > + if (planes_[i].fd != planes[i].fd.fd() || > + planes_[i].length != planes[i].length) > + return false; > + } > + > return true; > } > > @@ -579,6 +615,12 @@ int V4L2VideoDevice::open() > << "Opened device " << caps_.bus_info() << ": " > << caps_.driver() << ": " << caps_.card(); > > + ret = getFormat(&format_); > + if (ret) { > + LOG(V4L2, Error) << "Failed to get format"; > + return ret; > + } > + > return 0; > } > > @@ -668,6 +710,12 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type) > << "Opened device " << caps_.bus_info() << ": " > << caps_.driver() << ": " << caps_.card(); > > + ret = getFormat(&format_); > + if (ret) { > + LOG(V4L2, Error) << "Failed to get format"; > + return ret; > + } > + > return 0; > } > > @@ -761,12 +809,19 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) > */ > int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format) > { > + int ret = 0; > if (caps_.isMeta()) > - return trySetFormatMeta(format, true); > + ret = trySetFormatMeta(format, true); > else if (caps_.isMultiplanar()) > - return trySetFormatMultiplane(format, true); > + ret = trySetFormatMultiplane(format, true); > else > - return trySetFormatSingleplane(format, true); > + ret = trySetFormatSingleplane(format, true); > + > + /* Cache the set format on success. */ > + if (ret == 0) > + format_ = *format; > + > + return ret; > } > > int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format) > @@ -1152,8 +1207,13 @@ int V4L2VideoDevice::requestBuffers(unsigned int count, > * successful return the driver's internal buffer management is initialized in > * MMAP mode, and the video device is ready to accept queueBuffer() calls. > * > - * The number of planes and the plane sizes for the allocation are determined > - * by the currently active format on the device as set by setFormat(). > + * The number of planes and their offsets and sizes are determined by the > + * currently active format on the device as set by setFormat(). They do not map > + * to the V4L2 buffer planes, but to colour planes of the pixel format. For > + * instance, if the active format is formats::NV12, the allocated FrameBuffer > + * instances will have two planes, for the luma and chroma components, > + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or > + * V4L2_PIX_FMT_NV12M. > * > * Buffers allocated with this function shall later be free with > * releaseBuffers(). If buffers have already been allocated with > @@ -1171,7 +1231,7 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count, > if (ret < 0) > return ret; > > - cache_ = new V4L2BufferCache(*buffers); > + cache_ = new V4L2BufferCache(*buffers, format_.planesCount); > memoryType_ = V4L2_MEMORY_MMAP; > > return ret; > @@ -1190,8 +1250,13 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count, > * usable with any V4L2 video device in DMABUF mode, or with other dmabuf > * importers. > * > - * The number of planes and the plane sizes for the allocation are determined > - * by the currently active format on the device as set by setFormat(). > + * The number of planes and their offsets and sizes are determined by the > + * currently active format on the device as set by setFormat(). They do not map > + * to the V4L2 buffer planes, but to colour planes of the pixel format. For > + * instance, if the active format is formats::NV12, the allocated FrameBuffer > + * instances will have two planes, for the luma and chroma components, > + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or > + * V4L2_PIX_FMT_NV12M. > * > * Multiple independent sets of buffers can be allocated with multiple calls to > * this function. Device-specific limitations may apply regarding the minimum > @@ -1289,12 +1354,32 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > * \todo Set the right offset once V4L2 API provides a way. > */ > plane.offset = 0; > - plane.length = multiPlanar ? > - buf.m.planes[nplane].length : buf.length; > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; > > planes.push_back(std::move(plane)); > } > > + const auto &info = PixelFormatInfo::info(format_.fourcc); > + if (info.isValid() && info.numPlanes() != numPlanes) { > + ASSERT(numPlanes == 1u); > + const size_t numColorPlanes = info.numPlanes(); > + planes.resize(numColorPlanes); > + const FileDescriptor &fd = planes[0].fd; > + size_t offset = 0; > + for (size_t i = 0; i < numColorPlanes; ++i) { > + planes[i].fd = fd; > + planes[i].offset = offset; > + > + /* \todo Take the V4L2 stride into account */ > + const unsigned int vertSubSample = > + info.planes[i].verticalSubSampling; > + planes[i].length = > + info.stride(format_.size.width, i, 1u) * > + ((format_.size.height + vertSubSample - 1) / vertSubSample); > + offset += planes[i].length; > + } > + } > + > return std::make_unique<FrameBuffer>(std::move(planes)); > } > > @@ -1352,7 +1437,7 @@ int V4L2VideoDevice::importBuffers(unsigned int count) > if (ret) > return ret; > > - cache_ = new V4L2BufferCache(count); > + cache_ = new V4L2BufferCache(count, format_.planesCount); > > LOG(V4L2, Debug) << "Prepared to import " << count << " buffers"; > > diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp > index b3f2bec1..48f748bc 100644 > --- a/test/v4l2_videodevice/buffer_cache.cpp > +++ b/test/v4l2_videodevice/buffer_cache.cpp > @@ -166,7 +166,7 @@ public: > * Test cache of same size as there are buffers, the cache is > * created from a list of buffers and will be pre-populated. > */ > - V4L2BufferCache cacheFromBuffers(buffers); > + V4L2BufferCache cacheFromBuffers(buffers, 1u); > > if (testSequential(&cacheFromBuffers, buffers) != TestPass) > return TestFail; > @@ -181,7 +181,7 @@ public: > * Test cache of same size as there are buffers, the cache is > * not pre-populated. > */ > - V4L2BufferCache cacheFromNumbers(numBuffers); > + V4L2BufferCache cacheFromNumbers(numBuffers, 1u); > > if (testSequential(&cacheFromNumbers, buffers) != TestPass) > return TestFail; > @@ -196,7 +196,7 @@ public: > * Test cache half the size of number of buffers used, the cache > * is not pre-populated. > */ > - V4L2BufferCache cacheHalf(numBuffers / 2); > + V4L2BufferCache cacheHalf(numBuffers / 2, 1u); > > if (testRandom(&cacheHalf, buffers) != TestPass) > return TestFail;
Hi Laurent, On Thu, Aug 26, 2021 at 8:32 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > Thank you for the patch. > > I think the subject line is a bit long ;-) > Oops... I somehow remove a blank line between the subject and message. :-) > I'll review this as part of the series right after "[PATCH v3 0/3] > Improve CameraBuffer implementation". > > On Thu, Aug 26, 2021 at 08:25:37PM +0900, Hirokazu Honda wrote: > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > include/libcamera/internal/v4l2_videodevice.h | 10 +- > > src/libcamera/v4l2_videodevice.cpp | 141 ++++++++++++++---- > > test/v4l2_videodevice/buffer_cache.cpp | 6 +- > > 3 files changed, 123 insertions(+), 34 deletions(-) > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > index e767ec84..bfda6726 100644 > > --- a/include/libcamera/internal/v4l2_videodevice.h > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > @@ -114,8 +114,9 @@ struct V4L2Capability final : v4l2_capability { > > class V4L2BufferCache > > { > > public: > > - V4L2BufferCache(unsigned int numEntries); > > - V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers); > > + V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes); > > + V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers, > > + unsigned int numPlanes); > > ~V4L2BufferCache(); > > > > int get(const FrameBuffer &buffer); > > @@ -126,7 +127,8 @@ private: > > { > > public: > > Entry(); > > - Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer); > > + Entry(bool free, uint64_t lastUsed, unsigned int numPlanes, > > + const FrameBuffer &buffer); > > > > bool operator==(const FrameBuffer &buffer) const; > > > > @@ -149,6 +151,7 @@ private: > > > > std::atomic<uint64_t> lastUsedCounter_; > > std::vector<Entry> cache_; > > + unsigned int numPlanes_; > > /* \todo Expose the miss counter through an instrumentation API. */ > > unsigned int missCounter_; > > }; > > @@ -242,6 +245,7 @@ private: > > FrameBuffer *dequeueBuffer(); > > > > V4L2Capability caps_; > > + V4L2DeviceFormat format_; > > > > enum v4l2_buf_type bufferType_; > > enum v4l2_memory memoryType_; > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > index 2ff25af2..f5f8741a 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -25,6 +25,7 @@ > > > > #include <libcamera/file_descriptor.h> > > > > +#include "libcamera/internal/formats.h" > > #include "libcamera/internal/media_device.h" > > #include "libcamera/internal/media_object.h" > > > > @@ -157,13 +158,15 @@ LOG_DECLARE_CATEGORY(V4L2) > > /** > > * \brief Create an empty cache with \a numEntries entries > > * \param[in] numEntries Number of entries to reserve in the cache > > + * \param[in] numPlanes Number of V4L2 buffer planes > > * > > - * Create a cache with \a numEntries entries all marked as unused. The entries > > - * will be populated as the cache is used. This is typically used to implement > > - * buffer import, with buffers added to the cache as they are queued. > > + * Create a cache with \a numEntries entries all marked as unused. The entry is > > + * for \a numPlanes planes V4L2 buffer. The entries will be populated as the > > + * cache is used. This is typically used to implement buffer import, with > > + * buffers added to the cache as they are queued. > > */ > > -V4L2BufferCache::V4L2BufferCache(unsigned int numEntries) > > - : lastUsedCounter_(1), missCounter_(0) > > +V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes) > > + : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0) > > { > > cache_.resize(numEntries); > > } > > @@ -171,17 +174,20 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries) > > /** > > * \brief Create a pre-populated cache > > * \param[in] buffers Array of buffers to pre-populated with > > + * \param[in] numPlanes Number of V4L2 buffer planes > > * > > - * Create a cache pre-populated with \a buffers. This is typically used to > > - * implement buffer export, with all buffers added to the cache when they are > > - * allocated. > > + * Create a cache pre-populated with \a buffers. The entry is for \a numPlanes > > + * planes V4L2 buffer. This is typically used to implement buffer export, with > > + * all buffers added to the cache when they are allocated. > > */ > > -V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers) > > - : lastUsedCounter_(1), missCounter_(0) > > +V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers, > > + unsigned int numPlanes) > > + : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0) > > { > > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) > > cache_.emplace_back(true, > > lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel), > > + numPlanes_, > > *buffer.get()); > > } > > > > @@ -237,6 +243,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer) > > > > cache_[use] = Entry(false, > > lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel), > > + numPlanes_, > > buffer); > > > > return use; > > @@ -257,24 +264,53 @@ V4L2BufferCache::Entry::Entry() > > { > > } > > > > -V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer) > > +V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, > > + unsigned int numPlanes, const FrameBuffer &buffer) > > : free_(free), lastUsed_(lastUsed) > > { > > - for (const FrameBuffer::Plane &plane : buffer.planes()) > > - planes_.emplace_back(plane); > > + ASSERT(numPlanes <= buffer.planes().size()); > > + unsigned int length = 0; > > + for (const FrameBuffer::Plane &plane : buffer.planes()) { > > + ASSERT(plane.offset == length); > > + length += plane.length; > > + > > + if (planes_.size() < numPlanes) > > + planes_.emplace_back(plane); > > + } > > + > > + if (numPlanes == 1) > > + planes_[0].length = length; > > } > > > > bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > { > > const std::vector<FrameBuffer::Plane> &planes = buffer.planes(); > > > > - if (planes_.size() != planes.size()) > > + if (planes_.size() != planes.size() || planes_.size() == 1) > > return false; > > > > - for (unsigned int i = 0; i < planes.size(); i++) > > - if (planes_[i].fd != planes[i].fd.fd() || > > - planes_[i].length != planes[i].length) > > + if (planes_.size() == 1) { > > + /* > > + * planes_ is V4L2 single-planar format buffer. fds of > > + * FrameBuffer::Plane must be identical and the sum of plane > > + * size is the V4L2 buffer length. > > + */ > > + unsigned int length = 0; > > + for (unsigned int i = 0; i < planes.size(); i++) { > > + if (planes_[0].fd != planes[i].fd.fd()) > > + return false; > > + length += planes[i].length; > > + } > > + if (length != planes_[0].length) > > return false; > > + } else { > > + /* planes_ is V4L2 multi-planar format buffer. */ > > + for (unsigned int i = 0; i < planes.size(); i++) > > + if (planes_[i].fd != planes[i].fd.fd() || > > + planes_[i].length != planes[i].length) > > + return false; > > + } > > + > > return true; > > } > > > > @@ -579,6 +615,12 @@ int V4L2VideoDevice::open() > > << "Opened device " << caps_.bus_info() << ": " > > << caps_.driver() << ": " << caps_.card(); > > > > + ret = getFormat(&format_); > > + if (ret) { > > + LOG(V4L2, Error) << "Failed to get format"; > > + return ret; > > + } > > + > > return 0; > > } > > > > @@ -668,6 +710,12 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type) > > << "Opened device " << caps_.bus_info() << ": " > > << caps_.driver() << ": " << caps_.card(); > > > > + ret = getFormat(&format_); > > + if (ret) { > > + LOG(V4L2, Error) << "Failed to get format"; > > + return ret; > > + } > > + > > return 0; > > } > > > > @@ -761,12 +809,19 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) > > */ > > int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format) > > { > > + int ret = 0; > > if (caps_.isMeta()) > > - return trySetFormatMeta(format, true); > > + ret = trySetFormatMeta(format, true); > > else if (caps_.isMultiplanar()) > > - return trySetFormatMultiplane(format, true); > > + ret = trySetFormatMultiplane(format, true); > > else > > - return trySetFormatSingleplane(format, true); > > + ret = trySetFormatSingleplane(format, true); > > + > > + /* Cache the set format on success. */ > > + if (ret == 0) > > + format_ = *format; > > + > > + return ret; > > } > > > > int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format) > > @@ -1152,8 +1207,13 @@ int V4L2VideoDevice::requestBuffers(unsigned int count, > > * successful return the driver's internal buffer management is initialized in > > * MMAP mode, and the video device is ready to accept queueBuffer() calls. > > * > > - * The number of planes and the plane sizes for the allocation are determined > > - * by the currently active format on the device as set by setFormat(). > > + * The number of planes and their offsets and sizes are determined by the > > + * currently active format on the device as set by setFormat(). They do not map > > + * to the V4L2 buffer planes, but to colour planes of the pixel format. For > > + * instance, if the active format is formats::NV12, the allocated FrameBuffer > > + * instances will have two planes, for the luma and chroma components, > > + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or > > + * V4L2_PIX_FMT_NV12M. > > * > > * Buffers allocated with this function shall later be free with > > * releaseBuffers(). If buffers have already been allocated with > > @@ -1171,7 +1231,7 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count, > > if (ret < 0) > > return ret; > > > > - cache_ = new V4L2BufferCache(*buffers); > > + cache_ = new V4L2BufferCache(*buffers, format_.planesCount); > > memoryType_ = V4L2_MEMORY_MMAP; > > > > return ret; > > @@ -1190,8 +1250,13 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count, > > * usable with any V4L2 video device in DMABUF mode, or with other dmabuf > > * importers. > > * > > - * The number of planes and the plane sizes for the allocation are determined > > - * by the currently active format on the device as set by setFormat(). > > + * The number of planes and their offsets and sizes are determined by the > > + * currently active format on the device as set by setFormat(). They do not map > > + * to the V4L2 buffer planes, but to colour planes of the pixel format. For > > + * instance, if the active format is formats::NV12, the allocated FrameBuffer > > + * instances will have two planes, for the luma and chroma components, > > + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or > > + * V4L2_PIX_FMT_NV12M. > > * > > * Multiple independent sets of buffers can be allocated with multiple calls to > > * this function. Device-specific limitations may apply regarding the minimum > > @@ -1289,12 +1354,32 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > > * \todo Set the right offset once V4L2 API provides a way. > > */ > > plane.offset = 0; > > - plane.length = multiPlanar ? > > - buf.m.planes[nplane].length : buf.length; > > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; > > > > planes.push_back(std::move(plane)); > > } > > > > + const auto &info = PixelFormatInfo::info(format_.fourcc); > > + if (info.isValid() && info.numPlanes() != numPlanes) { > > + ASSERT(numPlanes == 1u); > > + const size_t numColorPlanes = info.numPlanes(); > > + planes.resize(numColorPlanes); > > + const FileDescriptor &fd = planes[0].fd; > > + size_t offset = 0; > > + for (size_t i = 0; i < numColorPlanes; ++i) { > > + planes[i].fd = fd; > > + planes[i].offset = offset; > > + > > + /* \todo Take the V4L2 stride into account */ > > + const unsigned int vertSubSample = > > + info.planes[i].verticalSubSampling; > > + planes[i].length = > > + info.stride(format_.size.width, i, 1u) * > > + ((format_.size.height + vertSubSample - 1) / vertSubSample); > > + offset += planes[i].length; > > + } > > + } > > + > > return std::make_unique<FrameBuffer>(std::move(planes)); > > } > > > > @@ -1352,7 +1437,7 @@ int V4L2VideoDevice::importBuffers(unsigned int count) > > if (ret) > > return ret; > > > > - cache_ = new V4L2BufferCache(count); > > + cache_ = new V4L2BufferCache(count, format_.planesCount); > > > > LOG(V4L2, Debug) << "Prepared to import " << count << " buffers"; > > > > diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp > > index b3f2bec1..48f748bc 100644 > > --- a/test/v4l2_videodevice/buffer_cache.cpp > > +++ b/test/v4l2_videodevice/buffer_cache.cpp > > @@ -166,7 +166,7 @@ public: > > * Test cache of same size as there are buffers, the cache is > > * created from a list of buffers and will be pre-populated. > > */ > > - V4L2BufferCache cacheFromBuffers(buffers); > > + V4L2BufferCache cacheFromBuffers(buffers, 1u); > > > > if (testSequential(&cacheFromBuffers, buffers) != TestPass) > > return TestFail; > > @@ -181,7 +181,7 @@ public: > > * Test cache of same size as there are buffers, the cache is > > * not pre-populated. > > */ > > - V4L2BufferCache cacheFromNumbers(numBuffers); > > + V4L2BufferCache cacheFromNumbers(numBuffers, 1u); > > > > if (testSequential(&cacheFromNumbers, buffers) != TestPass) > > return TestFail; > > @@ -196,7 +196,7 @@ public: > > * Test cache half the size of number of buffers used, the cache > > * is not pre-populated. > > */ > > - V4L2BufferCache cacheHalf(numBuffers / 2); > > + V4L2BufferCache cacheHalf(numBuffers / 2, 1u); > > > > if (testRandom(&cacheHalf, buffers) != TestPass) > > return TestFail; > > -- > Regards, > > Laurent Pinchart
Hi Hiro, Thank you for the patch. On Thu, Aug 26, 2021 at 08:25:37PM +0900, Hirokazu Honda wrote: > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > include/libcamera/internal/v4l2_videodevice.h | 10 +- > src/libcamera/v4l2_videodevice.cpp | 141 ++++++++++++++---- > test/v4l2_videodevice/buffer_cache.cpp | 6 +- > 3 files changed, 123 insertions(+), 34 deletions(-) > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > index e767ec84..bfda6726 100644 > --- a/include/libcamera/internal/v4l2_videodevice.h > +++ b/include/libcamera/internal/v4l2_videodevice.h > @@ -114,8 +114,9 @@ struct V4L2Capability final : v4l2_capability { > class V4L2BufferCache > { > public: > - V4L2BufferCache(unsigned int numEntries); > - V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers); > + V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes); > + V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers, > + unsigned int numPlanes); > ~V4L2BufferCache(); > > int get(const FrameBuffer &buffer); > @@ -126,7 +127,8 @@ private: > { > public: > Entry(); > - Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer); > + Entry(bool free, uint64_t lastUsed, unsigned int numPlanes, > + const FrameBuffer &buffer); > > bool operator==(const FrameBuffer &buffer) const; > > @@ -149,6 +151,7 @@ private: > > std::atomic<uint64_t> lastUsedCounter_; > std::vector<Entry> cache_; > + unsigned int numPlanes_; > /* \todo Expose the miss counter through an instrumentation API. */ > unsigned int missCounter_; > }; > @@ -242,6 +245,7 @@ private: > FrameBuffer *dequeueBuffer(); > > V4L2Capability caps_; > + V4L2DeviceFormat format_; > > enum v4l2_buf_type bufferType_; > enum v4l2_memory memoryType_; > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 2ff25af2..f5f8741a 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -25,6 +25,7 @@ > > #include <libcamera/file_descriptor.h> > > +#include "libcamera/internal/formats.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/media_object.h" > > @@ -157,13 +158,15 @@ LOG_DECLARE_CATEGORY(V4L2) > /** > * \brief Create an empty cache with \a numEntries entries > * \param[in] numEntries Number of entries to reserve in the cache > + * \param[in] numPlanes Number of V4L2 buffer planes > * > - * Create a cache with \a numEntries entries all marked as unused. The entries > - * will be populated as the cache is used. This is typically used to implement > - * buffer import, with buffers added to the cache as they are queued. > + * Create a cache with \a numEntries entries all marked as unused. The entry is > + * for \a numPlanes planes V4L2 buffer. The entries will be populated as the > + * cache is used. This is typically used to implement buffer import, with > + * buffers added to the cache as they are queued. > */ > -V4L2BufferCache::V4L2BufferCache(unsigned int numEntries) > - : lastUsedCounter_(1), missCounter_(0) > +V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes) > + : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0) > { > cache_.resize(numEntries); > } > @@ -171,17 +174,20 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries) > /** > * \brief Create a pre-populated cache > * \param[in] buffers Array of buffers to pre-populated with > + * \param[in] numPlanes Number of V4L2 buffer planes > * > - * Create a cache pre-populated with \a buffers. This is typically used to > - * implement buffer export, with all buffers added to the cache when they are > - * allocated. > + * Create a cache pre-populated with \a buffers. The entry is for \a numPlanes > + * planes V4L2 buffer. This is typically used to implement buffer export, with > + * all buffers added to the cache when they are allocated. > */ > -V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers) > - : lastUsedCounter_(1), missCounter_(0) > +V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers, > + unsigned int numPlanes) > + : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0) > { > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) > cache_.emplace_back(true, > lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel), > + numPlanes_, > *buffer.get()); > } > > @@ -237,6 +243,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer) > > cache_[use] = Entry(false, > lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel), > + numPlanes_, > buffer); > > return use; > @@ -257,24 +264,53 @@ V4L2BufferCache::Entry::Entry() > { > } > > -V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer) > +V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, > + unsigned int numPlanes, const FrameBuffer &buffer) > : free_(free), lastUsed_(lastUsed) > { > - for (const FrameBuffer::Plane &plane : buffer.planes()) > - planes_.emplace_back(plane); > + ASSERT(numPlanes <= buffer.planes().size()); > + unsigned int length = 0; > + for (const FrameBuffer::Plane &plane : buffer.planes()) { > + ASSERT(plane.offset == length); > + length += plane.length; > + > + if (planes_.size() < numPlanes) > + planes_.emplace_back(plane); > + } > + > + if (numPlanes == 1) > + planes_[0].length = length; > } > > bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > { > const std::vector<FrameBuffer::Plane> &planes = buffer.planes(); > > - if (planes_.size() != planes.size()) > + if (planes_.size() != planes.size() || planes_.size() == 1) > return false; > > - for (unsigned int i = 0; i < planes.size(); i++) > - if (planes_[i].fd != planes[i].fd.fd() || > - planes_[i].length != planes[i].length) > + if (planes_.size() == 1) { > + /* > + * planes_ is V4L2 single-planar format buffer. fds of > + * FrameBuffer::Plane must be identical and the sum of plane > + * size is the V4L2 buffer length. > + */ > + unsigned int length = 0; > + for (unsigned int i = 0; i < planes.size(); i++) { > + if (planes_[0].fd != planes[i].fd.fd()) > + return false; > + length += planes[i].length; > + } > + if (length != planes_[0].length) > return false; > + } else { > + /* planes_ is V4L2 multi-planar format buffer. */ > + for (unsigned int i = 0; i < planes.size(); i++) > + if (planes_[i].fd != planes[i].fd.fd() || > + planes_[i].length != planes[i].length) > + return false; > + } > + > return true; Is all of this actually needed ? What's the issue if we store, in the cache entries, the colour planes from FrameBuffer instance of the V4L2 buffer planes ? > } > > @@ -579,6 +615,12 @@ int V4L2VideoDevice::open() > << "Opened device " << caps_.bus_info() << ": " > << caps_.driver() << ": " << caps_.card(); > > + ret = getFormat(&format_); > + if (ret) { > + LOG(V4L2, Error) << "Failed to get format"; > + return ret; > + } > + > return 0; > } > > @@ -668,6 +710,12 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type) > << "Opened device " << caps_.bus_info() << ": " > << caps_.driver() << ": " << caps_.card(); > > + ret = getFormat(&format_); > + if (ret) { > + LOG(V4L2, Error) << "Failed to get format"; > + return ret; > + } > + > return 0; > } > > @@ -761,12 +809,19 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) > */ > int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format) > { > + int ret = 0; > if (caps_.isMeta()) > - return trySetFormatMeta(format, true); > + ret = trySetFormatMeta(format, true); > else if (caps_.isMultiplanar()) > - return trySetFormatMultiplane(format, true); > + ret = trySetFormatMultiplane(format, true); > else > - return trySetFormatSingleplane(format, true); > + ret = trySetFormatSingleplane(format, true); > + > + /* Cache the set format on success. */ > + if (ret == 0) > + format_ = *format; > + > + return ret; How about if (ret) return ret; format_ = *format; return 0; We usually return early on error, and handle the normal case in the top scope of the function. > } > > int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format) > @@ -1152,8 +1207,13 @@ int V4L2VideoDevice::requestBuffers(unsigned int count, > * successful return the driver's internal buffer management is initialized in > * MMAP mode, and the video device is ready to accept queueBuffer() calls. > * > - * The number of planes and the plane sizes for the allocation are determined > - * by the currently active format on the device as set by setFormat(). > + * The number of planes and their offsets and sizes are determined by the > + * currently active format on the device as set by setFormat(). They do not map > + * to the V4L2 buffer planes, but to colour planes of the pixel format. For > + * instance, if the active format is formats::NV12, the allocated FrameBuffer > + * instances will have two planes, for the luma and chroma components, > + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or > + * V4L2_PIX_FMT_NV12M. > * > * Buffers allocated with this function shall later be free with > * releaseBuffers(). If buffers have already been allocated with > @@ -1171,7 +1231,7 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count, > if (ret < 0) > return ret; > > - cache_ = new V4L2BufferCache(*buffers); > + cache_ = new V4L2BufferCache(*buffers, format_.planesCount); > memoryType_ = V4L2_MEMORY_MMAP; > > return ret; > @@ -1190,8 +1250,13 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count, > * usable with any V4L2 video device in DMABUF mode, or with other dmabuf > * importers. > * > - * The number of planes and the plane sizes for the allocation are determined > - * by the currently active format on the device as set by setFormat(). > + * The number of planes and their offsets and sizes are determined by the > + * currently active format on the device as set by setFormat(). They do not map > + * to the V4L2 buffer planes, but to colour planes of the pixel format. For > + * instance, if the active format is formats::NV12, the allocated FrameBuffer > + * instances will have two planes, for the luma and chroma components, > + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or > + * V4L2_PIX_FMT_NV12M. > * > * Multiple independent sets of buffers can be allocated with multiple calls to > * this function. Device-specific limitations may apply regarding the minimum > @@ -1289,12 +1354,32 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > * \todo Set the right offset once V4L2 API provides a way. > */ > plane.offset = 0; > - plane.length = multiPlanar ? > - buf.m.planes[nplane].length : buf.length; > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; > > planes.push_back(std::move(plane)); > } > > + const auto &info = PixelFormatInfo::info(format_.fourcc); > + if (info.isValid() && info.numPlanes() != numPlanes) { > + ASSERT(numPlanes == 1u); > + const size_t numColorPlanes = info.numPlanes(); > + planes.resize(numColorPlanes); > + const FileDescriptor &fd = planes[0].fd; > + size_t offset = 0; > + for (size_t i = 0; i < numColorPlanes; ++i) { > + planes[i].fd = fd; > + planes[i].offset = offset; > + > + /* \todo Take the V4L2 stride into account */ > + const unsigned int vertSubSample = > + info.planes[i].verticalSubSampling; > + planes[i].length = > + info.stride(format_.size.width, i, 1u) * > + ((format_.size.height + vertSubSample - 1) / vertSubSample); > + offset += planes[i].length; > + } > + } > + > return std::make_unique<FrameBuffer>(std::move(planes)); > } > > @@ -1352,7 +1437,7 @@ int V4L2VideoDevice::importBuffers(unsigned int count) > if (ret) > return ret; > > - cache_ = new V4L2BufferCache(count); > + cache_ = new V4L2BufferCache(count, format_.planesCount); > > LOG(V4L2, Debug) << "Prepared to import " << count << " buffers"; > > diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp > index b3f2bec1..48f748bc 100644 > --- a/test/v4l2_videodevice/buffer_cache.cpp > +++ b/test/v4l2_videodevice/buffer_cache.cpp > @@ -166,7 +166,7 @@ public: > * Test cache of same size as there are buffers, the cache is > * created from a list of buffers and will be pre-populated. > */ > - V4L2BufferCache cacheFromBuffers(buffers); > + V4L2BufferCache cacheFromBuffers(buffers, 1u); > > if (testSequential(&cacheFromBuffers, buffers) != TestPass) > return TestFail; > @@ -181,7 +181,7 @@ public: > * Test cache of same size as there are buffers, the cache is > * not pre-populated. > */ > - V4L2BufferCache cacheFromNumbers(numBuffers); > + V4L2BufferCache cacheFromNumbers(numBuffers, 1u); > > if (testSequential(&cacheFromNumbers, buffers) != TestPass) > return TestFail; > @@ -196,7 +196,7 @@ public: > * Test cache half the size of number of buffers used, the cache > * is not pre-populated. > */ > - V4L2BufferCache cacheHalf(numBuffers / 2); > + V4L2BufferCache cacheHalf(numBuffers / 2, 1u); > > if (testRandom(&cacheHalf, buffers) != TestPass) > return TestFail; I'm afraid the test now fails :-S Could you have a look at that ?
Hi Laurent, On Thu, Aug 26, 2021 at 10:03 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > Thank you for the patch. > > On Thu, Aug 26, 2021 at 08:25:37PM +0900, Hirokazu Honda wrote: > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > include/libcamera/internal/v4l2_videodevice.h | 10 +- > > src/libcamera/v4l2_videodevice.cpp | 141 ++++++++++++++---- > > test/v4l2_videodevice/buffer_cache.cpp | 6 +- > > 3 files changed, 123 insertions(+), 34 deletions(-) > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > index e767ec84..bfda6726 100644 > > --- a/include/libcamera/internal/v4l2_videodevice.h > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > @@ -114,8 +114,9 @@ struct V4L2Capability final : v4l2_capability { > > class V4L2BufferCache > > { > > public: > > - V4L2BufferCache(unsigned int numEntries); > > - V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers); > > + V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes); > > + V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers, > > + unsigned int numPlanes); > > ~V4L2BufferCache(); > > > > int get(const FrameBuffer &buffer); > > @@ -126,7 +127,8 @@ private: > > { > > public: > > Entry(); > > - Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer); > > + Entry(bool free, uint64_t lastUsed, unsigned int numPlanes, > > + const FrameBuffer &buffer); > > > > bool operator==(const FrameBuffer &buffer) const; > > > > @@ -149,6 +151,7 @@ private: > > > > std::atomic<uint64_t> lastUsedCounter_; > > std::vector<Entry> cache_; > > + unsigned int numPlanes_; > > /* \todo Expose the miss counter through an instrumentation API. */ > > unsigned int missCounter_; > > }; > > @@ -242,6 +245,7 @@ private: > > FrameBuffer *dequeueBuffer(); > > > > V4L2Capability caps_; > > + V4L2DeviceFormat format_; > > > > enum v4l2_buf_type bufferType_; > > enum v4l2_memory memoryType_; > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > index 2ff25af2..f5f8741a 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -25,6 +25,7 @@ > > > > #include <libcamera/file_descriptor.h> > > > > +#include "libcamera/internal/formats.h" > > #include "libcamera/internal/media_device.h" > > #include "libcamera/internal/media_object.h" > > > > @@ -157,13 +158,15 @@ LOG_DECLARE_CATEGORY(V4L2) > > /** > > * \brief Create an empty cache with \a numEntries entries > > * \param[in] numEntries Number of entries to reserve in the cache > > + * \param[in] numPlanes Number of V4L2 buffer planes > > * > > - * Create a cache with \a numEntries entries all marked as unused. The entries > > - * will be populated as the cache is used. This is typically used to implement > > - * buffer import, with buffers added to the cache as they are queued. > > + * Create a cache with \a numEntries entries all marked as unused. The entry is > > + * for \a numPlanes planes V4L2 buffer. The entries will be populated as the > > + * cache is used. This is typically used to implement buffer import, with > > + * buffers added to the cache as they are queued. > > */ > > -V4L2BufferCache::V4L2BufferCache(unsigned int numEntries) > > - : lastUsedCounter_(1), missCounter_(0) > > +V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes) > > + : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0) > > { > > cache_.resize(numEntries); > > } > > @@ -171,17 +174,20 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries) > > /** > > * \brief Create a pre-populated cache > > * \param[in] buffers Array of buffers to pre-populated with > > + * \param[in] numPlanes Number of V4L2 buffer planes > > * > > - * Create a cache pre-populated with \a buffers. This is typically used to > > - * implement buffer export, with all buffers added to the cache when they are > > - * allocated. > > + * Create a cache pre-populated with \a buffers. The entry is for \a numPlanes > > + * planes V4L2 buffer. This is typically used to implement buffer export, with > > + * all buffers added to the cache when they are allocated. > > */ > > -V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers) > > - : lastUsedCounter_(1), missCounter_(0) > > +V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers, > > + unsigned int numPlanes) > > + : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0) > > { > > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) > > cache_.emplace_back(true, > > lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel), > > + numPlanes_, > > *buffer.get()); > > } > > > > @@ -237,6 +243,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer) > > > > cache_[use] = Entry(false, > > lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel), > > + numPlanes_, > > buffer); > > > > return use; > > @@ -257,24 +264,53 @@ V4L2BufferCache::Entry::Entry() > > { > > } > > > > -V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer) > > +V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, > > + unsigned int numPlanes, const FrameBuffer &buffer) > > : free_(free), lastUsed_(lastUsed) > > { > > - for (const FrameBuffer::Plane &plane : buffer.planes()) > > - planes_.emplace_back(plane); > > + ASSERT(numPlanes <= buffer.planes().size()); > > + unsigned int length = 0; > > + for (const FrameBuffer::Plane &plane : buffer.planes()) { > > + ASSERT(plane.offset == length); > > + length += plane.length; > > + > > + if (planes_.size() < numPlanes) > > + planes_.emplace_back(plane); > > + } > > + > > + if (numPlanes == 1) > > + planes_[0].length = length; > > } > > > > bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > { > > const std::vector<FrameBuffer::Plane> &planes = buffer.planes(); > > > > - if (planes_.size() != planes.size()) > > + if (planes_.size() != planes.size() || planes_.size() == 1) > > return false; > > > > - for (unsigned int i = 0; i < planes.size(); i++) > > - if (planes_[i].fd != planes[i].fd.fd() || > > - planes_[i].length != planes[i].length) > > + if (planes_.size() == 1) { > > + /* > > + * planes_ is V4L2 single-planar format buffer. fds of > > + * FrameBuffer::Plane must be identical and the sum of plane > > + * size is the V4L2 buffer length. > > + */ > > + unsigned int length = 0; > > + for (unsigned int i = 0; i < planes.size(); i++) { > > + if (planes_[0].fd != planes[i].fd.fd()) > > + return false; > > + length += planes[i].length; > > + } > > + if (length != planes_[0].length) > > return false; > > + } else { > > + /* planes_ is V4L2 multi-planar format buffer. */ > > + for (unsigned int i = 0; i < planes.size(); i++) > > + if (planes_[i].fd != planes[i].fd.fd() || > > + planes_[i].length != planes[i].length) > > + return false; > > + } > > + > > return true; > > Is all of this actually needed ? What's the issue if we store, in the > cache entries, the colour planes from FrameBuffer instance of the V4L2 > buffer planes ? > Hmm, you're right. As far as we compare the V4L2 buffer planes, the conversion is unnecessary. I reverted this part of the change. > > } > > > > @@ -579,6 +615,12 @@ int V4L2VideoDevice::open() > > << "Opened device " << caps_.bus_info() << ": " > > << caps_.driver() << ": " << caps_.card(); > > > > + ret = getFormat(&format_); > > + if (ret) { > > + LOG(V4L2, Error) << "Failed to get format"; > > + return ret; > > + } > > + > > return 0; > > } > > > > @@ -668,6 +710,12 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type) > > << "Opened device " << caps_.bus_info() << ": " > > << caps_.driver() << ": " << caps_.card(); > > > > + ret = getFormat(&format_); > > + if (ret) { > > + LOG(V4L2, Error) << "Failed to get format"; > > + return ret; > > + } > > + > > return 0; > > } > > > > @@ -761,12 +809,19 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) > > */ > > int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format) > > { > > + int ret = 0; > > if (caps_.isMeta()) > > - return trySetFormatMeta(format, true); > > + ret = trySetFormatMeta(format, true); > > else if (caps_.isMultiplanar()) > > - return trySetFormatMultiplane(format, true); > > + ret = trySetFormatMultiplane(format, true); > > else > > - return trySetFormatSingleplane(format, true); > > + ret = trySetFormatSingleplane(format, true); > > + > > + /* Cache the set format on success. */ > > + if (ret == 0) > > + format_ = *format; > > + > > + return ret; > > How about > > if (ret) > return ret; > > format_ = *format; > return 0; > > We usually return early on error, and handle the normal case in the top > scope of the function. > > > } > > > > int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format) > > @@ -1152,8 +1207,13 @@ int V4L2VideoDevice::requestBuffers(unsigned int count, > > * successful return the driver's internal buffer management is initialized in > > * MMAP mode, and the video device is ready to accept queueBuffer() calls. > > * > > - * The number of planes and the plane sizes for the allocation are determined > > - * by the currently active format on the device as set by setFormat(). > > + * The number of planes and their offsets and sizes are determined by the > > + * currently active format on the device as set by setFormat(). They do not map > > + * to the V4L2 buffer planes, but to colour planes of the pixel format. For > > + * instance, if the active format is formats::NV12, the allocated FrameBuffer > > + * instances will have two planes, for the luma and chroma components, > > + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or > > + * V4L2_PIX_FMT_NV12M. > > * > > * Buffers allocated with this function shall later be free with > > * releaseBuffers(). If buffers have already been allocated with > > @@ -1171,7 +1231,7 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count, > > if (ret < 0) > > return ret; > > > > - cache_ = new V4L2BufferCache(*buffers); > > + cache_ = new V4L2BufferCache(*buffers, format_.planesCount); > > memoryType_ = V4L2_MEMORY_MMAP; > > > > return ret; > > @@ -1190,8 +1250,13 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count, > > * usable with any V4L2 video device in DMABUF mode, or with other dmabuf > > * importers. > > * > > - * The number of planes and the plane sizes for the allocation are determined > > - * by the currently active format on the device as set by setFormat(). > > + * The number of planes and their offsets and sizes are determined by the > > + * currently active format on the device as set by setFormat(). They do not map > > + * to the V4L2 buffer planes, but to colour planes of the pixel format. For > > + * instance, if the active format is formats::NV12, the allocated FrameBuffer > > + * instances will have two planes, for the luma and chroma components, > > + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or > > + * V4L2_PIX_FMT_NV12M. > > * > > * Multiple independent sets of buffers can be allocated with multiple calls to > > * this function. Device-specific limitations may apply regarding the minimum > > @@ -1289,12 +1354,32 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > > * \todo Set the right offset once V4L2 API provides a way. > > */ > > plane.offset = 0; > > - plane.length = multiPlanar ? > > - buf.m.planes[nplane].length : buf.length; > > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; > > > > planes.push_back(std::move(plane)); > > } > > > > + const auto &info = PixelFormatInfo::info(format_.fourcc); > > + if (info.isValid() && info.numPlanes() != numPlanes) { > > + ASSERT(numPlanes == 1u); > > + const size_t numColorPlanes = info.numPlanes(); > > + planes.resize(numColorPlanes); > > + const FileDescriptor &fd = planes[0].fd; > > + size_t offset = 0; > > + for (size_t i = 0; i < numColorPlanes; ++i) { > > + planes[i].fd = fd; > > + planes[i].offset = offset; > > + > > + /* \todo Take the V4L2 stride into account */ > > + const unsigned int vertSubSample = > > + info.planes[i].verticalSubSampling; > > + planes[i].length = > > + info.stride(format_.size.width, i, 1u) * > > + ((format_.size.height + vertSubSample - 1) / vertSubSample); > > + offset += planes[i].length; > > + } > > + } > > + > > return std::make_unique<FrameBuffer>(std::move(planes)); > > } > > > > @@ -1352,7 +1437,7 @@ int V4L2VideoDevice::importBuffers(unsigned int count) > > if (ret) > > return ret; > > > > - cache_ = new V4L2BufferCache(count); > > + cache_ = new V4L2BufferCache(count, format_.planesCount); > > > > LOG(V4L2, Debug) << "Prepared to import " << count << " buffers"; > > > > diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp > > index b3f2bec1..48f748bc 100644 > > --- a/test/v4l2_videodevice/buffer_cache.cpp > > +++ b/test/v4l2_videodevice/buffer_cache.cpp > > @@ -166,7 +166,7 @@ public: > > * Test cache of same size as there are buffers, the cache is > > * created from a list of buffers and will be pre-populated. > > */ > > - V4L2BufferCache cacheFromBuffers(buffers); > > + V4L2BufferCache cacheFromBuffers(buffers, 1u); > > > > if (testSequential(&cacheFromBuffers, buffers) != TestPass) > > return TestFail; > > @@ -181,7 +181,7 @@ public: > > * Test cache of same size as there are buffers, the cache is > > * not pre-populated. > > */ > > - V4L2BufferCache cacheFromNumbers(numBuffers); > > + V4L2BufferCache cacheFromNumbers(numBuffers, 1u); > > > > if (testSequential(&cacheFromNumbers, buffers) != TestPass) > > return TestFail; > > @@ -196,7 +196,7 @@ public: > > * Test cache half the size of number of buffers used, the cache > > * is not pre-populated. > > */ > > - V4L2BufferCache cacheHalf(numBuffers / 2); > > + V4L2BufferCache cacheHalf(numBuffers / 2, 1u); > > > > if (testRandom(&cacheHalf, buffers) != TestPass) > > return TestFail; > > I'm afraid the test now fails :-S Could you have a look at that ? > Sorry, I don't have a vivid environment. I hope the issue is gone by reverting V4L2BufferCache change. -Hiro > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h index e767ec84..bfda6726 100644 --- a/include/libcamera/internal/v4l2_videodevice.h +++ b/include/libcamera/internal/v4l2_videodevice.h @@ -114,8 +114,9 @@ struct V4L2Capability final : v4l2_capability { class V4L2BufferCache { public: - V4L2BufferCache(unsigned int numEntries); - V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers); + V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes); + V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers, + unsigned int numPlanes); ~V4L2BufferCache(); int get(const FrameBuffer &buffer); @@ -126,7 +127,8 @@ private: { public: Entry(); - Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer); + Entry(bool free, uint64_t lastUsed, unsigned int numPlanes, + const FrameBuffer &buffer); bool operator==(const FrameBuffer &buffer) const; @@ -149,6 +151,7 @@ private: std::atomic<uint64_t> lastUsedCounter_; std::vector<Entry> cache_; + unsigned int numPlanes_; /* \todo Expose the miss counter through an instrumentation API. */ unsigned int missCounter_; }; @@ -242,6 +245,7 @@ private: FrameBuffer *dequeueBuffer(); V4L2Capability caps_; + V4L2DeviceFormat format_; enum v4l2_buf_type bufferType_; enum v4l2_memory memoryType_; diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 2ff25af2..f5f8741a 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -25,6 +25,7 @@ #include <libcamera/file_descriptor.h> +#include "libcamera/internal/formats.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/media_object.h" @@ -157,13 +158,15 @@ LOG_DECLARE_CATEGORY(V4L2) /** * \brief Create an empty cache with \a numEntries entries * \param[in] numEntries Number of entries to reserve in the cache + * \param[in] numPlanes Number of V4L2 buffer planes * - * Create a cache with \a numEntries entries all marked as unused. The entries - * will be populated as the cache is used. This is typically used to implement - * buffer import, with buffers added to the cache as they are queued. + * Create a cache with \a numEntries entries all marked as unused. The entry is + * for \a numPlanes planes V4L2 buffer. The entries will be populated as the + * cache is used. This is typically used to implement buffer import, with + * buffers added to the cache as they are queued. */ -V4L2BufferCache::V4L2BufferCache(unsigned int numEntries) - : lastUsedCounter_(1), missCounter_(0) +V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes) + : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0) { cache_.resize(numEntries); } @@ -171,17 +174,20 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries) /** * \brief Create a pre-populated cache * \param[in] buffers Array of buffers to pre-populated with + * \param[in] numPlanes Number of V4L2 buffer planes * - * Create a cache pre-populated with \a buffers. This is typically used to - * implement buffer export, with all buffers added to the cache when they are - * allocated. + * Create a cache pre-populated with \a buffers. The entry is for \a numPlanes + * planes V4L2 buffer. This is typically used to implement buffer export, with + * all buffers added to the cache when they are allocated. */ -V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers) - : lastUsedCounter_(1), missCounter_(0) +V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers, + unsigned int numPlanes) + : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0) { for (const std::unique_ptr<FrameBuffer> &buffer : buffers) cache_.emplace_back(true, lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel), + numPlanes_, *buffer.get()); } @@ -237,6 +243,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer) cache_[use] = Entry(false, lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel), + numPlanes_, buffer); return use; @@ -257,24 +264,53 @@ V4L2BufferCache::Entry::Entry() { } -V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer) +V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, + unsigned int numPlanes, const FrameBuffer &buffer) : free_(free), lastUsed_(lastUsed) { - for (const FrameBuffer::Plane &plane : buffer.planes()) - planes_.emplace_back(plane); + ASSERT(numPlanes <= buffer.planes().size()); + unsigned int length = 0; + for (const FrameBuffer::Plane &plane : buffer.planes()) { + ASSERT(plane.offset == length); + length += plane.length; + + if (planes_.size() < numPlanes) + planes_.emplace_back(plane); + } + + if (numPlanes == 1) + planes_[0].length = length; } bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const { const std::vector<FrameBuffer::Plane> &planes = buffer.planes(); - if (planes_.size() != planes.size()) + if (planes_.size() != planes.size() || planes_.size() == 1) return false; - for (unsigned int i = 0; i < planes.size(); i++) - if (planes_[i].fd != planes[i].fd.fd() || - planes_[i].length != planes[i].length) + if (planes_.size() == 1) { + /* + * planes_ is V4L2 single-planar format buffer. fds of + * FrameBuffer::Plane must be identical and the sum of plane + * size is the V4L2 buffer length. + */ + unsigned int length = 0; + for (unsigned int i = 0; i < planes.size(); i++) { + if (planes_[0].fd != planes[i].fd.fd()) + return false; + length += planes[i].length; + } + if (length != planes_[0].length) return false; + } else { + /* planes_ is V4L2 multi-planar format buffer. */ + for (unsigned int i = 0; i < planes.size(); i++) + if (planes_[i].fd != planes[i].fd.fd() || + planes_[i].length != planes[i].length) + return false; + } + return true; } @@ -579,6 +615,12 @@ int V4L2VideoDevice::open() << "Opened device " << caps_.bus_info() << ": " << caps_.driver() << ": " << caps_.card(); + ret = getFormat(&format_); + if (ret) { + LOG(V4L2, Error) << "Failed to get format"; + return ret; + } + return 0; } @@ -668,6 +710,12 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type) << "Opened device " << caps_.bus_info() << ": " << caps_.driver() << ": " << caps_.card(); + ret = getFormat(&format_); + if (ret) { + LOG(V4L2, Error) << "Failed to get format"; + return ret; + } + return 0; } @@ -761,12 +809,19 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format) */ int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format) { + int ret = 0; if (caps_.isMeta()) - return trySetFormatMeta(format, true); + ret = trySetFormatMeta(format, true); else if (caps_.isMultiplanar()) - return trySetFormatMultiplane(format, true); + ret = trySetFormatMultiplane(format, true); else - return trySetFormatSingleplane(format, true); + ret = trySetFormatSingleplane(format, true); + + /* Cache the set format on success. */ + if (ret == 0) + format_ = *format; + + return ret; } int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format) @@ -1152,8 +1207,13 @@ int V4L2VideoDevice::requestBuffers(unsigned int count, * successful return the driver's internal buffer management is initialized in * MMAP mode, and the video device is ready to accept queueBuffer() calls. * - * The number of planes and the plane sizes for the allocation are determined - * by the currently active format on the device as set by setFormat(). + * The number of planes and their offsets and sizes are determined by the + * currently active format on the device as set by setFormat(). They do not map + * to the V4L2 buffer planes, but to colour planes of the pixel format. For + * instance, if the active format is formats::NV12, the allocated FrameBuffer + * instances will have two planes, for the luma and chroma components, + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or + * V4L2_PIX_FMT_NV12M. * * Buffers allocated with this function shall later be free with * releaseBuffers(). If buffers have already been allocated with @@ -1171,7 +1231,7 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count, if (ret < 0) return ret; - cache_ = new V4L2BufferCache(*buffers); + cache_ = new V4L2BufferCache(*buffers, format_.planesCount); memoryType_ = V4L2_MEMORY_MMAP; return ret; @@ -1190,8 +1250,13 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count, * usable with any V4L2 video device in DMABUF mode, or with other dmabuf * importers. * - * The number of planes and the plane sizes for the allocation are determined - * by the currently active format on the device as set by setFormat(). + * The number of planes and their offsets and sizes are determined by the + * currently active format on the device as set by setFormat(). They do not map + * to the V4L2 buffer planes, but to colour planes of the pixel format. For + * instance, if the active format is formats::NV12, the allocated FrameBuffer + * instances will have two planes, for the luma and chroma components, + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or + * V4L2_PIX_FMT_NV12M. * * Multiple independent sets of buffers can be allocated with multiple calls to * this function. Device-specific limitations may apply regarding the minimum @@ -1289,12 +1354,32 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) * \todo Set the right offset once V4L2 API provides a way. */ plane.offset = 0; - plane.length = multiPlanar ? - buf.m.planes[nplane].length : buf.length; + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; planes.push_back(std::move(plane)); } + const auto &info = PixelFormatInfo::info(format_.fourcc); + if (info.isValid() && info.numPlanes() != numPlanes) { + ASSERT(numPlanes == 1u); + const size_t numColorPlanes = info.numPlanes(); + planes.resize(numColorPlanes); + const FileDescriptor &fd = planes[0].fd; + size_t offset = 0; + for (size_t i = 0; i < numColorPlanes; ++i) { + planes[i].fd = fd; + planes[i].offset = offset; + + /* \todo Take the V4L2 stride into account */ + const unsigned int vertSubSample = + info.planes[i].verticalSubSampling; + planes[i].length = + info.stride(format_.size.width, i, 1u) * + ((format_.size.height + vertSubSample - 1) / vertSubSample); + offset += planes[i].length; + } + } + return std::make_unique<FrameBuffer>(std::move(planes)); } @@ -1352,7 +1437,7 @@ int V4L2VideoDevice::importBuffers(unsigned int count) if (ret) return ret; - cache_ = new V4L2BufferCache(count); + cache_ = new V4L2BufferCache(count, format_.planesCount); LOG(V4L2, Debug) << "Prepared to import " << count << " buffers"; diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp index b3f2bec1..48f748bc 100644 --- a/test/v4l2_videodevice/buffer_cache.cpp +++ b/test/v4l2_videodevice/buffer_cache.cpp @@ -166,7 +166,7 @@ public: * Test cache of same size as there are buffers, the cache is * created from a list of buffers and will be pre-populated. */ - V4L2BufferCache cacheFromBuffers(buffers); + V4L2BufferCache cacheFromBuffers(buffers, 1u); if (testSequential(&cacheFromBuffers, buffers) != TestPass) return TestFail; @@ -181,7 +181,7 @@ public: * Test cache of same size as there are buffers, the cache is * not pre-populated. */ - V4L2BufferCache cacheFromNumbers(numBuffers); + V4L2BufferCache cacheFromNumbers(numBuffers, 1u); if (testSequential(&cacheFromNumbers, buffers) != TestPass) return TestFail; @@ -196,7 +196,7 @@ public: * Test cache half the size of number of buffers used, the cache * is not pre-populated. */ - V4L2BufferCache cacheHalf(numBuffers / 2); + V4L2BufferCache cacheHalf(numBuffers / 2, 1u); if (testRandom(&cacheHalf, buffers) != TestPass) return TestFail;
Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- include/libcamera/internal/v4l2_videodevice.h | 10 +- src/libcamera/v4l2_videodevice.cpp | 141 ++++++++++++++---- test/v4l2_videodevice/buffer_cache.cpp | 6 +- 3 files changed, 123 insertions(+), 34 deletions(-)