| Message ID | 20251114005428.90024-10-kieran.bingham@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta: > Provide a FixedPoint Quantized type to support the Q4.8 format > representing values in the range [0 .. 15.9961] with a resolution of > 1/256. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > This type is used by the Mali-C55 AWB Gains > > src/ipa/libipa/fixedpoint.cpp | 9 +++++++++ > src/ipa/libipa/fixedpoint.h | 2 ++ > test/ipa/libipa/fixedpoint.cpp | 5 +++++ > 3 files changed, 16 insertions(+) > > diff --git a/src/ipa/libipa/fixedpoint.cpp b/src/ipa/libipa/fixedpoint.cpp > index a6f8ef351c32..03053668ed79 100644 > --- a/src/ipa/libipa/fixedpoint.cpp > +++ b/src/ipa/libipa/fixedpoint.cpp > @@ -139,6 +139,15 @@ namespace ipa { > * values in the range [0.0, 1.992] with a resolution of 1/128. > */ > > +/** > + * \typedef UQ4_8 > + * \brief 4.8 unsigned fixed-point quantizer > + * > + * A Quantized type using 4 bits for the integer part and 8 bits for the > + * fractional part, stored in an unsigned 16-bit integer (\c uint16_t). Represents > + * values in the range [0.0, 15.9961] with a resolution of 1/256. > + */ > + > /** > * \typedef Q5_4 > * \brief 5.4 signed fixed-point quantizer > diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h > index 8f095210a918..05dd97e64f40 100644 > --- a/src/ipa/libipa/fixedpoint.h > +++ b/src/ipa/libipa/fixedpoint.h > @@ -105,6 +105,8 @@ struct FixedPointQTraits { > using Q1_7 = Quantized<FixedPointQTraits<1, 7, int8_t>>; > using UQ1_7 = Quantized<FixedPointQTraits<1, 7, uint8_t>>; > > +using UQ4_8 = Quantized<FixedPointQTraits<4, 8, uint16_t>>; > + > using Q5_4 = Quantized<FixedPointQTraits<5, 4, int16_t>>; > using UQ5_8 = Quantized<FixedPointQTraits<5, 8, uint16_t>>; I'm wondering if we might want to add a type alias template instead of adding each manually. E.g. something along these lines: namespace fixp { namespace details { template<unsigned int Bits> constexpr auto qtype() { static_assert(Bits <= 64); if constexpr (Bits <= 8) return int8_t(); else if constexpr (Bits <= 16) return int16_t(); else if constexpr (Bits <= 32) return int32_t(); else if constexpr (Bits <= 64) return int64_t(); } } template<unsigned int I, unsigned int F> using Q = Quantized<FixedPointQTraits<I, F, decltype(details::qtype<I + F>())>>; template<unsigned int I, unsigned int F> using UQ = Quantized<FixedPointQTraits<I, F, std::make_unsigned_t<decltype(details::qtype<I + F>())>>>; } Regards, Barnabás Pőcze > > diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp > index c830b5f05a19..f3773dfe6ad4 100644 > --- a/test/ipa/libipa/fixedpoint.cpp > +++ b/test/ipa/libipa/fixedpoint.cpp > @@ -190,6 +190,11 @@ protected: > fails += quantizedCheck<Q4_7>(-0.4f, 0b1111'1001101, -0.398438f); /* 0x7cd */ > fails += quantizedCheck<Q4_7>(-1.4f, 0b1110'1001101, -1.39844f); /* 0x74d */ > > + /* UQ4_8(0 .. 15.9961) Min: [0x0000:0] -- Max: [0x0fff:15.9961] Step:0.00390625 */ > + introduce<UQ4_8>("UQ4_8"); > + fails += quantizedCheck<UQ4_8>( 0.0f, 0b0000'00000000, 0.00f); > + fails += quantizedCheck<UQ4_8>(16.0f, 0b1111'11111111, 15.9961f); > + > /* Q5_4(-16 .. 15.9375) Min: [0x0100:-16] -- Max: [0x00ff:15.9375] Step:0.0625 */ > introduce<Q5_4>("Q5_4"); > fails += quantizedCheck<Q5_4>(-16.00f, 0b10000'0000, -16.00f);
Quoting Barnabás Pőcze (2025-11-17 11:56:52) > Hi > > 2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta: > > Provide a FixedPoint Quantized type to support the Q4.8 format > > representing values in the range [0 .. 15.9961] with a resolution of > > 1/256. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > --- > > This type is used by the Mali-C55 AWB Gains > > > > src/ipa/libipa/fixedpoint.cpp | 9 +++++++++ > > src/ipa/libipa/fixedpoint.h | 2 ++ > > test/ipa/libipa/fixedpoint.cpp | 5 +++++ > > 3 files changed, 16 insertions(+) > > > > diff --git a/src/ipa/libipa/fixedpoint.cpp b/src/ipa/libipa/fixedpoint.cpp > > index a6f8ef351c32..03053668ed79 100644 > > --- a/src/ipa/libipa/fixedpoint.cpp > > +++ b/src/ipa/libipa/fixedpoint.cpp > > @@ -139,6 +139,15 @@ namespace ipa { > > * values in the range [0.0, 1.992] with a resolution of 1/128. > > */ > > > > +/** > > + * \typedef UQ4_8 > > + * \brief 4.8 unsigned fixed-point quantizer > > + * > > + * A Quantized type using 4 bits for the integer part and 8 bits for the > > + * fractional part, stored in an unsigned 16-bit integer (\c uint16_t). Represents > > + * values in the range [0.0, 15.9961] with a resolution of 1/256. > > + */ > > + > > /** > > * \typedef Q5_4 > > * \brief 5.4 signed fixed-point quantizer > > diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h > > index 8f095210a918..05dd97e64f40 100644 > > --- a/src/ipa/libipa/fixedpoint.h > > +++ b/src/ipa/libipa/fixedpoint.h > > @@ -105,6 +105,8 @@ struct FixedPointQTraits { > > using Q1_7 = Quantized<FixedPointQTraits<1, 7, int8_t>>; > > using UQ1_7 = Quantized<FixedPointQTraits<1, 7, uint8_t>>; > > > > +using UQ4_8 = Quantized<FixedPointQTraits<4, 8, uint16_t>>; > > + > > using Q5_4 = Quantized<FixedPointQTraits<5, 4, int16_t>>; > > using UQ5_8 = Quantized<FixedPointQTraits<5, 8, uint16_t>>; > > I'm wondering if we might want to add a type alias template instead of adding > each manually. E.g. something along these lines: > > namespace fixp { > > namespace details { > > template<unsigned int Bits> > constexpr auto qtype() > { > static_assert(Bits <= 64); > > if constexpr (Bits <= 8) return int8_t(); > else if constexpr (Bits <= 16) return int16_t(); > else if constexpr (Bits <= 32) return int32_t(); > else if constexpr (Bits <= 64) return int64_t(); > } > > } > > template<unsigned int I, unsigned int F> > using Q = Quantized<FixedPointQTraits<I, F, decltype(details::qtype<I + F>())>>; > > template<unsigned int I, unsigned int F> > using UQ = Quantized<FixedPointQTraits<I, F, std::make_unsigned_t<decltype(details::qtype<I + F>())>>>; > > } > This looks like magic, but I like it ;-) I guess that means we then do: using Q5_8 = Q<5, 8>; using UQ5_8 = UQ<5, 8>; Which gets the down to being really elegant, and we can even use that inline in code as just Q<4,4>(2.25f); if we wish. Very cool addition. > > Regards, > Barnabás Pőcze > > > > > > diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp > > index c830b5f05a19..f3773dfe6ad4 100644 > > --- a/test/ipa/libipa/fixedpoint.cpp > > +++ b/test/ipa/libipa/fixedpoint.cpp > > @@ -190,6 +190,11 @@ protected: > > fails += quantizedCheck<Q4_7>(-0.4f, 0b1111'1001101, -0.398438f); /* 0x7cd */ > > fails += quantizedCheck<Q4_7>(-1.4f, 0b1110'1001101, -1.39844f); /* 0x74d */ > > > > + /* UQ4_8(0 .. 15.9961) Min: [0x0000:0] -- Max: [0x0fff:15.9961] Step:0.00390625 */ > > + introduce<UQ4_8>("UQ4_8"); > > + fails += quantizedCheck<UQ4_8>( 0.0f, 0b0000'00000000, 0.00f); > > + fails += quantizedCheck<UQ4_8>(16.0f, 0b1111'11111111, 15.9961f); > > + > > /* Q5_4(-16 .. 15.9375) Min: [0x0100:-16] -- Max: [0x00ff:15.9375] Step:0.0625 */ > > introduce<Q5_4>("Q5_4"); > > fails += quantizedCheck<Q5_4>(-16.00f, 0b10000'0000, -16.00f); >
Hi Kieran, Thank you for the patch! Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com> Quoting Kieran Bingham (2025-11-14 00:54:13) > Provide a FixedPoint Quantized type to support the Q4.8 format > representing values in the range [0 .. 15.9961] with a resolution of > 1/256. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > This type is used by the Mali-C55 AWB Gains > > src/ipa/libipa/fixedpoint.cpp | 9 +++++++++ > src/ipa/libipa/fixedpoint.h | 2 ++ > test/ipa/libipa/fixedpoint.cpp | 5 +++++ > 3 files changed, 16 insertions(+) > > diff --git a/src/ipa/libipa/fixedpoint.cpp b/src/ipa/libipa/fixedpoint.cpp > index a6f8ef351c32..03053668ed79 100644 > --- a/src/ipa/libipa/fixedpoint.cpp > +++ b/src/ipa/libipa/fixedpoint.cpp > @@ -139,6 +139,15 @@ namespace ipa { > * values in the range [0.0, 1.992] with a resolution of 1/128. > */ > > +/** > + * \typedef UQ4_8 > + * \brief 4.8 unsigned fixed-point quantizer > + * > + * A Quantized type using 4 bits for the integer part and 8 bits for the > + * fractional part, stored in an unsigned 16-bit integer (\c uint16_t). Represents > + * values in the range [0.0, 15.9961] with a resolution of 1/256. > + */ > + > /** > * \typedef Q5_4 > * \brief 5.4 signed fixed-point quantizer > diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h > index 8f095210a918..05dd97e64f40 100644 > --- a/src/ipa/libipa/fixedpoint.h > +++ b/src/ipa/libipa/fixedpoint.h > @@ -105,6 +105,8 @@ struct FixedPointQTraits { > using Q1_7 = Quantized<FixedPointQTraits<1, 7, int8_t>>; > using UQ1_7 = Quantized<FixedPointQTraits<1, 7, uint8_t>>; > > +using UQ4_8 = Quantized<FixedPointQTraits<4, 8, uint16_t>>; > + > using Q5_4 = Quantized<FixedPointQTraits<5, 4, int16_t>>; > using UQ5_8 = Quantized<FixedPointQTraits<5, 8, uint16_t>>; > > diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp > index c830b5f05a19..f3773dfe6ad4 100644 > --- a/test/ipa/libipa/fixedpoint.cpp > +++ b/test/ipa/libipa/fixedpoint.cpp > @@ -190,6 +190,11 @@ protected: > fails += quantizedCheck<Q4_7>(-0.4f, 0b1111'1001101, -0.398438f); /* 0x7cd */ > fails += quantizedCheck<Q4_7>(-1.4f, 0b1110'1001101, -1.39844f); /* 0x74d */ > > + /* UQ4_8(0 .. 15.9961) Min: [0x0000:0] -- Max: [0x0fff:15.9961] Step:0.00390625 */ > + introduce<UQ4_8>("UQ4_8"); > + fails += quantizedCheck<UQ4_8>( 0.0f, 0b0000'00000000, 0.00f); > + fails += quantizedCheck<UQ4_8>(16.0f, 0b1111'11111111, 15.9961f); > + > /* Q5_4(-16 .. 15.9375) Min: [0x0100:-16] -- Max: [0x00ff:15.9375] Step:0.0625 */ > introduce<Q5_4>("Q5_4"); > fails += quantizedCheck<Q5_4>(-16.00f, 0b10000'0000, -16.00f); > -- > 2.51.1 >
On Mon, Nov 17, 2025 at 12:11:31PM +0000, Kieran Bingham wrote: > Quoting Barnabás Pőcze (2025-11-17 11:56:52) > > 2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta: > > > Provide a FixedPoint Quantized type to support the Q4.8 format > > > representing values in the range [0 .. 15.9961] with a resolution of > > > 1/256. > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > --- > > > This type is used by the Mali-C55 AWB Gains > > > > > > src/ipa/libipa/fixedpoint.cpp | 9 +++++++++ > > > src/ipa/libipa/fixedpoint.h | 2 ++ > > > test/ipa/libipa/fixedpoint.cpp | 5 +++++ > > > 3 files changed, 16 insertions(+) > > > > > > diff --git a/src/ipa/libipa/fixedpoint.cpp b/src/ipa/libipa/fixedpoint.cpp > > > index a6f8ef351c32..03053668ed79 100644 > > > --- a/src/ipa/libipa/fixedpoint.cpp > > > +++ b/src/ipa/libipa/fixedpoint.cpp > > > @@ -139,6 +139,15 @@ namespace ipa { > > > * values in the range [0.0, 1.992] with a resolution of 1/128. > > > */ > > > > > > +/** > > > + * \typedef UQ4_8 > > > + * \brief 4.8 unsigned fixed-point quantizer > > > + * > > > + * A Quantized type using 4 bits for the integer part and 8 bits for the > > > + * fractional part, stored in an unsigned 16-bit integer (\c uint16_t). Represents > > > + * values in the range [0.0, 15.9961] with a resolution of 1/256. > > > + */ > > > + > > > /** > > > * \typedef Q5_4 > > > * \brief 5.4 signed fixed-point quantizer > > > diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h > > > index 8f095210a918..05dd97e64f40 100644 > > > --- a/src/ipa/libipa/fixedpoint.h > > > +++ b/src/ipa/libipa/fixedpoint.h > > > @@ -105,6 +105,8 @@ struct FixedPointQTraits { > > > using Q1_7 = Quantized<FixedPointQTraits<1, 7, int8_t>>; > > > using UQ1_7 = Quantized<FixedPointQTraits<1, 7, uint8_t>>; > > > > > > +using UQ4_8 = Quantized<FixedPointQTraits<4, 8, uint16_t>>; > > > + > > > using Q5_4 = Quantized<FixedPointQTraits<5, 4, int16_t>>; > > > using UQ5_8 = Quantized<FixedPointQTraits<5, 8, uint16_t>>; > > > > I'm wondering if we might want to add a type alias template instead of adding > > each manually. E.g. something along these lines: > > > > namespace fixp { > > > > namespace details { > > > > template<unsigned int Bits> > > constexpr auto qtype() > > { > > static_assert(Bits <= 64); > > > > if constexpr (Bits <= 8) return int8_t(); > > else if constexpr (Bits <= 16) return int16_t(); > > else if constexpr (Bits <= 32) return int32_t(); > > else if constexpr (Bits <= 64) return int64_t(); > > } > > > > } > > > > template<unsigned int I, unsigned int F> > > using Q = Quantized<FixedPointQTraits<I, F, decltype(details::qtype<I + F>())>>; > > > > template<unsigned int I, unsigned int F> > > using UQ = Quantized<FixedPointQTraits<I, F, std::make_unsigned_t<decltype(details::qtype<I + F>())>>>; > > > > } I was going to propose something similar as I went through the patches and got increasingly bothered by the proliferation of types :-) > > > > This looks like magic, but I like it ;-) > > I guess that means we then do: > > using Q5_8 = Q<5, 8>; > using UQ5_8 = UQ<5, 8>; > > Which gets the down to being really elegant, and we can even use that > inline in code as just Q<4,4>(2.25f); if we wish. I'd use Q<4, 4> only, without a Q4_4 type alias. Or if you want to save three characters, I'd add the type aliases where used, not in fixedpoint.h. > Very cool addition. > > > > diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp > > > index c830b5f05a19..f3773dfe6ad4 100644 > > > --- a/test/ipa/libipa/fixedpoint.cpp > > > +++ b/test/ipa/libipa/fixedpoint.cpp > > > @@ -190,6 +190,11 @@ protected: > > > fails += quantizedCheck<Q4_7>(-0.4f, 0b1111'1001101, -0.398438f); /* 0x7cd */ > > > fails += quantizedCheck<Q4_7>(-1.4f, 0b1110'1001101, -1.39844f); /* 0x74d */ > > > > > > + /* UQ4_8(0 .. 15.9961) Min: [0x0000:0] -- Max: [0x0fff:15.9961] Step:0.00390625 */ > > > + introduce<UQ4_8>("UQ4_8"); > > > + fails += quantizedCheck<UQ4_8>( 0.0f, 0b0000'00000000, 0.00f); > > > + fails += quantizedCheck<UQ4_8>(16.0f, 0b1111'11111111, 15.9961f); Is there a way we could also generalize the tests instead of open-coding them ? That could be done later too if it's too complicated. > > > + > > > /* Q5_4(-16 .. 15.9375) Min: [0x0100:-16] -- Max: [0x00ff:15.9375] Step:0.0625 */ > > > introduce<Q5_4>("Q5_4"); > > > fails += quantizedCheck<Q5_4>(-16.00f, 0b10000'0000, -16.00f); > >
Quoting Laurent Pinchart (2025-11-19 04:31:15) > On Mon, Nov 17, 2025 at 12:11:31PM +0000, Kieran Bingham wrote: > > Quoting Barnabás Pőcze (2025-11-17 11:56:52) > > > 2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta: > > > > Provide a FixedPoint Quantized type to support the Q4.8 format > > > > representing values in the range [0 .. 15.9961] with a resolution of > > > > 1/256. > > > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > --- > > > > This type is used by the Mali-C55 AWB Gains > > > > > > > > src/ipa/libipa/fixedpoint.cpp | 9 +++++++++ > > > > src/ipa/libipa/fixedpoint.h | 2 ++ > > > > test/ipa/libipa/fixedpoint.cpp | 5 +++++ > > > > 3 files changed, 16 insertions(+) > > > > > > > > diff --git a/src/ipa/libipa/fixedpoint.cpp b/src/ipa/libipa/fixedpoint.cpp > > > > index a6f8ef351c32..03053668ed79 100644 > > > > --- a/src/ipa/libipa/fixedpoint.cpp > > > > +++ b/src/ipa/libipa/fixedpoint.cpp > > > > @@ -139,6 +139,15 @@ namespace ipa { > > > > * values in the range [0.0, 1.992] with a resolution of 1/128. > > > > */ > > > > > > > > +/** > > > > + * \typedef UQ4_8 > > > > + * \brief 4.8 unsigned fixed-point quantizer > > > > + * > > > > + * A Quantized type using 4 bits for the integer part and 8 bits for the > > > > + * fractional part, stored in an unsigned 16-bit integer (\c uint16_t). Represents > > > > + * values in the range [0.0, 15.9961] with a resolution of 1/256. > > > > + */ > > > > + > > > > /** > > > > * \typedef Q5_4 > > > > * \brief 5.4 signed fixed-point quantizer > > > > diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h > > > > index 8f095210a918..05dd97e64f40 100644 > > > > --- a/src/ipa/libipa/fixedpoint.h > > > > +++ b/src/ipa/libipa/fixedpoint.h > > > > @@ -105,6 +105,8 @@ struct FixedPointQTraits { > > > > using Q1_7 = Quantized<FixedPointQTraits<1, 7, int8_t>>; > > > > using UQ1_7 = Quantized<FixedPointQTraits<1, 7, uint8_t>>; > > > > > > > > +using UQ4_8 = Quantized<FixedPointQTraits<4, 8, uint16_t>>; > > > > + > > > > using Q5_4 = Quantized<FixedPointQTraits<5, 4, int16_t>>; > > > > using UQ5_8 = Quantized<FixedPointQTraits<5, 8, uint16_t>>; > > > > > > I'm wondering if we might want to add a type alias template instead of adding > > > each manually. E.g. something along these lines: > > > > > > namespace fixp { > > > > > > namespace details { > > > > > > template<unsigned int Bits> > > > constexpr auto qtype() > > > { > > > static_assert(Bits <= 64); > > > > > > if constexpr (Bits <= 8) return int8_t(); > > > else if constexpr (Bits <= 16) return int16_t(); > > > else if constexpr (Bits <= 32) return int32_t(); > > > else if constexpr (Bits <= 64) return int64_t(); > > > } > > > > > > } > > > > > > template<unsigned int I, unsigned int F> > > > using Q = Quantized<FixedPointQTraits<I, F, decltype(details::qtype<I + F>())>>; > > > > > > template<unsigned int I, unsigned int F> > > > using UQ = Quantized<FixedPointQTraits<I, F, std::make_unsigned_t<decltype(details::qtype<I + F>())>>>; > > > > > > } > > I was going to propose something similar as I went through the patches > and got increasingly bothered by the proliferation of types :-) So - I actaully /really/ like the addition of types. As a developer - especially when we come to adding / referencing a new fixed point type used by hardware - I would be really happy to be able to reference the documentation which states the full range of the type, and the step size ... which is why I've added all of that explicitly! I don't think it's a /requirement/ to add a type - but anywhere it's used in more places or is a common fixed point type I think it could be useful. But as you see later (ipa: rkisp1: ccm: Use Q4_7 format directly), when there is only a single line usage of the type I kept the Q local. To me, This: \typedef UQ4_8 \brief 4.8 unsigned fixed-point quantizer A Quantized type using 4 bits for the integer part and 8 bits for the fractional part, stored in an unsigned 16-bit integer (\c uint16_t). Represents values in the range [0.0, 15.9961] with a resolution of 1/256. is part of the value add for our documentation! > > This looks like magic, but I like it ;-) > > > > I guess that means we then do: > > > > using Q5_8 = Q<5, 8>; > > using UQ5_8 = UQ<5, 8>; > > > > Which gets the down to being really elegant, and we can even use that > > inline in code as just Q<4,4>(2.25f); if we wish. > > I'd use Q<4, 4> only, without a Q4_4 type alias. Or if you want to save > three characters, I'd add the type aliases where used, not in > fixedpoint.h. > > > Very cool addition. > > > > > > diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp > > > > index c830b5f05a19..f3773dfe6ad4 100644 > > > > --- a/test/ipa/libipa/fixedpoint.cpp > > > > +++ b/test/ipa/libipa/fixedpoint.cpp > > > > @@ -190,6 +190,11 @@ protected: > > > > fails += quantizedCheck<Q4_7>(-0.4f, 0b1111'1001101, -0.398438f); /* 0x7cd */ > > > > fails += quantizedCheck<Q4_7>(-1.4f, 0b1110'1001101, -1.39844f); /* 0x74d */ > > > > > > > > + /* UQ4_8(0 .. 15.9961) Min: [0x0000:0] -- Max: [0x0fff:15.9961] Step:0.00390625 */ > > > > + introduce<UQ4_8>("UQ4_8"); > > > > + fails += quantizedCheck<UQ4_8>( 0.0f, 0b0000'00000000, 0.00f); > > > > + fails += quantizedCheck<UQ4_8>(16.0f, 0b1111'11111111, 15.9961f); > > Is there a way we could also generalize the tests instead of open-coding > them ? That could be done later too if it's too complicated. Maybe - but these tests have been vital *for me* to make sure this was all correct. They pop up as soon as I break anything when I make a typo in all of the refactoring here. This process has really been test-driven-development based around guarnateeing that these checks return exactly as they are expected ;-) In other words - I have found having these open coded to be far more valuable than any generic. If it was generic I'd then want extra tests to make sure the generic was doing what I expected :D - these are really clear to me. > > > > + > > > > /* Q5_4(-16 .. 15.9375) Min: [0x0100:-16] -- Max: [0x00ff:15.9375] Step:0.0625 */ > > > > introduce<Q5_4>("Q5_4"); > > > > fails += quantizedCheck<Q5_4>(-16.00f, 0b10000'0000, -16.00f); > > > > > -- > Regards, > > Laurent Pinchart
On Wed, Nov 19, 2025 at 09:29:06AM +0000, Kieran Bingham wrote: > Quoting Laurent Pinchart (2025-11-19 04:31:15) > > On Mon, Nov 17, 2025 at 12:11:31PM +0000, Kieran Bingham wrote: > > > Quoting Barnabás Pőcze (2025-11-17 11:56:52) > > > > 2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta: > > > > > Provide a FixedPoint Quantized type to support the Q4.8 format > > > > > representing values in the range [0 .. 15.9961] with a resolution of > > > > > 1/256. > > > > > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > --- > > > > > This type is used by the Mali-C55 AWB Gains > > > > > > > > > > src/ipa/libipa/fixedpoint.cpp | 9 +++++++++ > > > > > src/ipa/libipa/fixedpoint.h | 2 ++ > > > > > test/ipa/libipa/fixedpoint.cpp | 5 +++++ > > > > > 3 files changed, 16 insertions(+) > > > > > > > > > > diff --git a/src/ipa/libipa/fixedpoint.cpp b/src/ipa/libipa/fixedpoint.cpp > > > > > index a6f8ef351c32..03053668ed79 100644 > > > > > --- a/src/ipa/libipa/fixedpoint.cpp > > > > > +++ b/src/ipa/libipa/fixedpoint.cpp > > > > > @@ -139,6 +139,15 @@ namespace ipa { > > > > > * values in the range [0.0, 1.992] with a resolution of 1/128. > > > > > */ > > > > > > > > > > +/** > > > > > + * \typedef UQ4_8 > > > > > + * \brief 4.8 unsigned fixed-point quantizer > > > > > + * > > > > > + * A Quantized type using 4 bits for the integer part and 8 bits for the > > > > > + * fractional part, stored in an unsigned 16-bit integer (\c uint16_t). Represents > > > > > + * values in the range [0.0, 15.9961] with a resolution of 1/256. > > > > > + */ > > > > > + > > > > > /** > > > > > * \typedef Q5_4 > > > > > * \brief 5.4 signed fixed-point quantizer > > > > > diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h > > > > > index 8f095210a918..05dd97e64f40 100644 > > > > > --- a/src/ipa/libipa/fixedpoint.h > > > > > +++ b/src/ipa/libipa/fixedpoint.h > > > > > @@ -105,6 +105,8 @@ struct FixedPointQTraits { > > > > > using Q1_7 = Quantized<FixedPointQTraits<1, 7, int8_t>>; > > > > > using UQ1_7 = Quantized<FixedPointQTraits<1, 7, uint8_t>>; > > > > > > > > > > +using UQ4_8 = Quantized<FixedPointQTraits<4, 8, uint16_t>>; > > > > > + > > > > > using Q5_4 = Quantized<FixedPointQTraits<5, 4, int16_t>>; > > > > > using UQ5_8 = Quantized<FixedPointQTraits<5, 8, uint16_t>>; > > > > > > > > I'm wondering if we might want to add a type alias template instead of adding > > > > each manually. E.g. something along these lines: > > > > > > > > namespace fixp { > > > > > > > > namespace details { > > > > > > > > template<unsigned int Bits> > > > > constexpr auto qtype() > > > > { > > > > static_assert(Bits <= 64); > > > > > > > > if constexpr (Bits <= 8) return int8_t(); > > > > else if constexpr (Bits <= 16) return int16_t(); > > > > else if constexpr (Bits <= 32) return int32_t(); > > > > else if constexpr (Bits <= 64) return int64_t(); > > > > } > > > > > > > > } > > > > > > > > template<unsigned int I, unsigned int F> > > > > using Q = Quantized<FixedPointQTraits<I, F, decltype(details::qtype<I + F>())>>; > > > > > > > > template<unsigned int I, unsigned int F> > > > > using UQ = Quantized<FixedPointQTraits<I, F, std::make_unsigned_t<decltype(details::qtype<I + F>())>>>; > > > > > > > > } > > > > I was going to propose something similar as I went through the patches > > and got increasingly bothered by the proliferation of types :-) > > So - I actaully /really/ like the addition of types. As a developer - > especially when we come to adding / referencing a new fixed point type > used by hardware - I would be really happy to be able to reference the > documentation which states the full range of the type, and the step size > ... which is why I've added all of that explicitly! > > I don't think it's a /requirement/ to add a type - but anywhere it's > used in more places or is a common fixed point type I think it could be > useful. But as you see later (ipa: rkisp1: ccm: Use Q4_7 format > directly), when there is only a single line usage of the type I kept the > Q local. > > To me, This: > > \typedef UQ4_8 > \brief 4.8 unsigned fixed-point quantizer > > A Quantized type using 4 bits for the integer part and 8 bits for the > fractional part, stored in an unsigned 16-bit integer (\c uint16_t). Represents > values in the range [0.0, 15.9961] with a resolution of 1/256. > > is part of the value add for our documentation! The range and resolution are trivially derived from the fixed-point representation. I don't see much value in documenting dozens of fixed-point types with what will be copy & paste of the same text with small changes. > > > This looks like magic, but I like it ;-) > > > > > > I guess that means we then do: > > > > > > using Q5_8 = Q<5, 8>; > > > using UQ5_8 = UQ<5, 8>; > > > > > > Which gets the down to being really elegant, and we can even use that > > > inline in code as just Q<4,4>(2.25f); if we wish. > > > > I'd use Q<4, 4> only, without a Q4_4 type alias. Or if you want to save > > three characters, I'd add the type aliases where used, not in > > fixedpoint.h. > > > > > Very cool addition. > > > > > > > > diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp > > > > > index c830b5f05a19..f3773dfe6ad4 100644 > > > > > --- a/test/ipa/libipa/fixedpoint.cpp > > > > > +++ b/test/ipa/libipa/fixedpoint.cpp > > > > > @@ -190,6 +190,11 @@ protected: > > > > > fails += quantizedCheck<Q4_7>(-0.4f, 0b1111'1001101, -0.398438f); /* 0x7cd */ > > > > > fails += quantizedCheck<Q4_7>(-1.4f, 0b1110'1001101, -1.39844f); /* 0x74d */ > > > > > > > > > > + /* UQ4_8(0 .. 15.9961) Min: [0x0000:0] -- Max: [0x0fff:15.9961] Step:0.00390625 */ > > > > > + introduce<UQ4_8>("UQ4_8"); > > > > > + fails += quantizedCheck<UQ4_8>( 0.0f, 0b0000'00000000, 0.00f); > > > > > + fails += quantizedCheck<UQ4_8>(16.0f, 0b1111'11111111, 15.9961f); > > > > Is there a way we could also generalize the tests instead of open-coding > > them ? That could be done later too if it's too complicated. > > Maybe - but these tests have been vital *for me* to make sure this was > all correct. They pop up as soon as I break anything when I make a typo > in all of the refactoring here. > > This process has really been test-driven-development based around > guarnateeing that these checks return exactly as they are expected ;-) > > In other words - I have found having these open coded to be far more > valuable than any generic. If it was generic I'd then want extra tests > to make sure the generic was doing what I expected :D - these are really > clear to me. If the test simply copies the implementation of the fixed point conversion, then it will indeed not be very useful. > > > > > + > > > > > /* Q5_4(-16 .. 15.9375) Min: [0x0100:-16] -- Max: [0x00ff:15.9375] Step:0.0625 */ > > > > > introduce<Q5_4>("Q5_4"); > > > > > fails += quantizedCheck<Q5_4>(-16.00f, 0b10000'0000, -16.00f);
diff --git a/src/ipa/libipa/fixedpoint.cpp b/src/ipa/libipa/fixedpoint.cpp index a6f8ef351c32..03053668ed79 100644 --- a/src/ipa/libipa/fixedpoint.cpp +++ b/src/ipa/libipa/fixedpoint.cpp @@ -139,6 +139,15 @@ namespace ipa { * values in the range [0.0, 1.992] with a resolution of 1/128. */ +/** + * \typedef UQ4_8 + * \brief 4.8 unsigned fixed-point quantizer + * + * A Quantized type using 4 bits for the integer part and 8 bits for the + * fractional part, stored in an unsigned 16-bit integer (\c uint16_t). Represents + * values in the range [0.0, 15.9961] with a resolution of 1/256. + */ + /** * \typedef Q5_4 * \brief 5.4 signed fixed-point quantizer diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h index 8f095210a918..05dd97e64f40 100644 --- a/src/ipa/libipa/fixedpoint.h +++ b/src/ipa/libipa/fixedpoint.h @@ -105,6 +105,8 @@ struct FixedPointQTraits { using Q1_7 = Quantized<FixedPointQTraits<1, 7, int8_t>>; using UQ1_7 = Quantized<FixedPointQTraits<1, 7, uint8_t>>; +using UQ4_8 = Quantized<FixedPointQTraits<4, 8, uint16_t>>; + using Q5_4 = Quantized<FixedPointQTraits<5, 4, int16_t>>; using UQ5_8 = Quantized<FixedPointQTraits<5, 8, uint16_t>>; diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp index c830b5f05a19..f3773dfe6ad4 100644 --- a/test/ipa/libipa/fixedpoint.cpp +++ b/test/ipa/libipa/fixedpoint.cpp @@ -190,6 +190,11 @@ protected: fails += quantizedCheck<Q4_7>(-0.4f, 0b1111'1001101, -0.398438f); /* 0x7cd */ fails += quantizedCheck<Q4_7>(-1.4f, 0b1110'1001101, -1.39844f); /* 0x74d */ + /* UQ4_8(0 .. 15.9961) Min: [0x0000:0] -- Max: [0x0fff:15.9961] Step:0.00390625 */ + introduce<UQ4_8>("UQ4_8"); + fails += quantizedCheck<UQ4_8>( 0.0f, 0b0000'00000000, 0.00f); + fails += quantizedCheck<UQ4_8>(16.0f, 0b1111'11111111, 15.9961f); + /* Q5_4(-16 .. 15.9375) Min: [0x0100:-16] -- Max: [0x00ff:15.9375] Step:0.0625 */ introduce<Q5_4>("Q5_4"); fails += quantizedCheck<Q5_4>(-16.00f, 0b10000'0000, -16.00f);
Provide a FixedPoint Quantized type to support the Q4.8 format representing values in the range [0 .. 15.9961] with a resolution of 1/256. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- This type is used by the Mali-C55 AWB Gains src/ipa/libipa/fixedpoint.cpp | 9 +++++++++ src/ipa/libipa/fixedpoint.h | 2 ++ test/ipa/libipa/fixedpoint.cpp | 5 +++++ 3 files changed, 16 insertions(+)