[v3,4/5] ipa: libipa: Introduce V4L2Params

24476 diff mbox series

The existing RkISP1Params helper classes allows the RkISP1 to handle
V4L2 extensible parameters format and the legacy RkIPS1-specific
fixed-size parameters format.

With the introduction of v4l2-isp.h in the Linux kernel the part of
the RkISP1Params helper class that handles extensible parameters can
be generalized so that other IPA modules can use the same helpers
to populate a v4l2-params compatible buffer.

Generalize the RkISP1Params class to a new libipa component named
V4L2Params and derive the existing RkISP1Params from it, leaving
in the RkISP1-specific implementation the handling of the legacy format.

Deriving RkISP1Params from V4L2Params requires changing the size
associated to each block to include the size of v4l2_params_block_header
in the ipa:rkisp1::kBlockTypeInfo map as the V4L2Params::block()
implementation doesn't account for that as RkIS1Params::block()
implementation did.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Tested-By: Antoine Bouyer <antoine.bouyer@nxp.com>
---
 src/ipa/libipa/meson.build     |   2 +
 src/ipa/libipa/v4l2_params.cpp | 262 +++++++++++++++++++++++++++++++++++++++++
 src/ipa/libipa/v4l2_params.h   | 142 ++++++++++++++++++++++
 src/ipa/rkisp1/params.cpp      |  93 +--------------
 src/ipa/rkisp1/params.h        | 175 +++++++++++++++------------
 5 files changed, 509 insertions(+), 165 deletions(-)

Hi

2025. 09. 26. 16:39 keltezéssel, Jacopo Mondi írta:
> The existing RkISP1Params helper classes allows the RkISP1 to handle
> V4L2 extensible parameters format and the legacy RkIPS1-specific
> fixed-size parameters format.
> 
> With the introduction of v4l2-isp.h in the Linux kernel the part of
> the RkISP1Params helper class that handles extensible parameters can
> be generalized so that other IPA modules can use the same helpers
> to populate a v4l2-params compatible buffer.
> 
> Generalize the RkISP1Params class to a new libipa component named
> V4L2Params and derive the existing RkISP1Params from it, leaving
> in the RkISP1-specific implementation the handling of the legacy format.
> 
> Deriving RkISP1Params from V4L2Params requires changing the size
> associated to each block to include the size of v4l2_params_block_header
> in the ipa:rkisp1::kBlockTypeInfo map as the V4L2Params::block()
> implementation doesn't account for that as RkIS1Params::block()
> implementation did.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Tested-By: Antoine Bouyer <antoine.bouyer@nxp.com>
> ---
>   src/ipa/libipa/meson.build     |   2 +
>   src/ipa/libipa/v4l2_params.cpp | 262 +++++++++++++++++++++++++++++++++++++++++
>   src/ipa/libipa/v4l2_params.h   | 142 ++++++++++++++++++++++
>   src/ipa/rkisp1/params.cpp      |  93 +--------------
>   src/ipa/rkisp1/params.h        | 175 +++++++++++++++------------
>   5 files changed, 509 insertions(+), 165 deletions(-)
> 
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 660be94054fa98b714b6bc586039081e45a6b4bc..4010739e710eb38aa6108eb8258c574a616bf3c0 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -16,6 +16,7 @@ libipa_headers = files([
>       'lsc_polynomial.h',
>       'lux.h',
>       'module.h',
> +    'v4l2_params.h',
>       'pwl.h',
>   ])
>   
> @@ -35,6 +36,7 @@ libipa_sources = files([
>       'lsc_polynomial.cpp',
>       'lux.cpp',
>       'module.cpp',
> +    'v4l2_params.cpp',
>       'pwl.cpp',
>   ])
>   
> diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp
> new file mode 100644
> index 0000000000000000000000000000000000000000..b116d0a2f79da8ab9cf695195b317f4038202262
> --- /dev/null
> +++ b/src/ipa/libipa/v4l2_params.cpp
> @@ -0,0 +1,262 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2025, Ideas On Board
> + *
> + * V4L2 Parameters
> + */
> +
> +#include "v4l2_params.h"
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +/**
> + * \file v4l2_params.cpp
> + * \brief Helper class to populate an ISP configuration buffer compatible with
> + * the generic V4L2 ISP extensible format
> + *
> + * The Linux kernel defines a generic buffer format for configuring ISP devices
> + * through a set of parameters in the form of V4L2 extensible parameters. The
> + * V4L2 extensible parameters define a serialization format for ISP parameters
> + * that allows userspace to populate a buffer of configuration data by appending
> + * them one after the other in a binary buffer.
> + *
> + * Each ISP driver compatible with the v4l2-isp format will define its own
> + * meta-output format identifier and defines the types of the configuration data
> + * of each ISP block that usually match the registers layout.
> + *
> + * The V4L2Params class represent the V4L2 extensible parameters buffer and
> + * allows users to populate the ISP configuration blocks, represented by the
> + * V4L2ParamBlock class instances.
> + *
> + * IPA implementations using this helpers should define an enumeration of ISP
> + * blocks the IPA module supports and use a set of common abstraction to help
> + * their derived implementation of V4L2Params translate the enumerated ISP block
> + * identifier to the actual type of the configuration data as defined by the
> + * kernel interface.
> + *
> + * As an example of this see the RkISP1 and Mali-C55 implementations.
> + */
> +
> +/**
> + * \class V4L2ParamsBlock
> + * \brief Helper class that represents a ISP configuration block
> + *
> + * Each ISP function is associated with a set of configuration parameters
> + * defined by the kernel interface.
> + *
> + * This class represents an ISP block configuration entry. It is constructed
> + * with a reference to the memory area where the block configuration will be
> + * stored in the parameters buffer. The template parameter represents
> + * the underlying kernel-defined ISP block configuration type and allow its
> + * user to easily cast it to said type to populate and read the configuration
> + * parameters.
> + *
> + * \sa V4L2Params::block()
> + */
> +
> +/**
> + * \fn V4L2ParamsBlock::V4L2ParamsBlock()
> + * \brief Construct a V4L2ParamsBlock with memory represented by \a data
> + * \param[in] data A view on the memory area where the ISP block is located
> + */
> +
> +/**
> + * \fn V4L2ParamsBlock::setEnabled()
> + * \brief Enable/disable an ISP configuration block
> + * \param[in] enabled The enable flag
> + */
> +
> +/**
> + * \fn V4L2ParamsBlock::operator->()
> + * \brief Access the ISP configuration block casting it to the kernel-defined
> + * ISP configuration type
> + *
> + * The V4L2ParamsBlock is templated with the kernel defined ISP configuration
> + * block type. This function allows users to easily cast a V4L2ParamsBlock to
> + * the underlying kernel-defined type in order to easily populate or read
> + * the ISP configuration data.
> + *
> + * \code{.cpp}
> + *
> + * // The kernel header defines the ISP configuration types, in example
> + * // struct my_isp_awb_config_data {
> + * //		u16 gain_ch00;
> + * //		u16 gain_ch01;
> + * //		u16 gain_ch10;
> + * //		u16 gain_ch11;
> + * //
> + * //  }
> + *
> + * template<> V4L2ParamsBlock<struct my_isp_awb_config_data> awbBlock
> + *
> + * awbBlock->gain_ch00 = ...;
> + * awbBlock->gain_ch01 = ...;
> + * awbBlock->gain_ch10 = ...;
> + * awbBlock->gain_ch11 = ...;
> + *
> + * \endcode
> + *
> + * Users of this class are not expected to create a V4L2ParamsBlock manually but
> + * should rather use V4L2Params::block() to retrieve a reference to the memory
> + * area used to construct a V4L2ParamsBlock<T> in their overloaded
> + * implementation of V4L2Params::block().
> + */
> +
> +/**
> + * \fn V4L2ParamsBlock::operator->() const
> + * \copydoc V4L2ParamsBlock::operator->()
> + */
> +
> +/**
> + * \fn V4L2ParamsBlock::operator*() const
> + * \copydoc V4L2ParamsBlock::operator->()
> + */
> +
> +/**
> + * \fn V4L2ParamsBlock::operator*()
> + * \copydoc V4L2ParamsBlock::operator->()
> + */
> +
> +/**
> + * \var V4L2ParamsBlock::data_
> + * \brief Memory area reserved for the ISP configuration block
> + */
> +
> + /**
> +  * \class V4L2Params
> +  * \brief Helper class that represent an ISP configuration buffer
> +  *
> + * ISP implementation compatible with v4l2-isp define their ISP configuration
> + * buffer types compatible with the struct v4l2_params_buffer type.
> + *
> + * This class represents an ISP configuration buffer. It is constructed
> + * with a reference to the memory mapped buffer that will be queued to the ISP.
> + *
> + * 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 'param_traits' type the helps the class associate
> + * a block type with the actual memory area that represents the ISP
> + * configuration block.
> + *
> + * \code{.cpp}
> + *
> + * // Define the supported ISP blocks
> + * enum class myISPBlocks {
> + *	Agc,
> + *	Awb,
> + *	...
> + * };
> + *
> + * // Associated to the ISP blocks the kernel types

   "Maps the C++ enum type to the kernel enum type and concrete parameter type"

Maybe something like the above?


> + * template<myISPBlocks B>
> + * struct block_type {
> + * };
> + *
> + * template<>
> + * struct block_type<myISPBlock::Agc> {
> + *	using type = struct my_isp_kernel_config_type_agc;
> + *	static constextpr struct my_isp_kernel_block_type MY_ISP_TYPE_AGC;

constextpr -> constexpr

And shouldn't it be something like `static constexpr kernel_enum_type blockType = MY_ISP_BLOCK_TYPE_AGC` ?



> + * };
> + *
> + * template<>
> + * struct block_type<myISPBlock::Awb> {
> + *	using type = struct my_isp_kernel_config_type_awb;
> + *	static constextpr struct my_isp_kernel_block_type MY_ISP_TYPE_AWB;

Same here.


> + * };
> + *
> + *
> + * // Convenience type to associated a block id to the 'block_type' overload

associated -> associate


> + * struct params_traits {
> + * 	using id_type = myISPBlocks;
> + * 	template<id_type Id> using id_to_details = block_type<Id>;
> + * };
> + *
> + * ...
> + *
> + * // Derive the V4L2Params class by providing params_traits
> + * class MyISPParams : public V4L2Params<params_traits>
> + * {
> + * public:
> + * 	MyISPParams::MyISPParams(Span<uint8_t> data)
> + * 		: V4L2Params(data, kVersion)
> + * 	{
> + * 	}
> + * };
> + *
> + * \endcode
> + *
> + * Users of this class can then easily access an ISP configuration block as a
> + * V4L2ParamsBlock instance.
> + *
> + * \code{.cpp}
> + *
> + * MyISPParams params(data);
> + *
> + * auto awb = params.block<myISPBlocks::AWB>();
> + * awb->gain00 = ...;
> + * awb->gain01 = ...;
> + * awb->gain10 = ...;
> + * awb->gain11 = ...;
> + * \endcode
> + */
> +
> +/**
> + * \fn V4L2Params::V4L2Params()
> + * \brief Construct a V4L2Params
> + * \param[in] data Reference to the v4l2-buffer memory mapped area
> + * \param[in] version The ISP parameters version the implementation supports
> + */
> +
> +/**
> + * \fn V4L2Params::size()
> + * \brief Retrieve the used size of the parameters buffer (in bytes)
> + *
> + * The parameters buffer size is mostly used to populate the v4l2_buffer
> + * bytesused field before queueing the buffer to the ISP.
> + *
> + * \return The number of bytes occupied by the ISP configuration parameters
> + */
> +
> +/**
> + * \fn V4L2Params::block()
> + * \brief Retrieve the location of an ISP configuration block a returns it
> + * \return A V4L2ParamsBlock instance that points to the ISP configuration block
> + */
> +
> +/**
> + * \fn V4L2Params::block(typename Traits::id_type type, unsigned int blockType, size_t blockSize)
> + * \brief Populate an ISP configuration block a returns a reference to its
> + * memory
> + * \param[in] type The ISP block identifier enumerated by the IPA module
> + * \param[in] blockType The kernel-defined ISP block identifier, used to
> + * populate the block header
> + * \param[in] blockSize The ISP block size, used to populate the block header
> + *
> + * Initialize the block header with \a blockType and \a blockSize and
> + * returns a reference to the memory used to store an ISP configuration block.
> + *
> + * IPA modules that derive the V4L2Params class shall use this function to
> + * retrieve the memory area that will be used to construct a V4L2ParamsBlock<T>
> + * before returning it to the caller.
> + */
> +
> +/**
> + * \var V4L2Params::data_
> + * \brief The ISP parameters buffer memory
> + */
> +
> +/**
> + * \var V4L2Params::used_
> + * \brief The number of bytes used in the parameters buffer
> + */
> +
> +/**
> + * \var V4L2Params::blocks_
> + * \brief Cache of ISP configuration blocks
> + */
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..c85552ec50ae6f3a1bd8a0fede3c793df2f6da4d
> --- /dev/null
> +++ b/src/ipa/libipa/v4l2_params.h
> @@ -0,0 +1,142 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2025, Ideas On Board
> + *
> + * V4L2 Parameters
> + */
> +
> +#pragma once
> +
> +#include <map>
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include <linux/media/v4l2-isp.h>
> +
> +#include <libcamera/base/span.h>
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +template<typename T>
> +class V4L2ParamsBlock
> +{
> +public:
> +	V4L2ParamsBlock(const Span<uint8_t> data)
> +		: data_(data)
> +	{
> +	}
> +
> +	virtual ~V4L2ParamsBlock() {}
> +
> +	virtual void setEnabled(bool enabled)
> +	{
> +		struct v4l2_params_block_header *header =
> +			reinterpret_cast<struct v4l2_params_block_header *>(data_.data());
> +
> +		header->flags &= ~(V4L2_PARAMS_FL_BLOCK_ENABLE |
> +				   V4L2_PARAMS_FL_BLOCK_DISABLE);
> +		header->flags |= enabled ? V4L2_PARAMS_FL_BLOCK_ENABLE
> +					 : V4L2_PARAMS_FL_BLOCK_DISABLE;
> +	}
> +
> +	virtual const T *operator->() const
> +	{
> +		return reinterpret_cast<const T *>(data_.data());
> +	}
> +
> +	virtual T *operator->()
> +	{
> +		return reinterpret_cast<T *>(data_.data());
> +	}
> +
> +	virtual const T &operator*() const
> +	{
> +		return *reinterpret_cast<const T *>(data_.data());
> +	}
> +
> +	virtual T &operator*()
> +	{
> +		return *reinterpret_cast<T *>(data_.data());
> +	}

I have been thinking about these virtual functions. And as far as I can tell,
at least with the current code, there seems to be no need for them. `RkISP1ParamsBlock`
also does not need to inherit `V4L2ParamsBlock` because, at least at the moment,
there is no code that operates on these parameter blocks in a "generic" fashion.
And even if there was, the specific type is encoded in a template parameter, so
any such generic code would need to be templated at least to some degree.

The above suggests to me that we could probably remove the virtual functions.
One option could be the following:
   * do not make `RkISP1ParamsBlock` inherit `V4L2ParamsBlock`
   * remove the "virtual" from the functions
   * move the `V4L2ParamsBlock<T>::setEnabled()` implementation into a separate function
   * use that function to implement `V4L2ParamsBlock<T>::setEnabled()`
     and `RkISP1ParamsBlock<T>::setEnabled()`

Thoughts? Or maybe just keep them virtual?


Regards,
Barnabás Pőcze


> +
> +protected:
> +	Span<uint8_t> data_;
> +};
> +
> +template<typename Traits>
> +class V4L2Params
> +{
> +public:
> +	V4L2Params(Span<uint8_t> data, unsigned int version)
> +		: data_(data)
> +	{
> +		struct v4l2_params_buffer *cfg =
> +			reinterpret_cast<struct v4l2_params_buffer *>(data_.data());
> +		cfg->data_size = 0;
> +		cfg->version = version;
> +		used_ = offsetof(struct v4l2_params_buffer, data);
> +	}
> +
> +	size_t size() const { return used_; }
> +
> +	template<typename Traits::id_type Id>
> +	auto block()
> +	{
> +		using Details = typename Traits::template id_to_details<Id>;
> +
> +		using Type = typename Details::type;
> +		constexpr auto kernelId = Details::blockType;
> +
> +		auto data = block(Id, kernelId, sizeof(Type));
> +		return V4L2ParamsBlock<Type>(data);
> +	}
> +
> +protected:
> +	Span<uint8_t> block(typename Traits::id_type type,
> +			    unsigned int blockType, size_t blockSize)
> +	{
> +		/*
> +		 * Look up the block in the cache first. If an algorithm
> +		 * requests the same block type twice, it should get the same
> +		 * block.
> +		 */
> +		auto cacheIt = blocks_.find(type);
> +		if (cacheIt != blocks_.end())
> +			return cacheIt->second;
> +
> +		/* Make sure we don't run out of space. */
> +		if (blockSize > data_.size() - used_)
> +			return {};
> +
> +		/* Allocate a new block, clear its memory, and initialize its header. */
> +		Span<uint8_t> block = data_.subspan(used_, blockSize);
> +		used_ += blockSize;
> +
> +		struct v4l2_params_buffer *cfg =
> +			reinterpret_cast<struct v4l2_params_buffer *>(data_.data());
> +		cfg->data_size += blockSize;
> +
> +		memset(block.data(), 0, block.size());
> +
> +		struct v4l2_params_block_header *header =
> +			reinterpret_cast<struct v4l2_params_block_header *>(block.data());
> +		header->type = blockType;
> +		header->size = block.size();
> +
> +		/* Update the cache. */
> +		blocks_[type] = block;
> +
> +		return block;
> +	}
> +
> +	Span<uint8_t> data_;
> +	size_t used_;
> +
> +	std::map<typename Traits::id_type, Span<uint8_t>> blocks_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp
> index 5edb36c91b87859d02c0a8b41efe977ff048def5..7040207c26557aa278050a1f7232cc6c380505b1 100644
> --- a/src/ipa/rkisp1/params.cpp
> +++ b/src/ipa/rkisp1/params.cpp
> @@ -35,7 +35,7 @@ struct BlockTypeInfo {
>   #define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit)			\
>   	{ BlockType::block, {							\
>   		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id,				\
> -		sizeof(struct rkisp1_cif_isp_##type##_config),			\
> +		sizeof(struct rkisp1_ext_params_##type##_config),		\
>   		offsetof(struct rkisp1_params_cfg, category.type##_config),	\
>   		RKISP1_CIF_ISP_MODULE_##bit,					\
>   	} }
> @@ -49,7 +49,7 @@ struct BlockTypeInfo {
>   #define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type)				\
>   	{ BlockType::block, {							\
>   		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id,				\
> -		sizeof(struct rkisp1_cif_isp_##type##_config),			\
> +		sizeof(struct rkisp1_ext_params_##type##_config),		\
>   		0, 0,								\
>   	} }
>   
> @@ -79,56 +79,6 @@ const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = {
>   
>   } /* namespace */
>   
> -RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,
> -					     const Span<uint8_t> &data)
> -	: params_(params), type_(type)
> -{
> -	if (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {
> -		header_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header));
> -		data_ = data.subspan(sizeof(rkisp1_ext_params_block_header));
> -	} else {
> -		data_ = data;
> -	}
> -}
> -
> -void RkISP1ParamsBlockBase::setEnabled(bool enabled)
> -{
> -	/*
> -	 * For the legacy fixed format, blocks are enabled in the top-level
> -	 * header. Delegate to the RkISP1Params class.
> -	 */
> -	if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)
> -		return params_->setBlockEnabled(type_, enabled);
> -
> -	/*
> -	 * For the extensible format, set the enable and disable flags in the
> -	 * block header directly.
> -	 */
> -	struct rkisp1_ext_params_block_header *header =
> -		reinterpret_cast<struct rkisp1_ext_params_block_header *>(header_.data());
> -	header->flags &= ~(RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |
> -			   RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE);
> -	header->flags |= enabled ? RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE
> -				 : RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE;
> -}
> -
> -RkISP1Params::RkISP1Params(uint32_t format, Span<uint8_t> data)
> -	: format_(format), data_(data), used_(0)
> -{
> -	if (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {
> -		struct rkisp1_ext_params_cfg *cfg =
> -			reinterpret_cast<struct rkisp1_ext_params_cfg *>(data.data());
> -
> -		cfg->version = RKISP1_EXT_PARAM_BUFFER_V1;
> -		cfg->data_size = 0;
> -
> -		used_ += offsetof(struct rkisp1_ext_params_cfg, data);
> -	} else {
> -		memset(data.data(), 0, data.size());
> -		used_ = sizeof(struct rkisp1_params_cfg);
> -	}
> -}
> -
>   void RkISP1Params::setBlockEnabled(BlockType type, bool enabled)
>   {
>   	const BlockTypeInfo &info = kBlockTypeInfo.at(type);
> @@ -178,44 +128,7 @@ Span<uint8_t> RkISP1Params::block(BlockType type)
>   		return data_.subspan(info.offset, info.size);
>   	}
>   
> -	/*
> -	 * For the extensible format, allocate memory for the block, including
> -	 * the header. Look up the block in the cache first. If an algorithm
> -	 * requests the same block type twice, it should get the same block.
> -	 */
> -	auto cacheIt = blocks_.find(type);
> -	if (cacheIt != blocks_.end())
> -		return cacheIt->second;
> -
> -	/* Make sure we don't run out of space. */
> -	size_t size = sizeof(struct rkisp1_ext_params_block_header)
> -		    + ((info.size + 7) & ~7);
> -	if (size > data_.size() - used_) {
> -		LOG(RkISP1Params, Error)
> -			<< "Out of memory to allocate block type "
> -			<< utils::to_underlying(type);
> -		return {};
> -	}
> -
> -	/* Allocate a new block, clear its memory, and initialize its header. */
> -	Span<uint8_t> block = data_.subspan(used_, size);
> -	used_ += size;
> -
> -	struct rkisp1_ext_params_cfg *cfg =
> -		reinterpret_cast<struct rkisp1_ext_params_cfg *>(data_.data());
> -	cfg->data_size += size;
> -
> -	memset(block.data(), 0, block.size());
> -
> -	struct rkisp1_ext_params_block_header *header =
> -		reinterpret_cast<struct rkisp1_ext_params_block_header *>(block.data());
> -	header->type = info.type;
> -	header->size = block.size();
> -
> -	/* Update the cache. */
> -	blocks_[type] = block;
> -
> -	return block;
> +	return V4L2Params::block(type, info.type, info.size);
>   }
>   
>   } /* namespace ipa::rkisp1 */
> diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h
> index 2e60528d102ec44a31417d4b146e74cace363efa..a7fbbc581c6df8a92dcc4fcfe1e620da6c469915 100644
> --- a/src/ipa/rkisp1/params.h
> +++ b/src/ipa/rkisp1/params.h
> @@ -7,13 +7,10 @@
>   
>   #pragma once
>   
> -#include <map>
> -#include <stdint.h>
> -
>   #include <linux/rkisp1-config.h>
> +#include <linux/videodev2.h>
>   
> -#include <libcamera/base/class.h>
> -#include <libcamera/base/span.h>
> +#include <libipa/v4l2_params.h>
>   
>   namespace libcamera {
>   
> @@ -49,115 +46,143 @@ template<BlockType B>
>   struct block_type {
>   };
>   
> -#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)		\
> +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct, id)		\
>   template<>								\
>   struct block_type<BlockType::blockType> {				\
>   	using type = struct rkisp1_cif_isp_##blockStruct##_config;	\
> +	static constexpr rkisp1_ext_params_block_type blockType =	\
> +		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id;			\
>   };
>   
> -RKISP1_DEFINE_BLOCK_TYPE(Bls, bls)
> -RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc)
> -RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg)
> -RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain)
> -RKISP1_DEFINE_BLOCK_TYPE(Flt, flt)
> -RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm)
> -RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk)
> -RKISP1_DEFINE_BLOCK_TYPE(Goc, goc)
> -RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf)
> -RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength)
> -RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc)
> -RKISP1_DEFINE_BLOCK_TYPE(Ie, ie)
> -RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc)
> -RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas)
> -RKISP1_DEFINE_BLOCK_TYPE(Hst, hst)
> -RKISP1_DEFINE_BLOCK_TYPE(Aec, aec)
> -RKISP1_DEFINE_BLOCK_TYPE(Afc, afc)
> -RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls)
> -RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve)
> -RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve)
> -RKISP1_DEFINE_BLOCK_TYPE(Wdr, wdr)
> +RKISP1_DEFINE_BLOCK_TYPE(Bls, bls, BLS)
> +RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc, DPCC)
> +RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg, SDG)
> +RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain, AWB_GAIN)
> +RKISP1_DEFINE_BLOCK_TYPE(Flt, flt, FLT)
> +RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm, BDM)
> +RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk, CTK)
> +RKISP1_DEFINE_BLOCK_TYPE(Goc, goc, GOC)
> +RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf, DPF)
> +RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength, DPF_STRENGTH)
> +RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc, CPROC)
> +RKISP1_DEFINE_BLOCK_TYPE(Ie, ie, IE)
> +RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc, LSC)
> +RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas, AWB_MEAS)
> +RKISP1_DEFINE_BLOCK_TYPE(Hst, hst, HST_MEAS)
> +RKISP1_DEFINE_BLOCK_TYPE(Aec, aec, AEC_MEAS)
> +RKISP1_DEFINE_BLOCK_TYPE(Afc, afc, AFC_MEAS)
> +RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls, COMPAND_BLS)
> +RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve, COMPAND_EXPAND)
> +RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve, COMPAND_COMPRESS)
> +RKISP1_DEFINE_BLOCK_TYPE(Wdr, wdr, WDR)
> +
> +struct params_traits {
> +	using id_type = BlockType;
> +
> +	template<id_type Id>
> +	using id_to_details = block_type<Id>;
> +};
>   
>   } /* namespace details */
>   
> -class RkISP1Params;
> +template<typename T>
> +class RkISP1ParamsBlock;
>   
> -class RkISP1ParamsBlockBase
> +class RkISP1Params : public V4L2Params<details::params_traits>
>   {
>   public:
> -	RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,
> -			      const Span<uint8_t> &data);
> +	static constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1;
> +
> +	RkISP1Params(uint32_t format, Span<uint8_t> data)
> +		: V4L2Params(data, kVersion), format_(format)
> +	{
> +		if (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {
> +			memset(data.data(), 0, data.size());
> +			used_ = sizeof(struct rkisp1_params_cfg);
> +		}
> +	}
> +
> +	template<details::params_traits::id_type id>
> +	auto block()
> +	{
> +		using Type = typename details::block_type<id>::type;
>   
> -	Span<uint8_t> data() const { return data_; }
> +		return RkISP1ParamsBlock<Type>(this, id, block(id));
> +	}
>   
> -	void setEnabled(bool enabled);
> +	uint32_t format() const { return format_; }
> +	void setBlockEnabled(BlockType type, bool enabled);
>   
>   private:
> -	LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase)
> +	Span<uint8_t> block(BlockType type);
>   
> -	RkISP1Params *params_;
> -	BlockType type_;
> -	Span<uint8_t> header_;
> -	Span<uint8_t> data_;
> +	uint32_t format_;
>   };
>   
> -template<BlockType B>
> -class RkISP1ParamsBlock : public RkISP1ParamsBlockBase
> +template<typename T>
> +class RkISP1ParamsBlock : public V4L2ParamsBlock<T>
>   {
>   public:
> -	using Type = typename details::block_type<B>::type;
> -
> -	RkISP1ParamsBlock(RkISP1Params *params, const Span<uint8_t> &data)
> -		: RkISP1ParamsBlockBase(params, B, data)
> +	RkISP1ParamsBlock(RkISP1Params *params, BlockType type,
> +			  const Span<uint8_t> data)
> +		: V4L2ParamsBlock<T>(data)
>   	{
> +		params_ = params;
> +		type_ = type;
> +
> +		/*
> +		 * cifData_ points to the actual configuration data
> +		 * (struct rkisp1_cif_isp_*) which is not prefixed by any header,
> +		 * for the legacy fixed format.
> +		 */
> +		if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)
> +			cifData_ = data;
> +		else
> +			cifData_ = data.subspan(sizeof(v4l2_params_block_header));
>   	}
>   
> -	const Type *operator->() const
> +	void setEnabled(bool enabled) override
>   	{
> -		return reinterpret_cast<const Type *>(data().data());
> +		/*
> +		 * For the legacy fixed format, blocks are enabled in the
> +		 * top-level header. Delegate to the RkISP1Params class.
> +		 */
> +		if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)
> +			return params_->setBlockEnabled(type_, enabled);
> +
> +		return V4L2ParamsBlock<T>::setEnabled(enabled);
>   	}
>   
> -	Type *operator->()
> +	/*
> +	 * Override the dereference operators to return a reference to the
> +	 * actual configuration data (struct rkisp1_cif_isp_*) skipping the
> +	 * 'v4l2_params_block_header' header.
> +	 */
> +
> +	virtual const T *operator->() const override
>   	{
> -		return reinterpret_cast<Type *>(data().data());
> +		return reinterpret_cast<const T *>(cifData_.data());
>   	}
>   
> -	const Type &operator*() const &
> +	virtual T *operator->() override
>   	{
> -		return *reinterpret_cast<const Type *>(data().data());
> +		return reinterpret_cast<T *>(cifData_.data());
>   	}
>   
> -	Type &operator*() &
> +	virtual const T &operator*() const override
>   	{
> -		return *reinterpret_cast<Type *>(data().data());
> +		return *reinterpret_cast<const T *>(cifData_.data());
>   	}
> -};
> -
> -class RkISP1Params
> -{
> -public:
> -	RkISP1Params(uint32_t format, Span<uint8_t> data);
>   
> -	template<BlockType B>
> -	RkISP1ParamsBlock<B> block()
> +	virtual T &operator*() override
>   	{
> -		return RkISP1ParamsBlock<B>(this, block(B));
> +		return *reinterpret_cast<T *>(cifData_.data());
>   	}
>   
> -	uint32_t format() const { return format_; }
> -	size_t size() const { return used_; }
> -
>   private:
> -	friend class RkISP1ParamsBlockBase;
> -
> -	Span<uint8_t> block(BlockType type);
> -	void setBlockEnabled(BlockType type, bool enabled);
> -
> -	uint32_t format_;
> -
> -	Span<uint8_t> data_;
> -	size_t used_;
> -
> -	std::map<BlockType, Span<uint8_t>> blocks_;
> +	RkISP1Params *params_;
> +	BlockType type_;
> +	Span<uint8_t> cifData_;
>   };
>   
>   } /* namespace ipa::rkisp1 */
>

Hi Barnabás

On Mon, Sep 29, 2025 at 11:15:22AM +0200, Barnabás Pőcze wrote:
> Hi
>
> 2025. 09. 26. 16:39 keltezéssel, Jacopo Mondi írta:
> > The existing RkISP1Params helper classes allows the RkISP1 to handle
> > V4L2 extensible parameters format and the legacy RkIPS1-specific
> > fixed-size parameters format.
> >
> > With the introduction of v4l2-isp.h in the Linux kernel the part of
> > the RkISP1Params helper class that handles extensible parameters can
> > be generalized so that other IPA modules can use the same helpers
> > to populate a v4l2-params compatible buffer.
> >
> > Generalize the RkISP1Params class to a new libipa component named
> > V4L2Params and derive the existing RkISP1Params from it, leaving
> > in the RkISP1-specific implementation the handling of the legacy format.
> >
> > Deriving RkISP1Params from V4L2Params requires changing the size
> > associated to each block to include the size of v4l2_params_block_header
> > in the ipa:rkisp1::kBlockTypeInfo map as the V4L2Params::block()
> > implementation doesn't account for that as RkIS1Params::block()
> > implementation did.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Tested-By: Antoine Bouyer <antoine.bouyer@nxp.com>
> > ---
> >   src/ipa/libipa/meson.build     |   2 +
> >   src/ipa/libipa/v4l2_params.cpp | 262 +++++++++++++++++++++++++++++++++++++++++
> >   src/ipa/libipa/v4l2_params.h   | 142 ++++++++++++++++++++++
> >   src/ipa/rkisp1/params.cpp      |  93 +--------------
> >   src/ipa/rkisp1/params.h        | 175 +++++++++++++++------------
> >   5 files changed, 509 insertions(+), 165 deletions(-)
> >
> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > index 660be94054fa98b714b6bc586039081e45a6b4bc..4010739e710eb38aa6108eb8258c574a616bf3c0 100644
> > --- a/src/ipa/libipa/meson.build
> > +++ b/src/ipa/libipa/meson.build
> > @@ -16,6 +16,7 @@ libipa_headers = files([
> >       'lsc_polynomial.h',
> >       'lux.h',
> >       'module.h',
> > +    'v4l2_params.h',
> >       'pwl.h',
> >   ])
> > @@ -35,6 +36,7 @@ libipa_sources = files([
> >       'lsc_polynomial.cpp',
> >       'lux.cpp',
> >       'module.cpp',
> > +    'v4l2_params.cpp',
> >       'pwl.cpp',
> >   ])
> > diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..b116d0a2f79da8ab9cf695195b317f4038202262
> > --- /dev/null
> > +++ b/src/ipa/libipa/v4l2_params.cpp
> > @@ -0,0 +1,262 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2025, Ideas On Board
> > + *
> > + * V4L2 Parameters
> > + */
> > +
> > +#include "v4l2_params.h"
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa {
> > +
> > +/**
> > + * \file v4l2_params.cpp
> > + * \brief Helper class to populate an ISP configuration buffer compatible with
> > + * the generic V4L2 ISP extensible format
> > + *
> > + * The Linux kernel defines a generic buffer format for configuring ISP devices
> > + * through a set of parameters in the form of V4L2 extensible parameters. The
> > + * V4L2 extensible parameters define a serialization format for ISP parameters
> > + * that allows userspace to populate a buffer of configuration data by appending
> > + * them one after the other in a binary buffer.
> > + *
> > + * Each ISP driver compatible with the v4l2-isp format will define its own
> > + * meta-output format identifier and defines the types of the configuration data
> > + * of each ISP block that usually match the registers layout.
> > + *
> > + * The V4L2Params class represent the V4L2 extensible parameters buffer and
> > + * allows users to populate the ISP configuration blocks, represented by the
> > + * V4L2ParamBlock class instances.
> > + *
> > + * IPA implementations using this helpers should define an enumeration of ISP
> > + * blocks the IPA module supports and use a set of common abstraction to help
> > + * their derived implementation of V4L2Params translate the enumerated ISP block
> > + * identifier to the actual type of the configuration data as defined by the
> > + * kernel interface.
> > + *
> > + * As an example of this see the RkISP1 and Mali-C55 implementations.
> > + */
> > +
> > +/**
> > + * \class V4L2ParamsBlock
> > + * \brief Helper class that represents a ISP configuration block
> > + *
> > + * Each ISP function is associated with a set of configuration parameters
> > + * defined by the kernel interface.
> > + *
> > + * This class represents an ISP block configuration entry. It is constructed
> > + * with a reference to the memory area where the block configuration will be
> > + * stored in the parameters buffer. The template parameter represents
> > + * the underlying kernel-defined ISP block configuration type and allow its
> > + * user to easily cast it to said type to populate and read the configuration
> > + * parameters.
> > + *
> > + * \sa V4L2Params::block()
> > + */
> > +
> > +/**
> > + * \fn V4L2ParamsBlock::V4L2ParamsBlock()
> > + * \brief Construct a V4L2ParamsBlock with memory represented by \a data
> > + * \param[in] data A view on the memory area where the ISP block is located
> > + */
> > +
> > +/**
> > + * \fn V4L2ParamsBlock::setEnabled()
> > + * \brief Enable/disable an ISP configuration block
> > + * \param[in] enabled The enable flag
> > + */
> > +
> > +/**
> > + * \fn V4L2ParamsBlock::operator->()
> > + * \brief Access the ISP configuration block casting it to the kernel-defined
> > + * ISP configuration type
> > + *
> > + * The V4L2ParamsBlock is templated with the kernel defined ISP configuration
> > + * block type. This function allows users to easily cast a V4L2ParamsBlock to
> > + * the underlying kernel-defined type in order to easily populate or read
> > + * the ISP configuration data.
> > + *
> > + * \code{.cpp}
> > + *
> > + * // The kernel header defines the ISP configuration types, in example
> > + * // struct my_isp_awb_config_data {
> > + * //		u16 gain_ch00;
> > + * //		u16 gain_ch01;
> > + * //		u16 gain_ch10;
> > + * //		u16 gain_ch11;
> > + * //
> > + * //  }
> > + *
> > + * template<> V4L2ParamsBlock<struct my_isp_awb_config_data> awbBlock
> > + *
> > + * awbBlock->gain_ch00 = ...;
> > + * awbBlock->gain_ch01 = ...;
> > + * awbBlock->gain_ch10 = ...;
> > + * awbBlock->gain_ch11 = ...;
> > + *
> > + * \endcode
> > + *
> > + * Users of this class are not expected to create a V4L2ParamsBlock manually but
> > + * should rather use V4L2Params::block() to retrieve a reference to the memory
> > + * area used to construct a V4L2ParamsBlock<T> in their overloaded
> > + * implementation of V4L2Params::block().
> > + */
> > +
> > +/**
> > + * \fn V4L2ParamsBlock::operator->() const
> > + * \copydoc V4L2ParamsBlock::operator->()
> > + */
> > +
> > +/**
> > + * \fn V4L2ParamsBlock::operator*() const
> > + * \copydoc V4L2ParamsBlock::operator->()
> > + */
> > +
> > +/**
> > + * \fn V4L2ParamsBlock::operator*()
> > + * \copydoc V4L2ParamsBlock::operator->()
> > + */
> > +
> > +/**
> > + * \var V4L2ParamsBlock::data_
> > + * \brief Memory area reserved for the ISP configuration block
> > + */
> > +
> > + /**
> > +  * \class V4L2Params
> > +  * \brief Helper class that represent an ISP configuration buffer
> > +  *
> > + * ISP implementation compatible with v4l2-isp define their ISP configuration
> > + * buffer types compatible with the struct v4l2_params_buffer type.
> > + *
> > + * This class represents an ISP configuration buffer. It is constructed
> > + * with a reference to the memory mapped buffer that will be queued to the ISP.
> > + *
> > + * 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 'param_traits' type the helps the class associate
> > + * a block type with the actual memory area that represents the ISP
> > + * configuration block.
> > + *
> > + * \code{.cpp}
> > + *
> > + * // Define the supported ISP blocks
> > + * enum class myISPBlocks {
> > + *	Agc,
> > + *	Awb,
> > + *	...
> > + * };
> > + *
> > + * // Associated to the ISP blocks the kernel types
>
>   "Maps the C++ enum type to the kernel enum type and concrete parameter type"
>
> Maybe something like the above?
>
>
> > + * template<myISPBlocks B>
> > + * struct block_type {
> > + * };
> > + *
> > + * template<>
> > + * struct block_type<myISPBlock::Agc> {
> > + *	using type = struct my_isp_kernel_config_type_agc;
> > + *	static constextpr struct my_isp_kernel_block_type MY_ISP_TYPE_AGC;
>
> constextpr -> constexpr
>
> And shouldn't it be something like `static constexpr kernel_enum_type blockType = MY_ISP_BLOCK_TYPE_AGC` ?
>
>
>
> > + * };
> > + *
> > + * template<>
> > + * struct block_type<myISPBlock::Awb> {
> > + *	using type = struct my_isp_kernel_config_type_awb;
> > + *	static constextpr struct my_isp_kernel_block_type MY_ISP_TYPE_AWB;
>
> Same here.
>
>
> > + * };
> > + *
> > + *
> > + * // Convenience type to associated a block id to the 'block_type' overload
>
> associated -> associate
>
>
> > + * struct params_traits {
> > + * 	using id_type = myISPBlocks;
> > + * 	template<id_type Id> using id_to_details = block_type<Id>;
> > + * };
> > + *
> > + * ...
> > + *
> > + * // Derive the V4L2Params class by providing params_traits
> > + * class MyISPParams : public V4L2Params<params_traits>
> > + * {
> > + * public:
> > + * 	MyISPParams::MyISPParams(Span<uint8_t> data)
> > + * 		: V4L2Params(data, kVersion)
> > + * 	{
> > + * 	}
> > + * };
> > + *
> > + * \endcode
> > + *
> > + * Users of this class can then easily access an ISP configuration block as a
> > + * V4L2ParamsBlock instance.
> > + *
> > + * \code{.cpp}
> > + *
> > + * MyISPParams params(data);
> > + *
> > + * auto awb = params.block<myISPBlocks::AWB>();
> > + * awb->gain00 = ...;
> > + * awb->gain01 = ...;
> > + * awb->gain10 = ...;
> > + * awb->gain11 = ...;
> > + * \endcode
> > + */
> > +
> > +/**
> > + * \fn V4L2Params::V4L2Params()
> > + * \brief Construct a V4L2Params
> > + * \param[in] data Reference to the v4l2-buffer memory mapped area
> > + * \param[in] version The ISP parameters version the implementation supports
> > + */
> > +
> > +/**
> > + * \fn V4L2Params::size()
> > + * \brief Retrieve the used size of the parameters buffer (in bytes)
> > + *
> > + * The parameters buffer size is mostly used to populate the v4l2_buffer
> > + * bytesused field before queueing the buffer to the ISP.
> > + *
> > + * \return The number of bytes occupied by the ISP configuration parameters
> > + */
> > +
> > +/**
> > + * \fn V4L2Params::block()
> > + * \brief Retrieve the location of an ISP configuration block a returns it
> > + * \return A V4L2ParamsBlock instance that points to the ISP configuration block
> > + */
> > +
> > +/**
> > + * \fn V4L2Params::block(typename Traits::id_type type, unsigned int blockType, size_t blockSize)
> > + * \brief Populate an ISP configuration block a returns a reference to its
> > + * memory
> > + * \param[in] type The ISP block identifier enumerated by the IPA module
> > + * \param[in] blockType The kernel-defined ISP block identifier, used to
> > + * populate the block header
> > + * \param[in] blockSize The ISP block size, used to populate the block header
> > + *
> > + * Initialize the block header with \a blockType and \a blockSize and
> > + * returns a reference to the memory used to store an ISP configuration block.
> > + *
> > + * IPA modules that derive the V4L2Params class shall use this function to
> > + * retrieve the memory area that will be used to construct a V4L2ParamsBlock<T>
> > + * before returning it to the caller.
> > + */
> > +
> > +/**
> > + * \var V4L2Params::data_
> > + * \brief The ISP parameters buffer memory
> > + */
> > +
> > +/**
> > + * \var V4L2Params::used_
> > + * \brief The number of bytes used in the parameters buffer
> > + */
> > +
> > +/**
> > + * \var V4L2Params::blocks_
> > + * \brief Cache of ISP configuration blocks
> > + */
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..c85552ec50ae6f3a1bd8a0fede3c793df2f6da4d
> > --- /dev/null
> > +++ b/src/ipa/libipa/v4l2_params.h
> > @@ -0,0 +1,142 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2025, Ideas On Board
> > + *
> > + * V4L2 Parameters
> > + */
> > +
> > +#pragma once
> > +
> > +#include <map>
> > +#include <stdint.h>
> > +#include <string.h>
> > +
> > +#include <linux/media/v4l2-isp.h>
> > +
> > +#include <libcamera/base/span.h>
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa {
> > +
> > +template<typename T>
> > +class V4L2ParamsBlock
> > +{
> > +public:
> > +	V4L2ParamsBlock(const Span<uint8_t> data)
> > +		: data_(data)
> > +	{
> > +	}
> > +
> > +	virtual ~V4L2ParamsBlock() {}
> > +
> > +	virtual void setEnabled(bool enabled)
> > +	{
> > +		struct v4l2_params_block_header *header =
> > +			reinterpret_cast<struct v4l2_params_block_header *>(data_.data());
> > +
> > +		header->flags &= ~(V4L2_PARAMS_FL_BLOCK_ENABLE |
> > +				   V4L2_PARAMS_FL_BLOCK_DISABLE);
> > +		header->flags |= enabled ? V4L2_PARAMS_FL_BLOCK_ENABLE
> > +					 : V4L2_PARAMS_FL_BLOCK_DISABLE;
> > +	}
> > +
> > +	virtual const T *operator->() const
> > +	{
> > +		return reinterpret_cast<const T *>(data_.data());
> > +	}
> > +
> > +	virtual T *operator->()
> > +	{
> > +		return reinterpret_cast<T *>(data_.data());
> > +	}
> > +
> > +	virtual const T &operator*() const
> > +	{
> > +		return *reinterpret_cast<const T *>(data_.data());
> > +	}
> > +
> > +	virtual T &operator*()
> > +	{
> > +		return *reinterpret_cast<T *>(data_.data());
> > +	}
>
> I have been thinking about these virtual functions. And as far as I can tell,
> at least with the current code, there seems to be no need for them. `RkISP1ParamsBlock`
> also does not need to inherit `V4L2ParamsBlock` because, at least at the moment,
> there is no code that operates on these parameter blocks in a "generic" fashion.

To make sure we're on the same page. Are you suggesting:

`RkISP1ParamsBlock` won't inherit from `V4L2ParamsBlock` so we can
remove 'virtual'

or

we should remove 'virtual' even if we maintain inheritance ?

From all functions or from setEnabled() only ?

> And even if there was, the specific type is encoded in a template parameter, so
> any such generic code would need to be templated at least to some degree.
>
> The above suggests to me that we could probably remove the virtual functions.
> One option could be the following:
>   * do not make `RkISP1ParamsBlock` inherit `V4L2ParamsBlock`
>   * remove the "virtual" from the functions
>   * move the `V4L2ParamsBlock<T>::setEnabled()` implementation into a separate function
>   * use that function to implement `V4L2ParamsBlock<T>::setEnabled()`
>     and `RkISP1ParamsBlock<T>::setEnabled()`
>
> Thoughts? Or maybe just keep them virtual?

What's the drawback of having them virtual ?

>
>
> Regards,
> Barnabás Pőcze
>
>
> > +
> > +protected:
> > +	Span<uint8_t> data_;
> > +};
> > +
> > +template<typename Traits>
> > +class V4L2Params
> > +{
> > +public:
> > +	V4L2Params(Span<uint8_t> data, unsigned int version)
> > +		: data_(data)
> > +	{
> > +		struct v4l2_params_buffer *cfg =
> > +			reinterpret_cast<struct v4l2_params_buffer *>(data_.data());
> > +		cfg->data_size = 0;
> > +		cfg->version = version;
> > +		used_ = offsetof(struct v4l2_params_buffer, data);
> > +	}
> > +
> > +	size_t size() const { return used_; }
> > +
> > +	template<typename Traits::id_type Id>
> > +	auto block()
> > +	{
> > +		using Details = typename Traits::template id_to_details<Id>;
> > +
> > +		using Type = typename Details::type;
> > +		constexpr auto kernelId = Details::blockType;
> > +
> > +		auto data = block(Id, kernelId, sizeof(Type));
> > +		return V4L2ParamsBlock<Type>(data);
> > +	}
> > +
> > +protected:
> > +	Span<uint8_t> block(typename Traits::id_type type,
> > +			    unsigned int blockType, size_t blockSize)
> > +	{
> > +		/*
> > +		 * Look up the block in the cache first. If an algorithm
> > +		 * requests the same block type twice, it should get the same
> > +		 * block.
> > +		 */
> > +		auto cacheIt = blocks_.find(type);
> > +		if (cacheIt != blocks_.end())
> > +			return cacheIt->second;
> > +
> > +		/* Make sure we don't run out of space. */
> > +		if (blockSize > data_.size() - used_)
> > +			return {};
> > +
> > +		/* Allocate a new block, clear its memory, and initialize its header. */
> > +		Span<uint8_t> block = data_.subspan(used_, blockSize);
> > +		used_ += blockSize;
> > +
> > +		struct v4l2_params_buffer *cfg =
> > +			reinterpret_cast<struct v4l2_params_buffer *>(data_.data());
> > +		cfg->data_size += blockSize;
> > +
> > +		memset(block.data(), 0, block.size());
> > +
> > +		struct v4l2_params_block_header *header =
> > +			reinterpret_cast<struct v4l2_params_block_header *>(block.data());
> > +		header->type = blockType;
> > +		header->size = block.size();
> > +
> > +		/* Update the cache. */
> > +		blocks_[type] = block;
> > +
> > +		return block;
> > +	}
> > +
> > +	Span<uint8_t> data_;
> > +	size_t used_;
> > +
> > +	std::map<typename Traits::id_type, Span<uint8_t>> blocks_;
> > +};
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp
> > index 5edb36c91b87859d02c0a8b41efe977ff048def5..7040207c26557aa278050a1f7232cc6c380505b1 100644
> > --- a/src/ipa/rkisp1/params.cpp
> > +++ b/src/ipa/rkisp1/params.cpp
> > @@ -35,7 +35,7 @@ struct BlockTypeInfo {
> >   #define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit)			\
> >   	{ BlockType::block, {							\
> >   		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id,				\
> > -		sizeof(struct rkisp1_cif_isp_##type##_config),			\
> > +		sizeof(struct rkisp1_ext_params_##type##_config),		\
> >   		offsetof(struct rkisp1_params_cfg, category.type##_config),	\
> >   		RKISP1_CIF_ISP_MODULE_##bit,					\
> >   	} }
> > @@ -49,7 +49,7 @@ struct BlockTypeInfo {
> >   #define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type)				\
> >   	{ BlockType::block, {							\
> >   		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id,				\
> > -		sizeof(struct rkisp1_cif_isp_##type##_config),			\
> > +		sizeof(struct rkisp1_ext_params_##type##_config),		\
> >   		0, 0,								\
> >   	} }
> > @@ -79,56 +79,6 @@ const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = {
> >   } /* namespace */
> > -RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,
> > -					     const Span<uint8_t> &data)
> > -	: params_(params), type_(type)
> > -{
> > -	if (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {
> > -		header_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header));
> > -		data_ = data.subspan(sizeof(rkisp1_ext_params_block_header));
> > -	} else {
> > -		data_ = data;
> > -	}
> > -}
> > -
> > -void RkISP1ParamsBlockBase::setEnabled(bool enabled)
> > -{
> > -	/*
> > -	 * For the legacy fixed format, blocks are enabled in the top-level
> > -	 * header. Delegate to the RkISP1Params class.
> > -	 */
> > -	if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)
> > -		return params_->setBlockEnabled(type_, enabled);
> > -
> > -	/*
> > -	 * For the extensible format, set the enable and disable flags in the
> > -	 * block header directly.
> > -	 */
> > -	struct rkisp1_ext_params_block_header *header =
> > -		reinterpret_cast<struct rkisp1_ext_params_block_header *>(header_.data());
> > -	header->flags &= ~(RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |
> > -			   RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE);
> > -	header->flags |= enabled ? RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE
> > -				 : RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE;
> > -}
> > -
> > -RkISP1Params::RkISP1Params(uint32_t format, Span<uint8_t> data)
> > -	: format_(format), data_(data), used_(0)
> > -{
> > -	if (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {
> > -		struct rkisp1_ext_params_cfg *cfg =
> > -			reinterpret_cast<struct rkisp1_ext_params_cfg *>(data.data());
> > -
> > -		cfg->version = RKISP1_EXT_PARAM_BUFFER_V1;
> > -		cfg->data_size = 0;
> > -
> > -		used_ += offsetof(struct rkisp1_ext_params_cfg, data);
> > -	} else {
> > -		memset(data.data(), 0, data.size());
> > -		used_ = sizeof(struct rkisp1_params_cfg);
> > -	}
> > -}
> > -
> >   void RkISP1Params::setBlockEnabled(BlockType type, bool enabled)
> >   {
> >   	const BlockTypeInfo &info = kBlockTypeInfo.at(type);
> > @@ -178,44 +128,7 @@ Span<uint8_t> RkISP1Params::block(BlockType type)
> >   		return data_.subspan(info.offset, info.size);
> >   	}
> > -	/*
> > -	 * For the extensible format, allocate memory for the block, including
> > -	 * the header. Look up the block in the cache first. If an algorithm
> > -	 * requests the same block type twice, it should get the same block.
> > -	 */
> > -	auto cacheIt = blocks_.find(type);
> > -	if (cacheIt != blocks_.end())
> > -		return cacheIt->second;
> > -
> > -	/* Make sure we don't run out of space. */
> > -	size_t size = sizeof(struct rkisp1_ext_params_block_header)
> > -		    + ((info.size + 7) & ~7);
> > -	if (size > data_.size() - used_) {
> > -		LOG(RkISP1Params, Error)
> > -			<< "Out of memory to allocate block type "
> > -			<< utils::to_underlying(type);
> > -		return {};
> > -	}
> > -
> > -	/* Allocate a new block, clear its memory, and initialize its header. */
> > -	Span<uint8_t> block = data_.subspan(used_, size);
> > -	used_ += size;
> > -
> > -	struct rkisp1_ext_params_cfg *cfg =
> > -		reinterpret_cast<struct rkisp1_ext_params_cfg *>(data_.data());
> > -	cfg->data_size += size;
> > -
> > -	memset(block.data(), 0, block.size());
> > -
> > -	struct rkisp1_ext_params_block_header *header =
> > -		reinterpret_cast<struct rkisp1_ext_params_block_header *>(block.data());
> > -	header->type = info.type;
> > -	header->size = block.size();
> > -
> > -	/* Update the cache. */
> > -	blocks_[type] = block;
> > -
> > -	return block;
> > +	return V4L2Params::block(type, info.type, info.size);
> >   }
> >   } /* namespace ipa::rkisp1 */
> > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h
> > index 2e60528d102ec44a31417d4b146e74cace363efa..a7fbbc581c6df8a92dcc4fcfe1e620da6c469915 100644
> > --- a/src/ipa/rkisp1/params.h
> > +++ b/src/ipa/rkisp1/params.h
> > @@ -7,13 +7,10 @@
> >   #pragma once
> > -#include <map>
> > -#include <stdint.h>
> > -
> >   #include <linux/rkisp1-config.h>
> > +#include <linux/videodev2.h>
> > -#include <libcamera/base/class.h>
> > -#include <libcamera/base/span.h>
> > +#include <libipa/v4l2_params.h>
> >   namespace libcamera {
> > @@ -49,115 +46,143 @@ template<BlockType B>
> >   struct block_type {
> >   };
> > -#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)		\
> > +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct, id)		\
> >   template<>								\
> >   struct block_type<BlockType::blockType> {				\
> >   	using type = struct rkisp1_cif_isp_##blockStruct##_config;	\
> > +	static constexpr rkisp1_ext_params_block_type blockType =	\
> > +		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id;			\
> >   };
> > -RKISP1_DEFINE_BLOCK_TYPE(Bls, bls)
> > -RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc)
> > -RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg)
> > -RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain)
> > -RKISP1_DEFINE_BLOCK_TYPE(Flt, flt)
> > -RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm)
> > -RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk)
> > -RKISP1_DEFINE_BLOCK_TYPE(Goc, goc)
> > -RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf)
> > -RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength)
> > -RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc)
> > -RKISP1_DEFINE_BLOCK_TYPE(Ie, ie)
> > -RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc)
> > -RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas)
> > -RKISP1_DEFINE_BLOCK_TYPE(Hst, hst)
> > -RKISP1_DEFINE_BLOCK_TYPE(Aec, aec)
> > -RKISP1_DEFINE_BLOCK_TYPE(Afc, afc)
> > -RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls)
> > -RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve)
> > -RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve)
> > -RKISP1_DEFINE_BLOCK_TYPE(Wdr, wdr)
> > +RKISP1_DEFINE_BLOCK_TYPE(Bls, bls, BLS)
> > +RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc, DPCC)
> > +RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg, SDG)
> > +RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain, AWB_GAIN)
> > +RKISP1_DEFINE_BLOCK_TYPE(Flt, flt, FLT)
> > +RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm, BDM)
> > +RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk, CTK)
> > +RKISP1_DEFINE_BLOCK_TYPE(Goc, goc, GOC)
> > +RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf, DPF)
> > +RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength, DPF_STRENGTH)
> > +RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc, CPROC)
> > +RKISP1_DEFINE_BLOCK_TYPE(Ie, ie, IE)
> > +RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc, LSC)
> > +RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas, AWB_MEAS)
> > +RKISP1_DEFINE_BLOCK_TYPE(Hst, hst, HST_MEAS)
> > +RKISP1_DEFINE_BLOCK_TYPE(Aec, aec, AEC_MEAS)
> > +RKISP1_DEFINE_BLOCK_TYPE(Afc, afc, AFC_MEAS)
> > +RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls, COMPAND_BLS)
> > +RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve, COMPAND_EXPAND)
> > +RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve, COMPAND_COMPRESS)
> > +RKISP1_DEFINE_BLOCK_TYPE(Wdr, wdr, WDR)
> > +
> > +struct params_traits {
> > +	using id_type = BlockType;
> > +
> > +	template<id_type Id>
> > +	using id_to_details = block_type<Id>;
> > +};
> >   } /* namespace details */
> > -class RkISP1Params;
> > +template<typename T>
> > +class RkISP1ParamsBlock;
> > -class RkISP1ParamsBlockBase
> > +class RkISP1Params : public V4L2Params<details::params_traits>
> >   {
> >   public:
> > -	RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,
> > -			      const Span<uint8_t> &data);
> > +	static constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1;
> > +
> > +	RkISP1Params(uint32_t format, Span<uint8_t> data)
> > +		: V4L2Params(data, kVersion), format_(format)
> > +	{
> > +		if (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {
> > +			memset(data.data(), 0, data.size());
> > +			used_ = sizeof(struct rkisp1_params_cfg);
> > +		}
> > +	}
> > +
> > +	template<details::params_traits::id_type id>
> > +	auto block()
> > +	{
> > +		using Type = typename details::block_type<id>::type;
> > -	Span<uint8_t> data() const { return data_; }
> > +		return RkISP1ParamsBlock<Type>(this, id, block(id));
> > +	}
> > -	void setEnabled(bool enabled);
> > +	uint32_t format() const { return format_; }
> > +	void setBlockEnabled(BlockType type, bool enabled);
> >   private:
> > -	LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase)
> > +	Span<uint8_t> block(BlockType type);
> > -	RkISP1Params *params_;
> > -	BlockType type_;
> > -	Span<uint8_t> header_;
> > -	Span<uint8_t> data_;
> > +	uint32_t format_;
> >   };
> > -template<BlockType B>
> > -class RkISP1ParamsBlock : public RkISP1ParamsBlockBase
> > +template<typename T>
> > +class RkISP1ParamsBlock : public V4L2ParamsBlock<T>
> >   {
> >   public:
> > -	using Type = typename details::block_type<B>::type;
> > -
> > -	RkISP1ParamsBlock(RkISP1Params *params, const Span<uint8_t> &data)
> > -		: RkISP1ParamsBlockBase(params, B, data)
> > +	RkISP1ParamsBlock(RkISP1Params *params, BlockType type,
> > +			  const Span<uint8_t> data)
> > +		: V4L2ParamsBlock<T>(data)
> >   	{
> > +		params_ = params;
> > +		type_ = type;
> > +
> > +		/*
> > +		 * cifData_ points to the actual configuration data
> > +		 * (struct rkisp1_cif_isp_*) which is not prefixed by any header,
> > +		 * for the legacy fixed format.
> > +		 */
> > +		if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)
> > +			cifData_ = data;
> > +		else
> > +			cifData_ = data.subspan(sizeof(v4l2_params_block_header));
> >   	}
> > -	const Type *operator->() const
> > +	void setEnabled(bool enabled) override
> >   	{
> > -		return reinterpret_cast<const Type *>(data().data());
> > +		/*
> > +		 * For the legacy fixed format, blocks are enabled in the
> > +		 * top-level header. Delegate to the RkISP1Params class.
> > +		 */
> > +		if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)
> > +			return params_->setBlockEnabled(type_, enabled);
> > +
> > +		return V4L2ParamsBlock<T>::setEnabled(enabled);
> >   	}
> > -	Type *operator->()
> > +	/*
> > +	 * Override the dereference operators to return a reference to the
> > +	 * actual configuration data (struct rkisp1_cif_isp_*) skipping the
> > +	 * 'v4l2_params_block_header' header.
> > +	 */
> > +
> > +	virtual const T *operator->() const override
> >   	{
> > -		return reinterpret_cast<Type *>(data().data());
> > +		return reinterpret_cast<const T *>(cifData_.data());
> >   	}
> > -	const Type &operator*() const &
> > +	virtual T *operator->() override
> >   	{
> > -		return *reinterpret_cast<const Type *>(data().data());
> > +		return reinterpret_cast<T *>(cifData_.data());
> >   	}
> > -	Type &operator*() &
> > +	virtual const T &operator*() const override
> >   	{
> > -		return *reinterpret_cast<Type *>(data().data());
> > +		return *reinterpret_cast<const T *>(cifData_.data());
> >   	}
> > -};
> > -
> > -class RkISP1Params
> > -{
> > -public:
> > -	RkISP1Params(uint32_t format, Span<uint8_t> data);
> > -	template<BlockType B>
> > -	RkISP1ParamsBlock<B> block()
> > +	virtual T &operator*() override
> >   	{
> > -		return RkISP1ParamsBlock<B>(this, block(B));
> > +		return *reinterpret_cast<T *>(cifData_.data());
> >   	}
> > -	uint32_t format() const { return format_; }
> > -	size_t size() const { return used_; }
> > -
> >   private:
> > -	friend class RkISP1ParamsBlockBase;
> > -
> > -	Span<uint8_t> block(BlockType type);
> > -	void setBlockEnabled(BlockType type, bool enabled);
> > -
> > -	uint32_t format_;
> > -
> > -	Span<uint8_t> data_;
> > -	size_t used_;
> > -
> > -	std::map<BlockType, Span<uint8_t>> blocks_;
> > +	RkISP1Params *params_;
> > +	BlockType type_;
> > +	Span<uint8_t> cifData_;
> >   };
> >   } /* namespace ipa::rkisp1 */
> >
>

2025. 09. 30. 11:49 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Mon, Sep 29, 2025 at 11:15:22AM +0200, Barnabás Pőcze wrote:
>> Hi
>>
>> 2025. 09. 26. 16:39 keltezéssel, Jacopo Mondi írta:
>>> The existing RkISP1Params helper classes allows the RkISP1 to handle
>>> V4L2 extensible parameters format and the legacy RkIPS1-specific
>>> fixed-size parameters format.
>>>
>>> With the introduction of v4l2-isp.h in the Linux kernel the part of
>>> the RkISP1Params helper class that handles extensible parameters can
>>> be generalized so that other IPA modules can use the same helpers
>>> to populate a v4l2-params compatible buffer.
>>>
>>> Generalize the RkISP1Params class to a new libipa component named
>>> V4L2Params and derive the existing RkISP1Params from it, leaving
>>> in the RkISP1-specific implementation the handling of the legacy format.
>>>
>>> Deriving RkISP1Params from V4L2Params requires changing the size
>>> associated to each block to include the size of v4l2_params_block_header
>>> in the ipa:rkisp1::kBlockTypeInfo map as the V4L2Params::block()
>>> implementation doesn't account for that as RkIS1Params::block()
>>> implementation did.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>> Tested-By: Antoine Bouyer <antoine.bouyer@nxp.com>
>>> ---
>>>    src/ipa/libipa/meson.build     |   2 +
>>>    src/ipa/libipa/v4l2_params.cpp | 262 +++++++++++++++++++++++++++++++++++++++++
>>>    src/ipa/libipa/v4l2_params.h   | 142 ++++++++++++++++++++++
>>>    src/ipa/rkisp1/params.cpp      |  93 +--------------
>>>    src/ipa/rkisp1/params.h        | 175 +++++++++++++++------------
>>>    5 files changed, 509 insertions(+), 165 deletions(-)
>>>
>>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
>>> index 660be94054fa98b714b6bc586039081e45a6b4bc..4010739e710eb38aa6108eb8258c574a616bf3c0 100644
>>> --- a/src/ipa/libipa/meson.build
>>> +++ b/src/ipa/libipa/meson.build
>>> @@ -16,6 +16,7 @@ libipa_headers = files([
>>>        'lsc_polynomial.h',
>>>        'lux.h',
>>>        'module.h',
>>> +    'v4l2_params.h',
>>>        'pwl.h',
>>>    ])
>>> @@ -35,6 +36,7 @@ libipa_sources = files([
>>>        'lsc_polynomial.cpp',
>>>        'lux.cpp',
>>>        'module.cpp',
>>> +    'v4l2_params.cpp',
>>>        'pwl.cpp',
>>>    ])
>>> diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..b116d0a2f79da8ab9cf695195b317f4038202262
>>> --- /dev/null
>>> +++ b/src/ipa/libipa/v4l2_params.cpp
>>> @@ -0,0 +1,262 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2025, Ideas On Board
>>> + *
>>> + * V4L2 Parameters
>>> + */
>>> +
>>> +#include "v4l2_params.h"
>>> +
>>> +namespace libcamera {
>>> +
>>> +namespace ipa {
>>> +
>>> +/**
>>> + * \file v4l2_params.cpp
>>> + * \brief Helper class to populate an ISP configuration buffer compatible with
>>> + * the generic V4L2 ISP extensible format
>>> + *
>>> + * The Linux kernel defines a generic buffer format for configuring ISP devices
>>> + * through a set of parameters in the form of V4L2 extensible parameters. The
>>> + * V4L2 extensible parameters define a serialization format for ISP parameters
>>> + * that allows userspace to populate a buffer of configuration data by appending
>>> + * them one after the other in a binary buffer.
>>> + *
>>> + * Each ISP driver compatible with the v4l2-isp format will define its own
>>> + * meta-output format identifier and defines the types of the configuration data
>>> + * of each ISP block that usually match the registers layout.
>>> + *
>>> + * The V4L2Params class represent the V4L2 extensible parameters buffer and
>>> + * allows users to populate the ISP configuration blocks, represented by the
>>> + * V4L2ParamBlock class instances.
>>> + *
>>> + * IPA implementations using this helpers should define an enumeration of ISP
>>> + * blocks the IPA module supports and use a set of common abstraction to help
>>> + * their derived implementation of V4L2Params translate the enumerated ISP block
>>> + * identifier to the actual type of the configuration data as defined by the
>>> + * kernel interface.
>>> + *
>>> + * As an example of this see the RkISP1 and Mali-C55 implementations.
>>> + */
>>> +
>>> +/**
>>> + * \class V4L2ParamsBlock
>>> + * \brief Helper class that represents a ISP configuration block
>>> + *
>>> + * Each ISP function is associated with a set of configuration parameters
>>> + * defined by the kernel interface.
>>> + *
>>> + * This class represents an ISP block configuration entry. It is constructed
>>> + * with a reference to the memory area where the block configuration will be
>>> + * stored in the parameters buffer. The template parameter represents
>>> + * the underlying kernel-defined ISP block configuration type and allow its
>>> + * user to easily cast it to said type to populate and read the configuration
>>> + * parameters.
>>> + *
>>> + * \sa V4L2Params::block()
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2ParamsBlock::V4L2ParamsBlock()
>>> + * \brief Construct a V4L2ParamsBlock with memory represented by \a data
>>> + * \param[in] data A view on the memory area where the ISP block is located
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2ParamsBlock::setEnabled()
>>> + * \brief Enable/disable an ISP configuration block
>>> + * \param[in] enabled The enable flag
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2ParamsBlock::operator->()
>>> + * \brief Access the ISP configuration block casting it to the kernel-defined
>>> + * ISP configuration type
>>> + *
>>> + * The V4L2ParamsBlock is templated with the kernel defined ISP configuration
>>> + * block type. This function allows users to easily cast a V4L2ParamsBlock to
>>> + * the underlying kernel-defined type in order to easily populate or read
>>> + * the ISP configuration data.
>>> + *
>>> + * \code{.cpp}
>>> + *
>>> + * // The kernel header defines the ISP configuration types, in example
>>> + * // struct my_isp_awb_config_data {
>>> + * //		u16 gain_ch00;
>>> + * //		u16 gain_ch01;
>>> + * //		u16 gain_ch10;
>>> + * //		u16 gain_ch11;
>>> + * //
>>> + * //  }
>>> + *
>>> + * template<> V4L2ParamsBlock<struct my_isp_awb_config_data> awbBlock
>>> + *
>>> + * awbBlock->gain_ch00 = ...;
>>> + * awbBlock->gain_ch01 = ...;
>>> + * awbBlock->gain_ch10 = ...;
>>> + * awbBlock->gain_ch11 = ...;
>>> + *
>>> + * \endcode
>>> + *
>>> + * Users of this class are not expected to create a V4L2ParamsBlock manually but
>>> + * should rather use V4L2Params::block() to retrieve a reference to the memory
>>> + * area used to construct a V4L2ParamsBlock<T> in their overloaded
>>> + * implementation of V4L2Params::block().
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2ParamsBlock::operator->() const
>>> + * \copydoc V4L2ParamsBlock::operator->()
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2ParamsBlock::operator*() const
>>> + * \copydoc V4L2ParamsBlock::operator->()
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2ParamsBlock::operator*()
>>> + * \copydoc V4L2ParamsBlock::operator->()
>>> + */
>>> +
>>> +/**
>>> + * \var V4L2ParamsBlock::data_
>>> + * \brief Memory area reserved for the ISP configuration block
>>> + */
>>> +
>>> + /**
>>> +  * \class V4L2Params
>>> +  * \brief Helper class that represent an ISP configuration buffer
>>> +  *
>>> + * ISP implementation compatible with v4l2-isp define their ISP configuration
>>> + * buffer types compatible with the struct v4l2_params_buffer type.
>>> + *
>>> + * This class represents an ISP configuration buffer. It is constructed
>>> + * with a reference to the memory mapped buffer that will be queued to the ISP.
>>> + *
>>> + * 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 'param_traits' type the helps the class associate
>>> + * a block type with the actual memory area that represents the ISP
>>> + * configuration block.
>>> + *
>>> + * \code{.cpp}
>>> + *
>>> + * // Define the supported ISP blocks
>>> + * enum class myISPBlocks {
>>> + *	Agc,
>>> + *	Awb,
>>> + *	...
>>> + * };
>>> + *
>>> + * // Associated to the ISP blocks the kernel types
>>
>>    "Maps the C++ enum type to the kernel enum type and concrete parameter type"
>>
>> Maybe something like the above?
>>
>>
>>> + * template<myISPBlocks B>
>>> + * struct block_type {
>>> + * };
>>> + *
>>> + * template<>
>>> + * struct block_type<myISPBlock::Agc> {
>>> + *	using type = struct my_isp_kernel_config_type_agc;
>>> + *	static constextpr struct my_isp_kernel_block_type MY_ISP_TYPE_AGC;
>>
>> constextpr -> constexpr
>>
>> And shouldn't it be something like `static constexpr kernel_enum_type blockType = MY_ISP_BLOCK_TYPE_AGC` ?
>>
>>
>>
>>> + * };
>>> + *
>>> + * template<>
>>> + * struct block_type<myISPBlock::Awb> {
>>> + *	using type = struct my_isp_kernel_config_type_awb;
>>> + *	static constextpr struct my_isp_kernel_block_type MY_ISP_TYPE_AWB;
>>
>> Same here.
>>
>>
>>> + * };
>>> + *
>>> + *
>>> + * // Convenience type to associated a block id to the 'block_type' overload
>>
>> associated -> associate
>>
>>
>>> + * struct params_traits {
>>> + * 	using id_type = myISPBlocks;
>>> + * 	template<id_type Id> using id_to_details = block_type<Id>;
>>> + * };
>>> + *
>>> + * ...
>>> + *
>>> + * // Derive the V4L2Params class by providing params_traits
>>> + * class MyISPParams : public V4L2Params<params_traits>
>>> + * {
>>> + * public:
>>> + * 	MyISPParams::MyISPParams(Span<uint8_t> data)
>>> + * 		: V4L2Params(data, kVersion)
>>> + * 	{
>>> + * 	}
>>> + * };
>>> + *
>>> + * \endcode
>>> + *
>>> + * Users of this class can then easily access an ISP configuration block as a
>>> + * V4L2ParamsBlock instance.
>>> + *
>>> + * \code{.cpp}
>>> + *
>>> + * MyISPParams params(data);
>>> + *
>>> + * auto awb = params.block<myISPBlocks::AWB>();
>>> + * awb->gain00 = ...;
>>> + * awb->gain01 = ...;
>>> + * awb->gain10 = ...;
>>> + * awb->gain11 = ...;
>>> + * \endcode
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2Params::V4L2Params()
>>> + * \brief Construct a V4L2Params
>>> + * \param[in] data Reference to the v4l2-buffer memory mapped area
>>> + * \param[in] version The ISP parameters version the implementation supports
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2Params::size()
>>> + * \brief Retrieve the used size of the parameters buffer (in bytes)
>>> + *
>>> + * The parameters buffer size is mostly used to populate the v4l2_buffer
>>> + * bytesused field before queueing the buffer to the ISP.
>>> + *
>>> + * \return The number of bytes occupied by the ISP configuration parameters
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2Params::block()
>>> + * \brief Retrieve the location of an ISP configuration block a returns it
>>> + * \return A V4L2ParamsBlock instance that points to the ISP configuration block
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2Params::block(typename Traits::id_type type, unsigned int blockType, size_t blockSize)
>>> + * \brief Populate an ISP configuration block a returns a reference to its
>>> + * memory
>>> + * \param[in] type The ISP block identifier enumerated by the IPA module
>>> + * \param[in] blockType The kernel-defined ISP block identifier, used to
>>> + * populate the block header
>>> + * \param[in] blockSize The ISP block size, used to populate the block header
>>> + *
>>> + * Initialize the block header with \a blockType and \a blockSize and
>>> + * returns a reference to the memory used to store an ISP configuration block.
>>> + *
>>> + * IPA modules that derive the V4L2Params class shall use this function to
>>> + * retrieve the memory area that will be used to construct a V4L2ParamsBlock<T>
>>> + * before returning it to the caller.
>>> + */
>>> +
>>> +/**
>>> + * \var V4L2Params::data_
>>> + * \brief The ISP parameters buffer memory
>>> + */
>>> +
>>> +/**
>>> + * \var V4L2Params::used_
>>> + * \brief The number of bytes used in the parameters buffer
>>> + */
>>> +
>>> +/**
>>> + * \var V4L2Params::blocks_
>>> + * \brief Cache of ISP configuration blocks
>>> + */
>>> +
>>> +} /* namespace ipa */
>>> +
>>> +} /* namespace libcamera */
>>> diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..c85552ec50ae6f3a1bd8a0fede3c793df2f6da4d
>>> --- /dev/null
>>> +++ b/src/ipa/libipa/v4l2_params.h
>>> @@ -0,0 +1,142 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2025, Ideas On Board
>>> + *
>>> + * V4L2 Parameters
>>> + */
>>> +
>>> +#pragma once
>>> +
>>> +#include <map>
>>> +#include <stdint.h>
>>> +#include <string.h>
>>> +
>>> +#include <linux/media/v4l2-isp.h>
>>> +
>>> +#include <libcamera/base/span.h>
>>> +
>>> +namespace libcamera {
>>> +
>>> +namespace ipa {
>>> +
>>> +template<typename T>
>>> +class V4L2ParamsBlock
>>> +{
>>> +public:
>>> +	V4L2ParamsBlock(const Span<uint8_t> data)
>>> +		: data_(data)
>>> +	{
>>> +	}
>>> +
>>> +	virtual ~V4L2ParamsBlock() {}
>>> +
>>> +	virtual void setEnabled(bool enabled)
>>> +	{
>>> +		struct v4l2_params_block_header *header =
>>> +			reinterpret_cast<struct v4l2_params_block_header *>(data_.data());
>>> +
>>> +		header->flags &= ~(V4L2_PARAMS_FL_BLOCK_ENABLE |
>>> +				   V4L2_PARAMS_FL_BLOCK_DISABLE);
>>> +		header->flags |= enabled ? V4L2_PARAMS_FL_BLOCK_ENABLE
>>> +					 : V4L2_PARAMS_FL_BLOCK_DISABLE;
>>> +	}
>>> +
>>> +	virtual const T *operator->() const
>>> +	{
>>> +		return reinterpret_cast<const T *>(data_.data());
>>> +	}
>>> +
>>> +	virtual T *operator->()
>>> +	{
>>> +		return reinterpret_cast<T *>(data_.data());
>>> +	}
>>> +
>>> +	virtual const T &operator*() const
>>> +	{
>>> +		return *reinterpret_cast<const T *>(data_.data());
>>> +	}
>>> +
>>> +	virtual T &operator*()
>>> +	{
>>> +		return *reinterpret_cast<T *>(data_.data());
>>> +	}
>>
>> I have been thinking about these virtual functions. And as far as I can tell,
>> at least with the current code, there seems to be no need for them. `RkISP1ParamsBlock`
>> also does not need to inherit `V4L2ParamsBlock` because, at least at the moment,
>> there is no code that operates on these parameter blocks in a "generic" fashion.
> 
> To make sure we're on the same page. Are you suggesting:
> 
> `RkISP1ParamsBlock` won't inherit from `V4L2ParamsBlock` so we can
> remove 'virtual'

This one.


> 
> or
> 
> we should remove 'virtual' even if we maintain inheritance ?
> 
>  From all functions or from setEnabled() only ?

All functions.


> 
>> And even if there was, the specific type is encoded in a template parameter, so
>> any such generic code would need to be templated at least to some degree.
>>
>> The above suggests to me that we could probably remove the virtual functions.
>> One option could be the following:
>>    * do not make `RkISP1ParamsBlock` inherit `V4L2ParamsBlock`
>>    * remove the "virtual" from the functions
>>    * move the `V4L2ParamsBlock<T>::setEnabled()` implementation into a separate function
>>    * use that function to implement `V4L2ParamsBlock<T>::setEnabled()`
>>      and `RkISP1ParamsBlock<T>::setEnabled()`
>>
>> Thoughts? Or maybe just keep them virtual?
> 
> What's the drawback of having them virtual ?

Primarily the associated overhead. Especially if something like `operator->` is virtual,
which is used multiple times usually. But to be fair, as far as I can see these types are
mostly returned by value and used directly, so a "reasonable" compiler should be able to
devirtualize the calls. So it is probably fine.


> 
>>
>>
>> Regards,
>> Barnabás Pőcze
>>
>>
>>> +
>>> +protected:
>>> +	Span<uint8_t> data_;
>>> +};
>>> +
>>> +template<typename Traits>
>>> +class V4L2Params
>>> +{
>>> +public:
>>> +	V4L2Params(Span<uint8_t> data, unsigned int version)
>>> +		: data_(data)
>>> +	{
>>> +		struct v4l2_params_buffer *cfg =
>>> +			reinterpret_cast<struct v4l2_params_buffer *>(data_.data());
>>> +		cfg->data_size = 0;
>>> +		cfg->version = version;
>>> +		used_ = offsetof(struct v4l2_params_buffer, data);
>>> +	}
>>> +
>>> +	size_t size() const { return used_; }
>>> +
>>> +	template<typename Traits::id_type Id>
>>> +	auto block()
>>> +	{
>>> +		using Details = typename Traits::template id_to_details<Id>;
>>> +
>>> +		using Type = typename Details::type;
>>> +		constexpr auto kernelId = Details::blockType;
>>> +
>>> +		auto data = block(Id, kernelId, sizeof(Type));
>>> +		return V4L2ParamsBlock<Type>(data);
>>> +	}
>>> +
>>> +protected:
>>> +	Span<uint8_t> block(typename Traits::id_type type,
>>> +			    unsigned int blockType, size_t blockSize)
>>> +	{
>>> +		/*
>>> +		 * Look up the block in the cache first. If an algorithm
>>> +		 * requests the same block type twice, it should get the same
>>> +		 * block.
>>> +		 */
>>> +		auto cacheIt = blocks_.find(type);
>>> +		if (cacheIt != blocks_.end())
>>> +			return cacheIt->second;
>>> +
>>> +		/* Make sure we don't run out of space. */
>>> +		if (blockSize > data_.size() - used_)
>>> +			return {};
>>> +
>>> +		/* Allocate a new block, clear its memory, and initialize its header. */
>>> +		Span<uint8_t> block = data_.subspan(used_, blockSize);
>>> +		used_ += blockSize;
>>> +
>>> +		struct v4l2_params_buffer *cfg =
>>> +			reinterpret_cast<struct v4l2_params_buffer *>(data_.data());
>>> +		cfg->data_size += blockSize;
>>> +
>>> +		memset(block.data(), 0, block.size());
>>> +
>>> +		struct v4l2_params_block_header *header =
>>> +			reinterpret_cast<struct v4l2_params_block_header *>(block.data());
>>> +		header->type = blockType;
>>> +		header->size = block.size();
>>> +
>>> +		/* Update the cache. */
>>> +		blocks_[type] = block;
>>> +
>>> +		return block;
>>> +	}
>>> +
>>> +	Span<uint8_t> data_;
>>> +	size_t used_;
>>> +
>>> +	std::map<typename Traits::id_type, Span<uint8_t>> blocks_;
>>> +};
>>> +
>>> +} /* namespace ipa */
>>> +
>>> +} /* namespace libcamera */
>>> diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp
>>> index 5edb36c91b87859d02c0a8b41efe977ff048def5..7040207c26557aa278050a1f7232cc6c380505b1 100644
>>> --- a/src/ipa/rkisp1/params.cpp
>>> +++ b/src/ipa/rkisp1/params.cpp
>>> @@ -35,7 +35,7 @@ struct BlockTypeInfo {
>>>    #define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit)			\
>>>    	{ BlockType::block, {							\
>>>    		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id,				\
>>> -		sizeof(struct rkisp1_cif_isp_##type##_config),			\
>>> +		sizeof(struct rkisp1_ext_params_##type##_config),		\
>>>    		offsetof(struct rkisp1_params_cfg, category.type##_config),	\
>>>    		RKISP1_CIF_ISP_MODULE_##bit,					\
>>>    	} }
>>> @@ -49,7 +49,7 @@ struct BlockTypeInfo {
>>>    #define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type)				\
>>>    	{ BlockType::block, {							\
>>>    		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id,				\
>>> -		sizeof(struct rkisp1_cif_isp_##type##_config),			\
>>> +		sizeof(struct rkisp1_ext_params_##type##_config),		\
>>>    		0, 0,								\
>>>    	} }
>>> @@ -79,56 +79,6 @@ const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = {
>>>    } /* namespace */
>>> -RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,
>>> -					     const Span<uint8_t> &data)
>>> -	: params_(params), type_(type)
>>> -{
>>> -	if (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {
>>> -		header_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header));
>>> -		data_ = data.subspan(sizeof(rkisp1_ext_params_block_header));
>>> -	} else {
>>> -		data_ = data;
>>> -	}
>>> -}
>>> -
>>> -void RkISP1ParamsBlockBase::setEnabled(bool enabled)
>>> -{
>>> -	/*
>>> -	 * For the legacy fixed format, blocks are enabled in the top-level
>>> -	 * header. Delegate to the RkISP1Params class.
>>> -	 */
>>> -	if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)
>>> -		return params_->setBlockEnabled(type_, enabled);
>>> -
>>> -	/*
>>> -	 * For the extensible format, set the enable and disable flags in the
>>> -	 * block header directly.
>>> -	 */
>>> -	struct rkisp1_ext_params_block_header *header =
>>> -		reinterpret_cast<struct rkisp1_ext_params_block_header *>(header_.data());
>>> -	header->flags &= ~(RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |
>>> -			   RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE);
>>> -	header->flags |= enabled ? RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE
>>> -				 : RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE;
>>> -}
>>> -
>>> -RkISP1Params::RkISP1Params(uint32_t format, Span<uint8_t> data)
>>> -	: format_(format), data_(data), used_(0)
>>> -{
>>> -	if (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {
>>> -		struct rkisp1_ext_params_cfg *cfg =
>>> -			reinterpret_cast<struct rkisp1_ext_params_cfg *>(data.data());
>>> -
>>> -		cfg->version = RKISP1_EXT_PARAM_BUFFER_V1;
>>> -		cfg->data_size = 0;
>>> -
>>> -		used_ += offsetof(struct rkisp1_ext_params_cfg, data);
>>> -	} else {
>>> -		memset(data.data(), 0, data.size());
>>> -		used_ = sizeof(struct rkisp1_params_cfg);
>>> -	}
>>> -}
>>> -
>>>    void RkISP1Params::setBlockEnabled(BlockType type, bool enabled)
>>>    {
>>>    	const BlockTypeInfo &info = kBlockTypeInfo.at(type);
>>> @@ -178,44 +128,7 @@ Span<uint8_t> RkISP1Params::block(BlockType type)
>>>    		return data_.subspan(info.offset, info.size);
>>>    	}
>>> -	/*
>>> -	 * For the extensible format, allocate memory for the block, including
>>> -	 * the header. Look up the block in the cache first. If an algorithm
>>> -	 * requests the same block type twice, it should get the same block.
>>> -	 */
>>> -	auto cacheIt = blocks_.find(type);
>>> -	if (cacheIt != blocks_.end())
>>> -		return cacheIt->second;
>>> -
>>> -	/* Make sure we don't run out of space. */
>>> -	size_t size = sizeof(struct rkisp1_ext_params_block_header)
>>> -		    + ((info.size + 7) & ~7);
>>> -	if (size > data_.size() - used_) {
>>> -		LOG(RkISP1Params, Error)
>>> -			<< "Out of memory to allocate block type "
>>> -			<< utils::to_underlying(type);
>>> -		return {};
>>> -	}
>>> -
>>> -	/* Allocate a new block, clear its memory, and initialize its header. */
>>> -	Span<uint8_t> block = data_.subspan(used_, size);
>>> -	used_ += size;
>>> -
>>> -	struct rkisp1_ext_params_cfg *cfg =
>>> -		reinterpret_cast<struct rkisp1_ext_params_cfg *>(data_.data());
>>> -	cfg->data_size += size;
>>> -
>>> -	memset(block.data(), 0, block.size());
>>> -
>>> -	struct rkisp1_ext_params_block_header *header =
>>> -		reinterpret_cast<struct rkisp1_ext_params_block_header *>(block.data());
>>> -	header->type = info.type;
>>> -	header->size = block.size();
>>> -
>>> -	/* Update the cache. */
>>> -	blocks_[type] = block;
>>> -
>>> -	return block;
>>> +	return V4L2Params::block(type, info.type, info.size);
>>>    }
>>>    } /* namespace ipa::rkisp1 */
>>> diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h
>>> index 2e60528d102ec44a31417d4b146e74cace363efa..a7fbbc581c6df8a92dcc4fcfe1e620da6c469915 100644
>>> --- a/src/ipa/rkisp1/params.h
>>> +++ b/src/ipa/rkisp1/params.h
>>> @@ -7,13 +7,10 @@
>>>    #pragma once
>>> -#include <map>
>>> -#include <stdint.h>
>>> -
>>>    #include <linux/rkisp1-config.h>
>>> +#include <linux/videodev2.h>
>>> -#include <libcamera/base/class.h>
>>> -#include <libcamera/base/span.h>
>>> +#include <libipa/v4l2_params.h>
>>>    namespace libcamera {
>>> @@ -49,115 +46,143 @@ template<BlockType B>
>>>    struct block_type {
>>>    };
>>> -#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)		\
>>> +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct, id)		\
>>>    template<>								\
>>>    struct block_type<BlockType::blockType> {				\
>>>    	using type = struct rkisp1_cif_isp_##blockStruct##_config;	\
>>> +	static constexpr rkisp1_ext_params_block_type blockType =	\
>>> +		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id;			\
>>>    };
>>> -RKISP1_DEFINE_BLOCK_TYPE(Bls, bls)
>>> -RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc)
>>> -RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg)
>>> -RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain)
>>> -RKISP1_DEFINE_BLOCK_TYPE(Flt, flt)
>>> -RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm)
>>> -RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk)
>>> -RKISP1_DEFINE_BLOCK_TYPE(Goc, goc)
>>> -RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf)
>>> -RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength)
>>> -RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc)
>>> -RKISP1_DEFINE_BLOCK_TYPE(Ie, ie)
>>> -RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc)
>>> -RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas)
>>> -RKISP1_DEFINE_BLOCK_TYPE(Hst, hst)
>>> -RKISP1_DEFINE_BLOCK_TYPE(Aec, aec)
>>> -RKISP1_DEFINE_BLOCK_TYPE(Afc, afc)
>>> -RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls)
>>> -RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve)
>>> -RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve)
>>> -RKISP1_DEFINE_BLOCK_TYPE(Wdr, wdr)
>>> +RKISP1_DEFINE_BLOCK_TYPE(Bls, bls, BLS)
>>> +RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc, DPCC)
>>> +RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg, SDG)
>>> +RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain, AWB_GAIN)
>>> +RKISP1_DEFINE_BLOCK_TYPE(Flt, flt, FLT)
>>> +RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm, BDM)
>>> +RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk, CTK)
>>> +RKISP1_DEFINE_BLOCK_TYPE(Goc, goc, GOC)
>>> +RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf, DPF)
>>> +RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength, DPF_STRENGTH)
>>> +RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc, CPROC)
>>> +RKISP1_DEFINE_BLOCK_TYPE(Ie, ie, IE)
>>> +RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc, LSC)
>>> +RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas, AWB_MEAS)
>>> +RKISP1_DEFINE_BLOCK_TYPE(Hst, hst, HST_MEAS)
>>> +RKISP1_DEFINE_BLOCK_TYPE(Aec, aec, AEC_MEAS)
>>> +RKISP1_DEFINE_BLOCK_TYPE(Afc, afc, AFC_MEAS)
>>> +RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls, COMPAND_BLS)
>>> +RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve, COMPAND_EXPAND)
>>> +RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve, COMPAND_COMPRESS)
>>> +RKISP1_DEFINE_BLOCK_TYPE(Wdr, wdr, WDR)
>>> +
>>> +struct params_traits {
>>> +	using id_type = BlockType;
>>> +
>>> +	template<id_type Id>
>>> +	using id_to_details = block_type<Id>;
>>> +};
>>>    } /* namespace details */
>>> -class RkISP1Params;
>>> +template<typename T>
>>> +class RkISP1ParamsBlock;
>>> -class RkISP1ParamsBlockBase
>>> +class RkISP1Params : public V4L2Params<details::params_traits>
>>>    {
>>>    public:
>>> -	RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,
>>> -			      const Span<uint8_t> &data);
>>> +	static constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1;
>>> +
>>> +	RkISP1Params(uint32_t format, Span<uint8_t> data)
>>> +		: V4L2Params(data, kVersion), format_(format)
>>> +	{
>>> +		if (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {
>>> +			memset(data.data(), 0, data.size());
>>> +			used_ = sizeof(struct rkisp1_params_cfg);
>>> +		}
>>> +	}
>>> +
>>> +	template<details::params_traits::id_type id>
>>> +	auto block()
>>> +	{
>>> +		using Type = typename details::block_type<id>::type;
>>> -	Span<uint8_t> data() const { return data_; }
>>> +		return RkISP1ParamsBlock<Type>(this, id, block(id));
>>> +	}
>>> -	void setEnabled(bool enabled);
>>> +	uint32_t format() const { return format_; }
>>> +	void setBlockEnabled(BlockType type, bool enabled);
>>>    private:
>>> -	LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase)
>>> +	Span<uint8_t> block(BlockType type);
>>> -	RkISP1Params *params_;
>>> -	BlockType type_;
>>> -	Span<uint8_t> header_;
>>> -	Span<uint8_t> data_;
>>> +	uint32_t format_;
>>>    };
>>> -template<BlockType B>
>>> -class RkISP1ParamsBlock : public RkISP1ParamsBlockBase
>>> +template<typename T>
>>> +class RkISP1ParamsBlock : public V4L2ParamsBlock<T>
>>>    {
>>>    public:
>>> -	using Type = typename details::block_type<B>::type;
>>> -
>>> -	RkISP1ParamsBlock(RkISP1Params *params, const Span<uint8_t> &data)
>>> -		: RkISP1ParamsBlockBase(params, B, data)
>>> +	RkISP1ParamsBlock(RkISP1Params *params, BlockType type,
>>> +			  const Span<uint8_t> data)
>>> +		: V4L2ParamsBlock<T>(data)
>>>    	{
>>> +		params_ = params;
>>> +		type_ = type;
>>> +
>>> +		/*
>>> +		 * cifData_ points to the actual configuration data
>>> +		 * (struct rkisp1_cif_isp_*) which is not prefixed by any header,
>>> +		 * for the legacy fixed format.
>>> +		 */
>>> +		if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)
>>> +			cifData_ = data;
>>> +		else
>>> +			cifData_ = data.subspan(sizeof(v4l2_params_block_header));
>>>    	}
>>> -	const Type *operator->() const
>>> +	void setEnabled(bool enabled) override
>>>    	{
>>> -		return reinterpret_cast<const Type *>(data().data());
>>> +		/*
>>> +		 * For the legacy fixed format, blocks are enabled in the
>>> +		 * top-level header. Delegate to the RkISP1Params class.
>>> +		 */
>>> +		if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)
>>> +			return params_->setBlockEnabled(type_, enabled);
>>> +
>>> +		return V4L2ParamsBlock<T>::setEnabled(enabled);
>>>    	}
>>> -	Type *operator->()
>>> +	/*
>>> +	 * Override the dereference operators to return a reference to the
>>> +	 * actual configuration data (struct rkisp1_cif_isp_*) skipping the
>>> +	 * 'v4l2_params_block_header' header.
>>> +	 */
>>> +
>>> +	virtual const T *operator->() const override
>>>    	{
>>> -		return reinterpret_cast<Type *>(data().data());
>>> +		return reinterpret_cast<const T *>(cifData_.data());
>>>    	}
>>> -	const Type &operator*() const &
>>> +	virtual T *operator->() override
>>>    	{
>>> -		return *reinterpret_cast<const Type *>(data().data());
>>> +		return reinterpret_cast<T *>(cifData_.data());
>>>    	}
>>> -	Type &operator*() &
>>> +	virtual const T &operator*() const override
>>>    	{
>>> -		return *reinterpret_cast<Type *>(data().data());
>>> +		return *reinterpret_cast<const T *>(cifData_.data());
>>>    	}
>>> -};
>>> -
>>> -class RkISP1Params
>>> -{
>>> -public:
>>> -	RkISP1Params(uint32_t format, Span<uint8_t> data);
>>> -	template<BlockType B>
>>> -	RkISP1ParamsBlock<B> block()
>>> +	virtual T &operator*() override
>>>    	{
>>> -		return RkISP1ParamsBlock<B>(this, block(B));
>>> +		return *reinterpret_cast<T *>(cifData_.data());
>>>    	}
>>> -	uint32_t format() const { return format_; }
>>> -	size_t size() const { return used_; }
>>> -
>>>    private:
>>> -	friend class RkISP1ParamsBlockBase;
>>> -
>>> -	Span<uint8_t> block(BlockType type);
>>> -	void setBlockEnabled(BlockType type, bool enabled);
>>> -
>>> -	uint32_t format_;
>>> -
>>> -	Span<uint8_t> data_;
>>> -	size_t used_;
>>> -
>>> -	std::map<BlockType, Span<uint8_t>> blocks_;
>>> +	RkISP1Params *params_;
>>> +	BlockType type_;
>>> +	Span<uint8_t> cifData_;
>>>    };
>>>    } /* namespace ipa::rkisp1 */
>>>
>>