[PATCH v1 05/11] ipa: rkisp1: Add ISP parameters abstraction class
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Thu Jul 4 13:50:25 CEST 2024
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 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;
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
>
More information about the libcamera-devel
mailing list