[PATCH] drm/i915: disable interrupts earlier in the driver unload code

[PATCH] drm/i915: disable interrupts earlier in the driver unload code

[Daniel Vetter]

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);
+
 	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

Re: [PATCH] drm/i915: disable interrupts earlier in the driver unload code

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: disable interrupts earlier in the driver unload code

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: disable interrupts earlier in the driver unload code

[Jani Nikula]

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

[PATCH] drm/i915: disable interrupts earlier in the driver unload code

[Daniel Vetter]

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.

v2: Disable polling after hpd handling is shut down - since Egbert's
hpd irq storm handling the hotplug work can re-arm the polling
handler. Spotted by Jani Nikula.

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 |   19 ++++++++++++-------
 drivers/gpu/drm/i915/intel_pm.c      |    3 +++
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 74156e2..988e543 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9530,12 +9530,23 @@ 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_irq_uninstall(dev);
+	cancel_work_sync(&dev_priv->hotplug_work);
+	/*
+	 * Due to the hpd irq storm handling the hotplug work can re-arm the
+	 * poll handlers. Hence disable polling after hpd handling is shut down.
+	 */
 	drm_kms_helper_poll_fini(dev);
+
 	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 +9564,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

Re: [PATCH] drm/i915: disable interrupts earlier in the driver unload code

[Jani Nikula]

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.
>
> v2: Disable polling after hpd handling is shut down - since Egbert's
> hpd irq storm handling the hotplug work can re-arm the polling
> handler. Spotted by Jani Nikula.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> 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 |   19 ++++++++++++-------
>  drivers/gpu/drm/i915/intel_pm.c      |    3 +++
>  2 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 74156e2..988e543 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9530,12 +9530,23 @@ 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_irq_uninstall(dev);
> +	cancel_work_sync(&dev_priv->hotplug_work);
> +	/*
> +	 * Due to the hpd irq storm handling the hotplug work can re-arm the
> +	 * poll handlers. Hence disable polling after hpd handling is shut down.
> +	 */
>  	drm_kms_helper_poll_fini(dev);
> +
>  	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 +9564,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