[{"id":19019,"web_url":"https://patchwork.libcamera.org/comment/19019/","msgid":"","date":"2021-08-23T23:54:54","subject":"Re: [libcamera-devel] [RFC PATCH v2 08/10] libcamera:\n\tv4l2_videodevice: Create color-format planes in createBuffer()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Mon, Aug 23, 2021 at 10:12:19PM +0900, Hirokazu Honda wrote:\n> V4L2VideDevice::createBuffer() creates the same number of\n> FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2\n> format single is single-planar format, the created number of\n> FrameBuffer::Planes is 1.\n> It should rather create the same number of FrameBuffer::Planes as the\n> color format planes.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/internal/v4l2_videodevice.h | 9 +-\n> src/libcamera/v4l2_videodevice.cpp | 147 ++++++++++++++----\n> test/v4l2_videodevice/buffer_cache.cpp | 6 +-\n> 3 files changed, 128 insertions(+), 34 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index e767ec84..69bb964a 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -114,8 +114,9 @@ struct V4L2Capability final : v4l2_capability {\n> class V4L2BufferCache\n> {\n> public:\n> -\tV4L2BufferCache(unsigned int numEntries);\n> -\tV4L2BufferCache(const std::vector> &buffers);\n> +\tV4L2BufferCache(unsigned int numEntries, unsigned int numPlanes);\n> +\tV4L2BufferCache(const std::vector> &buffers,\n> +\t\t\tunsigned int numPlanes);\n> \t~V4L2BufferCache();\n> \n> \tint get(const FrameBuffer &buffer);\n> @@ -126,7 +127,8 @@ private:\n> \t{\n> \tpublic:\n> \t\tEntry();\n> -\t\tEntry(bool free, uint64_t lastUsed, const FrameBuffer &buffer);\n> +\t\tEntry(bool free, uint64_t lastUsed, unsigned int numPlanes,\n> +\t\t const FrameBuffer &buffer);\n> \n> \t\tbool operator==(const FrameBuffer &buffer) const;\n> \n> @@ -149,6 +151,7 @@ private:\n> \n> \tstd::atomic lastUsedCounter_;\n> \tstd::vector cache_;\n> +\tunsigned int numPlanes_;\n> \t/* \\todo Expose the miss counter through an instrumentation API. */\n> \tunsigned int missCounter_;\n> };\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index ce60dff6..42b66bd3 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -25,6 +25,7 @@\n> \n> #include \n> \n> +#include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/media_object.h\"\n> \n> @@ -158,12 +159,13 @@ LOG_DECLARE_CATEGORY(V4L2)\n> * \\brief Create an empty cache with \\a numEntries entries\n> * \\param[in] numEntries Number of entries to reserve in the cache\n> *\n> - * Create a cache with \\a numEntries entries all marked as unused. The entries\n> - * will be populated as the cache is used. This is typically used to implement\n> - * buffer import, with buffers added to the cache as they are queued.\n> + * Create a cache with \\a numEntries entries all marked as unused. The entry is\n> + * for \\a numPlanes planes V4L2 buffer. The entries will be populated as the\n> + * cache is used. This is typically used to implement buffer import, with\n> + * buffers added to the cache as they are queued.\n> */\n> -V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)\n> -\t: lastUsedCounter_(1), missCounter_(0)\n> +V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes)\n> +\t: lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)\n> {\n> \tcache_.resize(numEntries);\n> }\n> @@ -172,16 +174,18 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)\n> * \\brief Create a pre-populated cache\n> * \\param[in] buffers Array of buffers to pre-populated with\n> *\n> - * Create a cache pre-populated with \\a buffers. This is typically used to\n> - * implement buffer export, with all buffers added to the cache when they are\n> - * allocated.\n> + * Create a cache pre-populated with \\a buffers. The entry is for \\a numPlanes\n> + * planes V4L2 buffer. This is typically used to implement buffer export, with\n> + * all buffers added to the cache when they are allocated.\n> */\n> -V4L2BufferCache::V4L2BufferCache(const std::vector> &buffers)\n> -\t: lastUsedCounter_(1), missCounter_(0)\n> +V4L2BufferCache::V4L2BufferCache(const std::vector> &buffers,\n> +\t\t\t\t unsigned int numPlanes)\n> +\t: lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)\n> {\n> \tfor (const std::unique_ptr &buffer : buffers)\n> \t\tcache_.emplace_back(true,\n> \t\t\t\t lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n> +\t\t\t\t numPlanes_,\n> \t\t\t\t *buffer.get());\n> }\n> \n> @@ -237,6 +241,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)\n> \n> \tcache_[use] = Entry(false,\n> \t\t\t lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n> +\t\t\t numPlanes_,\n> \t\t\t buffer);\n> \n> \treturn use;\n> @@ -257,24 +262,47 @@ V4L2BufferCache::Entry::Entry()\n> {\n> }\n> \n> -V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer)\n> +V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed,\n> +\t\t\t unsigned int numPlanes, const FrameBuffer &buffer)\n> \t: free_(free), lastUsed_(lastUsed)\n> {\n> -\tfor (const FrameBuffer::Plane &plane : buffer.planes())\n> -\t\tplanes_.emplace_back(plane);\n> +\tASSERT(numPlanes <= buffer.planes().size());\n> +\tunsigned int length = 0;\n> +\tfor (const FrameBuffer::Plane &plane : buffer.planes()) {\n> +\t\tASSERT(plane.offset == length);\n> +\t\tlength += plane.length;\n> +\n> +\t\tif (planes_.size() < numPlanes)\n> +\t\t\tplanes_.emplace_back(plane);\n> +\t}\n> +\n> +\tif (numPlanes == 1) {\n> +\t\tplanes_[0].length = length;\n> +\t}\n\nNo need for curly braces.\n\n> }\n> \n> bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> {\n> \tconst std::vector &planes = buffer.planes();\n> \n> -\tif (planes_.size() != planes.size())\n> +\tif (planes_.size() != planes.size() || planes_.size() == 1)\n> \t\treturn false;\n> -\n> -\tfor (unsigned int i = 0; i < planes.size(); i++)\n> -\t\tif (planes_[i].fd != planes[i].fd.fd() ||\n> -\t\t planes_[i].length != planes[i].length)\n> +\tif (planes_.size() == 1) {\n> +\t\tunsigned int length = 0;\n> +\t\tfor (unsigned int i = 0; i < planes.size(); i++) {\n> +\t\t\tif (planes_[0].fd != planes[i].fd.fd())\n> +\t\t\t\treturn false;\n> +\t\t\tlength += planes[i].length;\n> +\t\t}\n> +\t\tif (length != planes_[0].length)\n> \t\t\treturn false;\n> +\t} else {\n> +\t\tfor (unsigned int i = 0; i < planes.size(); i++)\n> +\t\t\tif (planes_[i].fd != planes[i].fd.fd() ||\n> +\t\t\t planes_[i].length != planes[i].length)\n> +\t\t\t\treturn false;\n> +\t}\n> +\n> \treturn true;\n> }\n\nI'm not sure to understand why all this is needed.\n\n> \n> @@ -1152,8 +1180,13 @@ int V4L2VideoDevice::requestBuffers(unsigned int count,\n> * successful return the driver's internal buffer management is initialized in\n> * MMAP mode, and the video device is ready to accept queueBuffer() calls.\n> *\n> - * The number of planes and the plane sizes for the allocation are determined\n> - * by the currently active format on the device as set by setFormat().\n> + * The number of planes and their offsets and sizes are determined by the\n> + * currently active format on the device as set by setFormat(). They do not map\n> + * to the V4L2 buffer planes, but to colour planes of the pixel format. For\n> + * instance, if the active format is formats::NV12, the allocated FrameBuffer\n> + * instances will have two planes, for the luma and chroma components,\n> + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or\n> + * V4L2_PIX_FMT_NV12M.\n> *\n> * Buffers allocated with this function shall later be free with\n> * releaseBuffers(). If buffers have already been allocated with\n> @@ -1167,11 +1200,19 @@ int V4L2VideoDevice::requestBuffers(unsigned int count,\n> int V4L2VideoDevice::allocateBuffers(unsigned int count,\n> \t\t\t\t std::vector> *buffers)\n> {\n> -\tint ret = createBuffers(count, buffers);\n> +\tV4L2DeviceFormat format{};\n> +\tint ret = getFormat(&format);\n> +\tif (ret < 0) {\n> +\t\tLOG(V4L2, Error)\n> +\t\t\t<< \"Failed to get format: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n\nInstead of calling getFormat() here and in two places below, how about\ncaching the format in setFormat() ?\n\n> +\n> +\tret = createBuffers(count, buffers);\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\tcache_ = new V4L2BufferCache(*buffers);\n> +\tcache_ = new V4L2BufferCache(*buffers, format.planesCount);\n> \tmemoryType_ = V4L2_MEMORY_MMAP;\n> \n> \treturn ret;\n> @@ -1190,8 +1231,13 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,\n> * usable with any V4L2 video device in DMABUF mode, or with other dmabuf\n> * importers.\n> *\n> - * The number of planes and the plane sizes for the allocation are determined\n> - * by the currently active format on the device as set by setFormat().\n> + * The number of planes and their offsets and sizes are determined by the\n> + * currently active format on the device as set by setFormat(). They do not map\n> + * to the V4L2 buffer planes, but to colour planes of the pixel format. For\n> + * instance, if the active format is formats::NV12, the allocated FrameBuffer\n> + * instances will have two planes, for the luma and chroma components,\n> + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or\n> + * V4L2_PIX_FMT_NV12M.\n> *\n> * Multiple independent sets of buffers can be allocated with multiple calls to\n> * this function. Device-specific limitations may apply regarding the minimum\n> @@ -1283,12 +1329,49 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\n> \n> \t\tFrameBuffer::Plane plane;\n> \t\tplane.fd = std::move(fd);\n> -\t\tplane.length = multiPlanar ?\n> -\t\t\tbuf.m.planes[nplane].length : buf.length;\n> +\t\t/*\n> +\t\t * V4L2 API doesn't provide dmabuf offset information of plane.\n> +\t\t * Set 0 as a placeholder offset.\n> +\t\t * \\todo Set the right offset once V4L2 API provides a way.\n> +\t\t */\n> +\t\tplane.offset = 0;\n> +\t\tplane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;\n> \n> \t\tplanes.push_back(std::move(plane));\n> \t}\n> \n> +\t/*\n> +\t * Get V4L2DeviceFormat of the frame buffer. If it is V4L2 single-planar\n> +\t * format, overwrite FrameBuffer::Plane to make it color format planes.\n> +\t * */\n> +\tV4L2DeviceFormat format{};\n> +\tret = getFormat(&format);\n> +\tif (ret < 0) {\n> +\t\tLOG(V4L2, Error)\n> +\t\t\t<< \"Failed to get format: \" << strerror(-ret);\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tconst auto &info = PixelFormatInfo::info(format.fourcc);\n> +\tif (info.isValid() && info.numPlanes() != numPlanes) {\n\nIf info isn't valid, should we return nullptr ?\n\n> +\t\tASSERT(numPlanes == 1u);\n> +\t\tconst size_t numColorPlanes = info.numPlanes();\n> +\t\tplanes.resize(numColorPlanes);\n> +\t\tconst FileDescriptor &fd = planes[0].fd;\n> +\t\tsize_t offset = 0;\n> +\t\tfor (size_t i = 0; i < numColorPlanes; ++i) {\n> +\t\t\tplanes[i].fd = fd;\n> +\t\t\tplanes[i].offset = offset;\n> +\n\nCould you please add a\n\n\t\t\t/* \\todo Take the V4L2 stride into account */\n\ncomment ? No need to address this now, but it will need to be done at\nsome point.\n\n> +\t\t\tconst unsigned int vertSubSample =\n> +\t\t\t\tinfo.planes[i].verticalSubSampling;\n> +\t\t\tplanes[i].length =\n> +\t\t\t\tinfo.stride(format.size.width, i, 1u) *\n> +\t\t\t\t((format.size.height + vertSubSample - 1) / vertSubSample);\n> +\t\t\toffset += planes[i].length;\n> +\t\t}\n> +\t}\n> +\n> \treturn std::make_unique(std::move(planes));\n> }\n> \n> @@ -1342,11 +1425,19 @@ int V4L2VideoDevice::importBuffers(unsigned int count)\n> \n> \tmemoryType_ = V4L2_MEMORY_DMABUF;\n> \n> -\tint ret = requestBuffers(count, V4L2_MEMORY_DMABUF);\n> +\tV4L2DeviceFormat format{};\n> +\tint ret = getFormat(&format);\n> +\tif (ret < 0) {\n> +\t\tLOG(V4L2, Error)\n> +\t\t\t<< \"Failed to get format: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = requestBuffers(count, V4L2_MEMORY_DMABUF);\n> \tif (ret)\n> \t\treturn ret;\n> \n> -\tcache_ = new V4L2BufferCache(count);\n> +\tcache_ = new V4L2BufferCache(count, format.planesCount);\n> \n> \tLOG(V4L2, Debug) << \"Prepared to import \" << count << \" buffers\";\n> \n> diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp\n> index b3f2bec1..48f748bc 100644\n> --- a/test/v4l2_videodevice/buffer_cache.cpp\n> +++ b/test/v4l2_videodevice/buffer_cache.cpp\n> @@ -166,7 +166,7 @@ public:\n> \t\t * Test cache of same size as there are buffers, the cache is\n> \t\t * created from a list of buffers and will be pre-populated.\n> \t\t */\n> -\t\tV4L2BufferCache cacheFromBuffers(buffers);\n> +\t\tV4L2BufferCache cacheFromBuffers(buffers, 1u);\n> \n> \t\tif (testSequential(&cacheFromBuffers, buffers) != TestPass)\n> \t\t\treturn TestFail;\n> @@ -181,7 +181,7 @@ public:\n> \t\t * Test cache of same size as there are buffers, the cache is\n> \t\t * not pre-populated.\n> \t\t */\n> -\t\tV4L2BufferCache cacheFromNumbers(numBuffers);\n> +\t\tV4L2BufferCache cacheFromNumbers(numBuffers, 1u);\n> \n> \t\tif (testSequential(&cacheFromNumbers, buffers) != TestPass)\n> \t\t\treturn TestFail;\n> @@ -196,7 +196,7 @@ public:\n> \t\t * Test cache half the size of number of buffers used, the cache\n> \t\t * is not pre-populated.\n> \t\t */\n> -\t\tV4L2BufferCache cacheHalf(numBuffers / 2);\n> +\t\tV4L2BufferCache cacheHalf(numBuffers / 2, 1u);\n> \n> \t\tif (testRandom(&cacheHalf, buffers) != TestPass)\n> \t\t\treturn TestFail;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0502CBD87C\n\tfor ;\n\tMon, 23 Aug 2021 23:55:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 544AD688A6;\n\tTue, 24 Aug 2021 01:55:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 90AE968890\n\tfor ;\n\tTue, 24 Aug 2021 01:55:05 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C03C92A5;\n\tTue, 24 Aug 2021 01:55:04 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"KUoTLW7I\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629762905;\n\tbh=yNreqhh4ov4ywV6cgy8k5YC8lhsU2MQ1BnuIt5i7NP8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KUoTLW7I7+5iBHDI/j9or7zGUtlFLxO6mJ66GAOQr2EXSWsxT7S23sJ1DiNCO4MJY\n\tOSEAkyoW5ydhJmXwAYsn/9z5Q9aicNg76RjvcOt4WVJBtFtsWdzh7e/OOySa6RB7J/\n\tWb8qjH5dGkTuJUaBQNh+ajgz5D3M+talOAW6jjPs=","Date":"Tue, 24 Aug 2021 02:54:54 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-9-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210823131221.1034059-9-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 08/10] libcamera:\n\tv4l2_videodevice: Create color-format planes in createBuffer()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19022,"web_url":"https://patchwork.libcamera.org/comment/19022/","msgid":"","date":"2021-08-24T00:26:57","subject":"Re: [libcamera-devel] [RFC PATCH v2 08/10] libcamera:\n\tv4l2_videodevice: Create color-format planes in createBuffer()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOne more comment.\n\nOn Tue, Aug 24, 2021 at 02:54:56AM +0300, Laurent Pinchart wrote:\n> On Mon, Aug 23, 2021 at 10:12:19PM +0900, Hirokazu Honda wrote:\n> > V4L2VideDevice::createBuffer() creates the same number of\n> > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2\n> > format single is single-planar format, the created number of\n> > FrameBuffer::Planes is 1.\n> > It should rather create the same number of FrameBuffer::Planes as the\n> > color format planes.\n> > \n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/internal/v4l2_videodevice.h | 9 +-\n> > src/libcamera/v4l2_videodevice.cpp | 147 ++++++++++++++----\n> > test/v4l2_videodevice/buffer_cache.cpp | 6 +-\n> > 3 files changed, 128 insertions(+), 34 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > index e767ec84..69bb964a 100644\n> > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > @@ -114,8 +114,9 @@ struct V4L2Capability final : v4l2_capability {\n> > class V4L2BufferCache\n> > {\n> > public:\n> > -\tV4L2BufferCache(unsigned int numEntries);\n> > -\tV4L2BufferCache(const std::vector> &buffers);\n> > +\tV4L2BufferCache(unsigned int numEntries, unsigned int numPlanes);\n> > +\tV4L2BufferCache(const std::vector> &buffers,\n> > +\t\t\tunsigned int numPlanes);\n\nI'm getting the following doxygen warnings:\n\ninclude/libcamera/internal/v4l2_videodevice.h:117: warning: The following parameter of libcamera::V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes) is not documented:\n parameter 'numPlanes'\ninclude/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:\n parameter 'numPlanes'\n\n> > \t~V4L2BufferCache();\n> > \n> > \tint get(const FrameBuffer &buffer);\n> > @@ -126,7 +127,8 @@ private:\n> > \t{\n> > \tpublic:\n> > \t\tEntry();\n> > -\t\tEntry(bool free, uint64_t lastUsed, const FrameBuffer &buffer);\n> > +\t\tEntry(bool free, uint64_t lastUsed, unsigned int numPlanes,\n> > +\t\t const FrameBuffer &buffer);\n> > \n> > \t\tbool operator==(const FrameBuffer &buffer) const;\n> > \n> > @@ -149,6 +151,7 @@ private:\n> > \n> > \tstd::atomic lastUsedCounter_;\n> > \tstd::vector cache_;\n> > +\tunsigned int numPlanes_;\n> > \t/* \\todo Expose the miss counter through an instrumentation API. */\n> > \tunsigned int missCounter_;\n> > };\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index ce60dff6..42b66bd3 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -25,6 +25,7 @@\n> > \n> > #include \n> > \n> > +#include \"libcamera/internal/formats.h\"\n> > #include \"libcamera/internal/media_device.h\"\n> > #include \"libcamera/internal/media_object.h\"\n> > \n> > @@ -158,12 +159,13 @@ LOG_DECLARE_CATEGORY(V4L2)\n> > * \\brief Create an empty cache with \\a numEntries entries\n> > * \\param[in] numEntries Number of entries to reserve in the cache\n> > *\n> > - * Create a cache with \\a numEntries entries all marked as unused. The entries\n> > - * will be populated as the cache is used. This is typically used to implement\n> > - * buffer import, with buffers added to the cache as they are queued.\n> > + * Create a cache with \\a numEntries entries all marked as unused. The entry is\n> > + * for \\a numPlanes planes V4L2 buffer. The entries will be populated as the\n> > + * cache is used. This is typically used to implement buffer import, with\n> > + * buffers added to the cache as they are queued.\n> > */\n> > -V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)\n> > -\t: lastUsedCounter_(1), missCounter_(0)\n> > +V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes)\n> > +\t: lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)\n> > {\n> > \tcache_.resize(numEntries);\n> > }\n> > @@ -172,16 +174,18 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)\n> > * \\brief Create a pre-populated cache\n> > * \\param[in] buffers Array of buffers to pre-populated with\n> > *\n> > - * Create a cache pre-populated with \\a buffers. This is typically used to\n> > - * implement buffer export, with all buffers added to the cache when they are\n> > - * allocated.\n> > + * Create a cache pre-populated with \\a buffers. The entry is for \\a numPlanes\n> > + * planes V4L2 buffer. This is typically used to implement buffer export, with\n> > + * all buffers added to the cache when they are allocated.\n> > */\n> > -V4L2BufferCache::V4L2BufferCache(const std::vector> &buffers)\n> > -\t: lastUsedCounter_(1), missCounter_(0)\n> > +V4L2BufferCache::V4L2BufferCache(const std::vector> &buffers,\n> > +\t\t\t\t unsigned int numPlanes)\n> > +\t: lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)\n> > {\n> > \tfor (const std::unique_ptr &buffer : buffers)\n> > \t\tcache_.emplace_back(true,\n> > \t\t\t\t lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n> > +\t\t\t\t numPlanes_,\n> > \t\t\t\t *buffer.get());\n> > }\n> > \n> > @@ -237,6 +241,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)\n> > \n> > \tcache_[use] = Entry(false,\n> > \t\t\t lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n> > +\t\t\t numPlanes_,\n> > \t\t\t buffer);\n> > \n> > \treturn use;\n> > @@ -257,24 +262,47 @@ V4L2BufferCache::Entry::Entry()\n> > {\n> > }\n> > \n> > -V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer)\n> > +V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed,\n> > +\t\t\t unsigned int numPlanes, const FrameBuffer &buffer)\n> > \t: free_(free), lastUsed_(lastUsed)\n> > {\n> > -\tfor (const FrameBuffer::Plane &plane : buffer.planes())\n> > -\t\tplanes_.emplace_back(plane);\n> > +\tASSERT(numPlanes <= buffer.planes().size());\n> > +\tunsigned int length = 0;\n> > +\tfor (const FrameBuffer::Plane &plane : buffer.planes()) {\n> > +\t\tASSERT(plane.offset == length);\n> > +\t\tlength += plane.length;\n> > +\n> > +\t\tif (planes_.size() < numPlanes)\n> > +\t\t\tplanes_.emplace_back(plane);\n> > +\t}\n> > +\n> > +\tif (numPlanes == 1) {\n> > +\t\tplanes_[0].length = length;\n> > +\t}\n> \n> No need for curly braces.\n> \n> > }\n> > \n> > bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> > {\n> > \tconst std::vector &planes = buffer.planes();\n> > \n> > -\tif (planes_.size() != planes.size())\n> > +\tif (planes_.size() != planes.size() || planes_.size() == 1)\n> > \t\treturn false;\n> > -\n> > -\tfor (unsigned int i = 0; i < planes.size(); i++)\n> > -\t\tif (planes_[i].fd != planes[i].fd.fd() ||\n> > -\t\t planes_[i].length != planes[i].length)\n> > +\tif (planes_.size() == 1) {\n> > +\t\tunsigned int length = 0;\n> > +\t\tfor (unsigned int i = 0; i < planes.size(); i++) {\n> > +\t\t\tif (planes_[0].fd != planes[i].fd.fd())\n> > +\t\t\t\treturn false;\n> > +\t\t\tlength += planes[i].length;\n> > +\t\t}\n> > +\t\tif (length != planes_[0].length)\n> > \t\t\treturn false;\n> > +\t} else {\n> > +\t\tfor (unsigned int i = 0; i < planes.size(); i++)\n> > +\t\t\tif (planes_[i].fd != planes[i].fd.fd() ||\n> > +\t\t\t planes_[i].length != planes[i].length)\n> > +\t\t\t\treturn false;\n> > +\t}\n> > +\n> > \treturn true;\n> > }\n> \n> I'm not sure to understand why all this is needed.\n> \n> > \n> > @@ -1152,8 +1180,13 @@ int V4L2VideoDevice::requestBuffers(unsigned int count,\n> > * successful return the driver's internal buffer management is initialized in\n> > * MMAP mode, and the video device is ready to accept queueBuffer() calls.\n> > *\n> > - * The number of planes and the plane sizes for the allocation are determined\n> > - * by the currently active format on the device as set by setFormat().\n> > + * The number of planes and their offsets and sizes are determined by the\n> > + * currently active format on the device as set by setFormat(). They do not map\n> > + * to the V4L2 buffer planes, but to colour planes of the pixel format. For\n> > + * instance, if the active format is formats::NV12, the allocated FrameBuffer\n> > + * instances will have two planes, for the luma and chroma components,\n> > + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or\n> > + * V4L2_PIX_FMT_NV12M.\n> > *\n> > * Buffers allocated with this function shall later be free with\n> > * releaseBuffers(). If buffers have already been allocated with\n> > @@ -1167,11 +1200,19 @@ int V4L2VideoDevice::requestBuffers(unsigned int count,\n> > int V4L2VideoDevice::allocateBuffers(unsigned int count,\n> > \t\t\t\t std::vector> *buffers)\n> > {\n> > -\tint ret = createBuffers(count, buffers);\n> > +\tV4L2DeviceFormat format{};\n> > +\tint ret = getFormat(&format);\n> > +\tif (ret < 0) {\n> > +\t\tLOG(V4L2, Error)\n> > +\t\t\t<< \"Failed to get format: \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> \n> Instead of calling getFormat() here and in two places below, how about\n> caching the format in setFormat() ?\n> \n> > +\n> > +\tret = createBuffers(count, buffers);\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> > \n> > -\tcache_ = new V4L2BufferCache(*buffers);\n> > +\tcache_ = new V4L2BufferCache(*buffers, format.planesCount);\n> > \tmemoryType_ = V4L2_MEMORY_MMAP;\n> > \n> > \treturn ret;\n> > @@ -1190,8 +1231,13 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,\n> > * usable with any V4L2 video device in DMABUF mode, or with other dmabuf\n> > * importers.\n> > *\n> > - * The number of planes and the plane sizes for the allocation are determined\n> > - * by the currently active format on the device as set by setFormat().\n> > + * The number of planes and their offsets and sizes are determined by the\n> > + * currently active format on the device as set by setFormat(). They do not map\n> > + * to the V4L2 buffer planes, but to colour planes of the pixel format. For\n> > + * instance, if the active format is formats::NV12, the allocated FrameBuffer\n> > + * instances will have two planes, for the luma and chroma components,\n> > + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or\n> > + * V4L2_PIX_FMT_NV12M.\n> > *\n> > * Multiple independent sets of buffers can be allocated with multiple calls to\n> > * this function. Device-specific limitations may apply regarding the minimum\n> > @@ -1283,12 +1329,49 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\n> > \n> > \t\tFrameBuffer::Plane plane;\n> > \t\tplane.fd = std::move(fd);\n> > -\t\tplane.length = multiPlanar ?\n> > -\t\t\tbuf.m.planes[nplane].length : buf.length;\n> > +\t\t/*\n> > +\t\t * V4L2 API doesn't provide dmabuf offset information of plane.\n> > +\t\t * Set 0 as a placeholder offset.\n> > +\t\t * \\todo Set the right offset once V4L2 API provides a way.\n> > +\t\t */\n> > +\t\tplane.offset = 0;\n> > +\t\tplane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;\n> > \n> > \t\tplanes.push_back(std::move(plane));\n> > \t}\n> > \n> > +\t/*\n> > +\t * Get V4L2DeviceFormat of the frame buffer. If it is V4L2 single-planar\n> > +\t * format, overwrite FrameBuffer::Plane to make it color format planes.\n> > +\t * */\n> > +\tV4L2DeviceFormat format{};\n> > +\tret = getFormat(&format);\n> > +\tif (ret < 0) {\n> > +\t\tLOG(V4L2, Error)\n> > +\t\t\t<< \"Failed to get format: \" << strerror(-ret);\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\tconst auto &info = PixelFormatInfo::info(format.fourcc);\n> > +\tif (info.isValid() && info.numPlanes() != numPlanes) {\n> \n> If info isn't valid, should we return nullptr ?\n> \n> > +\t\tASSERT(numPlanes == 1u);\n> > +\t\tconst size_t numColorPlanes = info.numPlanes();\n> > +\t\tplanes.resize(numColorPlanes);\n> > +\t\tconst FileDescriptor &fd = planes[0].fd;\n> > +\t\tsize_t offset = 0;\n> > +\t\tfor (size_t i = 0; i < numColorPlanes; ++i) {\n> > +\t\t\tplanes[i].fd = fd;\n> > +\t\t\tplanes[i].offset = offset;\n> > +\n> \n> Could you please add a\n> \n> \t\t\t/* \\todo Take the V4L2 stride into account */\n> \n> comment ? No need to address this now, but it will need to be done at\n> some point.\n> \n> > +\t\t\tconst unsigned int vertSubSample =\n> > +\t\t\t\tinfo.planes[i].verticalSubSampling;\n> > +\t\t\tplanes[i].length =\n> > +\t\t\t\tinfo.stride(format.size.width, i, 1u) *\n> > +\t\t\t\t((format.size.height + vertSubSample - 1) / vertSubSample);\n> > +\t\t\toffset += planes[i].length;\n> > +\t\t}\n> > +\t}\n> > +\n> > \treturn std::make_unique(std::move(planes));\n> > }\n> > \n> > @@ -1342,11 +1425,19 @@ int V4L2VideoDevice::importBuffers(unsigned int count)\n> > \n> > \tmemoryType_ = V4L2_MEMORY_DMABUF;\n> > \n> > -\tint ret = requestBuffers(count, V4L2_MEMORY_DMABUF);\n> > +\tV4L2DeviceFormat format{};\n> > +\tint ret = getFormat(&format);\n> > +\tif (ret < 0) {\n> > +\t\tLOG(V4L2, Error)\n> > +\t\t\t<< \"Failed to get format: \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tret = requestBuffers(count, V4L2_MEMORY_DMABUF);\n> > \tif (ret)\n> > \t\treturn ret;\n> > \n> > -\tcache_ = new V4L2BufferCache(count);\n> > +\tcache_ = new V4L2BufferCache(count, format.planesCount);\n> > \n> > \tLOG(V4L2, Debug) << \"Prepared to import \" << count << \" buffers\";\n> > \n> > diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp\n> > index b3f2bec1..48f748bc 100644\n> > --- a/test/v4l2_videodevice/buffer_cache.cpp\n> > +++ b/test/v4l2_videodevice/buffer_cache.cpp\n> > @@ -166,7 +166,7 @@ public:\n> > \t\t * Test cache of same size as there are buffers, the cache is\n> > \t\t * created from a list of buffers and will be pre-populated.\n> > \t\t */\n> > -\t\tV4L2BufferCache cacheFromBuffers(buffers);\n> > +\t\tV4L2BufferCache cacheFromBuffers(buffers, 1u);\n> > \n> > \t\tif (testSequential(&cacheFromBuffers, buffers) != TestPass)\n> > \t\t\treturn TestFail;\n> > @@ -181,7 +181,7 @@ public:\n> > \t\t * Test cache of same size as there are buffers, the cache is\n> > \t\t * not pre-populated.\n> > \t\t */\n> > -\t\tV4L2BufferCache cacheFromNumbers(numBuffers);\n> > +\t\tV4L2BufferCache cacheFromNumbers(numBuffers, 1u);\n> > \n> > \t\tif (testSequential(&cacheFromNumbers, buffers) != TestPass)\n> > \t\t\treturn TestFail;\n> > @@ -196,7 +196,7 @@ public:\n> > \t\t * Test cache half the size of number of buffers used, the cache\n> > \t\t * is not pre-populated.\n> > \t\t */\n> > -\t\tV4L2BufferCache cacheHalf(numBuffers / 2);\n> > +\t\tV4L2BufferCache cacheHalf(numBuffers / 2, 1u);\n> > \n> > \t\tif (testRandom(&cacheHalf, buffers) != TestPass)\n> > \t\t\treturn TestFail;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C1FF6BD87C\n\tfor ;\n\tTue, 24 Aug 2021 00:27:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B86B688A3;\n\tTue, 24 Aug 2021 02:27:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EAE7E68890\n\tfor ;\n\tTue, 24 Aug 2021 02:27:07 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 63F2F2A5;\n\tTue, 24 Aug 2021 02:27:07 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GDgoYwzi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629764827;\n\tbh=UWQYtXr0IzY1p+tH/YDPwuN9RnEX1P6/+rrbtBfzKS4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GDgoYwzin7INxQIcUpIUbGGkbETn3Y3Q9cjpNftoZDc3u+LbRczIgPnpk+9yHT7LT\n\t2GvoD4jHVj/M2b1Nzn+tqlNYZNfWuU2dWyuQcqJ987w6+4McDyq01Ww2cmKDYK5IcQ\n\ts1n7qeldSInKnvaC98YcioQ7oqXqgF6RyW0X/A3c=","Date":"Tue, 24 Aug 2021 03:26:57 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-9-hiroh@chromium.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 08/10] libcamera:\n\tv4l2_videodevice: Create color-format planes in createBuffer()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19035,"web_url":"https://patchwork.libcamera.org/comment/19035/","msgid":"","date":"2021-08-25T07:13:21","subject":"Re: [libcamera-devel] [RFC PATCH v2 08/10] libcamera:\n\tv4l2_videodevice: Create color-format planes in createBuffer()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for reviewing.\n\nOn Tue, Aug 24, 2021 at 9:27 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> One more comment.\n>\n> On Tue, Aug 24, 2021 at 02:54:56AM +0300, Laurent Pinchart wrote:\n> > On Mon, Aug 23, 2021 at 10:12:19PM +0900, Hirokazu Honda wrote:\n> > > V4L2VideDevice::createBuffer() creates the same number of\n> > > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2\n> > > format single is single-planar format, the created number of\n> > > FrameBuffer::Planes is 1.\n> > > It should rather create the same number of FrameBuffer::Planes as the\n> > > color format planes.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > include/libcamera/internal/v4l2_videodevice.h | 9 +-\n> > > src/libcamera/v4l2_videodevice.cpp | 147 ++++++++++++++----\n> > > test/v4l2_videodevice/buffer_cache.cpp | 6 +-\n> > > 3 files changed, 128 insertions(+), 34 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > index e767ec84..69bb964a 100644\n> > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > @@ -114,8 +114,9 @@ struct V4L2Capability final : v4l2_capability {\n> > > class V4L2BufferCache\n> > > {\n> > > public:\n> > > - V4L2BufferCache(unsigned int numEntries);\n> > > - V4L2BufferCache(const std::vector> &buffers);\n> > > + V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes);\n> > > + V4L2BufferCache(const std::vector> &buffers,\n> > > + unsigned int numPlanes);\n>\n> I'm getting the following doxygen warnings:\n>\n> include/libcamera/internal/v4l2_videodevice.h:117: warning: The following parameter of libcamera::V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes) is not documented:\n> parameter 'numPlanes'\n> 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:\n> parameter 'numPlanes'\n>\n> > > ~V4L2BufferCache();\n> > >\n> > > int get(const FrameBuffer &buffer);\n> > > @@ -126,7 +127,8 @@ private:\n> > > {\n> > > public:\n> > > Entry();\n> > > - Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer);\n> > > + Entry(bool free, uint64_t lastUsed, unsigned int numPlanes,\n> > > + const FrameBuffer &buffer);\n> > >\n> > > bool operator==(const FrameBuffer &buffer) const;\n> > >\n> > > @@ -149,6 +151,7 @@ private:\n> > >\n> > > std::atomic lastUsedCounter_;\n> > > std::vector cache_;\n> > > + unsigned int numPlanes_;\n> > > /* \\todo Expose the miss counter through an instrumentation API. */\n> > > unsigned int missCounter_;\n> > > };\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index ce60dff6..42b66bd3 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -25,6 +25,7 @@\n> > >\n> > > #include \n> > >\n> > > +#include \"libcamera/internal/formats.h\"\n> > > #include \"libcamera/internal/media_device.h\"\n> > > #include \"libcamera/internal/media_object.h\"\n> > >\n> > > @@ -158,12 +159,13 @@ LOG_DECLARE_CATEGORY(V4L2)\n> > > * \\brief Create an empty cache with \\a numEntries entries\n> > > * \\param[in] numEntries Number of entries to reserve in the cache\n> > > *\n> > > - * Create a cache with \\a numEntries entries all marked as unused. The entries\n> > > - * will be populated as the cache is used. This is typically used to implement\n> > > - * buffer import, with buffers added to the cache as they are queued.\n> > > + * Create a cache with \\a numEntries entries all marked as unused. The entry is\n> > > + * for \\a numPlanes planes V4L2 buffer. The entries will be populated as the\n> > > + * cache is used. This is typically used to implement buffer import, with\n> > > + * buffers added to the cache as they are queued.\n> > > */\n> > > -V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)\n> > > - : lastUsedCounter_(1), missCounter_(0)\n> > > +V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes)\n> > > + : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)\n> > > {\n> > > cache_.resize(numEntries);\n> > > }\n> > > @@ -172,16 +174,18 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)\n> > > * \\brief Create a pre-populated cache\n> > > * \\param[in] buffers Array of buffers to pre-populated with\n> > > *\n> > > - * Create a cache pre-populated with \\a buffers. This is typically used to\n> > > - * implement buffer export, with all buffers added to the cache when they are\n> > > - * allocated.\n> > > + * Create a cache pre-populated with \\a buffers. The entry is for \\a numPlanes\n> > > + * planes V4L2 buffer. This is typically used to implement buffer export, with\n> > > + * all buffers added to the cache when they are allocated.\n> > > */\n> > > -V4L2BufferCache::V4L2BufferCache(const std::vector> &buffers)\n> > > - : lastUsedCounter_(1), missCounter_(0)\n> > > +V4L2BufferCache::V4L2BufferCache(const std::vector> &buffers,\n> > > + unsigned int numPlanes)\n> > > + : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)\n> > > {\n> > > for (const std::unique_ptr &buffer : buffers)\n> > > cache_.emplace_back(true,\n> > > lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n> > > + numPlanes_,\n> > > *buffer.get());\n> > > }\n> > >\n> > > @@ -237,6 +241,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)\n> > >\n> > > cache_[use] = Entry(false,\n> > > lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n> > > + numPlanes_,\n> > > buffer);\n> > >\n> > > return use;\n> > > @@ -257,24 +262,47 @@ V4L2BufferCache::Entry::Entry()\n> > > {\n> > > }\n> > >\n> > > -V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer)\n> > > +V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed,\n> > > + unsigned int numPlanes, const FrameBuffer &buffer)\n> > > : free_(free), lastUsed_(lastUsed)\n> > > {\n> > > - for (const FrameBuffer::Plane &plane : buffer.planes())\n> > > - planes_.emplace_back(plane);\n> > > + ASSERT(numPlanes <= buffer.planes().size());\n> > > + unsigned int length = 0;\n> > > + for (const FrameBuffer::Plane &plane : buffer.planes()) {\n> > > + ASSERT(plane.offset == length);\n> > > + length += plane.length;\n> > > +\n> > > + if (planes_.size() < numPlanes)\n> > > + planes_.emplace_back(plane);\n> > > + }\n> > > +\n> > > + if (numPlanes == 1) {\n> > > + planes_[0].length = length;\n> > > + }\n> >\n> > No need for curly braces.\n> >\n> > > }\n> > >\n> > > bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> > > {\n> > > const std::vector &planes = buffer.planes();\n> > >\n> > > - if (planes_.size() != planes.size())\n> > > + if (planes_.size() != planes.size() || planes_.size() == 1)\n> > > return false;\n> > > -\n> > > - for (unsigned int i = 0; i < planes.size(); i++)\n> > > - if (planes_[i].fd != planes[i].fd.fd() ||\n> > > - planes_[i].length != planes[i].length)\n> > > + if (planes_.size() == 1) {\n> > > + unsigned int length = 0;\n> > > + for (unsigned int i = 0; i < planes.size(); i++) {\n> > > + if (planes_[0].fd != planes[i].fd.fd())\n> > > + return false;\n> > > + length += planes[i].length;\n> > > + }\n> > > + if (length != planes_[0].length)\n> > > return false;\n> > > + } else {\n> > > + for (unsigned int i = 0; i < planes.size(); i++)\n> > > + if (planes_[i].fd != planes[i].fd.fd() ||\n> > > + planes_[i].length != planes[i].length)\n> > > + return false;\n> > > + }\n> > > +\n> > > return true;\n> > > }\n> >\n> > I'm not sure to understand why all this is needed.\n> >\n\nI will add a comment to the next patch series.\n\n\n> > >\n> > > @@ -1152,8 +1180,13 @@ int V4L2VideoDevice::requestBuffers(unsigned int count,\n> > > * successful return the driver's internal buffer management is initialized in\n> > > * MMAP mode, and the video device is ready to accept queueBuffer() calls.\n> > > *\n> > > - * The number of planes and the plane sizes for the allocation are determined\n> > > - * by the currently active format on the device as set by setFormat().\n> > > + * The number of planes and their offsets and sizes are determined by the\n> > > + * currently active format on the device as set by setFormat(). They do not map\n> > > + * to the V4L2 buffer planes, but to colour planes of the pixel format. For\n> > > + * instance, if the active format is formats::NV12, the allocated FrameBuffer\n> > > + * instances will have two planes, for the luma and chroma components,\n> > > + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or\n> > > + * V4L2_PIX_FMT_NV12M.\n> > > *\n> > > * Buffers allocated with this function shall later be free with\n> > > * releaseBuffers(). If buffers have already been allocated with\n> > > @@ -1167,11 +1200,19 @@ int V4L2VideoDevice::requestBuffers(unsigned int count,\n> > > int V4L2VideoDevice::allocateBuffers(unsigned int count,\n> > > std::vector> *buffers)\n> > > {\n> > > - int ret = createBuffers(count, buffers);\n> > > + V4L2DeviceFormat format{};\n> > > + int ret = getFormat(&format);\n> > > + if (ret < 0) {\n> > > + LOG(V4L2, Error)\n> > > + << \"Failed to get format: \" << strerror(-ret);\n> > > + return ret;\n> > > + }\n> >\n> > Instead of calling getFormat() here and in two places below, how about\n> > caching the format in setFormat() ?\n> >\n> > > +\n> > > + ret = createBuffers(count, buffers);\n> > > if (ret < 0)\n> > > return ret;\n> > >\n> > > - cache_ = new V4L2BufferCache(*buffers);\n> > > + cache_ = new V4L2BufferCache(*buffers, format.planesCount);\n> > > memoryType_ = V4L2_MEMORY_MMAP;\n> > >\n> > > return ret;\n> > > @@ -1190,8 +1231,13 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,\n> > > * usable with any V4L2 video device in DMABUF mode, or with other dmabuf\n> > > * importers.\n> > > *\n> > > - * The number of planes and the plane sizes for the allocation are determined\n> > > - * by the currently active format on the device as set by setFormat().\n> > > + * The number of planes and their offsets and sizes are determined by the\n> > > + * currently active format on the device as set by setFormat(). They do not map\n> > > + * to the V4L2 buffer planes, but to colour planes of the pixel format. For\n> > > + * instance, if the active format is formats::NV12, the allocated FrameBuffer\n> > > + * instances will have two planes, for the luma and chroma components,\n> > > + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or\n> > > + * V4L2_PIX_FMT_NV12M.\n> > > *\n> > > * Multiple independent sets of buffers can be allocated with multiple calls to\n> > > * this function. Device-specific limitations may apply regarding the minimum\n> > > @@ -1283,12 +1329,49 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\n> > >\n> > > FrameBuffer::Plane plane;\n> > > plane.fd = std::move(fd);\n> > > - plane.length = multiPlanar ?\n> > > - buf.m.planes[nplane].length : buf.length;\n> > > + /*\n> > > + * V4L2 API doesn't provide dmabuf offset information of plane.\n> > > + * Set 0 as a placeholder offset.\n> > > + * \\todo Set the right offset once V4L2 API provides a way.\n> > > + */\n> > > + plane.offset = 0;\n> > > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;\n> > >\n> > > planes.push_back(std::move(plane));\n> > > }\n> > >\n> > > + /*\n> > > + * Get V4L2DeviceFormat of the frame buffer. If it is V4L2 single-planar\n> > > + * format, overwrite FrameBuffer::Plane to make it color format planes.\n> > > + * */\n> > > + V4L2DeviceFormat format{};\n> > > + ret = getFormat(&format);\n> > > + if (ret < 0) {\n> > > + LOG(V4L2, Error)\n> > > + << \"Failed to get format: \" << strerror(-ret);\n> > > + return nullptr;\n> > > + }\n> > > +\n> > > + const auto &info = PixelFormatInfo::info(format.fourcc);\n> > > + if (info.isValid() && info.numPlanes() != numPlanes) {\n> >\n> > If info isn't valid, should we return nullptr ?\n\ncreateBuffer() is called for non pixel format buffer for example,\n/dev/video10[33:out]: Invalid format: 0x0-ip3p\n\nI need to check by info.isValid() to filter out non pixel formats.\n\n-Hiro\n> >\n> > > + ASSERT(numPlanes == 1u);\n> > > + const size_t numColorPlanes = info.numPlanes();\n> > > + planes.resize(numColorPlanes);\n> > > + const FileDescriptor &fd = planes[0].fd;\n> > > + size_t offset = 0;\n> > > + for (size_t i = 0; i < numColorPlanes; ++i) {\n> > > + planes[i].fd = fd;\n> > > + planes[i].offset = offset;\n> > > +\n> >\n> > Could you please add a\n> >\n> > /* \\todo Take the V4L2 stride into account */\n> >\n> > comment ? No need to address this now, but it will need to be done at\n> > some point.\n> >\n> > > + const unsigned int vertSubSample =\n> > > + info.planes[i].verticalSubSampling;\n> > > + planes[i].length =\n> > > + info.stride(format.size.width, i, 1u) *\n> > > + ((format.size.height + vertSubSample - 1) / vertSubSample);\n> > > + offset += planes[i].length;\n> > > + }\n> > > + }\n> > > +\n> > > return std::make_unique(std::move(planes));\n> > > }\n> > >\n> > > @@ -1342,11 +1425,19 @@ int V4L2VideoDevice::importBuffers(unsigned int count)\n> > >\n> > > memoryType_ = V4L2_MEMORY_DMABUF;\n> > >\n> > > - int ret = requestBuffers(count, V4L2_MEMORY_DMABUF);\n> > > + V4L2DeviceFormat format{};\n> > > + int ret = getFormat(&format);\n> > > + if (ret < 0) {\n> > > + LOG(V4L2, Error)\n> > > + << \"Failed to get format: \" << strerror(-ret);\n> > > + return ret;\n> > > + }\n> > > +\n> > > + ret = requestBuffers(count, V4L2_MEMORY_DMABUF);\n> > > if (ret)\n> > > return ret;\n> > >\n> > > - cache_ = new V4L2BufferCache(count);\n> > > + cache_ = new V4L2BufferCache(count, format.planesCount);\n> > >\n> > > LOG(V4L2, Debug) << \"Prepared to import \" << count << \" buffers\";\n> > >\n> > > diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp\n> > > index b3f2bec1..48f748bc 100644\n> > > --- a/test/v4l2_videodevice/buffer_cache.cpp\n> > > +++ b/test/v4l2_videodevice/buffer_cache.cpp\n> > > @@ -166,7 +166,7 @@ public:\n> > > * Test cache of same size as there are buffers, the cache is\n> > > * created from a list of buffers and will be pre-populated.\n> > > */\n> > > - V4L2BufferCache cacheFromBuffers(buffers);\n> > > + V4L2BufferCache cacheFromBuffers(buffers, 1u);\n> > >\n> > > if (testSequential(&cacheFromBuffers, buffers) != TestPass)\n> > > return TestFail;\n> > > @@ -181,7 +181,7 @@ public:\n> > > * Test cache of same size as there are buffers, the cache is\n> > > * not pre-populated.\n> > > */\n> > > - V4L2BufferCache cacheFromNumbers(numBuffers);\n> > > + V4L2BufferCache cacheFromNumbers(numBuffers, 1u);\n> > >\n> > > if (testSequential(&cacheFromNumbers, buffers) != TestPass)\n> > > return TestFail;\n> > > @@ -196,7 +196,7 @@ public:\n> > > * Test cache half the size of number of buffers used, the cache\n> > > * is not pre-populated.\n> > > */\n> > > - V4L2BufferCache cacheHalf(numBuffers / 2);\n> > > + V4L2BufferCache cacheHalf(numBuffers / 2, 1u);\n> > >\n> > > if (testRandom(&cacheHalf, buffers) != TestPass)\n> > > return TestFail;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5D425BD87D\n\tfor ;\n\tWed, 25 Aug 2021 07:13:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B9DC5688A6;\n\tWed, 25 Aug 2021 09:13:34 +0200 (CEST)","from mail-ed1-x535.google.com (mail-ed1-x535.google.com\n\t[IPv6:2a00:1450:4864:20::535])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 82ED860259\n\tfor ;\n\tWed, 25 Aug 2021 09:13:33 +0200 (CEST)","by mail-ed1-x535.google.com with SMTP id q3so35511651edt.5\n\tfor ;\n\tWed, 25 Aug 2021 00:13:33 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"QKI2YWFJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=4O+EHcaoRFWbjOPt33d6qOSmufrMnwCh7mNIat4+vfw=;\n\tb=QKI2YWFJPPD71lBeRVOf/8GkN0jc3rkUqnydHTUYgwBX6fbsgRnjKj01XAT/dwloVK\n\ttdXOKW6On2f5kwrieH2n4P6erBy/URZR1JvY239i0FePe2oxo22yudAS+hVHDBHJXA+3\n\tv3akSj3o7BUAq+/IJjQhhrbdpw2MikAHKC8JQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=4O+EHcaoRFWbjOPt33d6qOSmufrMnwCh7mNIat4+vfw=;\n\tb=Dc3SVGXFF5/4gRPPwFBrtTIFVdIPEtcIdHe/3k/TsYIv7S8fZ0XzX520qS53mFwCPh\n\tecaldzY0rP9l14AdB/PhBRIFCh5PW2x+tQNaVxgXWYyPXZMqBk54YwKBFk/bwrzGQX0z\n\t15LOncLioLFsNzdJd75EJsblO500R6YzPgScvxbeOqWMr9wOtfKlgjzDkalgodpVdJ4S\n\tnqC4mS6jzI2jF2Jcutfm8VWLwlfYzM8HFYca8AZ9akl9kjbCTFs5UQp5hFaa5dFOC/FX\n\t8RfjCV2cSA817KbL9Wmw/0UBCY/564b1wlRggbNhLBnhDs8EiiLCIKZ2/SbVktz5Q1DW\n\tG+gQ==","X-Gm-Message-State":"AOAM531udZkL2ni+fEmdVC8f3Bi6ekHaguXAKkMT/H9OKojsTQ7dOIxn\n\tM8GmrL8mSTFZDyUu12Y0gpUjXNDqPVqCyuv9AIiwTQ==","X-Google-Smtp-Source":"ABdhPJy1thEJOys9r8OZeWs9m3E9aNVgkjHenkhJe6FkBsY6MRl4g7e+qmpWxDt5IaYqiTfVCb4jWPcJ7Or9NZtbLys=","X-Received":"by 2002:a05:6402:34d6:: with SMTP id\n\tw22mr47725284edc.244.1629875612855; \n\tWed, 25 Aug 2021 00:13:32 -0700 (PDT)","MIME-Version":"1.0","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-9-hiroh@chromium.org>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 25 Aug 2021 16:13:21 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 08/10] libcamera:\n\tv4l2_videodevice: Create color-format planes in createBuffer()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]