| Message ID | 20251114005428.90024-6-kieran.bingham@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta: > The fixedToFloatingPoint does not support unsigned Q types, and > incorrectly sign-extends all values which have the top most bit set in > the quantized values. Are none of the previously added tests affected? > > Fix this by ensuring that only signed types perform sign extension, and > simplify the calcuation for unsigned types. ^^^^^^^^^^ calculation > > Convert the storage of the test cases to signed types to correctly > represent their intended purpose, to prevent test failures. I'm only seeing two uses of `fixedToFloatingPoint()` in total. Both are in `src/ipa/mali-c55/algorithms/awb.cpp`. Those use `uint16_t`; are they affected by this change? The documentation just says `Q4.8`, but I imagine they are unsigned actually. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/libipa/fixedpoint.h | 3 +++ > test/ipa/libipa/fixedpoint.cpp | 6 +++--- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h > index 17db8a026502..d97e6d6135ad 100644 > --- a/src/ipa/libipa/fixedpoint.h > +++ b/src/ipa/libipa/fixedpoint.h > @@ -51,6 +51,9 @@ constexpr R fixedToFloatingPoint(T number) > static_assert(sizeof(int) >= sizeof(T)); > static_assert(I + F <= sizeof(T) * 8); > > + if constexpr (std::is_unsigned_v<T>) > + return static_cast<R>(number) / static_cast<R>(1 << F); > + I'd add the rest to the `else` so as to avoid the "instantiation" of that part. > /* > * Recreate the upper bits in case of a negative number by shifting the sign > * bit from the fixed point to the first bit of the unsigned and then right shifting > diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp > index 935412771851..3f2d9ac97fe5 100644 > --- a/test/ipa/libipa/fixedpoint.cpp > +++ b/test/ipa/libipa/fixedpoint.cpp > @@ -70,7 +70,7 @@ protected: > * The second 7.992 test is to test that unused bits don't > * affect the result. > */ > - std::map<double, uint16_t> testCases = { > + std::map<double, int16_t> testCases = { > { 7.992, 0x3ff }, > { 0.2, 0x01a }, > { -0.2, 0x7e6 }, > @@ -83,14 +83,14 @@ protected: > > int ret; > for (const auto &testCase : testCases) { > - ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first, > + ret = testSingleFixedPoint<4, 7, int16_t>(testCase.first, > testCase.second); > if (ret != TestPass) > return ret; > } > > /* Special case with a superfluous one in the unused bits */ > - ret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992); > + ret = testFixedToFloat<4, 7, int16_t, double>(0xbff, 7.992); > if (ret != TestPass) > return ret; >
Hi Kieran, Thank you for the patch! Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com> Quoting Kieran Bingham (2025-11-14 00:54:09) > The fixedToFloatingPoint does not support unsigned Q types, and > incorrectly sign-extends all values which have the top most bit set in > the quantized values. > > Fix this by ensuring that only signed types perform sign extension, and > simplify the calcuation for unsigned types. > > Convert the storage of the test cases to signed types to correctly > represent their intended purpose, to prevent test failures. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/libipa/fixedpoint.h | 3 +++ > test/ipa/libipa/fixedpoint.cpp | 6 +++--- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h > index 17db8a026502..d97e6d6135ad 100644 > --- a/src/ipa/libipa/fixedpoint.h > +++ b/src/ipa/libipa/fixedpoint.h > @@ -51,6 +51,9 @@ constexpr R fixedToFloatingPoint(T number) > static_assert(sizeof(int) >= sizeof(T)); > static_assert(I + F <= sizeof(T) * 8); > > + if constexpr (std::is_unsigned_v<T>) > + return static_cast<R>(number) / static_cast<R>(1 << F); > + > /* > * Recreate the upper bits in case of a negative number by shifting the sign > * bit from the fixed point to the first bit of the unsigned and then right shifting > diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp > index 935412771851..3f2d9ac97fe5 100644 > --- a/test/ipa/libipa/fixedpoint.cpp > +++ b/test/ipa/libipa/fixedpoint.cpp > @@ -70,7 +70,7 @@ protected: > * The second 7.992 test is to test that unused bits don't > * affect the result. > */ > - std::map<double, uint16_t> testCases = { > + std::map<double, int16_t> testCases = { > { 7.992, 0x3ff }, > { 0.2, 0x01a }, > { -0.2, 0x7e6 }, > @@ -83,14 +83,14 @@ protected: > > int ret; > for (const auto &testCase : testCases) { > - ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first, > + ret = testSingleFixedPoint<4, 7, int16_t>(testCase.first, > testCase.second); > if (ret != TestPass) > return ret; > } > > /* Special case with a superfluous one in the unused bits */ > - ret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992); > + ret = testFixedToFloat<4, 7, int16_t, double>(0xbff, 7.992); > if (ret != TestPass) > return ret; > > -- > 2.51.1 >
Quoting Barnabás Pőcze (2025-11-14 18:08:20) > 2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta: > > The fixedToFloatingPoint does not support unsigned Q types, and > > incorrectly sign-extends all values which have the top most bit set in > > the quantized values. > > Are none of the previously added tests affected? > The addition of + if constexpr (std::is_unsigned_v<T>) + return static_cast<R>(number) / static_cast<R>(1 << F); Fixes any issues. > > Fix this by ensuring that only signed types perform sign extension, and > > simplify the calcuation for unsigned types. > ^^^^^^^^^^ > calculation Thanks > > Convert the storage of the test cases to signed types to correctly > > represent their intended purpose, to prevent test failures. > > I'm only seeing two uses of `fixedToFloatingPoint()` in total. > Both are in `src/ipa/mali-c55/algorithms/awb.cpp`. Those use `uint16_t`; > are they affected by this change? The documentation just says `Q4.8`, > but I imagine they are unsigned actually. > Indeed, they are signed types. The implementation prior to this series stores 'signed' types in an unsigned storage. That's why it has to do internal signed bit extensions and was a big awkward part until I realised ... I could now remove it all - because I use signed/unsigned storage explicitly (which I much prefer). See the patches towards the end of the series. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/ipa/libipa/fixedpoint.h | 3 +++ > > test/ipa/libipa/fixedpoint.cpp | 6 +++--- > > 2 files changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h > > index 17db8a026502..d97e6d6135ad 100644 > > --- a/src/ipa/libipa/fixedpoint.h > > +++ b/src/ipa/libipa/fixedpoint.h > > @@ -51,6 +51,9 @@ constexpr R fixedToFloatingPoint(T number) > > static_assert(sizeof(int) >= sizeof(T)); > > static_assert(I + F <= sizeof(T) * 8); > > > > + if constexpr (std::is_unsigned_v<T>) > > + return static_cast<R>(number) / static_cast<R>(1 << F); > > + > > I'd add the rest to the `else` so as to avoid the "instantiation" of that part. Does the compiler not know to skip it ? It puts an undesirable extra level of indentation on the rest of the code block :S > > > /* > > * Recreate the upper bits in case of a negative number by shifting the sign > > * bit from the fixed point to the first bit of the unsigned and then right shifting > > diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp > > index 935412771851..3f2d9ac97fe5 100644 > > --- a/test/ipa/libipa/fixedpoint.cpp > > +++ b/test/ipa/libipa/fixedpoint.cpp > > @@ -70,7 +70,7 @@ protected: > > * The second 7.992 test is to test that unused bits don't > > * affect the result. > > */ > > - std::map<double, uint16_t> testCases = { > > + std::map<double, int16_t> testCases = { > > { 7.992, 0x3ff }, > > { 0.2, 0x01a }, > > { -0.2, 0x7e6 }, > > @@ -83,14 +83,14 @@ protected: > > > > int ret; > > for (const auto &testCase : testCases) { > > - ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first, > > + ret = testSingleFixedPoint<4, 7, int16_t>(testCase.first, > > testCase.second); > > if (ret != TestPass) > > return ret; > > } > > > > /* Special case with a superfluous one in the unused bits */ > > - ret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992); > > + ret = testFixedToFloat<4, 7, int16_t, double>(0xbff, 7.992); > > if (ret != TestPass) > > return ret; > > >
Quoting Kieran Bingham (2025-11-18 23:47:46) > Quoting Barnabás Pőcze (2025-11-14 18:08:20) > > 2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta: > > > The fixedToFloatingPoint does not support unsigned Q types, and > > > incorrectly sign-extends all values which have the top most bit set in > > > the quantized values. > > > > Are none of the previously added tests affected? > > > > The addition of > > + if constexpr (std::is_unsigned_v<T>) > + return static_cast<R>(number) / static_cast<R>(1 << F); > > Fixes any issues. However, now that I've added the static assert for min < max - it triggers in the build history! https://gitlab.freedesktop.org/camera/libcamera/-/jobs/88124955#L1323 So good spot - it did break before here. I'll move this fix to before introduction of the FixedPointQ types. > > > > > Fix this by ensuring that only signed types perform sign extension, and > > > simplify the calcuation for unsigned types. > > ^^^^^^^^^^ > > calculation > > Thanks > > > > > Convert the storage of the test cases to signed types to correctly > > > represent their intended purpose, to prevent test failures. > > > > I'm only seeing two uses of `fixedToFloatingPoint()` in total. > > Both are in `src/ipa/mali-c55/algorithms/awb.cpp`. Those use `uint16_t`; > > are they affected by this change? The documentation just says `Q4.8`, > > but I imagine they are unsigned actually. > > > > Indeed, they are signed types. The implementation prior to this series > stores 'signed' types in an unsigned storage. That's why it has to do > internal signed bit extensions and was a big awkward part until I > realised ... I could now remove it all - because I use signed/unsigned > storage explicitly (which I much prefer). > > See the patches towards the end of the series. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > > src/ipa/libipa/fixedpoint.h | 3 +++ > > > test/ipa/libipa/fixedpoint.cpp | 6 +++--- > > > 2 files changed, 6 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h > > > index 17db8a026502..d97e6d6135ad 100644 > > > --- a/src/ipa/libipa/fixedpoint.h > > > +++ b/src/ipa/libipa/fixedpoint.h > > > @@ -51,6 +51,9 @@ constexpr R fixedToFloatingPoint(T number) > > > static_assert(sizeof(int) >= sizeof(T)); > > > static_assert(I + F <= sizeof(T) * 8); > > > > > > + if constexpr (std::is_unsigned_v<T>) > > > + return static_cast<R>(number) / static_cast<R>(1 << F); > > > + > > > > I'd add the rest to the `else` so as to avoid the "instantiation" of that part. > > Does the compiler not know to skip it ? > It puts an undesirable extra level of indentation on the rest of the > code block :S > > > > > > > /* > > > * Recreate the upper bits in case of a negative number by shifting the sign > > > * bit from the fixed point to the first bit of the unsigned and then right shifting > > > diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp > > > index 935412771851..3f2d9ac97fe5 100644 > > > --- a/test/ipa/libipa/fixedpoint.cpp > > > +++ b/test/ipa/libipa/fixedpoint.cpp > > > @@ -70,7 +70,7 @@ protected: > > > * The second 7.992 test is to test that unused bits don't > > > * affect the result. > > > */ > > > - std::map<double, uint16_t> testCases = { > > > + std::map<double, int16_t> testCases = { > > > { 7.992, 0x3ff }, > > > { 0.2, 0x01a }, > > > { -0.2, 0x7e6 }, > > > @@ -83,14 +83,14 @@ protected: > > > > > > int ret; > > > for (const auto &testCase : testCases) { > > > - ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first, > > > + ret = testSingleFixedPoint<4, 7, int16_t>(testCase.first, > > > testCase.second); > > > if (ret != TestPass) > > > return ret; > > > } > > > > > > /* Special case with a superfluous one in the unused bits */ > > > - ret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992); > > > + ret = testFixedToFloat<4, 7, int16_t, double>(0xbff, 7.992); > > > if (ret != TestPass) > > > return ret; > > > > >
2025. 11. 19. 0:47 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2025-11-14 18:08:20) >> 2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta: >>> The fixedToFloatingPoint does not support unsigned Q types, and >>> incorrectly sign-extends all values which have the top most bit set in >>> the quantized values. >> >> Are none of the previously added tests affected? >> > > The addition of > > + if constexpr (std::is_unsigned_v<T>) > + return static_cast<R>(number) / static_cast<R>(1 << F); > > Fixes any issues. > > >>> Fix this by ensuring that only signed types perform sign extension, and >>> simplify the calcuation for unsigned types. >> ^^^^^^^^^^ >> calculation > > Thanks > > >>> Convert the storage of the test cases to signed types to correctly >>> represent their intended purpose, to prevent test failures. >> >> I'm only seeing two uses of `fixedToFloatingPoint()` in total. >> Both are in `src/ipa/mali-c55/algorithms/awb.cpp`. Those use `uint16_t`; >> are they affected by this change? The documentation just says `Q4.8`, >> but I imagine they are unsigned actually. >> > > Indeed, they are signed types. The implementation prior to this series > stores 'signed' types in an unsigned storage. That's why it has to do > internal signed bit extensions and was a big awkward part until I > realised ... I could now remove it all - because I use signed/unsigned > storage explicitly (which I much prefer). > > See the patches towards the end of the series. > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> --- >>> src/ipa/libipa/fixedpoint.h | 3 +++ >>> test/ipa/libipa/fixedpoint.cpp | 6 +++--- >>> 2 files changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h >>> index 17db8a026502..d97e6d6135ad 100644 >>> --- a/src/ipa/libipa/fixedpoint.h >>> +++ b/src/ipa/libipa/fixedpoint.h >>> @@ -51,6 +51,9 @@ constexpr R fixedToFloatingPoint(T number) >>> static_assert(sizeof(int) >= sizeof(T)); >>> static_assert(I + F <= sizeof(T) * 8); >>> >>> + if constexpr (std::is_unsigned_v<T>) >>> + return static_cast<R>(number) / static_cast<R>(1 << F); >>> + >> >> I'd add the rest to the `else` so as to avoid the "instantiation" of that part. > > Does the compiler not know to skip it ? > It puts an undesirable extra level of indentation on the rest of the > code block :S Well, the definition of `if constexpr` means that it will have a different effect. In any case, it's not significant here; I was mainly just wondering if the two paths could be more unified, but it's probably fine as is. > > >> >>> /* >>> * Recreate the upper bits in case of a negative number by shifting the sign >>> * bit from the fixed point to the first bit of the unsigned and then right shifting >>> diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp >>> index 935412771851..3f2d9ac97fe5 100644 >>> --- a/test/ipa/libipa/fixedpoint.cpp >>> +++ b/test/ipa/libipa/fixedpoint.cpp >>> @@ -70,7 +70,7 @@ protected: >>> * The second 7.992 test is to test that unused bits don't >>> * affect the result. >>> */ >>> - std::map<double, uint16_t> testCases = { >>> + std::map<double, int16_t> testCases = { >>> { 7.992, 0x3ff }, >>> { 0.2, 0x01a }, >>> { -0.2, 0x7e6 }, >>> @@ -83,14 +83,14 @@ protected: >>> >>> int ret; >>> for (const auto &testCase : testCases) { >>> - ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first, >>> + ret = testSingleFixedPoint<4, 7, int16_t>(testCase.first, >>> testCase.second); >>> if (ret != TestPass) >>> return ret; >>> } >>> >>> /* Special case with a superfluous one in the unused bits */ >>> - ret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992); >>> + ret = testFixedToFloat<4, 7, int16_t, double>(0xbff, 7.992); >>> if (ret != TestPass) >>> return ret; >>> >>
diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h index 17db8a026502..d97e6d6135ad 100644 --- a/src/ipa/libipa/fixedpoint.h +++ b/src/ipa/libipa/fixedpoint.h @@ -51,6 +51,9 @@ constexpr R fixedToFloatingPoint(T number) static_assert(sizeof(int) >= sizeof(T)); static_assert(I + F <= sizeof(T) * 8); + if constexpr (std::is_unsigned_v<T>) + return static_cast<R>(number) / static_cast<R>(1 << F); + /* * Recreate the upper bits in case of a negative number by shifting the sign * bit from the fixed point to the first bit of the unsigned and then right shifting diff --git a/test/ipa/libipa/fixedpoint.cpp b/test/ipa/libipa/fixedpoint.cpp index 935412771851..3f2d9ac97fe5 100644 --- a/test/ipa/libipa/fixedpoint.cpp +++ b/test/ipa/libipa/fixedpoint.cpp @@ -70,7 +70,7 @@ protected: * The second 7.992 test is to test that unused bits don't * affect the result. */ - std::map<double, uint16_t> testCases = { + std::map<double, int16_t> testCases = { { 7.992, 0x3ff }, { 0.2, 0x01a }, { -0.2, 0x7e6 }, @@ -83,14 +83,14 @@ protected: int ret; for (const auto &testCase : testCases) { - ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first, + ret = testSingleFixedPoint<4, 7, int16_t>(testCase.first, testCase.second); if (ret != TestPass) return ret; } /* Special case with a superfluous one in the unused bits */ - ret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992); + ret = testFixedToFloat<4, 7, int16_t, double>(0xbff, 7.992); if (ret != TestPass) return ret;
The fixedToFloatingPoint does not support unsigned Q types, and incorrectly sign-extends all values which have the top most bit set in the quantized values. Fix this by ensuring that only signed types perform sign extension, and simplify the calcuation for unsigned types. Convert the storage of the test cases to signed types to correctly represent their intended purpose, to prevent test failures. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/ipa/libipa/fixedpoint.h | 3 +++ test/ipa/libipa/fixedpoint.cpp | 6 +++--- 2 files changed, 6 insertions(+), 3 deletions(-)