[{"id":38795,"web_url":"https://patchwork.libcamera.org/comment/38795/","msgid":"<31916af4-32a0-4648-ae60-fe203cf18ea5@ideasonboard.com>","date":"2026-05-08T07:11:07","subject":"Re: [PATCH v2 2/3] 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. 07. 20:52 keltezéssel, Laurent Pinchart írta:\n> Hi Jacopo,\n> \n> Thank you for the patch.\n> \n> On Thu, May 07, 2026 at 12:07:55PM +0200, Jacopo Mondi wrote:\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 | 226 ++++++++++++++++++++++++++++++++++++++++++\n>> src/ipa/libipa/v4l2_stats.h | 123 +++++++++++++++++++++++\n>> 3 files changed, 351 insertions(+)\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..b3b3ffca185d\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/v4l2_stats.h\n>> @@ -0,0 +1,123 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2026, Ideas On Board\n>> + *\n>> + * V4L2 Stats\n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include \n>> +\n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa {\n>> +\n>> +LOG_DECLARE_CATEGORY(V4L2Stats)\n>> +\n>> +template\n>> +class V4L2StatsBlock\n>> +{\n>> +public:\n>> +\tV4L2StatsBlock(const Span data)\n\nThe top-level `const` can be dropped here.\n\n\n>> +\t\t: data_(data)\n>> +\t{\n>> +\t}\n>> +\n>> +\t~V4L2StatsBlock() {}\n> \n> Is this needed ?\n> \n>> +\n>> +\tconst T *operator->() const\n>> +\t{\n>> +\t\treturn reinterpret_cast(data_.data());\n>> +\t}\n>> +\n>> +\tconst T &operator*() const\n>> +\t{\n>> +\t\treturn *reinterpret_cast(data_.data());\n>> +\t}\n>> +\n>> +\tsize_t size() const\n>> +\t{\n>> +\t\treturn data_.size();\n>> +\t}\n> \n> A function that returns if the block is valid would be useful. This can\n> be queried with size(), but that's not self-explicit.\n\nEarlier I suggested just returning `const T *` instead of `V4L2StatsBlock`\nfrom the `block()` function, I think that satisfies this requirement easily,\nand I still think that would be a net simplification here.\n\n\n> \n>> +\n>> +protected:\n>> +\tSpan data_;\n>> +};\n>> +\n>> +template\n>> +class V4L2Stats\n>> +{\n>> +public:\n>> +\tV4L2Stats(Span data, unsigned int version)\n>> +\t\t: data_(data)\n>> +\t{\n>> +\t\tstruct v4l2_isp_buffer *stats =\n> \n> const\n> \n>> +\t\t\treinterpret_cast(data_.data());\n>> +\t\tused_ = stats->data_size;\n>> +\n>> +\t\tif (version != stats->version)\n>> +\t\t\tLOG(V4L2Stats, Error)\n>> +\t\t\t\t<< \"Unsupported v4l2-isp version: \" << stats->version;\n> \n> It would be good to also validate that stats->data_size fits in\n> data.size().\n> \n> When validation fails, the V4L2Stats instance will be invalid. There\n> should be a way for users to see that.\n\nIf all blocks are parsed when constructed, then adding something like\n\n static std::optional from(Span);\n\nshould work for this purpose?\n\n\n> \n> None of those checks depend on Traits or on member variables, so you\n> could move them to a separate non-template base class and avoid inlining\n> them. See the series I've just sent for V4L2Params for an example.\n> \n>> +\t}\n>> +\n>> +\tsize_t bytesused() const { return used_; }\n> \n> I don't see this being used in your RPP-X1 IPA code. What's the\n> envisioned use case ?\n> \n>> +\n>> +\ttemplate\n>> +\tauto block() 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 = block(kernelId, sizeof(Type));\n>> +\t\treturn V4L2StatsBlock(data);\n>> +\t}\n>> +\n>> +protected:\n>> +\tconst Span block(unsigned int blockType, size_t blockSize) const\n>> +\t{\n>> +\t\tstruct v4l2_isp_buffer *stats =\n>> +\t\t\treinterpret_cast(data_.data());\n>> +\n>> +\t\t__u8 *data = stats->data;\n>> +\t\twhile (data < stats->data + stats->data_size) {\n>> +\t\t\tstruct v4l2_isp_block_header *header =\n>> +\t\t\t\treinterpret_cast(data);\n> \n> const\n> \n>> +\n>> +\t\t\tif (header->type != blockType) {\n>> +\t\t\t\tdata += header->size;\n> \n> There's a risk of integer overflow.\n> \n>> +\t\t\t\tcontinue;\n>> +\t\t\t}\n>> +\n>> +\t\t\tif (header->size != blockSize) {\n>> +\t\t\t\tLOG(V4L2Stats, Error)\n>> +\t\t\t\t\t<< \"Block type \" << blockType\n>> +\t\t\t\t\t<< \" size mistmatch: expected \"\n>> +\t\t\t\t\t<< blockSize << \" got:\"\n>> +\t\t\t\t\t<< header->size;\n>> +\t\t\t\treturn {};\n>> +\t\t\t}\n>> +\n>> +\t\t\treturn Span(data, header->size);\n> \n> There's a risk of going beyond the end of the buffer.\n> \n>> +\t\t}\n> \n> I would expect that in a typical use case the algorithms will use all\n> available blocks, and different algorithms would use the same block.\n\nIsn't it more likely that if they need to, they will use data from the other algorithms \"frame context\"?\n\n\n> Have you considered introducing a caching mechanism like in V4L2Params ?\n> You could even populate the cache in the constructor by parsing the\n> whole buffer.\n> \n>> +\n>> +\t\tLOG(V4L2Stats, Error) << \"Unsupported stats block type: \"\n>> +\t\t\t\t << blockType;\n>> +\n>> +\t\treturn {};\n>> +\t}\n>> +\n>> +\tSpan data_;\n>> +\tsize_t used_;\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 C62C7BDCBD\n\tfor ;\n\tFri, 8 May 2026 07:11:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E13CD62FE1;\n\tFri, 8 May 2026 09:11:12 +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 6177062E6A\n\tfor ;\n\tFri, 8 May 2026 09:11:11 +0200 (CEST)","from [192.168.33.85] (185.221.140.217.nat.pool.zt.hu\n\t[185.221.140.217])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 770B9BCA;\n\tFri, 8 May 2026 09:11:06 +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=\"Optqse+B\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1778224266;\n\tbh=4O9V2tTG7Diy2ZgIql9hX2LXarWnsbxiKJeRaQOpZ+I=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Optqse+BXyhah+/wUg0AExFt5w17Ftc7goNrZJK++1JIdbFne+BNC9pUwvjowQbeH\n\tQbRHUlNmCPeLkpt78EBe8ESF9ixNk5OvMSHVMY3mtcSB+lROjubEinuVaZDOmmnRHY\n\tPYk4I75XlzW1AuBs2n3eaJt1Ae655uT0rMDuDG5s=","Message-ID":"<31916af4-32a0-4648-ae60-fe203cf18ea5@ideasonboard.com>","Date":"Fri, 8 May 2026 09:11:07 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 2/3] ipa: libipa: Introduce V4L2Stats","To":"Laurent Pinchart ,\n\tJacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tAntoine Bouyer ,\n\tKieran Bingham ","References":"<20260507-extensible-stats-v2-0-dfca939ad327@ideasonboard.com>\n\t<20260507-extensible-stats-v2-2-dfca939ad327@ideasonboard.com>\n\t<20260507185212.GA2041714@killaraus.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20260507185212.GA2041714@killaraus.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":38802,"web_url":"https://patchwork.libcamera.org/comment/38802/","msgid":"","date":"2026-05-08T08:57:33","subject":"Re: [PATCH v2 2/3] 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 Laurent\n\nOn Thu, May 07, 2026 at 09:52:12PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, May 07, 2026 at 12:07:55PM +0200, Jacopo Mondi wrote:\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 | 226 ++++++++++++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/v4l2_stats.h | 123 +++++++++++++++++++++++\n> > 3 files changed, 351 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..19947a8d9015\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/v4l2_stats.cpp\n> > @@ -0,0 +1,226 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2026, Ideas On Board\n> > + *\n> > + * V4L2 Statistics\n>\n> \"V4L2 ISP Statistics\" would be more accurate.\n>\n> Same in the .h files.\n>\n> > + */\n> > +\n> > +#include \"v4l2_stats.h\"\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 blocks, represented as\n> > + * a V4L2StatsBlock class instances.\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 V4L2StatsBlock\n> > + * \\brief Helper class that represents an ISP statistics block\n> > + *\n> > + * Each ISP measurement block produces a set of statistics data whose memory\n> > + * layout is defined by the kernel interface.\n> > + *\n> > + * This class represents an ISP statistics block entry. It is constructed\n> > + * with a reference to the memory area where the statistics block is\n> > + * stored in the statistics buffer. The template parameter represents\n> > + * the underlying kernel-defined ISP statistics block type and allows its\n> > + * user to easily cast it to said type to read the statistics data.\n> > + *\n> > + * \\sa V4L2Stats::block()\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2StatsBlock::V4L2StatsBlock()\n> > + * \\brief Construct a V4L2StatsBlock with memory represented by \\a data\n> > + * \\param[in] data The memory area where the statistics block is located\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2StatsBlock::operator->() const\n> > + * \\brief Access the statistics block casting it to the kernel-defined\n> > + * ISP block type\n> > + *\n> > + * The V4L2StatsBlock is templated with the kernel defined ISP block type. This\n> > + * function allows users to easily cast a V4L2StatsBlock to the underlying\n> > + * kernel-defined type in order to easily access the statistics data.\n> > + *\n> > + * \\code{.cpp}\n> > + *\n> > + * // The kernel header defines the ISP statistics types, for example\n> > + * // struct my_isp_awb_stats_data {\n> > + * //\t\tu16 mean_r;\n> > + * //\t\tu16 mean_g;\n> > + * //\t\tu16 mean_b;\n> > + * // }\n> > + *\n> > + * template<> V4L2StatsBlock awbStats = ...\n> > + *\n> > + * u16 mean_r = awbStats->mean_r;\n> > + * u16 mean_g = awbStats->mean_g;\n> > + * u16 mean_b = awbStats->mean_b;\n> > + *\n> > + * \\endcode\n> > + *\n> > + * Users of this class shall not create a V4L2StatsBlock manually but should\n> > + * use V4L2Stats::block().\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2StatsBlock::operator*() const\n> > + * \\copydoc V4L2StatsBlock::operator->() const\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2StatsBlock::size() const\n> > + * \\brief Retrieve the size (in bytes) of the ISP statistics block, including\n> > + * the block's header\n> > + * \\return The size of the stats block\n> > + */\n> > +\n> > +/**\n> > + * \\var V4L2StatsBlock::data_\n> > + * \\brief Memory area where ISP statistics have been serialized\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\n> > + * class MyISPStats : public V4L2Stats\n> > + * {\n> > + * public:\n> > + * \tMyISPStats::MyISPStats(Span data)\n> > + * \t\t: V4L2Stats(data)\n> > + * \t{\n> > + * \t}\n> > + * };\n> > + *\n> > + * \\endcode\n> > + *\n> > + * Users of this class can then easily access an ISP statistics block as a\n> > + * V4L2StatsBlock instance.\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::bytesused()\n> > + * \\brief Retrieve the size of the statistics buffer (in bytes)\n> > + * \\return The number of bytes occupied by the ISP statistics\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2Stats::block() const\n> > + * \\brief Retrieve the location of an ISP statistics block\n> > + * \\return A V4L2StatsBlock instance that points to the ISP statistics block\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2Stats::block(unsigned int blockType, size_t blockSize) const\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> > + * Parse the buffer of serialized statistics data and retrieve the location\n> > + * of the block with type \\a blockType of size \\a blockSize.\n> > + *\n> > + * IPA modules that derive the V4L2Stats class shall use this function to\n> > + * retrieve the memory area that will be used to construct a V4L2StatsBlock\n> > + * before returning it to the caller.\n> > + */\n> > +\n> > +/**\n> > + * \\var V4L2Stats::data_\n> > + * \\brief The ISP statistics buffer memory\n> > + */\n> > +\n> > +/**\n> > + * \\var V4L2Stats::used_\n> > + * \\brief The number of bytes used by the statistics buffer\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..b3b3ffca185d\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/v4l2_stats.h\n> > @@ -0,0 +1,123 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2026, Ideas On Board\n> > + *\n> > + * V4L2 Stats\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa {\n> > +\n> > +LOG_DECLARE_CATEGORY(V4L2Stats)\n> > +\n> > +template\n> > +class V4L2StatsBlock\n> > +{\n> > +public:\n> > +\tV4L2StatsBlock(const Span data)\n> > +\t\t: data_(data)\n> > +\t{\n> > +\t}\n> > +\n> > +\t~V4L2StatsBlock() {}\n>\n> Is this needed ?\n>\n\nProbably not\n\n> > +\n> > +\tconst T *operator->() const\n> > +\t{\n> > +\t\treturn reinterpret_cast(data_.data());\n> > +\t}\n> > +\n> > +\tconst T &operator*() const\n> > +\t{\n> > +\t\treturn *reinterpret_cast(data_.data());\n> > +\t}\n> > +\n> > +\tsize_t size() const\n> > +\t{\n> > +\t\treturn data_.size();\n> > +\t}\n>\n> A function that returns if the block is valid would be useful. This can\n> be queried with size(), but that's not self-explicit.\n>\n> > +\n> > +protected:\n> > +\tSpan data_;\n> > +};\n> > +\n> > +template\n> > +class V4L2Stats\n> > +{\n> > +public:\n> > +\tV4L2Stats(Span data, unsigned int version)\n> > +\t\t: data_(data)\n> > +\t{\n> > +\t\tstruct v4l2_isp_buffer *stats =\n>\n> const\n>\n> > +\t\t\treinterpret_cast(data_.data());\n> > +\t\tused_ = stats->data_size;\n> > +\n> > +\t\tif (version != stats->version)\n> > +\t\t\tLOG(V4L2Stats, Error)\n> > +\t\t\t\t<< \"Unsupported v4l2-isp version: \" << stats->version;\n>\n> It would be good to also validate that stats->data_size fits in\n> data.size().\n>\n\nThe kernel should validate that, as a driver using the v4l2-isp helper\nfrom\nhttps://patchwork.linuxtv.org/project/linux-media/patch/20260505-extensible-stats-v1-6-e16f326b8dad@ideasonboard.com/\nshouldn't be able to populate stat blocks past the end of the buffer\nsize.\n\n> When validation fails, the V4L2Stats instance will be invalid. There\n> should be a way for users to see that.\n>\n> None of those checks depend on Traits or on member variables, so you\n> could move them to a separate non-template base class and avoid inlining\n> them. See the series I've just sent for V4L2Params for an example.\n>\n\nI'll have a look\n\n> > +\t}\n> > +\n> > +\tsize_t bytesused() const { return used_; }\n>\n> I don't see this being used in your RPP-X1 IPA code. What's the\n> envisioned use case ?\n>\n\npossibily a leftover from V4L2Params\n\n> > +\n> > +\ttemplate\n> > +\tauto block() 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 = block(kernelId, sizeof(Type));\n> > +\t\treturn V4L2StatsBlock(data);\n> > +\t}\n> > +\n> > +protected:\n> > +\tconst Span block(unsigned int blockType, size_t blockSize) const\n> > +\t{\n> > +\t\tstruct v4l2_isp_buffer *stats =\n> > +\t\t\treinterpret_cast(data_.data());\n> > +\n> > +\t\t__u8 *data = stats->data;\n> > +\t\twhile (data < stats->data + stats->data_size) {\n> > +\t\t\tstruct v4l2_isp_block_header *header =\n> > +\t\t\t\treinterpret_cast(data);\n>\n> const\n>\n> > +\n> > +\t\t\tif (header->type != blockType) {\n> > +\t\t\t\tdata += header->size;\n>\n> There's a risk of integer overflow.\n\nAm not I just advancing the data pointer here ?\n\n>\n> > +\t\t\t\tcontinue;\n> > +\t\t\t}\n> > +\n> > +\t\t\tif (header->size != blockSize) {\n> > +\t\t\t\tLOG(V4L2Stats, Error)\n> > +\t\t\t\t\t<< \"Block type \" << blockType\n> > +\t\t\t\t\t<< \" size mistmatch: expected \"\n> > +\t\t\t\t\t<< blockSize << \" got:\"\n> > +\t\t\t\t\t<< header->size;\n> > +\t\t\t\treturn {};\n> > +\t\t\t}\n> > +\n> > +\t\t\treturn Span(data, header->size);\n>\n> There's a risk of going beyond the end of the buffer.\n\nShould I just check that 'data + header->size <= stats->data +\nstats->data_size' ?\n\n>\n> > +\t\t}\n>\n> I would expect that in a typical use case the algorithms will use all\n> available blocks, and different algorithms would use the same block.\n\nThis currently doesn't happen, right ? However it is certainly\npossible\n\n> Have you considered introducing a caching mechanism like in V4L2Params ?\n\nI did. But if the cache is a class member I lose all the cost-ness as\ncreating a V4L2StatsBlock would require to modify the V4L2Stats state\nto update the cache.\n\n> You could even populate the cache in the constructor by parsing the\n> whole buffer.\n\nHonestly, I thought this was an overkill as currently stats blocks are\naccessed once per frame.\n\nBut it's true that every access would require re-parsing the buffer to\nfind the right block, so it might be more efficient to just do that\nonce at class creation time. Once we have indexes stored in the cached\nlooking up a block would simply be a matter of adding the offset to\nthe stats->data[] pointer ?\n\n>\n> > +\n> > +\t\tLOG(V4L2Stats, Error) << \"Unsupported stats block type: \"\n> > +\t\t\t\t << blockType;\n> > +\n> > +\t\treturn {};\n> > +\t}\n> > +\n> > +\tSpan data_;\n> > +\tsize_t used_;\n> > +};\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\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 7ADE6BDCBD\n\tfor ;\n\tFri, 8 May 2026 08:57:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ACC0762FE1;\n\tFri, 8 May 2026 10:57:37 +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 4F33E62E6A\n\tfor ;\n\tFri, 8 May 2026 10:57:36 +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 93F7B9A4;\n\tFri, 8 May 2026 10:57: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=\"HS01L5tQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1778230651;\n\tbh=RmAgjN4iGHUG6Pi07sm0opURyl+eF3JYz0j7+b0+6s4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HS01L5tQuPnCQ+2mKsCoqXzUG+3DqerEuOxSTwBXw4vycaIUes3FE/YugxxmBJRsa\n\tO7s3sFYo08sCwCYJWCdNHIPopET+EBNkqeUjmevSYGtuOqBpDNFp0I5yYpSnnvygpG\n\tuFlspGcgHl6t63p0Npk0rDcTv3fSz09mthiidbDQ=","Date":"Fri, 8 May 2026 10:57:33 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tAntoine Bouyer , \n\tKieran Bingham ","Subject":"Re: [PATCH v2 2/3] ipa: libipa: Introduce V4L2Stats","Message-ID":"","References":"<20260507-extensible-stats-v2-0-dfca939ad327@ideasonboard.com>\n\t<20260507-extensible-stats-v2-2-dfca939ad327@ideasonboard.com>\n\t<20260507185212.GA2041714@killaraus.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20260507185212.GA2041714@killaraus.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":38803,"web_url":"https://patchwork.libcamera.org/comment/38803/","msgid":"","date":"2026-05-08T09:01:55","subject":"Re: [PATCH v2 2/3] 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 Fri, May 08, 2026 at 09:11:07AM +0200, Barnabás Pőcze wrote:\n> 2026. 05. 07. 20:52 keltezéssel, Laurent Pinchart írta:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Thu, May 07, 2026 at 12:07:55PM +0200, Jacopo Mondi wrote:\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 | 226 ++++++++++++++++++++++++++++++++++++++++++\n> > > src/ipa/libipa/v4l2_stats.h | 123 +++++++++++++++++++++++\n> > > 3 files changed, 351 insertions(+)\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..b3b3ffca185d\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/v4l2_stats.h\n> > > @@ -0,0 +1,123 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2026, Ideas On Board\n> > > + *\n> > > + * V4L2 Stats\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa {\n> > > +\n> > > +LOG_DECLARE_CATEGORY(V4L2Stats)\n> > > +\n> > > +template\n> > > +class V4L2StatsBlock\n> > > +{\n> > > +public:\n> > > +\tV4L2StatsBlock(const Span data)\n>\n> The top-level `const` can be dropped here.\n>\n>\n> > > +\t\t: data_(data)\n> > > +\t{\n> > > +\t}\n> > > +\n> > > +\t~V4L2StatsBlock() {}\n> >\n> > Is this needed ?\n> >\n> > > +\n> > > +\tconst T *operator->() const\n> > > +\t{\n> > > +\t\treturn reinterpret_cast(data_.data());\n> > > +\t}\n> > > +\n> > > +\tconst T &operator*() const\n> > > +\t{\n> > > +\t\treturn *reinterpret_cast(data_.data());\n> > > +\t}\n> > > +\n> > > +\tsize_t size() const\n> > > +\t{\n> > > +\t\treturn data_.size();\n> > > +\t}\n> >\n> > A function that returns if the block is valid would be useful. This can\n> > be queried with size(), but that's not self-explicit.\n>\n> Earlier I suggested just returning `const T *` instead of `V4L2StatsBlock`\n> from the `block()` function, I think that satisfies this requirement easily,\n> and I still think that would be a net simplification here.\n>\n\nAs said, none of the currently mainlined ISP drivers use extensible\nstats. If one of them gets ported to to use them we'll have to handle\ntwo formats as we currently do for RkISP1 parameters and\nV4L2StatsBlock would be the place where we'll handle the differences\nbetween a \"legacy\" format and an extensible one (possibily by simply\ndiscarding the header before returning the memory location of a stats\nblock).\n\nThe cost of abstraction is cheap but if there is consensus on\ndeferring the introduction of V4L2StatsBlock to when we'll have an\nexisting ISP ported to use extensible stats, I'll do so.\n\n>\n> >\n> > > +\n> > > +protected:\n> > > +\tSpan data_;\n> > > +};\n> > > +\n> > > +template\n> > > +class V4L2Stats\n> > > +{\n> > > +public:\n> > > +\tV4L2Stats(Span data, unsigned int version)\n> > > +\t\t: data_(data)\n> > > +\t{\n> > > +\t\tstruct v4l2_isp_buffer *stats =\n> >\n> > const\n> >\n> > > +\t\t\treinterpret_cast(data_.data());\n> > > +\t\tused_ = stats->data_size;\n> > > +\n> > > +\t\tif (version != stats->version)\n> > > +\t\t\tLOG(V4L2Stats, Error)\n> > > +\t\t\t\t<< \"Unsupported v4l2-isp version: \" << stats->version;\n> >\n> > It would be good to also validate that stats->data_size fits in\n> > data.size().\n> >\n> > When validation fails, the V4L2Stats instance will be invalid. There\n> > should be a way for users to see that.\n>\n> If all blocks are parsed when constructed, then adding something like\n>\n> static std::optional from(Span);\n>\n> should work for this purpose?\n\nAre you suggesting this a static method of V4L2Stats to replace the\nconstructor ?\n\n>\n>\n> >\n> > None of those checks depend on Traits or on member variables, so you\n> > could move them to a separate non-template base class and avoid inlining\n> > them. See the series I've just sent for V4L2Params for an example.\n> >\n> > > +\t}\n> > > +\n> > > +\tsize_t bytesused() const { return used_; }\n> >\n> > I don't see this being used in your RPP-X1 IPA code. What's the\n> > envisioned use case ?\n> >\n> > > +\n> > > +\ttemplate\n> > > +\tauto block() 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 = block(kernelId, sizeof(Type));\n> > > +\t\treturn V4L2StatsBlock(data);\n> > > +\t}\n> > > +\n> > > +protected:\n> > > +\tconst Span block(unsigned int blockType, size_t blockSize) const\n> > > +\t{\n> > > +\t\tstruct v4l2_isp_buffer *stats =\n> > > +\t\t\treinterpret_cast(data_.data());\n> > > +\n> > > +\t\t__u8 *data = stats->data;\n> > > +\t\twhile (data < stats->data + stats->data_size) {\n> > > +\t\t\tstruct v4l2_isp_block_header *header =\n> > > +\t\t\t\treinterpret_cast(data);\n> >\n> > const\n> >\n> > > +\n> > > +\t\t\tif (header->type != blockType) {\n> > > +\t\t\t\tdata += header->size;\n> >\n> > There's a risk of integer overflow.\n> >\n> > > +\t\t\t\tcontinue;\n> > > +\t\t\t}\n> > > +\n> > > +\t\t\tif (header->size != blockSize) {\n> > > +\t\t\t\tLOG(V4L2Stats, Error)\n> > > +\t\t\t\t\t<< \"Block type \" << blockType\n> > > +\t\t\t\t\t<< \" size mistmatch: expected \"\n> > > +\t\t\t\t\t<< blockSize << \" got:\"\n> > > +\t\t\t\t\t<< header->size;\n> > > +\t\t\t\treturn {};\n> > > +\t\t\t}\n> > > +\n> > > +\t\t\treturn Span(data, header->size);\n> >\n> > There's a risk of going beyond the end of the buffer.\n> >\n> > > +\t\t}\n> >\n> > I would expect that in a typical use case the algorithms will use all\n> > available blocks, and different algorithms would use the same block.\n>\n> Isn't it more likely that if they need to, they will use data from the other algorithms \"frame context\"?\n>\n>\n> > Have you considered introducing a caching mechanism like in V4L2Params ?\n> > You could even populate the cache in the constructor by parsing the\n> > whole buffer.\n> >\n> > > +\n> > > +\t\tLOG(V4L2Stats, Error) << \"Unsupported stats block type: \"\n> > > +\t\t\t\t << blockType;\n> > > +\n> > > +\t\treturn {};\n> > > +\t}\n> > > +\n> > > +\tSpan data_;\n> > > +\tsize_t used_;\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 94101BDCBD\n\tfor ;\n\tFri, 8 May 2026 09:02:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A273D62FEC;\n\tFri, 8 May 2026 11:01:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B446A62E6A\n\tfor ;\n\tFri, 8 May 2026 11:01:58 +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 E4F15BCA;\n\tFri, 8 May 2026 11:01:53 +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=\"kyEOCmJs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1778230914;\n\tbh=objv/lbOJ5sakHXdvileBfNoa1el/7Fk+A1zIGjTCVU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kyEOCmJsSKgHBwNZhtlBB8Ag3uLgHybs+XqYKregCb5nwfct8F48u1QZ0m7ZNte+P\n\t4MQAQqMqg5LWGC8xaJLcwoxPMjCjjnQFdrFLqy74mEpAo1xHBLnpJWRXCRJCUEALq6\n\teu1gjomcR/4xQ91/bIUb/y53bm3UVDqtm8m7wGx0=","Date":"Fri, 8 May 2026 11:01:55 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Laurent Pinchart , \n\tJacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org, \n\tAntoine Bouyer ,\n\tKieran Bingham ","Subject":"Re: [PATCH v2 2/3] ipa: libipa: Introduce V4L2Stats","Message-ID":"","References":"<20260507-extensible-stats-v2-0-dfca939ad327@ideasonboard.com>\n\t<20260507-extensible-stats-v2-2-dfca939ad327@ideasonboard.com>\n\t<20260507185212.GA2041714@killaraus.ideasonboard.com>\n\t<31916af4-32a0-4648-ae60-fe203cf18ea5@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<31916af4-32a0-4648-ae60-fe203cf18ea5@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":38807,"web_url":"https://patchwork.libcamera.org/comment/38807/","msgid":"<8161dbe9-3e30-45fd-9de2-a31bca78e0db@ideasonboard.com>","date":"2026-05-08T09:22:10","subject":"Re: [PATCH v2 2/3] 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. 08. 11:01 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Fri, May 08, 2026 at 09:11:07AM +0200, Barnabás Pőcze wrote:\n>> 2026. 05. 07. 20:52 keltezéssel, Laurent Pinchart írta:\n>>> Hi Jacopo,\n>>>\n>>> Thank you for the patch.\n>>>\n>>> On Thu, May 07, 2026 at 12:07:55PM +0200, Jacopo Mondi wrote:\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 | 226 ++++++++++++++++++++++++++++++++++++++++++\n>>>> src/ipa/libipa/v4l2_stats.h | 123 +++++++++++++++++++++++\n>>>> 3 files changed, 351 insertions(+)\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..b3b3ffca185d\n>>>> --- /dev/null\n>>>> +++ b/src/ipa/libipa/v4l2_stats.h\n>>>> @@ -0,0 +1,123 @@\n>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2026, Ideas On Board\n>>>> + *\n>>>> + * V4L2 Stats\n>>>> + */\n>>>> +\n>>>> +#pragma once\n>>>> +\n>>>> +#include \n>>>> +\n>>>> +#include \n>>>> +\n>>>> +#include \n>>>> +#include \n>>>> +\n>>>> +namespace libcamera {\n>>>> +\n>>>> +namespace ipa {\n>>>> +\n>>>> +LOG_DECLARE_CATEGORY(V4L2Stats)\n>>>> +\n>>>> +template\n>>>> +class V4L2StatsBlock\n>>>> +{\n>>>> +public:\n>>>> +\tV4L2StatsBlock(const Span data)\n>>\n>> The top-level `const` can be dropped here.\n>>\n>>\n>>>> +\t\t: data_(data)\n>>>> +\t{\n>>>> +\t}\n>>>> +\n>>>> +\t~V4L2StatsBlock() {}\n>>>\n>>> Is this needed ?\n>>>\n>>>> +\n>>>> +\tconst T *operator->() const\n>>>> +\t{\n>>>> +\t\treturn reinterpret_cast(data_.data());\n>>>> +\t}\n>>>> +\n>>>> +\tconst T &operator*() const\n>>>> +\t{\n>>>> +\t\treturn *reinterpret_cast(data_.data());\n>>>> +\t}\n>>>> +\n>>>> +\tsize_t size() const\n>>>> +\t{\n>>>> +\t\treturn data_.size();\n>>>> +\t}\n>>>\n>>> A function that returns if the block is valid would be useful. This can\n>>> be queried with size(), but that's not self-explicit.\n>>\n>> Earlier I suggested just returning `const T *` instead of `V4L2StatsBlock`\n>> from the `block()` function, I think that satisfies this requirement easily,\n>> and I still think that would be a net simplification here.\n>>\n> \n> As said, none of the currently mainlined ISP drivers use extensible\n> stats. If one of them gets ported to to use them we'll have to handle\n> two formats as we currently do for RkISP1 parameters and\n> V4L2StatsBlock would be the place where we'll handle the differences\n> between a \"legacy\" format and an extensible one (possibily by simply\n> discarding the header before returning the memory location of a stats\n> block).\n> \n> The cost of abstraction is cheap but if there is consensus on\n> deferring the introduction of V4L2StatsBlock to when we'll have an\n> existing ISP ported to use extensible stats, I'll do so.\n\nOkay, fair enough. It's a small part, so either way it shouldn't be too\ndifficult to change when old format support is implemented.\n\n\n> \n>>\n>>>\n>>>> +\n>>>> +protected:\n>>>> +\tSpan data_;\n>>>> +};\n>>>> +\n>>>> +template\n>>>> +class V4L2Stats\n>>>> +{\n>>>> +public:\n>>>> +\tV4L2Stats(Span data, unsigned int version)\n>>>> +\t\t: data_(data)\n>>>> +\t{\n>>>> +\t\tstruct v4l2_isp_buffer *stats =\n>>>\n>>> const\n>>>\n>>>> +\t\t\treinterpret_cast(data_.data());\n>>>> +\t\tused_ = stats->data_size;\n>>>> +\n>>>> +\t\tif (version != stats->version)\n>>>> +\t\t\tLOG(V4L2Stats, Error)\n>>>> +\t\t\t\t<< \"Unsupported v4l2-isp version: \" << stats->version;\n>>>\n>>> It would be good to also validate that stats->data_size fits in\n>>> data.size().\n>>>\n>>> When validation fails, the V4L2Stats instance will be invalid. There\n>>> should be a way for users to see that.\n>>\n>> If all blocks are parsed when constructed, then adding something like\n>>\n>> static std::optional from(Span);\n>>\n>> should work for this purpose?\n> \n> Are you suggesting this a static method of V4L2Stats to replace the\n> constructor ?\n\nIt was just a suggestion, if we want to validate the buffer and populate\nan (id -> span) mapping from the content, then I think a static factory\nfunction + private constructor could be one way to do it.\n\n\n> \n>>\n>>\n>>>\n>>> None of those checks depend on Traits or on member variables, so you\n>>> could move them to a separate non-template base class and avoid inlining\n>>> them. See the series I've just sent for V4L2Params for an example.\n>>>\n>>>> +\t}\n>>>> +\n>>>> +\tsize_t bytesused() const { return used_; }\n>>>\n>>> I don't see this being used in your RPP-X1 IPA code. What's the\n>>> envisioned use case ?\n>>>\n>>>> +\n>>>> +\ttemplate\n>>>> +\tauto block() 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 = block(kernelId, sizeof(Type));\n>>>> +\t\treturn V4L2StatsBlock(data);\n>>>> +\t}\n>>>> +\n>>>> +protected:\n>>>> +\tconst Span block(unsigned int blockType, size_t blockSize) const\n>>>> +\t{\n>>>> +\t\tstruct v4l2_isp_buffer *stats =\n>>>> +\t\t\treinterpret_cast(data_.data());\n>>>> +\n>>>> +\t\t__u8 *data = stats->data;\n>>>> +\t\twhile (data < stats->data + stats->data_size) {\n>>>> +\t\t\tstruct v4l2_isp_block_header *header =\n>>>> +\t\t\t\treinterpret_cast(data);\n>>>\n>>> const\n>>>\n>>>> +\n>>>> +\t\t\tif (header->type != blockType) {\n>>>> +\t\t\t\tdata += header->size;\n>>>\n>>> There's a risk of integer overflow.\n>>>\n>>>> +\t\t\t\tcontinue;\n>>>> +\t\t\t}\n>>>> +\n>>>> +\t\t\tif (header->size != blockSize) {\n>>>> +\t\t\t\tLOG(V4L2Stats, Error)\n>>>> +\t\t\t\t\t<< \"Block type \" << blockType\n>>>> +\t\t\t\t\t<< \" size mistmatch: expected \"\n>>>> +\t\t\t\t\t<< blockSize << \" got:\"\n>>>> +\t\t\t\t\t<< header->size;\n>>>> +\t\t\t\treturn {};\n>>>> +\t\t\t}\n>>>> +\n>>>> +\t\t\treturn Span(data, header->size);\n>>>\n>>> There's a risk of going beyond the end of the buffer.\n>>>\n>>>> +\t\t}\n>>>\n>>> I would expect that in a typical use case the algorithms will use all\n>>> available blocks, and different algorithms would use the same block.\n>>\n>> Isn't it more likely that if they need to, they will use data from the other algorithms \"frame context\"?\n>>\n>>\n>>> Have you considered introducing a caching mechanism like in V4L2Params ?\n>>> You could even populate the cache in the constructor by parsing the\n>>> whole buffer.\n>>>\n>>>> +\n>>>> +\t\tLOG(V4L2Stats, Error) << \"Unsupported stats block type: \"\n>>>> +\t\t\t\t << blockType;\n>>>> +\n>>>> +\t\treturn {};\n>>>> +\t}\n>>>> +\n>>>> +\tSpan data_;\n>>>> +\tsize_t used_;\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 D80A2BE173\n\tfor ;\n\tFri, 8 May 2026 09:22:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D727062FEC;\n\tFri, 8 May 2026 11:22:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F34C862E6A\n\tfor ;\n\tFri, 8 May 2026 11:22:13 +0200 (CEST)","from [192.168.33.85] (185.221.140.217.nat.pool.zt.hu\n\t[185.221.140.217])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A34B0BCA;\n\tFri, 8 May 2026 11:22:08 +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=\"IBE0Tkr6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1778232129;\n\tbh=oPfJIwnIqI1FXipO7IVaUTIPp+8rAAzpUUVPUOkUij0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=IBE0Tkr6avDUgNYypnLSeY2/MVJsy793xM4tb6QaElCxNzUPDXcVpvSj7NQXawOvG\n\tG5nI19IZyz4IU9MDTE8ZueHpkQ8nXb95pOdJw30nXtJ+pqcouEKCx8tv7KHePhDnu2\n\tPgp3SEGIYkJjG26WBku9CpxBrXyOu1JE5qMiF4yA=","Message-ID":"<8161dbe9-3e30-45fd-9de2-a31bca78e0db@ideasonboard.com>","Date":"Fri, 8 May 2026 11:22:10 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 2/3] ipa: libipa: Introduce V4L2Stats","To":"Jacopo Mondi ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org, Antoine Bouyer\n\t,\n\tKieran Bingham ","References":"<20260507-extensible-stats-v2-0-dfca939ad327@ideasonboard.com>\n\t<20260507-extensible-stats-v2-2-dfca939ad327@ideasonboard.com>\n\t<20260507185212.GA2041714@killaraus.ideasonboard.com>\n\t<31916af4-32a0-4648-ae60-fe203cf18ea5@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":38811,"web_url":"https://patchwork.libcamera.org/comment/38811/","msgid":"","date":"2026-05-08T09:32:56","subject":"Re: [PATCH v2 2/3] 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 Fri, May 08, 2026 at 11:22:10AM +0200, Barnabás Pőcze wrote:\n> 2026. 05. 08. 11:01 keltezéssel, Jacopo Mondi írta:\n> > Hi Barnabás\n> >\n> > On Fri, May 08, 2026 at 09:11:07AM +0200, Barnabás Pőcze wrote:\n> > > 2026. 05. 07. 20:52 keltezéssel, Laurent Pinchart írta:\n> > > > Hi Jacopo,\n> > > >\n> > > > Thank you for the patch.\n> > > >\n> > > > On Thu, May 07, 2026 at 12:07:55PM +0200, Jacopo Mondi wrote:\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 | 226 ++++++++++++++++++++++++++++++++++++++++++\n> > > > > src/ipa/libipa/v4l2_stats.h | 123 +++++++++++++++++++++++\n> > > > > 3 files changed, 351 insertions(+)\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..b3b3ffca185d\n> > > > > --- /dev/null\n> > > > > +++ b/src/ipa/libipa/v4l2_stats.h\n> > > > > @@ -0,0 +1,123 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2026, Ideas On Board\n> > > > > + *\n> > > > > + * V4L2 Stats\n> > > > > + */\n> > > > > +\n> > > > > +#pragma once\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +namespace ipa {\n> > > > > +\n> > > > > +LOG_DECLARE_CATEGORY(V4L2Stats)\n> > > > > +\n> > > > > +template\n> > > > > +class V4L2StatsBlock\n> > > > > +{\n> > > > > +public:\n> > > > > +\tV4L2StatsBlock(const Span data)\n> > >\n> > > The top-level `const` can be dropped here.\n> > >\n> > >\n> > > > > +\t\t: data_(data)\n> > > > > +\t{\n> > > > > +\t}\n> > > > > +\n> > > > > +\t~V4L2StatsBlock() {}\n> > > >\n> > > > Is this needed ?\n> > > >\n> > > > > +\n> > > > > +\tconst T *operator->() const\n> > > > > +\t{\n> > > > > +\t\treturn reinterpret_cast(data_.data());\n> > > > > +\t}\n> > > > > +\n> > > > > +\tconst T &operator*() const\n> > > > > +\t{\n> > > > > +\t\treturn *reinterpret_cast(data_.data());\n> > > > > +\t}\n> > > > > +\n> > > > > +\tsize_t size() const\n> > > > > +\t{\n> > > > > +\t\treturn data_.size();\n> > > > > +\t}\n> > > >\n> > > > A function that returns if the block is valid would be useful. This can\n> > > > be queried with size(), but that's not self-explicit.\n> > >\n> > > Earlier I suggested just returning `const T *` instead of `V4L2StatsBlock`\n> > > from the `block()` function, I think that satisfies this requirement easily,\n> > > and I still think that would be a net simplification here.\n> > >\n> >\n> > As said, none of the currently mainlined ISP drivers use extensible\n> > stats. If one of them gets ported to to use them we'll have to handle\n> > two formats as we currently do for RkISP1 parameters and\n> > V4L2StatsBlock would be the place where we'll handle the differences\n> > between a \"legacy\" format and an extensible one (possibily by simply\n> > discarding the header before returning the memory location of a stats\n> > block).\n> >\n> > The cost of abstraction is cheap but if there is consensus on\n> > deferring the introduction of V4L2StatsBlock to when we'll have an\n> > existing ISP ported to use extensible stats, I'll do so.\n>\n> Okay, fair enough. It's a small part, so either way it shouldn't be too\n> difficult to change when old format support is implemented.\n>\n\nWhat if we keep the abstraction for now and I add a bool() operator\nthat checks if the block is empty ?\n\n>\n> >\n> > >\n> > > >\n> > > > > +\n> > > > > +protected:\n> > > > > +\tSpan data_;\n> > > > > +};\n> > > > > +\n> > > > > +template\n> > > > > +class V4L2Stats\n> > > > > +{\n> > > > > +public:\n> > > > > +\tV4L2Stats(Span data, unsigned int version)\n> > > > > +\t\t: data_(data)\n> > > > > +\t{\n> > > > > +\t\tstruct v4l2_isp_buffer *stats =\n> > > >\n> > > > const\n> > > >\n> > > > > +\t\t\treinterpret_cast(data_.data());\n> > > > > +\t\tused_ = stats->data_size;\n> > > > > +\n> > > > > +\t\tif (version != stats->version)\n> > > > > +\t\t\tLOG(V4L2Stats, Error)\n> > > > > +\t\t\t\t<< \"Unsupported v4l2-isp version: \" << stats->version;\n> > > >\n> > > > It would be good to also validate that stats->data_size fits in\n> > > > data.size().\n> > > >\n> > > > When validation fails, the V4L2Stats instance will be invalid. There\n> > > > should be a way for users to see that.\n> > >\n> > > If all blocks are parsed when constructed, then adding something like\n> > >\n> > > static std::optional from(Span);\n> > >\n> > > should work for this purpose?\n> >\n> > Are you suggesting this a static method of V4L2Stats to replace the\n> > constructor ?\n>\n> It was just a suggestion, if we want to validate the buffer and populate\n> an (id -> span) mapping from the content, then I think a static factory\n> function + private constructor could be one way to do it.\n\nIndeed this could be a way to ensure a V4L2Stat has been constructed\ncorrectly and abort earlier if there are problems. I'll give\nV4L2Stats::from() a spin and see how it looks like.\n>\n>\n> >\n> > >\n> > >\n> > > >\n> > > > None of those checks depend on Traits or on member variables, so you\n> > > > could move them to a separate non-template base class and avoid inlining\n> > > > them. See the series I've just sent for V4L2Params for an example.\n> > > >\n> > > > > +\t}\n> > > > > +\n> > > > > +\tsize_t bytesused() const { return used_; }\n> > > >\n> > > > I don't see this being used in your RPP-X1 IPA code. What's the\n> > > > envisioned use case ?\n> > > >\n> > > > > +\n> > > > > +\ttemplate\n> > > > > +\tauto block() 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 = block(kernelId, sizeof(Type));\n> > > > > +\t\treturn V4L2StatsBlock(data);\n> > > > > +\t}\n> > > > > +\n> > > > > +protected:\n> > > > > +\tconst Span block(unsigned int blockType, size_t blockSize) const\n> > > > > +\t{\n> > > > > +\t\tstruct v4l2_isp_buffer *stats =\n> > > > > +\t\t\treinterpret_cast(data_.data());\n> > > > > +\n> > > > > +\t\t__u8 *data = stats->data;\n> > > > > +\t\twhile (data < stats->data + stats->data_size) {\n> > > > > +\t\t\tstruct v4l2_isp_block_header *header =\n> > > > > +\t\t\t\treinterpret_cast(data);\n> > > >\n> > > > const\n> > > >\n> > > > > +\n> > > > > +\t\t\tif (header->type != blockType) {\n> > > > > +\t\t\t\tdata += header->size;\n> > > >\n> > > > There's a risk of integer overflow.\n> > > >\n> > > > > +\t\t\t\tcontinue;\n> > > > > +\t\t\t}\n> > > > > +\n> > > > > +\t\t\tif (header->size != blockSize) {\n> > > > > +\t\t\t\tLOG(V4L2Stats, Error)\n> > > > > +\t\t\t\t\t<< \"Block type \" << blockType\n> > > > > +\t\t\t\t\t<< \" size mistmatch: expected \"\n> > > > > +\t\t\t\t\t<< blockSize << \" got:\"\n> > > > > +\t\t\t\t\t<< header->size;\n> > > > > +\t\t\t\treturn {};\n> > > > > +\t\t\t}\n> > > > > +\n> > > > > +\t\t\treturn Span(data, header->size);\n> > > >\n> > > > There's a risk of going beyond the end of the buffer.\n> > > >\n> > > > > +\t\t}\n> > > >\n> > > > I would expect that in a typical use case the algorithms will use all\n> > > > available blocks, and different algorithms would use the same block.\n> > >\n> > > Isn't it more likely that if they need to, they will use data from the other algorithms \"frame context\"?\n> > >\n> > >\n> > > > Have you considered introducing a caching mechanism like in V4L2Params ?\n> > > > You could even populate the cache in the constructor by parsing the\n> > > > whole buffer.\n> > > >\n> > > > > +\n> > > > > +\t\tLOG(V4L2Stats, Error) << \"Unsupported stats block type: \"\n> > > > > +\t\t\t\t << blockType;\n> > > > > +\n> > > > > +\t\treturn {};\n> > > > > +\t}\n> > > > > +\n> > > > > +\tSpan data_;\n> > > > > +\tsize_t used_;\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 2CC8CBDCBD\n\tfor ;\n\tFri, 8 May 2026 09:33:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DB14E63021;\n\tFri, 8 May 2026 11:32:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B36862E6A\n\tfor ;\n\tFri, 8 May 2026 11:32:59 +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 86C4BBCA;\n\tFri, 8 May 2026 11:32:54 +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=\"sr1hiXYg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1778232774;\n\tbh=zNgVWl4qzpdfsV+FlkEWRAKbeji0f0qiC+6Wbq844qY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sr1hiXYgVBylePRyKtsYzCCK9yPjUb6aWD9xQ2puqFRyUG4uF0TgqjfZIR+ePLTs4\n\t+OMPJ5/L3+YDkylaaoBTyWgFJaatd+2IuYfPlQ/Yp2Xw3/ncwlrhzMKmVlIq5S4OOO\n\t2lslOnMwjI4zdO0vgw2c0bJrkK3o6ytEajTT5YZg=","Date":"Fri, 8 May 2026 11:32:56 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tLaurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org, \n\tAntoine Bouyer ,\n\tKieran Bingham ","Subject":"Re: [PATCH v2 2/3] ipa: libipa: Introduce V4L2Stats","Message-ID":"","References":"<20260507-extensible-stats-v2-0-dfca939ad327@ideasonboard.com>\n\t<20260507-extensible-stats-v2-2-dfca939ad327@ideasonboard.com>\n\t<20260507185212.GA2041714@killaraus.ideasonboard.com>\n\t<31916af4-32a0-4648-ae60-fe203cf18ea5@ideasonboard.com>\n\t\n\t<8161dbe9-3e30-45fd-9de2-a31bca78e0db@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<8161dbe9-3e30-45fd-9de2-a31bca78e0db@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":38817,"web_url":"https://patchwork.libcamera.org/comment/38817/","msgid":"<20260508094953.GA2186850@killaraus.ideasonboard.com>","date":"2026-05-08T09:49:53","subject":"Re: [PATCH v2 2/3] ipa: libipa: Introduce V4L2Stats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, May 08, 2026 at 10:57:33AM +0200, Jacopo Mondi wrote:\n> On Thu, May 07, 2026 at 09:52:12PM +0300, Laurent Pinchart wrote:\n> > On Thu, May 07, 2026 at 12:07:55PM +0200, Jacopo Mondi wrote:\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 | 226 ++++++++++++++++++++++++++++++++++++++++++\n> > > src/ipa/libipa/v4l2_stats.h | 123 +++++++++++++++++++++++\n> > > 3 files changed, 351 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..19947a8d9015\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/v4l2_stats.cpp\n> > > @@ -0,0 +1,226 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2026, Ideas On Board\n> > > + *\n> > > + * V4L2 Statistics\n> >\n> > \"V4L2 ISP Statistics\" would be more accurate.\n> >\n> > Same in the .h files.\n> >\n> > > + */\n> > > +\n> > > +#include \"v4l2_stats.h\"\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 blocks, represented as\n> > > + * a V4L2StatsBlock class instances.\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 V4L2StatsBlock\n> > > + * \\brief Helper class that represents an ISP statistics block\n> > > + *\n> > > + * Each ISP measurement block produces a set of statistics data whose memory\n> > > + * layout is defined by the kernel interface.\n> > > + *\n> > > + * This class represents an ISP statistics block entry. It is constructed\n> > > + * with a reference to the memory area where the statistics block is\n> > > + * stored in the statistics buffer. The template parameter represents\n> > > + * the underlying kernel-defined ISP statistics block type and allows its\n> > > + * user to easily cast it to said type to read the statistics data.\n> > > + *\n> > > + * \\sa V4L2Stats::block()\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn V4L2StatsBlock::V4L2StatsBlock()\n> > > + * \\brief Construct a V4L2StatsBlock with memory represented by \\a data\n> > > + * \\param[in] data The memory area where the statistics block is located\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn V4L2StatsBlock::operator->() const\n> > > + * \\brief Access the statistics block casting it to the kernel-defined\n> > > + * ISP block type\n> > > + *\n> > > + * The V4L2StatsBlock is templated with the kernel defined ISP block type. This\n> > > + * function allows users to easily cast a V4L2StatsBlock to the underlying\n> > > + * kernel-defined type in order to easily access the statistics data.\n> > > + *\n> > > + * \\code{.cpp}\n> > > + *\n> > > + * // The kernel header defines the ISP statistics types, for example\n> > > + * // struct my_isp_awb_stats_data {\n> > > + * //\t\tu16 mean_r;\n> > > + * //\t\tu16 mean_g;\n> > > + * //\t\tu16 mean_b;\n> > > + * // }\n> > > + *\n> > > + * template<> V4L2StatsBlock awbStats = ...\n> > > + *\n> > > + * u16 mean_r = awbStats->mean_r;\n> > > + * u16 mean_g = awbStats->mean_g;\n> > > + * u16 mean_b = awbStats->mean_b;\n> > > + *\n> > > + * \\endcode\n> > > + *\n> > > + * Users of this class shall not create a V4L2StatsBlock manually but should\n> > > + * use V4L2Stats::block().\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn V4L2StatsBlock::operator*() const\n> > > + * \\copydoc V4L2StatsBlock::operator->() const\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn V4L2StatsBlock::size() const\n> > > + * \\brief Retrieve the size (in bytes) of the ISP statistics block, including\n> > > + * the block's header\n> > > + * \\return The size of the stats block\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var V4L2StatsBlock::data_\n> > > + * \\brief Memory area where ISP statistics have been serialized\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\n> > > + * class MyISPStats : public V4L2Stats\n> > > + * {\n> > > + * public:\n> > > + * \tMyISPStats::MyISPStats(Span data)\n> > > + * \t\t: V4L2Stats(data)\n> > > + * \t{\n> > > + * \t}\n> > > + * };\n> > > + *\n> > > + * \\endcode\n> > > + *\n> > > + * Users of this class can then easily access an ISP statistics block as a\n> > > + * V4L2StatsBlock instance.\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::bytesused()\n> > > + * \\brief Retrieve the size of the statistics buffer (in bytes)\n> > > + * \\return The number of bytes occupied by the ISP statistics\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn V4L2Stats::block() const\n> > > + * \\brief Retrieve the location of an ISP statistics block\n> > > + * \\return A V4L2StatsBlock instance that points to the ISP statistics block\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn V4L2Stats::block(unsigned int blockType, size_t blockSize) const\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> > > + * Parse the buffer of serialized statistics data and retrieve the location\n> > > + * of the block with type \\a blockType of size \\a blockSize.\n> > > + *\n> > > + * IPA modules that derive the V4L2Stats class shall use this function to\n> > > + * retrieve the memory area that will be used to construct a V4L2StatsBlock\n> > > + * before returning it to the caller.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var V4L2Stats::data_\n> > > + * \\brief The ISP statistics buffer memory\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var V4L2Stats::used_\n> > > + * \\brief The number of bytes used by the statistics buffer\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..b3b3ffca185d\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/v4l2_stats.h\n> > > @@ -0,0 +1,123 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2026, Ideas On Board\n> > > + *\n> > > + * V4L2 Stats\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa {\n> > > +\n> > > +LOG_DECLARE_CATEGORY(V4L2Stats)\n> > > +\n> > > +template\n> > > +class V4L2StatsBlock\n> > > +{\n> > > +public:\n> > > +\tV4L2StatsBlock(const Span data)\n> > > +\t\t: data_(data)\n> > > +\t{\n> > > +\t}\n> > > +\n> > > +\t~V4L2StatsBlock() {}\n> >\n> > Is this needed ?\n> \n> Probably not\n> \n> > > +\n> > > +\tconst T *operator->() const\n> > > +\t{\n> > > +\t\treturn reinterpret_cast(data_.data());\n> > > +\t}\n> > > +\n> > > +\tconst T &operator*() const\n> > > +\t{\n> > > +\t\treturn *reinterpret_cast(data_.data());\n> > > +\t}\n> > > +\n> > > +\tsize_t size() const\n> > > +\t{\n> > > +\t\treturn data_.size();\n> > > +\t}\n> >\n> > A function that returns if the block is valid would be useful. This can\n> > be queried with size(), but that's not self-explicit.\n> >\n> > > +\n> > > +protected:\n> > > +\tSpan data_;\n> > > +};\n> > > +\n> > > +template\n> > > +class V4L2Stats\n> > > +{\n> > > +public:\n> > > +\tV4L2Stats(Span data, unsigned int version)\n> > > +\t\t: data_(data)\n> > > +\t{\n> > > +\t\tstruct v4l2_isp_buffer *stats =\n> >\n> > const\n> >\n> > > +\t\t\treinterpret_cast(data_.data());\n> > > +\t\tused_ = stats->data_size;\n> > > +\n> > > +\t\tif (version != stats->version)\n> > > +\t\t\tLOG(V4L2Stats, Error)\n> > > +\t\t\t\t<< \"Unsupported v4l2-isp version: \" << stats->version;\n> >\n> > It would be good to also validate that stats->data_size fits in\n> > data.size().\n> \n> The kernel should validate that, as a driver using the v4l2-isp helper\n> from\n> https://patchwork.linuxtv.org/project/linux-media/patch/20260505-extensible-stats-v1-6-e16f326b8dad@ideasonboard.com/\n> shouldn't be able to populate stat blocks past the end of the buffer\n> size.\n\nThat's correct, but it would still be nice to avoid a crash in case the\nbuffer is not correctly populated.\n\n> > When validation fails, the V4L2Stats instance will be invalid. There\n> > should be a way for users to see that.\n> >\n> > None of those checks depend on Traits or on member variables, so you\n> > could move them to a separate non-template base class and avoid inlining\n> > them. See the series I've just sent for V4L2Params for an example.\n> \n> I'll have a look\n> \n> > > +\t}\n> > > +\n> > > +\tsize_t bytesused() const { return used_; }\n> >\n> > I don't see this being used in your RPP-X1 IPA code. What's the\n> > envisioned use case ?\n> \n> possibily a leftover from V4L2Params\n> \n> > > +\n> > > +\ttemplate\n> > > +\tauto block() 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 = block(kernelId, sizeof(Type));\n> > > +\t\treturn V4L2StatsBlock(data);\n> > > +\t}\n> > > +\n> > > +protected:\n> > > +\tconst Span block(unsigned int blockType, size_t blockSize) const\n> > > +\t{\n> > > +\t\tstruct v4l2_isp_buffer *stats =\n> > > +\t\t\treinterpret_cast(data_.data());\n> > > +\n> > > +\t\t__u8 *data = stats->data;\n> > > +\t\twhile (data < stats->data + stats->data_size) {\n> > > +\t\t\tstruct v4l2_isp_block_header *header =\n> > > +\t\t\t\treinterpret_cast(data);\n> >\n> > const\n> >\n> > > +\n> > > +\t\t\tif (header->type != blockType) {\n> > > +\t\t\t\tdata += header->size;\n> >\n> > There's a risk of integer overflow.\n> \n> Am not I just advancing the data pointer here ?\n\nIf header->size is very large the data pointer will move back and point\nto random memory.\n\n> > > +\t\t\t\tcontinue;\n> > > +\t\t\t}\n> > > +\n> > > +\t\t\tif (header->size != blockSize) {\n> > > +\t\t\t\tLOG(V4L2Stats, Error)\n> > > +\t\t\t\t\t<< \"Block type \" << blockType\n> > > +\t\t\t\t\t<< \" size mistmatch: expected \"\n> > > +\t\t\t\t\t<< blockSize << \" got:\"\n> > > +\t\t\t\t\t<< header->size;\n> > > +\t\t\t\treturn {};\n> > > +\t\t\t}\n> > > +\n> > > +\t\t\treturn Span(data, header->size);\n> >\n> > There's a risk of going beyond the end of the buffer.\n> \n> Should I just check that 'data + header->size <= stats->data +\n> stats->data_size' ?\n\nWith appropriate integer overflow handling :-)\n\n> > > +\t\t}\n> >\n> > I would expect that in a typical use case the algorithms will use all\n> > available blocks, and different algorithms would use the same block.\n> \n> This currently doesn't happen, right ? However it is certainly\n> possible\n> \n> > Have you considered introducing a caching mechanism like in V4L2Params ?\n> \n> I did. But if the cache is a class member I lose all the cost-ness as\n> creating a V4L2StatsBlock would require to modify the V4L2Stats state\n> to update the cache.\n> \n> > You could even populate the cache in the constructor by parsing the\n> > whole buffer.\n> \n> Honestly, I thought this was an overkill as currently stats blocks are\n> accessed once per frame.\n> \n> But it's true that every access would require re-parsing the buffer to\n> find the right block, so it might be more efficient to just do that\n> once at class creation time. Once we have indexes stored in the cached\n> looking up a block would simply be a matter of adding the offset to\n> the stats->data[] pointer ?\n\nSomething like that, yes.\n\nParams need to be constructed at runtime, but stats are not mutable, so\nI think parsing them in one go would be a good idea.\n\n> > > +\n> > > +\t\tLOG(V4L2Stats, Error) << \"Unsupported stats block type: \"\n> > > +\t\t\t\t << blockType;\n> > > +\n> > > +\t\treturn {};\n> > > +\t}\n> > > +\n> > > +\tSpan data_;\n> > > +\tsize_t used_;\n> > > +};\n> > > +\n> > > +} /* namespace ipa */\n> > > +\n> > > +} /* namespace libcamera */","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 BFA57BDCBD\n\tfor ;\n\tFri, 8 May 2026 09:49:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7AE6A62FE8;\n\tFri, 8 May 2026 11:49: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 0600562E6A\n\tfor ;\n\tFri, 8 May 2026 11:49:55 +0200 (CEST)","from killaraus.ideasonboard.com\n\t(2001-14ba-70f3-e800--a06.rev.dnainternet.fi\n\t[IPv6:2001:14ba:70f3:e800::a06])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 30502BCA;\n\tFri, 8 May 2026 11:49:50 +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=\"Xanf/zvs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1778233790;\n\tbh=3f62wowH3Ma8XvmaeXJGv6MOQgziaZXfa8ucR9xZHzM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Xanf/zvsBE+xfFwTsfaOXsLFm2gZX0jn+P6fb3F0UB+vTOJrRI8TfyLSKHTkjiiU7\n\tlNoQ3LbEEKKyEwEamiSqgPLs/9WSn2sX25tLXpfZqkcMwHNuH2CY1a17tvQ2FQDbHu\n\txOc4jo6o5ObfL57KqilZ1mZpJu7ApUYeKq6nPlew=","Date":"Fri, 8 May 2026 12:49:53 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tAntoine Bouyer ,\n\tKieran Bingham ","Subject":"Re: [PATCH v2 2/3] ipa: libipa: Introduce V4L2Stats","Message-ID":"<20260508094953.GA2186850@killaraus.ideasonboard.com>","References":"<20260507-extensible-stats-v2-0-dfca939ad327@ideasonboard.com>\n\t<20260507-extensible-stats-v2-2-dfca939ad327@ideasonboard.com>\n\t<20260507185212.GA2041714@killaraus.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":38821,"web_url":"https://patchwork.libcamera.org/comment/38821/","msgid":"<20260508115228.GG2176058@killaraus.ideasonboard.com>","date":"2026-05-08T11:52:28","subject":"Re: [PATCH v2 2/3] ipa: libipa: Introduce V4L2Stats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, May 08, 2026 at 11:32:56AM +0200, Jacopo Mondi wrote:\n> On Fri, May 08, 2026 at 11:22:10AM +0200, Barnabás Pőcze wrote:\n> > 2026. 05. 08. 11:01 keltezéssel, Jacopo Mondi írta:\n> > > On Fri, May 08, 2026 at 09:11:07AM +0200, Barnabás Pőcze wrote:\n> > > > 2026. 05. 07. 20:52 keltezéssel, Laurent Pinchart írta:\n> > > > > On Thu, May 07, 2026 at 12:07:55PM +0200, Jacopo Mondi wrote:\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 | 226 ++++++++++++++++++++++++++++++++++++++++++\n> > > > > > src/ipa/libipa/v4l2_stats.h | 123 +++++++++++++++++++++++\n> > > > > > 3 files changed, 351 insertions(+)\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..b3b3ffca185d\n> > > > > > --- /dev/null\n> > > > > > +++ b/src/ipa/libipa/v4l2_stats.h\n> > > > > > @@ -0,0 +1,123 @@\n> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > +/*\n> > > > > > + * Copyright (C) 2026, Ideas On Board\n> > > > > > + *\n> > > > > > + * V4L2 Stats\n> > > > > > + */\n> > > > > > +\n> > > > > > +#pragma once\n> > > > > > +\n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +\n> > > > > > +namespace libcamera {\n> > > > > > +\n> > > > > > +namespace ipa {\n> > > > > > +\n> > > > > > +LOG_DECLARE_CATEGORY(V4L2Stats)\n> > > > > > +\n> > > > > > +template\n> > > > > > +class V4L2StatsBlock\n> > > > > > +{\n> > > > > > +public:\n> > > > > > +\tV4L2StatsBlock(const Span data)\n> > > >\n> > > > The top-level `const` can be dropped here.\n> > > >\n> > > > > > +\t\t: data_(data)\n> > > > > > +\t{\n> > > > > > +\t}\n> > > > > > +\n> > > > > > +\t~V4L2StatsBlock() {}\n> > > > >\n> > > > > Is this needed ?\n> > > > >\n> > > > > > +\n> > > > > > +\tconst T *operator->() const\n> > > > > > +\t{\n> > > > > > +\t\treturn reinterpret_cast(data_.data());\n> > > > > > +\t}\n> > > > > > +\n> > > > > > +\tconst T &operator*() const\n> > > > > > +\t{\n> > > > > > +\t\treturn *reinterpret_cast(data_.data());\n> > > > > > +\t}\n> > > > > > +\n> > > > > > +\tsize_t size() const\n> > > > > > +\t{\n> > > > > > +\t\treturn data_.size();\n> > > > > > +\t}\n> > > > >\n> > > > > A function that returns if the block is valid would be useful. This can\n> > > > > be queried with size(), but that's not self-explicit.\n> > > >\n> > > > Earlier I suggested just returning `const T *` instead of `V4L2StatsBlock`\n> > > > from the `block()` function, I think that satisfies this requirement easily,\n> > > > and I still think that would be a net simplification here.\n> > >\n> > > As said, none of the currently mainlined ISP drivers use extensible\n> > > stats. If one of them gets ported to to use them we'll have to handle\n> > > two formats as we currently do for RkISP1 parameters and\n> > > V4L2StatsBlock would be the place where we'll handle the differences\n> > > between a \"legacy\" format and an extensible one (possibily by simply\n> > > discarding the header before returning the memory location of a stats\n> > > block).\n> > >\n> > > The cost of abstraction is cheap but if there is consensus on\n> > > deferring the introduction of V4L2StatsBlock to when we'll have an\n> > > existing ISP ported to use extensible stats, I'll do so.\n> >\n> > Okay, fair enough. It's a small part, so either way it shouldn't be too\n> > difficult to change when old format support is implemented.\n> \n> What if we keep the abstraction for now and I add a bool() operator\n> that checks if the block is empty ?\n\nOr return an std::optional<> from block()\n\n> > > > > > +\n> > > > > > +protected:\n> > > > > > +\tSpan data_;\n> > > > > > +};\n> > > > > > +\n> > > > > > +template\n> > > > > > +class V4L2Stats\n> > > > > > +{\n> > > > > > +public:\n> > > > > > +\tV4L2Stats(Span data, unsigned int version)\n> > > > > > +\t\t: data_(data)\n> > > > > > +\t{\n> > > > > > +\t\tstruct v4l2_isp_buffer *stats =\n> > > > >\n> > > > > const\n> > > > >\n> > > > > > +\t\t\treinterpret_cast(data_.data());\n> > > > > > +\t\tused_ = stats->data_size;\n> > > > > > +\n> > > > > > +\t\tif (version != stats->version)\n> > > > > > +\t\t\tLOG(V4L2Stats, Error)\n> > > > > > +\t\t\t\t<< \"Unsupported v4l2-isp version: \" << stats->version;\n> > > > >\n> > > > > It would be good to also validate that stats->data_size fits in\n> > > > > data.size().\n> > > > >\n> > > > > When validation fails, the V4L2Stats instance will be invalid. There\n> > > > > should be a way for users to see that.\n> > > >\n> > > > If all blocks are parsed when constructed, then adding something like\n> > > >\n> > > > static std::optional from(Span);\n> > > >\n> > > > should work for this purpose?\n> > >\n> > > Are you suggesting this a static method of V4L2Stats to replace the\n> > > constructor ?\n> >\n> > It was just a suggestion, if we want to validate the buffer and populate\n> > an (id -> span) mapping from the content, then I think a static factory\n> > function + private constructor could be one way to do it.\n> \n> Indeed this could be a way to ensure a V4L2Stat has been constructed\n> correctly and abort earlier if there are problems. I'll give\n> V4L2Stats::from() a spin and see how it looks like.\n> \n> > > > > None of those checks depend on Traits or on member variables, so you\n> > > > > could move them to a separate non-template base class and avoid inlining\n> > > > > them. See the series I've just sent for V4L2Params for an example.\n> > > > >\n> > > > > > +\t}\n> > > > > > +\n> > > > > > +\tsize_t bytesused() const { return used_; }\n> > > > >\n> > > > > I don't see this being used in your RPP-X1 IPA code. What's the\n> > > > > envisioned use case ?\n> > > > >\n> > > > > > +\n> > > > > > +\ttemplate\n> > > > > > +\tauto block() 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 = block(kernelId, sizeof(Type));\n> > > > > > +\t\treturn V4L2StatsBlock(data);\n> > > > > > +\t}\n> > > > > > +\n> > > > > > +protected:\n> > > > > > +\tconst Span block(unsigned int blockType, size_t blockSize) const\n> > > > > > +\t{\n> > > > > > +\t\tstruct v4l2_isp_buffer *stats =\n> > > > > > +\t\t\treinterpret_cast(data_.data());\n> > > > > > +\n> > > > > > +\t\t__u8 *data = stats->data;\n> > > > > > +\t\twhile (data < stats->data + stats->data_size) {\n> > > > > > +\t\t\tstruct v4l2_isp_block_header *header =\n> > > > > > +\t\t\t\treinterpret_cast(data);\n> > > > >\n> > > > > const\n> > > > >\n> > > > > > +\n> > > > > > +\t\t\tif (header->type != blockType) {\n> > > > > > +\t\t\t\tdata += header->size;\n> > > > >\n> > > > > There's a risk of integer overflow.\n> > > > >\n> > > > > > +\t\t\t\tcontinue;\n> > > > > > +\t\t\t}\n> > > > > > +\n> > > > > > +\t\t\tif (header->size != blockSize) {\n> > > > > > +\t\t\t\tLOG(V4L2Stats, Error)\n> > > > > > +\t\t\t\t\t<< \"Block type \" << blockType\n> > > > > > +\t\t\t\t\t<< \" size mistmatch: expected \"\n> > > > > > +\t\t\t\t\t<< blockSize << \" got:\"\n> > > > > > +\t\t\t\t\t<< header->size;\n> > > > > > +\t\t\t\treturn {};\n> > > > > > +\t\t\t}\n> > > > > > +\n> > > > > > +\t\t\treturn Span(data, header->size);\n> > > > >\n> > > > > There's a risk of going beyond the end of the buffer.\n> > > > >\n> > > > > > +\t\t}\n> > > > >\n> > > > > I would expect that in a typical use case the algorithms will use all\n> > > > > available blocks, and different algorithms would use the same block.\n> > > >\n> > > > Isn't it more likely that if they need to, they will use data from the other algorithms \"frame context\"?\n> > > >\n> > > > > Have you considered introducing a caching mechanism like in V4L2Params ?\n> > > > > You could even populate the cache in the constructor by parsing the\n> > > > > whole buffer.\n> > > > >\n> > > > > > +\n> > > > > > +\t\tLOG(V4L2Stats, Error) << \"Unsupported stats block type: \"\n> > > > > > +\t\t\t\t << blockType;\n> > > > > > +\n> > > > > > +\t\treturn {};\n> > > > > > +\t}\n> > > > > > +\n> > > > > > +\tSpan data_;\n> > > > > > +\tsize_t used_;\n> > > > > > +};\n> > > > > > +\n> > > > > > +} /* namespace ipa */\n> > > > > > +\n> > > > > > +} /* namespace libcamera */","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 80B44BDCBD\n\tfor ;\n\tFri, 8 May 2026 11:52:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C7BBD62FEA;\n\tFri, 8 May 2026 13:52:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 197D962FD3\n\tfor ;\n\tFri, 8 May 2026 13:52:30 +0200 (CEST)","from killaraus.ideasonboard.com\n\t(2001-14ba-70f3-e800--a06.rev.dnainternet.fi\n\t[IPv6:2001:14ba:70f3:e800::a06])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ECEA5BCA;\n\tFri, 8 May 2026 13:52:24 +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=\"bkF89cDk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1778241145;\n\tbh=Kt+8zeiPhSHO+uJ6wWUKm/lIJqses3viXoocCwsZsJw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bkF89cDk9RsCHF6HjDcc+LuCQk5oNd8aXeSX8TNh/L2SgzkrU6hpo9tUIXH3wyaxJ\n\t89q8Jcwi9H40h/Z9rkksjkU/6+vwMxWQmCDPpprjP/UKRQk2B1apPDJA0vUM2c5PyF\n\tGiMkii6srauR4YtjUpgED2hZxcpwpq2L3L3lBspU=","Date":"Fri, 8 May 2026 14:52:28 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org, Antoine Bouyer\n\t, Kieran Bingham\n\t","Subject":"Re: [PATCH v2 2/3] ipa: libipa: Introduce V4L2Stats","Message-ID":"<20260508115228.GG2176058@killaraus.ideasonboard.com>","References":"<20260507-extensible-stats-v2-0-dfca939ad327@ideasonboard.com>\n\t<20260507-extensible-stats-v2-2-dfca939ad327@ideasonboard.com>\n\t<20260507185212.GA2041714@killaraus.ideasonboard.com>\n\t<31916af4-32a0-4648-ae60-fe203cf18ea5@ideasonboard.com>\n\t\n\t<8161dbe9-3e30-45fd-9de2-a31bca78e0db@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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\" "}}]