[PATCH v1 05/11] ipa: rkisp1: Add ISP parameters abstraction class
Stefan Klug
stefan.klug at ideasonboard.com
Thu Jul 4 11:58:10 CEST 2024
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 at 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
>
More information about the libcamera-devel
mailing list