[v4,4/9] ipa: rkisp1: Add ISP parameters abstraction class

21036 diff mbox series

Individual algorithms of the rkisp1 IPA module access their
corresponding ISP parameters through the top-level structure
rkisp1_params_cfg. This will not work anymore with the new parameters
format. In order to ease the transition to the new format, abstract the
ISP parameters in a new RkISP1Params class that offers the same
interface regardless of the format.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
Changes since v3:

- Update to latest kernel API

Changes since v2:

- Rename Block to BlockType
- Fix grammer in comment

Changes since v1:

- Fix module enable and update fields for legacy format
- Log messages when block allocation fails
- Use uint32_t type for enableBit
- Reword comment explaining block caching
- Fix State::Disable enumerator value
- Disable copying of RkISP1ParamsBlock class
- Move the params enabled state handling to a setEnabled() function
---
 src/ipa/rkisp1/meson.build |   1 +
 src/ipa/rkisp1/params.cpp  | 219 +++++++++++++++++++++++++++++++++++++
 src/ipa/rkisp1/params.h    | 157 ++++++++++++++++++++++++++
 3 files changed, 377 insertions(+)
 create mode 100644 src/ipa/rkisp1/params.cpp
 create mode 100644 src/ipa/rkisp1/params.h

Hi Laurent
On Tue, Aug 27, 2024 at 04:40:38AM GMT, Laurent Pinchart wrote:
> Individual algorithms of the rkisp1 IPA module access their
> corresponding ISP parameters through the top-level structure
> rkisp1_params_cfg. This will not work anymore with the new parameters
> format. In order to ease the transition to the new format, abstract the
> ISP parameters in a new RkISP1Params class that offers the same
> interface regardless of the format.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
> Changes since v3:
>
> - Update to latest kernel API
>

I don't get it, what kernel API was the previous version based on ?

I tested against your v6.11/merge branch which if I'm not mistaken has
media-stage merged in.


> Changes since v2:
>
> - Rename Block to BlockType
> - Fix grammer in comment
>
> Changes since v1:
>
> - Fix module enable and update fields for legacy format
> - Log messages when block allocation fails
> - Use uint32_t type for enableBit
> - Reword comment explaining block caching
> - Fix State::Disable enumerator value
> - Disable copying of RkISP1ParamsBlock class
> - Move the params enabled state handling to a setEnabled() function
> ---
>  src/ipa/rkisp1/meson.build |   1 +
>  src/ipa/rkisp1/params.cpp  | 219 +++++++++++++++++++++++++++++++++++++
>  src/ipa/rkisp1/params.h    | 157 ++++++++++++++++++++++++++
>  3 files changed, 377 insertions(+)
>  create mode 100644 src/ipa/rkisp1/params.cpp
>  create mode 100644 src/ipa/rkisp1/params.h
>
> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build
> index 160ef52dd52e..34844f1498f9 100644
> --- a/src/ipa/rkisp1/meson.build
> +++ b/src/ipa/rkisp1/meson.build
> @@ -7,6 +7,7 @@ ipa_name = 'ipa_rkisp1'
>
>  rkisp1_ipa_sources = files([
>      'ipa_context.cpp',
> +    'params.cpp',
>      'rkisp1.cpp',
>      'utils.cpp',
>  ])
> diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp
> new file mode 100644
> index 000000000000..6bc6f89919fb
> --- /dev/null
> +++ b/src/ipa/rkisp1/params.cpp
> @@ -0,0 +1,219 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board
> + *
> + * RkISP1 ISP Parameters
> + */
> +
> +#include "params.h"
> +
> +#include <map>
> +#include <stddef.h>
> +#include <string.h>
> +
> +#include <linux/rkisp1-config.h>
> +#include <linux/videodev2.h>
> +
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/utils.h>
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(RkISP1Params)
> +
> +namespace ipa::rkisp1 {
> +
> +namespace {
> +
> +struct BlockTypeInfo {
> +	enum rkisp1_ext_params_block_type type;
> +	size_t size;
> +	size_t offset;
> +	uint32_t enableBit;
> +};
> +
> +#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),			\
> +		offsetof(struct rkisp1_params_cfg, category.type##_config),	\
> +		RKISP1_CIF_ISP_MODULE_##bit,					\
> +	} }
> +
> +#define RKISP1_BLOCK_TYPE_ENTRY_MEAS(block, id, type)				\
> +	RKISP1_BLOCK_TYPE_ENTRY(block, id##_MEAS, type, meas, id)
> +
> +#define RKISP1_BLOCK_TYPE_ENTRY_OTHERS(block, id, type)				\
> +	RKISP1_BLOCK_TYPE_ENTRY(block, id, type, others, id)
> +
> +#define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type)				\
> +	{ BlockType::block, {							\
> +		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id,				\
> +		sizeof(struct rkisp1_cif_isp_##type##_config),			\
> +		0, 0,								\
> +	} }
> +
> +const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = {
> +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Bls, BLS, bls),
> +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Dpcc, DPCC, dpcc),
> +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Sdg, SDG, sdg),
> +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(AwbGain, AWB_GAIN, awb_gain),
> +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Flt, FLT, flt),
> +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Bdm, BDM, bdm),
> +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Ctk, CTK, ctk),
> +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Goc, GOC, goc),
> +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Dpf, DPF, dpf),
> +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(DpfStrength, DPF_STRENGTH, dpf_strength),
> +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Cproc, CPROC, cproc),
> +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Ie, IE, ie),
> +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Lsc, LSC, lsc),
> +	RKISP1_BLOCK_TYPE_ENTRY_MEAS(Awb, AWB, awb_meas),
> +	RKISP1_BLOCK_TYPE_ENTRY_MEAS(Hst, HST, hst),
> +	RKISP1_BLOCK_TYPE_ENTRY_MEAS(Aec, AEC, aec),
> +	RKISP1_BLOCK_TYPE_ENTRY_MEAS(Afc, AFC, afc),
> +};
> +
> +} /* 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);
> +
> +	struct rkisp1_params_cfg *cfg =
> +		reinterpret_cast<struct rkisp1_params_cfg *>(data_.data());
> +	if (enabled)
> +		cfg->module_ens |= info.enableBit;
> +	else
> +		cfg->module_ens &= ~info.enableBit;
> +}
> +
> +Span<uint8_t> RkISP1Params::block(BlockType type)
> +{
> +	auto infoIt = kBlockTypeInfo.find(type);
> +	if (infoIt == kBlockTypeInfo.end()) {
> +		LOG(RkISP1Params, Error)
> +			<< "Invalid parameters block type "
> +			<< utils::to_underlying(type);
> +		return {};
> +	}
> +
> +	const BlockTypeInfo &info = infoIt->second;
> +
> +	/*
> +	 * For the legacy format, return a block referencing the fixed location
> +	 * of the data.
> +	 */
> +	if (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {
> +		/*
> +		 * Blocks available only in extended parameters have an offset
> +		 * of 0. Return nullptr in that case.
> +		 */
> +		if (info.offset == 0) {
> +			LOG(RkISP1Params, Error)
> +				<< "Block type " << utils::to_underlying(type)
> +				<< " unavailable in fixed parameters format";
> +			return {};
> +		}
> +
> +		struct rkisp1_params_cfg *cfg =
> +			reinterpret_cast<struct rkisp1_params_cfg *>(data_.data());
> +
> +		cfg->module_cfg_update |= info.enableBit;
> +		cfg->module_en_update |= info.enableBit;
> +
> +		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;
> +}
> +
> +} /* namespace ipa::rkisp1 */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h
> new file mode 100644
> index 000000000000..28a781bc447c
> --- /dev/null
> +++ b/src/ipa/rkisp1/params.h
> @@ -0,0 +1,157 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board
> + *
> + * RkISP1 ISP Parameters
> + */
> +
> +#pragma once
> +
> +#include <map>
> +#include <stdint.h>
> +
> +#include <linux/rkisp1-config.h>
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/span.h>
> +
> +namespace libcamera {
> +
> +namespace ipa::rkisp1 {
> +
> +enum class BlockType {
> +	Bls,
> +	Dpcc,
> +	Sdg,
> +	AwbGain,
> +	Flt,
> +	Bdm,
> +	Ctk,
> +	Goc,
> +	Dpf,
> +	DpfStrength,
> +	Cproc,
> +	Ie,
> +	Lsc,
> +	Awb,
> +	Hst,
> +	Aec,
> +	Afc,
> +};
> +
> +namespace details {
> +
> +template<BlockType B>
> +struct block_type {
> +};
> +
> +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)		\
> +template<>								\
> +struct block_type<BlockType::blockType> {				\
> +	using type = struct rkisp1_cif_isp_##blockStruct##_config;	\
> +};
> +
> +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)
> +
> +} /* namespace details */
> +
> +class RkISP1Params;
> +
> +class RkISP1ParamsBlockBase
> +{
> +public:
> +	RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,
> +			      const Span<uint8_t> &data);
> +
> +	Span<uint8_t> data() const { return data_; }
> +
> +	void setEnabled(bool enabled);
> +
> +private:
> +	LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase)
> +
> +	RkISP1Params *params_;
> +	BlockType type_;
> +	Span<uint8_t> header_;
> +	Span<uint8_t> data_;
> +};
> +
> +template<BlockType B>
> +class RkISP1ParamsBlock : public RkISP1ParamsBlockBase
> +{
> +public:
> +	using Type = typename details::block_type<B>::type;
> +
> +	RkISP1ParamsBlock(RkISP1Params *params, const Span<uint8_t> &data)
> +		: RkISP1ParamsBlockBase(params, B, data)
> +	{
> +	}
> +
> +	const Type *operator->() const
> +	{
> +		return reinterpret_cast<const Type *>(data().data());
> +	}
> +
> +	Type *operator->()
> +	{
> +		return reinterpret_cast<Type *>(data().data());
> +	}
> +
> +	const Type &operator*() const &
> +	{
> +		return *reinterpret_cast<const Type *>(data().data());
> +	}
> +
> +	Type &operator*() &
> +	{
> +		return *reinterpret_cast<Type *>(data().data());
> +	}
> +};
> +
> +class RkISP1Params
> +{
> +public:
> +	RkISP1Params(uint32_t format, Span<uint8_t> data);
> +
> +	template<BlockType B>
> +	RkISP1ParamsBlock<B> block()
> +	{
> +		return RkISP1ParamsBlock<B>(this, block(B));
> +	}
> +
> +	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_;
> +};
> +
> +} /* namespace ipa::rkisp1 */
> +
> +} /* namespace libcamera*/
> --
> Regards,
>
> Laurent Pinchart
>

On Tue, Aug 27, 2024 at 08:53:44AM +0200, Jacopo Mondi wrote:
> Hi Laurent
> On Tue, Aug 27, 2024 at 04:40:38AM GMT, Laurent Pinchart wrote:
> > Individual algorithms of the rkisp1 IPA module access their
> > corresponding ISP parameters through the top-level structure
> > rkisp1_params_cfg. This will not work anymore with the new parameters
> > format. In order to ease the transition to the new format, abstract the
> > ISP parameters in a new RkISP1Params class that offers the same
> > interface regardless of the format.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> > Changes since v3:
> >
> > - Update to latest kernel API
> >
> 
> I don't get it, what kernel API was the previous version based on ?

This should have been in the "changes since v2" section, sorry. I'm
referring to the usage of the .flags field, replacing .enabled.

> I tested against your v6.11/merge branch which if I'm not mistaken has
> media-stage merged in.
> 
> > Changes since v2:
> >
> > - Rename Block to BlockType
> > - Fix grammer in comment
> >
> > Changes since v1:
> >
> > - Fix module enable and update fields for legacy format
> > - Log messages when block allocation fails
> > - Use uint32_t type for enableBit
> > - Reword comment explaining block caching
> > - Fix State::Disable enumerator value
> > - Disable copying of RkISP1ParamsBlock class
> > - Move the params enabled state handling to a setEnabled() function
> > ---
> >  src/ipa/rkisp1/meson.build |   1 +
> >  src/ipa/rkisp1/params.cpp  | 219 +++++++++++++++++++++++++++++++++++++
> >  src/ipa/rkisp1/params.h    | 157 ++++++++++++++++++++++++++
> >  3 files changed, 377 insertions(+)
> >  create mode 100644 src/ipa/rkisp1/params.cpp
> >  create mode 100644 src/ipa/rkisp1/params.h
> >
> > diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build
> > index 160ef52dd52e..34844f1498f9 100644
> > --- a/src/ipa/rkisp1/meson.build
> > +++ b/src/ipa/rkisp1/meson.build
> > @@ -7,6 +7,7 @@ ipa_name = 'ipa_rkisp1'
> >
> >  rkisp1_ipa_sources = files([
> >      'ipa_context.cpp',
> > +    'params.cpp',
> >      'rkisp1.cpp',
> >      'utils.cpp',
> >  ])
> > diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp
> > new file mode 100644
> > index 000000000000..6bc6f89919fb
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/params.cpp
> > @@ -0,0 +1,219 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board
> > + *
> > + * RkISP1 ISP Parameters
> > + */
> > +
> > +#include "params.h"
> > +
> > +#include <map>
> > +#include <stddef.h>
> > +#include <string.h>
> > +
> > +#include <linux/rkisp1-config.h>
> > +#include <linux/videodev2.h>
> > +
> > +#include <libcamera/base/log.h>
> > +#include <libcamera/base/utils.h>
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(RkISP1Params)
> > +
> > +namespace ipa::rkisp1 {
> > +
> > +namespace {
> > +
> > +struct BlockTypeInfo {
> > +	enum rkisp1_ext_params_block_type type;
> > +	size_t size;
> > +	size_t offset;
> > +	uint32_t enableBit;
> > +};
> > +
> > +#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),			\
> > +		offsetof(struct rkisp1_params_cfg, category.type##_config),	\
> > +		RKISP1_CIF_ISP_MODULE_##bit,					\
> > +	} }
> > +
> > +#define RKISP1_BLOCK_TYPE_ENTRY_MEAS(block, id, type)				\
> > +	RKISP1_BLOCK_TYPE_ENTRY(block, id##_MEAS, type, meas, id)
> > +
> > +#define RKISP1_BLOCK_TYPE_ENTRY_OTHERS(block, id, type)				\
> > +	RKISP1_BLOCK_TYPE_ENTRY(block, id, type, others, id)
> > +
> > +#define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type)				\
> > +	{ BlockType::block, {							\
> > +		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id,				\
> > +		sizeof(struct rkisp1_cif_isp_##type##_config),			\
> > +		0, 0,								\
> > +	} }
> > +
> > +const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = {
> > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Bls, BLS, bls),
> > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Dpcc, DPCC, dpcc),
> > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Sdg, SDG, sdg),
> > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(AwbGain, AWB_GAIN, awb_gain),
> > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Flt, FLT, flt),
> > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Bdm, BDM, bdm),
> > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Ctk, CTK, ctk),
> > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Goc, GOC, goc),
> > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Dpf, DPF, dpf),
> > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(DpfStrength, DPF_STRENGTH, dpf_strength),
> > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Cproc, CPROC, cproc),
> > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Ie, IE, ie),
> > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Lsc, LSC, lsc),
> > +	RKISP1_BLOCK_TYPE_ENTRY_MEAS(Awb, AWB, awb_meas),
> > +	RKISP1_BLOCK_TYPE_ENTRY_MEAS(Hst, HST, hst),
> > +	RKISP1_BLOCK_TYPE_ENTRY_MEAS(Aec, AEC, aec),
> > +	RKISP1_BLOCK_TYPE_ENTRY_MEAS(Afc, AFC, afc),
> > +};
> > +
> > +} /* 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);
> > +
> > +	struct rkisp1_params_cfg *cfg =
> > +		reinterpret_cast<struct rkisp1_params_cfg *>(data_.data());
> > +	if (enabled)
> > +		cfg->module_ens |= info.enableBit;
> > +	else
> > +		cfg->module_ens &= ~info.enableBit;
> > +}
> > +
> > +Span<uint8_t> RkISP1Params::block(BlockType type)
> > +{
> > +	auto infoIt = kBlockTypeInfo.find(type);
> > +	if (infoIt == kBlockTypeInfo.end()) {
> > +		LOG(RkISP1Params, Error)
> > +			<< "Invalid parameters block type "
> > +			<< utils::to_underlying(type);
> > +		return {};
> > +	}
> > +
> > +	const BlockTypeInfo &info = infoIt->second;
> > +
> > +	/*
> > +	 * For the legacy format, return a block referencing the fixed location
> > +	 * of the data.
> > +	 */
> > +	if (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {
> > +		/*
> > +		 * Blocks available only in extended parameters have an offset
> > +		 * of 0. Return nullptr in that case.
> > +		 */
> > +		if (info.offset == 0) {
> > +			LOG(RkISP1Params, Error)
> > +				<< "Block type " << utils::to_underlying(type)
> > +				<< " unavailable in fixed parameters format";
> > +			return {};
> > +		}
> > +
> > +		struct rkisp1_params_cfg *cfg =
> > +			reinterpret_cast<struct rkisp1_params_cfg *>(data_.data());
> > +
> > +		cfg->module_cfg_update |= info.enableBit;
> > +		cfg->module_en_update |= info.enableBit;
> > +
> > +		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;
> > +}
> > +
> > +} /* namespace ipa::rkisp1 */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h
> > new file mode 100644
> > index 000000000000..28a781bc447c
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/params.h
> > @@ -0,0 +1,157 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board
> > + *
> > + * RkISP1 ISP Parameters
> > + */
> > +
> > +#pragma once
> > +
> > +#include <map>
> > +#include <stdint.h>
> > +
> > +#include <linux/rkisp1-config.h>
> > +
> > +#include <libcamera/base/class.h>
> > +#include <libcamera/base/span.h>
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::rkisp1 {
> > +
> > +enum class BlockType {
> > +	Bls,
> > +	Dpcc,
> > +	Sdg,
> > +	AwbGain,
> > +	Flt,
> > +	Bdm,
> > +	Ctk,
> > +	Goc,
> > +	Dpf,
> > +	DpfStrength,
> > +	Cproc,
> > +	Ie,
> > +	Lsc,
> > +	Awb,
> > +	Hst,
> > +	Aec,
> > +	Afc,
> > +};
> > +
> > +namespace details {
> > +
> > +template<BlockType B>
> > +struct block_type {
> > +};
> > +
> > +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)		\
> > +template<>								\
> > +struct block_type<BlockType::blockType> {				\
> > +	using type = struct rkisp1_cif_isp_##blockStruct##_config;	\
> > +};
> > +
> > +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)
> > +
> > +} /* namespace details */
> > +
> > +class RkISP1Params;
> > +
> > +class RkISP1ParamsBlockBase
> > +{
> > +public:
> > +	RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,
> > +			      const Span<uint8_t> &data);
> > +
> > +	Span<uint8_t> data() const { return data_; }
> > +
> > +	void setEnabled(bool enabled);
> > +
> > +private:
> > +	LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase)
> > +
> > +	RkISP1Params *params_;
> > +	BlockType type_;
> > +	Span<uint8_t> header_;
> > +	Span<uint8_t> data_;
> > +};
> > +
> > +template<BlockType B>
> > +class RkISP1ParamsBlock : public RkISP1ParamsBlockBase
> > +{
> > +public:
> > +	using Type = typename details::block_type<B>::type;
> > +
> > +	RkISP1ParamsBlock(RkISP1Params *params, const Span<uint8_t> &data)
> > +		: RkISP1ParamsBlockBase(params, B, data)
> > +	{
> > +	}
> > +
> > +	const Type *operator->() const
> > +	{
> > +		return reinterpret_cast<const Type *>(data().data());
> > +	}
> > +
> > +	Type *operator->()
> > +	{
> > +		return reinterpret_cast<Type *>(data().data());
> > +	}
> > +
> > +	const Type &operator*() const &
> > +	{
> > +		return *reinterpret_cast<const Type *>(data().data());
> > +	}
> > +
> > +	Type &operator*() &
> > +	{
> > +		return *reinterpret_cast<Type *>(data().data());
> > +	}
> > +};
> > +
> > +class RkISP1Params
> > +{
> > +public:
> > +	RkISP1Params(uint32_t format, Span<uint8_t> data);
> > +
> > +	template<BlockType B>
> > +	RkISP1ParamsBlock<B> block()
> > +	{
> > +		return RkISP1ParamsBlock<B>(this, block(B));
> > +	}
> > +
> > +	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_;
> > +};
> > +
> > +} /* namespace ipa::rkisp1 */
> > +
> > +} /* namespace libcamera*/

On Tue, Aug 27, 2024 at 10:43:39AM GMT, Laurent Pinchart wrote:
> On Tue, Aug 27, 2024 at 08:53:44AM +0200, Jacopo Mondi wrote:
> > Hi Laurent
> > On Tue, Aug 27, 2024 at 04:40:38AM GMT, Laurent Pinchart wrote:
> > > Individual algorithms of the rkisp1 IPA module access their
> > > corresponding ISP parameters through the top-level structure
> > > rkisp1_params_cfg. This will not work anymore with the new parameters
> > > format. In order to ease the transition to the new format, abstract the
> > > ISP parameters in a new RkISP1Params class that offers the same
> > > interface regardless of the format.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > ---
> > > Changes since v3:
> > >
> > > - Update to latest kernel API
> > >
> >
> > I don't get it, what kernel API was the previous version based on ?
>
> This should have been in the "changes since v2" section, sorry. I'm
> referring to the usage of the .flags field, replacing .enabled.
>

Ah ok, I was scared I have been testing against an older kernel
revision ;)

> > I tested against your v6.11/merge branch which if I'm not mistaken has
> > media-stage merged in.
> >
> > > Changes since v2:
> > >
> > > - Rename Block to BlockType
> > > - Fix grammer in comment
> > >
> > > Changes since v1:
> > >
> > > - Fix module enable and update fields for legacy format
> > > - Log messages when block allocation fails
> > > - Use uint32_t type for enableBit
> > > - Reword comment explaining block caching
> > > - Fix State::Disable enumerator value
> > > - Disable copying of RkISP1ParamsBlock class
> > > - Move the params enabled state handling to a setEnabled() function
> > > ---
> > >  src/ipa/rkisp1/meson.build |   1 +
> > >  src/ipa/rkisp1/params.cpp  | 219 +++++++++++++++++++++++++++++++++++++
> > >  src/ipa/rkisp1/params.h    | 157 ++++++++++++++++++++++++++
> > >  3 files changed, 377 insertions(+)
> > >  create mode 100644 src/ipa/rkisp1/params.cpp
> > >  create mode 100644 src/ipa/rkisp1/params.h
> > >
> > > diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build
> > > index 160ef52dd52e..34844f1498f9 100644
> > > --- a/src/ipa/rkisp1/meson.build
> > > +++ b/src/ipa/rkisp1/meson.build
> > > @@ -7,6 +7,7 @@ ipa_name = 'ipa_rkisp1'
> > >
> > >  rkisp1_ipa_sources = files([
> > >      'ipa_context.cpp',
> > > +    'params.cpp',
> > >      'rkisp1.cpp',
> > >      'utils.cpp',
> > >  ])
> > > diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp
> > > new file mode 100644
> > > index 000000000000..6bc6f89919fb
> > > --- /dev/null
> > > +++ b/src/ipa/rkisp1/params.cpp
> > > @@ -0,0 +1,219 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Ideas On Board
> > > + *
> > > + * RkISP1 ISP Parameters
> > > + */
> > > +
> > > +#include "params.h"
> > > +
> > > +#include <map>
> > > +#include <stddef.h>
> > > +#include <string.h>
> > > +
> > > +#include <linux/rkisp1-config.h>
> > > +#include <linux/videodev2.h>
> > > +
> > > +#include <libcamera/base/log.h>
> > > +#include <libcamera/base/utils.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DEFINE_CATEGORY(RkISP1Params)
> > > +
> > > +namespace ipa::rkisp1 {
> > > +
> > > +namespace {
> > > +
> > > +struct BlockTypeInfo {
> > > +	enum rkisp1_ext_params_block_type type;
> > > +	size_t size;
> > > +	size_t offset;
> > > +	uint32_t enableBit;
> > > +};
> > > +
> > > +#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),			\
> > > +		offsetof(struct rkisp1_params_cfg, category.type##_config),	\
> > > +		RKISP1_CIF_ISP_MODULE_##bit,					\
> > > +	} }
> > > +
> > > +#define RKISP1_BLOCK_TYPE_ENTRY_MEAS(block, id, type)				\
> > > +	RKISP1_BLOCK_TYPE_ENTRY(block, id##_MEAS, type, meas, id)
> > > +
> > > +#define RKISP1_BLOCK_TYPE_ENTRY_OTHERS(block, id, type)				\
> > > +	RKISP1_BLOCK_TYPE_ENTRY(block, id, type, others, id)
> > > +
> > > +#define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type)				\
> > > +	{ BlockType::block, {							\
> > > +		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id,				\
> > > +		sizeof(struct rkisp1_cif_isp_##type##_config),			\
> > > +		0, 0,								\
> > > +	} }
> > > +
> > > +const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = {
> > > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Bls, BLS, bls),
> > > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Dpcc, DPCC, dpcc),
> > > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Sdg, SDG, sdg),
> > > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(AwbGain, AWB_GAIN, awb_gain),
> > > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Flt, FLT, flt),
> > > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Bdm, BDM, bdm),
> > > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Ctk, CTK, ctk),
> > > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Goc, GOC, goc),
> > > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Dpf, DPF, dpf),
> > > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(DpfStrength, DPF_STRENGTH, dpf_strength),
> > > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Cproc, CPROC, cproc),
> > > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Ie, IE, ie),
> > > +	RKISP1_BLOCK_TYPE_ENTRY_OTHERS(Lsc, LSC, lsc),
> > > +	RKISP1_BLOCK_TYPE_ENTRY_MEAS(Awb, AWB, awb_meas),
> > > +	RKISP1_BLOCK_TYPE_ENTRY_MEAS(Hst, HST, hst),
> > > +	RKISP1_BLOCK_TYPE_ENTRY_MEAS(Aec, AEC, aec),
> > > +	RKISP1_BLOCK_TYPE_ENTRY_MEAS(Afc, AFC, afc),
> > > +};
> > > +
> > > +} /* 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);
> > > +
> > > +	struct rkisp1_params_cfg *cfg =
> > > +		reinterpret_cast<struct rkisp1_params_cfg *>(data_.data());
> > > +	if (enabled)
> > > +		cfg->module_ens |= info.enableBit;
> > > +	else
> > > +		cfg->module_ens &= ~info.enableBit;
> > > +}
> > > +
> > > +Span<uint8_t> RkISP1Params::block(BlockType type)
> > > +{
> > > +	auto infoIt = kBlockTypeInfo.find(type);
> > > +	if (infoIt == kBlockTypeInfo.end()) {
> > > +		LOG(RkISP1Params, Error)
> > > +			<< "Invalid parameters block type "
> > > +			<< utils::to_underlying(type);
> > > +		return {};
> > > +	}
> > > +
> > > +	const BlockTypeInfo &info = infoIt->second;
> > > +
> > > +	/*
> > > +	 * For the legacy format, return a block referencing the fixed location
> > > +	 * of the data.
> > > +	 */
> > > +	if (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {
> > > +		/*
> > > +		 * Blocks available only in extended parameters have an offset
> > > +		 * of 0. Return nullptr in that case.
> > > +		 */
> > > +		if (info.offset == 0) {
> > > +			LOG(RkISP1Params, Error)
> > > +				<< "Block type " << utils::to_underlying(type)
> > > +				<< " unavailable in fixed parameters format";
> > > +			return {};
> > > +		}
> > > +
> > > +		struct rkisp1_params_cfg *cfg =
> > > +			reinterpret_cast<struct rkisp1_params_cfg *>(data_.data());
> > > +
> > > +		cfg->module_cfg_update |= info.enableBit;
> > > +		cfg->module_en_update |= info.enableBit;
> > > +
> > > +		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;
> > > +}
> > > +
> > > +} /* namespace ipa::rkisp1 */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h
> > > new file mode 100644
> > > index 000000000000..28a781bc447c
> > > --- /dev/null
> > > +++ b/src/ipa/rkisp1/params.h
> > > @@ -0,0 +1,157 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Ideas On Board
> > > + *
> > > + * RkISP1 ISP Parameters
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <map>
> > > +#include <stdint.h>
> > > +
> > > +#include <linux/rkisp1-config.h>
> > > +
> > > +#include <libcamera/base/class.h>
> > > +#include <libcamera/base/span.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +namespace ipa::rkisp1 {
> > > +
> > > +enum class BlockType {
> > > +	Bls,
> > > +	Dpcc,
> > > +	Sdg,
> > > +	AwbGain,
> > > +	Flt,
> > > +	Bdm,
> > > +	Ctk,
> > > +	Goc,
> > > +	Dpf,
> > > +	DpfStrength,
> > > +	Cproc,
> > > +	Ie,
> > > +	Lsc,
> > > +	Awb,
> > > +	Hst,
> > > +	Aec,
> > > +	Afc,
> > > +};
> > > +
> > > +namespace details {
> > > +
> > > +template<BlockType B>
> > > +struct block_type {
> > > +};
> > > +
> > > +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)		\
> > > +template<>								\
> > > +struct block_type<BlockType::blockType> {				\
> > > +	using type = struct rkisp1_cif_isp_##blockStruct##_config;	\
> > > +};
> > > +
> > > +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)
> > > +
> > > +} /* namespace details */
> > > +
> > > +class RkISP1Params;
> > > +
> > > +class RkISP1ParamsBlockBase
> > > +{
> > > +public:
> > > +	RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,
> > > +			      const Span<uint8_t> &data);
> > > +
> > > +	Span<uint8_t> data() const { return data_; }
> > > +
> > > +	void setEnabled(bool enabled);
> > > +
> > > +private:
> > > +	LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase)
> > > +
> > > +	RkISP1Params *params_;
> > > +	BlockType type_;
> > > +	Span<uint8_t> header_;
> > > +	Span<uint8_t> data_;
> > > +};
> > > +
> > > +template<BlockType B>
> > > +class RkISP1ParamsBlock : public RkISP1ParamsBlockBase
> > > +{
> > > +public:
> > > +	using Type = typename details::block_type<B>::type;
> > > +
> > > +	RkISP1ParamsBlock(RkISP1Params *params, const Span<uint8_t> &data)
> > > +		: RkISP1ParamsBlockBase(params, B, data)
> > > +	{
> > > +	}
> > > +
> > > +	const Type *operator->() const
> > > +	{
> > > +		return reinterpret_cast<const Type *>(data().data());
> > > +	}
> > > +
> > > +	Type *operator->()
> > > +	{
> > > +		return reinterpret_cast<Type *>(data().data());
> > > +	}
> > > +
> > > +	const Type &operator*() const &
> > > +	{
> > > +		return *reinterpret_cast<const Type *>(data().data());
> > > +	}
> > > +
> > > +	Type &operator*() &
> > > +	{
> > > +		return *reinterpret_cast<Type *>(data().data());
> > > +	}
> > > +};
> > > +
> > > +class RkISP1Params
> > > +{
> > > +public:
> > > +	RkISP1Params(uint32_t format, Span<uint8_t> data);
> > > +
> > > +	template<BlockType B>
> > > +	RkISP1ParamsBlock<B> block()
> > > +	{
> > > +		return RkISP1ParamsBlock<B>(this, block(B));
> > > +	}
> > > +
> > > +	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_;
> > > +};
> > > +
> > > +} /* namespace ipa::rkisp1 */
> > > +
> > > +} /* namespace libcamera*/
>
> --
> Regards,
>
> Laurent Pinchart