Re: [PATCH] drm/i915/icl: Avoid Gen10 watermark workarounds in Gen11

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Aug 23, 2018 at 08:59:44AM +0530, Karthik B S wrote:
> Check added to skip the watermark workarounds intended for Gen10 and
> below platforms in Gen11.

This seems a bit ambiguous for me, could you please improve the commit
message a bit?

> 
> Signed-off-by: Karthik B S <karthik.b.s@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d99e5fa..1928fe0 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4677,28 +4677,35 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  	res_lines = div_round_up_fixed16(selected_result,
>  					 wp->plane_blocks_per_line);
>  
> -	/* Display WA #1125: skl,bxt,kbl,glk */
> -	if (level == 0 && wp->rc_surface)
> -		res_blocks += fixed16_to_u32_round_up(wp->y_tile_minimum);
> -
> -	/* Display WA #1126: skl,bxt,kbl,glk */
> -	if (level >= 1 && level <= 7) {
> -		if (wp->y_tiled) {
> +	if (INTEL_GEN(dev_priv) < 11) {
> +		/* Display WA #1125: skl,bxt,kbl,glk */
> +		if (level == 0 && wp->rc_surface)
>  			res_blocks += fixed16_to_u32_round_up(
>  							wp->y_tile_minimum);
> -			res_lines += wp->y_min_scanlines;
> -		} else {
> -			res_blocks++;
> +
> +		/* Display WA #1126: skl,bxt,kbl,glk */
> +		if (level >= 1 && level <= 7) {
> +			if (wp->y_tiled) {
> +				res_blocks += fixed16_to_u32_round_up(
> +							wp->y_tile_minimum);
> +				res_lines += wp->y_min_scanlines;
> +			} else {
> +				res_blocks++;
> +			}
>  		}
> +	}
>  
> -		/*
> -		 * Make sure result blocks for higher latency levels are atleast
> -		 * as high as level below the current level.
> -		 * Assumption in DDB algorithm optimization for special cases.
> -		 * Also covers Display WA #1125 for RC.
> -		 */
> +	/*
> +	 * Make sure result blocks for higher latency levels are atleast
> +	 * as high as level below the current level.
> +	 * Assumption in DDB algorithm optimization for special cases.
> +	 * Also covers Display WA #1125 for RC.
> +	 */
> +	if (level >= 1 && level <= 7) {
>  		if (result_prev->plane_res_b > res_blocks)
>  			res_blocks = result_prev->plane_res_b;

Everything above makes sense and I checked against spec and it is right.

> +		if (result_prev->plane_res_l > res_lines)
> +			res_lines = result_prev->plane_res_l;

My on;y concern here is with this line...
This seems a new addition that if needed needs to come in a
separated patch with its own justification.

Sorry for taking so long to review it.

Thanks,
Rodrigo.

>  	}
>  
>  	if (INTEL_GEN(dev_priv) >= 11) {
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx