Re: [PATCH 1/9] drm: Add helpers for x16 fixed point values

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 14 Jun 2024, Imre Deak <imre.deak@intel.com> wrote:
> Add helpers to convert between x16 fixed point and integer/fraction
> values. Also add the format/argument macros required to printk x16
> fixed point variables.
>
> These are needed by later patches dumping the Display Stream Compression
> configuration in DRM core and in the i915 driver to replace the
> corresponding bpp_x16 helpers defined locally in the driver.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_helper.c |  5 +++--
>  include/drm/drm_fixed.h                 | 23 +++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index 79a615667aab1..806f9c9764995 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -35,6 +35,7 @@
>  #include <drm/display/drm_dp_helper.h>
>  #include <drm/display/drm_dp_mst_helper.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_fixed.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_vblank.h>
>  #include <drm/drm_panel.h>
> @@ -4151,9 +4152,9 @@ int drm_dp_bw_overhead(int lane_count, int hactive,
>  	int symbol_cycles;
>  
>  	if (lane_count == 0 || hactive == 0 || bpp_x16 == 0) {
> -		DRM_DEBUG_KMS("Invalid BW overhead params: lane_count %d, hactive %d, bpp_x16 %d.%04d\n",
> +		DRM_DEBUG_KMS("Invalid BW overhead params: lane_count %d, hactive %d, bpp_x16 " DRM_X16_FMT "\n",
>  			      lane_count, hactive,
> -			      bpp_x16 >> 4, (bpp_x16 & 0xf) * 625);
> +			      DRM_X16_ARGS(bpp_x16));
>  		return 0;
>  	}
>  
> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
> index 81572d32db0c2..0fe2a7f50d54e 100644
> --- a/include/drm/drm_fixed.h
> +++ b/include/drm/drm_fixed.h
> @@ -214,4 +214,27 @@ static inline s64 drm_fixp_exp(s64 x)
>  	return sum;
>  }
>  
> +static inline int drm_x16_from_int(int val_int)
> +{
> +	return val_int << 4;
> +}
> +
> +static inline int drm_x16_to_int(int val_x16)
> +{
> +	return val_x16 >> 4;
> +}
> +
> +static inline int drm_x16_to_int_roundup(int val_x16)
> +{
> +	return (val_x16 + 0xf) >> 4;
> +}
> +
> +static inline int drm_x16_to_frac(int val_x16)
> +{
> +	return val_x16 & 0xf;
> +}

Sad trombone about the completely different naming scheme compared to
the rest of the file.

Not saying the existing naming is great, but neither is this. And
there's no way to unify except by renaming *both* afterwards.

We could devise a scheme now that could be used for the existing stuff
later, without renaming the new stuff.

*shrug*

BR,
Jani.



> +
> +#define DRM_X16_FMT		"%d.%04d"
> +#define DRM_X16_ARGS(val_x16)	drm_x16_to_int(val_x16), (drm_x16_to_frac(val_x16) * 625)
> +
>  #endif

-- 
Jani Nikula, Intel