| Message ID | 1366795160-2427-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 24, 2013 at 11:19:20AM +0200, Daniel Vetter wrote: > Our rps code relies on the interrupts being off to prevent re-arming > of the work items at inopportune moments. > > Also drop the redundant cancel_work for the main rps work, > disable_gt_powersave already takes care of that. > > Finally add a WARN_ON to ensure we obey that piece of ordering > constraint. Long term I want to lock down the setup/teardown code in a > similar way to how we painstakingly check modeset sequence constraints > already. Reminds me, did we every get around to flushing our deferred hw tasks upon suspend? I've a funny feeling that is still missing. -Chris
On Wed, Apr 24, 2013 at 10:35:16AM +0100, Chris Wilson wrote: > On Wed, Apr 24, 2013 at 11:19:20AM +0200, Daniel Vetter wrote: > > Our rps code relies on the interrupts being off to prevent re-arming > > of the work items at inopportune moments. > > > > Also drop the redundant cancel_work for the main rps work, > > disable_gt_powersave already takes care of that. > > > > Finally add a WARN_ON to ensure we obey that piece of ordering > > constraint. Long term I want to lock down the setup/teardown code in a > > similar way to how we painstakingly check modeset sequence constraints > > already. > > Reminds me, did we every get around to flushing our deferred hw tasks > upon suspend? I've a funny feeling that is still missing. I think with the unification between suspend and driver unload we're mostly there. Like I've said I think it's time to encode all these constraints properly into debug asserts, but the problem I have is that I don't have a good idea to encode things like "this work item is now guaranteed to never get re-armed again". There's the object debug code in the kernel, but that only fires when the work/timer is actually armed. Since a lot of the work stuff happens rarely or is not delayed that's not really good enough ... Ideas for this very much welcome, since the lack of these self-checks makes any attempt to solve this for real futile. devres.c might help a bit in preventing resource leaks and tidy up the code, but it doesn't help at all for correct ordering and cleanup of async workers/timers. -Daniel
On Wed, 24 Apr 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Our rps code relies on the interrupts being off to prevent re-arming > of the work items at inopportune moments. > > Also drop the redundant cancel_work for the main rps work, > disable_gt_powersave already takes care of that. > > Finally add a WARN_ON to ensure we obey that piece of ordering > constraint. Long term I want to lock down the setup/teardown code in a > similar way to how we painstakingly check modeset sequence constraints > already. > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 15 ++++++++------- > drivers/gpu/drm/i915/intel_pm.c | 3 +++ > 2 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 74156e2..7aa24aa 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9530,12 +9530,19 @@ void intel_modeset_cleanup(struct drm_device *dev) > struct drm_crtc *crtc; > struct intel_crtc *intel_crtc; > > + /* > + * Interrupts and polling as the first thing to avoid creating havoc. > + * Too much stuff here (turning of rps, connectors, ...) would > + * experience fancy races otherwise. > + */ > drm_kms_helper_poll_fini(dev); > + drm_irq_uninstall(dev); > + cancel_work_sync(&dev_priv->hotplug_work); > + Reviewed-by: Jani Nikula <jani.nikula@intel.com> However, not strictly part of this patch, it seems to me that theoretically it's possible (though very unlikely) polling gets disabled, hotplug irq fires and queues hotplug work, irqs get disabled, hotplug work runs (the cancel might just wait for it to finish), and re-enables polling by calling drm_kms_helper_poll_enable()... > mutex_lock(&dev->struct_mutex); > > intel_unregister_dsm_handler(); > > - > list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > /* Skip inactive CRTCs */ > if (!crtc->fb) > @@ -9553,12 +9560,6 @@ void intel_modeset_cleanup(struct drm_device *dev) > > mutex_unlock(&dev->struct_mutex); > > - /* Disable the irq before mode object teardown, for the irq might > - * enqueue unpin/hotplug work. */ > - drm_irq_uninstall(dev); > - cancel_work_sync(&dev_priv->hotplug_work); > - cancel_work_sync(&dev_priv->rps.work); > - > /* flush any delayed tasks or pending work */ > flush_scheduled_work(); > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 8b7f050..bb45122 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3671,6 +3671,9 @@ void intel_disable_gt_powersave(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + /* Interrupts should be disabled already to avoid re-arming. */ > + WARN_ON(dev->irq_enabled); > + > if (IS_IRONLAKE_M(dev)) { > ironlake_disable_drps(dev); > ironlake_disable_rc6(dev); > -- > 1.7.10.4
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 74156e2..7aa24aa 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9530,12 +9530,19 @@ void intel_modeset_cleanup(struct drm_device *dev) struct drm_crtc *crtc; struct intel_crtc *intel_crtc; + /* + * Interrupts and polling as the first thing to avoid creating havoc. + * Too much stuff here (turning of rps, connectors, ...) would + * experience fancy races otherwise. + */ drm_kms_helper_poll_fini(dev); + drm_irq_uninstall(dev); + cancel_work_sync(&dev_priv->hotplug_work); + mutex_lock(&dev->struct_mutex); intel_unregister_dsm_handler(); - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { /* Skip inactive CRTCs */ if (!crtc->fb) @@ -9553,12 +9560,6 @@ void intel_modeset_cleanup(struct drm_device *dev) mutex_unlock(&dev->struct_mutex); - /* Disable the irq before mode object teardown, for the irq might - * enqueue unpin/hotplug work. */ - drm_irq_uninstall(dev); - cancel_work_sync(&dev_priv->hotplug_work); - cancel_work_sync(&dev_priv->rps.work); - /* flush any delayed tasks or pending work */ flush_scheduled_work(); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 8b7f050..bb45122 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3671,6 +3671,9 @@ void intel_disable_gt_powersave(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + /* Interrupts should be disabled already to avoid re-arming. */ + WARN_ON(dev->irq_enabled); + if (IS_IRONLAKE_M(dev)) { ironlake_disable_drps(dev); ironlake_disable_rc6(dev);
Our rps code relies on the interrupts being off to prevent re-arming of the work items at inopportune moments. Also drop the redundant cancel_work for the main rps work, disable_gt_powersave already takes care of that. Finally add a WARN_ON to ensure we obey that piece of ordering constraint. Long term I want to lock down the setup/teardown code in a similar way to how we painstakingly check modeset sequence constraints already. Cc: Jesse Barnes <jbarnes@virtuousgeek.org> Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 15 ++++++++------- drivers/gpu/drm/i915/intel_pm.c | 3 +++ 2 files changed, 11 insertions(+), 7 deletions(-)