[{"id":39584,"web_url":"https://patchwork.libcamera.org/comment/39584/","msgid":"<178332036612.292172.1177269008649855565@neptunite.rasen.tech>","date":"2026-07-06T06:46:06","subject":"Re: [PATCH v4 2/2] ipa: libipa: Introduce V4L2Stats","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nThanks for the patch.\n\nQuoting Jacopo Mondi (2026-05-12 22:45:06)\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 | 243 ++++++++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/v4l2_stats.h | 67 ++++++++++++\n> 3 files changed, 312 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..7aa2add2f265\n> --- /dev/null\n> +++ b/src/ipa/libipa/v4l2_stats.cpp\n> @@ -0,0 +1,243 @@\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 a block type to\n> + * the memory location of a 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> + : data_(data), valid_(false)\n> +{\n> + const struct v4l2_isp_buffer *stats =\n> + reinterpret_cast(data_.data());\n> +\n> + if (data_.size() - sizeof(*stats) < stats->data_size) {\n> + LOG(V4L2Stats, Error)\n> + << \"Stats buffer size mismatch: \" << stats->data_size;\n> + return;\n> + }\n> +\n> + if (version != stats->version) {\n> + LOG(V4L2Stats, Error)\n> + << \"Unsupported v4l2-isp version: \" << stats->version;\n> + return;\n> + }\n> +\n> + /* Construct the cache for easier lookup. */\n> + size_t left = stats->data_size;\n> + const __u8 *d = stats->data;\n> +\n> + while (left > 0) {\n> + const struct v4l2_isp_block_header *header =\n> + reinterpret_cast(d);\n> +\n> + if (left < sizeof(*header) || header->size < sizeof(*header)) {\n> + LOG(V4L2Stats, Error)\n> + << \"Block type \" << header->type\n> + << \" size is not valid\";\n> + return;\n> + }\n> +\n> + if (left < header->size) {\n> + LOG(V4L2Stats, Error)\n> + << \"Not enough space for block type \" << header->type;\n> + return;\n> + }\n> +\n> + auto [it, inserted] = cache_.try_emplace(header->type, d, header->size);\n> + if (!inserted) {\n> + LOG(V4L2Stats, Error)\n> + << \"Duplicated block type \" << header->type;\n> + return;\n> + }\n> +\n> + d += header->size;\n> + left -= header->size;\n> + }\n> +\n> + valid_ = true;\n> +}\n> +\n> +/**\n> + * \\brief Retrieve an ISP statistics block a return a reference to it\n\ns/a return/and return/ ?\n\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> + const auto it = cache_.find(blockType);\n> + if (it == cache_.end()) {\n> + LOG(V4L2Stats, Error) << \"Unsupported stats block type: \"\n> + << blockType;\n> + return {};\n> + }\n> +\n> + const struct v4l2_isp_block_header *header =\n> + reinterpret_cast(it->second.data());\n> + if (header->size != blockSize) {\n> + LOG(V4L2Stats, Error)\n> + << \"Block type \" << blockType\n> + << \" size mistmatch: expected \"\n> + << blockSize << \" got:\"\n> + << header->size;\n> + return {};\n> + }\n> +\n> + return 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> + * \\class V4L2Stats\n> + * \\brief Helper class that represent an ISP statistics buffer\n\ns/represent/represents/\n\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> + * Agc,\n> + * Awb,\n> + * ...\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> + * using type = struct my_isp_kernel_stats_type_agc;\n> + * static constexpr kernel_enum_type blockType = MY_ISP_STATS_TYPE_AGC;\n> + * };\n> + *\n> + * template<>\n> + * struct block_type {\n> + * using type = struct my_isp_kernel_stats_type_awb;\n> + * static constexpr kernel_enum_type blockType = MY_ISP_STATS_TYPE_AWB;\n> + * };\n> + *\n> + * // Convenience type to associate a block id to the 'block_type' overload\n\nI realize that v4l2_params also has this but imo \"convenience\" makes this\nsounds optional, but I guess everybody just copies the reference implementation\nanyway so it hasn't been a problem...\n\n> + * struct stats_traits {\n> + * using id_type = myISPStats;\n> + * template 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> + * MyISPStats::MyISPStats(Span data)\n> + * : V4L2Stats(data, V4L2_ISP_VERSION_V1)\n> + * {\n> + * }\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> + *\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 or nullptr if the block is\n> + * not present in 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..b96395e5f551\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> +public:\n> + V4L2StatsBase(Span data, unsigned int version);\n> +\n> + Span block(unsigned int blockType, size_t blockSize) const;\n> + constexpr explicit operator bool()\n> + {\n> + return valid_;\n> + }\n> +\n> +private:\n> + std::map> cache_;\n> + Span data_;\n> + bool valid_;\n> +};\n> +\n> +template\n> +class V4L2Stats : public V4L2StatsBase\n> +{\n> +public:\n> + static_assert(std::is_same_v, uint16_t>);\n> +\n> + V4L2Stats(Span data, unsigned int version)\n> + : V4L2StatsBase(data, version)\n> + {\n> + }\n> +\n> + template\n> + const typename Traits::template id_to_details::type *\n> + block() const\n> + {\n> + using Details = typename Traits::template id_to_details;\n> +\n> + using Type = typename Details::type;\n> + constexpr auto kernelId = Details::blockType;\n> +\n> + auto data = V4L2StatsBase::block(kernelId, sizeof(Type));\n> +\n> + return data.size() > 0 ?\n> + reinterpret_cast(data.data()) : nullptr;\n\nI was wondering why the examples that you linked to in the repo needed a block\nclass specialization but now I see here why there is not one in this version. I\nlike this more.\n\n\nLooks good to me!\n\nReviewed-by: Paul Elder \n\n> + }\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> \n> -- \n> 2.53.0\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 495A5C3304\n\tfor ;\n\tMon, 6 Jul 2026 06:46:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 33F1365FF1;\n\tMon, 6 Jul 2026 08:46:13 +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 C644965F7C\n\tfor ;\n\tMon, 6 Jul 2026 08:46:11 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:172d:69a4:1f4a:e2])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 0186E33D;\n\tMon, 6 Jul 2026 08:45:22 +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=\"GpJfHH1/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1783320323;\n\tbh=yd55MqyZe3/pT/K0hP8NUHITjGfqkZ92Ze1t+AxZjLo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=GpJfHH1/tUHSQ8SqEejb0uP/nXA3SZPIrbNt+DQo0Z0oAnEngIeBF1cYsson6/UZN\n\tNS9Cce7f8GkTkxxTPA/weftNO5iXEmw+igvLi7whYvCT/sRJWZtU/J5Zfn5n50lOZT\n\tzWCoJZ+C4k1ztIn+v5YycHvTWIazEcsh7ThPqVUY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20260512-extensible-stats-v4-2-e0425f6171e2@ideasonboard.com>","References":"<20260512-extensible-stats-v4-0-e0425f6171e2@ideasonboard.com>\n\t<20260512-extensible-stats-v4-2-e0425f6171e2@ideasonboard.com>","Subject":"Re: [PATCH v4 2/2] ipa: libipa: Introduce V4L2Stats","From":"Paul Elder ","Cc":"Jacopo Mondi ,\n\tKieran Bingham ","To":"Antoine Bouyer ,\n\tJacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 06 Jul 2026 15:46:06 +0900","Message-ID":"<178332036612.292172.1177269008649855565@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","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\" "}}]