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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Matt Roper <matthew.d.roper@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/4] drm/i915: Fix wm latency==0 disable on skl+
Date: Tue, 5 Feb 2019 15:35:19 +0200	[thread overview]
Message-ID: <20190205133519.GO20097@intel.com> (raw)
In-Reply-To: <20190204230750.GK25534@mdroper-desk.amr.corp.intel.com>

On Mon, Feb 04, 2019 at 03:07:50PM -0800, Matt Roper wrote:
> On Mon, Feb 04, 2019 at 08:45:20PM +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.
> > 
> > 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>
> 
> Similarly, isn't the
> 
>         if (res_lines > 31)
>                 return;
> 
> farther down the function going to also cause a problem?  If we fail the
> line requirement then result->min_ddb_alloc and such never get set for
> this plane, which screws up the block sum at block allocation time.

Hmm. Yes, that does seem to be the case. I'll respin.

> 
> 
> Matt
> 
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index ed9786241307..d6186c90bc10 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) ||
> > -- 
> > 2.19.2
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx