[RFC PATCH v4 03/42] drm: Correctly round for fixp2int_round

[Pekka Paalanen]

On Mon, 26 Feb 2024 16:10:17 -0500
Harry Wentland <harry.wentland at amd.com> 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 at amd.com>
> ---
>  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;

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20240311/a0e1fe42/attachment-0001.sig>