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

24572 diff mbox series

The existing RkISP1Params helper class allows the RkISP1 IPA to handle
both the extensible parameters format and the legacy fixed-size 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 parameters buffer compatible with v4l2-isp.h.

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 | 254 +++++++++++++++++++++++++++++++++++++++++
 src/ipa/libipa/v4l2_params.h   | 142 +++++++++++++++++++++++
 src/ipa/rkisp1/params.cpp      |  93 +--------------
 src/ipa/rkisp1/params.h        | 175 ++++++++++++++++------------
 5 files changed, 501 insertions(+), 165 deletions(-)

Hi Jacopo

On 07/10/2025 19:17, Jacopo Mondi wrote:
> The existing RkISP1Params helper class allows the RkISP1 IPA to handle
> both the extensible parameters format and the legacy fixed-size 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 parameters buffer compatible with v4l2-isp.h.
> 
> 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 | 254 +++++++++++++++++++++++++++++++++++++++++
>   src/ipa/libipa/v4l2_params.h   | 142 +++++++++++++++++++++++
>   src/ipa/rkisp1/params.cpp      |  93 +--------------
>   src/ipa/rkisp1/params.h        | 175 ++++++++++++++++------------
>   5 files changed, 501 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',
>   ])

These two aren't alphabetical.>
> diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp
> new file mode 100644
> index 0000000000000000000000000000000000000000..278b5e4259be4798d0f719c8b1876ef654a14c75
> --- /dev/null
> +++ b/src/ipa/libipa/v4l2_params.cpp
> @@ -0,0 +1,254 @@
> +/* 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 format
> + *
> + * The Linux kernel defines a generic buffer format for configuring ISP devices
> + * through a set of parameters in the form of V4L2 ISP generic parameters. The
> + * V4L2 ISP 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.

I think this could do with some revision. Perhaps something like:

The Linux kernel defines a generic buffer format for configuring ISP devices. The format describes a 
serialisation method for ISP parameters that allows userspace to populate a buffer of configuration 
data by appending blocks to the buffer with common headers but device-specific contents one after 
the other.

> + *
> + * The V4L2Params class implementes support for working with V4L2 ISP parameters

s/implementes/implements. And maybe just "...implements support for the V4L2 ISP parameters buffer 
format and allows..."> + * buffer and allows users to populate the ISP configuration blocks, represented
> + * as 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

s/the IPA module supports/supported by the IPA module. s/abstraction/abstractions

> + * 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 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 shall not create a V4L2ParamsBlock manually but should
> + * use 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
> +  *
> + * 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
> + * 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 'param_traits' type the helps the class associate

s/type the helps/type that helps

> + * 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,
> + *	...
> + * };
> + *
> + * // Maps the C++ enum type to the kernel enum type and concrete parameter type
> + * template<myISPBlocks B>
> + * struct block_type {
> + * };
> + *
> + * template<>
> + * struct block_type<myISPBlock::Agc> {
> + *	using type = struct my_isp_kernel_config_type_agc;
> + *	static constexpr kernel_enum_type blockType = MY_ISP_TYPE_AGC;
> + * };
> + *
> + * template<>
> + * struct block_type<myISPBlock::Awb> {
> + *	using type = struct my_isp_kernel_config_type_awb;
> + *	static constexpr kernel_enum_type blockType = MY_ISP_TYPE_AWB;
> + * };
> + *
> + *
> + * // Convenience type to associate a block id to the 'block_type' overload
> + * 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

s/a/an instance of

> + * \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

I think I would prefer to call this "used()"; to me "size()" should return either the maximum size 
of the flexible data member or else the size of the entire buffer.

> + */
> +
> +/**
> + * \fn V4L2Params::block()
> + * \brief Retrieve the location of an ISP configuration block a returns it

Either "Retrieve the location of an ISP configuration block and return it" or "Retrieves the 
location of an ISP configuration block and 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..bf98bb2aba88506e3ad304a995505deba8e0712a
> --- /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_isp_params_block_header *header =
> +			reinterpret_cast<struct v4l2_isp_params_block_header *>(data_.data());
> +
> +		header->flags &= ~(V4L2_ISP_PARAMS_FL_BLOCK_ENABLE |
> +				   V4L2_ISP_PARAMS_FL_BLOCK_DISABLE);
> +		header->flags |= enabled ? V4L2_ISP_PARAMS_FL_BLOCK_ENABLE
> +					 : V4L2_ISP_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());
> +	}
> +
> +protected:
> +	Span<uint8_t> data_;
> +};
> +
> +template<typename Traits>
> +class V4L2Params
> +{
> +public:
> +	V4L2Params(Span<uint8_t> data, unsigned int version)
> +		: data_(data)
> +	{
> +		struct v4l2_isp_params_buffer *cfg =
> +			reinterpret_cast<struct v4l2_isp_params_buffer *>(data_.data());
> +		cfg->data_size = 0;
> +		cfg->version = version;
> +		used_ = offsetof(struct v4l2_isp_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);

What happens if the type ID isn't one of the ones defined for the IPA?

> +	}
> +
> +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 {};

This seems a little risky to me, because we pass that empty Span to V4L2ParamsBlock in its 
constructor, which simply stores it and returns. Shouldn't something error out in that chain of events?

> +
> +		/* Allocate a new block, clear its memory, and initialize its header. */
> +		Span<uint8_t> block = data_.subspan(used_, blockSize);
> +		used_ += blockSize;
> +
> +		struct v4l2_isp_params_buffer *cfg =
> +			reinterpret_cast<struct v4l2_isp_params_buffer *>(data_.data());

Is it worth just storing this pointer in the constructor perhaps?

> +		cfg->data_size += blockSize;
> +
> +		memset(block.data(), 0, block.size());
> +
> +		struct v4l2_isp_params_block_header *header =
> +			reinterpret_cast<struct v4l2_isp_params_block_header *>(block.data());
> +		header->type = blockType;
> +		header->size = block.size();

You could avoid another cast here I think by doing this in the outer block() with something like...

block = V4L2ParamsBlock<Type>(data);
block->header.type = kernelId;
block->header.size = sizeof(Type);

return block;

A later edit: Although perhaps that doesn't make sense, as far as I know nothing actually guarantees 
that the extensible params block header is called "header"

Thanks
Dan

> +
> +		/* 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..eddb37d5c000b6b1de1c698ad915f2b42da58b81 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 final : 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_isp_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_isp_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 Dan

   thanks for review

On Fri, Oct 10, 2025 at 04:00:23PM +0100, Dan Scally wrote:
> Hi Jacopo
>
> On 07/10/2025 19:17, Jacopo Mondi wrote:
> > The existing RkISP1Params helper class allows the RkISP1 IPA to handle
> > both the extensible parameters format and the legacy fixed-size 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 parameters buffer compatible with v4l2-isp.h.
> >
> > 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 | 254 +++++++++++++++++++++++++++++++++++++++++
> >   src/ipa/libipa/v4l2_params.h   | 142 +++++++++++++++++++++++
> >   src/ipa/rkisp1/params.cpp      |  93 +--------------
> >   src/ipa/rkisp1/params.h        | 175 ++++++++++++++++------------
> >   5 files changed, 501 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',
> >   ])
>
> These two aren't alphabetical.>
> > diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..278b5e4259be4798d0f719c8b1876ef654a14c75
> > --- /dev/null
> > +++ b/src/ipa/libipa/v4l2_params.cpp
> > @@ -0,0 +1,254 @@
> > +/* 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 format
> > + *
> > + * The Linux kernel defines a generic buffer format for configuring ISP devices
> > + * through a set of parameters in the form of V4L2 ISP generic parameters. The
> > + * V4L2 ISP 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.
>
> I think this could do with some revision. Perhaps something like:
>
> The Linux kernel defines a generic buffer format for configuring ISP
> devices. The format describes a serialisation method for ISP parameters that
> allows userspace to populate a buffer of configuration data by appending
> blocks to the buffer with common headers but device-specific contents one
> after the other.
>
Thanks! reads way better


> > + *
> > + * The V4L2Params class implementes support for working with V4L2 ISP parameters
>
> s/implementes/implements. And maybe just "...implements support for the V4L2
> ISP parameters buffer format and allows..."> + * buffer and allows users to
> populate the ISP configuration blocks, represented

ack

> > + * as 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
>
> s/the IPA module supports/supported by the IPA module. s/abstraction/abstractions

indeed

>
> > + * 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 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 shall not create a V4L2ParamsBlock manually but should
> > + * use 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
> > +  *
> > + * 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
> > + * 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 'param_traits' type the helps the class associate
>
> s/type the helps/type that helps
>
> > + * 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,
> > + *	...
> > + * };
> > + *
> > + * // Maps the C++ enum type to the kernel enum type and concrete parameter type
> > + * template<myISPBlocks B>
> > + * struct block_type {
> > + * };
> > + *
> > + * template<>
> > + * struct block_type<myISPBlock::Agc> {
> > + *	using type = struct my_isp_kernel_config_type_agc;
> > + *	static constexpr kernel_enum_type blockType = MY_ISP_TYPE_AGC;
> > + * };
> > + *
> > + * template<>
> > + * struct block_type<myISPBlock::Awb> {
> > + *	using type = struct my_isp_kernel_config_type_awb;
> > + *	static constexpr kernel_enum_type blockType = MY_ISP_TYPE_AWB;
> > + * };
> > + *
> > + *
> > + * // Convenience type to associate a block id to the 'block_type' overload
> > + * 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
>
> s/a/an instance of
>
> > + * \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
>
> I think I would prefer to call this "used()"; to me "size()" should return
> either the maximum size of the flexible data member or else the size of the
> entire buffer.

or, as you suggested below bytesused() ?
>
> > + */
> > +
> > +/**
> > + * \fn V4L2Params::block()
> > + * \brief Retrieve the location of an ISP configuration block a returns it
>
> Either "Retrieve the location of an ISP configuration block and return it"
> or "Retrieves the location of an ISP configuration block and returns it".
>

I think we mostly use imperative ? I'll check

> > + * \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..bf98bb2aba88506e3ad304a995505deba8e0712a
> > --- /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_isp_params_block_header *header =
> > +			reinterpret_cast<struct v4l2_isp_params_block_header *>(data_.data());
> > +
> > +		header->flags &= ~(V4L2_ISP_PARAMS_FL_BLOCK_ENABLE |
> > +				   V4L2_ISP_PARAMS_FL_BLOCK_DISABLE);
> > +		header->flags |= enabled ? V4L2_ISP_PARAMS_FL_BLOCK_ENABLE
> > +					 : V4L2_ISP_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());
> > +	}
> > +
> > +protected:
> > +	Span<uint8_t> data_;
> > +};
> > +
> > +template<typename Traits>
> > +class V4L2Params
> > +{
> > +public:
> > +	V4L2Params(Span<uint8_t> data, unsigned int version)
> > +		: data_(data)
> > +	{
> > +		struct v4l2_isp_params_buffer *cfg =
> > +			reinterpret_cast<struct v4l2_isp_params_buffer *>(data_.data());
> > +		cfg->data_size = 0;
> > +		cfg->version = version;
> > +		used_ = offsetof(struct v4l2_isp_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);
>
>What happens if the type ID isn't one of the ones defined for the IPA?

What 'type ID' ? :)

Are you suggesting what happens in an IPA defines a valid value for
the template argument 'Traits::id_type Id' in the block ids
enumeration, but doesn't define any 'struct block_type<ID>' ?

You get a compiler error I presume as the template generation fails
because the compiler can't resolve id_to_details<Id>;

>
> > +	}
> > +
> > +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 {};
>
> This seems a little risky to me, because we pass that empty Span to
> V4L2ParamsBlock in its constructor, which simply stores it and returns.
> Shouldn't something error out in that chain of events?
>

We should at least warn, at least

This should segfault as soon as some tries to access the
V4L2ParamsBlock<> constructed with an empty span, though..

(confirmed, it does)

how handle it more gracefully... I'm not sure. We could assert, it's
equally harsh but it provides more context.


> > +
> > +		/* Allocate a new block, clear its memory, and initialize its header. */
> > +		Span<uint8_t> block = data_.subspan(used_, blockSize);
> > +		used_ += blockSize;
> > +
> > +		struct v4l2_isp_params_buffer *cfg =
> > +			reinterpret_cast<struct v4l2_isp_params_buffer *>(data_.data());
>
> Is it worth just storing this pointer in the constructor perhaps?
>

As we access it at construction time, probably yes

> > +		cfg->data_size += blockSize;
> > +
> > +		memset(block.data(), 0, block.size());
> > +
> > +		struct v4l2_isp_params_block_header *header =
> > +			reinterpret_cast<struct v4l2_isp_params_block_header *>(block.data());
> > +		header->type = blockType;
> > +		header->size = block.size();
>
> You could avoid another cast here I think by doing this in the outer block() with something like...
>
> block = V4L2ParamsBlock<Type>(data);
> block->header.type = kernelId;
> block->header.size = sizeof(Type);
>
> return block;
>

It should be possible. But the rkisp1 overload then would have to do
the same.. at the cost of a cast, I would prefer to return an
initialized block from here

> A later edit: Although perhaps that doesn't make sense, as far as I know
> nothing actually guarantees that the extensible params block header is
> called "header"

That too, yes

Thanks
  j

>
> Thanks
> Dan
>
> > +
> > +		/* 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..eddb37d5c000b6b1de1c698ad915f2b42da58b81 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 final : 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_isp_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_isp_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 */
> >
>