Re: [PATCH v2 1/4] drm/i915: Fix wm latency==0 disable on skl+

From: Matt Roper <matthew.d.roper@intel.com>
To: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 1/4] drm/i915: Fix wm latency==0 disable on skl+
Date: Tue, 5 Feb 2019 07:32:10 -0800	[thread overview]
Message-ID: <20190205153210.GO25534@mdroper-desk.amr.corp.intel.com> (raw)
In-Reply-To: <20190205134213.8485-1-ville.syrjala@linux.intel.com>

On Tue, Feb 05, 2019 at 03:42:13PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When adding the early latency==0 check back I neglected to
> realize that we no longer have a way to return a failure
> from the wm computation like we had in the past (since we
> now calculate wms before ddb allocations). Also plane_en
> being false doesn't actually indicate that the level is
> invalid as it wil also happen when the plane is not
> enabled.
> 
> skl_allocate_pipe_ddb() starts scanning from the maximum
> watermark level and it stops as soon as it finds a level
> that is deemed viable. The assumption being that if level
> n+1 is valid then level n is valid as well. Thus if we
> now disable any watermark level by zeroing its latency
> the code will think that level to be actually valid
> and won't confirm whether the actually enabled lower
> watermark level(s) actually fit into the allotted ddb
> space. This results in hilarious watermark values that
> exceed the ddb allocation of the plane.
> 
> The way we must now indicate a failure is to assign an
> unreasoanbly big value to min_ddb_alloc which will then
> make skl_allocate_pipe_ddb() reject the entire level.
> 
> v2: Also do the same for the lines>31 case (Matt)
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ed9786241307..92b52bb634ad 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4694,8 +4694,11 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *cstate,
>  	uint_fixed_16_16_t selected_result;
>  	u32 res_blocks, res_lines, min_ddb_alloc = 0;
>  
> -	if (latency == 0)
> +	if (latency == 0) {
> +		/* reject it */
> +		result->min_ddb_alloc = U16_MAX;
>  		return;
> +	}
>  
>  	/* Display WA #1141: kbl,cfl */
>  	if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
> @@ -4783,8 +4786,11 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *cstate,
>  	if (!skl_wm_has_lines(dev_priv, level))
>  		res_lines = 0;
>  
> -	if (res_lines > 31)
> +	if (res_lines > 31) {
> +		/* reject it */
> +		result->min_ddb_alloc = U16_MAX;
>  		return;
> +	}

We might also want to change 'blocks' in skl_allocate_pipe_ddb() to a
u32 so that we don't just overflow our sum.  It would be pretty easy for

        U16_MAX + real val < alloc_size

given that 'blocks' is stored as a u16 currently.

Aside from that,

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

>  
>  	/*
>  	 * If res_lines is valid, assume we can use this watermark level
> -- 
> 2.19.2
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx