| Message ID | 20210823131221.1034059-9-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. On Mon, Aug 23, 2021 at 10:12:19PM +0900, Hirokazu Honda wrote: > V4L2VideDevice::createBuffer() creates the same number of > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 > format single is single-planar format, the created number of > FrameBuffer::Planes is 1. > It should rather create the same number of FrameBuffer::Planes as the > color format planes. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > include/libcamera/internal/v4l2_videodevice.h | 9 +- > src/libcamera/v4l2_videodevice.cpp | 147 ++++++++++++++---- > test/v4l2_videodevice/buffer_cache.cpp | 6 +- > 3 files changed, 128 insertions(+), 34 deletions(-) > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > index e767ec84..69bb964a 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_; > }; > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index ce60dff6..42b66bd3 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" > > @@ -158,12 +159,13 @@ LOG_DECLARE_CATEGORY(V4L2) > * \brief Create an empty cache with \a numEntries entries > * \param[in] numEntries Number of entries to reserve in the cache > * > - * 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); > } > @@ -172,16 +174,18 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries) > * \brief Create a pre-populated cache > * \param[in] buffers Array of buffers to pre-populated with > * > - * 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 +241,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 +262,47 @@ 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; > + } No need for curly braces. > } > > 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) { > + 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 { > + 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; > } I'm not sure to understand why all this is needed. > > @@ -1152,8 +1180,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 > @@ -1167,11 +1200,19 @@ int V4L2VideoDevice::requestBuffers(unsigned int count, > int V4L2VideoDevice::allocateBuffers(unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > - int ret = createBuffers(count, buffers); > + V4L2DeviceFormat format{}; > + int ret = getFormat(&format); > + if (ret < 0) { > + LOG(V4L2, Error) > + << "Failed to get format: " << strerror(-ret); > + return ret; > + } Instead of calling getFormat() here and in two places below, how about caching the format in setFormat() ? > + > + ret = createBuffers(count, buffers); > if (ret < 0) > return ret; > > - cache_ = new V4L2BufferCache(*buffers); > + cache_ = new V4L2BufferCache(*buffers, format.planesCount); > memoryType_ = V4L2_MEMORY_MMAP; > > return ret; > @@ -1190,8 +1231,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 > @@ -1283,12 +1329,49 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > > FrameBuffer::Plane plane; > plane.fd = std::move(fd); > - plane.length = multiPlanar ? > - buf.m.planes[nplane].length : buf.length; > + /* > + * V4L2 API doesn't provide dmabuf offset information of plane. > + * Set 0 as a placeholder offset. > + * \todo Set the right offset once V4L2 API provides a way. > + */ > + plane.offset = 0; > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; > > planes.push_back(std::move(plane)); > } > > + /* > + * Get V4L2DeviceFormat of the frame buffer. If it is V4L2 single-planar > + * format, overwrite FrameBuffer::Plane to make it color format planes. > + * */ > + V4L2DeviceFormat format{}; > + ret = getFormat(&format); > + if (ret < 0) { > + LOG(V4L2, Error) > + << "Failed to get format: " << strerror(-ret); > + return nullptr; > + } > + > + const auto &info = PixelFormatInfo::info(format.fourcc); > + if (info.isValid() && info.numPlanes() != numPlanes) { If info isn't valid, should we return nullptr ? > + 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; > + Could you please add a /* \todo Take the V4L2 stride into account */ comment ? No need to address this now, but it will need to be done at some point. > + 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)); > } > > @@ -1342,11 +1425,19 @@ int V4L2VideoDevice::importBuffers(unsigned int count) > > memoryType_ = V4L2_MEMORY_DMABUF; > > - int ret = requestBuffers(count, V4L2_MEMORY_DMABUF); > + V4L2DeviceFormat format{}; > + int ret = getFormat(&format); > + if (ret < 0) { > + LOG(V4L2, Error) > + << "Failed to get format: " << strerror(-ret); > + return ret; > + } > + > + ret = requestBuffers(count, V4L2_MEMORY_DMABUF); > 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 Hiro, One more comment. On Tue, Aug 24, 2021 at 02:54:56AM +0300, Laurent Pinchart wrote: > On Mon, Aug 23, 2021 at 10:12:19PM +0900, Hirokazu Honda wrote: > > V4L2VideDevice::createBuffer() creates the same number of > > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 > > format single is single-planar format, the created number of > > FrameBuffer::Planes is 1. > > It should rather create the same number of FrameBuffer::Planes as the > > color format planes. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > include/libcamera/internal/v4l2_videodevice.h | 9 +- > > src/libcamera/v4l2_videodevice.cpp | 147 ++++++++++++++---- > > test/v4l2_videodevice/buffer_cache.cpp | 6 +- > > 3 files changed, 128 insertions(+), 34 deletions(-) > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > index e767ec84..69bb964a 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); I'm getting the following doxygen warnings: include/libcamera/internal/v4l2_videodevice.h:117: warning: The following parameter of libcamera::V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes) is not documented: parameter 'numPlanes' include/libcamera/internal/v4l2_videodevice.h:118: warning: The following parameter of libcamera::V4L2BufferCache::V4L2BufferCache(const std::vector< std::unique_ptr< FrameBuffer > > &buffers, unsigned int numPlanes) is not documented: parameter '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_; > > }; > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > index ce60dff6..42b66bd3 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" > > > > @@ -158,12 +159,13 @@ LOG_DECLARE_CATEGORY(V4L2) > > * \brief Create an empty cache with \a numEntries entries > > * \param[in] numEntries Number of entries to reserve in the cache > > * > > - * 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); > > } > > @@ -172,16 +174,18 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries) > > * \brief Create a pre-populated cache > > * \param[in] buffers Array of buffers to pre-populated with > > * > > - * 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 +241,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 +262,47 @@ 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; > > + } > > No need for curly braces. > > > } > > > > 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) { > > + 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 { > > + 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; > > } > > I'm not sure to understand why all this is needed. > > > > > @@ -1152,8 +1180,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 > > @@ -1167,11 +1200,19 @@ int V4L2VideoDevice::requestBuffers(unsigned int count, > > int V4L2VideoDevice::allocateBuffers(unsigned int count, > > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > > { > > - int ret = createBuffers(count, buffers); > > + V4L2DeviceFormat format{}; > > + int ret = getFormat(&format); > > + if (ret < 0) { > > + LOG(V4L2, Error) > > + << "Failed to get format: " << strerror(-ret); > > + return ret; > > + } > > Instead of calling getFormat() here and in two places below, how about > caching the format in setFormat() ? > > > + > > + ret = createBuffers(count, buffers); > > if (ret < 0) > > return ret; > > > > - cache_ = new V4L2BufferCache(*buffers); > > + cache_ = new V4L2BufferCache(*buffers, format.planesCount); > > memoryType_ = V4L2_MEMORY_MMAP; > > > > return ret; > > @@ -1190,8 +1231,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 > > @@ -1283,12 +1329,49 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > > > > FrameBuffer::Plane plane; > > plane.fd = std::move(fd); > > - plane.length = multiPlanar ? > > - buf.m.planes[nplane].length : buf.length; > > + /* > > + * V4L2 API doesn't provide dmabuf offset information of plane. > > + * Set 0 as a placeholder offset. > > + * \todo Set the right offset once V4L2 API provides a way. > > + */ > > + plane.offset = 0; > > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; > > > > planes.push_back(std::move(plane)); > > } > > > > + /* > > + * Get V4L2DeviceFormat of the frame buffer. If it is V4L2 single-planar > > + * format, overwrite FrameBuffer::Plane to make it color format planes. > > + * */ > > + V4L2DeviceFormat format{}; > > + ret = getFormat(&format); > > + if (ret < 0) { > > + LOG(V4L2, Error) > > + << "Failed to get format: " << strerror(-ret); > > + return nullptr; > > + } > > + > > + const auto &info = PixelFormatInfo::info(format.fourcc); > > + if (info.isValid() && info.numPlanes() != numPlanes) { > > If info isn't valid, should we return nullptr ? > > > + 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; > > + > > Could you please add a > > /* \todo Take the V4L2 stride into account */ > > comment ? No need to address this now, but it will need to be done at > some point. > > > + 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)); > > } > > > > @@ -1342,11 +1425,19 @@ int V4L2VideoDevice::importBuffers(unsigned int count) > > > > memoryType_ = V4L2_MEMORY_DMABUF; > > > > - int ret = requestBuffers(count, V4L2_MEMORY_DMABUF); > > + V4L2DeviceFormat format{}; > > + int ret = getFormat(&format); > > + if (ret < 0) { > > + LOG(V4L2, Error) > > + << "Failed to get format: " << strerror(-ret); > > + return ret; > > + } > > + > > + ret = requestBuffers(count, V4L2_MEMORY_DMABUF); > > 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, thank you for reviewing. On Tue, Aug 24, 2021 at 9:27 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > One more comment. > > On Tue, Aug 24, 2021 at 02:54:56AM +0300, Laurent Pinchart wrote: > > On Mon, Aug 23, 2021 at 10:12:19PM +0900, Hirokazu Honda wrote: > > > V4L2VideDevice::createBuffer() creates the same number of > > > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 > > > format single is single-planar format, the created number of > > > FrameBuffer::Planes is 1. > > > It should rather create the same number of FrameBuffer::Planes as the > > > color format planes. > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > --- > > > include/libcamera/internal/v4l2_videodevice.h | 9 +- > > > src/libcamera/v4l2_videodevice.cpp | 147 ++++++++++++++---- > > > test/v4l2_videodevice/buffer_cache.cpp | 6 +- > > > 3 files changed, 128 insertions(+), 34 deletions(-) > > > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > > index e767ec84..69bb964a 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); > > I'm getting the following doxygen warnings: > > include/libcamera/internal/v4l2_videodevice.h:117: warning: The following parameter of libcamera::V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes) is not documented: > parameter 'numPlanes' > include/libcamera/internal/v4l2_videodevice.h:118: warning: The following parameter of libcamera::V4L2BufferCache::V4L2BufferCache(const std::vector< std::unique_ptr< FrameBuffer > > &buffers, unsigned int numPlanes) is not documented: > parameter '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_; > > > }; > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > index ce60dff6..42b66bd3 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" > > > > > > @@ -158,12 +159,13 @@ LOG_DECLARE_CATEGORY(V4L2) > > > * \brief Create an empty cache with \a numEntries entries > > > * \param[in] numEntries Number of entries to reserve in the cache > > > * > > > - * 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); > > > } > > > @@ -172,16 +174,18 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries) > > > * \brief Create a pre-populated cache > > > * \param[in] buffers Array of buffers to pre-populated with > > > * > > > - * 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 +241,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 +262,47 @@ 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; > > > + } > > > > No need for curly braces. > > > > > } > > > > > > 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) { > > > + 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 { > > > + 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; > > > } > > > > I'm not sure to understand why all this is needed. > > I will add a comment to the next patch series. > > > > > > @@ -1152,8 +1180,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 > > > @@ -1167,11 +1200,19 @@ int V4L2VideoDevice::requestBuffers(unsigned int count, > > > int V4L2VideoDevice::allocateBuffers(unsigned int count, > > > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > > > { > > > - int ret = createBuffers(count, buffers); > > > + V4L2DeviceFormat format{}; > > > + int ret = getFormat(&format); > > > + if (ret < 0) { > > > + LOG(V4L2, Error) > > > + << "Failed to get format: " << strerror(-ret); > > > + return ret; > > > + } > > > > Instead of calling getFormat() here and in two places below, how about > > caching the format in setFormat() ? > > > > > + > > > + ret = createBuffers(count, buffers); > > > if (ret < 0) > > > return ret; > > > > > > - cache_ = new V4L2BufferCache(*buffers); > > > + cache_ = new V4L2BufferCache(*buffers, format.planesCount); > > > memoryType_ = V4L2_MEMORY_MMAP; > > > > > > return ret; > > > @@ -1190,8 +1231,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 > > > @@ -1283,12 +1329,49 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) > > > > > > FrameBuffer::Plane plane; > > > plane.fd = std::move(fd); > > > - plane.length = multiPlanar ? > > > - buf.m.planes[nplane].length : buf.length; > > > + /* > > > + * V4L2 API doesn't provide dmabuf offset information of plane. > > > + * Set 0 as a placeholder offset. > > > + * \todo Set the right offset once V4L2 API provides a way. > > > + */ > > > + plane.offset = 0; > > > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; > > > > > > planes.push_back(std::move(plane)); > > > } > > > > > > + /* > > > + * Get V4L2DeviceFormat of the frame buffer. If it is V4L2 single-planar > > > + * format, overwrite FrameBuffer::Plane to make it color format planes. > > > + * */ > > > + V4L2DeviceFormat format{}; > > > + ret = getFormat(&format); > > > + if (ret < 0) { > > > + LOG(V4L2, Error) > > > + << "Failed to get format: " << strerror(-ret); > > > + return nullptr; > > > + } > > > + > > > + const auto &info = PixelFormatInfo::info(format.fourcc); > > > + if (info.isValid() && info.numPlanes() != numPlanes) { > > > > If info isn't valid, should we return nullptr ? createBuffer() is called for non pixel format buffer for example, /dev/video10[33:out]: Invalid format: 0x0-ip3p I need to check by info.isValid() to filter out non pixel formats. -Hiro > > > > > + 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; > > > + > > > > Could you please add a > > > > /* \todo Take the V4L2 stride into account */ > > > > comment ? No need to address this now, but it will need to be done at > > some point. > > > > > + 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)); > > > } > > > > > > @@ -1342,11 +1425,19 @@ int V4L2VideoDevice::importBuffers(unsigned int count) > > > > > > memoryType_ = V4L2_MEMORY_DMABUF; > > > > > > - int ret = requestBuffers(count, V4L2_MEMORY_DMABUF); > > > + V4L2DeviceFormat format{}; > > > + int ret = getFormat(&format); > > > + if (ret < 0) { > > > + LOG(V4L2, Error) > > > + << "Failed to get format: " << strerror(-ret); > > > + return ret; > > > + } > > > + > > > + ret = requestBuffers(count, V4L2_MEMORY_DMABUF); > > > 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
diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h index e767ec84..69bb964a 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_; }; diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index ce60dff6..42b66bd3 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" @@ -158,12 +159,13 @@ LOG_DECLARE_CATEGORY(V4L2) * \brief Create an empty cache with \a numEntries entries * \param[in] numEntries Number of entries to reserve in the cache * - * 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); } @@ -172,16 +174,18 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries) * \brief Create a pre-populated cache * \param[in] buffers Array of buffers to pre-populated with * - * 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 +241,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 +262,47 @@ 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) { + 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 { + 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; } @@ -1152,8 +1180,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 @@ -1167,11 +1200,19 @@ int V4L2VideoDevice::requestBuffers(unsigned int count, int V4L2VideoDevice::allocateBuffers(unsigned int count, std::vector<std::unique_ptr<FrameBuffer>> *buffers) { - int ret = createBuffers(count, buffers); + V4L2DeviceFormat format{}; + int ret = getFormat(&format); + if (ret < 0) { + LOG(V4L2, Error) + << "Failed to get format: " << strerror(-ret); + return ret; + } + + ret = createBuffers(count, buffers); if (ret < 0) return ret; - cache_ = new V4L2BufferCache(*buffers); + cache_ = new V4L2BufferCache(*buffers, format.planesCount); memoryType_ = V4L2_MEMORY_MMAP; return ret; @@ -1190,8 +1231,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 @@ -1283,12 +1329,49 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index) FrameBuffer::Plane plane; plane.fd = std::move(fd); - plane.length = multiPlanar ? - buf.m.planes[nplane].length : buf.length; + /* + * V4L2 API doesn't provide dmabuf offset information of plane. + * Set 0 as a placeholder offset. + * \todo Set the right offset once V4L2 API provides a way. + */ + plane.offset = 0; + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length; planes.push_back(std::move(plane)); } + /* + * Get V4L2DeviceFormat of the frame buffer. If it is V4L2 single-planar + * format, overwrite FrameBuffer::Plane to make it color format planes. + * */ + V4L2DeviceFormat format{}; + ret = getFormat(&format); + if (ret < 0) { + LOG(V4L2, Error) + << "Failed to get format: " << strerror(-ret); + return nullptr; + } + + 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; + + 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)); } @@ -1342,11 +1425,19 @@ int V4L2VideoDevice::importBuffers(unsigned int count) memoryType_ = V4L2_MEMORY_DMABUF; - int ret = requestBuffers(count, V4L2_MEMORY_DMABUF); + V4L2DeviceFormat format{}; + int ret = getFormat(&format); + if (ret < 0) { + LOG(V4L2, Error) + << "Failed to get format: " << strerror(-ret); + return ret; + } + + ret = requestBuffers(count, V4L2_MEMORY_DMABUF); 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;
V4L2VideDevice::createBuffer() creates the same number of FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 format single is single-planar format, the created number of FrameBuffer::Planes is 1. It should rather create the same number of FrameBuffer::Planes as the color format planes. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- include/libcamera/internal/v4l2_videodevice.h | 9 +- src/libcamera/v4l2_videodevice.cpp | 147 ++++++++++++++---- test/v4l2_videodevice/buffer_cache.cpp | 6 +- 3 files changed, 128 insertions(+), 34 deletions(-)