[Intel-gfx] [PATCH 09/12] drm/i915: rip our vblank reset hacks for runtime PM

[Daniel Vetter]

On Tue, May 20, 2014 at 05:03:33PM +0300, Ville Syrjälä wrote:
> On Tue, May 20, 2014 at 03:38:14PM +0200, Daniel Vetter wrote:
> > On Tue, May 20, 2014 at 03:03:41PM +0300, Ville Syrjälä wrote:
> > > On Wed, May 14, 2014 at 08:51:11PM +0200, Daniel Vetter wrote:
> > > > Now that we unconditionally dtrt when disabling/enabling crtcs we
> > > > don't need any hacks any longer to keep the vblank logic sane when
> > > > all the registers go poof. So let's rip it all out.
> > > 
> > > Hmm. drm_update_vblank_count() will now see some kind of diff between
> > > the last and current value when the registers got cloberred. So the
> > > vblank counter reported to userspace will jump. But I guess that's fine
> > > as long as userspace realizes that the counter is not at all reliable
> > > across modesets.
> > 
> > I've added checks for this (the rpm varianst) and for the similiar
> > suspend/resume issues (the suspend variants) to kms_flip. It seems to work
> > and we don't actually jump to far. But maybe the tests are horribly
> > broken.
> 
> Hmm. If we can force the power well off at the start of the test and then
> set a mode, I'd expect the vblank counter to jump by almost max_vblank_count
> since the hw counter would appear to wrap.

Oh, I think the tests are busted ... Need to look at this again.
-Daniel

> 
> > 
> > Can you please take a closer look? I've thought that the entire point of
> > this series (well, one of them) was to finally fix this gag and avoid
> > handing totally bogus frame counter values to userspace. Especially for
> > system suspend/resume where userspace might get susprised ...
> 
> I was mostly interested in making vblank interrupts work during plane
> enable/disable. Anything else is a bonus.
> 
> > -Daniel
> > 
> > > 
> > > > 
> > > > This essentially undoes
> > > > 
> > > > commit 9dbd8febb4dbc9199fcf340b882eb930e36b65b6
> > > > Author: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > > Date:   Tue Jul 23 10:48:11 2013 -0300
> > > > 
> > > >     drm/i915: update last_vblank when disabling the power well
> > > > 
> > > > Apparently igt/kms_flip is already powerful enough to exercise this
> > > > properly, yay! See the reference regression report for details.
> > > > 
> > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=66808
> > > > Testcase: igt/kms_flip/*-vs-rpm
> > > > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_pm.c | 34 ----------------------------------
> > > >  1 file changed, 34 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 75c1c766b507..45fa43f16bb3 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -5423,33 +5423,6 @@ static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv)
> > > >  	}
> > > >  }
> > > >  
> > > > -static void reset_vblank_counter(struct drm_device *dev, enum pipe pipe)
> > > > -{
> > > > -	assert_spin_locked(&dev->vbl_lock);
> > > > -
> > > > -	dev->vblank[pipe].last = 0;
> > > > -}
> > > > -
> > > > -static void hsw_power_well_post_disable(struct drm_i915_private *dev_priv)
> > > > -{
> > > > -	struct drm_device *dev = dev_priv->dev;
> > > > -	enum pipe pipe;
> > > > -	unsigned long irqflags;
> > > > -
> > > > -	/*
> > > > -	 * After this, the registers on the pipes that are part of the power
> > > > -	 * well will become zero, so we have to adjust our counters according to
> > > > -	 * that.
> > > > -	 *
> > > > -	 * FIXME: Should we do this in general in drm_vblank_post_modeset?
> > > > -	 */
> > > > -	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> > > > -	for_each_pipe(pipe)
> > > > -		if (pipe != PIPE_A)
> > > > -			reset_vblank_counter(dev, pipe);
> > > > -	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> > > > -}
> > > > -
> > > >  static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> > > >  			       struct i915_power_well *power_well, bool enable)
> > > >  {
> > > > @@ -5478,8 +5451,6 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> > > >  			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
> > > >  			POSTING_READ(HSW_PWR_WELL_DRIVER);
> > > >  			DRM_DEBUG_KMS("Requesting to disable the power well\n");
> > > > -
> > > > -			hsw_power_well_post_disable(dev_priv);
> > > >  		}
> > > >  	}
> > > >  }
> > > > @@ -5646,11 +5617,6 @@ static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv,
> > > >  	valleyview_disable_display_irqs(dev_priv);
> > > >  	spin_unlock_irq(&dev_priv->irq_lock);
> > > >  
> > > > -	spin_lock_irq(&dev->vbl_lock);
> > > > -	for_each_pipe(pipe)
> > > > -		reset_vblank_counter(dev, pipe);
> > > > -	spin_unlock_irq(&dev->vbl_lock);
> > > > -
> > > >  	vlv_set_power_well(dev_priv, power_well, false);
> > > >  }
> > > >  
> > > > -- 
> > > > 1.8.3.1
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch