[Intel-gfx] [PATCH 6/8] drm/i915/skl: Updated watermark programming

[Damien Lespiau]

On Wed, Feb 25, 2015 at 04:47:22PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> Recent BSpect updates have changed the watermark calculation to avoid
> display flickering in some cases.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---

There are really several changes in this patch, it would have been
easier for the review to split them (and that's usually how we are
supposed to split patches).

  - Convert the inner computations to number of blocks
  - W/A to increase the nb of blocks for level 1-7 by 1
  - Move max block test to >= instead of >

Anyway, which the couple of comments below addressd (of which only the
'>=' is a real problem), this is:

Reviewed-by: Damien Lespiau <damien.lespiau at intel.com>

>  drivers/gpu/drm/i915/intel_pm.c | 52 +++++++++++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f7c9938..626c3c2 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2595,7 +2595,7 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t bytes_per_pixel,
>  	if (latency == 0)
>  		return UINT_MAX;
>  
> -	wm_intermediate_val = latency * pixel_rate * bytes_per_pixel;
> +	wm_intermediate_val = latency * pixel_rate * bytes_per_pixel / 512;
>  	ret = DIV_ROUND_UP(wm_intermediate_val, 1000);
>  
>  	return ret;
> @@ -2605,15 +2605,18 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
>  			       uint32_t horiz_pixels, uint8_t bytes_per_pixel,
>  			       uint32_t latency)
>  {
> -	uint32_t ret, plane_bytes_per_line, wm_intermediate_val;
> +	uint32_t ret;
> +	uint32_t plane_bytes_per_line, plane_blocks_per_line;
> +	uint32_t wm_intermediate_val;
>  
>  	if (latency == 0)
>  		return UINT_MAX;
>  
>  	plane_bytes_per_line = horiz_pixels * bytes_per_pixel;
> +	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>  	wm_intermediate_val = latency * pixel_rate;
>  	ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000) *
> -				plane_bytes_per_line;
> +				plane_blocks_per_line;
>  
>  	return ret;
>  }
> @@ -2693,39 +2696,47 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>  	}
>  }
>  
> -static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
> +static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> +				 struct skl_pipe_wm_parameters *p,
>  				 struct intel_plane_wm_parameters *p_params,
>  				 uint16_t ddb_allocation,
> -				 uint32_t mem_value,
> +				 int level,
>  				 uint16_t *out_blocks, /* out */
>  				 uint8_t *out_lines /* out */)
>  {
> -	uint32_t method1, method2, plane_bytes_per_line, res_blocks, res_lines;
> -	uint32_t result_bytes;
> +	uint32_t latency = dev_priv->wm.skl_latency[level];
> +	uint32_t method1, method2;
> +	uint32_t plane_bytes_per_line, plane_blocks_per_line;
> +	uint32_t res_blocks, res_lines;
> +	uint32_t result_blocks;

we now have res_blocks and result_blocks, a bit confusing. Maybe rename
result_blocks to selected_result (which happens to be in blocks)

>  
> -	if (mem_value == 0 || !p->active || !p_params->enabled)
> +	if (latency == 0 || !p->active || !p_params->enabled)
>  		return false;
>  
>  	method1 = skl_wm_method1(p->pixel_rate,
>  				 p_params->bytes_per_pixel,
> -				 mem_value);
> +				 latency);
>  	method2 = skl_wm_method2(p->pixel_rate,
>  				 p->pipe_htotal,
>  				 p_params->horiz_pixels,
>  				 p_params->bytes_per_pixel,
> -				 mem_value);
> +				 latency);
>  
>  	plane_bytes_per_line = p_params->horiz_pixels *
>  					p_params->bytes_per_pixel;
> +	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>  
>  	/* For now xtile and linear */
> -	if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
> -		result_bytes = min(method1, method2);
> +	if ((ddb_allocation / plane_blocks_per_line) >= 1)
> +		result_blocks = min(method1, method2);
>  	else
> -		result_bytes = method1;
> +		result_blocks = method1;
> +
> +	res_blocks = result_blocks + 1;
> +	res_lines = DIV_ROUND_UP(result_blocks, plane_blocks_per_line);
>  
> -	res_blocks = DIV_ROUND_UP(result_bytes, 512) + 1;
> -	res_lines = DIV_ROUND_UP(result_bytes, plane_bytes_per_line);
> +	if (level >=1 && level <= 7)

a space is missing before the 1 :)

> +		res_blocks++;
>  
>  	if (res_blocks > ddb_allocation || res_lines > 31)

res_blocks >= ddb_allocation

>  		return false;
> @@ -2744,23 +2755,24 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>  				 int num_planes,
>  				 struct skl_wm_level *result)
>  {
> -	uint16_t latency = dev_priv->wm.skl_latency[level];
>  	uint16_t ddb_blocks;
>  	int i;
>  
>  	for (i = 0; i < num_planes; i++) {
>  		ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
>  
> -		result->plane_en[i] = skl_compute_plane_wm(p, &p->plane[i],
> +		result->plane_en[i] = skl_compute_plane_wm(dev_priv,
> +						p, &p->plane[i],
>  						ddb_blocks,
> -						latency,
> +						level,
>  						&result->plane_res_b[i],
>  						&result->plane_res_l[i]);
>  	}
>  
>  	ddb_blocks = skl_ddb_entry_size(&ddb->cursor[pipe]);
> -	result->cursor_en = skl_compute_plane_wm(p, &p->cursor, ddb_blocks,
> -						 latency, &result->cursor_res_b,
> +	result->cursor_en = skl_compute_plane_wm(dev_priv, p, &p->cursor,
> +						 ddb_blocks, level,
> +						 &result->cursor_res_b,
>  						 &result->cursor_res_l);
>  }
>  
> -- 
> 2.3.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx