Re: [PATCH 1/8] drm/i915/skl+: calculate pixel_rate & relative_data_rate in fixed point

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Ter, 2017-02-28 às 17:01 +0530, Mahesh Kumar escreveu:
> This patch make changes to calculate adjusted plane pixel rate &
> plane downscale amount using fixed_point functions available. This
> also
> adds few fixed point function to facilitate calculation.

Before we go into the full review I need to ask: but why does the patch
do what it says it does? Is this a bug fix? Is this needed for some
rework later in the series? What happens if we don't merge this patch
but try to merge the others without it? Having those questions answered
in the commit message help not only the patch reviewers, but also all
the future git-bisecters and anybody else who happens to end up
touching this code change.


> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 34 +++++++++++++++++++++++++++++
> ----
>  drivers/gpu/drm/i915/intel_pm.c | 42 ++++++++++++++++++++-----------
> ----------
>  2 files changed, 51 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index eed9ead1b592..f26f61b0e7c8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -170,8 +170,8 @@ static inline uint_fixed_16_16_t
> max_fixed_16_16(uint_fixed_16_16_t max1,
>  	return max;
>  }
>  
> -static inline uint_fixed_16_16_t fixed_16_16_div_round_up(uint32_t
> val,
> -							  uint32_t
> d)
> +static inline uint_fixed_16_16_t fixed_16_16_div(uint32_t val,
> +						 uint32_t d)

Why are we renaming this? It still rounds up.

>  {
>  	uint_fixed_16_16_t fp, res;
>  
> @@ -180,8 +180,8 @@ static inline uint_fixed_16_16_t
> fixed_16_16_div_round_up(uint32_t val,
>  	return res;
>  }
>  
> -static inline uint_fixed_16_16_t
> fixed_16_16_div_round_up_u64(uint32_t val,
> -							      uint32
> _t d)
> +static inline uint_fixed_16_16_t fixed_16_16_div_u64(uint32_t val,
> +						     uint32_t d)

Why are we renaming this? It still rounds up.

>  {
>  	uint_fixed_16_16_t res;
>  	uint64_t interm_val;
> @@ -206,6 +206,32 @@ static inline uint_fixed_16_16_t
> mul_u32_fixed_16_16(uint32_t val,
>  	return fp;
>  }
>  
> +static inline uint32_t mul_u32_fixed_16_16_round_up(uint32_t val,
> +						    uint_fixed_16_16
> _t mul)
> +{
> +	uint64_t intermediate_val;
> +	uint32_t result;
> +
> +	intermediate_val = (uint64_t) val * mul.val;
> +	intermediate_val = DIV_ROUND_UP_ULL(intermediate_val, 1 <<
> 16);
> +	WARN_ON(intermediate_val >> 32);
> +	result = clamp_t(uint32_t, intermediate_val, 0, ~0);
> +	return result;
> +}
> +
> +static inline uint_fixed_16_16_t mul_fixed_16_16(uint_fixed_16_16_t
> val,
> +						 uint_fixed_16_16_t
> mul)
> +{
> +	uint64_t intermediate_val;
> +	uint_fixed_16_16_t fp;
> +
> +	intermediate_val = (uint64_t) val.val * mul.val;
> +	intermediate_val = intermediate_val >> 16;
> +	WARN_ON(intermediate_val >> 32);
> +	fp.val = clamp_t(uint32_t, intermediate_val, 0, ~0);
> +	return fp;
> +}
> +
>  static inline const char *yesno(bool v)
>  {
>  	return v ? "yes" : "no";
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 169c4908ad5b..09562d86c0fb 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3161,28 +3161,30 @@ void skl_ddb_get_hw_state(struct
> drm_i915_private *dev_priv,
>   * Return value is provided in 16.16 fixed point form to retain
> fractional part.
>   * Caller should take care of dividing & rounding off the value.
>   */
> -static uint32_t
> +static uint_fixed_16_16_t
>  skl_plane_downscale_amount(const struct intel_plane_state *pstate)
>  {
> -	uint32_t downscale_h, downscale_w;
>  	uint32_t src_w, src_h, dst_w, dst_h;
> +	uint_fixed_16_16_t fp_w_ratio, fp_h_ratio;
> +	uint_fixed_16_16_t downscale_h, downscale_w;
>  
>  	if (WARN_ON(!pstate->base.visible))
> -		return DRM_PLANE_HELPER_NO_SCALING;
> +		return u32_to_fixed_16_16(0);
>  
> -	/* n.b., src is 16.16 fixed point, dst is whole integer */
> -	src_w = drm_rect_width(&pstate->base.src);
> -	src_h = drm_rect_height(&pstate->base.src);
> +	src_w = drm_rect_width(&pstate->base.src) >> 16;
> +	src_h = drm_rect_height(&pstate->base.src) >> 16;
>  	dst_w = drm_rect_width(&pstate->base.dst);
>  	dst_h = drm_rect_height(&pstate->base.dst);
>  	if (drm_rotation_90_or_270(pstate->base.rotation))
>  		swap(dst_w, dst_h);
>  
> -	downscale_h = max(src_h / dst_h,
> (uint32_t)DRM_PLANE_HELPER_NO_SCALING);
> -	downscale_w = max(src_w / dst_w,
> (uint32_t)DRM_PLANE_HELPER_NO_SCALING);
> +	fp_w_ratio = fixed_16_16_div(src_w, dst_w);
> +	fp_h_ratio = fixed_16_16_div(src_h, dst_h);
> +	downscale_w = max_fixed_16_16(fp_w_ratio,
> u32_to_fixed_16_16(1));
> +	downscale_h = max_fixed_16_16(fp_h_ratio,
> u32_to_fixed_16_16(1));
>  
>  	/* Provide result in 16.16 fixed point */
> -	return (uint64_t)downscale_w * downscale_h >> 16;
> +	return mul_fixed_16_16(downscale_w, downscale_h);
>  }
>  
>  static unsigned int
> @@ -3191,10 +3193,11 @@ skl_plane_relative_data_rate(const struct
> intel_crtc_state *cstate,
>  			     int y)
>  {
>  	struct intel_plane_state *intel_pstate =
> to_intel_plane_state(pstate);
> -	uint32_t down_scale_amount, data_rate;
> +	uint32_t data_rate;
>  	uint32_t width = 0, height = 0;
>  	struct drm_framebuffer *fb;
>  	u32 format;
> +	uint_fixed_16_16_t downscale_amount;
>  
>  	if (!intel_pstate->base.visible)
>  		return 0;
> @@ -3226,9 +3229,9 @@ skl_plane_relative_data_rate(const struct
> intel_crtc_state *cstate,
>  		data_rate = width * height * fb->format->cpp[0];
>  	}
>  
> -	down_scale_amount =
> skl_plane_downscale_amount(intel_pstate);
> +	downscale_amount = skl_plane_downscale_amount(intel_pstate);
>  
> -	return (uint64_t)data_rate * down_scale_amount >> 16;
> +	return mul_u32_fixed_16_16_round_up(data_rate,
> downscale_amount);
>  }
>  
>  /*
> @@ -3488,7 +3491,7 @@ static uint_fixed_16_16_t
> skl_wm_method1(uint32_t pixel_rate, uint8_t cpp,
>  		return FP_16_16_MAX;
>  
>  	wm_intermediate_val = latency * pixel_rate * cpp;
> -	ret = fixed_16_16_div_round_up_u64(wm_intermediate_val, 1000
> * 512);
> +	ret = fixed_16_16_div_u64(wm_intermediate_val, 1000 * 512);
>  	return ret;
>  }
>  
> @@ -3514,8 +3517,7 @@ static uint32_t
> skl_adjusted_plane_pixel_rate(const struct intel_crtc_state *cst
>  					      struct
> intel_plane_state *pstate)
>  {
>  	uint64_t adjusted_pixel_rate;
> -	uint64_t downscale_amount;
> -	uint64_t pixel_rate;
> +	uint_fixed_16_16_t downscale_amount;
>  
>  	/* Shouldn't reach here on disabled planes... */
>  	if (WARN_ON(!pstate->base.visible))
> @@ -3528,10 +3530,8 @@ static uint32_t
> skl_adjusted_plane_pixel_rate(const struct intel_crtc_state *cst
>  	adjusted_pixel_rate = cstate->pixel_rate;
>  	downscale_amount = skl_plane_downscale_amount(pstate);
>  
> -	pixel_rate = adjusted_pixel_rate * downscale_amount >> 16;
> -	WARN_ON(pixel_rate != clamp_t(uint32_t, pixel_rate, 0, ~0));
> -
> -	return pixel_rate;
> +	return mul_u32_fixed_16_16_round_up(adjusted_pixel_rate,
> +					    downscale_amount);
>  }
>  
>  static int skl_compute_plane_wm(const struct drm_i915_private
> *dev_priv,
> @@ -3617,8 +3617,8 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>  	if (y_tiled) {
>  		interm_pbpl = DIV_ROUND_UP(plane_bytes_per_line *
>  					   y_min_scanlines, 512);
> -		plane_blocks_per_line =
> -		      fixed_16_16_div_round_up(interm_pbpl,
> y_min_scanlines);
> +		plane_blocks_per_line = fixed_16_16_div(interm_pbpl,
> +							y_min_scanli
> nes);
>  	} else if (x_tiled) {
>  		interm_pbpl = DIV_ROUND_UP(plane_bytes_per_line,
> 512);
>  		plane_blocks_per_line =
> u32_to_fixed_16_16(interm_pbpl);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx