[Intel-gfx] [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6 v3

[Daniel Vetter]

On Tue, Apr 23, 2013 at 04:28:17PM +0300, Ville Syrjälä wrote:
> On Tue, Apr 23, 2013 at 11:45:03AM +0300, Jani Nikula wrote:
> > On Tue, 23 Apr 2013, Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
> > > @@ -2895,6 +2913,8 @@ static void valleyview_enable_rps(struct drm_device *dev)
> > >  	DRM_DEBUG_DRIVER("setting GPU freq to %d\n",
> > >  			 vlv_gpu_freq(dev_priv->mem_freq, rpe));
> > >  
> > > +	INIT_DELAYED_WORK(&dev_priv->rps.vlv_work, vlv_rps_timer_work);
> > > +
> > >  	valleyview_set_rps(dev_priv->dev, rpe);
> > >  
> > >  	/* requires MSI enabled */
> > > @@ -3640,6 +3660,8 @@ void intel_disable_gt_powersave(struct drm_device *dev)
> > >  		mutex_lock(&dev_priv->rps.hw_lock);
> > >  		gen6_disable_rps(dev);
> > >  		mutex_unlock(&dev_priv->rps.hw_lock);
> > 
> > Theoretically the work could get executed here, between unlock and
> > cancel. Does that matter?
> 
> AFAICS even gen6_pm_rps_work() might be executing even after
> gen6_disable_rps() was called.
> 
> I'm not quite sure if allowing gen6_pm_rps_work() to do stuff after
> gen6_disable_rps() was called is already a problem on it's own. At
> least it can overwrite RPNSWREQ on !VLV. BTW that register doesn't seem
> to exist on VLV and yet we're poking at it from gen6_disable_rps()...

Yeah, ordering in our rps teardown code seems screwed-up, since we disable
interrupts a bit too late. Now disable_rps does clear PM_IIR and disable
all interrupts, but like you've said it doesn't bother to stop the
rps_work, so disable_rps can still get rearmed.

Two things:
- I do not know how much we really should care here for now, since ...

- Setup/teardown ordering is already a giant mess and did blow up a lot of
  times recently. I wonder whether we shouldn't just start to massively
  sprinkle asserts all over the place to double/triple check ordering
  sequence constrainst, similarly to how we currently handle the modeset
  sequence.

Another tool to (partially) alleviate these issues (especially failure
path handling) is devres.c, which I've just recently stumbled over.

Definitely an area to ponder, since imo setup/teardown is the currently
worst part (in terms of likeliness to break unnoticed) in our driver.

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