[{"id":38862,"web_url":"https://patchwork.libcamera.org/comment/38862/","msgid":"<1abfc895-844f-404b-9d0a-aece9dac47a1@ideasonboard.com>","date":"2026-05-12T08:27:22","subject":"Re: [PATCH v3 2/2] ipa: libipa: Introduce V4L2Stats","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2026. 05. 08. 18:48 keltezéssel, Jacopo Mondi írta:\n> Add a V4L2Stats class similar in spirit to the existing V4L2Params\n> class to allow IPA modules to easily sub-class it to access ISP\n> statistics blocks serialized into a v4l2_isp_buffer.\n> \n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Kieran Bingham \n> ---\n> src/ipa/libipa/meson.build | 2 +\n> src/ipa/libipa/v4l2_stats.cpp | 256 ++++++++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/v4l2_stats.h | 67 +++++++++++\n> 3 files changed, 325 insertions(+)\n> \n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 963c5ee73063..16f4b095f220 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -19,6 +19,7 @@ libipa_headers = files([\n> 'pwl.h',\n> 'quantized.h',\n> 'v4l2_params.h',\n> + 'v4l2_stats.h',\n> ])\n> \n> libipa_sources = files([\n> @@ -40,6 +41,7 @@ libipa_sources = files([\n> 'pwl.cpp',\n> 'quantized.cpp',\n> 'v4l2_params.cpp',\n> + 'v4l2_stats.cpp',\n> ])\n> \n> libipa_includes = include_directories('..')\n> diff --git a/src/ipa/libipa/v4l2_stats.cpp b/src/ipa/libipa/v4l2_stats.cpp\n> new file mode 100644\n> index 000000000000..0f807b2431a4\n> --- /dev/null\n> +++ b/src/ipa/libipa/v4l2_stats.cpp\n> @@ -0,0 +1,256 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2026, Ideas On Board\n> + *\n> + * V4L2 ISP Statistics\n> + */\n> +\n> +#include \"v4l2_stats.h\"\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +LOG_DEFINE_CATEGORY(V4L2Stats)\n> +\n> +/**\n> + * \\file v4l2_stats.cpp\n> + * \\brief Helper class to handle an ISP statistics buffer compatible with\n> + * the generic V4L2 ISP format\n> + *\n> + * The Linux kernel defines a generic buffer format for ISP statistics.\n> + * The format describes a serialisation method that allows userspace to\n> + * access statistics data from a binary buffer.\n> + *\n> + * The V4L2Stats class implements support for the V4L2 ISP statistics buffer\n> + * format and allows users to retrieve an ISP statistics block.\n> + *\n> + * IPA implementations using these helpers should define an enumeration of ISP\n> + * blocks supported by the IPA module and use a set of common abstractions to\n> + * help their derived implementation of V4L2Stats translate the enumerated ISP\n> + * block identifiers to the actual type of the statistics data as defined by\n> + * the kernel interface.\n> + */\n> +\n> +/**\n> + * \\class V4L2StatsBase\n> + * \\brief Base class for V4L2Stats\n> + *\n> + * The V4L2StatsBase is an integral part of V4L2Stats. It serves as a\n> + * container for all code that does not depend on the V4L2Stats template\n> + * arguments, to avoid duplicate copies of inline code.\n> + */\n> +\n> +/**\n> + * \\brief Construct an instance of V4L2StatsBase\n> + * \\param[in] data Reference to the v4l2-buffer memory mapped area\n> + * \\param[in] version The ISP parameters version the implementation supports\n> + *\n> + * Parse the statistics buffer and construct a cache that maps the block type to\n> + * the memory location of the statistics block in the buffer.\n> + *\n> + * After construction users of this class shall check the validity of the\n> + * constructed instance using operator bool().\n> + */\n> +V4L2StatsBase::V4L2StatsBase(Span data, unsigned int version)\n> +\t: data_(data), valid_(false)\n> +{\n> +\tconst struct v4l2_isp_buffer *stats =\n> +\t\treinterpret_cast(data_.data());\n\nI'd add\n\n if (data_.size() < sizeof(*stats))\n // error\n\n\n> +\n> +\tif (data_.size() - sizeof(*stats) < stats->data_size) {\n> +\t\tLOG(V4L2Stats, Error)\n> +\t\t\t<< \"Stats buffer size mismatch: \" << stats->data_size;\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (version != stats->version) {\n> +\t\tLOG(V4L2Stats, Error)\n> +\t\t\t<< \"Unsupported v4l2-isp version: \" << stats->version;\n> +\t\treturn;\n> +\t}\n> +\n> +\t/* Construct the cache for easier lookup. */\n> +\tsize_t left = stats->data_size;\n> +\tconst __u8 *d = stats->data;\n> +\n> +\twhile (left > 0) {\n> +\t\tconst struct v4l2_isp_block_header *header =\n> +\t\t\treinterpret_cast(d);\n\nI would also add\n\n if (left < sizeof(*header) || header->size < sizeof(*header))\n // error\n\n\n> +\n> +\t\tif (header->size > stats->data + stats->data_size - d) {\n\nI'd change this to be\n\n if (left < header->size)\n\nas that seems to me simpler to understand.\n\n\n> +\t\t\tLOG(V4L2Stats, Error)\n> +\t\t\t\t<< \"Block type \" << header->type\n> +\t\t\t\t<< \" size is not valid\";\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\tcache_[header->type] = Span(d, header->size);\n\nand here\n\n auto [it, inserted] = cache_.try_emplace(header->type, d, header->size);\n if (!inserted)\n // error: duplicate\n\n\n> +\t\td += header->size;\n> +\t\tleft -= header->size;\n> +\t}\n> +\n> +\tvalid_ = true;\n> +}\n> +\n> +/**\n> + * \\brief Retrieve an ISP statistics block a returns a reference to it\n> + * \\param[in] blockType The kernel-defined ISP block identifier, used to\n> + * identify the block header\n> + * \\param[in] blockSize The ISP statistics block size, for validation\n> + *\n> + * Retrieve a span to the statistics block memory location by accessing the\n> + * cache built at class construction time.\n> + *\n> + * \\return The memory location of the ISP statistics block, or an empty Span\n> + * if \\a blockType is not supported\n> + */\n> +Span V4L2StatsBase::block(unsigned int blockType, size_t blockSize) const\n> +{\n> +\tconst auto it = cache_.find(blockType);\n> +\tif (it == cache_.end()) {\n> +\t\tLOG(V4L2Stats, Error) << \"Unsupported stats block type: \"\n> +\t\t\t\t << blockType;\n> +\t\treturn {};\n> +\t}\n> +\n> +\tconst struct v4l2_isp_block_header *header =\n> +\t\treinterpret_cast(it->second.data());\n> +\tif (header->size != blockSize) {\n> +\t\tLOG(V4L2Stats, Error)\n> +\t\t\t<< \"Block type \" << blockType\n> +\t\t\t<< \" size mistmatch: expected \"\n> +\t\t\t<< blockSize << \" got:\"\n> +\t\t\t<< header->size;\n> +\t\treturn {};\n> +\t}\n> +\n> +\treturn it->second;\n> +}\n> +\n> +/**\n> + * \\fn V4L2StatsBase::operator bool()\n> + * \\brief Retrieve if a V4L2StatsBase has been successfully constructed\n> + * \\return True if the instance has been constructed successfully, false\n> + * otherwise\n> + */\n> +\n> +/**\n> + * \\var V4L2StatsBase::cache_\n> + * \\brief Map the statistics block types to the memory area where stats are\n> + * located\n> + */\n> +\n> +/**\n> + * \\var V4L2StatsBase::data_\n> + * \\brief The ISP statistics buffer memory\n> + */\n> +\n> +/**\n> + * \\var V4L2StatsBase::valid_\n> + * \\brief Flag to signal valid construction of a V4L2StatsBase instance\n> + */\n> +\n> +/**\n> + * \\class V4L2Stats\n> + * \\brief Helper class that represent an ISP statistics buffer\n> + *\n> + * This class represents an ISP statistics buffer. It is constructed with a\n> + * reference to the memory mapped buffer that has been dequeued from the ISP\n> + * driver.\n> + *\n> + * This class is templated with the type of the enumeration of ISP blocks that\n> + * each IPA module is expected to support. IPA modules are expected to derive\n> + * this class by providing a 'stats_traits' type that helps the class associate\n> + * a block type with the actual memory area that represents the ISP statistics\n> + * block.\n> + *\n> + * \\code{.cpp}\n> + *\n> + * // Define the supported ISP statistics blocks\n> + * enum class myISPStats {\n> + *\tAgc,\n> + *\tAwb,\n> + *\t...\n> + * };\n> + *\n> + * // Maps the C++ enum type to the kernel enum type and concrete parameter type\n> + * template\n> + * struct block_type {\n> + * };\n> + *\n> + * template<>\n> + * struct block_type {\n> + *\tusing type = struct my_isp_kernel_stats_type_agc;\n> + *\tstatic constexpr kernel_enum_type blockType = MY_ISP_STATS_TYPE_AGC;\n> + * };\n> + *\n> + * template<>\n> + * struct block_type {\n> + *\tusing type = struct my_isp_kernel_stats_type_awb;\n> + *\tstatic constexpr kernel_enum_type blockType = MY_ISP_STATS_TYPE_AWB;\n> + * };\n> + *\n> + *\n> + * // Convenience type to associate a block id to the 'block_type' overload\n> + * struct stats_traits {\n> + * \tusing id_type = myISPStats;\n> + * \ttemplate using id_to_details = block_type;\n> + * };\n> + *\n> + * ...\n> + *\n> + * // Derive the V4L2Stats class by providing stats_traits; return an\n> + * // std::optional to make it easy to check if the class has been constructed\n> + * // correctly\n> + * class MyISPStats : public V4L2Stats\n> + * {\n> + * public:\n> + *\tstatic std::optional from(Span data)\n> + *\t{\n> + *\t\tMyISPStats s(data, V4L2_ISP_VERSION_V..);\n> + *\n> + *\t\treturn s ? std::make_optional(s) : std::nullopt;\n> + *\t}\n> + *\n> + * private:\n> + * \tMyISPStats::MyISPStats(Span data, unsigned int version)\n> + * \t\t: V4L2Stats(data, version)\n> + * \t{\n> + * \t}\n> + * };\n> + *\n> + * \\endcode\n> + *\n> + * Users of this class can then easily access an ISP statistics block using the\n> + * block() function.\n> + *\n> + * \\code{.cpp}\n> + *\n> + * MyISPStats stats(data);\n> + *\n> + * auto awb = stats.block();\n> + * auto mean_r = awb->mean_r;\n> + * auto mean_g = awb->mean_g;\n> + * auto mean_b = awb->mean_b;\n> + * \\endcode\n> + */\n> +\n> +/**\n> + * \\fn V4L2Stats::V4L2Stats()\n> + * \\brief Construct an instance of V4L2Stats\n> + * \\param[in] data Reference to the v4l2-buffer memory mapped area\n> + * \\param[in] version The expected V4L2 ISP serialization format version\n> + */\n> +\n> +/**\n> + * \\fn V4L2Stats::block() const\n> + * \\brief Retrieve a pointer to an ISP statistics block\n> + * \\return A pointer to the ISP statistics block\n> + */\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/v4l2_stats.h b/src/ipa/libipa/v4l2_stats.h\n> new file mode 100644\n> index 000000000000..88e23f363f36\n> --- /dev/null\n> +++ b/src/ipa/libipa/v4l2_stats.h\n> @@ -0,0 +1,67 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2026, Ideas On Board\n> + *\n> + * V4L2 ISP Statistics\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class V4L2StatsBase\n> +{\n> +protected:\n> +\tV4L2StatsBase(Span data, unsigned int version);\n> +\n> +\tSpan block(unsigned int blockType, size_t blockSize) const;\n> +\tconstexpr explicit operator bool()\n> +\t{\n> +\t\treturn valid_;\n> +\t}\n> +\n> +\tstd::map> cache_;\n> +\tSpan data_;\n> +\tbool valid_;\n> +};\n> +\n> +template\n> +class V4L2Stats : public V4L2StatsBase\n> +{\n> +public:\n> +\tstatic_assert(std::is_same_v, uint16_t>);\n> +\n> +\ttemplate\n> +\tconst typename Traits::template id_to_details::type *\n> +\tblock() const\n> +\t{\n> +\t\tusing Details = typename Traits::template id_to_details;\n> +\n> +\t\tusing Type = typename Details::type;\n> +\t\tconstexpr auto kernelId = Details::blockType;\n> +\n> +\t\tauto data = V4L2StatsBase::block(kernelId, sizeof(Type));\n> +\n> +\t\treturn data.size() > 0 ?\n> +\t\t reinterpret_cast(data.data()) : nullptr;\n> +\t}\n> +\n> +protected:\n\nDo we want to keep this protected? This would prevent doing something like\n\n using MyISPStats = V4L2Stats;\n\nwhich I imagine would work in many cases (or no?)? I see the std::optional example\nin the documentation, but since there is a `valid_` member, maybe that is not that\nneeded.\n\n\n> +\tV4L2Stats(Span data, unsigned int version)\n> +\t\t: V4L2StatsBase(data, version)\n> +\t{\n> +\t}\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n>","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 B5FDABDB1C\n\tfor ;\n\tTue, 12 May 2026 08:27:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BABEC6301E;\n\tTue, 12 May 2026 10:27:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD7B762FE1\n\tfor ;\n\tTue, 12 May 2026 10:27:26 +0200 (CEST)","from [192.168.33.39] (185.182.215.166.nat.pool.zt.hu\n\t[185.182.215.166])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EB60AC59;\n\tTue, 12 May 2026 10:27:18 +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=\"Nkr7tpHB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1778574439;\n\tbh=vTEMZUqweX4ZqtXA5nyCOLCtfNuS9q+CgL5IS4pcJWs=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Nkr7tpHBqY/fFgX0TmeT6S0o7YNH0un+6irTd4kBwHfFzO39BNpn2aERBR4NaaQfb\n\tS62TMZBkfWzW4Avw8EbN/H1N9V1/TOjvWH6Dw8ZXvLxy687XSSG85vJMGR1iCaJ1mK\n\t/9IL3W/0E4/QAUovQlwm7VKoerUiOP0Plig2QhNQ=","Message-ID":"<1abfc895-844f-404b-9d0a-aece9dac47a1@ideasonboard.com>","Date":"Tue, 12 May 2026 10:27:22 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 2/2] ipa: libipa: Introduce V4L2Stats","To":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org,\n\tAntoine Bouyer ","Cc":"Kieran Bingham ","References":"<20260508-extensible-stats-v3-0-f2174ab4e124@ideasonboard.com>\n\t<20260508-extensible-stats-v3-2-f2174ab4e124@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20260508-extensible-stats-v3-2-f2174ab4e124@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":38869,"web_url":"https://patchwork.libcamera.org/comment/38869/","msgid":"","date":"2026-05-12T12:01:34","subject":"Re: [PATCH v3 2/2] ipa: libipa: Introduce V4L2Stats","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Tue, May 12, 2026 at 10:27:22AM +0200, Barnabás Pőcze wrote:\n> Hi\n>\n> 2026. 05. 08. 18:48 keltezéssel, Jacopo Mondi írta:\n> > Add a V4L2Stats class similar in spirit to the existing V4L2Params\n> > class to allow IPA modules to easily sub-class it to access ISP\n> > statistics blocks serialized into a v4l2_isp_buffer.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > Reviewed-by: Kieran Bingham \n> > ---\n> > src/ipa/libipa/meson.build | 2 +\n> > src/ipa/libipa/v4l2_stats.cpp | 256 ++++++++++++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/v4l2_stats.h | 67 +++++++++++\n> > 3 files changed, 325 insertions(+)\n> >\n> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > index 963c5ee73063..16f4b095f220 100644\n> > --- a/src/ipa/libipa/meson.build\n> > +++ b/src/ipa/libipa/meson.build\n> > @@ -19,6 +19,7 @@ libipa_headers = files([\n> > 'pwl.h',\n> > 'quantized.h',\n> > 'v4l2_params.h',\n> > + 'v4l2_stats.h',\n> > ])\n> > libipa_sources = files([\n> > @@ -40,6 +41,7 @@ libipa_sources = files([\n> > 'pwl.cpp',\n> > 'quantized.cpp',\n> > 'v4l2_params.cpp',\n> > + 'v4l2_stats.cpp',\n> > ])\n> > libipa_includes = include_directories('..')\n> > diff --git a/src/ipa/libipa/v4l2_stats.cpp b/src/ipa/libipa/v4l2_stats.cpp\n> > new file mode 100644\n> > index 000000000000..0f807b2431a4\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/v4l2_stats.cpp\n> > @@ -0,0 +1,256 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2026, Ideas On Board\n> > + *\n> > + * V4L2 ISP Statistics\n> > + */\n> > +\n> > +#include \"v4l2_stats.h\"\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa {\n> > +\n> > +LOG_DEFINE_CATEGORY(V4L2Stats)\n> > +\n> > +/**\n> > + * \\file v4l2_stats.cpp\n> > + * \\brief Helper class to handle an ISP statistics buffer compatible with\n> > + * the generic V4L2 ISP format\n> > + *\n> > + * The Linux kernel defines a generic buffer format for ISP statistics.\n> > + * The format describes a serialisation method that allows userspace to\n> > + * access statistics data from a binary buffer.\n> > + *\n> > + * The V4L2Stats class implements support for the V4L2 ISP statistics buffer\n> > + * format and allows users to retrieve an ISP statistics block.\n> > + *\n> > + * IPA implementations using these helpers should define an enumeration of ISP\n> > + * blocks supported by the IPA module and use a set of common abstractions to\n> > + * help their derived implementation of V4L2Stats translate the enumerated ISP\n> > + * block identifiers to the actual type of the statistics data as defined by\n> > + * the kernel interface.\n> > + */\n> > +\n> > +/**\n> > + * \\class V4L2StatsBase\n> > + * \\brief Base class for V4L2Stats\n> > + *\n> > + * The V4L2StatsBase is an integral part of V4L2Stats. It serves as a\n> > + * container for all code that does not depend on the V4L2Stats template\n> > + * arguments, to avoid duplicate copies of inline code.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct an instance of V4L2StatsBase\n> > + * \\param[in] data Reference to the v4l2-buffer memory mapped area\n> > + * \\param[in] version The ISP parameters version the implementation supports\n> > + *\n> > + * Parse the statistics buffer and construct a cache that maps the block type to\n> > + * the memory location of the statistics block in the buffer.\n> > + *\n> > + * After construction users of this class shall check the validity of the\n> > + * constructed instance using operator bool().\n> > + */\n> > +V4L2StatsBase::V4L2StatsBase(Span data, unsigned int version)\n> > +\t: data_(data), valid_(false)\n> > +{\n> > +\tconst struct v4l2_isp_buffer *stats =\n> > +\t\treinterpret_cast(data_.data());\n>\n> I'd add\n>\n> if (data_.size() < sizeof(*stats))\n> // error\n>\n\nTrue, that's possible\n\n>\n> > +\n> > +\tif (data_.size() - sizeof(*stats) < stats->data_size) {\n> > +\t\tLOG(V4L2Stats, Error)\n> > +\t\t\t<< \"Stats buffer size mismatch: \" << stats->data_size;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tif (version != stats->version) {\n> > +\t\tLOG(V4L2Stats, Error)\n> > +\t\t\t<< \"Unsupported v4l2-isp version: \" << stats->version;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\t/* Construct the cache for easier lookup. */\n> > +\tsize_t left = stats->data_size;\n> > +\tconst __u8 *d = stats->data;\n> > +\n> > +\twhile (left > 0) {\n> > +\t\tconst struct v4l2_isp_block_header *header =\n> > +\t\t\treinterpret_cast(d);\n>\n> I would also add\n>\n> if (left < sizeof(*header) || header->size < sizeof(*header))\n> // error\n>\n>\n> > +\n> > +\t\tif (header->size > stats->data + stats->data_size - d) {\n>\n> I'd change this to be\n>\n> if (left < header->size)\n\nI can make a single error condition\n\n if (header->size < sizeof(*header) || left < header->size)\n\nthis should also catch the \"left < sizeof(*header)\" case ?\n\n>\n> as that seems to me simpler to understand.\n>\n>\n> > +\t\t\tLOG(V4L2Stats, Error)\n> > +\t\t\t\t<< \"Block type \" << header->type\n> > +\t\t\t\t<< \" size is not valid\";\n> > +\t\t\treturn;\n> > +\t\t}\n> > +\n> > +\t\tcache_[header->type] = Span(d, header->size);\n>\n> and here\n>\n> auto [it, inserted] = cache_.try_emplace(header->type, d, header->size);\n> if (!inserted)\n> // error: duplicate\n\nack\n\n>\n>\n> > +\t\td += header->size;\n> > +\t\tleft -= header->size;\n> > +\t}\n> > +\n> > +\tvalid_ = true;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve an ISP statistics block a returns a reference to it\n> > + * \\param[in] blockType The kernel-defined ISP block identifier, used to\n> > + * identify the block header\n> > + * \\param[in] blockSize The ISP statistics block size, for validation\n> > + *\n> > + * Retrieve a span to the statistics block memory location by accessing the\n> > + * cache built at class construction time.\n> > + *\n> > + * \\return The memory location of the ISP statistics block, or an empty Span\n> > + * if \\a blockType is not supported\n> > + */\n> > +Span V4L2StatsBase::block(unsigned int blockType, size_t blockSize) const\n> > +{\n> > +\tconst auto it = cache_.find(blockType);\n> > +\tif (it == cache_.end()) {\n> > +\t\tLOG(V4L2Stats, Error) << \"Unsupported stats block type: \"\n> > +\t\t\t\t << blockType;\n> > +\t\treturn {};\n> > +\t}\n> > +\n> > +\tconst struct v4l2_isp_block_header *header =\n> > +\t\treinterpret_cast(it->second.data());\n> > +\tif (header->size != blockSize) {\n> > +\t\tLOG(V4L2Stats, Error)\n> > +\t\t\t<< \"Block type \" << blockType\n> > +\t\t\t<< \" size mistmatch: expected \"\n> > +\t\t\t<< blockSize << \" got:\"\n> > +\t\t\t<< header->size;\n> > +\t\treturn {};\n> > +\t}\n> > +\n> > +\treturn it->second;\n> > +}\n> > +\n> > +/**\n> > + * \\fn V4L2StatsBase::operator bool()\n> > + * \\brief Retrieve if a V4L2StatsBase has been successfully constructed\n> > + * \\return True if the instance has been constructed successfully, false\n> > + * otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\var V4L2StatsBase::cache_\n> > + * \\brief Map the statistics block types to the memory area where stats are\n> > + * located\n> > + */\n> > +\n> > +/**\n> > + * \\var V4L2StatsBase::data_\n> > + * \\brief The ISP statistics buffer memory\n> > + */\n> > +\n> > +/**\n> > + * \\var V4L2StatsBase::valid_\n> > + * \\brief Flag to signal valid construction of a V4L2StatsBase instance\n> > + */\n> > +\n> > +/**\n> > + * \\class V4L2Stats\n> > + * \\brief Helper class that represent an ISP statistics buffer\n> > + *\n> > + * This class represents an ISP statistics buffer. It is constructed with a\n> > + * reference to the memory mapped buffer that has been dequeued from the ISP\n> > + * driver.\n> > + *\n> > + * This class is templated with the type of the enumeration of ISP blocks that\n> > + * each IPA module is expected to support. IPA modules are expected to derive\n> > + * this class by providing a 'stats_traits' type that helps the class associate\n> > + * a block type with the actual memory area that represents the ISP statistics\n> > + * block.\n> > + *\n> > + * \\code{.cpp}\n> > + *\n> > + * // Define the supported ISP statistics blocks\n> > + * enum class myISPStats {\n> > + *\tAgc,\n> > + *\tAwb,\n> > + *\t...\n> > + * };\n> > + *\n> > + * // Maps the C++ enum type to the kernel enum type and concrete parameter type\n> > + * template\n> > + * struct block_type {\n> > + * };\n> > + *\n> > + * template<>\n> > + * struct block_type {\n> > + *\tusing type = struct my_isp_kernel_stats_type_agc;\n> > + *\tstatic constexpr kernel_enum_type blockType = MY_ISP_STATS_TYPE_AGC;\n> > + * };\n> > + *\n> > + * template<>\n> > + * struct block_type {\n> > + *\tusing type = struct my_isp_kernel_stats_type_awb;\n> > + *\tstatic constexpr kernel_enum_type blockType = MY_ISP_STATS_TYPE_AWB;\n> > + * };\n> > + *\n> > + *\n> > + * // Convenience type to associate a block id to the 'block_type' overload\n> > + * struct stats_traits {\n> > + * \tusing id_type = myISPStats;\n> > + * \ttemplate using id_to_details = block_type;\n> > + * };\n> > + *\n> > + * ...\n> > + *\n> > + * // Derive the V4L2Stats class by providing stats_traits; return an\n> > + * // std::optional to make it easy to check if the class has been constructed\n> > + * // correctly\n> > + * class MyISPStats : public V4L2Stats\n> > + * {\n> > + * public:\n> > + *\tstatic std::optional from(Span data)\n> > + *\t{\n> > + *\t\tMyISPStats s(data, V4L2_ISP_VERSION_V..);\n> > + *\n> > + *\t\treturn s ? std::make_optional(s) : std::nullopt;\n> > + *\t}\n> > + *\n> > + * private:\n> > + * \tMyISPStats::MyISPStats(Span data, unsigned int version)\n> > + * \t\t: V4L2Stats(data, version)\n> > + * \t{\n> > + * \t}\n> > + * };\n> > + *\n> > + * \\endcode\n> > + *\n> > + * Users of this class can then easily access an ISP statistics block using the\n> > + * block() function.\n> > + *\n> > + * \\code{.cpp}\n> > + *\n> > + * MyISPStats stats(data);\n> > + *\n> > + * auto awb = stats.block();\n> > + * auto mean_r = awb->mean_r;\n> > + * auto mean_g = awb->mean_g;\n> > + * auto mean_b = awb->mean_b;\n> > + * \\endcode\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2Stats::V4L2Stats()\n> > + * \\brief Construct an instance of V4L2Stats\n> > + * \\param[in] data Reference to the v4l2-buffer memory mapped area\n> > + * \\param[in] version The expected V4L2 ISP serialization format version\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2Stats::block() const\n> > + * \\brief Retrieve a pointer to an ISP statistics block\n> > + * \\return A pointer to the ISP statistics block\n> > + */\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/v4l2_stats.h b/src/ipa/libipa/v4l2_stats.h\n> > new file mode 100644\n> > index 000000000000..88e23f363f36\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/v4l2_stats.h\n> > @@ -0,0 +1,67 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2026, Ideas On Board\n> > + *\n> > + * V4L2 ISP Statistics\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa {\n> > +\n> > +class V4L2StatsBase\n> > +{\n> > +protected:\n> > +\tV4L2StatsBase(Span data, unsigned int version);\n> > +\n> > +\tSpan block(unsigned int blockType, size_t blockSize) const;\n> > +\tconstexpr explicit operator bool()\n> > +\t{\n> > +\t\treturn valid_;\n> > +\t}\n> > +\n> > +\tstd::map> cache_;\n> > +\tSpan data_;\n> > +\tbool valid_;\n> > +};\n> > +\n> > +template\n> > +class V4L2Stats : public V4L2StatsBase\n> > +{\n> > +public:\n> > +\tstatic_assert(std::is_same_v, uint16_t>);\n> > +\n> > +\ttemplate\n> > +\tconst typename Traits::template id_to_details::type *\n> > +\tblock() const\n> > +\t{\n> > +\t\tusing Details = typename Traits::template id_to_details;\n> > +\n> > +\t\tusing Type = typename Details::type;\n> > +\t\tconstexpr auto kernelId = Details::blockType;\n> > +\n> > +\t\tauto data = V4L2StatsBase::block(kernelId, sizeof(Type));\n> > +\n> > +\t\treturn data.size() > 0 ?\n> > +\t\t reinterpret_cast(data.data()) : nullptr;\n> > +\t}\n> > +\n> > +protected:\n>\n> Do we want to keep this protected? This would prevent doing something like\n>\n> using MyISPStats = V4L2Stats;\n\nThat was actually intentional to force users to implement a static\n::from() as suggested by the documentation\n\n>\n> which I imagine would work in many cases (or no?)? I see the std::optional example\n> in the documentation, but since there is a `valid_` member, maybe that is not that\n> needed.\n>\n\nAnd that's where we were at one revision ago.\n\nI'll drop optional<> then ..\n\n>\n> > +\tV4L2Stats(Span data, unsigned int version)\n> > +\t\t: V4L2StatsBase(data, version)\n> > +\t{\n> > +\t}\n> > +};\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> >\n>","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 A5EF9BDCBD\n\tfor ;\n\tTue, 12 May 2026 12:01:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9089A63026;\n\tTue, 12 May 2026 14:01:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 146816271A\n\tfor ;\n\tTue, 12 May 2026 14:01:38 +0200 (CEST)","from ideasonboard.com (net-93-65-100-155.cust.vodafonedsl.it\n\t[93.65.100.155])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2410DC59;\n\tTue, 12 May 2026 14:01:30 +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=\"HsrQWh4L\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1778587290;\n\tbh=sw2GLBxSUXKyGPOKWl4UVopd+rB1VKMAQvJpqZqdUds=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HsrQWh4LEaXCyd6IlLofGPWShs4BUtE5qjBbYQGjqdN8CkZhVnuetBgvWEUdcYMPf\n\tA//RrUgRg1OsXKW1q5wQJ8PlIOI/7DvhOZioGEK/TZZm8JClUAJyWHaykCG8srGqFm\n\tYFoQf5+rH+4604dub5fRLAb+fPLPzCi+gzMjQuL4=","Date":"Tue, 12 May 2026 14:01:34 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tAntoine Bouyer , \n\tKieran Bingham ","Subject":"Re: [PATCH v3 2/2] ipa: libipa: Introduce V4L2Stats","Message-ID":"","References":"<20260508-extensible-stats-v3-0-f2174ab4e124@ideasonboard.com>\n\t<20260508-extensible-stats-v3-2-f2174ab4e124@ideasonboard.com>\n\t<1abfc895-844f-404b-9d0a-aece9dac47a1@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<1abfc895-844f-404b-9d0a-aece9dac47a1@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":38870,"web_url":"https://patchwork.libcamera.org/comment/38870/","msgid":"<49ffe6b3-d0dc-4ac7-b9af-88ee2c02acda@ideasonboard.com>","date":"2026-05-12T12:19:09","subject":"Re: [PATCH v3 2/2] ipa: libipa: Introduce V4L2Stats","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2026. 05. 12. 14:01 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Tue, May 12, 2026 at 10:27:22AM +0200, Barnabás Pőcze wrote:\n>> Hi\n>>\n>> 2026. 05. 08. 18:48 keltezéssel, Jacopo Mondi írta:\n>>> Add a V4L2Stats class similar in spirit to the existing V4L2Params\n>>> class to allow IPA modules to easily sub-class it to access ISP\n>>> statistics blocks serialized into a v4l2_isp_buffer.\n>>>\n>>> Signed-off-by: Jacopo Mondi \n>>> Reviewed-by: Kieran Bingham \n>>> ---\n>>> src/ipa/libipa/meson.build | 2 +\n>>> src/ipa/libipa/v4l2_stats.cpp | 256 ++++++++++++++++++++++++++++++++++++++++++\n>>> src/ipa/libipa/v4l2_stats.h | 67 +++++++++++\n>>> 3 files changed, 325 insertions(+)\n>>>\n>>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>>> index 963c5ee73063..16f4b095f220 100644\n>>> --- a/src/ipa/libipa/meson.build\n>>> +++ b/src/ipa/libipa/meson.build\n>>> @@ -19,6 +19,7 @@ libipa_headers = files([\n>>> 'pwl.h',\n>>> 'quantized.h',\n>>> 'v4l2_params.h',\n>>> + 'v4l2_stats.h',\n>>> ])\n>>> libipa_sources = files([\n>>> @@ -40,6 +41,7 @@ libipa_sources = files([\n>>> 'pwl.cpp',\n>>> 'quantized.cpp',\n>>> 'v4l2_params.cpp',\n>>> + 'v4l2_stats.cpp',\n>>> ])\n>>> libipa_includes = include_directories('..')\n>>> diff --git a/src/ipa/libipa/v4l2_stats.cpp b/src/ipa/libipa/v4l2_stats.cpp\n>>> new file mode 100644\n>>> index 000000000000..0f807b2431a4\n>>> --- /dev/null\n>>> +++ b/src/ipa/libipa/v4l2_stats.cpp\n>>> @@ -0,0 +1,256 @@\n> [...]\n>>> +/**\n>>> + * \\brief Construct an instance of V4L2StatsBase\n>>> + * \\param[in] data Reference to the v4l2-buffer memory mapped area\n>>> + * \\param[in] version The ISP parameters version the implementation supports\n>>> + *\n>>> + * Parse the statistics buffer and construct a cache that maps the block type to\n>>> + * the memory location of the statistics block in the buffer.\n>>> + *\n>>> + * After construction users of this class shall check the validity of the\n>>> + * constructed instance using operator bool().\n>>> + */\n>>> +V4L2StatsBase::V4L2StatsBase(Span data, unsigned int version)\n>>> +\t: data_(data), valid_(false)\n>>> +{\n>>> +\tconst struct v4l2_isp_buffer *stats =\n>>> +\t\treinterpret_cast(data_.data());\n>>\n>> I'd add\n>>\n>> if (data_.size() < sizeof(*stats))\n>> // error\n>>\n> \n> True, that's possible\n> \n>>\n>>> +\n>>> +\tif (data_.size() - sizeof(*stats) < stats->data_size) {\n>>> +\t\tLOG(V4L2Stats, Error)\n>>> +\t\t\t<< \"Stats buffer size mismatch: \" << stats->data_size;\n>>> +\t\treturn;\n>>> +\t}\n>>> +\n>>> +\tif (version != stats->version) {\n>>> +\t\tLOG(V4L2Stats, Error)\n>>> +\t\t\t<< \"Unsupported v4l2-isp version: \" << stats->version;\n>>> +\t\treturn;\n>>> +\t}\n>>> +\n>>> +\t/* Construct the cache for easier lookup. */\n>>> +\tsize_t left = stats->data_size;\n>>> +\tconst __u8 *d = stats->data;\n>>> +\n>>> +\twhile (left > 0) {\n>>> +\t\tconst struct v4l2_isp_block_header *header =\n>>> +\t\t\treinterpret_cast(d);\n>>\n>> I would also add\n>>\n>> if (left < sizeof(*header) || header->size < sizeof(*header))\n>> // error\n>>\n>>\n>>> +\n>>> +\t\tif (header->size > stats->data + stats->data_size - d) {\n>>\n>> I'd change this to be\n>>\n>> if (left < header->size)\n> \n> I can make a single error condition\n> \n> if (header->size < sizeof(*header) || left < header->size)\n> \n> this should also catch the \"left < sizeof(*header)\" case ?\n\nThat might dereference `header` even if `left < sizeof(*header)`,\nwhen there isn't a header in the buffer. And yes, this is very \"paranoid\"\nin some respects as the kernel should probably guarantee these, but\nI think it makes most sense to handle invalid input as much possible.\n\n\n> \n>>\n>> as that seems to me simpler to understand.\n>>\n>>\n>>> +\t\t\tLOG(V4L2Stats, Error)\n>>> +\t\t\t\t<< \"Block type \" << header->type\n>>> +\t\t\t\t<< \" size is not valid\";\n>>> +\t\t\treturn;\n>>> +\t\t}\n>>> +\n>>> +\t\tcache_[header->type] = Span(d, header->size);\n>>\n>> and here\n>>\n>> auto [it, inserted] = cache_.try_emplace(header->type, d, header->size);\n>> if (!inserted)\n>> // error: duplicate\n> \n> ack\n> \n>>\n>>\n>>> +\t\td += header->size;\n>>> +\t\tleft -= header->size;\n>>> +\t}\n>>> +\n>>> +\tvalid_ = true;\n>>> +}\n> [...]\n>>> diff --git a/src/ipa/libipa/v4l2_stats.h b/src/ipa/libipa/v4l2_stats.h\n>>> new file mode 100644\n>>> index 000000000000..88e23f363f36\n>>> --- /dev/null\n>>> +++ b/src/ipa/libipa/v4l2_stats.h\n>>> @@ -0,0 +1,67 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2026, Ideas On Board\n>>> + *\n>>> + * V4L2 ISP Statistics\n>>> + */\n>>> +\n>>> +#pragma once\n>>> +\n>>> +#include \n>>> +#include \n>>> +\n>>> +#include \n>>> +\n>>> +#include \n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +namespace ipa {\n>>> +\n>>> +class V4L2StatsBase\n>>> +{\n>>> +protected:\n>>> +\tV4L2StatsBase(Span data, unsigned int version);\n>>> +\n>>> +\tSpan block(unsigned int blockType, size_t blockSize) const;\n>>> +\tconstexpr explicit operator bool()\n>>> +\t{\n>>> +\t\treturn valid_;\n>>> +\t}\n>>> +\n>>> +\tstd::map> cache_;\n>>> +\tSpan data_;\n>>> +\tbool valid_;\n>>> +};\n>>> +\n>>> +template\n>>> +class V4L2Stats : public V4L2StatsBase\n>>> +{\n>>> +public:\n>>> +\tstatic_assert(std::is_same_v, uint16_t>);\n>>> +\n>>> +\ttemplate\n>>> +\tconst typename Traits::template id_to_details::type *\n>>> +\tblock() const\n>>> +\t{\n>>> +\t\tusing Details = typename Traits::template id_to_details;\n>>> +\n>>> +\t\tusing Type = typename Details::type;\n>>> +\t\tconstexpr auto kernelId = Details::blockType;\n>>> +\n>>> +\t\tauto data = V4L2StatsBase::block(kernelId, sizeof(Type));\n>>> +\n>>> +\t\treturn data.size() > 0 ?\n>>> +\t\t reinterpret_cast(data.data()) : nullptr;\n>>> +\t}\n>>> +\n>>> +protected:\n>>\n>> Do we want to keep this protected? This would prevent doing something like\n>>\n>> using MyISPStats = V4L2Stats;\n> \n> That was actually intentional to force users to implement a static\n> ::from() as suggested by the documentation\n> \n>>\n>> which I imagine would work in many cases (or no?)? I see the std::optional example\n>> in the documentation, but since there is a `valid_` member, maybe that is not that\n>> needed.\n>>\n> \n> And that's where we were at one revision ago.\n> \n> I'll drop optional<> then ..\n\nSorry, I probably wasn't clear how I imagined the std::optional here. Having a `valid_`\nmember and using `std::optional` is mutually exclusive in my view. As for the std::optional,\nI was thinking of having a private constructor for `V4L2Stats` and a public static `from()`\nmethod that returns an std::optional. Now arguably if inheritance is to be used, using an\nstd::optional will make the derived classes a bit more complex. So I think either is ok,\nbut it's probably better to avoid mixing the two.\n\n\n> \n>>\n>>> +\tV4L2Stats(Span data, unsigned int version)\n>>> +\t\t: V4L2StatsBase(data, version)\n>>> +\t{\n>>> +\t}\n>>> +};\n>>> +\n>>> +} /* namespace ipa */\n>>> +\n>>> +} /* namespace libcamera */\n>>>\n>>","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 B3021BDB1C\n\tfor ;\n\tTue, 12 May 2026 12:19:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A550F62FEA;\n\tTue, 12 May 2026 14:19:14 +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 36B106271A\n\tfor ;\n\tTue, 12 May 2026 14:19:13 +0200 (CEST)","from [192.168.33.39] (185.182.215.166.nat.pool.zt.hu\n\t[185.182.215.166])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2ED0FC59;\n\tTue, 12 May 2026 14:19:05 +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=\"T3/fmh57\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1778588345;\n\tbh=xxc725iirTSAQzDzjnjaPsQ0/RSXDVzDu50WcIgmLyk=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=T3/fmh57igV6/1gkxkbOd3+xpwrRERED5mb7zh+4qo1TEpz2oxFdNzQP2sT1Qob/J\n\t+6ms1UnbSqF+ylIW/cKs6qvukkABSvmPqT6yiCD7fcwh52Kce0iNE/sigtHLnRMJhi\n\tEjzvapwU61aor5/qozSmMYUCH3Hkg/6nX1NEfVlI=","Message-ID":"<49ffe6b3-d0dc-4ac7-b9af-88ee2c02acda@ideasonboard.com>","Date":"Tue, 12 May 2026 14:19:09 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 2/2] ipa: libipa: Introduce V4L2Stats","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tAntoine Bouyer ,\n\tKieran Bingham ","References":"<20260508-extensible-stats-v3-0-f2174ab4e124@ideasonboard.com>\n\t<20260508-extensible-stats-v3-2-f2174ab4e124@ideasonboard.com>\n\t<1abfc895-844f-404b-9d0a-aece9dac47a1@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":38871,"web_url":"https://patchwork.libcamera.org/comment/38871/","msgid":"","date":"2026-05-12T12:29:50","subject":"Re: [PATCH v3 2/2] ipa: libipa: Introduce V4L2Stats","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Tue, May 12, 2026 at 02:19:09PM +0200, Barnabás Pőcze wrote:\n> 2026. 05. 12. 14:01 keltezéssel, Jacopo Mondi írta:\n> > Hi Barnabás\n> >\n> > On Tue, May 12, 2026 at 10:27:22AM +0200, Barnabás Pőcze wrote:\n> > > Hi\n> > >\n> > > 2026. 05. 08. 18:48 keltezéssel, Jacopo Mondi írta:\n> > > > Add a V4L2Stats class similar in spirit to the existing V4L2Params\n> > > > class to allow IPA modules to easily sub-class it to access ISP\n> > > > statistics blocks serialized into a v4l2_isp_buffer.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > Reviewed-by: Kieran Bingham \n> > > > ---\n> > > > src/ipa/libipa/meson.build | 2 +\n> > > > src/ipa/libipa/v4l2_stats.cpp | 256 ++++++++++++++++++++++++++++++++++++++++++\n> > > > src/ipa/libipa/v4l2_stats.h | 67 +++++++++++\n> > > > 3 files changed, 325 insertions(+)\n> > > >\n> > > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > > > index 963c5ee73063..16f4b095f220 100644\n> > > > --- a/src/ipa/libipa/meson.build\n> > > > +++ b/src/ipa/libipa/meson.build\n> > > > @@ -19,6 +19,7 @@ libipa_headers = files([\n> > > > 'pwl.h',\n> > > > 'quantized.h',\n> > > > 'v4l2_params.h',\n> > > > + 'v4l2_stats.h',\n> > > > ])\n> > > > libipa_sources = files([\n> > > > @@ -40,6 +41,7 @@ libipa_sources = files([\n> > > > 'pwl.cpp',\n> > > > 'quantized.cpp',\n> > > > 'v4l2_params.cpp',\n> > > > + 'v4l2_stats.cpp',\n> > > > ])\n> > > > libipa_includes = include_directories('..')\n> > > > diff --git a/src/ipa/libipa/v4l2_stats.cpp b/src/ipa/libipa/v4l2_stats.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..0f807b2431a4\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/libipa/v4l2_stats.cpp\n> > > > @@ -0,0 +1,256 @@\n> > [...]\n> > > > +/**\n> > > > + * \\brief Construct an instance of V4L2StatsBase\n> > > > + * \\param[in] data Reference to the v4l2-buffer memory mapped area\n> > > > + * \\param[in] version The ISP parameters version the implementation supports\n> > > > + *\n> > > > + * Parse the statistics buffer and construct a cache that maps the block type to\n> > > > + * the memory location of the statistics block in the buffer.\n> > > > + *\n> > > > + * After construction users of this class shall check the validity of the\n> > > > + * constructed instance using operator bool().\n> > > > + */\n> > > > +V4L2StatsBase::V4L2StatsBase(Span data, unsigned int version)\n> > > > +\t: data_(data), valid_(false)\n> > > > +{\n> > > > +\tconst struct v4l2_isp_buffer *stats =\n> > > > +\t\treinterpret_cast(data_.data());\n> > >\n> > > I'd add\n> > >\n> > > if (data_.size() < sizeof(*stats))\n> > > // error\n> > >\n> >\n> > True, that's possible\n> >\n> > >\n> > > > +\n> > > > +\tif (data_.size() - sizeof(*stats) < stats->data_size) {\n> > > > +\t\tLOG(V4L2Stats, Error)\n> > > > +\t\t\t<< \"Stats buffer size mismatch: \" << stats->data_size;\n> > > > +\t\treturn;\n> > > > +\t}\n> > > > +\n> > > > +\tif (version != stats->version) {\n> > > > +\t\tLOG(V4L2Stats, Error)\n> > > > +\t\t\t<< \"Unsupported v4l2-isp version: \" << stats->version;\n> > > > +\t\treturn;\n> > > > +\t}\n> > > > +\n> > > > +\t/* Construct the cache for easier lookup. */\n> > > > +\tsize_t left = stats->data_size;\n> > > > +\tconst __u8 *d = stats->data;\n> > > > +\n> > > > +\twhile (left > 0) {\n> > > > +\t\tconst struct v4l2_isp_block_header *header =\n> > > > +\t\t\treinterpret_cast(d);\n> > >\n> > > I would also add\n> > >\n> > > if (left < sizeof(*header) || header->size < sizeof(*header))\n> > > // error\n> > >\n> > >\n> > > > +\n> > > > +\t\tif (header->size > stats->data + stats->data_size - d) {\n> > >\n> > > I'd change this to be\n> > >\n> > > if (left < header->size)\n> >\n> > I can make a single error condition\n> >\n> > if (header->size < sizeof(*header) || left < header->size)\n> >\n> > this should also catch the \"left < sizeof(*header)\" case ?\n>\n> That might dereference `header` even if `left < sizeof(*header)`,\n> when there isn't a header in the buffer. And yes, this is very \"paranoid\"\n\nin paranoia stat virtus\n\n> in some respects as the kernel should probably guarantee these, but\n> I think it makes most sense to handle invalid input as much possible.\n\nThis shouldn't happen, but I'm fine being careful\n\n>\n>\n> >\n> > >\n> > > as that seems to me simpler to understand.\n> > >\n> > >\n> > > > +\t\t\tLOG(V4L2Stats, Error)\n> > > > +\t\t\t\t<< \"Block type \" << header->type\n> > > > +\t\t\t\t<< \" size is not valid\";\n> > > > +\t\t\treturn;\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tcache_[header->type] = Span(d, header->size);\n> > >\n> > > and here\n> > >\n> > > auto [it, inserted] = cache_.try_emplace(header->type, d, header->size);\n> > > if (!inserted)\n> > > // error: duplicate\n> >\n> > ack\n> >\n> > >\n> > >\n> > > > +\t\td += header->size;\n> > > > +\t\tleft -= header->size;\n> > > > +\t}\n> > > > +\n> > > > +\tvalid_ = true;\n> > > > +}\n> > [...]\n> > > > diff --git a/src/ipa/libipa/v4l2_stats.h b/src/ipa/libipa/v4l2_stats.h\n> > > > new file mode 100644\n> > > > index 000000000000..88e23f363f36\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/libipa/v4l2_stats.h\n> > > > @@ -0,0 +1,67 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2026, Ideas On Board\n> > > > + *\n> > > > + * V4L2 ISP Statistics\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +namespace ipa {\n> > > > +\n> > > > +class V4L2StatsBase\n> > > > +{\n> > > > +protected:\n> > > > +\tV4L2StatsBase(Span data, unsigned int version);\n> > > > +\n> > > > +\tSpan block(unsigned int blockType, size_t blockSize) const;\n> > > > +\tconstexpr explicit operator bool()\n> > > > +\t{\n> > > > +\t\treturn valid_;\n> > > > +\t}\n> > > > +\n> > > > +\tstd::map> cache_;\n> > > > +\tSpan data_;\n> > > > +\tbool valid_;\n> > > > +};\n> > > > +\n> > > > +template\n> > > > +class V4L2Stats : public V4L2StatsBase\n> > > > +{\n> > > > +public:\n> > > > +\tstatic_assert(std::is_same_v, uint16_t>);\n> > > > +\n> > > > +\ttemplate\n> > > > +\tconst typename Traits::template id_to_details::type *\n> > > > +\tblock() const\n> > > > +\t{\n> > > > +\t\tusing Details = typename Traits::template id_to_details;\n> > > > +\n> > > > +\t\tusing Type = typename Details::type;\n> > > > +\t\tconstexpr auto kernelId = Details::blockType;\n> > > > +\n> > > > +\t\tauto data = V4L2StatsBase::block(kernelId, sizeof(Type));\n> > > > +\n> > > > +\t\treturn data.size() > 0 ?\n> > > > +\t\t reinterpret_cast(data.data()) : nullptr;\n> > > > +\t}\n> > > > +\n> > > > +protected:\n> > >\n> > > Do we want to keep this protected? This would prevent doing something like\n> > >\n> > > using MyISPStats = V4L2Stats;\n> >\n> > That was actually intentional to force users to implement a static\n> > ::from() as suggested by the documentation\n> >\n> > >\n> > > which I imagine would work in many cases (or no?)? I see the std::optional example\n> > > in the documentation, but since there is a `valid_` member, maybe that is not that\n> > > needed.\n> > >\n> >\n> > And that's where we were at one revision ago.\n> >\n> > I'll drop optional<> then ..\n>\n> Sorry, I probably wasn't clear how I imagined the std::optional here. Having a `valid_`\n> member and using `std::optional` is mutually exclusive in my view. As for the std::optional,\n> I was thinking of having a private constructor for `V4L2Stats` and a public static `from()`\n> method that returns an std::optional. Now arguably if inheritance is to be used, using an\n> std::optional will make the derived classes a bit more complex. So I think either is ok,\n> but it's probably better to avoid mixing the two.\n>\n\nProblem is that, when I tried to introduce a static ::from() in the\nV4L2Stats base class, telling the compiler I would like the return\ntype to be an optional quickly escalated into something I\nconsidered not worth pursuing. I might of course have missed how to do\nthat properly.\n\nFor the moment, I think we're fine with operator bool() in the base\nclass.\n\n>\n> >\n> > >\n> > > > +\tV4L2Stats(Span data, unsigned int version)\n> > > > +\t\t: V4L2StatsBase(data, version)\n> > > > +\t{\n> > > > +\t}\n> > > > +};\n> > > > +\n> > > > +} /* namespace ipa */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > >\n> > >\n>","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 4628DBDCBD\n\tfor ;\n\tTue, 12 May 2026 12:29:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 344B563020;\n\tTue, 12 May 2026 14:29:56 +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 32C3C6271A\n\tfor ;\n\tTue, 12 May 2026 14:29:54 +0200 (CEST)","from ideasonboard.com (net-93-65-100-155.cust.vodafonedsl.it\n\t[93.65.100.155])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2D8F4C59;\n\tTue, 12 May 2026 14:29:46 +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=\"atrpHk44\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1778588986;\n\tbh=6QD5vJrrHHVmN5oFJI2zL1Z4pTH2I36tSKYyBemOzqs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=atrpHk44GtyelJSoJrnnXM3jf6dnAAN6gKhPUVBnfa7YXIDiI5gIfaTS7VdkpPFmC\n\tjhwIrNVoO1Mm1DW3HR+XDOaQBXmwlugdyQLqb09m2tDlDS32cBO9aQ1xVWwNn1lXNA\n\t654DlopQK8wgtXLNIneljiYkoGMsAePxIIJwL04E=","Date":"Tue, 12 May 2026 14:29:50 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tAntoine Bouyer , \n\tKieran Bingham ","Subject":"Re: [PATCH v3 2/2] ipa: libipa: Introduce V4L2Stats","Message-ID":"","References":"<20260508-extensible-stats-v3-0-f2174ab4e124@ideasonboard.com>\n\t<20260508-extensible-stats-v3-2-f2174ab4e124@ideasonboard.com>\n\t<1abfc895-844f-404b-9d0a-aece9dac47a1@ideasonboard.com>\n\t\n\t<49ffe6b3-d0dc-4ac7-b9af-88ee2c02acda@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<49ffe6b3-d0dc-4ac7-b9af-88ee2c02acda@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]