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

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

Commit Message

Laurent Pinchart July 4, 2024, 4:20 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>
---
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  | 217 +++++++++++++++++++++++++++++++++++++
 src/ipa/rkisp1/params.h    | 157 +++++++++++++++++++++++++++
 3 files changed, 375 insertions(+)
 create mode 100644 src/ipa/rkisp1/params.cpp
 create mode 100644 src/ipa/rkisp1/params.h

Comments

Paul Elder July 5, 2024, 12:06 p.m. UTC | #1 
On Thu, Jul 04, 2024 at 07:20:29PM +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.

Wow that's magical (also not too bad of template magic as I expected).

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> 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  | 217 +++++++++++++++++++++++++++++++++++++
>  src/ipa/rkisp1/params.h    | 157 +++++++++++++++++++++++++++
>  3 files changed, 375 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..7155038deb18
> --- /dev/null
> +++ b/src/ipa/rkisp1/params.cpp
> @@ -0,0 +1,217 @@
> +/* 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)			\
> +	{ 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 */
> +
> +RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, Block 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 field in the block header
> +	 * directly.
> +	 */
> +	struct rkisp1_ext_params_block_header *header =
> +		reinterpret_cast<struct rkisp1_ext_params_block_header *>(header_.data());
> +	header->enable = enabled ? RKISP1_EXT_PARAMS_BLOCK_ENABLE
> +		       : RKISP1_EXT_PARAMS_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(Block 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(Block 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 in extended parameters only have an offset

s/extended/fixed/ ?

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> +		 * 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..7a7bf07299b2
> --- /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 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 */
> +
> +class RkISP1Params;
> +
> +class RkISP1ParamsBlockBase
> +{
> +public:
> +	RkISP1ParamsBlockBase(RkISP1Params *params, Block type,
> +			      const Span<uint8_t> &data);
> +
> +	Span<uint8_t> data() const { return data_; }
> +
> +	void setEnabled(bool enabled);
> +
> +private:
> +	LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase);
> +
> +	RkISP1Params *params_;
> +	Block type_;
> +	Span<uint8_t> header_;
> +	Span<uint8_t> data_;
> +};
> +
> +template<Block 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<Block 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(Block type);
> +	void setBlockEnabled(Block type, bool enabled);
> +
> +	uint32_t format_;
> +
> +	Span<uint8_t> data_;
> +	size_t used_;
> +
> +	std::map<Block, Span<uint8_t>> blocks_;
> +};
> +
> +} /* namespace ipa::rkisp1 */
> +
> +} /* namespace libcamera*/
Stefan Klug July 5, 2024, 1:24 p.m. UTC | #2 
Hi Laurent,

Thank you for the patch.

On Thu, Jul 04, 2024 at 07:20:29PM +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>
> ---
> 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  | 217 +++++++++++++++++++++++++++++++++++++
>  src/ipa/rkisp1/params.h    | 157 +++++++++++++++++++++++++++
>  3 files changed, 375 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..7155038deb18
> --- /dev/null
> +++ b/src/ipa/rkisp1/params.cpp
> @@ -0,0 +1,217 @@
> +/* 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)			\
> +	{ 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 = {

This took my brain a few cycles more. What about naming the enum
"BlockType" instead of "Block". To me mapping a BlockType to a
BlockTypeInfo feels more natural (the actual place where I stumbled is
the declaration of the block cache further down).

> +	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, Block 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 field in the block header
> +	 * directly.
> +	 */
> +	struct rkisp1_ext_params_block_header *header =
> +		reinterpret_cast<struct rkisp1_ext_params_block_header *>(header_.data());
> +	header->enable = enabled ? RKISP1_EXT_PARAMS_BLOCK_ENABLE
> +		       : RKISP1_EXT_PARAMS_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(Block 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(Block 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 in extended parameters only 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..7a7bf07299b2
> --- /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 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 */
> +
> +class RkISP1Params;
> +
> +class RkISP1ParamsBlockBase
> +{
> +public:
> +	RkISP1ParamsBlockBase(RkISP1Params *params, Block type,
> +			      const Span<uint8_t> &data);
> +
> +	Span<uint8_t> data() const { return data_; }
> +
> +	void setEnabled(bool enabled);

I like that.

> +
> +private:
> +	LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase);
> +
> +	RkISP1Params *params_;
> +	Block type_;
> +	Span<uint8_t> header_;
> +	Span<uint8_t> data_;
> +};
> +
> +template<Block 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<Block 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(Block type);
> +	void setBlockEnabled(Block type, bool enabled);
> +
> +	uint32_t format_;
> +
> +	Span<uint8_t> data_;
> +	size_t used_;
> +
> +	std::map<Block, Span<uint8_t>> blocks_;

This could become
std::map<BlockType, Span<uint8_t>> blocks_;


Whichever you prefer.

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 

Cheers,
Stefan


> +};
> +
> +} /* namespace ipa::rkisp1 */
> +
> +} /* namespace libcamera*/
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart July 5, 2024, 1:44 p.m. UTC | #3 
On Fri, Jul 05, 2024 at 09:06:17PM +0900, Paul Elder wrote:
> On Thu, Jul 04, 2024 at 07:20:29PM +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.
> 
> Wow that's magical (also not too bad of template magic as I expected).

Sometimes it's white magic :-)

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 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  | 217 +++++++++++++++++++++++++++++++++++++
> >  src/ipa/rkisp1/params.h    | 157 +++++++++++++++++++++++++++
> >  3 files changed, 375 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..7155038deb18
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/params.cpp
> > @@ -0,0 +1,217 @@
> > +/* 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)			\
> > +	{ 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 */
> > +
> > +RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, Block 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 field in the block header
> > +	 * directly.
> > +	 */
> > +	struct rkisp1_ext_params_block_header *header =
> > +		reinterpret_cast<struct rkisp1_ext_params_block_header *>(header_.data());
> > +	header->enable = enabled ? RKISP1_EXT_PARAMS_BLOCK_ENABLE
> > +		       : RKISP1_EXT_PARAMS_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(Block 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(Block 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 in extended parameters only have an offset
> 
> s/extended/fixed/ ?

No, I meant extended here. The blocks that are only available with the
extended API (e.g. companding) have an offset of 0, they're detected
here and we return an error as the fixed format doesn't support them.

> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> > +		 * 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..7a7bf07299b2
> > --- /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 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 */
> > +
> > +class RkISP1Params;
> > +
> > +class RkISP1ParamsBlockBase
> > +{
> > +public:
> > +	RkISP1ParamsBlockBase(RkISP1Params *params, Block type,
> > +			      const Span<uint8_t> &data);
> > +
> > +	Span<uint8_t> data() const { return data_; }
> > +
> > +	void setEnabled(bool enabled);
> > +
> > +private:
> > +	LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase);
> > +
> > +	RkISP1Params *params_;
> > +	Block type_;
> > +	Span<uint8_t> header_;
> > +	Span<uint8_t> data_;
> > +};
> > +
> > +template<Block 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<Block 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(Block type);
> > +	void setBlockEnabled(Block type, bool enabled);
> > +
> > +	uint32_t format_;
> > +
> > +	Span<uint8_t> data_;
> > +	size_t used_;
> > +
> > +	std::map<Block, Span<uint8_t>> blocks_;
> > +};
> > +
> > +} /* namespace ipa::rkisp1 */
> > +
> > +} /* namespace libcamera*/
Paul Elder July 11, 2024, 4:35 a.m. UTC | #4 
On Fri, Jul 05, 2024 at 04:44:36PM +0300, Laurent Pinchart wrote:
> On Fri, Jul 05, 2024 at 09:06:17PM +0900, Paul Elder wrote:
> > On Thu, Jul 04, 2024 at 07:20:29PM +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.
> > 
> > Wow that's magical (also not too bad of template magic as I expected).
> 
> Sometimes it's white magic :-)
> 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 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  | 217 +++++++++++++++++++++++++++++++++++++
> > >  src/ipa/rkisp1/params.h    | 157 +++++++++++++++++++++++++++
> > >  3 files changed, 375 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..7155038deb18
> > > --- /dev/null
> > > +++ b/src/ipa/rkisp1/params.cpp
> > > @@ -0,0 +1,217 @@
> > > +/* 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)			\
> > > +	{ 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 */
> > > +
> > > +RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, Block 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 field in the block header
> > > +	 * directly.
> > > +	 */
> > > +	struct rkisp1_ext_params_block_header *header =
> > > +		reinterpret_cast<struct rkisp1_ext_params_block_header *>(header_.data());
> > > +	header->enable = enabled ? RKISP1_EXT_PARAMS_BLOCK_ENABLE
> > > +		       : RKISP1_EXT_PARAMS_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(Block 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(Block 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 in extended parameters only have an offset
> > 
> > s/extended/fixed/ ?
> 
> No, I meant extended here. The blocks that are only available with the
> extended API (e.g. companding) have an offset of 0, they're detected
> here and we return an error as the fixed format doesn't support them.

Ah, in that case it should be "Blocks available only in extended parameters
have..."


Paul

> 
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > > +		 * 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..7a7bf07299b2
> > > --- /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 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 */
> > > +
> > > +class RkISP1Params;
> > > +
> > > +class RkISP1ParamsBlockBase
> > > +{
> > > +public:
> > > +	RkISP1ParamsBlockBase(RkISP1Params *params, Block type,
> > > +			      const Span<uint8_t> &data);
> > > +
> > > +	Span<uint8_t> data() const { return data_; }
> > > +
> > > +	void setEnabled(bool enabled);
> > > +
> > > +private:
> > > +	LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase);
> > > +
> > > +	RkISP1Params *params_;
> > > +	Block type_;
> > > +	Span<uint8_t> header_;
> > > +	Span<uint8_t> data_;
> > > +};
> > > +
> > > +template<Block 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<Block 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(Block type);
> > > +	void setBlockEnabled(Block type, bool enabled);
> > > +
> > > +	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..7155038deb18
--- /dev/null
+++ b/src/ipa/rkisp1/params.cpp
@@ -0,0 +1,217 @@ 
+/* 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)			\
+	{ 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 */
+
+RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, Block 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 field in the block header
+	 * directly.
+	 */
+	struct rkisp1_ext_params_block_header *header =
+		reinterpret_cast<struct rkisp1_ext_params_block_header *>(header_.data());
+	header->enable = enabled ? RKISP1_EXT_PARAMS_BLOCK_ENABLE
+		       : RKISP1_EXT_PARAMS_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(Block 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(Block 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 in extended parameters only 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..7a7bf07299b2
--- /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 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 */
+
+class RkISP1Params;
+
+class RkISP1ParamsBlockBase
+{
+public:
+	RkISP1ParamsBlockBase(RkISP1Params *params, Block type,
+			      const Span<uint8_t> &data);
+
+	Span<uint8_t> data() const { return data_; }
+
+	void setEnabled(bool enabled);
+
+private:
+	LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase);
+
+	RkISP1Params *params_;
+	Block type_;
+	Span<uint8_t> header_;
+	Span<uint8_t> data_;
+};
+
+template<Block 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<Block 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(Block type);
+	void setBlockEnabled(Block type, bool enabled);
+
+	uint32_t format_;
+
+	Span<uint8_t> data_;
+	size_t used_;
+
+	std::map<Block, Span<uint8_t>> blocks_;
+};
+
+} /* namespace ipa::rkisp1 */
+
+} /* namespace libcamera*/