Re: [PATCH 2/2] drm/i915: Ignore stale wm register values on resume on ilk-bdw

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, May 13, 2016 at 09:58:55AM -0700, Matt Roper wrote:
> On Fri, May 13, 2016 at 05:55:18PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > When we resume the watermark register may contain some BIOS leftovers,
> > or just the hardware reset values. We should ignore those as the
> > pipes will be off anyway, and so frobbing around with intermediate
> > watermarks doesn't make much sense.
> > 
> > In fact I think we should just throw the skip_intermediate_wm flag
> > out, and instead properly sanitize the "active" watermarks to match
> > the current plane and pipe states.
> 
> That's what the flag was originally added for.  The easiest way to
> sanitize the active watermarks is to run the current atomic state
> through our whole atomic check process to calculate all derived state
> plus the watermark values, and then just push the optimal values into
> the hardware.  The flag tells us to just skip calculating the
> intermediate values (which might be bogus and/or fail anyway) since all
> we care about are the final/optimal values.

We should still care about the transition to avoid any underruns,
so we should rather have proper intermediate watermarks here. I
think the problem we're working around here is basically poor hw state
readout. That is, we should more or less just discard the results of
the hw readout for any plane/pipe that is disabled.

The end result should be one less special case to worry about. The less
we have of those the better.

If we would ever run into a genuine case where we couldn't do the
intermediate watermarks properly, then we could always just shut
everything down first, and then bring it back up from scratch.

> 
> Anyway, this looks pretty similar to the fix I wrote right before I went
> OOO, but never got a chance to send out; I set the skip flag when the
> atomic state was created in the suspend function and you do it in the
> resume function, but I think the result should be the same either way,
> so overall lgtm.
> 
> For both patches:
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> 
> > The actual wm state readout might
> > also need a bit of work. But for now, let's continue with the
> > skip_intermediate_wm to keep the fix more minimal.
> > 
> > Fixes this sort of errors on resume
> > [drm:ilk_validate_pipe_wm] LP0 watermark invalid
> > [drm:intel_crtc_atomic_check] No valid intermediate pipe watermarks are possible
> > [drm:intel_display_resume [i915]] *ERROR* Restoring old state failed with -22
> > and a boatload of subsequent modeset BAT fails on my ILK.
> > 
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark programming (v11)")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index a08c4a45b8d3..938653063f15 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12003,6 +12003,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> >  			DRM_DEBUG_KMS("No valid intermediate pipe watermarks are possible\n");
> >  			return ret;
> >  		}
> > +	} else if (dev_priv->display.compute_intermediate_wm) {
> > +		if (HAS_PCH_SPLIT(dev_priv) && INTEL_GEN(dev_priv) < 9)
> > +			pipe_config->wm.intermediate = pipe_config->wm.optimal.ilk;
> >  	}
> >  
> >  	if (INTEL_INFO(dev)->gen >= 9) {
> > @@ -15991,6 +15994,9 @@ retry:
> >  
> >  		state->acquire_ctx = &ctx;
> >  
> > +		/* ignore any reset values/BIOS leftovers in the WM registers */
> > +		to_intel_atomic_state(state)->skip_intermediate_wm = true;
> > +
> >  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> >  			/*
> >  			 * Force recalculation even if we restore
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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