From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH] drm/i915: disable interrupts earlier in the driver unload code Date: Wed, 24 Apr 2013 13:08:05 +0300 Message-ID: <87a9oodywq.fsf@intel.com> References: <1366795160-2427-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 85FD3E632D for ; Wed, 24 Apr 2013 03:08:11 -0700 (PDT) In-Reply-To: <1366795160-2427-1-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Intel Graphics Development Cc: Daniel Vetter List-Id: intel-gfx@lists.freedesktop.org On Wed, 24 Apr 2013, 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. > > Cc: Jesse Barnes > Cc: Jani Nikula > Signed-off-by: Daniel Vetter > --- > 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 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