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

[Matt Roper]

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.


Matt


> 
> --Imre
> 
> 
> >  	mutex_lock(&dev->struct_mutex);
> >  	drm_atomic_helper_cleanup_planes(dev, state);
> >  	mutex_unlock(&dev->struct_mutex);

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795