| Message ID | 20260507-extensible-stats-v2-0-dfca939ad327@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
2026. 05. 07. 20:52 keltezéssel, Laurent Pinchart írta: > Hi Jacopo, > > Thank you for the patch. > > On Thu, May 07, 2026 at 12:07:55PM +0200, Jacopo Mondi wrote: >> Add a V4L2Stats class similar in spirit to the existing V4L2Params >> class to allow IPA modules to easily sub-class it to access ISP >> statistics blocks serialized into a v4l2_isp_buffer. >> >> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/ipa/libipa/meson.build | 2 + >> src/ipa/libipa/v4l2_stats.cpp | 226 ++++++++++++++++++++++++++++++++++++++++++ >> src/ipa/libipa/v4l2_stats.h | 123 +++++++++++++++++++++++ >> 3 files changed, 351 insertions(+) >> > [...] >> diff --git a/src/ipa/libipa/v4l2_stats.h b/src/ipa/libipa/v4l2_stats.h >> new file mode 100644 >> index 000000000000..b3b3ffca185d >> --- /dev/null >> +++ b/src/ipa/libipa/v4l2_stats.h >> @@ -0,0 +1,123 @@ >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> +/* >> + * Copyright (C) 2026, Ideas On Board >> + * >> + * V4L2 Stats >> + */ >> + >> +#pragma once >> + >> +#include <stdint.h> >> + >> +#include <linux/media/v4l2-isp.h> >> + >> +#include <libcamera/base/log.h> >> +#include <libcamera/base/span.h> >> + >> +namespace libcamera { >> + >> +namespace ipa { >> + >> +LOG_DECLARE_CATEGORY(V4L2Stats) >> + >> +template<typename T> >> +class V4L2StatsBlock >> +{ >> +public: >> + V4L2StatsBlock(const Span<const uint8_t> data) The top-level `const` can be dropped here. >> + : data_(data) >> + { >> + } >> + >> + ~V4L2StatsBlock() {} > > Is this needed ? > >> + >> + const T *operator->() const >> + { >> + return reinterpret_cast<const T *>(data_.data()); >> + } >> + >> + const T &operator*() const >> + { >> + return *reinterpret_cast<const T *>(data_.data()); >> + } >> + >> + size_t size() const >> + { >> + return data_.size(); >> + } > > A function that returns if the block is valid would be useful. This can > be queried with size(), but that's not self-explicit. Earlier I suggested just returning `const T *` instead of `V4L2StatsBlock<T>` from the `block()` function, I think that satisfies this requirement easily, and I still think that would be a net simplification here. > >> + >> +protected: >> + Span<const uint8_t> data_; >> +}; >> + >> +template<typename Traits> >> +class V4L2Stats >> +{ >> +public: >> + V4L2Stats(Span<uint8_t> data, unsigned int version) >> + : data_(data) >> + { >> + struct v4l2_isp_buffer *stats = > > const > >> + reinterpret_cast<struct v4l2_isp_buffer *>(data_.data()); >> + used_ = stats->data_size; >> + >> + if (version != stats->version) >> + LOG(V4L2Stats, Error) >> + << "Unsupported v4l2-isp version: " << stats->version; > > It would be good to also validate that stats->data_size fits in > data.size(). > > When validation fails, the V4L2Stats instance will be invalid. There > should be a way for users to see that. If all blocks are parsed when constructed, then adding something like static std::optional<V4L2Stats> from(Span<const uint8_t>); should work for this purpose? > > None of those checks depend on Traits or on member variables, so you > could move them to a separate non-template base class and avoid inlining > them. See the series I've just sent for V4L2Params for an example. > >> + } >> + >> + size_t bytesused() const { return used_; } > > I don't see this being used in your RPP-X1 IPA code. What's the > envisioned use case ? > >> + >> + template<typename Traits::id_type Id> >> + auto block() const >> + { >> + using Details = typename Traits::template id_to_details<Id>; >> + >> + using Type = typename Details::type; >> + constexpr auto kernelId = Details::blockType; >> + >> + auto data = block(kernelId, sizeof(Type)); >> + return V4L2StatsBlock<Type>(data); >> + } >> + >> +protected: >> + const Span<const uint8_t> block(unsigned int blockType, size_t blockSize) const >> + { >> + struct v4l2_isp_buffer *stats = >> + reinterpret_cast<struct v4l2_isp_buffer *>(data_.data()); >> + >> + __u8 *data = stats->data; >> + while (data < stats->data + stats->data_size) { >> + struct v4l2_isp_block_header *header = >> + reinterpret_cast<struct v4l2_isp_block_header *>(data); > > const > >> + >> + if (header->type != blockType) { >> + data += header->size; > > There's a risk of integer overflow. > >> + continue; >> + } >> + >> + if (header->size != blockSize) { >> + LOG(V4L2Stats, Error) >> + << "Block type " << blockType >> + << " size mistmatch: expected " >> + << blockSize << " got:" >> + << header->size; >> + return {}; >> + } >> + >> + return Span<const uint8_t>(data, header->size); > > There's a risk of going beyond the end of the buffer. > >> + } > > I would expect that in a typical use case the algorithms will use all > available blocks, and different algorithms would use the same block. Isn't it more likely that if they need to, they will use data from the other algorithms "frame context"? > Have you considered introducing a caching mechanism like in V4L2Params ? > You could even populate the cache in the constructor by parsing the > whole buffer. > >> + >> + LOG(V4L2Stats, Error) << "Unsupported stats block type: " >> + << blockType; >> + >> + return {}; >> + } >> + >> + Span<uint8_t> data_; >> + size_t used_; >> +}; >> + >> +} /* namespace ipa */ >> + >> +} /* namespace libcamera */ >
Hi Laurent On Thu, May 07, 2026 at 09:52:12PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, May 07, 2026 at 12:07:55PM +0200, Jacopo Mondi wrote: > > Add a V4L2Stats class similar in spirit to the existing V4L2Params > > class to allow IPA modules to easily sub-class it to access ISP > > statistics blocks serialized into a v4l2_isp_buffer. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/ipa/libipa/meson.build | 2 + > > src/ipa/libipa/v4l2_stats.cpp | 226 ++++++++++++++++++++++++++++++++++++++++++ > > src/ipa/libipa/v4l2_stats.h | 123 +++++++++++++++++++++++ > > 3 files changed, 351 insertions(+) > > > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build > > index 963c5ee73063..16f4b095f220 100644 > > --- a/src/ipa/libipa/meson.build > > +++ b/src/ipa/libipa/meson.build > > @@ -19,6 +19,7 @@ libipa_headers = files([ > > 'pwl.h', > > 'quantized.h', > > 'v4l2_params.h', > > + 'v4l2_stats.h', > > ]) > > > > libipa_sources = files([ > > @@ -40,6 +41,7 @@ libipa_sources = files([ > > 'pwl.cpp', > > 'quantized.cpp', > > 'v4l2_params.cpp', > > + 'v4l2_stats.cpp', > > ]) > > > > libipa_includes = include_directories('..') > > diff --git a/src/ipa/libipa/v4l2_stats.cpp b/src/ipa/libipa/v4l2_stats.cpp > > new file mode 100644 > > index 000000000000..19947a8d9015 > > --- /dev/null > > +++ b/src/ipa/libipa/v4l2_stats.cpp > > @@ -0,0 +1,226 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2026, Ideas On Board > > + * > > + * V4L2 Statistics > > "V4L2 ISP Statistics" would be more accurate. > > Same in the .h files. > > > + */ > > + > > +#include "v4l2_stats.h" > > + > > +namespace libcamera { > > + > > +namespace ipa { > > + > > +LOG_DEFINE_CATEGORY(V4L2Stats) > > + > > +/** > > + * \file v4l2_stats.cpp > > + * \brief Helper class to handle an ISP statistics buffer compatible with > > + * the generic V4L2 ISP format > > + * > > + * The Linux kernel defines a generic buffer format for ISP statistics. > > + * The format describes a serialisation method that allows userspace to > > + * access statistics data from a binary buffer. > > + * > > + * The V4L2Stats class implements support for the V4L2 ISP statistics buffer > > + * format and allows users to retrieve an ISP statistics blocks, represented as > > + * a V4L2StatsBlock class instances. > > + * > > + * IPA implementations using these helpers should define an enumeration of ISP > > + * blocks supported by the IPA module and use a set of common abstractions to > > + * help their derived implementation of V4L2Stats translate the enumerated ISP > > + * block identifiers to the actual type of the statistics data as defined by > > + * the kernel interface. > > + */ > > + > > +/** > > + * \class V4L2StatsBlock > > + * \brief Helper class that represents an ISP statistics block > > + * > > + * Each ISP measurement block produces a set of statistics data whose memory > > + * layout is defined by the kernel interface. > > + * > > + * This class represents an ISP statistics block entry. It is constructed > > + * with a reference to the memory area where the statistics block is > > + * stored in the statistics buffer. The template parameter represents > > + * the underlying kernel-defined ISP statistics block type and allows its > > + * user to easily cast it to said type to read the statistics data. > > + * > > + * \sa V4L2Stats::block() > > + */ > > + > > +/** > > + * \fn V4L2StatsBlock::V4L2StatsBlock() > > + * \brief Construct a V4L2StatsBlock with memory represented by \a data > > + * \param[in] data The memory area where the statistics block is located > > + */ > > + > > +/** > > + * \fn V4L2StatsBlock::operator->() const > > + * \brief Access the statistics block casting it to the kernel-defined > > + * ISP block type > > + * > > + * The V4L2StatsBlock is templated with the kernel defined ISP block type. This > > + * function allows users to easily cast a V4L2StatsBlock to the underlying > > + * kernel-defined type in order to easily access the statistics data. > > + * > > + * \code{.cpp} > > + * > > + * // The kernel header defines the ISP statistics types, for example > > + * // struct my_isp_awb_stats_data { > > + * // u16 mean_r; > > + * // u16 mean_g; > > + * // u16 mean_b; > > + * // } > > + * > > + * template<> V4L2StatsBlock<struct my_isp_awb_stats_data> awbStats = ... > > + * > > + * u16 mean_r = awbStats->mean_r; > > + * u16 mean_g = awbStats->mean_g; > > + * u16 mean_b = awbStats->mean_b; > > + * > > + * \endcode > > + * > > + * Users of this class shall not create a V4L2StatsBlock manually but should > > + * use V4L2Stats::block(). > > + */ > > + > > +/** > > + * \fn V4L2StatsBlock::operator*() const > > + * \copydoc V4L2StatsBlock::operator->() const > > + */ > > + > > +/** > > + * \fn V4L2StatsBlock::size() const > > + * \brief Retrieve the size (in bytes) of the ISP statistics block, including > > + * the block's header > > + * \return The size of the stats block > > + */ > > + > > +/** > > + * \var V4L2StatsBlock::data_ > > + * \brief Memory area where ISP statistics have been serialized > > + */ > > + > > +/** > > + * \class V4L2Stats > > + * \brief Helper class that represent an ISP statistics buffer > > + * > > + * This class represents an ISP statistics buffer. It is constructed with a > > + * reference to the memory mapped buffer that has been dequeued from the ISP > > + * driver. > > + * > > + * This class is templated with the type of the enumeration of ISP blocks that > > + * each IPA module is expected to support. IPA modules are expected to derive > > + * this class by providing a 'stats_traits' type that helps the class associate > > + * a block type with the actual memory area that represents the ISP statistics > > + * block. > > + * > > + * \code{.cpp} > > + * > > + * // Define the supported ISP statistics blocks > > + * enum class myISPStats { > > + * Agc, > > + * Awb, > > + * ... > > + * }; > > + * > > + * // Maps the C++ enum type to the kernel enum type and concrete parameter type > > + * template<myISPStats B> > > + * struct block_type { > > + * }; > > + * > > + * template<> > > + * struct block_type<myISPStats::Agc> { > > + * using type = struct my_isp_kernel_stats_type_agc; > > + * static constexpr kernel_enum_type blockType = MY_ISP_STATS_TYPE_AGC; > > + * }; > > + * > > + * template<> > > + * struct block_type<myISPStats::Awb> { > > + * using type = struct my_isp_kernel_stats_type_awb; > > + * static constexpr kernel_enum_type blockType = MY_ISP_STATS_TYPE_AWB; > > + * }; > > + * > > + * > > + * // Convenience type to associate a block id to the 'block_type' overload > > + * struct stats_traits { > > + * using id_type = myISPStats; > > + * template<id_type Id> using id_to_details = block_type<Id>; > > + * }; > > + * > > + * ... > > + * > > + * // Derive the V4L2Stats class by providing stats_traits > > + * class MyISPStats : public V4L2Stats<stats_traits> > > + * { > > + * public: > > + * MyISPStats::MyISPStats(Span<uint8_t> data) > > + * : V4L2Stats(data) > > + * { > > + * } > > + * }; > > + * > > + * \endcode > > + * > > + * Users of this class can then easily access an ISP statistics block as a > > + * V4L2StatsBlock instance. > > + * > > + * \code{.cpp} > > + * > > + * MyISPStats stats(data); > > + * > > + * auto awb = stats.block<myISPStats::AWB>(); > > + * auto mean_r = awb->mean_r; > > + * auto mean_g = awb->mean_g; > > + * auto mean_b = awb->mean_b; > > + * \endcode > > + */ > > + > > +/** > > + * \fn V4L2Stats::V4L2Stats() > > + * \brief Construct an instance of V4L2Stats > > + * \param[in] data Reference to the v4l2-buffer memory mapped area > > + * \param[in] version The expected V4L2 ISP serialization format version > > + */ > > + > > +/** > > + * \fn V4L2Stats::bytesused() > > + * \brief Retrieve the size of the statistics buffer (in bytes) > > + * \return The number of bytes occupied by the ISP statistics > > + */ > > + > > +/** > > + * \fn V4L2Stats::block() const > > + * \brief Retrieve the location of an ISP statistics block > > + * \return A V4L2StatsBlock instance that points to the ISP statistics block > > + */ > > + > > +/** > > + * \fn V4L2Stats::block(unsigned int blockType, size_t blockSize) const > > + * \brief Retrieve an ISP statistics block a returns a reference to it > > + * \param[in] blockType The kernel-defined ISP block identifier, used to > > + * identify the block header > > + * \param[in] blockSize The ISP statistics block size, for validation > > + * > > + * Parse the buffer of serialized statistics data and retrieve the location > > + * of the block with type \a blockType of size \a blockSize. > > + * > > + * IPA modules that derive the V4L2Stats class shall use this function to > > + * retrieve the memory area that will be used to construct a V4L2StatsBlock<T> > > + * before returning it to the caller. > > + */ > > + > > +/** > > + * \var V4L2Stats::data_ > > + * \brief The ISP statistics buffer memory > > + */ > > + > > +/** > > + * \var V4L2Stats::used_ > > + * \brief The number of bytes used by the statistics buffer > > + */ > > + > > +} /* namespace ipa */ > > + > > +} /* namespace libcamera */ > > diff --git a/src/ipa/libipa/v4l2_stats.h b/src/ipa/libipa/v4l2_stats.h > > new file mode 100644 > > index 000000000000..b3b3ffca185d > > --- /dev/null > > +++ b/src/ipa/libipa/v4l2_stats.h > > @@ -0,0 +1,123 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2026, Ideas On Board > > + * > > + * V4L2 Stats > > + */ > > + > > +#pragma once > > + > > +#include <stdint.h> > > + > > +#include <linux/media/v4l2-isp.h> > > + > > +#include <libcamera/base/log.h> > > +#include <libcamera/base/span.h> > > + > > +namespace libcamera { > > + > > +namespace ipa { > > + > > +LOG_DECLARE_CATEGORY(V4L2Stats) > > + > > +template<typename T> > > +class V4L2StatsBlock > > +{ > > +public: > > + V4L2StatsBlock(const Span<const uint8_t> data) > > + : data_(data) > > + { > > + } > > + > > + ~V4L2StatsBlock() {} > > Is this needed ? > Probably not > > + > > + const T *operator->() const > > + { > > + return reinterpret_cast<const T *>(data_.data()); > > + } > > + > > + const T &operator*() const > > + { > > + return *reinterpret_cast<const T *>(data_.data()); > > + } > > + > > + size_t size() const > > + { > > + return data_.size(); > > + } > > A function that returns if the block is valid would be useful. This can > be queried with size(), but that's not self-explicit. > > > + > > +protected: > > + Span<const uint8_t> data_; > > +}; > > + > > +template<typename Traits> > > +class V4L2Stats > > +{ > > +public: > > + V4L2Stats(Span<uint8_t> data, unsigned int version) > > + : data_(data) > > + { > > + struct v4l2_isp_buffer *stats = > > const > > > + reinterpret_cast<struct v4l2_isp_buffer *>(data_.data()); > > + used_ = stats->data_size; > > + > > + if (version != stats->version) > > + LOG(V4L2Stats, Error) > > + << "Unsupported v4l2-isp version: " << stats->version; > > It would be good to also validate that stats->data_size fits in > data.size(). > The kernel should validate that, as a driver using the v4l2-isp helper from https://patchwork.linuxtv.org/project/linux-media/patch/20260505-extensible-stats-v1-6-e16f326b8dad@ideasonboard.com/ shouldn't be able to populate stat blocks past the end of the buffer size. > When validation fails, the V4L2Stats instance will be invalid. There > should be a way for users to see that. > > None of those checks depend on Traits or on member variables, so you > could move them to a separate non-template base class and avoid inlining > them. See the series I've just sent for V4L2Params for an example. > I'll have a look > > + } > > + > > + size_t bytesused() const { return used_; } > > I don't see this being used in your RPP-X1 IPA code. What's the > envisioned use case ? > possibily a leftover from V4L2Params > > + > > + template<typename Traits::id_type Id> > > + auto block() const > > + { > > + using Details = typename Traits::template id_to_details<Id>; > > + > > + using Type = typename Details::type; > > + constexpr auto kernelId = Details::blockType; > > + > > + auto data = block(kernelId, sizeof(Type)); > > + return V4L2StatsBlock<Type>(data); > > + } > > + > > +protected: > > + const Span<const uint8_t> block(unsigned int blockType, size_t blockSize) const > > + { > > + struct v4l2_isp_buffer *stats = > > + reinterpret_cast<struct v4l2_isp_buffer *>(data_.data()); > > + > > + __u8 *data = stats->data; > > + while (data < stats->data + stats->data_size) { > > + struct v4l2_isp_block_header *header = > > + reinterpret_cast<struct v4l2_isp_block_header *>(data); > > const > > > + > > + if (header->type != blockType) { > > + data += header->size; > > There's a risk of integer overflow. Am not I just advancing the data pointer here ? > > > + continue; > > + } > > + > > + if (header->size != blockSize) { > > + LOG(V4L2Stats, Error) > > + << "Block type " << blockType > > + << " size mistmatch: expected " > > + << blockSize << " got:" > > + << header->size; > > + return {}; > > + } > > + > > + return Span<const uint8_t>(data, header->size); > > There's a risk of going beyond the end of the buffer. Should I just check that 'data + header->size <= stats->data + stats->data_size' ? > > > + } > > I would expect that in a typical use case the algorithms will use all > available blocks, and different algorithms would use the same block. This currently doesn't happen, right ? However it is certainly possible > Have you considered introducing a caching mechanism like in V4L2Params ? I did. But if the cache is a class member I lose all the cost-ness as creating a V4L2StatsBlock would require to modify the V4L2Stats state to update the cache. > You could even populate the cache in the constructor by parsing the > whole buffer. Honestly, I thought this was an overkill as currently stats blocks are accessed once per frame. But it's true that every access would require re-parsing the buffer to find the right block, so it might be more efficient to just do that once at class creation time. Once we have indexes stored in the cached looking up a block would simply be a matter of adding the offset to the stats->data[] pointer ? > > > + > > + LOG(V4L2Stats, Error) << "Unsupported stats block type: " > > + << blockType; > > + > > + return {}; > > + } > > + > > + Span<uint8_t> data_; > > + size_t used_; > > +}; > > + > > +} /* namespace ipa */ > > + > > +} /* namespace libcamera */ > > -- > Regards, > > Laurent Pinchart
Hi Barnabás On Fri, May 08, 2026 at 09:11:07AM +0200, Barnabás Pőcze wrote: > 2026. 05. 07. 20:52 keltezéssel, Laurent Pinchart írta: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Thu, May 07, 2026 at 12:07:55PM +0200, Jacopo Mondi wrote: > > > Add a V4L2Stats class similar in spirit to the existing V4L2Params > > > class to allow IPA modules to easily sub-class it to access ISP > > > statistics blocks serialized into a v4l2_isp_buffer. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > > src/ipa/libipa/meson.build | 2 + > > > src/ipa/libipa/v4l2_stats.cpp | 226 ++++++++++++++++++++++++++++++++++++++++++ > > > src/ipa/libipa/v4l2_stats.h | 123 +++++++++++++++++++++++ > > > 3 files changed, 351 insertions(+) > > > > > [...] > > > diff --git a/src/ipa/libipa/v4l2_stats.h b/src/ipa/libipa/v4l2_stats.h > > > new file mode 100644 > > > index 000000000000..b3b3ffca185d > > > --- /dev/null > > > +++ b/src/ipa/libipa/v4l2_stats.h > > > @@ -0,0 +1,123 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2026, Ideas On Board > > > + * > > > + * V4L2 Stats > > > + */ > > > + > > > +#pragma once > > > + > > > +#include <stdint.h> > > > + > > > +#include <linux/media/v4l2-isp.h> > > > + > > > +#include <libcamera/base/log.h> > > > +#include <libcamera/base/span.h> > > > + > > > +namespace libcamera { > > > + > > > +namespace ipa { > > > + > > > +LOG_DECLARE_CATEGORY(V4L2Stats) > > > + > > > +template<typename T> > > > +class V4L2StatsBlock > > > +{ > > > +public: > > > + V4L2StatsBlock(const Span<const uint8_t> data) > > The top-level `const` can be dropped here. > > > > > + : data_(data) > > > + { > > > + } > > > + > > > + ~V4L2StatsBlock() {} > > > > Is this needed ? > > > > > + > > > + const T *operator->() const > > > + { > > > + return reinterpret_cast<const T *>(data_.data()); > > > + } > > > + > > > + const T &operator*() const > > > + { > > > + return *reinterpret_cast<const T *>(data_.data()); > > > + } > > > + > > > + size_t size() const > > > + { > > > + return data_.size(); > > > + } > > > > A function that returns if the block is valid would be useful. This can > > be queried with size(), but that's not self-explicit. > > Earlier I suggested just returning `const T *` instead of `V4L2StatsBlock<T>` > from the `block()` function, I think that satisfies this requirement easily, > and I still think that would be a net simplification here. > As said, none of the currently mainlined ISP drivers use extensible stats. If one of them gets ported to to use them we'll have to handle two formats as we currently do for RkISP1 parameters and V4L2StatsBlock would be the place where we'll handle the differences between a "legacy" format and an extensible one (possibily by simply discarding the header before returning the memory location of a stats block). The cost of abstraction is cheap but if there is consensus on deferring the introduction of V4L2StatsBlock to when we'll have an existing ISP ported to use extensible stats, I'll do so. > > > > > > + > > > +protected: > > > + Span<const uint8_t> data_; > > > +}; > > > + > > > +template<typename Traits> > > > +class V4L2Stats > > > +{ > > > +public: > > > + V4L2Stats(Span<uint8_t> data, unsigned int version) > > > + : data_(data) > > > + { > > > + struct v4l2_isp_buffer *stats = > > > > const > > > > > + reinterpret_cast<struct v4l2_isp_buffer *>(data_.data()); > > > + used_ = stats->data_size; > > > + > > > + if (version != stats->version) > > > + LOG(V4L2Stats, Error) > > > + << "Unsupported v4l2-isp version: " << stats->version; > > > > It would be good to also validate that stats->data_size fits in > > data.size(). > > > > When validation fails, the V4L2Stats instance will be invalid. There > > should be a way for users to see that. > > If all blocks are parsed when constructed, then adding something like > > static std::optional<V4L2Stats> from(Span<const uint8_t>); > > should work for this purpose? Are you suggesting this a static method of V4L2Stats to replace the constructor ? > > > > > > None of those checks depend on Traits or on member variables, so you > > could move them to a separate non-template base class and avoid inlining > > them. See the series I've just sent for V4L2Params for an example. > > > > > + } > > > + > > > + size_t bytesused() const { return used_; } > > > > I don't see this being used in your RPP-X1 IPA code. What's the > > envisioned use case ? > > > > > + > > > + template<typename Traits::id_type Id> > > > + auto block() const > > > + { > > > + using Details = typename Traits::template id_to_details<Id>; > > > + > > > + using Type = typename Details::type; > > > + constexpr auto kernelId = Details::blockType; > > > + > > > + auto data = block(kernelId, sizeof(Type)); > > > + return V4L2StatsBlock<Type>(data); > > > + } > > > + > > > +protected: > > > + const Span<const uint8_t> block(unsigned int blockType, size_t blockSize) const > > > + { > > > + struct v4l2_isp_buffer *stats = > > > + reinterpret_cast<struct v4l2_isp_buffer *>(data_.data()); > > > + > > > + __u8 *data = stats->data; > > > + while (data < stats->data + stats->data_size) { > > > + struct v4l2_isp_block_header *header = > > > + reinterpret_cast<struct v4l2_isp_block_header *>(data); > > > > const > > > > > + > > > + if (header->type != blockType) { > > > + data += header->size; > > > > There's a risk of integer overflow. > > > > > + continue; > > > + } > > > + > > > + if (header->size != blockSize) { > > > + LOG(V4L2Stats, Error) > > > + << "Block type " << blockType > > > + << " size mistmatch: expected " > > > + << blockSize << " got:" > > > + << header->size; > > > + return {}; > > > + } > > > + > > > + return Span<const uint8_t>(data, header->size); > > > > There's a risk of going beyond the end of the buffer. > > > > > + } > > > > I would expect that in a typical use case the algorithms will use all > > available blocks, and different algorithms would use the same block. > > Isn't it more likely that if they need to, they will use data from the other algorithms "frame context"? > > > > Have you considered introducing a caching mechanism like in V4L2Params ? > > You could even populate the cache in the constructor by parsing the > > whole buffer. > > > > > + > > > + LOG(V4L2Stats, Error) << "Unsupported stats block type: " > > > + << blockType; > > > + > > > + return {}; > > > + } > > > + > > > + Span<uint8_t> data_; > > > + size_t used_; > > > +}; > > > + > > > +} /* namespace ipa */ > > > + > > > +} /* namespace libcamera */ > > >
2026. 05. 08. 11:01 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Fri, May 08, 2026 at 09:11:07AM +0200, Barnabás Pőcze wrote: >> 2026. 05. 07. 20:52 keltezéssel, Laurent Pinchart írta: >>> Hi Jacopo, >>> >>> Thank you for the patch. >>> >>> On Thu, May 07, 2026 at 12:07:55PM +0200, Jacopo Mondi wrote: >>>> Add a V4L2Stats class similar in spirit to the existing V4L2Params >>>> class to allow IPA modules to easily sub-class it to access ISP >>>> statistics blocks serialized into a v4l2_isp_buffer. >>>> >>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> --- >>>> src/ipa/libipa/meson.build | 2 + >>>> src/ipa/libipa/v4l2_stats.cpp | 226 ++++++++++++++++++++++++++++++++++++++++++ >>>> src/ipa/libipa/v4l2_stats.h | 123 +++++++++++++++++++++++ >>>> 3 files changed, 351 insertions(+) >>>> >>> [...] >>>> diff --git a/src/ipa/libipa/v4l2_stats.h b/src/ipa/libipa/v4l2_stats.h >>>> new file mode 100644 >>>> index 000000000000..b3b3ffca185d >>>> --- /dev/null >>>> +++ b/src/ipa/libipa/v4l2_stats.h >>>> @@ -0,0 +1,123 @@ >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >>>> +/* >>>> + * Copyright (C) 2026, Ideas On Board >>>> + * >>>> + * V4L2 Stats >>>> + */ >>>> + >>>> +#pragma once >>>> + >>>> +#include <stdint.h> >>>> + >>>> +#include <linux/media/v4l2-isp.h> >>>> + >>>> +#include <libcamera/base/log.h> >>>> +#include <libcamera/base/span.h> >>>> + >>>> +namespace libcamera { >>>> + >>>> +namespace ipa { >>>> + >>>> +LOG_DECLARE_CATEGORY(V4L2Stats) >>>> + >>>> +template<typename T> >>>> +class V4L2StatsBlock >>>> +{ >>>> +public: >>>> + V4L2StatsBlock(const Span<const uint8_t> data) >> >> The top-level `const` can be dropped here. >> >> >>>> + : data_(data) >>>> + { >>>> + } >>>> + >>>> + ~V4L2StatsBlock() {} >>> >>> Is this needed ? >>> >>>> + >>>> + const T *operator->() const >>>> + { >>>> + return reinterpret_cast<const T *>(data_.data()); >>>> + } >>>> + >>>> + const T &operator*() const >>>> + { >>>> + return *reinterpret_cast<const T *>(data_.data()); >>>> + } >>>> + >>>> + size_t size() const >>>> + { >>>> + return data_.size(); >>>> + } >>> >>> A function that returns if the block is valid would be useful. This can >>> be queried with size(), but that's not self-explicit. >> >> Earlier I suggested just returning `const T *` instead of `V4L2StatsBlock<T>` >> from the `block()` function, I think that satisfies this requirement easily, >> and I still think that would be a net simplification here. >> > > As said, none of the currently mainlined ISP drivers use extensible > stats. If one of them gets ported to to use them we'll have to handle > two formats as we currently do for RkISP1 parameters and > V4L2StatsBlock would be the place where we'll handle the differences > between a "legacy" format and an extensible one (possibily by simply > discarding the header before returning the memory location of a stats > block). > > The cost of abstraction is cheap but if there is consensus on > deferring the introduction of V4L2StatsBlock to when we'll have an > existing ISP ported to use extensible stats, I'll do so. Okay, fair enough. It's a small part, so either way it shouldn't be too difficult to change when old format support is implemented. > >> >>> >>>> + >>>> +protected: >>>> + Span<const uint8_t> data_; >>>> +}; >>>> + >>>> +template<typename Traits> >>>> +class V4L2Stats >>>> +{ >>>> +public: >>>> + V4L2Stats(Span<uint8_t> data, unsigned int version) >>>> + : data_(data) >>>> + { >>>> + struct v4l2_isp_buffer *stats = >>> >>> const >>> >>>> + reinterpret_cast<struct v4l2_isp_buffer *>(data_.data()); >>>> + used_ = stats->data_size; >>>> + >>>> + if (version != stats->version) >>>> + LOG(V4L2Stats, Error) >>>> + << "Unsupported v4l2-isp version: " << stats->version; >>> >>> It would be good to also validate that stats->data_size fits in >>> data.size(). >>> >>> When validation fails, the V4L2Stats instance will be invalid. There >>> should be a way for users to see that. >> >> If all blocks are parsed when constructed, then adding something like >> >> static std::optional<V4L2Stats> from(Span<const uint8_t>); >> >> should work for this purpose? > > Are you suggesting this a static method of V4L2Stats to replace the > constructor ? It was just a suggestion, if we want to validate the buffer and populate an (id -> span) mapping from the content, then I think a static factory function + private constructor could be one way to do it. > >> >> >>> >>> None of those checks depend on Traits or on member variables, so you >>> could move them to a separate non-template base class and avoid inlining >>> them. See the series I've just sent for V4L2Params for an example. >>> >>>> + } >>>> + >>>> + size_t bytesused() const { return used_; } >>> >>> I don't see this being used in your RPP-X1 IPA code. What's the >>> envisioned use case ? >>> >>>> + >>>> + template<typename Traits::id_type Id> >>>> + auto block() const >>>> + { >>>> + using Details = typename Traits::template id_to_details<Id>; >>>> + >>>> + using Type = typename Details::type; >>>> + constexpr auto kernelId = Details::blockType; >>>> + >>>> + auto data = block(kernelId, sizeof(Type)); >>>> + return V4L2StatsBlock<Type>(data); >>>> + } >>>> + >>>> +protected: >>>> + const Span<const uint8_t> block(unsigned int blockType, size_t blockSize) const >>>> + { >>>> + struct v4l2_isp_buffer *stats = >>>> + reinterpret_cast<struct v4l2_isp_buffer *>(data_.data()); >>>> + >>>> + __u8 *data = stats->data; >>>> + while (data < stats->data + stats->data_size) { >>>> + struct v4l2_isp_block_header *header = >>>> + reinterpret_cast<struct v4l2_isp_block_header *>(data); >>> >>> const >>> >>>> + >>>> + if (header->type != blockType) { >>>> + data += header->size; >>> >>> There's a risk of integer overflow. >>> >>>> + continue; >>>> + } >>>> + >>>> + if (header->size != blockSize) { >>>> + LOG(V4L2Stats, Error) >>>> + << "Block type " << blockType >>>> + << " size mistmatch: expected " >>>> + << blockSize << " got:" >>>> + << header->size; >>>> + return {}; >>>> + } >>>> + >>>> + return Span<const uint8_t>(data, header->size); >>> >>> There's a risk of going beyond the end of the buffer. >>> >>>> + } >>> >>> I would expect that in a typical use case the algorithms will use all >>> available blocks, and different algorithms would use the same block. >> >> Isn't it more likely that if they need to, they will use data from the other algorithms "frame context"? >> >> >>> Have you considered introducing a caching mechanism like in V4L2Params ? >>> You could even populate the cache in the constructor by parsing the >>> whole buffer. >>> >>>> + >>>> + LOG(V4L2Stats, Error) << "Unsupported stats block type: " >>>> + << blockType; >>>> + >>>> + return {}; >>>> + } >>>> + >>>> + Span<uint8_t> data_; >>>> + size_t used_; >>>> +}; >>>> + >>>> +} /* namespace ipa */ >>>> + >>>> +} /* namespace libcamera */ >>> >>
Hi Barnabás On Fri, May 08, 2026 at 11:22:10AM +0200, Barnabás Pőcze wrote: > 2026. 05. 08. 11:01 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Fri, May 08, 2026 at 09:11:07AM +0200, Barnabás Pőcze wrote: > > > 2026. 05. 07. 20:52 keltezéssel, Laurent Pinchart írta: > > > > Hi Jacopo, > > > > > > > > Thank you for the patch. > > > > > > > > On Thu, May 07, 2026 at 12:07:55PM +0200, Jacopo Mondi wrote: > > > > > Add a V4L2Stats class similar in spirit to the existing V4L2Params > > > > > class to allow IPA modules to easily sub-class it to access ISP > > > > > statistics blocks serialized into a v4l2_isp_buffer. > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > --- > > > > > src/ipa/libipa/meson.build | 2 + > > > > > src/ipa/libipa/v4l2_stats.cpp | 226 ++++++++++++++++++++++++++++++++++++++++++ > > > > > src/ipa/libipa/v4l2_stats.h | 123 +++++++++++++++++++++++ > > > > > 3 files changed, 351 insertions(+) > > > > > > > > > [...] > > > > > diff --git a/src/ipa/libipa/v4l2_stats.h b/src/ipa/libipa/v4l2_stats.h > > > > > new file mode 100644 > > > > > index 000000000000..b3b3ffca185d > > > > > --- /dev/null > > > > > +++ b/src/ipa/libipa/v4l2_stats.h > > > > > @@ -0,0 +1,123 @@ > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > +/* > > > > > + * Copyright (C) 2026, Ideas On Board > > > > > + * > > > > > + * V4L2 Stats > > > > > + */ > > > > > + > > > > > +#pragma once > > > > > + > > > > > +#include <stdint.h> > > > > > + > > > > > +#include <linux/media/v4l2-isp.h> > > > > > + > > > > > +#include <libcamera/base/log.h> > > > > > +#include <libcamera/base/span.h> > > > > > + > > > > > +namespace libcamera { > > > > > + > > > > > +namespace ipa { > > > > > + > > > > > +LOG_DECLARE_CATEGORY(V4L2Stats) > > > > > + > > > > > +template<typename T> > > > > > +class V4L2StatsBlock > > > > > +{ > > > > > +public: > > > > > + V4L2StatsBlock(const Span<const uint8_t> data) > > > > > > The top-level `const` can be dropped here. > > > > > > > > > > > + : data_(data) > > > > > + { > > > > > + } > > > > > + > > > > > + ~V4L2StatsBlock() {} > > > > > > > > Is this needed ? > > > > > > > > > + > > > > > + const T *operator->() const > > > > > + { > > > > > + return reinterpret_cast<const T *>(data_.data()); > > > > > + } > > > > > + > > > > > + const T &operator*() const > > > > > + { > > > > > + return *reinterpret_cast<const T *>(data_.data()); > > > > > + } > > > > > + > > > > > + size_t size() const > > > > > + { > > > > > + return data_.size(); > > > > > + } > > > > > > > > A function that returns if the block is valid would be useful. This can > > > > be queried with size(), but that's not self-explicit. > > > > > > Earlier I suggested just returning `const T *` instead of `V4L2StatsBlock<T>` > > > from the `block()` function, I think that satisfies this requirement easily, > > > and I still think that would be a net simplification here. > > > > > > > As said, none of the currently mainlined ISP drivers use extensible > > stats. If one of them gets ported to to use them we'll have to handle > > two formats as we currently do for RkISP1 parameters and > > V4L2StatsBlock would be the place where we'll handle the differences > > between a "legacy" format and an extensible one (possibily by simply > > discarding the header before returning the memory location of a stats > > block). > > > > The cost of abstraction is cheap but if there is consensus on > > deferring the introduction of V4L2StatsBlock to when we'll have an > > existing ISP ported to use extensible stats, I'll do so. > > Okay, fair enough. It's a small part, so either way it shouldn't be too > difficult to change when old format support is implemented. > What if we keep the abstraction for now and I add a bool() operator that checks if the block is empty ? > > > > > > > > > > > > > > > + > > > > > +protected: > > > > > + Span<const uint8_t> data_; > > > > > +}; > > > > > + > > > > > +template<typename Traits> > > > > > +class V4L2Stats > > > > > +{ > > > > > +public: > > > > > + V4L2Stats(Span<uint8_t> data, unsigned int version) > > > > > + : data_(data) > > > > > + { > > > > > + struct v4l2_isp_buffer *stats = > > > > > > > > const > > > > > > > > > + reinterpret_cast<struct v4l2_isp_buffer *>(data_.data()); > > > > > + used_ = stats->data_size; > > > > > + > > > > > + if (version != stats->version) > > > > > + LOG(V4L2Stats, Error) > > > > > + << "Unsupported v4l2-isp version: " << stats->version; > > > > > > > > It would be good to also validate that stats->data_size fits in > > > > data.size(). > > > > > > > > When validation fails, the V4L2Stats instance will be invalid. There > > > > should be a way for users to see that. > > > > > > If all blocks are parsed when constructed, then adding something like > > > > > > static std::optional<V4L2Stats> from(Span<const uint8_t>); > > > > > > should work for this purpose? > > > > Are you suggesting this a static method of V4L2Stats to replace the > > constructor ? > > It was just a suggestion, if we want to validate the buffer and populate > an (id -> span) mapping from the content, then I think a static factory > function + private constructor could be one way to do it. Indeed this could be a way to ensure a V4L2Stat has been constructed correctly and abort earlier if there are problems. I'll give V4L2Stats::from() a spin and see how it looks like. > > > > > > > > > > > > > > > > > > None of those checks depend on Traits or on member variables, so you > > > > could move them to a separate non-template base class and avoid inlining > > > > them. See the series I've just sent for V4L2Params for an example. > > > > > > > > > + } > > > > > + > > > > > + size_t bytesused() const { return used_; } > > > > > > > > I don't see this being used in your RPP-X1 IPA code. What's the > > > > envisioned use case ? > > > > > > > > > + > > > > > + template<typename Traits::id_type Id> > > > > > + auto block() const > > > > > + { > > > > > + using Details = typename Traits::template id_to_details<Id>; > > > > > + > > > > > + using Type = typename Details::type; > > > > > + constexpr auto kernelId = Details::blockType; > > > > > + > > > > > + auto data = block(kernelId, sizeof(Type)); > > > > > + return V4L2StatsBlock<Type>(data); > > > > > + } > > > > > + > > > > > +protected: > > > > > + const Span<const uint8_t> block(unsigned int blockType, size_t blockSize) const > > > > > + { > > > > > + struct v4l2_isp_buffer *stats = > > > > > + reinterpret_cast<struct v4l2_isp_buffer *>(data_.data()); > > > > > + > > > > > + __u8 *data = stats->data; > > > > > + while (data < stats->data + stats->data_size) { > > > > > + struct v4l2_isp_block_header *header = > > > > > + reinterpret_cast<struct v4l2_isp_block_header *>(data); > > > > > > > > const > > > > > > > > > + > > > > > + if (header->type != blockType) { > > > > > + data += header->size; > > > > > > > > There's a risk of integer overflow. > > > > > > > > > + continue; > > > > > + } > > > > > + > > > > > + if (header->size != blockSize) { > > > > > + LOG(V4L2Stats, Error) > > > > > + << "Block type " << blockType > > > > > + << " size mistmatch: expected " > > > > > + << blockSize << " got:" > > > > > + << header->size; > > > > > + return {}; > > > > > + } > > > > > + > > > > > + return Span<const uint8_t>(data, header->size); > > > > > > > > There's a risk of going beyond the end of the buffer. > > > > > > > > > + } > > > > > > > > I would expect that in a typical use case the algorithms will use all > > > > available blocks, and different algorithms would use the same block. > > > > > > Isn't it more likely that if they need to, they will use data from the other algorithms "frame context"? > > > > > > > > > > Have you considered introducing a caching mechanism like in V4L2Params ? > > > > You could even populate the cache in the constructor by parsing the > > > > whole buffer. > > > > > > > > > + > > > > > + LOG(V4L2Stats, Error) << "Unsupported stats block type: " > > > > > + << blockType; > > > > > + > > > > > + return {}; > > > > > + } > > > > > + > > > > > + Span<uint8_t> data_; > > > > > + size_t used_; > > > > > +}; > > > > > + > > > > > +} /* namespace ipa */ > > > > > + > > > > > +} /* namespace libcamera */ > > > > > > > >
On Fri, May 08, 2026 at 10:57:33AM +0200, Jacopo Mondi wrote: > On Thu, May 07, 2026 at 09:52:12PM +0300, Laurent Pinchart wrote: > > On Thu, May 07, 2026 at 12:07:55PM +0200, Jacopo Mondi wrote: > > > Add a V4L2Stats class similar in spirit to the existing V4L2Params > > > class to allow IPA modules to easily sub-class it to access ISP > > > statistics blocks serialized into a v4l2_isp_buffer. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > > src/ipa/libipa/meson.build | 2 + > > > src/ipa/libipa/v4l2_stats.cpp | 226 ++++++++++++++++++++++++++++++++++++++++++ > > > src/ipa/libipa/v4l2_stats.h | 123 +++++++++++++++++++++++ > > > 3 files changed, 351 insertions(+) > > > > > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build > > > index 963c5ee73063..16f4b095f220 100644 > > > --- a/src/ipa/libipa/meson.build > > > +++ b/src/ipa/libipa/meson.build > > > @@ -19,6 +19,7 @@ libipa_headers = files([ > > > 'pwl.h', > > > 'quantized.h', > > > 'v4l2_params.h', > > > + 'v4l2_stats.h', > > > ]) > > > > > > libipa_sources = files([ > > > @@ -40,6 +41,7 @@ libipa_sources = files([ > > > 'pwl.cpp', > > > 'quantized.cpp', > > > 'v4l2_params.cpp', > > > + 'v4l2_stats.cpp', > > > ]) > > > > > > libipa_includes = include_directories('..') > > > diff --git a/src/ipa/libipa/v4l2_stats.cpp b/src/ipa/libipa/v4l2_stats.cpp > > > new file mode 100644 > > > index 000000000000..19947a8d9015 > > > --- /dev/null > > > +++ b/src/ipa/libipa/v4l2_stats.cpp > > > @@ -0,0 +1,226 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2026, Ideas On Board > > > + * > > > + * V4L2 Statistics > > > > "V4L2 ISP Statistics" would be more accurate. > > > > Same in the .h files. > > > > > + */ > > > + > > > +#include "v4l2_stats.h" > > > + > > > +namespace libcamera { > > > + > > > +namespace ipa { > > > + > > > +LOG_DEFINE_CATEGORY(V4L2Stats) > > > + > > > +/** > > > + * \file v4l2_stats.cpp > > > + * \brief Helper class to handle an ISP statistics buffer compatible with > > > + * the generic V4L2 ISP format > > > + * > > > + * The Linux kernel defines a generic buffer format for ISP statistics. > > > + * The format describes a serialisation method that allows userspace to > > > + * access statistics data from a binary buffer. > > > + * > > > + * The V4L2Stats class implements support for the V4L2 ISP statistics buffer > > > + * format and allows users to retrieve an ISP statistics blocks, represented as > > > + * a V4L2StatsBlock class instances. > > > + * > > > + * IPA implementations using these helpers should define an enumeration of ISP > > > + * blocks supported by the IPA module and use a set of common abstractions to > > > + * help their derived implementation of V4L2Stats translate the enumerated ISP > > > + * block identifiers to the actual type of the statistics data as defined by > > > + * the kernel interface. > > > + */ > > > + > > > +/** > > > + * \class V4L2StatsBlock > > > + * \brief Helper class that represents an ISP statistics block > > > + * > > > + * Each ISP measurement block produces a set of statistics data whose memory > > > + * layout is defined by the kernel interface. > > > + * > > > + * This class represents an ISP statistics block entry. It is constructed > > > + * with a reference to the memory area where the statistics block is > > > + * stored in the statistics buffer. The template parameter represents > > > + * the underlying kernel-defined ISP statistics block type and allows its > > > + * user to easily cast it to said type to read the statistics data. > > > + * > > > + * \sa V4L2Stats::block() > > > + */ > > > + > > > +/** > > > + * \fn V4L2StatsBlock::V4L2StatsBlock() > > > + * \brief Construct a V4L2StatsBlock with memory represented by \a data > > > + * \param[in] data The memory area where the statistics block is located > > > + */ > > > + > > > +/** > > > + * \fn V4L2StatsBlock::operator->() const > > > + * \brief Access the statistics block casting it to the kernel-defined > > > + * ISP block type > > > + * > > > + * The V4L2StatsBlock is templated with the kernel defined ISP block type. This > > > + * function allows users to easily cast a V4L2StatsBlock to the underlying > > > + * kernel-defined type in order to easily access the statistics data. > > > + * > > > + * \code{.cpp} > > > + * > > > + * // The kernel header defines the ISP statistics types, for example > > > + * // struct my_isp_awb_stats_data { > > > + * // u16 mean_r; > > > + * // u16 mean_g; > > > + * // u16 mean_b; > > > + * // } > > > + * > > > + * template<> V4L2StatsBlock<struct my_isp_awb_stats_data> awbStats = ... > > > + * > > > + * u16 mean_r = awbStats->mean_r; > > > + * u16 mean_g = awbStats->mean_g; > > > + * u16 mean_b = awbStats->mean_b; > > > + * > > > + * \endcode > > > + * > > > + * Users of this class shall not create a V4L2StatsBlock manually but should > > > + * use V4L2Stats::block(). > > > + */ > > > + > > > +/** > > > + * \fn V4L2StatsBlock::operator*() const > > > + * \copydoc V4L2StatsBlock::operator->() const > > > + */ > > > + > > > +/** > > > + * \fn V4L2StatsBlock::size() const > > > + * \brief Retrieve the size (in bytes) of the ISP statistics block, including > > > + * the block's header > > > + * \return The size of the stats block > > > + */ > > > + > > > +/** > > > + * \var V4L2StatsBlock::data_ > > > + * \brief Memory area where ISP statistics have been serialized > > > + */ > > > + > > > +/** > > > + * \class V4L2Stats > > > + * \brief Helper class that represent an ISP statistics buffer > > > + * > > > + * This class represents an ISP statistics buffer. It is constructed with a > > > + * reference to the memory mapped buffer that has been dequeued from the ISP > > > + * driver. > > > + * > > > + * This class is templated with the type of the enumeration of ISP blocks that > > > + * each IPA module is expected to support. IPA modules are expected to derive > > > + * this class by providing a 'stats_traits' type that helps the class associate > > > + * a block type with the actual memory area that represents the ISP statistics > > > + * block. > > > + * > > > + * \code{.cpp} > > > + * > > > + * // Define the supported ISP statistics blocks > > > + * enum class myISPStats { > > > + * Agc, > > > + * Awb, > > > + * ... > > > + * }; > > > + * > > > + * // Maps the C++ enum type to the kernel enum type and concrete parameter type > > > + * template<myISPStats B> > > > + * struct block_type { > > > + * }; > > > + * > > > + * template<> > > > + * struct block_type<myISPStats::Agc> { > > > + * using type = struct my_isp_kernel_stats_type_agc; > > > + * static constexpr kernel_enum_type blockType = MY_ISP_STATS_TYPE_AGC; > > > + * }; > > > + * > > > + * template<> > > > + * struct block_type<myISPStats::Awb> { > > > + * using type = struct my_isp_kernel_stats_type_awb; > > > + * static constexpr kernel_enum_type blockType = MY_ISP_STATS_TYPE_AWB; > > > + * }; > > > + * > > > + * > > > + * // Convenience type to associate a block id to the 'block_type' overload > > > + * struct stats_traits { > > > + * using id_type = myISPStats; > > > + * template<id_type Id> using id_to_details = block_type<Id>; > > > + * }; > > > + * > > > + * ... > > > + * > > > + * // Derive the V4L2Stats class by providing stats_traits > > > + * class MyISPStats : public V4L2Stats<stats_traits> > > > + * { > > > + * public: > > > + * MyISPStats::MyISPStats(Span<uint8_t> data) > > > + * : V4L2Stats(data) > > > + * { > > > + * } > > > + * }; > > > + * > > > + * \endcode > > > + * > > > + * Users of this class can then easily access an ISP statistics block as a > > > + * V4L2StatsBlock instance. > > > + * > > > + * \code{.cpp} > > > + * > > > + * MyISPStats stats(data); > > > + * > > > + * auto awb = stats.block<myISPStats::AWB>(); > > > + * auto mean_r = awb->mean_r; > > > + * auto mean_g = awb->mean_g; > > > + * auto mean_b = awb->mean_b; > > > + * \endcode > > > + */ > > > + > > > +/** > > > + * \fn V4L2Stats::V4L2Stats() > > > + * \brief Construct an instance of V4L2Stats > > > + * \param[in] data Reference to the v4l2-buffer memory mapped area > > > + * \param[in] version The expected V4L2 ISP serialization format version > > > + */ > > > + > > > +/** > > > + * \fn V4L2Stats::bytesused() > > > + * \brief Retrieve the size of the statistics buffer (in bytes) > > > + * \return The number of bytes occupied by the ISP statistics > > > + */ > > > + > > > +/** > > > + * \fn V4L2Stats::block() const > > > + * \brief Retrieve the location of an ISP statistics block > > > + * \return A V4L2StatsBlock instance that points to the ISP statistics block > > > + */ > > > + > > > +/** > > > + * \fn V4L2Stats::block(unsigned int blockType, size_t blockSize) const > > > + * \brief Retrieve an ISP statistics block a returns a reference to it > > > + * \param[in] blockType The kernel-defined ISP block identifier, used to > > > + * identify the block header > > > + * \param[in] blockSize The ISP statistics block size, for validation > > > + * > > > + * Parse the buffer of serialized statistics data and retrieve the location > > > + * of the block with type \a blockType of size \a blockSize. > > > + * > > > + * IPA modules that derive the V4L2Stats class shall use this function to > > > + * retrieve the memory area that will be used to construct a V4L2StatsBlock<T> > > > + * before returning it to the caller. > > > + */ > > > + > > > +/** > > > + * \var V4L2Stats::data_ > > > + * \brief The ISP statistics buffer memory > > > + */ > > > + > > > +/** > > > + * \var V4L2Stats::used_ > > > + * \brief The number of bytes used by the statistics buffer > > > + */ > > > + > > > +} /* namespace ipa */ > > > + > > > +} /* namespace libcamera */ > > > diff --git a/src/ipa/libipa/v4l2_stats.h b/src/ipa/libipa/v4l2_stats.h > > > new file mode 100644 > > > index 000000000000..b3b3ffca185d > > > --- /dev/null > > > +++ b/src/ipa/libipa/v4l2_stats.h > > > @@ -0,0 +1,123 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2026, Ideas On Board > > > + * > > > + * V4L2 Stats > > > + */ > > > + > > > +#pragma once > > > + > > > +#include <stdint.h> > > > + > > > +#include <linux/media/v4l2-isp.h> > > > + > > > +#include <libcamera/base/log.h> > > > +#include <libcamera/base/span.h> > > > + > > > +namespace libcamera { > > > + > > > +namespace ipa { > > > + > > > +LOG_DECLARE_CATEGORY(V4L2Stats) > > > + > > > +template<typename T> > > > +class V4L2StatsBlock > > > +{ > > > +public: > > > + V4L2StatsBlock(const Span<const uint8_t> data) > > > + : data_(data) > > > + { > > > + } > > > + > > > + ~V4L2StatsBlock() {} > > > > Is this needed ? > > Probably not > > > > + > > > + const T *operator->() const > > > + { > > > + return reinterpret_cast<const T *>(data_.data()); > > > + } > > > + > > > + const T &operator*() const > > > + { > > > + return *reinterpret_cast<const T *>(data_.data()); > > > + } > > > + > > > + size_t size() const > > > + { > > > + return data_.size(); > > > + } > > > > A function that returns if the block is valid would be useful. This can > > be queried with size(), but that's not self-explicit. > > > > > + > > > +protected: > > > + Span<const uint8_t> data_; > > > +}; > > > + > > > +template<typename Traits> > > > +class V4L2Stats > > > +{ > > > +public: > > > + V4L2Stats(Span<uint8_t> data, unsigned int version) > > > + : data_(data) > > > + { > > > + struct v4l2_isp_buffer *stats = > > > > const > > > > > + reinterpret_cast<struct v4l2_isp_buffer *>(data_.data()); > > > + used_ = stats->data_size; > > > + > > > + if (version != stats->version) > > > + LOG(V4L2Stats, Error) > > > + << "Unsupported v4l2-isp version: " << stats->version; > > > > It would be good to also validate that stats->data_size fits in > > data.size(). > > The kernel should validate that, as a driver using the v4l2-isp helper > from > https://patchwork.linuxtv.org/project/linux-media/patch/20260505-extensible-stats-v1-6-e16f326b8dad@ideasonboard.com/ > shouldn't be able to populate stat blocks past the end of the buffer > size. That's correct, but it would still be nice to avoid a crash in case the buffer is not correctly populated. > > When validation fails, the V4L2Stats instance will be invalid. There > > should be a way for users to see that. > > > > None of those checks depend on Traits or on member variables, so you > > could move them to a separate non-template base class and avoid inlining > > them. See the series I've just sent for V4L2Params for an example. > > I'll have a look > > > > + } > > > + > > > + size_t bytesused() const { return used_; } > > > > I don't see this being used in your RPP-X1 IPA code. What's the > > envisioned use case ? > > possibily a leftover from V4L2Params > > > > + > > > + template<typename Traits::id_type Id> > > > + auto block() const > > > + { > > > + using Details = typename Traits::template id_to_details<Id>; > > > + > > > + using Type = typename Details::type; > > > + constexpr auto kernelId = Details::blockType; > > > + > > > + auto data = block(kernelId, sizeof(Type)); > > > + return V4L2StatsBlock<Type>(data); > > > + } > > > + > > > +protected: > > > + const Span<const uint8_t> block(unsigned int blockType, size_t blockSize) const > > > + { > > > + struct v4l2_isp_buffer *stats = > > > + reinterpret_cast<struct v4l2_isp_buffer *>(data_.data()); > > > + > > > + __u8 *data = stats->data; > > > + while (data < stats->data + stats->data_size) { > > > + struct v4l2_isp_block_header *header = > > > + reinterpret_cast<struct v4l2_isp_block_header *>(data); > > > > const > > > > > + > > > + if (header->type != blockType) { > > > + data += header->size; > > > > There's a risk of integer overflow. > > Am not I just advancing the data pointer here ? If header->size is very large the data pointer will move back and point to random memory. > > > + continue; > > > + } > > > + > > > + if (header->size != blockSize) { > > > + LOG(V4L2Stats, Error) > > > + << "Block type " << blockType > > > + << " size mistmatch: expected " > > > + << blockSize << " got:" > > > + << header->size; > > > + return {}; > > > + } > > > + > > > + return Span<const uint8_t>(data, header->size); > > > > There's a risk of going beyond the end of the buffer. > > Should I just check that 'data + header->size <= stats->data + > stats->data_size' ? With appropriate integer overflow handling :-) > > > + } > > > > I would expect that in a typical use case the algorithms will use all > > available blocks, and different algorithms would use the same block. > > This currently doesn't happen, right ? However it is certainly > possible > > > Have you considered introducing a caching mechanism like in V4L2Params ? > > I did. But if the cache is a class member I lose all the cost-ness as > creating a V4L2StatsBlock would require to modify the V4L2Stats state > to update the cache. > > > You could even populate the cache in the constructor by parsing the > > whole buffer. > > Honestly, I thought this was an overkill as currently stats blocks are > accessed once per frame. > > But it's true that every access would require re-parsing the buffer to > find the right block, so it might be more efficient to just do that > once at class creation time. Once we have indexes stored in the cached > looking up a block would simply be a matter of adding the offset to > the stats->data[] pointer ? Something like that, yes. Params need to be constructed at runtime, but stats are not mutable, so I think parsing them in one go would be a good idea. > > > + > > > + LOG(V4L2Stats, Error) << "Unsupported stats block type: " > > > + << blockType; > > > + > > > + return {}; > > > + } > > > + > > > + Span<uint8_t> data_; > > > + size_t used_; > > > +}; > > > + > > > +} /* namespace ipa */ > > > + > > > +} /* namespace libcamera */
On Fri, May 08, 2026 at 11:32:56AM +0200, Jacopo Mondi wrote: > On Fri, May 08, 2026 at 11:22:10AM +0200, Barnabás Pőcze wrote: > > 2026. 05. 08. 11:01 keltezéssel, Jacopo Mondi írta: > > > On Fri, May 08, 2026 at 09:11:07AM +0200, Barnabás Pőcze wrote: > > > > 2026. 05. 07. 20:52 keltezéssel, Laurent Pinchart írta: > > > > > On Thu, May 07, 2026 at 12:07:55PM +0200, Jacopo Mondi wrote: > > > > > > Add a V4L2Stats class similar in spirit to the existing V4L2Params > > > > > > class to allow IPA modules to easily sub-class it to access ISP > > > > > > statistics blocks serialized into a v4l2_isp_buffer. > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > --- > > > > > > src/ipa/libipa/meson.build | 2 + > > > > > > src/ipa/libipa/v4l2_stats.cpp | 226 ++++++++++++++++++++++++++++++++++++++++++ > > > > > > src/ipa/libipa/v4l2_stats.h | 123 +++++++++++++++++++++++ > > > > > > 3 files changed, 351 insertions(+) > > > > > > > > > > > [...] > > > > > > diff --git a/src/ipa/libipa/v4l2_stats.h b/src/ipa/libipa/v4l2_stats.h > > > > > > new file mode 100644 > > > > > > index 000000000000..b3b3ffca185d > > > > > > --- /dev/null > > > > > > +++ b/src/ipa/libipa/v4l2_stats.h > > > > > > @@ -0,0 +1,123 @@ > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > > +/* > > > > > > + * Copyright (C) 2026, Ideas On Board > > > > > > + * > > > > > > + * V4L2 Stats > > > > > > + */ > > > > > > + > > > > > > +#pragma once > > > > > > + > > > > > > +#include <stdint.h> > > > > > > + > > > > > > +#include <linux/media/v4l2-isp.h> > > > > > > + > > > > > > +#include <libcamera/base/log.h> > > > > > > +#include <libcamera/base/span.h> > > > > > > + > > > > > > +namespace libcamera { > > > > > > + > > > > > > +namespace ipa { > > > > > > + > > > > > > +LOG_DECLARE_CATEGORY(V4L2Stats) > > > > > > + > > > > > > +template<typename T> > > > > > > +class V4L2StatsBlock > > > > > > +{ > > > > > > +public: > > > > > > + V4L2StatsBlock(const Span<const uint8_t> data) > > > > > > > > The top-level `const` can be dropped here. > > > > > > > > > > + : data_(data) > > > > > > + { > > > > > > + } > > > > > > + > > > > > > + ~V4L2StatsBlock() {} > > > > > > > > > > Is this needed ? > > > > > > > > > > > + > > > > > > + const T *operator->() const > > > > > > + { > > > > > > + return reinterpret_cast<const T *>(data_.data()); > > > > > > + } > > > > > > + > > > > > > + const T &operator*() const > > > > > > + { > > > > > > + return *reinterpret_cast<const T *>(data_.data()); > > > > > > + } > > > > > > + > > > > > > + size_t size() const > > > > > > + { > > > > > > + return data_.size(); > > > > > > + } > > > > > > > > > > A function that returns if the block is valid would be useful. This can > > > > > be queried with size(), but that's not self-explicit. > > > > > > > > Earlier I suggested just returning `const T *` instead of `V4L2StatsBlock<T>` > > > > from the `block()` function, I think that satisfies this requirement easily, > > > > and I still think that would be a net simplification here. > > > > > > As said, none of the currently mainlined ISP drivers use extensible > > > stats. If one of them gets ported to to use them we'll have to handle > > > two formats as we currently do for RkISP1 parameters and > > > V4L2StatsBlock would be the place where we'll handle the differences > > > between a "legacy" format and an extensible one (possibily by simply > > > discarding the header before returning the memory location of a stats > > > block). > > > > > > The cost of abstraction is cheap but if there is consensus on > > > deferring the introduction of V4L2StatsBlock to when we'll have an > > > existing ISP ported to use extensible stats, I'll do so. > > > > Okay, fair enough. It's a small part, so either way it shouldn't be too > > difficult to change when old format support is implemented. > > What if we keep the abstraction for now and I add a bool() operator > that checks if the block is empty ? Or return an std::optional<> from block() > > > > > > + > > > > > > +protected: > > > > > > + Span<const uint8_t> data_; > > > > > > +}; > > > > > > + > > > > > > +template<typename Traits> > > > > > > +class V4L2Stats > > > > > > +{ > > > > > > +public: > > > > > > + V4L2Stats(Span<uint8_t> data, unsigned int version) > > > > > > + : data_(data) > > > > > > + { > > > > > > + struct v4l2_isp_buffer *stats = > > > > > > > > > > const > > > > > > > > > > > + reinterpret_cast<struct v4l2_isp_buffer *>(data_.data()); > > > > > > + used_ = stats->data_size; > > > > > > + > > > > > > + if (version != stats->version) > > > > > > + LOG(V4L2Stats, Error) > > > > > > + << "Unsupported v4l2-isp version: " << stats->version; > > > > > > > > > > It would be good to also validate that stats->data_size fits in > > > > > data.size(). > > > > > > > > > > When validation fails, the V4L2Stats instance will be invalid. There > > > > > should be a way for users to see that. > > > > > > > > If all blocks are parsed when constructed, then adding something like > > > > > > > > static std::optional<V4L2Stats> from(Span<const uint8_t>); > > > > > > > > should work for this purpose? > > > > > > Are you suggesting this a static method of V4L2Stats to replace the > > > constructor ? > > > > It was just a suggestion, if we want to validate the buffer and populate > > an (id -> span) mapping from the content, then I think a static factory > > function + private constructor could be one way to do it. > > Indeed this could be a way to ensure a V4L2Stat has been constructed > correctly and abort earlier if there are problems. I'll give > V4L2Stats::from() a spin and see how it looks like. > > > > > > None of those checks depend on Traits or on member variables, so you > > > > > could move them to a separate non-template base class and avoid inlining > > > > > them. See the series I've just sent for V4L2Params for an example. > > > > > > > > > > > + } > > > > > > + > > > > > > + size_t bytesused() const { return used_; } > > > > > > > > > > I don't see this being used in your RPP-X1 IPA code. What's the > > > > > envisioned use case ? > > > > > > > > > > > + > > > > > > + template<typename Traits::id_type Id> > > > > > > + auto block() const > > > > > > + { > > > > > > + using Details = typename Traits::template id_to_details<Id>; > > > > > > + > > > > > > + using Type = typename Details::type; > > > > > > + constexpr auto kernelId = Details::blockType; > > > > > > + > > > > > > + auto data = block(kernelId, sizeof(Type)); > > > > > > + return V4L2StatsBlock<Type>(data); > > > > > > + } > > > > > > + > > > > > > +protected: > > > > > > + const Span<const uint8_t> block(unsigned int blockType, size_t blockSize) const > > > > > > + { > > > > > > + struct v4l2_isp_buffer *stats = > > > > > > + reinterpret_cast<struct v4l2_isp_buffer *>(data_.data()); > > > > > > + > > > > > > + __u8 *data = stats->data; > > > > > > + while (data < stats->data + stats->data_size) { > > > > > > + struct v4l2_isp_block_header *header = > > > > > > + reinterpret_cast<struct v4l2_isp_block_header *>(data); > > > > > > > > > > const > > > > > > > > > > > + > > > > > > + if (header->type != blockType) { > > > > > > + data += header->size; > > > > > > > > > > There's a risk of integer overflow. > > > > > > > > > > > + continue; > > > > > > + } > > > > > > + > > > > > > + if (header->size != blockSize) { > > > > > > + LOG(V4L2Stats, Error) > > > > > > + << "Block type " << blockType > > > > > > + << " size mistmatch: expected " > > > > > > + << blockSize << " got:" > > > > > > + << header->size; > > > > > > + return {}; > > > > > > + } > > > > > > + > > > > > > + return Span<const uint8_t>(data, header->size); > > > > > > > > > > There's a risk of going beyond the end of the buffer. > > > > > > > > > > > + } > > > > > > > > > > I would expect that in a typical use case the algorithms will use all > > > > > available blocks, and different algorithms would use the same block. > > > > > > > > Isn't it more likely that if they need to, they will use data from the other algorithms "frame context"? > > > > > > > > > Have you considered introducing a caching mechanism like in V4L2Params ? > > > > > You could even populate the cache in the constructor by parsing the > > > > > whole buffer. > > > > > > > > > > > + > > > > > > + LOG(V4L2Stats, Error) << "Unsupported stats block type: " > > > > > > + << blockType; > > > > > > + > > > > > > + return {}; > > > > > > + } > > > > > > + > > > > > > + Span<uint8_t> data_; > > > > > > + size_t used_; > > > > > > +}; > > > > > > + > > > > > > +} /* namespace ipa */ > > > > > > + > > > > > > +} /* namespace libcamera */
Support for extensible stats has been submitted for review first by Antonie then broken-out to a dedicated patch series here: https://patchwork.linuxtv.org/project/linux-media/list/?series=24772 With the same spirit as V4L2Params, introduce a V4L2Stats class to help IPA modules handle a buffer of V4L2 ISP statistics. There are currently no users in-tree of the class but we have an example user in the RPP-X1 IPA module available at: https://gitlab.freedesktop.org/jmondi/libcamera/-/blob/v4h/jmondi/rppx1/src/ipa/rppx1/stats.h https://gitlab.freedesktop.org/jmondi/libcamera/-/blob/v4h/jmondi/rppx1/src/ipa/rppx1/algorithms/awb.cpp#L365 on top, a drive-by change to V4L2Params. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- Changes in v2: - Address Kieran's and Barnabas' comment --- Jacopo Mondi (3): include: v4l2-isp: Update to support extensible stats ipa: libipa: Introduce V4L2Stats ipa: libipa: v4l2_params: Fix code example in doc include/linux/media/v4l2-isp.h | 125 ++++++++++++++--------- src/ipa/libipa/meson.build | 2 + src/ipa/libipa/v4l2_params.cpp | 4 +- src/ipa/libipa/v4l2_stats.cpp | 226 +++++++++++++++++++++++++++++++++++++++++ src/ipa/libipa/v4l2_stats.h | 123 ++++++++++++++++++++++ 5 files changed, 432 insertions(+), 48 deletions(-) --- base-commit: 39d8133fd17c1e963981490105a154d78eb6cb43 change-id: 20260505-extensible-stats-772287be4ba8 Best regards,