[Intel-gfx] [PATCH 04/11] drm/i915/skl+: calculate pixel_rate & relative_data_rate in fixed point

[Matt Roper]

On Mon, May 08, 2017 at 05:18:55PM +0530, Mahesh Kumar wrote:
> This patch make changes to calculate adjusted plane pixel rate &
> plane downscale amount using fixed_point functions available.
> This patch will give uniformity in code, & will help to avoid mixing of
> 32bit uint32_t variable for fixed-16.16 with fixed_16_16_t variables in
> later patch in the series.
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 66b542ba47ad..6414701ef09e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3360,26 +3360,27 @@ 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_crtc_state *cstate,
>  			   const struct intel_plane_state *pstate)
>  {
>  	struct intel_plane *plane = to_intel_plane(pstate->base.plane);
> -	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(!intel_wm_plane_visible(cstate, pstate)))
> -		return DRM_PLANE_HELPER_NO_SCALING;
> +		return u32_to_fixed_16_16(0);

I don't think this really changes anything, right?  Both of the places
that call this function have already bailed out and returned 0 data rate
if the plane is invisible.  Not that it hurts anything either.

>  
>  	/* n.b., src is 16.16 fixed point, dst is whole integer */
>  	if (plane->id == PLANE_CURSOR) {
> -		src_w = pstate->base.src_w;
> -		src_h = pstate->base.src_h;
> +		src_w = pstate->base.src_w >> 16;
> +		src_h = pstate->base.src_h >> 16;
>  		dst_w = pstate->base.crtc_w;
>  		dst_h = pstate->base.crtc_h;
>  	} else {
> -		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);
>  	}
> @@ -3387,11 +3388,13 @@ skl_plane_downscale_amount(const struct intel_crtc_state *cstate,
>  	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);

I think we can drop the comment here now; it was originally added to
remind why we were doing the extra shift at return, but now it's pretty
obvious at this point that everything here is dealing with fixed point.

With or without the comment dropped,

Reviewed-by: Matt Roper <matthew.d.roper at intel.com>

>  }
>  
>  static unsigned int
> @@ -3401,10 +3404,11 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
>  {
>  	struct intel_plane *plane = to_intel_plane(pstate->plane);
>  	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 down_scale_amount;
>  
>  	if (!intel_pstate->base.visible)
>  		return 0;
> @@ -3438,7 +3442,7 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
>  
>  	down_scale_amount = skl_plane_downscale_amount(cstate, intel_pstate);
>  
> -	return (uint64_t)data_rate * down_scale_amount >> 16;
> +	return mul_u32_fixed_16_16_round_up(data_rate, down_scale_amount);
>  }
>  
>  /*
> @@ -3724,8 +3728,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(!intel_wm_plane_visible(cstate, pstate)))
> @@ -3738,10 +3741,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(cstate, 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,
> -- 
> 2.11.0
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795