Re: [Intel-gfx] [PATCH 6/9] drm/i915/gen9: fix plane_blocks_per_line on watermarks calculations

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 22/09/2016 22:00, Paulo Zanoni wrote:
> The confusing thing is that plane_blocks_per_line is listed as part of
> the method 2 calculation but is also used for other things. We
> calculated it in two different places and different ways: one inside
> skl_wm_method2() and the other inside skl_compute_plane_wm(). The
> skl_wm_method2() implementation is the one that matches the
> specification.
>
> With this patch we fix the skl_compute_plane_wm() calculation and just
> pass it as a parameter to skl_wm_method2(). We also take care to not
> modify the value of plane_bytes_per_line since we're going to rely on
> it having a correct value in later patches.
>
> This should affect the watermarks for Linear and Y-tiled.
>
>  From my analysis, it looks like the two plane_blocks_per_line
> variables got out of sync on 0fda65680e92, but we can't really say
> that commit was a regression, it looks like just an incomplete fix.
> There's always the possibility that 0fda65680e92 matched our
> specification at that time, and then later the specification changed.

Yes, it wouldn't be a regression since before that commit changing 
tiling with page flips did not work at all.

Whether or not calculation was fully correct at the time I am not sure. 
The formulas changed at least twice after the initial implementation so  
both are possible.

Regards,

Tvrtko

> v2: Try to add a "Fixes" tag (Maarten).
>
> Fixes: 0fda65680e92 ("drm/i915/skl: Update watermarks for Y tiling")
> Cc: stable@vger.kernel.org
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Lyude <cpaul@redhat.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_pm.c | 39 +++++++++++++++------------------------
>   1 file changed, 15 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a7f5f7f..a6ae7b7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3494,30 +3494,14 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t cpp, uint32_t latenc
>   }
>   
>   static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
> -			       uint32_t horiz_pixels, uint8_t cpp,
> -			       uint64_t tiling, uint32_t latency,
> -			       uint32_t y_min_scanlines)
> +			       uint32_t latency, uint32_t plane_blocks_per_line)
>   {
>   	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 * cpp;
> -
> -	if (tiling == I915_FORMAT_MOD_Y_TILED ||
> -	    tiling == I915_FORMAT_MOD_Yf_TILED) {
> -		plane_bytes_per_line *= y_min_scanlines;
> -		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
> -		plane_blocks_per_line /= y_min_scanlines;
> -	} else if (tiling == DRM_FORMAT_MOD_NONE) {
> -		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512) + 1;
> -	} else {
> -		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_blocks_per_line;
> @@ -3606,17 +3590,24 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>   		y_min_scanlines = 4;
>   	}
>   
> +	plane_bytes_per_line = width * cpp;
> +	if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> +	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
> +		plane_blocks_per_line =
> +		      DIV_ROUND_UP(plane_bytes_per_line * y_min_scanlines, 512);
> +		plane_blocks_per_line /= y_min_scanlines;
> +	} else if (fb->modifier[0] == DRM_FORMAT_MOD_NONE) {
> +		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512)
> +					+ 1;
> +	} else {
> +		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
> +	}
> +
>   	method1 = skl_wm_method1(plane_pixel_rate, cpp, latency);
>   	method2 = skl_wm_method2(plane_pixel_rate,
>   				 cstate->base.adjusted_mode.crtc_htotal,
> -				 width,
> -				 cpp,
> -				 fb->modifier[0],
>   				 latency,
> -				 y_min_scanlines);
> -
> -	plane_bytes_per_line = width * cpp;
> -	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
> +				 plane_blocks_per_line);
>   
>   	if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>   	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {