[Intel-gfx] [PATCH] drm/i915: Wait until after wm optimization to drop runtime PM reference

[Imre Deak]

On ma, 2016-03-07 at 08:10 -0800, Matt Roper wrote:
> On Sat, Mar 05, 2016 at 03:25:05AM +0200, Imre Deak wrote:
> > Hi Matt,
> > 
> > On Fri, 2016-03-04 at 15:59 -0800, Matt Roper wrote:
> > > At the end of an atomic commit, we currently wait for vblanks to
> > > complete, call put() on the various runtime PM references, and then
> > > try
> > > to optimize our watermarks (on platforms that need two-step watermark
> > > programming).  This can lead to watermark registers being programmed
> > > while the power well is powered down.  We need to wait until after
> > > watermark optimization is complete before dropping our runtime power
> > > references.
> > > 
> > > Note that in the future the watermark optimization is probably going
> > > to
> > > move to an asynchronous workqueue task that happens at some arbitrary
> > > point after vblank.  When we make that change, we'll no longer
> > > necessarily be operating under the power reference held here, so
> > > we'll
> > > need to wrap the watermark register programmin in a call to
> > > intel_runtime_pm_get_if_in_use() or similar.
> > > 
> > > Cc: arun.siluvery at linux.intel.com
> > > Cc: ville.syrjala at linux.intel.com
> > > Cc: maarten.lankhorst at linux.intel.com
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94349
> > > Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark
> > > programming (v11)")
> > > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++----------
> > >  1 file changed, 10 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 62d36a7..0af08d7 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13789,16 +13789,6 @@ static int intel_atomic_commit(struct
> > > drm_device *dev,
> > >  	if (!state->legacy_cursor_update)
> > >  		intel_atomic_wait_for_vblanks(dev, dev_priv,
> > > crtc_vblank_mask);
> > >  
> > > -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > -		intel_post_plane_update(to_intel_crtc(crtc));
> > > -
> > > -		if (put_domains[i])
> > > -			modeset_put_power_domains(dev_priv,
> > > put_domains[i]);
> > > -	}
> > > -
> > > -	if (intel_state->modeset)
> > > -		intel_display_power_put(dev_priv,
> > > POWER_DOMAIN_MODESET);
> > > -
> > >  	/*
> > >  	 * Now that the vblank has passed, we can go ahead and
> > > program the
> > >  	 * optimal watermarks on platforms that need two-step
> > > watermark
> > > @@ -13813,6 +13803,16 @@ static int intel_atomic_commit(struct
> > > drm_device *dev,
> > >  			dev_priv-
> > > > display.optimize_watermarks(intel_cstate);
> > >  	}
> > >  
> > > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > +		intel_post_plane_update(to_intel_crtc(crtc));
> > > +
> > > +		if (put_domains[i])
> > > +			modeset_put_power_domains(dev_priv,
> > > put_domains[i]);
> > > +	}
> > > +
> > > +	if (intel_state->modeset)
> > > +> 		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
> > > +
> > 
> > Since the error was caused by writing some WM register while the HW is
> > suspended, I don't see what would be the point of programming it if the
> > register loses its value anyway after dropping the power reference.
> > What would make sense to me is to avoid programming the WM registers if
> > all the outputs are disabled (like they were when the bug triggered)
> > and program them next time around when any output gets enabled.
> 
> Yeah, that was actually the first approach I went with:
> 
>         https://patchwork.freedesktop.org/patch/75772/
> 
> Ville felt like it was more of a workaround than a fix though, so I
> posted this alternative instead.

Ok, I haven't noticed that patch.

One other thing that I realized only after sending my email is that -
on some old platforms at least - it would still make sense to program
the WM state even with all outputs disabled: the description of
FW_BLC_Self[15] on GEN3 for example could mean that memory self refresh
is disabled whenever we set this bit to 0, independently of the display
output state, although this description is somewhat vague.

--Imre