Re: [PATCH] drm/i915: Suppress hotplug work during PM suspend/resume

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 19, 2012 at 06:10:18PM +0200, Takashi Iwai wrote:
> The hotplug work can be still kicked off via irq during PM, and this
> may conflict with the resume procedure.  For example, eDP on SNB
> machine shows WARNING like below during the resume:
> 
>   WARNING: at /usr/src/packages/BUILD/kernel-default-3.0.13/linux-3.0/drivers/gpu/drm/i915/intel_dp.c:332 intel_dp_check_edp+0x73/0xd0 [i915]()
>   Hardware name: HP Z1 Workstation
>   eDP powered off while attempting aux channel communication.
>   Supported: Yes
>   Pid: 3210, comm: kworker/u:49 Tainted: G         C N  3.0.13-0.27-default #1
>   Call Trace:
>    [<ffffffff810048b5>] dump_trace+0x75/0x300
>    [<ffffffff8143ea0f>] dump_stack+0x69/0x6f
>    [<ffffffff81059e2b>] warn_slowpath_common+0x7b/0xc0
>    [<ffffffff81059f25>] warn_slowpath_fmt+0x45/0x50
>    [<ffffffffa01fa9f3>] intel_dp_check_edp+0x73/0xd0 [i915]
>    [<ffffffffa01fae4b>] intel_dp_aux_native_write+0x1b/0xe0 [i915]
>    [<ffffffffa01fb033>] intel_dp_set_link_train+0x73/0xa0 [i915]
>    [<ffffffffa01fb58e>] intel_dp_start_link_train+0x16e/0x400 [i915]
>    [<ffffffffa01fbc6c>] intel_dp_complete_link_train+0x1fc/0x3d0 [i915]
>    [<ffffffffa01fcf4c>] intel_dp_check_link_status+0x12c/0x1d0 [i915]
>    [<ffffffffa01cf22e>] i915_hotplug_work_func+0x6e/0xa0 [i915]
>    [<ffffffff810747bc>] process_one_work+0x16c/0x350
>    [<ffffffff8107734a>] worker_thread+0x17a/0x410
>    [<ffffffff8107b676>] kthread+0x96/0xa0
>    [<ffffffff8144a7c4>] kernel_thread_helper+0x4/0x10
>   DWARF2 unwinder stuck at kernel_thread_helper+0x4/0x10
> 
> This patch adds a flag to disable the hotplug during PM operation for
> avoiding such a race.
> 
> Cc: <stable@kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

I haven't looked to closely, but isn't cancelling the hotplug work after
we disable the irqs in the suspend path good enough? This here feels a bit
like ducttapeing over the problem.

I'm asking because we seem to have other problems with work queue items
that leak across s/r and cause havoc on resume. So extracting this
quiescenting code from module unload and also running it at suspend time
sounds more like the right thing.
-Daniel

> ---
> 
> It's a pretty simplistic solution that just ignores the hotplug work.
> We may keep some pending flag and process it later, if ignoring the
> event matters...
> 
>  drivers/gpu/drm/i915/i915_drv.c |    2 ++
>  drivers/gpu/drm/i915/i915_drv.h |    1 +
>  drivers/gpu/drm/i915/i915_irq.c |    9 ++++++---
>  3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index ae8a64f..dcbc8be 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -471,6 +471,7 @@ static int i915_drm_freeze(struct drm_device *dev)
>  
>  	/* Modeset on resume, not lid events */
>  	dev_priv->modeset_on_lid = 0;
> +	dev_priv->disable_hotplug_in_pm = true;
>  
>  	console_lock();
>  	intel_fbdev_set_suspend(dev, 1);
> @@ -548,6 +549,7 @@ static int i915_drm_thaw(struct drm_device *dev)
>  
>  	intel_opregion_init(dev);
>  
> +	dev_priv->disable_hotplug_in_pm = false;
>  	dev_priv->modeset_on_lid = 0;
>  
>  	console_lock();
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5fabc6c..80d38ce 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -360,6 +360,7 @@ typedef struct drm_i915_private {
>  	u32 gt_irq_mask;
>  	u32 pch_irq_mask;
>  
> +	bool disable_hotplug_in_pm;
>  	u32 hotplug_supported_mask;
>  	struct work_struct hotplug_work;
>  
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index afd4e03..f56f4bc 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -527,7 +527,8 @@ static irqreturn_t ivybridge_irq_handler(DRM_IRQ_ARGS)
>  
>  	/* check event from PCH */
>  	if (de_iir & DE_PCH_EVENT_IVB) {
> -		if (pch_iir & SDE_HOTPLUG_MASK_CPT)
> +		if (!dev_priv->disable_hotplug_in_pm &&
> +		    (pch_iir & SDE_HOTPLUG_MASK_CPT))
>  			queue_work(dev_priv->wq, &dev_priv->hotplug_work);
>  		pch_irq_handler(dev);
>  	}
> @@ -627,7 +628,8 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
>  
>  	/* check event from PCH */
>  	if (de_iir & DE_PCH_EVENT) {
> -		if (pch_iir & hotplug_mask)
> +		if (!dev_priv->disable_hotplug_in_pm &&
> +		    (pch_iir & hotplug_mask))
>  			queue_work(dev_priv->wq, &dev_priv->hotplug_work);
>  		pch_irq_handler(dev);
>  	}
> @@ -1345,7 +1347,8 @@ static irqreturn_t i915_driver_irq_handler(DRM_IRQ_ARGS)
>  
>  			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
>  				  hotplug_status);
> -			if (hotplug_status & dev_priv->hotplug_supported_mask)
> +			if (!dev_priv->disable_hotplug_in_pm &&
> +			    (hotplug_status & dev_priv->hotplug_supported_mask))
>  				queue_work(dev_priv->wq,
>  					   &dev_priv->hotplug_work);
>  
> -- 
> 1.7.9.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48