[{"id":19089,"web_url":"https://patchwork.libcamera.org/comment/19089/","msgid":"","date":"2021-08-26T11:32:18","subject":"Re: [libcamera-devel] [PATCH v3 7/9] V4L2VideDevice::createBuffer()\n\tcreates the same number of FrameBuffer::Planes as V4L2 format\n\tplanes.\n\tTherefore, if the v4l2 format single is single-planar format,\n\tthe created number of FrameBuffer::Planes is 1. It should rather\n\tcreate the\n\tsame number of FrameBuffer::Planes as the color format planes.","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\nI think the subject line is a bit long ;-)\n\nI'll review this as part of the series right after \"[PATCH v3 0/3]\nImprove CameraBuffer implementation\".\n\nOn Thu, Aug 26, 2021 at 08:25:37PM +0900, Hirokazu Honda wrote:\n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/internal/v4l2_videodevice.h | 10 +-\n> src/libcamera/v4l2_videodevice.cpp | 141 ++++++++++++++----\n> test/v4l2_videodevice/buffer_cache.cpp | 6 +-\n> 3 files changed, 123 insertions(+), 34 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index e767ec84..bfda6726 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> @@ -242,6 +245,7 @@ private:\n> \tFrameBuffer *dequeueBuffer();\n> \n> \tV4L2Capability caps_;\n> +\tV4L2DeviceFormat format_;\n> \n> \tenum v4l2_buf_type bufferType_;\n> \tenum v4l2_memory memoryType_;\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 2ff25af2..f5f8741a 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> @@ -157,13 +158,15 @@ LOG_DECLARE_CATEGORY(V4L2)\n> /**\n> * \\brief Create an empty cache with \\a numEntries entries\n> * \\param[in] numEntries Number of entries to reserve in the cache\n> + * \\param[in] numPlanes Number of V4L2 buffer planes\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> @@ -171,17 +174,20 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)\n> /**\n> * \\brief Create a pre-populated cache\n> * \\param[in] buffers Array of buffers to pre-populated with\n> + * \\param[in] numPlanes Number of V4L2 buffer planes\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 +243,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 +264,53 @@ 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> }\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\t/*\n> +\t\t * planes_ is V4L2 single-planar format buffer. fds of\n> +\t\t * FrameBuffer::Plane must be identical and the sum of plane\n> +\t\t * size is the V4L2 buffer length.\n> +\t\t */\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\t/* planes_ is V4L2 multi-planar format buffer. */\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> @@ -579,6 +615,12 @@ int V4L2VideoDevice::open()\n> \t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> \t\t<< caps_.driver() << \": \" << caps_.card();\n> \n> +\tret = getFormat(&format_);\n> +\tif (ret) {\n> +\t\tLOG(V4L2, Error) << \"Failed to get format\";\n> +\t\treturn ret;\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> @@ -668,6 +710,12 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> \t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> \t\t<< caps_.driver() << \": \" << caps_.card();\n> \n> +\tret = getFormat(&format_);\n> +\tif (ret) {\n> +\t\tLOG(V4L2, Error) << \"Failed to get format\";\n> +\t\treturn ret;\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> @@ -761,12 +809,19 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n> */\n> int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)\n> {\n> +\tint ret = 0;\n> \tif (caps_.isMeta())\n> -\t\treturn trySetFormatMeta(format, true);\n> +\t\tret = trySetFormatMeta(format, true);\n> \telse if (caps_.isMultiplanar())\n> -\t\treturn trySetFormatMultiplane(format, true);\n> +\t\tret = trySetFormatMultiplane(format, true);\n> \telse\n> -\t\treturn trySetFormatSingleplane(format, true);\n> +\t\tret = trySetFormatSingleplane(format, true);\n> +\n> +\t/* Cache the set format on success. */\n> +\tif (ret == 0)\n> +\t\tformat_ = *format;\n> +\n> +\treturn ret;\n> }\n> \n> int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)\n> @@ -1152,8 +1207,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> @@ -1171,7 +1231,7 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,\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 +1250,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> @@ -1289,12 +1354,32 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\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 ?\n> -\t\t\tbuf.m.planes[nplane].length : buf.length;\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> +\tconst auto &info = PixelFormatInfo::info(format_.fourcc);\n> +\tif (info.isValid() && info.numPlanes() != numPlanes) {\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> +\t\t\t/* \\todo Take the V4L2 stride into account */\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> @@ -1352,7 +1437,7 @@ int V4L2VideoDevice::importBuffers(unsigned int count)\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 B0504BD87D\n\tfor ;\n\tThu, 26 Aug 2021 11:32:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1CA10688E5;\n\tThu, 26 Aug 2021 13:32:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 15B836888F\n\tfor ;\n\tThu, 26 Aug 2021 13:32:32 +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 86F6A191F;\n\tThu, 26 Aug 2021 13:32:31 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ke8/7w9Q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629977551;\n\tbh=8pu3IjwJu8SW/SMf5XSREKGAciMWPaucJsQk+pp1gXs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ke8/7w9Qs/dZhig+ycuSiJMU11hnyWPRmDUJrfk9rYQx/gAqCmouHRly7BOl+dZvr\n\taVxP3VVP5xMf62ukUO2zBiE7iT411TnR11lSI9gVJ07xG/apztL31TiI1MwfcDM2v1\n\teWrb16vH/W8dvuIX9UUpAn+R5iVgRp5UIrTt+73w=","Date":"Thu, 26 Aug 2021 14:32:18 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210826112539.170694-1-hiroh@chromium.org>\n\t<20210826112539.170694-8-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210826112539.170694-8-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3 7/9] V4L2VideDevice::createBuffer()\n\tcreates the same number of FrameBuffer::Planes as V4L2 format\n\tplanes.\n\tTherefore, if the v4l2 format single is single-planar format,\n\tthe created number of FrameBuffer::Planes is 1. It should rather\n\tcreate the\n\tsame number of FrameBuffer::Planes as the color format planes.","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":19090,"web_url":"https://patchwork.libcamera.org/comment/19090/","msgid":"","date":"2021-08-26T11:33:53","subject":"Re: [libcamera-devel] [PATCH v3 7/9] V4L2VideDevice::createBuffer()\n\tcreates the same number of FrameBuffer::Planes as V4L2 format\n\tplanes.\n\tTherefore, if the v4l2 format single is single-planar format,\n\tthe created number of FrameBuffer::Planes is 1. It should rather\n\tcreate the\n\tsame number of FrameBuffer::Planes as the color format planes.","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Thu, Aug 26, 2021 at 8:32 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> I think the subject line is a bit long ;-)\n>\n\nOops... I somehow remove a blank line between the subject and message. :-)\n\n\n> I'll review this as part of the series right after \"[PATCH v3 0/3]\n> Improve CameraBuffer implementation\".\n>\n> On Thu, Aug 26, 2021 at 08:25:37PM +0900, Hirokazu Honda wrote:\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/internal/v4l2_videodevice.h | 10 +-\n> > src/libcamera/v4l2_videodevice.cpp | 141 ++++++++++++++----\n> > test/v4l2_videodevice/buffer_cache.cpp | 6 +-\n> > 3 files changed, 123 insertions(+), 34 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > index e767ec84..bfda6726 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> > ~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> > @@ -242,6 +245,7 @@ private:\n> > FrameBuffer *dequeueBuffer();\n> >\n> > V4L2Capability caps_;\n> > + V4L2DeviceFormat format_;\n> >\n> > enum v4l2_buf_type bufferType_;\n> > enum v4l2_memory memoryType_;\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 2ff25af2..f5f8741a 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> > @@ -157,13 +158,15 @@ LOG_DECLARE_CATEGORY(V4L2)\n> > /**\n> > * \\brief Create an empty cache with \\a numEntries entries\n> > * \\param[in] numEntries Number of entries to reserve in the cache\n> > + * \\param[in] numPlanes Number of V4L2 buffer planes\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> > @@ -171,17 +174,20 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)\n> > /**\n> > * \\brief Create a pre-populated cache\n> > * \\param[in] buffers Array of buffers to pre-populated with\n> > + * \\param[in] numPlanes Number of V4L2 buffer planes\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 +243,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 +264,53 @@ 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> > 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> > + /*\n> > + * planes_ is V4L2 single-planar format buffer. fds of\n> > + * FrameBuffer::Plane must be identical and the sum of plane\n> > + * size is the V4L2 buffer length.\n> > + */\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> > + /* planes_ is V4L2 multi-planar format buffer. */\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> > @@ -579,6 +615,12 @@ int V4L2VideoDevice::open()\n> > << \"Opened device \" << caps_.bus_info() << \": \"\n> > << caps_.driver() << \": \" << caps_.card();\n> >\n> > + ret = getFormat(&format_);\n> > + if (ret) {\n> > + LOG(V4L2, Error) << \"Failed to get format\";\n> > + return ret;\n> > + }\n> > +\n> > return 0;\n> > }\n> >\n> > @@ -668,6 +710,12 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> > << \"Opened device \" << caps_.bus_info() << \": \"\n> > << caps_.driver() << \": \" << caps_.card();\n> >\n> > + ret = getFormat(&format_);\n> > + if (ret) {\n> > + LOG(V4L2, Error) << \"Failed to get format\";\n> > + return ret;\n> > + }\n> > +\n> > return 0;\n> > }\n> >\n> > @@ -761,12 +809,19 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n> > */\n> > int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)\n> > {\n> > + int ret = 0;\n> > if (caps_.isMeta())\n> > - return trySetFormatMeta(format, true);\n> > + ret = trySetFormatMeta(format, true);\n> > else if (caps_.isMultiplanar())\n> > - return trySetFormatMultiplane(format, true);\n> > + ret = trySetFormatMultiplane(format, true);\n> > else\n> > - return trySetFormatSingleplane(format, true);\n> > + ret = trySetFormatSingleplane(format, true);\n> > +\n> > + /* Cache the set format on success. */\n> > + if (ret == 0)\n> > + format_ = *format;\n> > +\n> > + return ret;\n> > }\n> >\n> > int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)\n> > @@ -1152,8 +1207,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> > @@ -1171,7 +1231,7 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,\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 +1250,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> > @@ -1289,12 +1354,32 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\n> > * \\todo Set the right offset once V4L2 API provides a way.\n> > */\n> > plane.offset = 0;\n> > - plane.length = multiPlanar ?\n> > - buf.m.planes[nplane].length : buf.length;\n> > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;\n> >\n> > planes.push_back(std::move(plane));\n> > }\n> >\n> > + const auto &info = PixelFormatInfo::info(format_.fourcc);\n> > + if (info.isValid() && info.numPlanes() != numPlanes) {\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> > + /* \\todo Take the V4L2 stride into account */\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> > @@ -1352,7 +1437,7 @@ int V4L2VideoDevice::importBuffers(unsigned int count)\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 3E633BD87D\n\tfor ;\n\tThu, 26 Aug 2021 11:34:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 06B7A6890E;\n\tThu, 26 Aug 2021 13:34:08 +0200 (CEST)","from mail-ej1-x62d.google.com (mail-ej1-x62d.google.com\n\t[IPv6:2a00:1450:4864:20::62d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E32AD688A5\n\tfor ;\n\tThu, 26 Aug 2021 13:34:05 +0200 (CEST)","by mail-ej1-x62d.google.com with SMTP id mf2so5521968ejb.9\n\tfor ;\n\tThu, 26 Aug 2021 04:34:05 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"A1sBL2aU\"; 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=QfU2yTIqFf0fFPPgyhnx8dQ20+/LGrAFnWw28ssXKbU=;\n\tb=A1sBL2aUZlZqt9OWY+9hOitXXmtppdby91sLNL+CNteZT1Ej+/2GC4T0+PAGgWCmF0\n\tln7NYQiQo+v0XaCAW7ZUVaqbsk9JHxbtgQ041E9ItBYBsBY3qARP8rHnIJMhYhe0jGgY\n\tkAtnw1WG79N5cd2Ti9oQs8lolq9vu6fgz/kCY=","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=QfU2yTIqFf0fFPPgyhnx8dQ20+/LGrAFnWw28ssXKbU=;\n\tb=YKfv3mwiLyYwVRNuBUcmDyLrtxGS+gOxaga7ipi2/iT4fzNGZYe+AK/MBy49/aybHu\n\t97mD4v1m6VmUfYLVQoyOJUJ/h4zwLhr2GJlAnkIw5dpEsYeDxsy0QVxTtMNjVg+lXVae\n\ttk/djgPnwWMVoPDfqrw9Mg79kfWK9vetPAQ2VxAIEVlWwBkd5KIIq/XKTkgu8ZYbjF8B\n\tNnRYxq1GLXrIR63EGhnMtzzCarRhJ8DXVy6D66YLlM/AtpvAcPJqDTlj9b7Fi/uW3Kqi\n\t9VhGlLRNT3Wh/rISS/DQiCkYuvAv26xAXuHsOo+1xMl66LYz5YVo++TW8RATrFYaeKin\n\tV3Og==","X-Gm-Message-State":"AOAM5316IiHJhtPggp06OHIpENBZpJI7zgmD7kAuEgZ9HCcdyy6GOku2\n\tJE7fPiTE2Q7MaznAtU6TiPpYY/x0moawOBtAvKBO2Q==","X-Google-Smtp-Source":"ABdhPJyZQC+EU+X4XbgqF+hT+oglZXFHVlOgfEZ/Y2jn9cW3MVTqSLAr+xA2q7/9Fzb8/y+1QnerMr/xIqw/7E7emfQ=","X-Received":"by 2002:a17:906:57c1:: with SMTP id\n\tu1mr3889793ejr.292.1629977645330; \n\tThu, 26 Aug 2021 04:34:05 -0700 (PDT)","MIME-Version":"1.0","References":"<20210826112539.170694-1-hiroh@chromium.org>\n\t<20210826112539.170694-8-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 26 Aug 2021 20:33:53 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 7/9] V4L2VideDevice::createBuffer()\n\tcreates the same number of FrameBuffer::Planes as V4L2 format\n\tplanes.\n\tTherefore, if the v4l2 format single is single-planar format,\n\tthe created number of FrameBuffer::Planes is 1. It should rather\n\tcreate the\n\tsame number of FrameBuffer::Planes as the color format planes.","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\" "}},{"id":19095,"web_url":"https://patchwork.libcamera.org/comment/19095/","msgid":"","date":"2021-08-26T13:03:01","subject":"Re: [libcamera-devel] [PATCH v3 7/9] V4L2VideDevice::createBuffer()\n\tcreates the same number of FrameBuffer::Planes as V4L2 format\n\tplanes.\n\tTherefore, if the v4l2 format single is single-planar format,\n\tthe created number of FrameBuffer::Planes is 1. It should rather\n\tcreate the\n\tsame number of FrameBuffer::Planes as the color format planes.","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 Thu, Aug 26, 2021 at 08:25:37PM +0900, Hirokazu Honda wrote:\n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/internal/v4l2_videodevice.h | 10 +-\n> src/libcamera/v4l2_videodevice.cpp | 141 ++++++++++++++----\n> test/v4l2_videodevice/buffer_cache.cpp | 6 +-\n> 3 files changed, 123 insertions(+), 34 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index e767ec84..bfda6726 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> @@ -242,6 +245,7 @@ private:\n> \tFrameBuffer *dequeueBuffer();\n> \n> \tV4L2Capability caps_;\n> +\tV4L2DeviceFormat format_;\n> \n> \tenum v4l2_buf_type bufferType_;\n> \tenum v4l2_memory memoryType_;\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 2ff25af2..f5f8741a 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> @@ -157,13 +158,15 @@ LOG_DECLARE_CATEGORY(V4L2)\n> /**\n> * \\brief Create an empty cache with \\a numEntries entries\n> * \\param[in] numEntries Number of entries to reserve in the cache\n> + * \\param[in] numPlanes Number of V4L2 buffer planes\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> @@ -171,17 +174,20 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)\n> /**\n> * \\brief Create a pre-populated cache\n> * \\param[in] buffers Array of buffers to pre-populated with\n> + * \\param[in] numPlanes Number of V4L2 buffer planes\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 +243,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 +264,53 @@ 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> }\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\t/*\n> +\t\t * planes_ is V4L2 single-planar format buffer. fds of\n> +\t\t * FrameBuffer::Plane must be identical and the sum of plane\n> +\t\t * size is the V4L2 buffer length.\n> +\t\t */\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\t/* planes_ is V4L2 multi-planar format buffer. */\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\nIs all of this actually needed ? What's the issue if we store, in the\ncache entries, the colour planes from FrameBuffer instance of the V4L2\nbuffer planes ?\n\n> }\n> \n> @@ -579,6 +615,12 @@ int V4L2VideoDevice::open()\n> \t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> \t\t<< caps_.driver() << \": \" << caps_.card();\n> \n> +\tret = getFormat(&format_);\n> +\tif (ret) {\n> +\t\tLOG(V4L2, Error) << \"Failed to get format\";\n> +\t\treturn ret;\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> @@ -668,6 +710,12 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> \t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> \t\t<< caps_.driver() << \": \" << caps_.card();\n> \n> +\tret = getFormat(&format_);\n> +\tif (ret) {\n> +\t\tLOG(V4L2, Error) << \"Failed to get format\";\n> +\t\treturn ret;\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> @@ -761,12 +809,19 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n> */\n> int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)\n> {\n> +\tint ret = 0;\n> \tif (caps_.isMeta())\n> -\t\treturn trySetFormatMeta(format, true);\n> +\t\tret = trySetFormatMeta(format, true);\n> \telse if (caps_.isMultiplanar())\n> -\t\treturn trySetFormatMultiplane(format, true);\n> +\t\tret = trySetFormatMultiplane(format, true);\n> \telse\n> -\t\treturn trySetFormatSingleplane(format, true);\n> +\t\tret = trySetFormatSingleplane(format, true);\n> +\n> +\t/* Cache the set format on success. */\n> +\tif (ret == 0)\n> +\t\tformat_ = *format;\n> +\n> +\treturn ret;\n\nHow about\n\n\tif (ret)\n\t\treturn ret;\n\n\tformat_ = *format;\n\treturn 0;\n\nWe usually return early on error, and handle the normal case in the top\nscope of the function.\n\n> }\n> \n> int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)\n> @@ -1152,8 +1207,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> @@ -1171,7 +1231,7 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,\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 +1250,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> @@ -1289,12 +1354,32 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\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 ?\n> -\t\t\tbuf.m.planes[nplane].length : buf.length;\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> +\tconst auto &info = PixelFormatInfo::info(format_.fourcc);\n> +\tif (info.isValid() && info.numPlanes() != numPlanes) {\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> +\t\t\t/* \\todo Take the V4L2 stride into account */\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> @@ -1352,7 +1437,7 @@ int V4L2VideoDevice::importBuffers(unsigned int count)\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;\n\nI'm afraid the test now fails :-S Could you have a look at that ?","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 D09B7BD87C\n\tfor ;\n\tThu, 26 Aug 2021 13:03:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3AE7568922;\n\tThu, 26 Aug 2021 15:03:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 417D668915\n\tfor ;\n\tThu, 26 Aug 2021 15:03:14 +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 A3862191F;\n\tThu, 26 Aug 2021 15:03:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IoJcimYz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629982993;\n\tbh=nHRCRTDypjp0ZuUXsNWmd6+2LgcQ/dEP/lW502jg+PA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IoJcimYzWwSnsy6a71j1xKNtfNsmpF17zOjJMNTx/TAWFoDJ3nZ5PHyp/lALeqlJP\n\tqux8w/DaIhGgS8YmzSj5TbZ8NN554Eyz7wlbhfoqW+Lh7wR0LPv1em1a8Kb2R6YW1C\n\t77KuCcmWHitUb09e1hQ/vmkklze5pmLl3vv2UQDw=","Date":"Thu, 26 Aug 2021 16:03:01 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210826112539.170694-1-hiroh@chromium.org>\n\t<20210826112539.170694-8-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210826112539.170694-8-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3 7/9] V4L2VideDevice::createBuffer()\n\tcreates the same number of FrameBuffer::Planes as V4L2 format\n\tplanes.\n\tTherefore, if the v4l2 format single is single-planar format,\n\tthe created number of FrameBuffer::Planes is 1. It should rather\n\tcreate the\n\tsame number of FrameBuffer::Planes as the color format planes.","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":19121,"web_url":"https://patchwork.libcamera.org/comment/19121/","msgid":"","date":"2021-08-27T06:42:18","subject":"Re: [libcamera-devel] [PATCH v3 7/9] V4L2VideDevice::createBuffer()\n\tcreates the same number of FrameBuffer::Planes as V4L2 format\n\tplanes.\n\tTherefore, if the v4l2 format single is single-planar format,\n\tthe created number of FrameBuffer::Planes is 1. It should rather\n\tcreate the\n\tsame number of FrameBuffer::Planes as the color format planes.","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\n\nOn Thu, Aug 26, 2021 at 10:03 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Thu, Aug 26, 2021 at 08:25:37PM +0900, Hirokazu Honda wrote:\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/internal/v4l2_videodevice.h | 10 +-\n> > src/libcamera/v4l2_videodevice.cpp | 141 ++++++++++++++----\n> > test/v4l2_videodevice/buffer_cache.cpp | 6 +-\n> > 3 files changed, 123 insertions(+), 34 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > index e767ec84..bfda6726 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> > ~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> > @@ -242,6 +245,7 @@ private:\n> > FrameBuffer *dequeueBuffer();\n> >\n> > V4L2Capability caps_;\n> > + V4L2DeviceFormat format_;\n> >\n> > enum v4l2_buf_type bufferType_;\n> > enum v4l2_memory memoryType_;\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 2ff25af2..f5f8741a 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> > @@ -157,13 +158,15 @@ LOG_DECLARE_CATEGORY(V4L2)\n> > /**\n> > * \\brief Create an empty cache with \\a numEntries entries\n> > * \\param[in] numEntries Number of entries to reserve in the cache\n> > + * \\param[in] numPlanes Number of V4L2 buffer planes\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> > @@ -171,17 +174,20 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)\n> > /**\n> > * \\brief Create a pre-populated cache\n> > * \\param[in] buffers Array of buffers to pre-populated with\n> > + * \\param[in] numPlanes Number of V4L2 buffer planes\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 +243,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 +264,53 @@ 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> > 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> > + /*\n> > + * planes_ is V4L2 single-planar format buffer. fds of\n> > + * FrameBuffer::Plane must be identical and the sum of plane\n> > + * size is the V4L2 buffer length.\n> > + */\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> > + /* planes_ is V4L2 multi-planar format buffer. */\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> Is all of this actually needed ? What's the issue if we store, in the\n> cache entries, the colour planes from FrameBuffer instance of the V4L2\n> buffer planes ?\n>\n\nHmm, you're right. As far as we compare the V4L2 buffer planes, the\nconversion is unnecessary.\nI reverted this part of the change.\n\n> > }\n> >\n> > @@ -579,6 +615,12 @@ int V4L2VideoDevice::open()\n> > << \"Opened device \" << caps_.bus_info() << \": \"\n> > << caps_.driver() << \": \" << caps_.card();\n> >\n> > + ret = getFormat(&format_);\n> > + if (ret) {\n> > + LOG(V4L2, Error) << \"Failed to get format\";\n> > + return ret;\n> > + }\n> > +\n> > return 0;\n> > }\n> >\n> > @@ -668,6 +710,12 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> > << \"Opened device \" << caps_.bus_info() << \": \"\n> > << caps_.driver() << \": \" << caps_.card();\n> >\n> > + ret = getFormat(&format_);\n> > + if (ret) {\n> > + LOG(V4L2, Error) << \"Failed to get format\";\n> > + return ret;\n> > + }\n> > +\n> > return 0;\n> > }\n> >\n> > @@ -761,12 +809,19 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)\n> > */\n> > int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)\n> > {\n> > + int ret = 0;\n> > if (caps_.isMeta())\n> > - return trySetFormatMeta(format, true);\n> > + ret = trySetFormatMeta(format, true);\n> > else if (caps_.isMultiplanar())\n> > - return trySetFormatMultiplane(format, true);\n> > + ret = trySetFormatMultiplane(format, true);\n> > else\n> > - return trySetFormatSingleplane(format, true);\n> > + ret = trySetFormatSingleplane(format, true);\n> > +\n> > + /* Cache the set format on success. */\n> > + if (ret == 0)\n> > + format_ = *format;\n> > +\n> > + return ret;\n>\n> How about\n>\n> if (ret)\n> return ret;\n>\n> format_ = *format;\n> return 0;\n>\n> We usually return early on error, and handle the normal case in the top\n> scope of the function.\n>\n> > }\n> >\n> > int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)\n> > @@ -1152,8 +1207,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> > @@ -1171,7 +1231,7 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,\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 +1250,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> > @@ -1289,12 +1354,32 @@ std::unique_ptr V4L2VideoDevice::createBuffer(unsigned int index)\n> > * \\todo Set the right offset once V4L2 API provides a way.\n> > */\n> > plane.offset = 0;\n> > - plane.length = multiPlanar ?\n> > - buf.m.planes[nplane].length : buf.length;\n> > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;\n> >\n> > planes.push_back(std::move(plane));\n> > }\n> >\n> > + const auto &info = PixelFormatInfo::info(format_.fourcc);\n> > + if (info.isValid() && info.numPlanes() != numPlanes) {\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> > + /* \\todo Take the V4L2 stride into account */\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> > @@ -1352,7 +1437,7 @@ int V4L2VideoDevice::importBuffers(unsigned int count)\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> I'm afraid the test now fails :-S Could you have a look at that ?\n>\n\nSorry, I don't have a vivid environment. I hope the issue is gone by\nreverting V4L2BufferCache change.\n\n-Hiro\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 4FB25BD87D\n\tfor ;\n\tFri, 27 Aug 2021 06:42:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BD1FD61534;\n\tFri, 27 Aug 2021 08:42:32 +0200 (CEST)","from mail-ej1-x633.google.com (mail-ej1-x633.google.com\n\t[IPv6:2a00:1450:4864:20::633])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A22EF60288\n\tfor ;\n\tFri, 27 Aug 2021 08:42:30 +0200 (CEST)","by mail-ej1-x633.google.com with SMTP id mf2so11618000ejb.9\n\tfor ;\n\tThu, 26 Aug 2021 23:42:30 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"WQ7ObvYJ\"; 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=ccNof05VGqOFLhaYCgFVsYpg6u1ShE6cr+NTyF/s+3c=;\n\tb=WQ7ObvYJs8qPdSziFNSS4flreXR1Ylw4Y01VPlkPYXFBXCPLau8AwCoGECcmtt8LfY\n\tfRDMoon8qrh14vVgNaOsLoDWzHXPqB0lB6TBfhwZLi6Gh4s/vKjlR2LOqjR/FCp+J+CD\n\tuq90JT1YddJbJWqw5cHV8IoQNY8vTCk10EyzY=","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=ccNof05VGqOFLhaYCgFVsYpg6u1ShE6cr+NTyF/s+3c=;\n\tb=c9+sccGD1Ti0SRRyBY3Ppf72YmWn32oc30q5E3Em7cR/Y61SEcls2+SnqnlM9cHq42\n\tZJEkZT23C/i+9PSdeMmG8HMlqdOT1NvMOE2PQqo6MDeh6gFcBN8RkDcxi9j3HZG6ld0H\n\tKEhqKR/h+eODWV/vjyHxAKnoT7dUygN5+MogS3xXISfE2KidebTDtaefShsqDBQSJZrB\n\tHyZdkS/AluASoQeZFsTjQwF7vZ/dfDHzgoKWhEmXjyJEr1WjGUV0muGFSqnlVqlcwz4Z\n\t2CqPEcH+gi/xMb/4XFAhhGxNM9Zg8dnbBt9lpa9Axd0f9aQpPayN2SRUUVO5tpYhD3yg\n\tyBuQ==","X-Gm-Message-State":"AOAM5314LcOdMmnvN/XQQLmTHOOLww6NB/5ezs2S6nXe+Mcqn6LZm2Zp\n\twbLixOsYDNHpCxXb/B2tJayeSVPXalwloCVJlCwITiPSbXM=","X-Google-Smtp-Source":"ABdhPJzuNNGWGN+v26wp8sLWQRzZLMBAAx/WuOMVy5C7T8kfo+ciQbmhfwUVN0TajXIEqi28KvSanzNVL10m9gotQSk=","X-Received":"by 2002:a17:907:969f:: with SMTP id\n\thd31mr8261442ejc.475.1630046549921; \n\tThu, 26 Aug 2021 23:42:29 -0700 (PDT)","MIME-Version":"1.0","References":"<20210826112539.170694-1-hiroh@chromium.org>\n\t<20210826112539.170694-8-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 27 Aug 2021 15:42:18 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 7/9] V4L2VideDevice::createBuffer()\n\tcreates the same number of FrameBuffer::Planes as V4L2 format\n\tplanes.\n\tTherefore, if the v4l2 format single is single-planar format,\n\tthe created number of FrameBuffer::Planes is 1. It should rather\n\tcreate the\n\tsame number of FrameBuffer::Planes as the color format planes.","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\" "}}]