| Message ID | 20260507213721.2137448-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
2026. 05. 07. 23:37 keltezéssel, Laurent Pinchart írta: > The Linux kernel V4L2 extensible ISP parameters API uses a 16-bit > integer for the block type. We can therefore standardize on the same > type for the underlying type of the Traits::id_type enum class, as there > will never be more block types in libcamera than in the kernel. This > will help sharing code between different specializations of V4L2Params. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/ipa/libipa/v4l2_params.cpp | 2 +- > src/ipa/libipa/v4l2_params.h | 11 +++++++---- > src/ipa/mali-c55/params.h | 4 +++- > src/ipa/rkisp1/params.cpp | 3 ++- > src/ipa/rkisp1/params.h | 4 +++- > 5 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp > index c2971f1caf28..d44a366a60b8 100644 > --- a/src/ipa/libipa/v4l2_params.cpp > +++ b/src/ipa/libipa/v4l2_params.cpp > @@ -217,7 +217,7 @@ namespace ipa { > */ > > /** > - * \fn V4L2Params::block(typename Traits::id_type type, unsigned int blockType, size_t blockSize) > + * \fn V4L2Params::block(uint16_t type, unsigned int blockType, size_t blockSize) > * \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 > diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h > index f400b37d74b5..4f84360ee449 100644 > --- a/src/ipa/libipa/v4l2_params.h > +++ b/src/ipa/libipa/v4l2_params.h > @@ -15,6 +15,7 @@ > > #include <libcamera/base/log.h> > #include <libcamera/base/span.h> > +#include <libcamera/base/utils.h> > > namespace libcamera { > > @@ -72,6 +73,8 @@ template<typename Traits> > class V4L2Params > { > public: > + static_assert(std::is_same_v<std::underlying_type_t<typename Traits::id_type>, uint16_t>); > + > V4L2Params(Span<uint8_t> data, unsigned int version) > : data_(data) > { > @@ -93,13 +96,13 @@ public: > using Type = typename Details::type; > constexpr auto kernelId = Details::blockType; > > - auto data = block(Id, kernelId, sizeof(Type)); > + auto data = block(utils::to_underlying(Id), kernelId, sizeof(Type)); > return V4L2ParamsBlock<Type>(data); > } > > protected: > - Span<uint8_t> block(typename Traits::id_type type, > - unsigned int blockType, size_t blockSize) > + Span<uint8_t> block(uint16_t type, unsigned int blockType, > + size_t blockSize) > { > /* > * Look up the block in the cache first. If an algorithm > @@ -144,7 +147,7 @@ protected: > Span<uint8_t> data_; > size_t used_; > > - std::map<typename Traits::id_type, Span<uint8_t>> blocks_; > + std::map<uint16_t, Span<uint8_t>> blocks_; A bit unfortunate that now debuggers won't show the keys nicely, but oh well. > }; > > } /* namespace ipa */ > diff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h > index 64be68583ddb..3abcb7f94916 100644 > --- a/src/ipa/mali-c55/params.h > +++ b/src/ipa/mali-c55/params.h > @@ -7,6 +7,8 @@ > > #pragma once > > +#include <stdint.h> > + > #include <linux/media/arm/mali-c55-config.h> > #include <linux/videodev2.h> > > @@ -16,7 +18,7 @@ namespace libcamera { > > namespace ipa::mali_c55 { > > -enum class MaliC55Blocks { > +enum class MaliC55Blocks : uint16_t { > Bls, > AexpHist, > AexpHistWeights, > diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp > index 7040207c2655..b8abbdf6ec66 100644 > --- a/src/ipa/rkisp1/params.cpp > +++ b/src/ipa/rkisp1/params.cpp > @@ -128,7 +128,8 @@ Span<uint8_t> RkISP1Params::block(BlockType type) > return data_.subspan(info.offset, info.size); > } > > - return V4L2Params::block(type, info.type, info.size); > + return V4L2Params::block(utils::to_underlying(type), info.type, > + info.size); Does this warrant a line break? It's the same length as the reinterpret_cast a couple lines above. Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > } > > } /* namespace ipa::rkisp1 */ > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h > index eddb37d5c000..8e3672ca8340 100644 > --- a/src/ipa/rkisp1/params.h > +++ b/src/ipa/rkisp1/params.h > @@ -7,6 +7,8 @@ > > #pragma once > > +#include <stdint.h> > + > #include <linux/rkisp1-config.h> > #include <linux/videodev2.h> > > @@ -16,7 +18,7 @@ namespace libcamera { > > namespace ipa::rkisp1 { > > -enum class BlockType { > +enum class BlockType : uint16_t { > Bls, > Dpcc, > Sdg, > -- > Regards, > > Laurent Pinchart >
Quoting Barnabás Pőcze (2026-05-08 09:35:26) > 2026. 05. 07. 23:37 keltezéssel, Laurent Pinchart írta: > > The Linux kernel V4L2 extensible ISP parameters API uses a 16-bit > > integer for the block type. We can therefore standardize on the same > > type for the underlying type of the Traits::id_type enum class, as there > > will never be more block types in libcamera than in the kernel. This > > will help sharing code between different specializations of V4L2Params. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/ipa/libipa/v4l2_params.cpp | 2 +- > > src/ipa/libipa/v4l2_params.h | 11 +++++++---- > > src/ipa/mali-c55/params.h | 4 +++- > > src/ipa/rkisp1/params.cpp | 3 ++- > > src/ipa/rkisp1/params.h | 4 +++- > > 5 files changed, 16 insertions(+), 8 deletions(-) > > > > diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp > > index c2971f1caf28..d44a366a60b8 100644 > > --- a/src/ipa/libipa/v4l2_params.cpp > > +++ b/src/ipa/libipa/v4l2_params.cpp > > @@ -217,7 +217,7 @@ namespace ipa { > > */ > > > > /** > > - * \fn V4L2Params::block(typename Traits::id_type type, unsigned int blockType, size_t blockSize) > > + * \fn V4L2Params::block(uint16_t type, unsigned int blockType, size_t blockSize) > > * \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 > > diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h > > index f400b37d74b5..4f84360ee449 100644 > > --- a/src/ipa/libipa/v4l2_params.h > > +++ b/src/ipa/libipa/v4l2_params.h > > @@ -15,6 +15,7 @@ > > > > #include <libcamera/base/log.h> > > #include <libcamera/base/span.h> > > +#include <libcamera/base/utils.h> > > > > namespace libcamera { > > > > @@ -72,6 +73,8 @@ template<typename Traits> > > class V4L2Params > > { > > public: > > + static_assert(std::is_same_v<std::underlying_type_t<typename Traits::id_type>, uint16_t>); > > + Surely with this ^ > > V4L2Params(Span<uint8_t> data, unsigned int version) > > : data_(data) > > { > > @@ -93,13 +96,13 @@ public: > > using Type = typename Details::type; > > constexpr auto kernelId = Details::blockType; > > > > - auto data = block(Id, kernelId, sizeof(Type)); > > + auto data = block(utils::to_underlying(Id), kernelId, sizeof(Type)); > > return V4L2ParamsBlock<Type>(data); > > } > > > > protected: > > - Span<uint8_t> block(typename Traits::id_type type, > > - unsigned int blockType, size_t blockSize) > > + Span<uint8_t> block(uint16_t type, unsigned int blockType, > > + size_t blockSize) we don't need to change this > > { > > /* > > * Look up the block in the cache first. If an algorithm > > @@ -144,7 +147,7 @@ protected: > > Span<uint8_t> data_; > > size_t used_; > > > > - std::map<typename Traits::id_type, Span<uint8_t>> blocks_; > > + std::map<uint16_t, Span<uint8_t>> blocks_; > or this, > A bit unfortunate that now debuggers won't show the keys nicely, but oh well. And then this would still work ? -- Kieran > > > > }; > > > > } /* namespace ipa */ > > diff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h > > index 64be68583ddb..3abcb7f94916 100644 > > --- a/src/ipa/mali-c55/params.h > > +++ b/src/ipa/mali-c55/params.h > > @@ -7,6 +7,8 @@ > > > > #pragma once > > > > +#include <stdint.h> > > + > > #include <linux/media/arm/mali-c55-config.h> > > #include <linux/videodev2.h> > > > > @@ -16,7 +18,7 @@ namespace libcamera { > > > > namespace ipa::mali_c55 { > > > > -enum class MaliC55Blocks { > > +enum class MaliC55Blocks : uint16_t { > > Bls, > > AexpHist, > > AexpHistWeights, > > diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp > > index 7040207c2655..b8abbdf6ec66 100644 > > --- a/src/ipa/rkisp1/params.cpp > > +++ b/src/ipa/rkisp1/params.cpp > > @@ -128,7 +128,8 @@ Span<uint8_t> RkISP1Params::block(BlockType type) > > return data_.subspan(info.offset, info.size); > > } > > > > - return V4L2Params::block(type, info.type, info.size); > > + return V4L2Params::block(utils::to_underlying(type), info.type, > > + info.size); > > Does this warrant a line break? It's the same length as the reinterpret_cast > a couple lines above. > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > } > > > > } /* namespace ipa::rkisp1 */ > > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h > > index eddb37d5c000..8e3672ca8340 100644 > > --- a/src/ipa/rkisp1/params.h > > +++ b/src/ipa/rkisp1/params.h > > @@ -7,6 +7,8 @@ > > > > #pragma once > > > > +#include <stdint.h> > > + > > #include <linux/rkisp1-config.h> > > #include <linux/videodev2.h> > > > > @@ -16,7 +18,7 @@ namespace libcamera { > > > > namespace ipa::rkisp1 { > > > > -enum class BlockType { > > +enum class BlockType : uint16_t { > > Bls, > > Dpcc, > > Sdg, > > -- > > Regards, > > > > Laurent Pinchart > > >
2026. 05. 08. 11:02 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2026-05-08 09:35:26) >> 2026. 05. 07. 23:37 keltezéssel, Laurent Pinchart írta: >>> The Linux kernel V4L2 extensible ISP parameters API uses a 16-bit >>> integer for the block type. We can therefore standardize on the same >>> type for the underlying type of the Traits::id_type enum class, as there >>> will never be more block types in libcamera than in the kernel. This >>> will help sharing code between different specializations of V4L2Params. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> src/ipa/libipa/v4l2_params.cpp | 2 +- >>> src/ipa/libipa/v4l2_params.h | 11 +++++++---- >>> src/ipa/mali-c55/params.h | 4 +++- >>> src/ipa/rkisp1/params.cpp | 3 ++- >>> src/ipa/rkisp1/params.h | 4 +++- >>> 5 files changed, 16 insertions(+), 8 deletions(-) >>> >>> diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp >>> index c2971f1caf28..d44a366a60b8 100644 >>> --- a/src/ipa/libipa/v4l2_params.cpp >>> +++ b/src/ipa/libipa/v4l2_params.cpp >>> @@ -217,7 +217,7 @@ namespace ipa { >>> */ >>> >>> /** >>> - * \fn V4L2Params::block(typename Traits::id_type type, unsigned int blockType, size_t blockSize) >>> + * \fn V4L2Params::block(uint16_t type, unsigned int blockType, size_t blockSize) >>> * \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 >>> diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h >>> index f400b37d74b5..4f84360ee449 100644 >>> --- a/src/ipa/libipa/v4l2_params.h >>> +++ b/src/ipa/libipa/v4l2_params.h >>> @@ -15,6 +15,7 @@ >>> >>> #include <libcamera/base/log.h> >>> #include <libcamera/base/span.h> >>> +#include <libcamera/base/utils.h> >>> >>> namespace libcamera { >>> >>> @@ -72,6 +73,8 @@ template<typename Traits> >>> class V4L2Params >>> { >>> public: >>> + static_assert(std::is_same_v<std::underlying_type_t<typename Traits::id_type>, uint16_t>); >>> + > > Surely with this ^ > >>> V4L2Params(Span<uint8_t> data, unsigned int version) >>> : data_(data) >>> { >>> @@ -93,13 +96,13 @@ public: >>> using Type = typename Details::type; >>> constexpr auto kernelId = Details::blockType; >>> >>> - auto data = block(Id, kernelId, sizeof(Type)); >>> + auto data = block(utils::to_underlying(Id), kernelId, sizeof(Type)); >>> return V4L2ParamsBlock<Type>(data); >>> } >>> >>> protected: >>> - Span<uint8_t> block(typename Traits::id_type type, >>> - unsigned int blockType, size_t blockSize) >>> + Span<uint8_t> block(uint16_t type, unsigned int blockType, >>> + size_t blockSize) > > we don't need to change this > >>> { >>> /* >>> * Look up the block in the cache first. If an algorithm >>> @@ -144,7 +147,7 @@ protected: >>> Span<uint8_t> data_; >>> size_t used_; >>> >>> - std::map<typename Traits::id_type, Span<uint8_t>> blocks_; >>> + std::map<uint16_t, Span<uint8_t>> blocks_; >> > > or this, > >> A bit unfortunate that now debuggers won't show the keys nicely, but oh well. > > And then this would still work ? The next patch requires that the map does not depend on `Traits`, and I don't see a way around it. It's a minor thing in any case. > > -- > Kieran > > >> >> >>> }; >>> >>> } /* namespace ipa */ >>> diff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h >>> index 64be68583ddb..3abcb7f94916 100644 >>> --- a/src/ipa/mali-c55/params.h >>> +++ b/src/ipa/mali-c55/params.h >>> @@ -7,6 +7,8 @@ >>> >>> #pragma once >>> >>> +#include <stdint.h> >>> + >>> #include <linux/media/arm/mali-c55-config.h> >>> #include <linux/videodev2.h> >>> >>> @@ -16,7 +18,7 @@ namespace libcamera { >>> >>> namespace ipa::mali_c55 { >>> >>> -enum class MaliC55Blocks { >>> +enum class MaliC55Blocks : uint16_t { >>> Bls, >>> AexpHist, >>> AexpHistWeights, >>> diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp >>> index 7040207c2655..b8abbdf6ec66 100644 >>> --- a/src/ipa/rkisp1/params.cpp >>> +++ b/src/ipa/rkisp1/params.cpp >>> @@ -128,7 +128,8 @@ Span<uint8_t> RkISP1Params::block(BlockType type) >>> return data_.subspan(info.offset, info.size); >>> } >>> >>> - return V4L2Params::block(type, info.type, info.size); >>> + return V4L2Params::block(utils::to_underlying(type), info.type, >>> + info.size); >> >> Does this warrant a line break? It's the same length as the reinterpret_cast >> a couple lines above. >> >> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> >> >>> } >>> >>> } /* namespace ipa::rkisp1 */ >>> diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h >>> index eddb37d5c000..8e3672ca8340 100644 >>> --- a/src/ipa/rkisp1/params.h >>> +++ b/src/ipa/rkisp1/params.h >>> @@ -7,6 +7,8 @@ >>> >>> #pragma once >>> >>> +#include <stdint.h> >>> + >>> #include <linux/rkisp1-config.h> >>> #include <linux/videodev2.h> >>> >>> @@ -16,7 +18,7 @@ namespace libcamera { >>> >>> namespace ipa::rkisp1 { >>> >>> -enum class BlockType { >>> +enum class BlockType : uint16_t { >>> Bls, >>> Dpcc, >>> Sdg, >>> -- >>> Regards, >>> >>> Laurent Pinchart >>> >>
Quoting Barnabás Pőcze (2026-05-08 10:25:59) > 2026. 05. 08. 11:02 keltezéssel, Kieran Bingham írta: > > Quoting Barnabás Pőcze (2026-05-08 09:35:26) > >> 2026. 05. 07. 23:37 keltezéssel, Laurent Pinchart írta: > >>> The Linux kernel V4L2 extensible ISP parameters API uses a 16-bit > >>> integer for the block type. We can therefore standardize on the same > >>> type for the underlying type of the Traits::id_type enum class, as there > >>> will never be more block types in libcamera than in the kernel. This > >>> will help sharing code between different specializations of V4L2Params. > >>> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> src/ipa/libipa/v4l2_params.cpp | 2 +- > >>> src/ipa/libipa/v4l2_params.h | 11 +++++++---- > >>> src/ipa/mali-c55/params.h | 4 +++- > >>> src/ipa/rkisp1/params.cpp | 3 ++- > >>> src/ipa/rkisp1/params.h | 4 +++- > >>> 5 files changed, 16 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp > >>> index c2971f1caf28..d44a366a60b8 100644 > >>> --- a/src/ipa/libipa/v4l2_params.cpp > >>> +++ b/src/ipa/libipa/v4l2_params.cpp > >>> @@ -217,7 +217,7 @@ namespace ipa { > >>> */ > >>> > >>> /** > >>> - * \fn V4L2Params::block(typename Traits::id_type type, unsigned int blockType, size_t blockSize) > >>> + * \fn V4L2Params::block(uint16_t type, unsigned int blockType, size_t blockSize) > >>> * \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 > >>> diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h > >>> index f400b37d74b5..4f84360ee449 100644 > >>> --- a/src/ipa/libipa/v4l2_params.h > >>> +++ b/src/ipa/libipa/v4l2_params.h > >>> @@ -15,6 +15,7 @@ > >>> > >>> #include <libcamera/base/log.h> > >>> #include <libcamera/base/span.h> > >>> +#include <libcamera/base/utils.h> > >>> > >>> namespace libcamera { > >>> > >>> @@ -72,6 +73,8 @@ template<typename Traits> > >>> class V4L2Params > >>> { > >>> public: > >>> + static_assert(std::is_same_v<std::underlying_type_t<typename Traits::id_type>, uint16_t>); > >>> + > > > > Surely with this ^ > > > >>> V4L2Params(Span<uint8_t> data, unsigned int version) > >>> : data_(data) > >>> { > >>> @@ -93,13 +96,13 @@ public: > >>> using Type = typename Details::type; > >>> constexpr auto kernelId = Details::blockType; > >>> > >>> - auto data = block(Id, kernelId, sizeof(Type)); > >>> + auto data = block(utils::to_underlying(Id), kernelId, sizeof(Type)); > >>> return V4L2ParamsBlock<Type>(data); > >>> } > >>> > >>> protected: > >>> - Span<uint8_t> block(typename Traits::id_type type, > >>> - unsigned int blockType, size_t blockSize) > >>> + Span<uint8_t> block(uint16_t type, unsigned int blockType, > >>> + size_t blockSize) > > > > we don't need to change this > > > >>> { > >>> /* > >>> * Look up the block in the cache first. If an algorithm > >>> @@ -144,7 +147,7 @@ protected: > >>> Span<uint8_t> data_; > >>> size_t used_; > >>> > >>> - std::map<typename Traits::id_type, Span<uint8_t>> blocks_; > >>> + std::map<uint16_t, Span<uint8_t>> blocks_; > >> > > > > or this, > > > >> A bit unfortunate that now debuggers won't show the keys nicely, but oh well. > > > > And then this would still work ? > > The next patch requires that the map does not depend on `Traits`, > and I don't see a way around it. It's a minor thing in any case. Ack, ok then: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > -- > > Kieran > > > > > >> > >> > >>> }; > >>> > >>> } /* namespace ipa */ > >>> diff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h > >>> index 64be68583ddb..3abcb7f94916 100644 > >>> --- a/src/ipa/mali-c55/params.h > >>> +++ b/src/ipa/mali-c55/params.h > >>> @@ -7,6 +7,8 @@ > >>> > >>> #pragma once > >>> > >>> +#include <stdint.h> > >>> + > >>> #include <linux/media/arm/mali-c55-config.h> > >>> #include <linux/videodev2.h> > >>> > >>> @@ -16,7 +18,7 @@ namespace libcamera { > >>> > >>> namespace ipa::mali_c55 { > >>> > >>> -enum class MaliC55Blocks { > >>> +enum class MaliC55Blocks : uint16_t { > >>> Bls, > >>> AexpHist, > >>> AexpHistWeights, > >>> diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp > >>> index 7040207c2655..b8abbdf6ec66 100644 > >>> --- a/src/ipa/rkisp1/params.cpp > >>> +++ b/src/ipa/rkisp1/params.cpp > >>> @@ -128,7 +128,8 @@ Span<uint8_t> RkISP1Params::block(BlockType type) > >>> return data_.subspan(info.offset, info.size); > >>> } > >>> > >>> - return V4L2Params::block(type, info.type, info.size); > >>> + return V4L2Params::block(utils::to_underlying(type), info.type, > >>> + info.size); > >> > >> Does this warrant a line break? It's the same length as the reinterpret_cast > >> a couple lines above. > >> > >> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> > >> > >>> } > >>> > >>> } /* namespace ipa::rkisp1 */ > >>> diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h > >>> index eddb37d5c000..8e3672ca8340 100644 > >>> --- a/src/ipa/rkisp1/params.h > >>> +++ b/src/ipa/rkisp1/params.h > >>> @@ -7,6 +7,8 @@ > >>> > >>> #pragma once > >>> > >>> +#include <stdint.h> > >>> + > >>> #include <linux/rkisp1-config.h> > >>> #include <linux/videodev2.h> > >>> > >>> @@ -16,7 +18,7 @@ namespace libcamera { > >>> > >>> namespace ipa::rkisp1 { > >>> > >>> -enum class BlockType { > >>> +enum class BlockType : uint16_t { > >>> Bls, > >>> Dpcc, > >>> Sdg, > >>> -- > >>> Regards, > >>> > >>> Laurent Pinchart > >>> > >> >
On Fri, May 08, 2026 at 10:02:59AM +0100, Kieran Bingham wrote: > Quoting Barnabás Pőcze (2026-05-08 09:35:26) > > 2026. 05. 07. 23:37 keltezéssel, Laurent Pinchart írta: > > > The Linux kernel V4L2 extensible ISP parameters API uses a 16-bit > > > integer for the block type. We can therefore standardize on the same > > > type for the underlying type of the Traits::id_type enum class, as there > > > will never be more block types in libcamera than in the kernel. This > > > will help sharing code between different specializations of V4L2Params. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/ipa/libipa/v4l2_params.cpp | 2 +- > > > src/ipa/libipa/v4l2_params.h | 11 +++++++---- > > > src/ipa/mali-c55/params.h | 4 +++- > > > src/ipa/rkisp1/params.cpp | 3 ++- > > > src/ipa/rkisp1/params.h | 4 +++- > > > 5 files changed, 16 insertions(+), 8 deletions(-) > > > > > > diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp > > > index c2971f1caf28..d44a366a60b8 100644 > > > --- a/src/ipa/libipa/v4l2_params.cpp > > > +++ b/src/ipa/libipa/v4l2_params.cpp > > > @@ -217,7 +217,7 @@ namespace ipa { > > > */ > > > > > > /** > > > - * \fn V4L2Params::block(typename Traits::id_type type, unsigned int blockType, size_t blockSize) > > > + * \fn V4L2Params::block(uint16_t type, unsigned int blockType, size_t blockSize) > > > * \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 > > > diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h > > > index f400b37d74b5..4f84360ee449 100644 > > > --- a/src/ipa/libipa/v4l2_params.h > > > +++ b/src/ipa/libipa/v4l2_params.h > > > @@ -15,6 +15,7 @@ > > > > > > #include <libcamera/base/log.h> > > > #include <libcamera/base/span.h> > > > +#include <libcamera/base/utils.h> > > > > > > namespace libcamera { > > > > > > @@ -72,6 +73,8 @@ template<typename Traits> > > > class V4L2Params > > > { > > > public: > > > + static_assert(std::is_same_v<std::underlying_type_t<typename Traits::id_type>, uint16_t>); > > > + > > Surely with this ^ > > > > V4L2Params(Span<uint8_t> data, unsigned int version) > > > : data_(data) > > > { > > > @@ -93,13 +96,13 @@ public: > > > using Type = typename Details::type; > > > constexpr auto kernelId = Details::blockType; > > > > > > - auto data = block(Id, kernelId, sizeof(Type)); > > > + auto data = block(utils::to_underlying(Id), kernelId, sizeof(Type)); > > > return V4L2ParamsBlock<Type>(data); > > > } > > > > > > protected: > > > - Span<uint8_t> block(typename Traits::id_type type, > > > - unsigned int blockType, size_t blockSize) > > > + Span<uint8_t> block(uint16_t type, unsigned int blockType, > > > + size_t blockSize) > > we don't need to change this > > > > { > > > /* > > > * Look up the block in the cache first. If an algorithm > > > @@ -144,7 +147,7 @@ protected: > > > Span<uint8_t> data_; > > > size_t used_; > > > > > > - std::map<typename Traits::id_type, Span<uint8_t>> blocks_; > > > + std::map<uint16_t, Span<uint8_t>> blocks_; > > > > or this, > > > A bit unfortunate that now debuggers won't show the keys nicely, but oh well. > > And then this would still work ? Those two changes are needed as both blocks_ and block() are moved to the base non-template class in the next patch. I could drop the changes here and move them to 3/3. I don't think that's what you have in mind though :-) > > > }; > > > > > > } /* namespace ipa */ > > > diff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h > > > index 64be68583ddb..3abcb7f94916 100644 > > > --- a/src/ipa/mali-c55/params.h > > > +++ b/src/ipa/mali-c55/params.h > > > @@ -7,6 +7,8 @@ > > > > > > #pragma once > > > > > > +#include <stdint.h> > > > + > > > #include <linux/media/arm/mali-c55-config.h> > > > #include <linux/videodev2.h> > > > > > > @@ -16,7 +18,7 @@ namespace libcamera { > > > > > > namespace ipa::mali_c55 { > > > > > > -enum class MaliC55Blocks { > > > +enum class MaliC55Blocks : uint16_t { > > > Bls, > > > AexpHist, > > > AexpHistWeights, > > > diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp > > > index 7040207c2655..b8abbdf6ec66 100644 > > > --- a/src/ipa/rkisp1/params.cpp > > > +++ b/src/ipa/rkisp1/params.cpp > > > @@ -128,7 +128,8 @@ Span<uint8_t> RkISP1Params::block(BlockType type) > > > return data_.subspan(info.offset, info.size); > > > } > > > > > > - return V4L2Params::block(type, info.type, info.size); > > > + return V4L2Params::block(utils::to_underlying(type), info.type, > > > + info.size); > > > > Does this warrant a line break? It's the same length as the reinterpret_cast > > a couple lines above. I could avoid it here, but it would come back in the nest patch as the line becomes longer there. > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > > } > > > > > > } /* namespace ipa::rkisp1 */ > > > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h > > > index eddb37d5c000..8e3672ca8340 100644 > > > --- a/src/ipa/rkisp1/params.h > > > +++ b/src/ipa/rkisp1/params.h > > > @@ -7,6 +7,8 @@ > > > > > > #pragma once > > > > > > +#include <stdint.h> > > > + > > > #include <linux/rkisp1-config.h> > > > #include <linux/videodev2.h> > > > > > > @@ -16,7 +18,7 @@ namespace libcamera { > > > > > > namespace ipa::rkisp1 { > > > > > > -enum class BlockType { > > > +enum class BlockType : uint16_t { > > > Bls, > > > Dpcc, > > > Sdg,
diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp index c2971f1caf28..d44a366a60b8 100644 --- a/src/ipa/libipa/v4l2_params.cpp +++ b/src/ipa/libipa/v4l2_params.cpp @@ -217,7 +217,7 @@ namespace ipa { */ /** - * \fn V4L2Params::block(typename Traits::id_type type, unsigned int blockType, size_t blockSize) + * \fn V4L2Params::block(uint16_t type, unsigned int blockType, size_t blockSize) * \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 diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h index f400b37d74b5..4f84360ee449 100644 --- a/src/ipa/libipa/v4l2_params.h +++ b/src/ipa/libipa/v4l2_params.h @@ -15,6 +15,7 @@ #include <libcamera/base/log.h> #include <libcamera/base/span.h> +#include <libcamera/base/utils.h> namespace libcamera { @@ -72,6 +73,8 @@ template<typename Traits> class V4L2Params { public: + static_assert(std::is_same_v<std::underlying_type_t<typename Traits::id_type>, uint16_t>); + V4L2Params(Span<uint8_t> data, unsigned int version) : data_(data) { @@ -93,13 +96,13 @@ public: using Type = typename Details::type; constexpr auto kernelId = Details::blockType; - auto data = block(Id, kernelId, sizeof(Type)); + auto data = block(utils::to_underlying(Id), kernelId, sizeof(Type)); return V4L2ParamsBlock<Type>(data); } protected: - Span<uint8_t> block(typename Traits::id_type type, - unsigned int blockType, size_t blockSize) + Span<uint8_t> block(uint16_t type, unsigned int blockType, + size_t blockSize) { /* * Look up the block in the cache first. If an algorithm @@ -144,7 +147,7 @@ protected: Span<uint8_t> data_; size_t used_; - std::map<typename Traits::id_type, Span<uint8_t>> blocks_; + std::map<uint16_t, Span<uint8_t>> blocks_; }; } /* namespace ipa */ diff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h index 64be68583ddb..3abcb7f94916 100644 --- a/src/ipa/mali-c55/params.h +++ b/src/ipa/mali-c55/params.h @@ -7,6 +7,8 @@ #pragma once +#include <stdint.h> + #include <linux/media/arm/mali-c55-config.h> #include <linux/videodev2.h> @@ -16,7 +18,7 @@ namespace libcamera { namespace ipa::mali_c55 { -enum class MaliC55Blocks { +enum class MaliC55Blocks : uint16_t { Bls, AexpHist, AexpHistWeights, diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp index 7040207c2655..b8abbdf6ec66 100644 --- a/src/ipa/rkisp1/params.cpp +++ b/src/ipa/rkisp1/params.cpp @@ -128,7 +128,8 @@ Span<uint8_t> RkISP1Params::block(BlockType type) return data_.subspan(info.offset, info.size); } - return V4L2Params::block(type, info.type, info.size); + return V4L2Params::block(utils::to_underlying(type), info.type, + info.size); } } /* namespace ipa::rkisp1 */ diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h index eddb37d5c000..8e3672ca8340 100644 --- a/src/ipa/rkisp1/params.h +++ b/src/ipa/rkisp1/params.h @@ -7,6 +7,8 @@ #pragma once +#include <stdint.h> + #include <linux/rkisp1-config.h> #include <linux/videodev2.h> @@ -16,7 +18,7 @@ namespace libcamera { namespace ipa::rkisp1 { -enum class BlockType { +enum class BlockType : uint16_t { Bls, Dpcc, Sdg,
The Linux kernel V4L2 extensible ISP parameters API uses a 16-bit integer for the block type. We can therefore standardize on the same type for the underlying type of the Traits::id_type enum class, as there will never be more block types in libcamera than in the kernel. This will help sharing code between different specializations of V4L2Params. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/ipa/libipa/v4l2_params.cpp | 2 +- src/ipa/libipa/v4l2_params.h | 11 +++++++---- src/ipa/mali-c55/params.h | 4 +++- src/ipa/rkisp1/params.cpp | 3 ++- src/ipa/rkisp1/params.h | 4 +++- 5 files changed, 16 insertions(+), 8 deletions(-)