[v1,05/11] ipa: rkisp1: Add ISP parameters abstraction class
diff mbox series

Message ID 20240703225230.3530-6-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • rkisp1: Support BLS on i.MX8MP
Related show

Commit Message

Laurent Pinchart July 3, 2024, 10:52 p.m. UTC 
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>
---
 src/ipa/rkisp1/meson.build |   1 +
 src/ipa/rkisp1/params.cpp  | 169 +++++++++++++++++++++++++++++++++++++
 src/ipa/rkisp1/params.h    | 140 ++++++++++++++++++++++++++++++
 3 files changed, 310 insertions(+)
 create mode 100644 src/ipa/rkisp1/params.cpp
 create mode 100644 src/ipa/rkisp1/params.h

Comments

Stefan Klug July 4, 2024, 9:58 a.m. UTC | #1 
Hi Laurent,

Thank you for the patch.

On Thu, Jul 04, 2024 at 01:52:24AM +0300, 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>
> ---
>  src/ipa/rkisp1/meson.build |   1 +
>  src/ipa/rkisp1/params.cpp  | 169 +++++++++++++++++++++++++++++++++++++
>  src/ipa/rkisp1/params.h    | 140 ++++++++++++++++++++++++++++++
>  3 files changed, 310 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 e8b266f1ccca..65eef5d69c32 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..ac25ade1c8c4
> --- /dev/null
> +++ b/src/ipa/rkisp1/params.cpp
> @@ -0,0 +1,169 @@
> +/* 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>
> +
> +namespace libcamera {
> +
> +namespace ipa::rkisp1 {
> +
> +namespace {
> +
> +struct BlockTypeInfo {
> +	enum rkisp1_ext_params_block_type type;
> +	size_t size;
> +	size_t offset;
> +	__u32 enableBit;
> +};
> +
> +#define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit)			\
> +	{ Block::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)				\
> +	{ Block::block, {							\
> +		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id,				\
> +		sizeof(struct rkisp1_cif_isp_##type##_config),			\
> +		0, 0,								\
> +	} }
> +
> +const std::map<Block, 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 */
> +
> +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);
> +	}
> +}
> +
> +Span<uint8_t> RkISP1Params::block(Block type, State state)
> +{
> +	auto infoIt = kBlockTypeInfo.find(type);
> +	if (infoIt == kBlockTypeInfo.end())
> +		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 in extended parameters only have an offset
> +		 * of 0. Return nullptr in that case.
> +		 */
> +		if (info.offset == 0)
> +			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;
> +		cfg->module_ens = state == State::Enable ? info.enableBit : 0;

I guess I'm missing something here. But shouldn't these only modify the
relevant bit, as the cfg structure is shared among all algorithms? 

> +
> +		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. Caching helps providing the
> +	 * same behaviour for the legacy and extensible formats when block() is
> +	 * called multiple times for the same block, which simplifies the
> +	 * implementation of algorithms and avoids hard to debug issues.
> +	 */
> +	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_)
> +		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->enable = state == State::Enable
> +		       ? RKISP1_EXT_PARAMS_BLOCK_ENABLE
> +		       : RKISP1_EXT_PARAMS_BLOCK_DISABLE;
> +	header->size = block.size();
> +
> +	/* Skip the block header to get the data. */
> +	block = block.subspan(sizeof(*header));
> +
> +	/* 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..ddd081de7894
> --- /dev/null
> +++ b/src/ipa/rkisp1/params.h
> @@ -0,0 +1,140 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board
> + *
> + * RkISP1 ISP Parameters
> + */
> +
> +#pragma once
> +
> +#include <map>
> +#include <optional>
> +#include <stdint.h>
> +
> +#include <linux/rkisp1-config.h>
> +
> +#include <libcamera/base/span.h>
> +
> +namespace libcamera {
> +
> +namespace ipa::rkisp1 {
> +
> +enum class Block {
> +	Bls,
> +	Dpcc,
> +	Sdg,
> +	AwbGain,
> +	Flt,
> +	Bdm,
> +	Ctk,
> +	Goc,
> +	Dpf,
> +	DpfStrength,
> +	Cproc,
> +	Ie,
> +	Lsc,
> +	Awb,
> +	Hst,
> +	Aec,
> +	Afc,
> +};
> +
> +namespace details {
> +
> +template<Block B>
> +struct block_type {
> +};
> +
> +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)		\
> +template<>								\
> +struct block_type<Block::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 */
> +
> +template<Block B>
> +class RkISP1ParamsBlock
> +{
> +public:
> +	using Type = typename details::block_type<B>::type;
> +
> +	RkISP1ParamsBlock(const Span<uint8_t> &data)
> +		: data_(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());
> +	}
> +
> +private:
> +	Span<uint8_t> data_;
> +};
> +
> +class RkISP1Params
> +{
> +public:
> +	enum class State {
> +		Disable = 1,
> +		Enable = 1,
> +	};
> +
> +	RkISP1Params(uint32_t format, Span<uint8_t> data);
> +
> +	template<Block B>
> +	RkISP1ParamsBlock<B> block(State state)

You mentioned that on irc already. To me it would feel more natural to
do: 

block = params->block<Type>();
block.setEnabled(true/false);
block->someconfig...

But I didn't check the implications that would have on the
implementation :-)

Otherwise I like that concept.

Regards,
Stefan


> +	{
> +		return RkISP1ParamsBlock<B>(block(B, state));
> +	}
> +
> +	size_t size() const { return used_; }
> +
> +private:
> +	Span<uint8_t> block(Block type, State state);
> +
> +	uint32_t format_;
> +
> +	Span<uint8_t> data_;
> +	size_t used_;
> +
> +	std::map<Block, Span<uint8_t>> blocks_;
> +};
> +
> +} /* namespace ipa::rkisp1 */
> +
> +} /* namespace libcamera*/
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart July 4, 2024, 10:04 a.m. UTC | #2 
On Thu, Jul 04, 2024 at 11:58:10AM +0200, Stefan Klug wrote:
> Hi Laurent,
> 
> Thank you for the patch.
> 
> On Thu, Jul 04, 2024 at 01:52:24AM +0300, 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>
> > ---
> >  src/ipa/rkisp1/meson.build |   1 +
> >  src/ipa/rkisp1/params.cpp  | 169 +++++++++++++++++++++++++++++++++++++
> >  src/ipa/rkisp1/params.h    | 140 ++++++++++++++++++++++++++++++
> >  3 files changed, 310 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 e8b266f1ccca..65eef5d69c32 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..ac25ade1c8c4
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/params.cpp
> > @@ -0,0 +1,169 @@
> > +/* 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>
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::rkisp1 {
> > +
> > +namespace {
> > +
> > +struct BlockTypeInfo {
> > +	enum rkisp1_ext_params_block_type type;
> > +	size_t size;
> > +	size_t offset;
> > +	__u32 enableBit;
> > +};
> > +
> > +#define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit)			\
> > +	{ Block::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)				\
> > +	{ Block::block, {							\
> > +		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id,				\
> > +		sizeof(struct rkisp1_cif_isp_##type##_config),			\
> > +		0, 0,								\
> > +	} }
> > +
> > +const std::map<Block, 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 */
> > +
> > +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);
> > +	}
> > +}
> > +
> > +Span<uint8_t> RkISP1Params::block(Block type, State state)
> > +{
> > +	auto infoIt = kBlockTypeInfo.find(type);
> > +	if (infoIt == kBlockTypeInfo.end())
> > +		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 in extended parameters only have an offset
> > +		 * of 0. Return nullptr in that case.
> > +		 */
> > +		if (info.offset == 0)
> > +			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;
> > +		cfg->module_ens = state == State::Enable ? info.enableBit : 0;
> 
> I guess I'm missing something here. But shouldn't these only modify the
> relevant bit, as the cfg structure is shared among all algorithms? 

How did I miss that... This clearly needs to be |=.

> > +
> > +		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. Caching helps providing the
> > +	 * same behaviour for the legacy and extensible formats when block() is
> > +	 * called multiple times for the same block, which simplifies the
> > +	 * implementation of algorithms and avoids hard to debug issues.
> > +	 */
> > +	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_)
> > +		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->enable = state == State::Enable
> > +		       ? RKISP1_EXT_PARAMS_BLOCK_ENABLE
> > +		       : RKISP1_EXT_PARAMS_BLOCK_DISABLE;
> > +	header->size = block.size();
> > +
> > +	/* Skip the block header to get the data. */
> > +	block = block.subspan(sizeof(*header));
> > +
> > +	/* 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..ddd081de7894
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/params.h
> > @@ -0,0 +1,140 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board
> > + *
> > + * RkISP1 ISP Parameters
> > + */
> > +
> > +#pragma once
> > +
> > +#include <map>
> > +#include <optional>
> > +#include <stdint.h>
> > +
> > +#include <linux/rkisp1-config.h>
> > +
> > +#include <libcamera/base/span.h>
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::rkisp1 {
> > +
> > +enum class Block {
> > +	Bls,
> > +	Dpcc,
> > +	Sdg,
> > +	AwbGain,
> > +	Flt,
> > +	Bdm,
> > +	Ctk,
> > +	Goc,
> > +	Dpf,
> > +	DpfStrength,
> > +	Cproc,
> > +	Ie,
> > +	Lsc,
> > +	Awb,
> > +	Hst,
> > +	Aec,
> > +	Afc,
> > +};
> > +
> > +namespace details {
> > +
> > +template<Block B>
> > +struct block_type {
> > +};
> > +
> > +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)		\
> > +template<>								\
> > +struct block_type<Block::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 */
> > +
> > +template<Block B>
> > +class RkISP1ParamsBlock
> > +{
> > +public:
> > +	using Type = typename details::block_type<B>::type;
> > +
> > +	RkISP1ParamsBlock(const Span<uint8_t> &data)
> > +		: data_(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());
> > +	}
> > +
> > +private:
> > +	Span<uint8_t> data_;
> > +};
> > +
> > +class RkISP1Params
> > +{
> > +public:
> > +	enum class State {
> > +		Disable = 1,
> > +		Enable = 1,
> > +	};
> > +
> > +	RkISP1Params(uint32_t format, Span<uint8_t> data);
> > +
> > +	template<Block B>
> > +	RkISP1ParamsBlock<B> block(State state)
> 
> You mentioned that on irc already. To me it would feel more natural to
> do: 
> 
> block = params->block<Type>();
> block.setEnabled(true/false);
> block->someconfig...

I've considered that too. I'll give it a try. Would you object this
being changed on top of the series ?

> But I didn't check the implications that would have on the
> implementation :-)
> 
> Otherwise I like that concept.
> 
> > +	{
> > +		return RkISP1ParamsBlock<B>(block(B, state));
> > +	}
> > +
> > +	size_t size() const { return used_; }
> > +
> > +private:
> > +	Span<uint8_t> block(Block type, State state);
> > +
> > +	uint32_t format_;
> > +
> > +	Span<uint8_t> data_;
> > +	size_t used_;
> > +
> > +	std::map<Block, Span<uint8_t>> blocks_;
> > +};
> > +
> > +} /* namespace ipa::rkisp1 */
> > +
> > +} /* namespace libcamera*/
Jacopo Mondi July 4, 2024, 11:50 a.m. UTC | #3 
Hi Laurent

On Thu, Jul 04, 2024 at 01:52:24AM 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>
> ---
>  src/ipa/rkisp1/meson.build |   1 +
>  src/ipa/rkisp1/params.cpp  | 169 +++++++++++++++++++++++++++++++++++++
>  src/ipa/rkisp1/params.h    | 140 ++++++++++++++++++++++++++++++
>  3 files changed, 310 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 e8b266f1ccca..65eef5d69c32 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..ac25ade1c8c4
> --- /dev/null
> +++ b/src/ipa/rkisp1/params.cpp
> @@ -0,0 +1,169 @@
> +/* 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>
> +
> +namespace libcamera {
> +
> +namespace ipa::rkisp1 {
> +
> +namespace {
> +
> +struct BlockTypeInfo {
> +	enum rkisp1_ext_params_block_type type;
> +	size_t size;
> +	size_t offset;
> +	__u32 enableBit;

Why __u32 and not just unsigned int or uint32_t ?

> +};
> +
> +#define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit)			\
> +	{ Block::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)				\
> +	{ Block::block, {							\
> +		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id,				\
> +		sizeof(struct rkisp1_cif_isp_##type##_config),			\
> +		0, 0,								\
> +	} }
> +
> +const std::map<Block, 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 */
> +
> +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);
> +	}
> +}
> +
> +Span<uint8_t> RkISP1Params::block(Block type, State state)
> +{
> +	auto infoIt = kBlockTypeInfo.find(type);
> +	if (infoIt == kBlockTypeInfo.end())
> +		return {};

As this can only be caused by a development errors (right?) should
this be made Fatal ?

> +
> +	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 in extended parameters only have an offset
> +		 * of 0. Return nullptr in that case.
> +		 */
> +		if (info.offset == 0)
> +			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;
> +		cfg->module_ens = state == State::Enable ? info.enableBit : 0;
> +
> +		return data_.subspan(info.offset, info.size);

clever

> +	}
> +
> +	/*
> +	 * For the extensible format, allocate memory for the block, including
> +	 * the header.
> +	 */
> +
> +	/* Look up the block in the cache first. Caching helps providing the

        /*
	 * Look up the block in the cache first. Caching helps providing the

> +	 * same behaviour for the legacy and extensible formats when block() is

I'm not sure this is very much related to legacy vs extensible or just
to the fact when an algorithm references a block, it wants to get the
same piece of memory every time

> +	 * called multiple times for the same block, which simplifies the
> +	 * implementation of algorithms and avoids hard to debug issues.
> +	 */
> +	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);

blocks are 8-bytes aligned, do you need the + 7.. ?

> +	if (size > data_.size() - used_)
> +		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());

Isn't it better to memset the whole data block to 0 at construction
time ?

> +
> +	struct rkisp1_ext_params_block_header *header =
> +		reinterpret_cast<struct rkisp1_ext_params_block_header *>(block.data());
> +	header->type = info.type;
> +	header->enable = state == State::Enable
> +		       ? RKISP1_EXT_PARAMS_BLOCK_ENABLE
> +		       : RKISP1_EXT_PARAMS_BLOCK_DISABLE;
> +	header->size = block.size();
> +
> +	/* Skip the block header to get the data. */
> +	block = block.subspan(sizeof(*header));
> +
> +	/* 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..ddd081de7894
> --- /dev/null
> +++ b/src/ipa/rkisp1/params.h
> @@ -0,0 +1,140 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board
> + *
> + * RkISP1 ISP Parameters
> + */
> +
> +#pragma once
> +
> +#include <map>
> +#include <optional>

Do you need optional ?

> +#include <stdint.h>
> +
> +#include <linux/rkisp1-config.h>
> +
> +#include <libcamera/base/span.h>
> +
> +namespace libcamera {
> +
> +namespace ipa::rkisp1 {
> +
> +enum class Block {
> +	Bls,
> +	Dpcc,
> +	Sdg,
> +	AwbGain,
> +	Flt,
> +	Bdm,
> +	Ctk,
> +	Goc,
> +	Dpf,
> +	DpfStrength,
> +	Cproc,
> +	Ie,
> +	Lsc,
> +	Awb,
> +	Hst,
> +	Aec,
> +	Afc,
> +};
> +
> +namespace details {
> +
> +template<Block B>
> +struct block_type {
> +};
> +
> +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)		\
> +template<>								\
> +struct block_type<Block::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 */
> +
> +template<Block B>
> +class RkISP1ParamsBlock
> +{
> +public:
> +	using Type = typename details::block_type<B>::type;

clever!

I have a gut feeling we could generate the templates specializations
of RkISP1ParamsBlock<> without going through details::block_type<> but
this is surely nice

> +
> +	RkISP1ParamsBlock(const Span<uint8_t> &data)
> +		: data_(data)
> +	{

is it necessary to expliticlty delete copy and move constructors ?

> +	}
> +
> +	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());
> +	}
> +
> +private:
> +	Span<uint8_t> data_;
> +};
> +
> +class RkISP1Params
> +{
> +public:
> +	enum class State {
> +		Disable = 1,

Disable = 0,

or simply drop the initializers

very nice and elegant!

> +		Enable = 1,
> +	};
> +
> +	RkISP1Params(uint32_t format, Span<uint8_t> data);
> +
> +	template<Block B>
> +	RkISP1ParamsBlock<B> block(State state)
> +	{
> +		return RkISP1ParamsBlock<B>(block(B, state));
> +	}
> +
> +	size_t size() const { return used_; }
> +
> +private:
> +	Span<uint8_t> block(Block type, State state);
> +
> +	uint32_t format_;
> +
> +	Span<uint8_t> data_;
> +	size_t used_;
> +
> +	std::map<Block, Span<uint8_t>> blocks_;
> +};
> +
> +} /* namespace ipa::rkisp1 */
> +
> +} /* namespace libcamera*/
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart July 4, 2024, 1:24 p.m. UTC | #4 
On Thu, Jul 04, 2024 at 01:50:25PM +0200, Jacopo Mondi wrote:
> Hi Laurent
> 
> On Thu, Jul 04, 2024 at 01:52:24AM 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>
> > ---
> >  src/ipa/rkisp1/meson.build |   1 +
> >  src/ipa/rkisp1/params.cpp  | 169 +++++++++++++++++++++++++++++++++++++
> >  src/ipa/rkisp1/params.h    | 140 ++++++++++++++++++++++++++++++
> >  3 files changed, 310 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 e8b266f1ccca..65eef5d69c32 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..ac25ade1c8c4
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/params.cpp
> > @@ -0,0 +1,169 @@
> > +/* 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>
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::rkisp1 {
> > +
> > +namespace {
> > +
> > +struct BlockTypeInfo {
> > +	enum rkisp1_ext_params_block_type type;
> > +	size_t size;
> > +	size_t offset;
> > +	__u32 enableBit;
> 
> Why __u32 and not just unsigned int or uint32_t ?

Because it's a bit for the kernel API. It doesn't matter much though,
I'll switch back to uint32_t.

> > +};
> > +
> > +#define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit)			\
> > +	{ Block::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)				\
> > +	{ Block::block, {							\
> > +		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id,				\
> > +		sizeof(struct rkisp1_cif_isp_##type##_config),			\
> > +		0, 0,								\
> > +	} }
> > +
> > +const std::map<Block, 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 */
> > +
> > +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);
> > +	}
> > +}
> > +
> > +Span<uint8_t> RkISP1Params::block(Block type, State state)
> > +{
> > +	auto infoIt = kBlockTypeInfo.find(type);
> > +	if (infoIt == kBlockTypeInfo.end())
> > +		return {};
> 
> As this can only be caused by a development errors (right?) should
> this be made Fatal ?

I can do that.

> > +
> > +	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 in extended parameters only have an offset
> > +		 * of 0. Return nullptr in that case.
> > +		 */
> > +		if (info.offset == 0)
> > +			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;
> > +		cfg->module_ens = state == State::Enable ? info.enableBit : 0;
> > +
> > +		return data_.subspan(info.offset, info.size);
> 
> clever

Thanks :-)

> > +	}
> > +
> > +	/*
> > +	 * For the extensible format, allocate memory for the block, including
> > +	 * the header.
> > +	 */
> > +
> > +	/* Look up the block in the cache first. Caching helps providing the
> 
>       /*
> 	 * Look up the block in the cache first. Caching helps providing the
> 
> > +	 * same behaviour for the legacy and extensible formats when block() is
> 
> I'm not sure this is very much related to legacy vs extensible or just
> to the fact when an algorithm references a block, it wants to get the
> same piece of memory every time

For the legacy format that's what happens, so I wanted the same to be
true for the extensible format. I'll reword the message.

> > +	 * called multiple times for the same block, which simplifies the
> > +	 * implementation of algorithms and avoids hard to debug issues.
> > +	 */
> > +	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);
> 
> blocks are 8-bytes aligned, do you need the + 7.. ?

The inner data structures (e.g. rkisp1_cif_isp_bls_config) are not
marked as aligned in the UAPI.

> > +	if (size > data_.size() - used_)
> > +		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());
> 
> Isn't it better to memset the whole data block to 0 at construction
> time ?

That's more costly, given that most of the time only a small number of
blocks will be included.

> > +
> > +	struct rkisp1_ext_params_block_header *header =
> > +		reinterpret_cast<struct rkisp1_ext_params_block_header *>(block.data());
> > +	header->type = info.type;
> > +	header->enable = state == State::Enable
> > +		       ? RKISP1_EXT_PARAMS_BLOCK_ENABLE
> > +		       : RKISP1_EXT_PARAMS_BLOCK_DISABLE;
> > +	header->size = block.size();
> > +
> > +	/* Skip the block header to get the data. */
> > +	block = block.subspan(sizeof(*header));
> > +
> > +	/* 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..ddd081de7894
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/params.h
> > @@ -0,0 +1,140 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board
> > + *
> > + * RkISP1 ISP Parameters
> > + */
> > +
> > +#pragma once
> > +
> > +#include <map>
> > +#include <optional>
> 
> Do you need optional ?

I used to :-) Probably not anymore.

> > +#include <stdint.h>
> > +
> > +#include <linux/rkisp1-config.h>
> > +
> > +#include <libcamera/base/span.h>
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::rkisp1 {
> > +
> > +enum class Block {
> > +	Bls,
> > +	Dpcc,
> > +	Sdg,
> > +	AwbGain,
> > +	Flt,
> > +	Bdm,
> > +	Ctk,
> > +	Goc,
> > +	Dpf,
> > +	DpfStrength,
> > +	Cproc,
> > +	Ie,
> > +	Lsc,
> > +	Awb,
> > +	Hst,
> > +	Aec,
> > +	Afc,
> > +};
> > +
> > +namespace details {
> > +
> > +template<Block B>
> > +struct block_type {
> > +};
> > +
> > +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)		\
> > +template<>								\
> > +struct block_type<Block::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 */
> > +
> > +template<Block B>
> > +class RkISP1ParamsBlock
> > +{
> > +public:
> > +	using Type = typename details::block_type<B>::type;
> 
> clever!
> 
> I have a gut feeling we could generate the templates specializations
> of RkISP1ParamsBlock<> without going through details::block_type<> but
> this is surely nice
> 
> > +
> > +	RkISP1ParamsBlock(const Span<uint8_t> &data)
> > +		: data_(data)
> > +	{
> 
> is it necessary to expliticlty delete copy and move constructors ?

Given the current implementation copying would hurt, but I can try
disabling them.

> > +	}
> > +
> > +	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());
> > +	}
> > +
> > +private:
> > +	Span<uint8_t> data_;
> > +};
> > +
> > +class RkISP1Params
> > +{
> > +public:
> > +	enum class State {
> > +		Disable = 1,
> 
> Disable = 0,
> 
> or simply drop the initializers
> 
> very nice and elegant!
> 
> > +		Enable = 1,
> > +	};
> > +
> > +	RkISP1Params(uint32_t format, Span<uint8_t> data);
> > +
> > +	template<Block B>
> > +	RkISP1ParamsBlock<B> block(State state)
> > +	{
> > +		return RkISP1ParamsBlock<B>(block(B, state));
> > +	}
> > +
> > +	size_t size() const { return used_; }
> > +
> > +private:
> > +	Span<uint8_t> block(Block type, State state);
> > +
> > +	uint32_t format_;
> > +
> > +	Span<uint8_t> data_;
> > +	size_t used_;
> > +
> > +	std::map<Block, Span<uint8_t>> blocks_;
> > +};
> > +
> > +} /* namespace ipa::rkisp1 */
> > +
> > +} /* namespace libcamera*/

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build
index e8b266f1ccca..65eef5d69c32 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..ac25ade1c8c4
--- /dev/null
+++ b/src/ipa/rkisp1/params.cpp
@@ -0,0 +1,169 @@ 
+/* 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>
+
+namespace libcamera {
+
+namespace ipa::rkisp1 {
+
+namespace {
+
+struct BlockTypeInfo {
+	enum rkisp1_ext_params_block_type type;
+	size_t size;
+	size_t offset;
+	__u32 enableBit;
+};
+
+#define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit)			\
+	{ Block::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)				\
+	{ Block::block, {							\
+		RKISP1_EXT_PARAMS_BLOCK_TYPE_##id,				\
+		sizeof(struct rkisp1_cif_isp_##type##_config),			\
+		0, 0,								\
+	} }
+
+const std::map<Block, 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 */
+
+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);
+	}
+}
+
+Span<uint8_t> RkISP1Params::block(Block type, State state)
+{
+	auto infoIt = kBlockTypeInfo.find(type);
+	if (infoIt == kBlockTypeInfo.end())
+		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 in extended parameters only have an offset
+		 * of 0. Return nullptr in that case.
+		 */
+		if (info.offset == 0)
+			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;
+		cfg->module_ens = state == State::Enable ? info.enableBit : 0;
+
+		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. Caching helps providing the
+	 * same behaviour for the legacy and extensible formats when block() is
+	 * called multiple times for the same block, which simplifies the
+	 * implementation of algorithms and avoids hard to debug issues.
+	 */
+	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_)
+		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->enable = state == State::Enable
+		       ? RKISP1_EXT_PARAMS_BLOCK_ENABLE
+		       : RKISP1_EXT_PARAMS_BLOCK_DISABLE;
+	header->size = block.size();
+
+	/* Skip the block header to get the data. */
+	block = block.subspan(sizeof(*header));
+
+	/* 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..ddd081de7894
--- /dev/null
+++ b/src/ipa/rkisp1/params.h
@@ -0,0 +1,140 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2024, Ideas On Board
+ *
+ * RkISP1 ISP Parameters
+ */
+
+#pragma once
+
+#include <map>
+#include <optional>
+#include <stdint.h>
+
+#include <linux/rkisp1-config.h>
+
+#include <libcamera/base/span.h>
+
+namespace libcamera {
+
+namespace ipa::rkisp1 {
+
+enum class Block {
+	Bls,
+	Dpcc,
+	Sdg,
+	AwbGain,
+	Flt,
+	Bdm,
+	Ctk,
+	Goc,
+	Dpf,
+	DpfStrength,
+	Cproc,
+	Ie,
+	Lsc,
+	Awb,
+	Hst,
+	Aec,
+	Afc,
+};
+
+namespace details {
+
+template<Block B>
+struct block_type {
+};
+
+#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)		\
+template<>								\
+struct block_type<Block::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 */
+
+template<Block B>
+class RkISP1ParamsBlock
+{
+public:
+	using Type = typename details::block_type<B>::type;
+
+	RkISP1ParamsBlock(const Span<uint8_t> &data)
+		: data_(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());
+	}
+
+private:
+	Span<uint8_t> data_;
+};
+
+class RkISP1Params
+{
+public:
+	enum class State {
+		Disable = 1,
+		Enable = 1,
+	};
+
+	RkISP1Params(uint32_t format, Span<uint8_t> data);
+
+	template<Block B>
+	RkISP1ParamsBlock<B> block(State state)
+	{
+		return RkISP1ParamsBlock<B>(block(B, state));
+	}
+
+	size_t size() const { return used_; }
+
+private:
+	Span<uint8_t> block(Block type, State state);
+
+	uint32_t format_;
+
+	Span<uint8_t> data_;
+	size_t used_;
+
+	std::map<Block, Span<uint8_t>> blocks_;
+};
+
+} /* namespace ipa::rkisp1 */
+
+} /* namespace libcamera*/