Message ID | 20250829-v4l2-params-v1-3-340773fb69ff@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi 2025. 08. 29. 13:54 keltezéssel, Jacopo Mondi írta: > The existing RkISP1Params helper classes allows the RkISP1 to handle > V4L2 extensible parameters format and the legacy RkIPS1-specific > fixed-size parameters format. > > With the introduction of v4l2-params in the Linux kernel the part of > the RkISP1Params helper class that handles extensible parameters can > be generalized so that other IPA modules can use the same helpers > to populate a v4l2-params compatible buffer. > > Generalize the RkISP1Params class to a new libipa component named > V4L2Params and derive the existing RkISP1Params from it, leaving > in the RkISP1-specific implementation the handling of the legacy format. > > Deriving RkISP1Params from V4L2Params requires changing the size > associated to each block to include the size of v4l2_params_block_header > in the ipa:rkisp1::kBlockTypeInfo map as the V4L2Params::block() > implementation doesn't account for that as RkIS1Params::block() > implementation did. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/ipa/libipa/meson.build | 2 + > src/ipa/libipa/v4l2_params.cpp | 252 +++++++++++++++++++++++++++++++++++++++++ > src/ipa/libipa/v4l2_params.h | 135 ++++++++++++++++++++++ > src/ipa/rkisp1/params.cpp | 93 +-------------- > src/ipa/rkisp1/params.h | 108 ++++++++---------- > 5 files changed, 438 insertions(+), 152 deletions(-) > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build > index 660be94054fa98b714b6bc586039081e45a6b4bc..4010739e710eb38aa6108eb8258c574a616bf3c0 100644 > --- a/src/ipa/libipa/meson.build > +++ b/src/ipa/libipa/meson.build > @@ -16,6 +16,7 @@ libipa_headers = files([ > 'lsc_polynomial.h', > 'lux.h', > 'module.h', > + 'v4l2_params.h', > 'pwl.h', > ]) > > @@ -35,6 +36,7 @@ libipa_sources = files([ > 'lsc_polynomial.cpp', > 'lux.cpp', > 'module.cpp', > + 'v4l2_params.cpp', > 'pwl.cpp', > ]) > > diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp > new file mode 100644 > index 0000000000000000000000000000000000000000..674018065ace0e1b6b48b1630e556cef590d1e84 > --- /dev/null > +++ b/src/ipa/libipa/v4l2_params.cpp > @@ -0,0 +1,252 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2025, Ideas On Board > + * > + * V4L2 Parameters > + */ > + > +#include "v4l2_params.h" > + > +namespace libcamera { > + > +namespace ipa { > + > +/** > + * \file v4l2_params.cpp > + * \brief Helper class to populate a v4l2-params compatible parameters buffer > + * > + * The Linux kernel defines a generic buffer format for configuring ISP devices > + * through a set of parameters in the form of V4L2 extensible parameters. The > + * V4L2 extensible parameters define a serialization format for ISP parameters > + * that allows userspace to populate a buffer of configuration data by appending > + * them one after the other in a binary buffer. > + * > + * Each ISP driver compatible with the v4l2-params format will define its own > + * meta-output format identifier and defines the types of the configuration data > + * of each ISP block that usually match the registers layout. > + * > + * The V4L2Params class represent the V4L2 extensible parameters buffer and > + * allows users to populate the ISP configuration blocks, represented by the > + * V4L2ParamBlock class instances. > + * > + * IPA implementations using this helpers should define an enumeration of ISP > + * blocks the IPA module supports and use a set of common abstraction to help > + * their derived implementation of V4L2Params translate the enumerated ISP block > + * identifier to the actual type of the configuration data as defined by the > + * kernel interface. > + * > + * As an example of this see the RkISP1 and Mali-C55 implementations. > + */ > + > +/** > + * \class V4L2ParamsBlock > + * \brief Helper class that represents a ISP configuration block > + * > + * Each ISP function is associated with a set of configuration parameters > + * defined by the kernel interface. > + * > + * This class represents an ISP block configuration entry. It is constructed > + * with a reference to the memory area where the block configuration will be > + * stored in the parameters buffer. The template parameter represents > + * the underlying kernel-defined ISP block configuration type and allow its > + * user to easily cast it to said type to populate and read the configuration > + * parameters. > + * > + * \sa V4L2Params::block() > + */ > + > +/** > + * \fn V4L2ParamsBlock::V4L2ParamsBlock() > + * \brief Construct a V4L2ParamsBlock with memory represented by \a data > + * \param[in] data A view on the memory area where the ISP block is located > + */ > + > +/** > + * \fn V4L2ParamsBlock::setEnabled() > + * \brief Enable/disable an ISP configuration block > + * \param[in] enabled The enable flag > + */ > + > +/** > + * \fn V4L2ParamsBlock::header() > + * \brief Retrieve a reference to the header (struct v4l2_params_block_header) > + * \return The block header > + */ > + > +/** > + * \fn V4L2ParamsBlock::data() > + * \brief Retrieve a reference to block configuration data memory area > + * \return The block data > + */ > + > +/** > + * \fn V4L2ParamsBlock::operator->() > + * \brief Access the ISP configuration block casting it to the kernel-defined > + * ISP configuration type > + * > + * The V4L2ParamsBlock is templated with the kernel defined ISP configuration > + * block type. This function allows users to easily cast a V4L2ParamsBlock to > + * the underlying kernel-defined type in order to easily populate or read > + * the ISP configuration data. > + * > + * \code{.cpp} > + * > + * // The kernel header defines the ISP configuration types, in example > + * // struct my_isp_awb_config_data { > + * // u16 gain_ch00; > + * // u16 gain_ch01; > + * // u16 gain_ch10; > + * // u16 gain_ch11; > + * // > + * // } > + * > + * template<> V4L2ParamsBlock<struct my_isp_awb_config_data> awbBlock > + * > + * awbBlock->gain_ch00 = ...; > + * awbBlock->gain_ch01 = ...; > + * awbBlock->gain_ch10 = ...; > + * awbBlock->gain_ch11 = ...; > + * > + * \endcode > + * > + * Users of this class are not expected to create a V4L2ParamsBlock manually but > + * should rather use V4L2Params::block() to retrieve a reference to the memory > + * area used to construct a V4L2ParamsBlock<T> in their overloaded > + * implementation of V4L2Params::block(). > + */ > + > +/** > + * \fn V4L2ParamsBlock::operator->() const > + * \copydoc V4L2ParamsBlock::operator->() > + */ > + > +/** > + * \fn V4L2ParamsBlock::operator*() const > + * \copydoc V4L2ParamsBlock::operator->() > + */ > + > +/** > + * \fn V4L2ParamsBlock::operator*() > + * \copydoc V4L2ParamsBlock::operator->() > + */ > + > + /** > + * \class V4L2Params > + * \brief Helper class that represent an ISP configuration buffer > + * > + * ISP implementation compatible with v4l2-params define their ISP configuration > + * buffer types compatible with the struct v4l2_params_buffer type. > + * > + * This class represents an ISP configuration buffer. It is constructed > + * with a reference to the memory mapped buffer that will be queued to the ISP. > + * > + * This class is templated with the type of the enumeration of ISP blocks that > + * each IPA module is expected to support. IPA modules are expected to derive > + * this class and use the V4L2Params::block() function to retrieve the memory > + * area for each ISP configuration block and use it to construct a > + * V4L2ParamsBlock<T> with it before returning it to the user. > + * > + * \code{.cpp} > + * > + * enum class myISPBlocks { > + * Agc, > + * Awb, > + * ... > + * }; > + * > + * template<myISPBlocks B> > + * struct block_type { > + * }; > + * > + * template<> > + * struct block_type<myISPBlock::Agc> { > + * using type = struct my_isp_kernel_config_type_agc; > + * }; > + * > + * template<> > + * struct block_type<myISPBlock::Awb> { > + * using type = struct my_isp_kernel_config_type_awb; > + * }; > + * > + * ... > + * > + * class MyISPParams : public V4L2Params<myISPBlocks> > + * { > + * public: > + * template<myISPBlocks B> > + * auto block() > + * { > + * > + * // Use the kernel defined configuration type as template > + * // argument to V4L2ParamsBlock. > + * using Type = typename details::block_type<B>::type; > + * > + * // Each IPA module should provide the information required > + * // to populate the block header > + * > + * ... > + * > + * auto data = V4L2Params::block(B, blockType, blockSize); > + * > + * return V4L2ParamsBlock<Type>(data); > + * } > + * }; > + * > + * \endcode > + * > + * As an example, see the RkISP1Params and MaliC55Params implementations. > + */ > + > +/** > + * \fn V4L2Params::V4L2Params() > + * \brief Construct a V4L2Params > + * \param[in] data Reference to the v4l2-buffer memory mapped area > + * \param[in] version The ISP parameters version the implementation supports > + */ > + > +/** > + * \fn V4L2Params::size() > + * \brief Retrieve the used size of the parameters buffer (in bytes) > + * > + * The parameters buffer size is mostly used to populate the v4l2_buffer > + * bytesused field before queueing the buffer to the ISP. > + * > + * \return The number of bytes occupied by the ISP configuration parameters > + */ > + > +/** > + * \fn V4L2Params::block() > + * \brief Populate an ISP configuration block a returns a reference to its > + * memory > + * \param[in] type The ISP block identifier enumerated by the IPA module > + * \param[in] blockType The kernel-defined ISP block identifier, used to > + * populate the block header > + * \param[in] blockSize The ISP block size, used to populate the block header > + * > + * > + * Initialize the block header with \a blockType and \a blockSize and > + * returns a reference to the memory used to store an ISP configuration block. > + * > + * IPA modules that derive the V4L2Params class shall use this function to > + * retrieve the memory area that will be used to construct a V4L2ParamsBlock<T> > + * before returning it to the caller. > + */ > + > +/** > + * \var V4L2Params::data_ > + * \brief The ISP parameters buffer memory > + */ > + > +/** > + * \var V4L2Params::used_ > + * \brief The number of bytes used in the parameters buffer > + */ > + > +/** > + * \var V4L2Params::blocks_ > + * \brief Cache of ISP configuration blocks > + */ > + > +} /* namespace ipa */ > + > +} /* namespace libcamera */ > diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h > new file mode 100644 > index 0000000000000000000000000000000000000000..5586096c7ee8a2d20877838564e8074e0fc3d1ce > --- /dev/null > +++ b/src/ipa/libipa/v4l2_params.h > @@ -0,0 +1,135 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2025, Ideas On Board > + * > + * V4L2 Parameters > + */ > + > +#pragma once > + > +#include <map> > +#include <stdint.h> > +#include <string.h> > + > +#include <linux/media/v4l2-extensible-params.h> > + > +#include <libcamera/base/class.h> > +#include <libcamera/base/span.h> > + > +namespace libcamera { > + > +namespace ipa { > + > +template<typename T> > +class V4L2ParamsBlock > +{ > +public: > + V4L2ParamsBlock(const Span<uint8_t> &data) I think taking by value is fine, no need for the const ref. > + { > + header_ = data.subspan(0, sizeof(v4l2_params_block_header)); > + data_ = data.subspan(sizeof(v4l2_params_block_header)); I'd probably do these in the member init list. > + } > + > + void setEnabled(bool enabled) > + { > + struct v4l2_params_block_header *header = > + reinterpret_cast<struct v4l2_params_block_header *>(header_.data()); > + > + header->flags &= ~(V4L2_PARAMS_FL_BLOCK_ENABLE | > + V4L2_PARAMS_FL_BLOCK_DISABLE); > + header->flags |= enabled ? V4L2_PARAMS_FL_BLOCK_ENABLE > + : V4L2_PARAMS_FL_BLOCK_DISABLE; > + } > + > + Span<uint8_t> header() const { return header_; } > + Span<uint8_t> data() const { return data_; } > + > + const T *operator->() const > + { > + return reinterpret_cast<const T *>(data().data()); > + } > + > + T *operator->() > + { > + return reinterpret_cast<T *>(data().data()); > + } > + > + const T &operator*() const > + { > + return *reinterpret_cast<const T *>(data().data()); > + } > + > + T &operator*() > + { > + return *reinterpret_cast<T *>(data().data()); > + } > + > +private: > + LIBCAMERA_DISABLE_COPY(V4L2ParamsBlock) Why disable? I would assume that it's OK to copy this? > + > + Span<uint8_t> header_; > + Span<uint8_t> data_; > +}; > + > +template<typename T> > +class V4L2Params > +{ > +public: > + V4L2Params(Span<uint8_t> data, unsigned int version) > + : data_(data) > + { > + struct v4l2_params_buffer *cfg = > + reinterpret_cast<struct v4l2_params_buffer *>(data_.data()); > + cfg->data_size = 0; > + cfg->version = version; > + used_ = offsetof(struct v4l2_params_buffer, data); > + } > + > + size_t size() const { return used_; } > + > +protected: > + Span<uint8_t> block(T type, unsigned int blockType, size_t blockSize) This can modified so that the derived classes need not implement any extra logic. One option is to pass the `details::block_type` template as a template argument to V4L2Params and then use that to map the id to the type: template<typename T, template<T> typename IdToType> class V4L2Params { template<T ID> auto block() { using Type = typename IdToType<ID>::type; ... } but I think it's simpler and more extensible to pass another type that holds all this compile-time information. It could look something like this: diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h index 5586096c7..019f06509 100644 --- a/src/ipa/libipa/v4l2_params.h +++ b/src/ipa/libipa/v4l2_params.h @@ -71,7 +71,7 @@ private: Span<uint8_t> data_; }; -template<typename T> +template<typename Traits> class V4L2Params { public: @@ -87,8 +87,21 @@ public: size_t size() const { return used_; } + template<typename Traits::id_type Id> + auto block() + { + using Details = typename Traits::template id_to_details<Id>; + + using Type = typename Details::type; + constexpr auto kernelId = Details::blockType; + + auto data = block(Id, kernelId, sizeof(Type)); + + return V4L2ParamsBlock<Type>(data); + } + protected: - Span<uint8_t> block(T type, unsigned int blockType, size_t blockSize) + Span<uint8_t> block(typename Traits::id_type type, unsigned int blockType, size_t blockSize) { /* * Look up the block in the cache first. If an algorithm @@ -127,7 +140,7 @@ protected: Span<uint8_t> data_; size_t used_; - std::map<T, Span<uint8_t>> blocks_; + std::map<typename Traits::id_type, Span<uint8_t>> blocks_; }; } /* namespace ipa */ diff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h index 1cc56dbad..975122999 100644 --- a/src/ipa/mali-c55/params.h +++ b/src/ipa/mali-c55/params.h @@ -58,30 +58,24 @@ MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingConfig, mesh_shading_config, MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingSel, mesh_shading_selection, MESH_SHADING_SELECTION) +struct params_traits { + using id_type = MaliC55Blocks; + + template<id_type Id> + using id_to_details = block_type<Id>; +}; + } /* namespace details */ -class MaliC55Params : public V4L2Params<MaliC55Blocks> +class MaliC55Params : public V4L2Params<details::params_traits> { public: static constexpr unsigned int kVersion = MALI_C55_PARAM_BUFFER_V1; MaliC55Params(Span<uint8_t> data) - : V4L2Params<MaliC55Blocks>(data, kVersion) + : V4L2Params(data, kVersion) { } - - template<MaliC55Blocks B> - auto block() - { - using Type = typename details::block_type<B>::type; - - auto blockType = details::block_type<B>::blockType; - size_t blockSize = sizeof(Type); - - auto data = V4L2Params::block(B, blockType, blockSize); - - return V4L2ParamsBlock<Type>(data); - } }; } /* namespace ipa::mali_c55 */ diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h index 7a2127664..824a5924f 100644 --- a/src/ipa/rkisp1/params.h +++ b/src/ipa/rkisp1/params.h @@ -45,45 +45,54 @@ template<BlockType B> struct block_type { }; -#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct) \ +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct, id) \ template<> \ struct block_type<BlockType::blockType> { \ using type = struct rkisp1_cif_isp_##blockStruct##_config; \ + static constexpr rkisp1_ext_params_block_type blockType = \ + RKISP1_EXT_PARAMS_BLOCK_TYPE_##id; \ }; -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) -RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls) -RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve) -RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve) +RKISP1_DEFINE_BLOCK_TYPE(Bls, bls, BLS) +RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc, DPCC) +RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg, SDG) +RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain, AWB_GAIN) +RKISP1_DEFINE_BLOCK_TYPE(Flt, flt, FLT) +RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm, BDM) +RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk, CTK) +RKISP1_DEFINE_BLOCK_TYPE(Goc, goc, GOC) +RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf, DPF) +RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength, DPF_STRENGTH) +RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc, CPROC) +RKISP1_DEFINE_BLOCK_TYPE(Ie, ie, IE) +RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc, LSC) +RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas, AWB_MEAS) +RKISP1_DEFINE_BLOCK_TYPE(Hst, hst, HST_MEAS) +RKISP1_DEFINE_BLOCK_TYPE(Aec, aec, AEC_MEAS) +RKISP1_DEFINE_BLOCK_TYPE(Afc, afc, AFC_MEAS) +RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls, COMPAND_BLS) +RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve, COMPAND_EXPAND) +RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve, COMPAND_COMPRESS) + +struct params_traits { + using id_type = BlockType; + + template<id_type Id> + using id_to_details = block_type<Id>; +}; } /* namespace details */ template<typename T> class RkISP1ParamsBlock; -class RkISP1Params : public V4L2Params<BlockType> +class RkISP1Params : public V4L2Params<details::params_traits> { public: static constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1; RkISP1Params(uint32_t format, Span<uint8_t> data) - : V4L2Params<BlockType>(data, kVersion), format_(format) + : V4L2Params(data, kVersion), format_(format) { if (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) { memset(data.data(), 0, data.size()); Admittedly I haven't tested it, and the rkisp1 parts are not there yet. But I believe this direction could simplify the code and make it easier to implement for other parameter sets. For rkisp1, I think as a first step `kBlockTypeInfo` should be moved completely into `rkisp1::details::block_type` template specializations to do the mapping at compile time, similarly to how the proposed `MaliC55Params` does it. Then I believe one could realistically extend this `V4L2Params` to even support fixed-offset formats generically, if needed, but that might be an overkill. > + { > + /* > + * 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. */ > + if (blockSize > data_.size() - used_) > + return {}; > + > + /* Allocate a new block, clear its memory, and initialize its header. */ > + Span<uint8_t> block = data_.subspan(used_, blockSize); > + used_ += blockSize; > + > + struct v4l2_params_buffer *cfg = > + reinterpret_cast<struct v4l2_params_buffer *>(data_.data()); > + cfg->data_size += blockSize; > + > + memset(block.data(), 0, block.size()); > + > + struct v4l2_params_block_header *header = > + reinterpret_cast<struct v4l2_params_block_header *>(block.data()); > + header->type = blockType; > + header->size = block.size(); > + > + /* Update the cache. */ > + blocks_[type] = block; > + > + return block; > + } > + > + Span<uint8_t> data_; > + size_t used_; > + > + std::map<T, Span<uint8_t>> blocks_; > +}; > + > +} /* namespace ipa */ > + > +} /* namespace libcamera */ > diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp > index 4c0b051ce65da1686323ee9c66b82e12669a754d..2b692e1a1f199d6c118af0938e1aaecef6186a2c 100644 > --- a/src/ipa/rkisp1/params.cpp > +++ b/src/ipa/rkisp1/params.cpp > @@ -35,7 +35,7 @@ struct BlockTypeInfo { > #define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit) \ > { BlockType::block, { \ > RKISP1_EXT_PARAMS_BLOCK_TYPE_##id, \ > - sizeof(struct rkisp1_cif_isp_##type##_config), \ > + sizeof(struct rkisp1_ext_params_##type##_config), \ > offsetof(struct rkisp1_params_cfg, category.type##_config), \ > RKISP1_CIF_ISP_MODULE_##bit, \ > } } > @@ -49,7 +49,7 @@ struct BlockTypeInfo { > #define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type) \ > { BlockType::block, { \ > RKISP1_EXT_PARAMS_BLOCK_TYPE_##id, \ > - sizeof(struct rkisp1_cif_isp_##type##_config), \ > + sizeof(struct rkisp1_ext_params_##type##_config), \ > 0, 0, \ > } } > > @@ -78,56 +78,6 @@ const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = { > > } /* namespace */ > > -RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type, > - const Span<uint8_t> &data) > - : params_(params), type_(type) > -{ > - if (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) { > - header_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header)); > - data_ = data.subspan(sizeof(rkisp1_ext_params_block_header)); > - } else { > - data_ = data; > - } > -} > - > -void RkISP1ParamsBlockBase::setEnabled(bool enabled) > -{ > - /* > - * For the legacy fixed format, blocks are enabled in the top-level > - * header. Delegate to the RkISP1Params class. > - */ > - if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS) > - return params_->setBlockEnabled(type_, enabled); > - > - /* > - * For the extensible format, set the enable and disable flags in the > - * block header directly. > - */ > - struct rkisp1_ext_params_block_header *header = > - reinterpret_cast<struct rkisp1_ext_params_block_header *>(header_.data()); > - header->flags &= ~(RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE | > - RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE); > - header->flags |= enabled ? RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE > - : RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE; > -} > - > -RkISP1Params::RkISP1Params(uint32_t format, Span<uint8_t> data) > - : format_(format), data_(data), used_(0) > -{ > - if (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) { > - struct rkisp1_ext_params_cfg *cfg = > - reinterpret_cast<struct rkisp1_ext_params_cfg *>(data.data()); > - > - cfg->version = RKISP1_EXT_PARAM_BUFFER_V1; > - cfg->data_size = 0; > - > - used_ += offsetof(struct rkisp1_ext_params_cfg, data); > - } else { > - memset(data.data(), 0, data.size()); > - used_ = sizeof(struct rkisp1_params_cfg); > - } > -} > - > void RkISP1Params::setBlockEnabled(BlockType type, bool enabled) > { > const BlockTypeInfo &info = kBlockTypeInfo.at(type); > @@ -177,44 +127,7 @@ Span<uint8_t> RkISP1Params::block(BlockType type) > 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; > + return V4L2Params::block(type, info.type, info.size); > } > > } /* namespace ipa::rkisp1 */ > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h > index 40450e34497a3aa71b5b0cda2bf045a1cc0e012f..7a21276648162a127317c75cb01d1c0059b96ee1 100644 > --- a/src/ipa/rkisp1/params.h > +++ b/src/ipa/rkisp1/params.h > @@ -7,13 +7,10 @@ > > #pragma once > > -#include <map> > -#include <stdint.h> > - > #include <linux/rkisp1-config.h> > +#include <linux/videodev2.h> > > -#include <libcamera/base/class.h> > -#include <libcamera/base/span.h> > +#include <libipa/v4l2_params.h> > > namespace libcamera { > > @@ -77,85 +74,72 @@ RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve) > > } /* namespace details */ > > -class RkISP1Params; > +template<typename T> > +class RkISP1ParamsBlock; > > -class RkISP1ParamsBlockBase > +class RkISP1Params : public V4L2Params<BlockType> > { > public: > - RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type, > - const Span<uint8_t> &data); > - > - Span<uint8_t> data() const { return data_; } > - > - void setEnabled(bool enabled); > + static constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1; > > -private: > - LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase) > - > - RkISP1Params *params_; > - BlockType type_; > - Span<uint8_t> header_; > - Span<uint8_t> data_; > -}; > - > -template<BlockType B> > -class RkISP1ParamsBlock : public RkISP1ParamsBlockBase > -{ > -public: > - using Type = typename details::block_type<B>::type; > - > - RkISP1ParamsBlock(RkISP1Params *params, const Span<uint8_t> &data) > - : RkISP1ParamsBlockBase(params, B, data) > + RkISP1Params(uint32_t format, Span<uint8_t> data) > + : V4L2Params<BlockType>(data, kVersion), format_(format) > { > + if (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) { > + memset(data.data(), 0, data.size()); > + used_ = sizeof(struct rkisp1_params_cfg); > + } > } > > - const Type *operator->() const > + template<BlockType B> > + auto block() > { > - return reinterpret_cast<const Type *>(data().data()); > - } > + using Type = typename details::block_type<B>::type; > > - Type *operator->() > - { > - return reinterpret_cast<Type *>(data().data()); > + return RkISP1ParamsBlock<Type>(this, B, block(B)); > } > > - const Type &operator*() const & > - { > - return *reinterpret_cast<const Type *>(data().data()); > - } > + uint32_t format() const { return format_; } > + void setBlockEnabled(BlockType type, bool enabled); > > - Type &operator*() & > - { > - return *reinterpret_cast<Type *>(data().data()); > - } > +private: > + Span<uint8_t> block(BlockType type); > + > + uint32_t format_; > }; > > -class RkISP1Params > +template<typename T> > +class RkISP1ParamsBlock : public V4L2ParamsBlock<T> > { > public: > - RkISP1Params(uint32_t format, Span<uint8_t> data); > - > - template<BlockType B> > - RkISP1ParamsBlock<B> block() > + RkISP1ParamsBlock(RkISP1Params *params, BlockType type, > + const Span<uint8_t> &data) > + : V4L2ParamsBlock<T>(data) > { > - return RkISP1ParamsBlock<B>(this, block(B)); > + params_ = params; > + type_ = type; > + > + /* Legacy param format has no header */ > + if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS) > + data_ = data; > } > > - uint32_t format() const { return format_; } > - size_t size() const { return used_; } > + void 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); > + > + return V4L2ParamsBlock<T>::setEnabled(enabled); > + } I have one concern regarding this design. An `RkISP1ParamsBlock` instance will not work as its base type correctly in all cases. That is, calling the base class `setEnabled()`: RkISP1ParamsBlock<...> *p = ...; p->V4L2ParamsBlock<...>::setEnabled(...); // or static_cast<V4L2ParamsBlock<...> *>(p)->setEnabled(...); will not work correctly if the format is the "non-extensible" one. Regards, Barnabás Pőcze > > private: > - friend class RkISP1ParamsBlockBase; > - > - Span<uint8_t> block(BlockType type); > - void setBlockEnabled(BlockType type, bool enabled); > - > - uint32_t format_; > - > + RkISP1Params *params_; > + BlockType type_; > Span<uint8_t> data_; > - size_t used_; > - > - std::map<BlockType, Span<uint8_t>> blocks_; > }; > > } /* namespace ipa::rkisp1 */ >
Hi Barnabás thanks for review On Mon, Sep 08, 2025 at 06:09:08PM +0200, Barnabás Pőcze wrote: > Hi > > 2025. 08. 29. 13:54 keltezéssel, Jacopo Mondi írta: > > The existing RkISP1Params helper classes allows the RkISP1 to handle > > V4L2 extensible parameters format and the legacy RkIPS1-specific > > fixed-size parameters format. > > > > With the introduction of v4l2-params in the Linux kernel the part of > > the RkISP1Params helper class that handles extensible parameters can > > be generalized so that other IPA modules can use the same helpers > > to populate a v4l2-params compatible buffer. > > > > Generalize the RkISP1Params class to a new libipa component named > > V4L2Params and derive the existing RkISP1Params from it, leaving > > in the RkISP1-specific implementation the handling of the legacy format. > > > > Deriving RkISP1Params from V4L2Params requires changing the size > > associated to each block to include the size of v4l2_params_block_header > > in the ipa:rkisp1::kBlockTypeInfo map as the V4L2Params::block() > > implementation doesn't account for that as RkIS1Params::block() > > implementation did. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > src/ipa/libipa/meson.build | 2 + > > src/ipa/libipa/v4l2_params.cpp | 252 +++++++++++++++++++++++++++++++++++++++++ > > src/ipa/libipa/v4l2_params.h | 135 ++++++++++++++++++++++ > > src/ipa/rkisp1/params.cpp | 93 +-------------- > > src/ipa/rkisp1/params.h | 108 ++++++++---------- > > 5 files changed, 438 insertions(+), 152 deletions(-) > > > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build > > index 660be94054fa98b714b6bc586039081e45a6b4bc..4010739e710eb38aa6108eb8258c574a616bf3c0 100644 > > --- a/src/ipa/libipa/meson.build > > +++ b/src/ipa/libipa/meson.build > > @@ -16,6 +16,7 @@ libipa_headers = files([ > > 'lsc_polynomial.h', > > 'lux.h', > > 'module.h', > > + 'v4l2_params.h', > > 'pwl.h', > > ]) > > @@ -35,6 +36,7 @@ libipa_sources = files([ > > 'lsc_polynomial.cpp', > > 'lux.cpp', > > 'module.cpp', > > + 'v4l2_params.cpp', > > 'pwl.cpp', > > ]) > > diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp > > new file mode 100644 > > index 0000000000000000000000000000000000000000..674018065ace0e1b6b48b1630e556cef590d1e84 > > --- /dev/null > > +++ b/src/ipa/libipa/v4l2_params.cpp > > @@ -0,0 +1,252 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2025, Ideas On Board > > + * > > + * V4L2 Parameters > > + */ > > + > > +#include "v4l2_params.h" > > + > > +namespace libcamera { > > + > > +namespace ipa { > > + > > +/** > > + * \file v4l2_params.cpp > > + * \brief Helper class to populate a v4l2-params compatible parameters buffer > > + * > > + * The Linux kernel defines a generic buffer format for configuring ISP devices > > + * through a set of parameters in the form of V4L2 extensible parameters. The > > + * V4L2 extensible parameters define a serialization format for ISP parameters > > + * that allows userspace to populate a buffer of configuration data by appending > > + * them one after the other in a binary buffer. > > + * > > + * Each ISP driver compatible with the v4l2-params format will define its own > > + * meta-output format identifier and defines the types of the configuration data > > + * of each ISP block that usually match the registers layout. > > + * > > + * The V4L2Params class represent the V4L2 extensible parameters buffer and > > + * allows users to populate the ISP configuration blocks, represented by the > > + * V4L2ParamBlock class instances. > > + * > > + * IPA implementations using this helpers should define an enumeration of ISP > > + * blocks the IPA module supports and use a set of common abstraction to help > > + * their derived implementation of V4L2Params translate the enumerated ISP block > > + * identifier to the actual type of the configuration data as defined by the > > + * kernel interface. > > + * > > + * As an example of this see the RkISP1 and Mali-C55 implementations. > > + */ > > + > > +/** > > + * \class V4L2ParamsBlock > > + * \brief Helper class that represents a ISP configuration block > > + * > > + * Each ISP function is associated with a set of configuration parameters > > + * defined by the kernel interface. > > + * > > + * This class represents an ISP block configuration entry. It is constructed > > + * with a reference to the memory area where the block configuration will be > > + * stored in the parameters buffer. The template parameter represents > > + * the underlying kernel-defined ISP block configuration type and allow its > > + * user to easily cast it to said type to populate and read the configuration > > + * parameters. > > + * > > + * \sa V4L2Params::block() > > + */ > > + > > +/** > > + * \fn V4L2ParamsBlock::V4L2ParamsBlock() > > + * \brief Construct a V4L2ParamsBlock with memory represented by \a data > > + * \param[in] data A view on the memory area where the ISP block is located > > + */ > > + > > +/** > > + * \fn V4L2ParamsBlock::setEnabled() > > + * \brief Enable/disable an ISP configuration block > > + * \param[in] enabled The enable flag > > + */ > > + > > +/** > > + * \fn V4L2ParamsBlock::header() > > + * \brief Retrieve a reference to the header (struct v4l2_params_block_header) > > + * \return The block header > > + */ > > + > > +/** > > + * \fn V4L2ParamsBlock::data() > > + * \brief Retrieve a reference to block configuration data memory area > > + * \return The block data > > + */ > > + > > +/** > > + * \fn V4L2ParamsBlock::operator->() > > + * \brief Access the ISP configuration block casting it to the kernel-defined > > + * ISP configuration type > > + * > > + * The V4L2ParamsBlock is templated with the kernel defined ISP configuration > > + * block type. This function allows users to easily cast a V4L2ParamsBlock to > > + * the underlying kernel-defined type in order to easily populate or read > > + * the ISP configuration data. > > + * > > + * \code{.cpp} > > + * > > + * // The kernel header defines the ISP configuration types, in example > > + * // struct my_isp_awb_config_data { > > + * // u16 gain_ch00; > > + * // u16 gain_ch01; > > + * // u16 gain_ch10; > > + * // u16 gain_ch11; > > + * // > > + * // } > > + * > > + * template<> V4L2ParamsBlock<struct my_isp_awb_config_data> awbBlock > > + * > > + * awbBlock->gain_ch00 = ...; > > + * awbBlock->gain_ch01 = ...; > > + * awbBlock->gain_ch10 = ...; > > + * awbBlock->gain_ch11 = ...; > > + * > > + * \endcode > > + * > > + * Users of this class are not expected to create a V4L2ParamsBlock manually but > > + * should rather use V4L2Params::block() to retrieve a reference to the memory > > + * area used to construct a V4L2ParamsBlock<T> in their overloaded > > + * implementation of V4L2Params::block(). > > + */ > > + > > +/** > > + * \fn V4L2ParamsBlock::operator->() const > > + * \copydoc V4L2ParamsBlock::operator->() > > + */ > > + > > +/** > > + * \fn V4L2ParamsBlock::operator*() const > > + * \copydoc V4L2ParamsBlock::operator->() > > + */ > > + > > +/** > > + * \fn V4L2ParamsBlock::operator*() > > + * \copydoc V4L2ParamsBlock::operator->() > > + */ > > + > > + /** > > + * \class V4L2Params > > + * \brief Helper class that represent an ISP configuration buffer > > + * > > + * ISP implementation compatible with v4l2-params define their ISP configuration > > + * buffer types compatible with the struct v4l2_params_buffer type. > > + * > > + * This class represents an ISP configuration buffer. It is constructed > > + * with a reference to the memory mapped buffer that will be queued to the ISP. > > + * > > + * This class is templated with the type of the enumeration of ISP blocks that > > + * each IPA module is expected to support. IPA modules are expected to derive > > + * this class and use the V4L2Params::block() function to retrieve the memory > > + * area for each ISP configuration block and use it to construct a > > + * V4L2ParamsBlock<T> with it before returning it to the user. > > + * > > + * \code{.cpp} > > + * > > + * enum class myISPBlocks { > > + * Agc, > > + * Awb, > > + * ... > > + * }; > > + * > > + * template<myISPBlocks B> > > + * struct block_type { > > + * }; > > + * > > + * template<> > > + * struct block_type<myISPBlock::Agc> { > > + * using type = struct my_isp_kernel_config_type_agc; > > + * }; > > + * > > + * template<> > > + * struct block_type<myISPBlock::Awb> { > > + * using type = struct my_isp_kernel_config_type_awb; > > + * }; > > + * > > + * ... > > + * > > + * class MyISPParams : public V4L2Params<myISPBlocks> > > + * { > > + * public: > > + * template<myISPBlocks B> > > + * auto block() > > + * { > > + * > > + * // Use the kernel defined configuration type as template > > + * // argument to V4L2ParamsBlock. > > + * using Type = typename details::block_type<B>::type; > > + * > > + * // Each IPA module should provide the information required > > + * // to populate the block header > > + * > > + * ... > > + * > > + * auto data = V4L2Params::block(B, blockType, blockSize); > > + * > > + * return V4L2ParamsBlock<Type>(data); > > + * } > > + * }; > > + * > > + * \endcode > > + * > > + * As an example, see the RkISP1Params and MaliC55Params implementations. > > + */ > > + > > +/** > > + * \fn V4L2Params::V4L2Params() > > + * \brief Construct a V4L2Params > > + * \param[in] data Reference to the v4l2-buffer memory mapped area > > + * \param[in] version The ISP parameters version the implementation supports > > + */ > > + > > +/** > > + * \fn V4L2Params::size() > > + * \brief Retrieve the used size of the parameters buffer (in bytes) > > + * > > + * The parameters buffer size is mostly used to populate the v4l2_buffer > > + * bytesused field before queueing the buffer to the ISP. > > + * > > + * \return The number of bytes occupied by the ISP configuration parameters > > + */ > > + > > +/** > > + * \fn V4L2Params::block() > > + * \brief Populate an ISP configuration block a returns a reference to its > > + * memory > > + * \param[in] type The ISP block identifier enumerated by the IPA module > > + * \param[in] blockType The kernel-defined ISP block identifier, used to > > + * populate the block header > > + * \param[in] blockSize The ISP block size, used to populate the block header > > + * > > + * > > + * Initialize the block header with \a blockType and \a blockSize and > > + * returns a reference to the memory used to store an ISP configuration block. > > + * > > + * IPA modules that derive the V4L2Params class shall use this function to > > + * retrieve the memory area that will be used to construct a V4L2ParamsBlock<T> > > + * before returning it to the caller. > > + */ > > + > > +/** > > + * \var V4L2Params::data_ > > + * \brief The ISP parameters buffer memory > > + */ > > + > > +/** > > + * \var V4L2Params::used_ > > + * \brief The number of bytes used in the parameters buffer > > + */ > > + > > +/** > > + * \var V4L2Params::blocks_ > > + * \brief Cache of ISP configuration blocks > > + */ > > + > > +} /* namespace ipa */ > > + > > +} /* namespace libcamera */ > > diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h > > new file mode 100644 > > index 0000000000000000000000000000000000000000..5586096c7ee8a2d20877838564e8074e0fc3d1ce > > --- /dev/null > > +++ b/src/ipa/libipa/v4l2_params.h > > @@ -0,0 +1,135 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2025, Ideas On Board > > + * > > + * V4L2 Parameters > > + */ > > + > > +#pragma once > > + > > +#include <map> > > +#include <stdint.h> > > +#include <string.h> > > + > > +#include <linux/media/v4l2-extensible-params.h> > > + > > +#include <libcamera/base/class.h> > > +#include <libcamera/base/span.h> > > + > > +namespace libcamera { > > + > > +namespace ipa { > > + > > +template<typename T> > > +class V4L2ParamsBlock > > +{ > > +public: > > + V4L2ParamsBlock(const Span<uint8_t> &data) > > I think taking by value is fine, no need for the const ref. > right indeed > > > + { > > + header_ = data.subspan(0, sizeof(v4l2_params_block_header)); > > + data_ = data.subspan(sizeof(v4l2_params_block_header)); > > I'd probably do these in the member init list. > is this just a style preference or are there reasons I'm missing ? > > > + } > > + > > + void setEnabled(bool enabled) > > + { > > + struct v4l2_params_block_header *header = > > + reinterpret_cast<struct v4l2_params_block_header *>(header_.data()); > > + > > + header->flags &= ~(V4L2_PARAMS_FL_BLOCK_ENABLE | > > + V4L2_PARAMS_FL_BLOCK_DISABLE); > > + header->flags |= enabled ? V4L2_PARAMS_FL_BLOCK_ENABLE > > + : V4L2_PARAMS_FL_BLOCK_DISABLE; > > + } > > + > > + Span<uint8_t> header() const { return header_; } > > + Span<uint8_t> data() const { return data_; } > > + > > + const T *operator->() const > > + { > > + return reinterpret_cast<const T *>(data().data()); > > + } > > + > > + T *operator->() > > + { > > + return reinterpret_cast<T *>(data().data()); > > + } > > + > > + const T &operator*() const > > + { > > + return *reinterpret_cast<const T *>(data().data()); > > + } > > + > > + T &operator*() > > + { > > + return *reinterpret_cast<T *>(data().data()); > > + } > > + > > +private: > > + LIBCAMERA_DISABLE_COPY(V4L2ParamsBlock) > > Why disable? I would assume that it's OK to copy this? > tbh I think I just copied this from RkISP1Params. However, why would one copy an instance of V4L2ParamsBlock ? > > > + > > + Span<uint8_t> header_; > > + Span<uint8_t> data_; > > +}; > > + > > +template<typename T> > > +class V4L2Params > > +{ > > +public: > > + V4L2Params(Span<uint8_t> data, unsigned int version) > > + : data_(data) > > + { > > + struct v4l2_params_buffer *cfg = > > + reinterpret_cast<struct v4l2_params_buffer *>(data_.data()); > > + cfg->data_size = 0; > > + cfg->version = version; > > + used_ = offsetof(struct v4l2_params_buffer, data); > > + } > > + > > + size_t size() const { return used_; } > > + > > +protected: > > + Span<uint8_t> block(T type, unsigned int blockType, size_t blockSize) > > This can modified so that the derived classes need not implement any extra logic. > One option is to pass the `details::block_type` template as a template argument > to V4L2Params and then use that to map the id to the type: > > template<typename T, template<T> typename IdToType> > class V4L2Params { > > template<T ID> > auto block() { > using Type = typename IdToType<ID>::type; > ... > } > > but I think it's simpler and more extensible to pass another type that holds > all this compile-time information. It could look something like this: > > diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h > index 5586096c7..019f06509 100644 > --- a/src/ipa/libipa/v4l2_params.h > +++ b/src/ipa/libipa/v4l2_params.h > @@ -71,7 +71,7 @@ private: > Span<uint8_t> data_; > }; > -template<typename T> > +template<typename Traits> > class V4L2Params > { > public: > @@ -87,8 +87,21 @@ public: > size_t size() const { return used_; } > + template<typename Traits::id_type Id> > + auto block() > + { > + using Details = typename Traits::template id_to_details<Id>; I can't grok 'typename Traits::template' but it's ok, I'll trust you here > + > + using Type = typename Details::type; > + constexpr auto kernelId = Details::blockType; > + > + auto data = block(Id, kernelId, sizeof(Type)); > + > + return V4L2ParamsBlock<Type>(data); > + } > + > protected: > - Span<uint8_t> block(T type, unsigned int blockType, size_t blockSize) > + Span<uint8_t> block(typename Traits::id_type type, unsigned int blockType, size_t blockSize) > { > /* > * Look up the block in the cache first. If an algorithm > @@ -127,7 +140,7 @@ protected: > Span<uint8_t> data_; > size_t used_; > - std::map<T, Span<uint8_t>> blocks_; > + std::map<typename Traits::id_type, Span<uint8_t>> blocks_; > }; > } /* namespace ipa */ > diff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h > index 1cc56dbad..975122999 100644 > --- a/src/ipa/mali-c55/params.h > +++ b/src/ipa/mali-c55/params.h > @@ -58,30 +58,24 @@ MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingConfig, mesh_shading_config, > MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingSel, mesh_shading_selection, > MESH_SHADING_SELECTION) > +struct params_traits { > + using id_type = MaliC55Blocks; > + > + template<id_type Id> > + using id_to_details = block_type<Id>; > +}; > + > } /* namespace details */ > -class MaliC55Params : public V4L2Params<MaliC55Blocks> > +class MaliC55Params : public V4L2Params<details::params_traits> > { > public: > static constexpr unsigned int kVersion = MALI_C55_PARAM_BUFFER_V1; > MaliC55Params(Span<uint8_t> data) > - : V4L2Params<MaliC55Blocks>(data, kVersion) > + : V4L2Params(data, kVersion) > { > } > - > - template<MaliC55Blocks B> > - auto block() > - { > - using Type = typename details::block_type<B>::type; > - > - auto blockType = details::block_type<B>::blockType; > - size_t blockSize = sizeof(Type); > - > - auto data = V4L2Params::block(B, blockType, blockSize); > - > - return V4L2ParamsBlock<Type>(data); > - } Thanks! 'struct param_traits' and the associated changes in V4L2Params allowed me to simplify the MaliC55 implementation as you suggested > }; > } /* namespace ipa::mali_c55 */ > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h > index 7a2127664..824a5924f 100644 > --- a/src/ipa/rkisp1/params.h > +++ b/src/ipa/rkisp1/params.h > @@ -45,45 +45,54 @@ template<BlockType B> > struct block_type { > }; > -#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct) \ > +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct, id) \ > template<> \ > struct block_type<BlockType::blockType> { \ > using type = struct rkisp1_cif_isp_##blockStruct##_config; \ > + static constexpr rkisp1_ext_params_block_type blockType = \ > + RKISP1_EXT_PARAMS_BLOCK_TYPE_##id; \ > }; > -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) > -RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls) > -RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve) > -RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve) > +RKISP1_DEFINE_BLOCK_TYPE(Bls, bls, BLS) > +RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc, DPCC) > +RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg, SDG) > +RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain, AWB_GAIN) > +RKISP1_DEFINE_BLOCK_TYPE(Flt, flt, FLT) > +RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm, BDM) > +RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk, CTK) > +RKISP1_DEFINE_BLOCK_TYPE(Goc, goc, GOC) > +RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf, DPF) > +RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength, DPF_STRENGTH) > +RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc, CPROC) > +RKISP1_DEFINE_BLOCK_TYPE(Ie, ie, IE) > +RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc, LSC) > +RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas, AWB_MEAS) > +RKISP1_DEFINE_BLOCK_TYPE(Hst, hst, HST_MEAS) > +RKISP1_DEFINE_BLOCK_TYPE(Aec, aec, AEC_MEAS) > +RKISP1_DEFINE_BLOCK_TYPE(Afc, afc, AFC_MEAS) > +RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls, COMPAND_BLS) > +RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve, COMPAND_EXPAND) > +RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve, COMPAND_COMPRESS) > + > +struct params_traits { > + using id_type = BlockType; > + > + template<id_type Id> > + using id_to_details = block_type<Id>; > +}; > } /* namespace details */ > template<typename T> > class RkISP1ParamsBlock; > -class RkISP1Params : public V4L2Params<BlockType> > +class RkISP1Params : public V4L2Params<details::params_traits> > { > public: > static constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1; > RkISP1Params(uint32_t format, Span<uint8_t> data) > - : V4L2Params<BlockType>(data, kVersion), format_(format) > + : V4L2Params(data, kVersion), format_(format) > { > if (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) { > memset(data.data(), 0, data.size()); > > > Admittedly I haven't tested it, and the rkisp1 parts are not there yet. > But I believe this direction could simplify the code and make it easier > to implement for other parameter sets. Please note that for RkISP1 I had to keep an override for RkISP1Params::block() as we need to support both legacy and extensible > > For rkisp1, I think as a first step `kBlockTypeInfo` should be moved completely > into `rkisp1::details::block_type` template specializations to do the mapping > at compile time, similarly to how the proposed `MaliC55Params` does it. yeah, I don't like that map too much. But, if I may use it as an excuse, it was there already so we're not doing any worse. I could have removed it like it's done for Mali, but the 'enableBit' used by setEnabled() to support the legacy format made it more complicated getting rid of it (I've not invested much time in trying, admittedly). Anyway, could easily be done on top ? > > Then I believe one could realistically extend this `V4L2Params` to even support > fixed-offset formats generically, if needed, but that might be an overkill. Not sure I got what you mean here, > > > > + { > > + /* > > + * 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. */ > > + if (blockSize > data_.size() - used_) > > + return {}; > > + > > + /* Allocate a new block, clear its memory, and initialize its header. */ > > + Span<uint8_t> block = data_.subspan(used_, blockSize); > > + used_ += blockSize; > > + > > + struct v4l2_params_buffer *cfg = > > + reinterpret_cast<struct v4l2_params_buffer *>(data_.data()); > > + cfg->data_size += blockSize; > > + > > + memset(block.data(), 0, block.size()); > > + > > + struct v4l2_params_block_header *header = > > + reinterpret_cast<struct v4l2_params_block_header *>(block.data()); > > + header->type = blockType; > > + header->size = block.size(); > > + > > + /* Update the cache. */ > > + blocks_[type] = block; > > + > > + return block; > > + } > > + > > + Span<uint8_t> data_; > > + size_t used_; > > + > > + std::map<T, Span<uint8_t>> blocks_; > > +}; > > + > > +} /* namespace ipa */ > > + > > +} /* namespace libcamera */ > > diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp > > index 4c0b051ce65da1686323ee9c66b82e12669a754d..2b692e1a1f199d6c118af0938e1aaecef6186a2c 100644 > > --- a/src/ipa/rkisp1/params.cpp > > +++ b/src/ipa/rkisp1/params.cpp > > @@ -35,7 +35,7 @@ struct BlockTypeInfo { > > #define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit) \ > > { BlockType::block, { \ > > RKISP1_EXT_PARAMS_BLOCK_TYPE_##id, \ > > - sizeof(struct rkisp1_cif_isp_##type##_config), \ > > + sizeof(struct rkisp1_ext_params_##type##_config), \ > > offsetof(struct rkisp1_params_cfg, category.type##_config), \ > > RKISP1_CIF_ISP_MODULE_##bit, \ > > } } > > @@ -49,7 +49,7 @@ struct BlockTypeInfo { > > #define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type) \ > > { BlockType::block, { \ > > RKISP1_EXT_PARAMS_BLOCK_TYPE_##id, \ > > - sizeof(struct rkisp1_cif_isp_##type##_config), \ > > + sizeof(struct rkisp1_ext_params_##type##_config), \ > > 0, 0, \ > > } } > > @@ -78,56 +78,6 @@ const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = { > > } /* namespace */ > > -RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type, > > - const Span<uint8_t> &data) > > - : params_(params), type_(type) > > -{ > > - if (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) { > > - header_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header)); > > - data_ = data.subspan(sizeof(rkisp1_ext_params_block_header)); > > - } else { > > - data_ = data; > > - } > > -} > > - > > -void RkISP1ParamsBlockBase::setEnabled(bool enabled) > > -{ > > - /* > > - * For the legacy fixed format, blocks are enabled in the top-level > > - * header. Delegate to the RkISP1Params class. > > - */ > > - if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS) > > - return params_->setBlockEnabled(type_, enabled); > > - > > - /* > > - * For the extensible format, set the enable and disable flags in the > > - * block header directly. > > - */ > > - struct rkisp1_ext_params_block_header *header = > > - reinterpret_cast<struct rkisp1_ext_params_block_header *>(header_.data()); > > - header->flags &= ~(RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE | > > - RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE); > > - header->flags |= enabled ? RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE > > - : RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE; > > -} > > - > > -RkISP1Params::RkISP1Params(uint32_t format, Span<uint8_t> data) > > - : format_(format), data_(data), used_(0) > > -{ > > - if (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) { > > - struct rkisp1_ext_params_cfg *cfg = > > - reinterpret_cast<struct rkisp1_ext_params_cfg *>(data.data()); > > - > > - cfg->version = RKISP1_EXT_PARAM_BUFFER_V1; > > - cfg->data_size = 0; > > - > > - used_ += offsetof(struct rkisp1_ext_params_cfg, data); > > - } else { > > - memset(data.data(), 0, data.size()); > > - used_ = sizeof(struct rkisp1_params_cfg); > > - } > > -} > > - > > void RkISP1Params::setBlockEnabled(BlockType type, bool enabled) > > { > > const BlockTypeInfo &info = kBlockTypeInfo.at(type); > > @@ -177,44 +127,7 @@ Span<uint8_t> RkISP1Params::block(BlockType type) > > 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; > > + return V4L2Params::block(type, info.type, info.size); > > } > > } /* namespace ipa::rkisp1 */ > > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h > > index 40450e34497a3aa71b5b0cda2bf045a1cc0e012f..7a21276648162a127317c75cb01d1c0059b96ee1 100644 > > --- a/src/ipa/rkisp1/params.h > > +++ b/src/ipa/rkisp1/params.h > > @@ -7,13 +7,10 @@ > > #pragma once > > -#include <map> > > -#include <stdint.h> > > - > > #include <linux/rkisp1-config.h> > > +#include <linux/videodev2.h> > > -#include <libcamera/base/class.h> > > -#include <libcamera/base/span.h> > > +#include <libipa/v4l2_params.h> > > namespace libcamera { > > @@ -77,85 +74,72 @@ RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve) > > } /* namespace details */ > > -class RkISP1Params; > > +template<typename T> > > +class RkISP1ParamsBlock; > > -class RkISP1ParamsBlockBase > > +class RkISP1Params : public V4L2Params<BlockType> > > { > > public: > > - RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type, > > - const Span<uint8_t> &data); > > - > > - Span<uint8_t> data() const { return data_; } > > - > > - void setEnabled(bool enabled); > > + static constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1; > > -private: > > - LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase) > > - > > - RkISP1Params *params_; > > - BlockType type_; > > - Span<uint8_t> header_; > > - Span<uint8_t> data_; > > -}; > > - > > -template<BlockType B> > > -class RkISP1ParamsBlock : public RkISP1ParamsBlockBase > > -{ > > -public: > > - using Type = typename details::block_type<B>::type; > > - > > - RkISP1ParamsBlock(RkISP1Params *params, const Span<uint8_t> &data) > > - : RkISP1ParamsBlockBase(params, B, data) > > + RkISP1Params(uint32_t format, Span<uint8_t> data) > > + : V4L2Params<BlockType>(data, kVersion), format_(format) > > { > > + if (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) { > > + memset(data.data(), 0, data.size()); > > + used_ = sizeof(struct rkisp1_params_cfg); > > + } > > } > > - const Type *operator->() const > > + template<BlockType B> > > + auto block() > > { > > - return reinterpret_cast<const Type *>(data().data()); > > - } > > + using Type = typename details::block_type<B>::type; > > - Type *operator->() > > - { > > - return reinterpret_cast<Type *>(data().data()); > > + return RkISP1ParamsBlock<Type>(this, B, block(B)); > > } > > - const Type &operator*() const & > > - { > > - return *reinterpret_cast<const Type *>(data().data()); > > - } > > + uint32_t format() const { return format_; } > > + void setBlockEnabled(BlockType type, bool enabled); > > - Type &operator*() & > > - { > > - return *reinterpret_cast<Type *>(data().data()); > > - } > > +private: > > + Span<uint8_t> block(BlockType type); > > + > > + uint32_t format_; > > }; > > -class RkISP1Params > > +template<typename T> > > +class RkISP1ParamsBlock : public V4L2ParamsBlock<T> > > { > > public: > > - RkISP1Params(uint32_t format, Span<uint8_t> data); > > - > > - template<BlockType B> > > - RkISP1ParamsBlock<B> block() > > + RkISP1ParamsBlock(RkISP1Params *params, BlockType type, > > + const Span<uint8_t> &data) > > + : V4L2ParamsBlock<T>(data) > > { > > - return RkISP1ParamsBlock<B>(this, block(B)); > > + params_ = params; > > + type_ = type; > > + > > + /* Legacy param format has no header */ > > + if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS) > > + data_ = data; > > } > > - uint32_t format() const { return format_; } > > - size_t size() const { return used_; } > > + void 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); > > + > > + return V4L2ParamsBlock<T>::setEnabled(enabled); > > + } > > I have one concern regarding this design. An `RkISP1ParamsBlock` instance > will not work as its base type correctly in all cases. That is, calling the > base class `setEnabled()`: > > RkISP1ParamsBlock<...> *p = ...; > > p->V4L2ParamsBlock<...>::setEnabled(...); > // or > static_cast<V4L2ParamsBlock<...> *>(p)->setEnabled(...); > > will not work correctly if the format is the "non-extensible" one. The only user of this class is the RkISP1 IPA, why would it forcefully use the base class implementation ? If there's an easy way to avoid the above, I'll anyway happily consider it. Thanks j > > > Regards, > Barnabás Pőcze > > > > private: > > - friend class RkISP1ParamsBlockBase; > > - > > - Span<uint8_t> block(BlockType type); > > - void setBlockEnabled(BlockType type, bool enabled); > > - > > - uint32_t format_; > > - > > + RkISP1Params *params_; > > + BlockType type_; > > Span<uint8_t> data_; > > - size_t used_; > > - > > - std::map<BlockType, Span<uint8_t>> blocks_; > > }; > > } /* namespace ipa::rkisp1 */ > > >
Hi 2025. 09. 16. 9:50 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > thanks for review > > On Mon, Sep 08, 2025 at 06:09:08PM +0200, Barnabás Pőcze wrote: >> Hi >> >> 2025. 08. 29. 13:54 keltezéssel, Jacopo Mondi írta: >>> The existing RkISP1Params helper classes allows the RkISP1 to handle >>> V4L2 extensible parameters format and the legacy RkIPS1-specific >>> fixed-size parameters format. >>> >>> With the introduction of v4l2-params in the Linux kernel the part of >>> the RkISP1Params helper class that handles extensible parameters can >>> be generalized so that other IPA modules can use the same helpers >>> to populate a v4l2-params compatible buffer. >>> >>> Generalize the RkISP1Params class to a new libipa component named >>> V4L2Params and derive the existing RkISP1Params from it, leaving >>> in the RkISP1-specific implementation the handling of the legacy format. >>> >>> Deriving RkISP1Params from V4L2Params requires changing the size >>> associated to each block to include the size of v4l2_params_block_header >>> in the ipa:rkisp1::kBlockTypeInfo map as the V4L2Params::block() >>> implementation doesn't account for that as RkIS1Params::block() >>> implementation did. >>> >>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>> --- >>> src/ipa/libipa/meson.build | 2 + >>> src/ipa/libipa/v4l2_params.cpp | 252 +++++++++++++++++++++++++++++++++++++++++ >>> src/ipa/libipa/v4l2_params.h | 135 ++++++++++++++++++++++ >>> src/ipa/rkisp1/params.cpp | 93 +-------------- >>> src/ipa/rkisp1/params.h | 108 ++++++++---------- >>> 5 files changed, 438 insertions(+), 152 deletions(-) >>> >>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build >>> index 660be94054fa98b714b6bc586039081e45a6b4bc..4010739e710eb38aa6108eb8258c574a616bf3c0 100644 >>> --- a/src/ipa/libipa/meson.build >>> +++ b/src/ipa/libipa/meson.build >>> @@ -16,6 +16,7 @@ libipa_headers = files([ >>> 'lsc_polynomial.h', >>> 'lux.h', >>> 'module.h', >>> + 'v4l2_params.h', >>> 'pwl.h', >>> ]) >>> @@ -35,6 +36,7 @@ libipa_sources = files([ >>> 'lsc_polynomial.cpp', >>> 'lux.cpp', >>> 'module.cpp', >>> + 'v4l2_params.cpp', >>> 'pwl.cpp', >>> ]) >>> diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp >>> new file mode 100644 >>> index 0000000000000000000000000000000000000000..674018065ace0e1b6b48b1630e556cef590d1e84 >>> --- /dev/null >>> +++ b/src/ipa/libipa/v4l2_params.cpp >>> @@ -0,0 +1,252 @@ >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >>> +/* >>> + * Copyright (C) 2025, Ideas On Board >>> + * >>> + * V4L2 Parameters >>> + */ >>> + >>> +#include "v4l2_params.h" >>> + >>> +namespace libcamera { >>> + >>> +namespace ipa { >>> + >>> +/** >>> + * \file v4l2_params.cpp >>> + * \brief Helper class to populate a v4l2-params compatible parameters buffer >>> + * >>> + * The Linux kernel defines a generic buffer format for configuring ISP devices >>> + * through a set of parameters in the form of V4L2 extensible parameters. The >>> + * V4L2 extensible parameters define a serialization format for ISP parameters >>> + * that allows userspace to populate a buffer of configuration data by appending >>> + * them one after the other in a binary buffer. >>> + * >>> + * Each ISP driver compatible with the v4l2-params format will define its own >>> + * meta-output format identifier and defines the types of the configuration data >>> + * of each ISP block that usually match the registers layout. >>> + * >>> + * The V4L2Params class represent the V4L2 extensible parameters buffer and >>> + * allows users to populate the ISP configuration blocks, represented by the >>> + * V4L2ParamBlock class instances. >>> + * >>> + * IPA implementations using this helpers should define an enumeration of ISP >>> + * blocks the IPA module supports and use a set of common abstraction to help >>> + * their derived implementation of V4L2Params translate the enumerated ISP block >>> + * identifier to the actual type of the configuration data as defined by the >>> + * kernel interface. >>> + * >>> + * As an example of this see the RkISP1 and Mali-C55 implementations. >>> + */ >>> + >>> +/** >>> + * \class V4L2ParamsBlock >>> + * \brief Helper class that represents a ISP configuration block >>> + * >>> + * Each ISP function is associated with a set of configuration parameters >>> + * defined by the kernel interface. >>> + * >>> + * This class represents an ISP block configuration entry. It is constructed >>> + * with a reference to the memory area where the block configuration will be >>> + * stored in the parameters buffer. The template parameter represents >>> + * the underlying kernel-defined ISP block configuration type and allow its >>> + * user to easily cast it to said type to populate and read the configuration >>> + * parameters. >>> + * >>> + * \sa V4L2Params::block() >>> + */ >>> + >>> +/** >>> + * \fn V4L2ParamsBlock::V4L2ParamsBlock() >>> + * \brief Construct a V4L2ParamsBlock with memory represented by \a data >>> + * \param[in] data A view on the memory area where the ISP block is located >>> + */ >>> + >>> +/** >>> + * \fn V4L2ParamsBlock::setEnabled() >>> + * \brief Enable/disable an ISP configuration block >>> + * \param[in] enabled The enable flag >>> + */ >>> + >>> +/** >>> + * \fn V4L2ParamsBlock::header() >>> + * \brief Retrieve a reference to the header (struct v4l2_params_block_header) >>> + * \return The block header >>> + */ >>> + >>> +/** >>> + * \fn V4L2ParamsBlock::data() >>> + * \brief Retrieve a reference to block configuration data memory area >>> + * \return The block data >>> + */ >>> + >>> +/** >>> + * \fn V4L2ParamsBlock::operator->() >>> + * \brief Access the ISP configuration block casting it to the kernel-defined >>> + * ISP configuration type >>> + * >>> + * The V4L2ParamsBlock is templated with the kernel defined ISP configuration >>> + * block type. This function allows users to easily cast a V4L2ParamsBlock to >>> + * the underlying kernel-defined type in order to easily populate or read >>> + * the ISP configuration data. >>> + * >>> + * \code{.cpp} >>> + * >>> + * // The kernel header defines the ISP configuration types, in example >>> + * // struct my_isp_awb_config_data { >>> + * // u16 gain_ch00; >>> + * // u16 gain_ch01; >>> + * // u16 gain_ch10; >>> + * // u16 gain_ch11; >>> + * // >>> + * // } >>> + * >>> + * template<> V4L2ParamsBlock<struct my_isp_awb_config_data> awbBlock >>> + * >>> + * awbBlock->gain_ch00 = ...; >>> + * awbBlock->gain_ch01 = ...; >>> + * awbBlock->gain_ch10 = ...; >>> + * awbBlock->gain_ch11 = ...; >>> + * >>> + * \endcode >>> + * >>> + * Users of this class are not expected to create a V4L2ParamsBlock manually but >>> + * should rather use V4L2Params::block() to retrieve a reference to the memory >>> + * area used to construct a V4L2ParamsBlock<T> in their overloaded >>> + * implementation of V4L2Params::block(). >>> + */ >>> + >>> +/** >>> + * \fn V4L2ParamsBlock::operator->() const >>> + * \copydoc V4L2ParamsBlock::operator->() >>> + */ >>> + >>> +/** >>> + * \fn V4L2ParamsBlock::operator*() const >>> + * \copydoc V4L2ParamsBlock::operator->() >>> + */ >>> + >>> +/** >>> + * \fn V4L2ParamsBlock::operator*() >>> + * \copydoc V4L2ParamsBlock::operator->() >>> + */ >>> + >>> + /** >>> + * \class V4L2Params >>> + * \brief Helper class that represent an ISP configuration buffer >>> + * >>> + * ISP implementation compatible with v4l2-params define their ISP configuration >>> + * buffer types compatible with the struct v4l2_params_buffer type. >>> + * >>> + * This class represents an ISP configuration buffer. It is constructed >>> + * with a reference to the memory mapped buffer that will be queued to the ISP. >>> + * >>> + * This class is templated with the type of the enumeration of ISP blocks that >>> + * each IPA module is expected to support. IPA modules are expected to derive >>> + * this class and use the V4L2Params::block() function to retrieve the memory >>> + * area for each ISP configuration block and use it to construct a >>> + * V4L2ParamsBlock<T> with it before returning it to the user. >>> + * >>> + * \code{.cpp} >>> + * >>> + * enum class myISPBlocks { >>> + * Agc, >>> + * Awb, >>> + * ... >>> + * }; >>> + * >>> + * template<myISPBlocks B> >>> + * struct block_type { >>> + * }; >>> + * >>> + * template<> >>> + * struct block_type<myISPBlock::Agc> { >>> + * using type = struct my_isp_kernel_config_type_agc; >>> + * }; >>> + * >>> + * template<> >>> + * struct block_type<myISPBlock::Awb> { >>> + * using type = struct my_isp_kernel_config_type_awb; >>> + * }; >>> + * >>> + * ... >>> + * >>> + * class MyISPParams : public V4L2Params<myISPBlocks> >>> + * { >>> + * public: >>> + * template<myISPBlocks B> >>> + * auto block() >>> + * { >>> + * >>> + * // Use the kernel defined configuration type as template >>> + * // argument to V4L2ParamsBlock. >>> + * using Type = typename details::block_type<B>::type; >>> + * >>> + * // Each IPA module should provide the information required >>> + * // to populate the block header >>> + * >>> + * ... >>> + * >>> + * auto data = V4L2Params::block(B, blockType, blockSize); >>> + * >>> + * return V4L2ParamsBlock<Type>(data); >>> + * } >>> + * }; >>> + * >>> + * \endcode >>> + * >>> + * As an example, see the RkISP1Params and MaliC55Params implementations. >>> + */ >>> + >>> +/** >>> + * \fn V4L2Params::V4L2Params() >>> + * \brief Construct a V4L2Params >>> + * \param[in] data Reference to the v4l2-buffer memory mapped area >>> + * \param[in] version The ISP parameters version the implementation supports >>> + */ >>> + >>> +/** >>> + * \fn V4L2Params::size() >>> + * \brief Retrieve the used size of the parameters buffer (in bytes) >>> + * >>> + * The parameters buffer size is mostly used to populate the v4l2_buffer >>> + * bytesused field before queueing the buffer to the ISP. >>> + * >>> + * \return The number of bytes occupied by the ISP configuration parameters >>> + */ >>> + >>> +/** >>> + * \fn V4L2Params::block() >>> + * \brief Populate an ISP configuration block a returns a reference to its >>> + * memory >>> + * \param[in] type The ISP block identifier enumerated by the IPA module >>> + * \param[in] blockType The kernel-defined ISP block identifier, used to >>> + * populate the block header >>> + * \param[in] blockSize The ISP block size, used to populate the block header >>> + * >>> + * >>> + * Initialize the block header with \a blockType and \a blockSize and >>> + * returns a reference to the memory used to store an ISP configuration block. >>> + * >>> + * IPA modules that derive the V4L2Params class shall use this function to >>> + * retrieve the memory area that will be used to construct a V4L2ParamsBlock<T> >>> + * before returning it to the caller. >>> + */ >>> + >>> +/** >>> + * \var V4L2Params::data_ >>> + * \brief The ISP parameters buffer memory >>> + */ >>> + >>> +/** >>> + * \var V4L2Params::used_ >>> + * \brief The number of bytes used in the parameters buffer >>> + */ >>> + >>> +/** >>> + * \var V4L2Params::blocks_ >>> + * \brief Cache of ISP configuration blocks >>> + */ >>> + >>> +} /* namespace ipa */ >>> + >>> +} /* namespace libcamera */ >>> diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h >>> new file mode 100644 >>> index 0000000000000000000000000000000000000000..5586096c7ee8a2d20877838564e8074e0fc3d1ce >>> --- /dev/null >>> +++ b/src/ipa/libipa/v4l2_params.h >>> @@ -0,0 +1,135 @@ >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >>> +/* >>> + * Copyright (C) 2025, Ideas On Board >>> + * >>> + * V4L2 Parameters >>> + */ >>> + >>> +#pragma once >>> + >>> +#include <map> >>> +#include <stdint.h> >>> +#include <string.h> >>> + >>> +#include <linux/media/v4l2-extensible-params.h> >>> + >>> +#include <libcamera/base/class.h> >>> +#include <libcamera/base/span.h> >>> + >>> +namespace libcamera { >>> + >>> +namespace ipa { >>> + >>> +template<typename T> >>> +class V4L2ParamsBlock >>> +{ >>> +public: >>> + V4L2ParamsBlock(const Span<uint8_t> &data) >> >> I think taking by value is fine, no need for the const ref. >> > > right indeed > >> >>> + { >>> + header_ = data.subspan(0, sizeof(v4l2_params_block_header)); >>> + data_ = data.subspan(sizeof(v4l2_params_block_header)); >> >> I'd probably do these in the member init list. >> > > is this just a style preference or are there reasons I'm missing ? There is a difference: one is an assignment, one is a constructor call; so I always try to use the member init list. Although the difference does not really matter much here, so you could say it's a preference thing. > >> >>> + } >>> + >>> + void setEnabled(bool enabled) >>> + { >>> + struct v4l2_params_block_header *header = >>> + reinterpret_cast<struct v4l2_params_block_header *>(header_.data()); >>> + >>> + header->flags &= ~(V4L2_PARAMS_FL_BLOCK_ENABLE | >>> + V4L2_PARAMS_FL_BLOCK_DISABLE); >>> + header->flags |= enabled ? V4L2_PARAMS_FL_BLOCK_ENABLE >>> + : V4L2_PARAMS_FL_BLOCK_DISABLE; >>> + } >>> + >>> + Span<uint8_t> header() const { return header_; } >>> + Span<uint8_t> data() const { return data_; } >>> + >>> + const T *operator->() const >>> + { >>> + return reinterpret_cast<const T *>(data().data()); >>> + } >>> + >>> + T *operator->() >>> + { >>> + return reinterpret_cast<T *>(data().data()); >>> + } >>> + >>> + const T &operator*() const >>> + { >>> + return *reinterpret_cast<const T *>(data().data()); >>> + } >>> + >>> + T &operator*() >>> + { >>> + return *reinterpret_cast<T *>(data().data()); >>> + } >>> + >>> +private: >>> + LIBCAMERA_DISABLE_COPY(V4L2ParamsBlock) >> >> Why disable? I would assume that it's OK to copy this? >> > > tbh I think I just copied this from RkISP1Params. > > However, why would one copy an instance of V4L2ParamsBlock ? My thinking was that it is a "reference-like type", so there is no reason to prevent copying since it works and might be useful in some cases (e.g. passing to functions, etc.). > >> >>> + >>> + Span<uint8_t> header_; >>> + Span<uint8_t> data_; >>> +}; >>> + >>> +template<typename T> >>> +class V4L2Params >>> +{ >>> +public: >>> + V4L2Params(Span<uint8_t> data, unsigned int version) >>> + : data_(data) >>> + { >>> + struct v4l2_params_buffer *cfg = >>> + reinterpret_cast<struct v4l2_params_buffer *>(data_.data()); >>> + cfg->data_size = 0; >>> + cfg->version = version; >>> + used_ = offsetof(struct v4l2_params_buffer, data); >>> + } >>> + >>> + size_t size() const { return used_; } >>> + >>> +protected: >>> + Span<uint8_t> block(T type, unsigned int blockType, size_t blockSize) >> >> This can modified so that the derived classes need not implement any extra logic. >> One option is to pass the `details::block_type` template as a template argument >> to V4L2Params and then use that to map the id to the type: >> >> template<typename T, template<T> typename IdToType> >> class V4L2Params { >> >> template<T ID> >> auto block() { >> using Type = typename IdToType<ID>::type; >> ... >> } >> >> but I think it's simpler and more extensible to pass another type that holds >> all this compile-time information. It could look something like this: >> >> diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h >> index 5586096c7..019f06509 100644 >> --- a/src/ipa/libipa/v4l2_params.h >> +++ b/src/ipa/libipa/v4l2_params.h >> @@ -71,7 +71,7 @@ private: >> Span<uint8_t> data_; >> }; >> -template<typename T> >> +template<typename Traits> >> class V4L2Params >> { >> public: >> @@ -87,8 +87,21 @@ public: >> size_t size() const { return used_; } >> + template<typename Traits::id_type Id> >> + auto block() >> + { >> + using Details = typename Traits::template id_to_details<Id>; > > I can't grok 'typename Traits::template' > > but it's ok, I'll trust you here See https://www.en.cppreference.com/w/cpp/language/dependent_name.html under "The template disambiguator for dependent names". It just tells the compiler that `id_to_details` is a template. This is quite similar to having to use the `typename` keyword in some cases. (see "The typename disambiguator for dependent names" on the linked site) > >> + >> + using Type = typename Details::type; >> + constexpr auto kernelId = Details::blockType; >> + >> + auto data = block(Id, kernelId, sizeof(Type)); >> + >> + return V4L2ParamsBlock<Type>(data); >> + } >> + >> protected: >> - Span<uint8_t> block(T type, unsigned int blockType, size_t blockSize) >> + Span<uint8_t> block(typename Traits::id_type type, unsigned int blockType, size_t blockSize) >> { >> /* >> * Look up the block in the cache first. If an algorithm >> @@ -127,7 +140,7 @@ protected: >> Span<uint8_t> data_; >> size_t used_; >> - std::map<T, Span<uint8_t>> blocks_; >> + std::map<typename Traits::id_type, Span<uint8_t>> blocks_; >> }; >> } /* namespace ipa */ >> diff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h >> index 1cc56dbad..975122999 100644 >> --- a/src/ipa/mali-c55/params.h >> +++ b/src/ipa/mali-c55/params.h >> @@ -58,30 +58,24 @@ MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingConfig, mesh_shading_config, >> MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingSel, mesh_shading_selection, >> MESH_SHADING_SELECTION) >> +struct params_traits { >> + using id_type = MaliC55Blocks; >> + >> + template<id_type Id> >> + using id_to_details = block_type<Id>; >> +}; >> + >> } /* namespace details */ >> -class MaliC55Params : public V4L2Params<MaliC55Blocks> >> +class MaliC55Params : public V4L2Params<details::params_traits> >> { >> public: >> static constexpr unsigned int kVersion = MALI_C55_PARAM_BUFFER_V1; >> MaliC55Params(Span<uint8_t> data) >> - : V4L2Params<MaliC55Blocks>(data, kVersion) >> + : V4L2Params(data, kVersion) >> { >> } >> - >> - template<MaliC55Blocks B> >> - auto block() >> - { >> - using Type = typename details::block_type<B>::type; >> - >> - auto blockType = details::block_type<B>::blockType; >> - size_t blockSize = sizeof(Type); >> - >> - auto data = V4L2Params::block(B, blockType, blockSize); >> - >> - return V4L2ParamsBlock<Type>(data); >> - } > > Thanks! > > 'struct param_traits' and the associated changes in V4L2Params allowed > me to simplify the MaliC55 implementation as you suggested > >> }; >> } /* namespace ipa::mali_c55 */ >> diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h >> index 7a2127664..824a5924f 100644 >> --- a/src/ipa/rkisp1/params.h >> +++ b/src/ipa/rkisp1/params.h >> @@ -45,45 +45,54 @@ template<BlockType B> >> struct block_type { >> }; >> -#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct) \ >> +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct, id) \ >> template<> \ >> struct block_type<BlockType::blockType> { \ >> using type = struct rkisp1_cif_isp_##blockStruct##_config; \ >> + static constexpr rkisp1_ext_params_block_type blockType = \ >> + RKISP1_EXT_PARAMS_BLOCK_TYPE_##id; \ >> }; >> -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) >> -RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls) >> -RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve) >> -RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve) >> +RKISP1_DEFINE_BLOCK_TYPE(Bls, bls, BLS) >> +RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc, DPCC) >> +RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg, SDG) >> +RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain, AWB_GAIN) >> +RKISP1_DEFINE_BLOCK_TYPE(Flt, flt, FLT) >> +RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm, BDM) >> +RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk, CTK) >> +RKISP1_DEFINE_BLOCK_TYPE(Goc, goc, GOC) >> +RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf, DPF) >> +RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength, DPF_STRENGTH) >> +RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc, CPROC) >> +RKISP1_DEFINE_BLOCK_TYPE(Ie, ie, IE) >> +RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc, LSC) >> +RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas, AWB_MEAS) >> +RKISP1_DEFINE_BLOCK_TYPE(Hst, hst, HST_MEAS) >> +RKISP1_DEFINE_BLOCK_TYPE(Aec, aec, AEC_MEAS) >> +RKISP1_DEFINE_BLOCK_TYPE(Afc, afc, AFC_MEAS) >> +RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls, COMPAND_BLS) >> +RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve, COMPAND_EXPAND) >> +RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve, COMPAND_COMPRESS) >> + >> +struct params_traits { >> + using id_type = BlockType; >> + >> + template<id_type Id> >> + using id_to_details = block_type<Id>; >> +}; >> } /* namespace details */ >> template<typename T> >> class RkISP1ParamsBlock; >> -class RkISP1Params : public V4L2Params<BlockType> >> +class RkISP1Params : public V4L2Params<details::params_traits> >> { >> public: >> static constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1; >> RkISP1Params(uint32_t format, Span<uint8_t> data) >> - : V4L2Params<BlockType>(data, kVersion), format_(format) >> + : V4L2Params(data, kVersion), format_(format) >> { >> if (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) { >> memset(data.data(), 0, data.size()); >> >> >> Admittedly I haven't tested it, and the rkisp1 parts are not there yet. >> But I believe this direction could simplify the code and make it easier >> to implement for other parameter sets. > > Please note that for RkISP1 I had to keep an override for > RkISP1Params::block() as we need to support both legacy and extensible > >> >> For rkisp1, I think as a first step `kBlockTypeInfo` should be moved completely >> into `rkisp1::details::block_type` template specializations to do the mapping >> at compile time, similarly to how the proposed `MaliC55Params` does it. > > yeah, I don't like that map too much. But, if I may use it as an > excuse, it was there already so we're not doing any worse. > > I could have removed it like it's done for Mali, but the 'enableBit' > used by setEnabled() to support the legacy format made it more > complicated getting rid of it (I've not invested much time in trying, > admittedly). > > Anyway, could easily be done on top ? Yes. > >> >> Then I believe one could realistically extend this `V4L2Params` to even support >> fixed-offset formats generically, if needed, but that might be an overkill. > > Not sure I got what you mean here, What I meant is that it is probably possible to support "legacy" parameter formats like the rkisp1 one in the base template. But if that's the only one that is relevant for us, then it's very likely an overkill. >> >> >>> + { >>> + /* >>> + * 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. */ >>> + if (blockSize > data_.size() - used_) >>> + return {}; >>> + >>> + /* Allocate a new block, clear its memory, and initialize its header. */ >>> + Span<uint8_t> block = data_.subspan(used_, blockSize); >>> + used_ += blockSize; >>> + >>> + struct v4l2_params_buffer *cfg = >>> + reinterpret_cast<struct v4l2_params_buffer *>(data_.data()); >>> + cfg->data_size += blockSize; >>> + >>> + memset(block.data(), 0, block.size()); >>> + >>> + struct v4l2_params_block_header *header = >>> + reinterpret_cast<struct v4l2_params_block_header *>(block.data()); >>> + header->type = blockType; >>> + header->size = block.size(); >>> + >>> + /* Update the cache. */ >>> + blocks_[type] = block; >>> + >>> + return block; >>> + } >>> + >>> + Span<uint8_t> data_; >>> + size_t used_; >>> + >>> + std::map<T, Span<uint8_t>> blocks_; >>> +}; >>> + >>> +} /* namespace ipa */ >>> + >>> +} /* namespace libcamera */ >>> diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp >>> index 4c0b051ce65da1686323ee9c66b82e12669a754d..2b692e1a1f199d6c118af0938e1aaecef6186a2c 100644 >>> --- a/src/ipa/rkisp1/params.cpp >>> +++ b/src/ipa/rkisp1/params.cpp >>> @@ -35,7 +35,7 @@ struct BlockTypeInfo { >>> #define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit) \ >>> { BlockType::block, { \ >>> RKISP1_EXT_PARAMS_BLOCK_TYPE_##id, \ >>> - sizeof(struct rkisp1_cif_isp_##type##_config), \ >>> + sizeof(struct rkisp1_ext_params_##type##_config), \ >>> offsetof(struct rkisp1_params_cfg, category.type##_config), \ >>> RKISP1_CIF_ISP_MODULE_##bit, \ >>> } } >>> @@ -49,7 +49,7 @@ struct BlockTypeInfo { >>> #define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type) \ >>> { BlockType::block, { \ >>> RKISP1_EXT_PARAMS_BLOCK_TYPE_##id, \ >>> - sizeof(struct rkisp1_cif_isp_##type##_config), \ >>> + sizeof(struct rkisp1_ext_params_##type##_config), \ >>> 0, 0, \ >>> } } >>> @@ -78,56 +78,6 @@ const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = { >>> } /* namespace */ >>> -RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type, >>> - const Span<uint8_t> &data) >>> - : params_(params), type_(type) >>> -{ >>> - if (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) { >>> - header_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header)); >>> - data_ = data.subspan(sizeof(rkisp1_ext_params_block_header)); >>> - } else { >>> - data_ = data; >>> - } >>> -} >>> - >>> -void RkISP1ParamsBlockBase::setEnabled(bool enabled) >>> -{ >>> - /* >>> - * For the legacy fixed format, blocks are enabled in the top-level >>> - * header. Delegate to the RkISP1Params class. >>> - */ >>> - if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS) >>> - return params_->setBlockEnabled(type_, enabled); >>> - >>> - /* >>> - * For the extensible format, set the enable and disable flags in the >>> - * block header directly. >>> - */ >>> - struct rkisp1_ext_params_block_header *header = >>> - reinterpret_cast<struct rkisp1_ext_params_block_header *>(header_.data()); >>> - header->flags &= ~(RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE | >>> - RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE); >>> - header->flags |= enabled ? RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE >>> - : RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE; >>> -} >>> - >>> -RkISP1Params::RkISP1Params(uint32_t format, Span<uint8_t> data) >>> - : format_(format), data_(data), used_(0) >>> -{ >>> - if (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) { >>> - struct rkisp1_ext_params_cfg *cfg = >>> - reinterpret_cast<struct rkisp1_ext_params_cfg *>(data.data()); >>> - >>> - cfg->version = RKISP1_EXT_PARAM_BUFFER_V1; >>> - cfg->data_size = 0; >>> - >>> - used_ += offsetof(struct rkisp1_ext_params_cfg, data); >>> - } else { >>> - memset(data.data(), 0, data.size()); >>> - used_ = sizeof(struct rkisp1_params_cfg); >>> - } >>> -} >>> - >>> void RkISP1Params::setBlockEnabled(BlockType type, bool enabled) >>> { >>> const BlockTypeInfo &info = kBlockTypeInfo.at(type); >>> @@ -177,44 +127,7 @@ Span<uint8_t> RkISP1Params::block(BlockType type) >>> 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; >>> + return V4L2Params::block(type, info.type, info.size); >>> } >>> } /* namespace ipa::rkisp1 */ >>> diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h >>> index 40450e34497a3aa71b5b0cda2bf045a1cc0e012f..7a21276648162a127317c75cb01d1c0059b96ee1 100644 >>> --- a/src/ipa/rkisp1/params.h >>> +++ b/src/ipa/rkisp1/params.h >>> @@ -7,13 +7,10 @@ >>> #pragma once >>> -#include <map> >>> -#include <stdint.h> >>> - >>> #include <linux/rkisp1-config.h> >>> +#include <linux/videodev2.h> >>> -#include <libcamera/base/class.h> >>> -#include <libcamera/base/span.h> >>> +#include <libipa/v4l2_params.h> >>> namespace libcamera { >>> @@ -77,85 +74,72 @@ RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve) >>> } /* namespace details */ >>> -class RkISP1Params; >>> +template<typename T> >>> +class RkISP1ParamsBlock; >>> -class RkISP1ParamsBlockBase >>> +class RkISP1Params : public V4L2Params<BlockType> >>> { >>> public: >>> - RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type, >>> - const Span<uint8_t> &data); >>> - >>> - Span<uint8_t> data() const { return data_; } >>> - >>> - void setEnabled(bool enabled); >>> + static constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1; >>> -private: >>> - LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase) >>> - >>> - RkISP1Params *params_; >>> - BlockType type_; >>> - Span<uint8_t> header_; >>> - Span<uint8_t> data_; >>> -}; >>> - >>> -template<BlockType B> >>> -class RkISP1ParamsBlock : public RkISP1ParamsBlockBase >>> -{ >>> -public: >>> - using Type = typename details::block_type<B>::type; >>> - >>> - RkISP1ParamsBlock(RkISP1Params *params, const Span<uint8_t> &data) >>> - : RkISP1ParamsBlockBase(params, B, data) >>> + RkISP1Params(uint32_t format, Span<uint8_t> data) >>> + : V4L2Params<BlockType>(data, kVersion), format_(format) >>> { >>> + if (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) { >>> + memset(data.data(), 0, data.size()); >>> + used_ = sizeof(struct rkisp1_params_cfg); >>> + } >>> } >>> - const Type *operator->() const >>> + template<BlockType B> >>> + auto block() >>> { >>> - return reinterpret_cast<const Type *>(data().data()); >>> - } >>> + using Type = typename details::block_type<B>::type; >>> - Type *operator->() >>> - { >>> - return reinterpret_cast<Type *>(data().data()); >>> + return RkISP1ParamsBlock<Type>(this, B, block(B)); >>> } >>> - const Type &operator*() const & >>> - { >>> - return *reinterpret_cast<const Type *>(data().data()); >>> - } >>> + uint32_t format() const { return format_; } >>> + void setBlockEnabled(BlockType type, bool enabled); >>> - Type &operator*() & >>> - { >>> - return *reinterpret_cast<Type *>(data().data()); >>> - } >>> +private: >>> + Span<uint8_t> block(BlockType type); >>> + >>> + uint32_t format_; >>> }; >>> -class RkISP1Params >>> +template<typename T> >>> +class RkISP1ParamsBlock : public V4L2ParamsBlock<T> >>> { >>> public: >>> - RkISP1Params(uint32_t format, Span<uint8_t> data); >>> - >>> - template<BlockType B> >>> - RkISP1ParamsBlock<B> block() >>> + RkISP1ParamsBlock(RkISP1Params *params, BlockType type, >>> + const Span<uint8_t> &data) >>> + : V4L2ParamsBlock<T>(data) >>> { >>> - return RkISP1ParamsBlock<B>(this, block(B)); >>> + params_ = params; >>> + type_ = type; >>> + >>> + /* Legacy param format has no header */ >>> + if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS) >>> + data_ = data; >>> } >>> - uint32_t format() const { return format_; } >>> - size_t size() const { return used_; } >>> + void 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); >>> + >>> + return V4L2ParamsBlock<T>::setEnabled(enabled); >>> + } >> >> I have one concern regarding this design. An `RkISP1ParamsBlock` instance >> will not work as its base type correctly in all cases. That is, calling the >> base class `setEnabled()`: >> >> RkISP1ParamsBlock<...> *p = ...; >> >> p->V4L2ParamsBlock<...>::setEnabled(...); >> // or >> static_cast<V4L2ParamsBlock<...> *>(p)->setEnabled(...); >> >> will not work correctly if the format is the "non-extensible" one. > > The only user of this class is the RkISP1 IPA, why would it forcefully > use the base class implementation ? It probably wouldn't, I just wanted to make a note of this. Although I can imagine that with time some parts would want to operate on `V4L2ParamsBlock` objects directly, in a generic fashion, and then this might be an issue. > > If there's an easy way to avoid the above, I'll anyway happily consider > it. Sadly I don't think I have an easy solution here. I'll take a look again at the next version. Regards, Barnabás Pőcze > > Thanks > j > >> >> >> Regards, >> Barnabás Pőcze >> >> >>> private: >>> - friend class RkISP1ParamsBlockBase; >>> - >>> - Span<uint8_t> block(BlockType type); >>> - void setBlockEnabled(BlockType type, bool enabled); >>> - >>> - uint32_t format_; >>> - >>> + RkISP1Params *params_; >>> + BlockType type_; >>> Span<uint8_t> data_; >>> - size_t used_; >>> - >>> - std::map<BlockType, Span<uint8_t>> blocks_; >>> }; >>> } /* namespace ipa::rkisp1 */ >>> >>
Hi Barnabás On Tue, Sep 16, 2025 at 12:48:27PM +0200, Barnabás Pőcze wrote: > Hi > > > 2025. 09. 16. 9:50 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > thanks for review > > > > On Mon, Sep 08, 2025 at 06:09:08PM +0200, Barnabás Pőcze wrote: > > > Hi > > > > > > 2025. 08. 29. 13:54 keltezéssel, Jacopo Mondi írta: > > > > The existing RkISP1Params helper classes allows the RkISP1 to handle > > > > V4L2 extensible parameters format and the legacy RkIPS1-specific > > > > fixed-size parameters format. > > > > > > > > With the introduction of v4l2-params in the Linux kernel the part of > > > > the RkISP1Params helper class that handles extensible parameters can > > > > be generalized so that other IPA modules can use the same helpers > > > > to populate a v4l2-params compatible buffer. > > > > > > > > Generalize the RkISP1Params class to a new libipa component named > > > > V4L2Params and derive the existing RkISP1Params from it, leaving > > > > in the RkISP1-specific implementation the handling of the legacy format. > > > > > > > > Deriving RkISP1Params from V4L2Params requires changing the size > > > > associated to each block to include the size of v4l2_params_block_header > > > > in the ipa:rkisp1::kBlockTypeInfo map as the V4L2Params::block() > > > > implementation doesn't account for that as RkIS1Params::block() > > > > implementation did. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > --- > > > > src/ipa/libipa/meson.build | 2 + > > > > src/ipa/libipa/v4l2_params.cpp | 252 +++++++++++++++++++++++++++++++++++++++++ > > > > src/ipa/libipa/v4l2_params.h | 135 ++++++++++++++++++++++ > > > > src/ipa/rkisp1/params.cpp | 93 +-------------- > > > > src/ipa/rkisp1/params.h | 108 ++++++++---------- > > > > 5 files changed, 438 insertions(+), 152 deletions(-) > > > > > > > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build > > > > index 660be94054fa98b714b6bc586039081e45a6b4bc..4010739e710eb38aa6108eb8258c574a616bf3c0 100644 > > > > --- a/src/ipa/libipa/meson.build > > > > +++ b/src/ipa/libipa/meson.build > > > > @@ -16,6 +16,7 @@ libipa_headers = files([ > > > > 'lsc_polynomial.h', > > > > 'lux.h', > > > > 'module.h', > > > > + 'v4l2_params.h', > > > > 'pwl.h', > > > > ]) > > > > @@ -35,6 +36,7 @@ libipa_sources = files([ > > > > 'lsc_polynomial.cpp', > > > > 'lux.cpp', > > > > 'module.cpp', > > > > + 'v4l2_params.cpp', > > > > 'pwl.cpp', > > > > ]) > > > > diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp > > > > new file mode 100644 > > > > index 0000000000000000000000000000000000000000..674018065ace0e1b6b48b1630e556cef590d1e84 > > > > --- /dev/null > > > > +++ b/src/ipa/libipa/v4l2_params.cpp > > > > @@ -0,0 +1,252 @@ > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > +/* > > > > + * Copyright (C) 2025, Ideas On Board > > > > + * > > > > + * V4L2 Parameters > > > > + */ > > > > + > > > > +#include "v4l2_params.h" > > > > + > > > > +namespace libcamera { > > > > + > > > > +namespace ipa { > > > > + > > > > +/** > > > > + * \file v4l2_params.cpp > > > > + * \brief Helper class to populate a v4l2-params compatible parameters buffer > > > > + * > > > > + * The Linux kernel defines a generic buffer format for configuring ISP devices > > > > + * through a set of parameters in the form of V4L2 extensible parameters. The > > > > + * V4L2 extensible parameters define a serialization format for ISP parameters > > > > + * that allows userspace to populate a buffer of configuration data by appending > > > > + * them one after the other in a binary buffer. > > > > + * > > > > + * Each ISP driver compatible with the v4l2-params format will define its own > > > > + * meta-output format identifier and defines the types of the configuration data > > > > + * of each ISP block that usually match the registers layout. > > > > + * > > > > + * The V4L2Params class represent the V4L2 extensible parameters buffer and > > > > + * allows users to populate the ISP configuration blocks, represented by the > > > > + * V4L2ParamBlock class instances. > > > > + * > > > > + * IPA implementations using this helpers should define an enumeration of ISP > > > > + * blocks the IPA module supports and use a set of common abstraction to help > > > > + * their derived implementation of V4L2Params translate the enumerated ISP block > > > > + * identifier to the actual type of the configuration data as defined by the > > > > + * kernel interface. > > > > + * > > > > + * As an example of this see the RkISP1 and Mali-C55 implementations. > > > > + */ > > > > + > > > > +/** > > > > + * \class V4L2ParamsBlock > > > > + * \brief Helper class that represents a ISP configuration block > > > > + * > > > > + * Each ISP function is associated with a set of configuration parameters > > > > + * defined by the kernel interface. > > > > + * > > > > + * This class represents an ISP block configuration entry. It is constructed > > > > + * with a reference to the memory area where the block configuration will be > > > > + * stored in the parameters buffer. The template parameter represents > > > > + * the underlying kernel-defined ISP block configuration type and allow its > > > > + * user to easily cast it to said type to populate and read the configuration > > > > + * parameters. > > > > + * > > > > + * \sa V4L2Params::block() > > > > + */ > > > > + > > > > +/** > > > > + * \fn V4L2ParamsBlock::V4L2ParamsBlock() > > > > + * \brief Construct a V4L2ParamsBlock with memory represented by \a data > > > > + * \param[in] data A view on the memory area where the ISP block is located > > > > + */ > > > > + > > > > +/** > > > > + * \fn V4L2ParamsBlock::setEnabled() > > > > + * \brief Enable/disable an ISP configuration block > > > > + * \param[in] enabled The enable flag > > > > + */ > > > > + > > > > +/** > > > > + * \fn V4L2ParamsBlock::header() > > > > + * \brief Retrieve a reference to the header (struct v4l2_params_block_header) > > > > + * \return The block header > > > > + */ > > > > + > > > > +/** > > > > + * \fn V4L2ParamsBlock::data() > > > > + * \brief Retrieve a reference to block configuration data memory area > > > > + * \return The block data > > > > + */ > > > > + > > > > +/** > > > > + * \fn V4L2ParamsBlock::operator->() > > > > + * \brief Access the ISP configuration block casting it to the kernel-defined > > > > + * ISP configuration type > > > > + * > > > > + * The V4L2ParamsBlock is templated with the kernel defined ISP configuration > > > > + * block type. This function allows users to easily cast a V4L2ParamsBlock to > > > > + * the underlying kernel-defined type in order to easily populate or read > > > > + * the ISP configuration data. > > > > + * > > > > + * \code{.cpp} > > > > + * > > > > + * // The kernel header defines the ISP configuration types, in example > > > > + * // struct my_isp_awb_config_data { > > > > + * // u16 gain_ch00; > > > > + * // u16 gain_ch01; > > > > + * // u16 gain_ch10; > > > > + * // u16 gain_ch11; > > > > + * // > > > > + * // } > > > > + * > > > > + * template<> V4L2ParamsBlock<struct my_isp_awb_config_data> awbBlock > > > > + * > > > > + * awbBlock->gain_ch00 = ...; > > > > + * awbBlock->gain_ch01 = ...; > > > > + * awbBlock->gain_ch10 = ...; > > > > + * awbBlock->gain_ch11 = ...; > > > > + * > > > > + * \endcode > > > > + * > > > > + * Users of this class are not expected to create a V4L2ParamsBlock manually but > > > > + * should rather use V4L2Params::block() to retrieve a reference to the memory > > > > + * area used to construct a V4L2ParamsBlock<T> in their overloaded > > > > + * implementation of V4L2Params::block(). > > > > + */ > > > > + > > > > +/** > > > > + * \fn V4L2ParamsBlock::operator->() const > > > > + * \copydoc V4L2ParamsBlock::operator->() > > > > + */ > > > > + > > > > +/** > > > > + * \fn V4L2ParamsBlock::operator*() const > > > > + * \copydoc V4L2ParamsBlock::operator->() > > > > + */ > > > > + > > > > +/** > > > > + * \fn V4L2ParamsBlock::operator*() > > > > + * \copydoc V4L2ParamsBlock::operator->() > > > > + */ > > > > + > > > > + /** > > > > + * \class V4L2Params > > > > + * \brief Helper class that represent an ISP configuration buffer > > > > + * > > > > + * ISP implementation compatible with v4l2-params define their ISP configuration > > > > + * buffer types compatible with the struct v4l2_params_buffer type. > > > > + * > > > > + * This class represents an ISP configuration buffer. It is constructed > > > > + * with a reference to the memory mapped buffer that will be queued to the ISP. > > > > + * > > > > + * This class is templated with the type of the enumeration of ISP blocks that > > > > + * each IPA module is expected to support. IPA modules are expected to derive > > > > + * this class and use the V4L2Params::block() function to retrieve the memory > > > > + * area for each ISP configuration block and use it to construct a > > > > + * V4L2ParamsBlock<T> with it before returning it to the user. > > > > + * > > > > + * \code{.cpp} > > > > + * > > > > + * enum class myISPBlocks { > > > > + * Agc, > > > > + * Awb, > > > > + * ... > > > > + * }; > > > > + * > > > > + * template<myISPBlocks B> > > > > + * struct block_type { > > > > + * }; > > > > + * > > > > + * template<> > > > > + * struct block_type<myISPBlock::Agc> { > > > > + * using type = struct my_isp_kernel_config_type_agc; > > > > + * }; > > > > + * > > > > + * template<> > > > > + * struct block_type<myISPBlock::Awb> { > > > > + * using type = struct my_isp_kernel_config_type_awb; > > > > + * }; > > > > + * > > > > + * ... > > > > + * > > > > + * class MyISPParams : public V4L2Params<myISPBlocks> > > > > + * { > > > > + * public: > > > > + * template<myISPBlocks B> > > > > + * auto block() > > > > + * { > > > > + * > > > > + * // Use the kernel defined configuration type as template > > > > + * // argument to V4L2ParamsBlock. > > > > + * using Type = typename details::block_type<B>::type; > > > > + * > > > > + * // Each IPA module should provide the information required > > > > + * // to populate the block header > > > > + * > > > > + * ... > > > > + * > > > > + * auto data = V4L2Params::block(B, blockType, blockSize); > > > > + * > > > > + * return V4L2ParamsBlock<Type>(data); > > > > + * } > > > > + * }; > > > > + * > > > > + * \endcode > > > > + * > > > > + * As an example, see the RkISP1Params and MaliC55Params implementations. > > > > + */ > > > > + > > > > +/** > > > > + * \fn V4L2Params::V4L2Params() > > > > + * \brief Construct a V4L2Params > > > > + * \param[in] data Reference to the v4l2-buffer memory mapped area > > > > + * \param[in] version The ISP parameters version the implementation supports > > > > + */ > > > > + > > > > +/** > > > > + * \fn V4L2Params::size() > > > > + * \brief Retrieve the used size of the parameters buffer (in bytes) > > > > + * > > > > + * The parameters buffer size is mostly used to populate the v4l2_buffer > > > > + * bytesused field before queueing the buffer to the ISP. > > > > + * > > > > + * \return The number of bytes occupied by the ISP configuration parameters > > > > + */ > > > > + > > > > +/** > > > > + * \fn V4L2Params::block() > > > > + * \brief Populate an ISP configuration block a returns a reference to its > > > > + * memory > > > > + * \param[in] type The ISP block identifier enumerated by the IPA module > > > > + * \param[in] blockType The kernel-defined ISP block identifier, used to > > > > + * populate the block header > > > > + * \param[in] blockSize The ISP block size, used to populate the block header > > > > + * > > > > + * > > > > + * Initialize the block header with \a blockType and \a blockSize and > > > > + * returns a reference to the memory used to store an ISP configuration block. > > > > + * > > > > + * IPA modules that derive the V4L2Params class shall use this function to > > > > + * retrieve the memory area that will be used to construct a V4L2ParamsBlock<T> > > > > + * before returning it to the caller. > > > > + */ > > > > + > > > > +/** > > > > + * \var V4L2Params::data_ > > > > + * \brief The ISP parameters buffer memory > > > > + */ > > > > + > > > > +/** > > > > + * \var V4L2Params::used_ > > > > + * \brief The number of bytes used in the parameters buffer > > > > + */ > > > > + > > > > +/** > > > > + * \var V4L2Params::blocks_ > > > > + * \brief Cache of ISP configuration blocks > > > > + */ > > > > + > > > > +} /* namespace ipa */ > > > > + > > > > +} /* namespace libcamera */ > > > > diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h > > > > new file mode 100644 > > > > index 0000000000000000000000000000000000000000..5586096c7ee8a2d20877838564e8074e0fc3d1ce > > > > --- /dev/null > > > > +++ b/src/ipa/libipa/v4l2_params.h > > > > @@ -0,0 +1,135 @@ > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > +/* > > > > + * Copyright (C) 2025, Ideas On Board > > > > + * > > > > + * V4L2 Parameters > > > > + */ > > > > + > > > > +#pragma once > > > > + > > > > +#include <map> > > > > +#include <stdint.h> > > > > +#include <string.h> > > > > + > > > > +#include <linux/media/v4l2-extensible-params.h> > > > > + > > > > +#include <libcamera/base/class.h> > > > > +#include <libcamera/base/span.h> > > > > + > > > > +namespace libcamera { > > > > + > > > > +namespace ipa { > > > > + > > > > +template<typename T> > > > > +class V4L2ParamsBlock > > > > +{ > > > > +public: > > > > + V4L2ParamsBlock(const Span<uint8_t> &data) > > > > > > I think taking by value is fine, no need for the const ref. > > > > > > > right indeed > > > > > > > > > + { > > > > + header_ = data.subspan(0, sizeof(v4l2_params_block_header)); > > > > + data_ = data.subspan(sizeof(v4l2_params_block_header)); > > > > > > I'd probably do these in the member init list. > > > > > > > is this just a style preference or are there reasons I'm missing ? > > There is a difference: one is an assignment, one is a constructor call; > so I always try to use the member init list. Although the difference does > not really matter much here, so you could say it's a preference thing. > No problem, I'll use the member init list > > > > > > > > > > + } > > > > + > > > > + void setEnabled(bool enabled) > > > > + { > > > > + struct v4l2_params_block_header *header = > > > > + reinterpret_cast<struct v4l2_params_block_header *>(header_.data()); > > > > + > > > > + header->flags &= ~(V4L2_PARAMS_FL_BLOCK_ENABLE | > > > > + V4L2_PARAMS_FL_BLOCK_DISABLE); > > > > + header->flags |= enabled ? V4L2_PARAMS_FL_BLOCK_ENABLE > > > > + : V4L2_PARAMS_FL_BLOCK_DISABLE; > > > > + } > > > > + > > > > + Span<uint8_t> header() const { return header_; } > > > > + Span<uint8_t> data() const { return data_; } > > > > + > > > > + const T *operator->() const > > > > + { > > > > + return reinterpret_cast<const T *>(data().data()); > > > > + } > > > > + > > > > + T *operator->() > > > > + { > > > > + return reinterpret_cast<T *>(data().data()); > > > > + } > > > > + > > > > + const T &operator*() const > > > > + { > > > > + return *reinterpret_cast<const T *>(data().data()); > > > > + } > > > > + > > > > + T &operator*() > > > > + { > > > > + return *reinterpret_cast<T *>(data().data()); > > > > + } > > > > + > > > > +private: > > > > + LIBCAMERA_DISABLE_COPY(V4L2ParamsBlock) > > > > > > Why disable? I would assume that it's OK to copy this? > > > > > > > tbh I think I just copied this from RkISP1Params. > > > > However, why would one copy an instance of V4L2ParamsBlock ? > > My thinking was that it is a "reference-like type", so there is no reason to > prevent copying since it works and might be useful in some cases (e.g. passing > to functions, etc.). > Right. I'll drop this > > > > > > > > > > + > > > > + Span<uint8_t> header_; > > > > + Span<uint8_t> data_; > > > > +}; > > > > + > > > > +template<typename T> > > > > +class V4L2Params > > > > +{ > > > > +public: > > > > + V4L2Params(Span<uint8_t> data, unsigned int version) > > > > + : data_(data) > > > > + { > > > > + struct v4l2_params_buffer *cfg = > > > > + reinterpret_cast<struct v4l2_params_buffer *>(data_.data()); > > > > + cfg->data_size = 0; > > > > + cfg->version = version; > > > > + used_ = offsetof(struct v4l2_params_buffer, data); > > > > + } > > > > + > > > > + size_t size() const { return used_; } > > > > + > > > > +protected: > > > > + Span<uint8_t> block(T type, unsigned int blockType, size_t blockSize) > > > > > > This can modified so that the derived classes need not implement any extra logic. > > > One option is to pass the `details::block_type` template as a template argument > > > to V4L2Params and then use that to map the id to the type: > > > > > > template<typename T, template<T> typename IdToType> > > > class V4L2Params { > > > > > > template<T ID> > > > auto block() { > > > using Type = typename IdToType<ID>::type; > > > ... > > > } > > > > > > but I think it's simpler and more extensible to pass another type that holds > > > all this compile-time information. It could look something like this: > > > > > > diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h > > > index 5586096c7..019f06509 100644 > > > --- a/src/ipa/libipa/v4l2_params.h > > > +++ b/src/ipa/libipa/v4l2_params.h > > > @@ -71,7 +71,7 @@ private: > > > Span<uint8_t> data_; > > > }; > > > -template<typename T> > > > +template<typename Traits> > > > class V4L2Params > > > { > > > public: > > > @@ -87,8 +87,21 @@ public: > > > size_t size() const { return used_; } > > > + template<typename Traits::id_type Id> > > > + auto block() > > > + { > > > + using Details = typename Traits::template id_to_details<Id>; > > > > I can't grok 'typename Traits::template' > > > > but it's ok, I'll trust you here > > See https://www.en.cppreference.com/w/cpp/language/dependent_name.html > under "The template disambiguator for dependent names". It just tells > the compiler that `id_to_details` is a template. > > This is quite similar to having to use the `typename` keyword in some cases. > (see "The typename disambiguator for dependent names" on the linked site) > Thanks, I'll educate myself > > > > > > + > > > + using Type = typename Details::type; > > > + constexpr auto kernelId = Details::blockType; > > > + > > > + auto data = block(Id, kernelId, sizeof(Type)); > > > + > > > + return V4L2ParamsBlock<Type>(data); > > > + } > > > + > > > protected: > > > - Span<uint8_t> block(T type, unsigned int blockType, size_t blockSize) > > > + Span<uint8_t> block(typename Traits::id_type type, unsigned int blockType, size_t blockSize) > > > { > > > /* > > > * Look up the block in the cache first. If an algorithm > > > @@ -127,7 +140,7 @@ protected: > > > Span<uint8_t> data_; > > > size_t used_; > > > - std::map<T, Span<uint8_t>> blocks_; > > > + std::map<typename Traits::id_type, Span<uint8_t>> blocks_; > > > }; > > > } /* namespace ipa */ > > > diff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h > > > index 1cc56dbad..975122999 100644 > > > --- a/src/ipa/mali-c55/params.h > > > +++ b/src/ipa/mali-c55/params.h > > > @@ -58,30 +58,24 @@ MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingConfig, mesh_shading_config, > > > MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingSel, mesh_shading_selection, > > > MESH_SHADING_SELECTION) > > > +struct params_traits { > > > + using id_type = MaliC55Blocks; > > > + > > > + template<id_type Id> > > > + using id_to_details = block_type<Id>; > > > +}; > > > + > > > } /* namespace details */ > > > -class MaliC55Params : public V4L2Params<MaliC55Blocks> > > > +class MaliC55Params : public V4L2Params<details::params_traits> > > > { > > > public: > > > static constexpr unsigned int kVersion = MALI_C55_PARAM_BUFFER_V1; > > > MaliC55Params(Span<uint8_t> data) > > > - : V4L2Params<MaliC55Blocks>(data, kVersion) > > > + : V4L2Params(data, kVersion) > > > { > > > } > > > - > > > - template<MaliC55Blocks B> > > > - auto block() > > > - { > > > - using Type = typename details::block_type<B>::type; > > > - > > > - auto blockType = details::block_type<B>::blockType; > > > - size_t blockSize = sizeof(Type); > > > - > > > - auto data = V4L2Params::block(B, blockType, blockSize); > > > - > > > - return V4L2ParamsBlock<Type>(data); > > > - } > > > > Thanks! > > > > 'struct param_traits' and the associated changes in V4L2Params allowed > > me to simplify the MaliC55 implementation as you suggested > > > > > }; > > > } /* namespace ipa::mali_c55 */ > > > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h > > > index 7a2127664..824a5924f 100644 > > > --- a/src/ipa/rkisp1/params.h > > > +++ b/src/ipa/rkisp1/params.h > > > @@ -45,45 +45,54 @@ template<BlockType B> > > > struct block_type { > > > }; > > > -#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct) \ > > > +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct, id) \ > > > template<> \ > > > struct block_type<BlockType::blockType> { \ > > > using type = struct rkisp1_cif_isp_##blockStruct##_config; \ > > > + static constexpr rkisp1_ext_params_block_type blockType = \ > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_##id; \ > > > }; > > > -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) > > > -RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls) > > > -RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve) > > > -RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve) > > > +RKISP1_DEFINE_BLOCK_TYPE(Bls, bls, BLS) > > > +RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc, DPCC) > > > +RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg, SDG) > > > +RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain, AWB_GAIN) > > > +RKISP1_DEFINE_BLOCK_TYPE(Flt, flt, FLT) > > > +RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm, BDM) > > > +RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk, CTK) > > > +RKISP1_DEFINE_BLOCK_TYPE(Goc, goc, GOC) > > > +RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf, DPF) > > > +RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength, DPF_STRENGTH) > > > +RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc, CPROC) > > > +RKISP1_DEFINE_BLOCK_TYPE(Ie, ie, IE) > > > +RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc, LSC) > > > +RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas, AWB_MEAS) > > > +RKISP1_DEFINE_BLOCK_TYPE(Hst, hst, HST_MEAS) > > > +RKISP1_DEFINE_BLOCK_TYPE(Aec, aec, AEC_MEAS) > > > +RKISP1_DEFINE_BLOCK_TYPE(Afc, afc, AFC_MEAS) > > > +RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls, COMPAND_BLS) > > > +RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve, COMPAND_EXPAND) > > > +RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve, COMPAND_COMPRESS) > > > + > > > +struct params_traits { > > > + using id_type = BlockType; > > > + > > > + template<id_type Id> > > > + using id_to_details = block_type<Id>; > > > +}; > > > } /* namespace details */ > > > template<typename T> > > > class RkISP1ParamsBlock; > > > -class RkISP1Params : public V4L2Params<BlockType> > > > +class RkISP1Params : public V4L2Params<details::params_traits> > > > { > > > public: > > > static constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1; > > > RkISP1Params(uint32_t format, Span<uint8_t> data) > > > - : V4L2Params<BlockType>(data, kVersion), format_(format) > > > + : V4L2Params(data, kVersion), format_(format) > > > { > > > if (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) { > > > memset(data.data(), 0, data.size()); > > > > > > > > > Admittedly I haven't tested it, and the rkisp1 parts are not there yet. > > > But I believe this direction could simplify the code and make it easier > > > to implement for other parameter sets. > > > > Please note that for RkISP1 I had to keep an override for > > RkISP1Params::block() as we need to support both legacy and extensible > > > > > > > > For rkisp1, I think as a first step `kBlockTypeInfo` should be moved completely > > > into `rkisp1::details::block_type` template specializations to do the mapping > > > at compile time, similarly to how the proposed `MaliC55Params` does it. > > > > yeah, I don't like that map too much. But, if I may use it as an > > excuse, it was there already so we're not doing any worse. > > > > I could have removed it like it's done for Mali, but the 'enableBit' > > used by setEnabled() to support the legacy format made it more > > complicated getting rid of it (I've not invested much time in trying, > > admittedly). > > > > Anyway, could easily be done on top ? > > Yes. > > > > > > > > > > Then I believe one could realistically extend this `V4L2Params` to even support > > > fixed-offset formats generically, if needed, but that might be an overkill. > > > > Not sure I got what you mean here, > > What I meant is that it is probably possible to support "legacy" parameter formats > like the rkisp1 one in the base template. But if that's the only one that is > relevant for us, then it's very likely an overkill. > I don't expect other platforms we supports to have to support both formats at the moment, so I would leave this option out for the time being > > > > > > > > > > > + { > > > > + /* > > > > + * 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. */ > > > > + if (blockSize > data_.size() - used_) > > > > + return {}; > > > > + > > > > + /* Allocate a new block, clear its memory, and initialize its header. */ > > > > + Span<uint8_t> block = data_.subspan(used_, blockSize); > > > > + used_ += blockSize; > > > > + > > > > + struct v4l2_params_buffer *cfg = > > > > + reinterpret_cast<struct v4l2_params_buffer *>(data_.data()); > > > > + cfg->data_size += blockSize; > > > > + > > > > + memset(block.data(), 0, block.size()); > > > > + > > > > + struct v4l2_params_block_header *header = > > > > + reinterpret_cast<struct v4l2_params_block_header *>(block.data()); > > > > + header->type = blockType; > > > > + header->size = block.size(); > > > > + > > > > + /* Update the cache. */ > > > > + blocks_[type] = block; > > > > + > > > > + return block; > > > > + } > > > > + > > > > + Span<uint8_t> data_; > > > > + size_t used_; > > > > + > > > > + std::map<T, Span<uint8_t>> blocks_; > > > > +}; > > > > + > > > > +} /* namespace ipa */ > > > > + > > > > +} /* namespace libcamera */ > > > > diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp > > > > index 4c0b051ce65da1686323ee9c66b82e12669a754d..2b692e1a1f199d6c118af0938e1aaecef6186a2c 100644 > > > > --- a/src/ipa/rkisp1/params.cpp > > > > +++ b/src/ipa/rkisp1/params.cpp > > > > @@ -35,7 +35,7 @@ struct BlockTypeInfo { > > > > #define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit) \ > > > > { BlockType::block, { \ > > > > RKISP1_EXT_PARAMS_BLOCK_TYPE_##id, \ > > > > - sizeof(struct rkisp1_cif_isp_##type##_config), \ > > > > + sizeof(struct rkisp1_ext_params_##type##_config), \ > > > > offsetof(struct rkisp1_params_cfg, category.type##_config), \ > > > > RKISP1_CIF_ISP_MODULE_##bit, \ > > > > } } > > > > @@ -49,7 +49,7 @@ struct BlockTypeInfo { > > > > #define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type) \ > > > > { BlockType::block, { \ > > > > RKISP1_EXT_PARAMS_BLOCK_TYPE_##id, \ > > > > - sizeof(struct rkisp1_cif_isp_##type##_config), \ > > > > + sizeof(struct rkisp1_ext_params_##type##_config), \ > > > > 0, 0, \ > > > > } } > > > > @@ -78,56 +78,6 @@ const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = { > > > > } /* namespace */ > > > > -RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type, > > > > - const Span<uint8_t> &data) > > > > - : params_(params), type_(type) > > > > -{ > > > > - if (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) { > > > > - header_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header)); > > > > - data_ = data.subspan(sizeof(rkisp1_ext_params_block_header)); > > > > - } else { > > > > - data_ = data; > > > > - } > > > > -} > > > > - > > > > -void RkISP1ParamsBlockBase::setEnabled(bool enabled) > > > > -{ > > > > - /* > > > > - * For the legacy fixed format, blocks are enabled in the top-level > > > > - * header. Delegate to the RkISP1Params class. > > > > - */ > > > > - if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS) > > > > - return params_->setBlockEnabled(type_, enabled); > > > > - > > > > - /* > > > > - * For the extensible format, set the enable and disable flags in the > > > > - * block header directly. > > > > - */ > > > > - struct rkisp1_ext_params_block_header *header = > > > > - reinterpret_cast<struct rkisp1_ext_params_block_header *>(header_.data()); > > > > - header->flags &= ~(RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE | > > > > - RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE); > > > > - header->flags |= enabled ? RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE > > > > - : RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE; > > > > -} > > > > - > > > > -RkISP1Params::RkISP1Params(uint32_t format, Span<uint8_t> data) > > > > - : format_(format), data_(data), used_(0) > > > > -{ > > > > - if (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) { > > > > - struct rkisp1_ext_params_cfg *cfg = > > > > - reinterpret_cast<struct rkisp1_ext_params_cfg *>(data.data()); > > > > - > > > > - cfg->version = RKISP1_EXT_PARAM_BUFFER_V1; > > > > - cfg->data_size = 0; > > > > - > > > > - used_ += offsetof(struct rkisp1_ext_params_cfg, data); > > > > - } else { > > > > - memset(data.data(), 0, data.size()); > > > > - used_ = sizeof(struct rkisp1_params_cfg); > > > > - } > > > > -} > > > > - > > > > void RkISP1Params::setBlockEnabled(BlockType type, bool enabled) > > > > { > > > > const BlockTypeInfo &info = kBlockTypeInfo.at(type); > > > > @@ -177,44 +127,7 @@ Span<uint8_t> RkISP1Params::block(BlockType type) > > > > 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; > > > > + return V4L2Params::block(type, info.type, info.size); > > > > } > > > > } /* namespace ipa::rkisp1 */ > > > > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h > > > > index 40450e34497a3aa71b5b0cda2bf045a1cc0e012f..7a21276648162a127317c75cb01d1c0059b96ee1 100644 > > > > --- a/src/ipa/rkisp1/params.h > > > > +++ b/src/ipa/rkisp1/params.h > > > > @@ -7,13 +7,10 @@ > > > > #pragma once > > > > -#include <map> > > > > -#include <stdint.h> > > > > - > > > > #include <linux/rkisp1-config.h> > > > > +#include <linux/videodev2.h> > > > > -#include <libcamera/base/class.h> > > > > -#include <libcamera/base/span.h> > > > > +#include <libipa/v4l2_params.h> > > > > namespace libcamera { > > > > @@ -77,85 +74,72 @@ RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve) > > > > } /* namespace details */ > > > > -class RkISP1Params; > > > > +template<typename T> > > > > +class RkISP1ParamsBlock; > > > > -class RkISP1ParamsBlockBase > > > > +class RkISP1Params : public V4L2Params<BlockType> > > > > { > > > > public: > > > > - RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type, > > > > - const Span<uint8_t> &data); > > > > - > > > > - Span<uint8_t> data() const { return data_; } > > > > - > > > > - void setEnabled(bool enabled); > > > > + static constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1; > > > > -private: > > > > - LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase) > > > > - > > > > - RkISP1Params *params_; > > > > - BlockType type_; > > > > - Span<uint8_t> header_; > > > > - Span<uint8_t> data_; > > > > -}; > > > > - > > > > -template<BlockType B> > > > > -class RkISP1ParamsBlock : public RkISP1ParamsBlockBase > > > > -{ > > > > -public: > > > > - using Type = typename details::block_type<B>::type; > > > > - > > > > - RkISP1ParamsBlock(RkISP1Params *params, const Span<uint8_t> &data) > > > > - : RkISP1ParamsBlockBase(params, B, data) > > > > + RkISP1Params(uint32_t format, Span<uint8_t> data) > > > > + : V4L2Params<BlockType>(data, kVersion), format_(format) > > > > { > > > > + if (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) { > > > > + memset(data.data(), 0, data.size()); > > > > + used_ = sizeof(struct rkisp1_params_cfg); > > > > + } > > > > } > > > > - const Type *operator->() const > > > > + template<BlockType B> > > > > + auto block() > > > > { > > > > - return reinterpret_cast<const Type *>(data().data()); > > > > - } > > > > + using Type = typename details::block_type<B>::type; > > > > - Type *operator->() > > > > - { > > > > - return reinterpret_cast<Type *>(data().data()); > > > > + return RkISP1ParamsBlock<Type>(this, B, block(B)); > > > > } > > > > - const Type &operator*() const & > > > > - { > > > > - return *reinterpret_cast<const Type *>(data().data()); > > > > - } > > > > + uint32_t format() const { return format_; } > > > > + void setBlockEnabled(BlockType type, bool enabled); > > > > - Type &operator*() & > > > > - { > > > > - return *reinterpret_cast<Type *>(data().data()); > > > > - } > > > > +private: > > > > + Span<uint8_t> block(BlockType type); > > > > + > > > > + uint32_t format_; > > > > }; > > > > -class RkISP1Params > > > > +template<typename T> > > > > +class RkISP1ParamsBlock : public V4L2ParamsBlock<T> > > > > { > > > > public: > > > > - RkISP1Params(uint32_t format, Span<uint8_t> data); > > > > - > > > > - template<BlockType B> > > > > - RkISP1ParamsBlock<B> block() > > > > + RkISP1ParamsBlock(RkISP1Params *params, BlockType type, > > > > + const Span<uint8_t> &data) > > > > + : V4L2ParamsBlock<T>(data) > > > > { > > > > - return RkISP1ParamsBlock<B>(this, block(B)); > > > > + params_ = params; > > > > + type_ = type; > > > > + > > > > + /* Legacy param format has no header */ > > > > + if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS) > > > > + data_ = data; > > > > } > > > > - uint32_t format() const { return format_; } > > > > - size_t size() const { return used_; } > > > > + void 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); > > > > + > > > > + return V4L2ParamsBlock<T>::setEnabled(enabled); > > > > + } > > > > > > I have one concern regarding this design. An `RkISP1ParamsBlock` instance > > > will not work as its base type correctly in all cases. That is, calling the > > > base class `setEnabled()`: > > > > > > RkISP1ParamsBlock<...> *p = ...; > > > > > > p->V4L2ParamsBlock<...>::setEnabled(...); > > > // or > > > static_cast<V4L2ParamsBlock<...> *>(p)->setEnabled(...); > > > > > > will not work correctly if the format is the "non-extensible" one. > > > > The only user of this class is the RkISP1 IPA, why would it forcefully > > use the base class implementation ? > > It probably wouldn't, I just wanted to make a note of this. Although I can > imagine that with time some parts would want to operate on `V4L2ParamsBlock` > objects directly, in a generic fashion, and then this might be an issue. > Only for RkISP1 which supports both legacy and extensible, right ? > > > > > If there's an easy way to avoid the above, I'll anyway happily consider > > it. > > Sadly I don't think I have an easy solution here. I'll take a look again > at the next version. thanks, let's not be concerned with this for the moment. Next version out soon, thanks! > > > Regards, > Barnabás Pőcze > > > > > > Thanks > > j > > > > > > > > > > > Regards, > > > Barnabás Pőcze > > > > > > > > > > private: > > > > - friend class RkISP1ParamsBlockBase; > > > > - > > > > - Span<uint8_t> block(BlockType type); > > > > - void setBlockEnabled(BlockType type, bool enabled); > > > > - > > > > - uint32_t format_; > > > > - > > > > + RkISP1Params *params_; > > > > + BlockType type_; > > > > Span<uint8_t> data_; > > > > - size_t used_; > > > > - > > > > - std::map<BlockType, Span<uint8_t>> blocks_; > > > > }; > > > > } /* namespace ipa::rkisp1 */ > > > > > > > >
diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build index 660be94054fa98b714b6bc586039081e45a6b4bc..4010739e710eb38aa6108eb8258c574a616bf3c0 100644 --- a/src/ipa/libipa/meson.build +++ b/src/ipa/libipa/meson.build @@ -16,6 +16,7 @@ libipa_headers = files([ 'lsc_polynomial.h', 'lux.h', 'module.h', + 'v4l2_params.h', 'pwl.h', ]) @@ -35,6 +36,7 @@ libipa_sources = files([ 'lsc_polynomial.cpp', 'lux.cpp', 'module.cpp', + 'v4l2_params.cpp', 'pwl.cpp', ]) diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp new file mode 100644 index 0000000000000000000000000000000000000000..674018065ace0e1b6b48b1630e556cef590d1e84 --- /dev/null +++ b/src/ipa/libipa/v4l2_params.cpp @@ -0,0 +1,252 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2025, Ideas On Board + * + * V4L2 Parameters + */ + +#include "v4l2_params.h" + +namespace libcamera { + +namespace ipa { + +/** + * \file v4l2_params.cpp + * \brief Helper class to populate a v4l2-params compatible parameters buffer + * + * The Linux kernel defines a generic buffer format for configuring ISP devices + * through a set of parameters in the form of V4L2 extensible parameters. The + * V4L2 extensible parameters define a serialization format for ISP parameters + * that allows userspace to populate a buffer of configuration data by appending + * them one after the other in a binary buffer. + * + * Each ISP driver compatible with the v4l2-params format will define its own + * meta-output format identifier and defines the types of the configuration data + * of each ISP block that usually match the registers layout. + * + * The V4L2Params class represent the V4L2 extensible parameters buffer and + * allows users to populate the ISP configuration blocks, represented by the + * V4L2ParamBlock class instances. + * + * IPA implementations using this helpers should define an enumeration of ISP + * blocks the IPA module supports and use a set of common abstraction to help + * their derived implementation of V4L2Params translate the enumerated ISP block + * identifier to the actual type of the configuration data as defined by the + * kernel interface. + * + * As an example of this see the RkISP1 and Mali-C55 implementations. + */ + +/** + * \class V4L2ParamsBlock + * \brief Helper class that represents a ISP configuration block + * + * Each ISP function is associated with a set of configuration parameters + * defined by the kernel interface. + * + * This class represents an ISP block configuration entry. It is constructed + * with a reference to the memory area where the block configuration will be + * stored in the parameters buffer. The template parameter represents + * the underlying kernel-defined ISP block configuration type and allow its + * user to easily cast it to said type to populate and read the configuration + * parameters. + * + * \sa V4L2Params::block() + */ + +/** + * \fn V4L2ParamsBlock::V4L2ParamsBlock() + * \brief Construct a V4L2ParamsBlock with memory represented by \a data + * \param[in] data A view on the memory area where the ISP block is located + */ + +/** + * \fn V4L2ParamsBlock::setEnabled() + * \brief Enable/disable an ISP configuration block + * \param[in] enabled The enable flag + */ + +/** + * \fn V4L2ParamsBlock::header() + * \brief Retrieve a reference to the header (struct v4l2_params_block_header) + * \return The block header + */ + +/** + * \fn V4L2ParamsBlock::data() + * \brief Retrieve a reference to block configuration data memory area + * \return The block data + */ + +/** + * \fn V4L2ParamsBlock::operator->() + * \brief Access the ISP configuration block casting it to the kernel-defined + * ISP configuration type + * + * The V4L2ParamsBlock is templated with the kernel defined ISP configuration + * block type. This function allows users to easily cast a V4L2ParamsBlock to + * the underlying kernel-defined type in order to easily populate or read + * the ISP configuration data. + * + * \code{.cpp} + * + * // The kernel header defines the ISP configuration types, in example + * // struct my_isp_awb_config_data { + * // u16 gain_ch00; + * // u16 gain_ch01; + * // u16 gain_ch10; + * // u16 gain_ch11; + * // + * // } + * + * template<> V4L2ParamsBlock<struct my_isp_awb_config_data> awbBlock + * + * awbBlock->gain_ch00 = ...; + * awbBlock->gain_ch01 = ...; + * awbBlock->gain_ch10 = ...; + * awbBlock->gain_ch11 = ...; + * + * \endcode + * + * Users of this class are not expected to create a V4L2ParamsBlock manually but + * should rather use V4L2Params::block() to retrieve a reference to the memory + * area used to construct a V4L2ParamsBlock<T> in their overloaded + * implementation of V4L2Params::block(). + */ + +/** + * \fn V4L2ParamsBlock::operator->() const + * \copydoc V4L2ParamsBlock::operator->() + */ + +/** + * \fn V4L2ParamsBlock::operator*() const + * \copydoc V4L2ParamsBlock::operator->() + */ + +/** + * \fn V4L2ParamsBlock::operator*() + * \copydoc V4L2ParamsBlock::operator->() + */ + + /** + * \class V4L2Params + * \brief Helper class that represent an ISP configuration buffer + * + * ISP implementation compatible with v4l2-params define their ISP configuration + * buffer types compatible with the struct v4l2_params_buffer type. + * + * This class represents an ISP configuration buffer. It is constructed + * with a reference to the memory mapped buffer that will be queued to the ISP. + * + * This class is templated with the type of the enumeration of ISP blocks that + * each IPA module is expected to support. IPA modules are expected to derive + * this class and use the V4L2Params::block() function to retrieve the memory + * area for each ISP configuration block and use it to construct a + * V4L2ParamsBlock<T> with it before returning it to the user. + * + * \code{.cpp} + * + * enum class myISPBlocks { + * Agc, + * Awb, + * ... + * }; + * + * template<myISPBlocks B> + * struct block_type { + * }; + * + * template<> + * struct block_type<myISPBlock::Agc> { + * using type = struct my_isp_kernel_config_type_agc; + * }; + * + * template<> + * struct block_type<myISPBlock::Awb> { + * using type = struct my_isp_kernel_config_type_awb; + * }; + * + * ... + * + * class MyISPParams : public V4L2Params<myISPBlocks> + * { + * public: + * template<myISPBlocks B> + * auto block() + * { + * + * // Use the kernel defined configuration type as template + * // argument to V4L2ParamsBlock. + * using Type = typename details::block_type<B>::type; + * + * // Each IPA module should provide the information required + * // to populate the block header + * + * ... + * + * auto data = V4L2Params::block(B, blockType, blockSize); + * + * return V4L2ParamsBlock<Type>(data); + * } + * }; + * + * \endcode + * + * As an example, see the RkISP1Params and MaliC55Params implementations. + */ + +/** + * \fn V4L2Params::V4L2Params() + * \brief Construct a V4L2Params + * \param[in] data Reference to the v4l2-buffer memory mapped area + * \param[in] version The ISP parameters version the implementation supports + */ + +/** + * \fn V4L2Params::size() + * \brief Retrieve the used size of the parameters buffer (in bytes) + * + * The parameters buffer size is mostly used to populate the v4l2_buffer + * bytesused field before queueing the buffer to the ISP. + * + * \return The number of bytes occupied by the ISP configuration parameters + */ + +/** + * \fn V4L2Params::block() + * \brief Populate an ISP configuration block a returns a reference to its + * memory + * \param[in] type The ISP block identifier enumerated by the IPA module + * \param[in] blockType The kernel-defined ISP block identifier, used to + * populate the block header + * \param[in] blockSize The ISP block size, used to populate the block header + * + * + * Initialize the block header with \a blockType and \a blockSize and + * returns a reference to the memory used to store an ISP configuration block. + * + * IPA modules that derive the V4L2Params class shall use this function to + * retrieve the memory area that will be used to construct a V4L2ParamsBlock<T> + * before returning it to the caller. + */ + +/** + * \var V4L2Params::data_ + * \brief The ISP parameters buffer memory + */ + +/** + * \var V4L2Params::used_ + * \brief The number of bytes used in the parameters buffer + */ + +/** + * \var V4L2Params::blocks_ + * \brief Cache of ISP configuration blocks + */ + +} /* namespace ipa */ + +} /* namespace libcamera */ diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h new file mode 100644 index 0000000000000000000000000000000000000000..5586096c7ee8a2d20877838564e8074e0fc3d1ce --- /dev/null +++ b/src/ipa/libipa/v4l2_params.h @@ -0,0 +1,135 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2025, Ideas On Board + * + * V4L2 Parameters + */ + +#pragma once + +#include <map> +#include <stdint.h> +#include <string.h> + +#include <linux/media/v4l2-extensible-params.h> + +#include <libcamera/base/class.h> +#include <libcamera/base/span.h> + +namespace libcamera { + +namespace ipa { + +template<typename T> +class V4L2ParamsBlock +{ +public: + V4L2ParamsBlock(const Span<uint8_t> &data) + { + header_ = data.subspan(0, sizeof(v4l2_params_block_header)); + data_ = data.subspan(sizeof(v4l2_params_block_header)); + } + + void setEnabled(bool enabled) + { + struct v4l2_params_block_header *header = + reinterpret_cast<struct v4l2_params_block_header *>(header_.data()); + + header->flags &= ~(V4L2_PARAMS_FL_BLOCK_ENABLE | + V4L2_PARAMS_FL_BLOCK_DISABLE); + header->flags |= enabled ? V4L2_PARAMS_FL_BLOCK_ENABLE + : V4L2_PARAMS_FL_BLOCK_DISABLE; + } + + Span<uint8_t> header() const { return header_; } + Span<uint8_t> data() const { return data_; } + + const T *operator->() const + { + return reinterpret_cast<const T *>(data().data()); + } + + T *operator->() + { + return reinterpret_cast<T *>(data().data()); + } + + const T &operator*() const + { + return *reinterpret_cast<const T *>(data().data()); + } + + T &operator*() + { + return *reinterpret_cast<T *>(data().data()); + } + +private: + LIBCAMERA_DISABLE_COPY(V4L2ParamsBlock) + + Span<uint8_t> header_; + Span<uint8_t> data_; +}; + +template<typename T> +class V4L2Params +{ +public: + V4L2Params(Span<uint8_t> data, unsigned int version) + : data_(data) + { + struct v4l2_params_buffer *cfg = + reinterpret_cast<struct v4l2_params_buffer *>(data_.data()); + cfg->data_size = 0; + cfg->version = version; + used_ = offsetof(struct v4l2_params_buffer, data); + } + + size_t size() const { return used_; } + +protected: + Span<uint8_t> block(T type, unsigned int blockType, size_t blockSize) + { + /* + * 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. */ + if (blockSize > data_.size() - used_) + return {}; + + /* Allocate a new block, clear its memory, and initialize its header. */ + Span<uint8_t> block = data_.subspan(used_, blockSize); + used_ += blockSize; + + struct v4l2_params_buffer *cfg = + reinterpret_cast<struct v4l2_params_buffer *>(data_.data()); + cfg->data_size += blockSize; + + memset(block.data(), 0, block.size()); + + struct v4l2_params_block_header *header = + reinterpret_cast<struct v4l2_params_block_header *>(block.data()); + header->type = blockType; + header->size = block.size(); + + /* Update the cache. */ + blocks_[type] = block; + + return block; + } + + Span<uint8_t> data_; + size_t used_; + + std::map<T, Span<uint8_t>> blocks_; +}; + +} /* namespace ipa */ + +} /* namespace libcamera */ diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp index 4c0b051ce65da1686323ee9c66b82e12669a754d..2b692e1a1f199d6c118af0938e1aaecef6186a2c 100644 --- a/src/ipa/rkisp1/params.cpp +++ b/src/ipa/rkisp1/params.cpp @@ -35,7 +35,7 @@ struct BlockTypeInfo { #define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit) \ { BlockType::block, { \ RKISP1_EXT_PARAMS_BLOCK_TYPE_##id, \ - sizeof(struct rkisp1_cif_isp_##type##_config), \ + sizeof(struct rkisp1_ext_params_##type##_config), \ offsetof(struct rkisp1_params_cfg, category.type##_config), \ RKISP1_CIF_ISP_MODULE_##bit, \ } } @@ -49,7 +49,7 @@ struct BlockTypeInfo { #define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type) \ { BlockType::block, { \ RKISP1_EXT_PARAMS_BLOCK_TYPE_##id, \ - sizeof(struct rkisp1_cif_isp_##type##_config), \ + sizeof(struct rkisp1_ext_params_##type##_config), \ 0, 0, \ } } @@ -78,56 +78,6 @@ const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = { } /* namespace */ -RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type, - const Span<uint8_t> &data) - : params_(params), type_(type) -{ - if (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) { - header_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header)); - data_ = data.subspan(sizeof(rkisp1_ext_params_block_header)); - } else { - data_ = data; - } -} - -void RkISP1ParamsBlockBase::setEnabled(bool enabled) -{ - /* - * For the legacy fixed format, blocks are enabled in the top-level - * header. Delegate to the RkISP1Params class. - */ - if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS) - return params_->setBlockEnabled(type_, enabled); - - /* - * For the extensible format, set the enable and disable flags in the - * block header directly. - */ - struct rkisp1_ext_params_block_header *header = - reinterpret_cast<struct rkisp1_ext_params_block_header *>(header_.data()); - header->flags &= ~(RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE | - RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE); - header->flags |= enabled ? RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE - : RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE; -} - -RkISP1Params::RkISP1Params(uint32_t format, Span<uint8_t> data) - : format_(format), data_(data), used_(0) -{ - if (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) { - struct rkisp1_ext_params_cfg *cfg = - reinterpret_cast<struct rkisp1_ext_params_cfg *>(data.data()); - - cfg->version = RKISP1_EXT_PARAM_BUFFER_V1; - cfg->data_size = 0; - - used_ += offsetof(struct rkisp1_ext_params_cfg, data); - } else { - memset(data.data(), 0, data.size()); - used_ = sizeof(struct rkisp1_params_cfg); - } -} - void RkISP1Params::setBlockEnabled(BlockType type, bool enabled) { const BlockTypeInfo &info = kBlockTypeInfo.at(type); @@ -177,44 +127,7 @@ Span<uint8_t> RkISP1Params::block(BlockType type) 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; + return V4L2Params::block(type, info.type, info.size); } } /* namespace ipa::rkisp1 */ diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h index 40450e34497a3aa71b5b0cda2bf045a1cc0e012f..7a21276648162a127317c75cb01d1c0059b96ee1 100644 --- a/src/ipa/rkisp1/params.h +++ b/src/ipa/rkisp1/params.h @@ -7,13 +7,10 @@ #pragma once -#include <map> -#include <stdint.h> - #include <linux/rkisp1-config.h> +#include <linux/videodev2.h> -#include <libcamera/base/class.h> -#include <libcamera/base/span.h> +#include <libipa/v4l2_params.h> namespace libcamera { @@ -77,85 +74,72 @@ RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve) } /* namespace details */ -class RkISP1Params; +template<typename T> +class RkISP1ParamsBlock; -class RkISP1ParamsBlockBase +class RkISP1Params : public V4L2Params<BlockType> { public: - RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type, - const Span<uint8_t> &data); - - Span<uint8_t> data() const { return data_; } - - void setEnabled(bool enabled); + static constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1; -private: - LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase) - - RkISP1Params *params_; - BlockType type_; - Span<uint8_t> header_; - Span<uint8_t> data_; -}; - -template<BlockType B> -class RkISP1ParamsBlock : public RkISP1ParamsBlockBase -{ -public: - using Type = typename details::block_type<B>::type; - - RkISP1ParamsBlock(RkISP1Params *params, const Span<uint8_t> &data) - : RkISP1ParamsBlockBase(params, B, data) + RkISP1Params(uint32_t format, Span<uint8_t> data) + : V4L2Params<BlockType>(data, kVersion), format_(format) { + if (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) { + memset(data.data(), 0, data.size()); + used_ = sizeof(struct rkisp1_params_cfg); + } } - const Type *operator->() const + template<BlockType B> + auto block() { - return reinterpret_cast<const Type *>(data().data()); - } + using Type = typename details::block_type<B>::type; - Type *operator->() - { - return reinterpret_cast<Type *>(data().data()); + return RkISP1ParamsBlock<Type>(this, B, block(B)); } - const Type &operator*() const & - { - return *reinterpret_cast<const Type *>(data().data()); - } + uint32_t format() const { return format_; } + void setBlockEnabled(BlockType type, bool enabled); - Type &operator*() & - { - return *reinterpret_cast<Type *>(data().data()); - } +private: + Span<uint8_t> block(BlockType type); + + uint32_t format_; }; -class RkISP1Params +template<typename T> +class RkISP1ParamsBlock : public V4L2ParamsBlock<T> { public: - RkISP1Params(uint32_t format, Span<uint8_t> data); - - template<BlockType B> - RkISP1ParamsBlock<B> block() + RkISP1ParamsBlock(RkISP1Params *params, BlockType type, + const Span<uint8_t> &data) + : V4L2ParamsBlock<T>(data) { - return RkISP1ParamsBlock<B>(this, block(B)); + params_ = params; + type_ = type; + + /* Legacy param format has no header */ + if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS) + data_ = data; } - uint32_t format() const { return format_; } - size_t size() const { return used_; } + void 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); + + return V4L2ParamsBlock<T>::setEnabled(enabled); + } private: - friend class RkISP1ParamsBlockBase; - - Span<uint8_t> block(BlockType type); - void setBlockEnabled(BlockType type, bool enabled); - - uint32_t format_; - + RkISP1Params *params_; + BlockType type_; Span<uint8_t> data_; - size_t used_; - - std::map<BlockType, Span<uint8_t>> blocks_; }; } /* namespace ipa::rkisp1 */
The existing RkISP1Params helper classes allows the RkISP1 to handle V4L2 extensible parameters format and the legacy RkIPS1-specific fixed-size parameters format. With the introduction of v4l2-params in the Linux kernel the part of the RkISP1Params helper class that handles extensible parameters can be generalized so that other IPA modules can use the same helpers to populate a v4l2-params compatible buffer. Generalize the RkISP1Params class to a new libipa component named V4L2Params and derive the existing RkISP1Params from it, leaving in the RkISP1-specific implementation the handling of the legacy format. Deriving RkISP1Params from V4L2Params requires changing the size associated to each block to include the size of v4l2_params_block_header in the ipa:rkisp1::kBlockTypeInfo map as the V4L2Params::block() implementation doesn't account for that as RkIS1Params::block() implementation did. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- src/ipa/libipa/meson.build | 2 + src/ipa/libipa/v4l2_params.cpp | 252 +++++++++++++++++++++++++++++++++++++++++ src/ipa/libipa/v4l2_params.h | 135 ++++++++++++++++++++++ src/ipa/rkisp1/params.cpp | 93 +-------------- src/ipa/rkisp1/params.h | 108 ++++++++---------- 5 files changed, 438 insertions(+), 152 deletions(-)