On Mon, 26 Feb 2024 16:10:17 -0500
Harry Wentland <harry.wentland@xxxxxxx> wrote:
> A value of 0x80000000 and higher should round up, and
> below should round down. VKMS Kunit tests for lerp_u16
> showed that this is not the case. Fix it.
>
> 1 << (DRM_FIXED_POINT_HALF - 1) =
> 1 << 15 = 0x8000
>
> This is not 0.5, but 0.00000762939453125.
>
> Instead of some smart math use a simple if/else to
> round up or down. This helps people like me to understand
> what the function does.
>
> Signed-off-by: Harry Wentland <harry.wentland@xxxxxxx>
> ---
> include/drm/drm_fixed.h | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
> index cb842ba80ddd..8ee549f68537 100644
> --- a/include/drm/drm_fixed.h
> +++ b/include/drm/drm_fixed.h
> @@ -77,6 +77,8 @@ static inline u32 dfixed_div(fixed20_12 A, fixed20_12 B)
> #define DRM_FIXED_DIGITS_MASK (~DRM_FIXED_DECIMAL_MASK)
> #define DRM_FIXED_EPSILON 1LL
> #define DRM_FIXED_ALMOST_ONE (DRM_FIXED_ONE - DRM_FIXED_EPSILON)
> +#define DRM_FIXED_FRACTIONAL 0xffffffffll
> +#define DRM_FIXED_HALF 0x80000000ll
>
> /**
> * @drm_sm2fixp
> @@ -106,11 +108,6 @@ static inline int drm_fixp2int(s64 a)
> return ((s64)a) >> DRM_FIXED_POINT;
> }
>
> -static inline int drm_fixp2int_round(s64 a)
> -{
> - return drm_fixp2int(a + (1 << (DRM_FIXED_POINT_HALF - 1)));
> -}
> -
> static inline int drm_fixp2int_ceil(s64 a)
> {
> if (a >= 0)
> @@ -119,6 +116,14 @@ static inline int drm_fixp2int_ceil(s64 a)
> return drm_fixp2int(a - DRM_FIXED_ALMOST_ONE);
> }
>
> +static inline int drm_fixp2int_round(s64 a)
> +{
> + if ((a & DRM_FIXED_FRACTIONAL) < DRM_FIXED_HALF)
> + return drm_fixp2int(a);
So, if we take -epsilon (which is -1 in raw s64 value, the largest
possible value less than zero), this would return -1.0? That does not
sound right.
However, the "add 0.5 and truncate" trick always works, so I'd
recommend sticking that.
Thanks,
pq
> + else
> + return drm_fixp2int_ceil(a);
> +}
> +
> static inline unsigned drm_fixp_msbset(s64 a)
> {
> unsigned shift, sign = (a >> 63) & 1;
Attachment:
pgpCF6VgJBjCT.pgp
Description: OpenPGP digital signature
