Re: [PATCH 2/4] drm/i915: cancel hotplug and dig_port work during suspend and unload

Ville Syrjälä
2014-08-12T12:14:00Z

On Mon, Aug 11, 2014 at 09:54:15PM +0300, Imre Deak wrote:
> Make sure these work handlers don't run after we system suspend or
> unload the driver. Note that we don't cancel the handlers during runtime
> suspend. That could lead to a lockup, since we take a runtime PM ref
> from the handlers themselves. Fortunaltely canceling there is not needed
> since the RPM ref itself provides for the needed serialization.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 8 ++++++++
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_display.c | 3 +--
> 3 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index ec96f9a..0653761 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -494,6 +494,13 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
> return true;
> }
>
> +void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
> +{
> + cancel_work_sync(&dev_priv->hotplug_work);
> + cancel_work_sync(&dev_priv->dig_port_work);

Since dig_port_work can queue a hotplug_work shouldn't these two be
swapped?

I wonder if we should also clear hpd_event_bits and
{long,short}_hpd_port_mask before cancelling the works? At least it
might make the works end a bit quicker if the are already running.

I also noticed that we don't seem to grab any rpm/powerwell references
in ->hpd_pulse() or i915_digport_work_func(). That doesn't seem right.
Or maybe you already addressed that in another patch?

> + cancel_delayed_work_sync(&dev_priv->hotplug_reenable_work);
> +}
> +
> static int i915_drm_freeze(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -538,6 +545,7 @@ static int i915_drm_freeze(struct drm_device *dev)
> flush_delayed_work(&dev_priv->rps.delayed_resume_work);
>
> intel_runtime_pm_disable_interrupts(dev);
> + intel_hpd_cancel_work(dev_priv);
>
> intel_suspend_gt_powersave(dev);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 35d150a..8776847 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2218,6 +2218,7 @@ extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
> int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
>
> extern void intel_console_resume(struct work_struct *work);
> +void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
>
> /* i915_irq.c */
> void i915_queue_hangcheck(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b3a3168..ab90301 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13100,8 +13100,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
> * experience fancy races otherwise.
> */
> drm_irq_uninstall(dev);
> - cancel_work_sync(&dev_priv->hotplug_work);
> - cancel_delayed_work_sync(&dev_priv->hotplug_reenable_work);
> + intel_hpd_cancel_work(dev_priv);
> dev_priv->pm._irqs_disabled = true;
>
> /*
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Ville Syrjälä
Intel OTC