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