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

[Jani Nikula <jani.nikula@intel.com>]

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