Re: [Intel-gfx] [PATCH v6 3/4] drm/i915/display: add hotplug.suspended flag

[Imre Deak <imre.deak@intel.com>]

On Fri, Jul 22, 2022 at 02:51:42PM +0200, Andrzej Hajda wrote:
> HPD events during driver removal can be generated by hardware and
> software frameworks - drm_dp_mst, the former we can avoid by disabling
> interrupts, the latter can be triggered by any drm_dp_mst transaction,
> and this is too late. Introducing suspended flag allows to solve this
> chicken-egg problem.

intel_hpd_cancel_work() is always called after suspending MST and
disabling IRQs (with the order I suggested in patch 1). If both of these
have disabled the corresponding functionality (MST, IRQs) properly with
all their MST/IRQ scheduled works guaranteed to not get rescheduled,
then it's not clear how could either intel_hpd_trigger_irq() or an IRQ
work run. So the problematic sequence would need a better explanation.

There's also already
dev_priv->runtime_pm.irqs_enabled
showing if hotplug interrupts are disabled (along with all other IRQs).

> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5950
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  2 +-
>  drivers/gpu/drm/i915/display/intel_hotplug.c | 11 ++++++++++-
>  drivers/gpu/drm/i915/display/intel_hotplug.h |  2 +-
>  drivers/gpu/drm/i915/i915_driver.c           |  4 ++--
>  drivers/gpu/drm/i915/i915_drv.h              |  2 ++
>  5 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index f1c765ac7ab8aa..cd6139bb36151b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -9022,7 +9022,7 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915)
>  	intel_dp_mst_suspend(i915);
>  
>  	/* MST is the last user of HPD work */
> -	intel_hpd_cancel_work(i915);
> +	intel_hpd_suspend(i915);
>  
>  	/* poll work can call into fbdev, hence clean that up afterwards */
>  	intel_fbdev_fini(i915);
> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c
> index 5f8b4f481cff9a..e1d384cb99df6b 100644
> --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
> @@ -303,6 +303,8 @@ static void i915_digport_work_func(struct work_struct *work)
>  	u32 old_bits = 0;
>  
>  	spin_lock_irq(&dev_priv->irq_lock);
> +	if (dev_priv->hotplug.suspended)
> +		return spin_unlock_irq(&dev_priv->irq_lock);
>  	long_port_mask = dev_priv->hotplug.long_port_mask;
>  	dev_priv->hotplug.long_port_mask = 0;
>  	short_port_mask = dev_priv->hotplug.short_port_mask;
> @@ -353,6 +355,8 @@ void intel_hpd_trigger_irq(struct intel_digital_port *dig_port)
>  	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
>  
>  	spin_lock_irq(&i915->irq_lock);
> +	if (i915->hotplug.suspended)
> +		return spin_unlock_irq(&i915->irq_lock);
>  	i915->hotplug.short_port_mask |= BIT(dig_port->base.port);
>  	spin_unlock_irq(&i915->irq_lock);
>  
> @@ -475,6 +479,9 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
>  
>  	spin_lock(&dev_priv->irq_lock);
>  
> +	if (dev_priv->hotplug.suspended)
> +		return spin_unlock(&dev_priv->irq_lock);
> +
>  	/*
>  	 * Determine whether ->hpd_pulse() exists for each pin, and
>  	 * whether we have a short or a long pulse. This is needed
> @@ -603,6 +610,7 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
>  	 * just to make the assert_spin_locked checks happy.
>  	 */
>  	spin_lock_irq(&dev_priv->irq_lock);
> +	dev_priv->hotplug.suspended = false;
>  	intel_hpd_irq_setup(dev_priv);
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
> @@ -721,13 +729,14 @@ void intel_hpd_init_work(struct drm_i915_private *dev_priv)
>  			  intel_hpd_irq_storm_reenable_work);
>  }
>  
> -void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
> +void intel_hpd_suspend(struct drm_i915_private *dev_priv)
>  {
>  	if (!HAS_DISPLAY(dev_priv))
>  		return;
>  
>  	spin_lock_irq(&dev_priv->irq_lock);
>  
> +	dev_priv->hotplug.suspended = true;
>  	dev_priv->hotplug.long_port_mask = 0;
>  	dev_priv->hotplug.short_port_mask = 0;
>  	dev_priv->hotplug.event_bits = 0;
> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.h b/drivers/gpu/drm/i915/display/intel_hotplug.h
> index b87e95d606e668..54bddc4dd63421 100644
> --- a/drivers/gpu/drm/i915/display/intel_hotplug.h
> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.h
> @@ -23,7 +23,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
>  void intel_hpd_trigger_irq(struct intel_digital_port *dig_port);
>  void intel_hpd_init(struct drm_i915_private *dev_priv);
>  void intel_hpd_init_work(struct drm_i915_private *dev_priv);
> -void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
> +void intel_hpd_suspend(struct drm_i915_private *dev_priv);
>  enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
>  				   enum port port);
>  bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin);
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index deb8a8b76965a1..57a063a306e3a4 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1092,7 +1092,7 @@ void i915_driver_shutdown(struct drm_i915_private *i915)
>  	intel_dp_mst_suspend(i915);
>  
>  	intel_runtime_pm_disable_interrupts(i915);
> -	intel_hpd_cancel_work(i915);
> +	intel_hpd_suspend(i915);
>  
>  	intel_suspend_encoders(i915);
>  	intel_shutdown_encoders(i915);
> @@ -1161,7 +1161,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>  	intel_dp_mst_suspend(dev_priv);
>  
>  	intel_runtime_pm_disable_interrupts(dev_priv);
> -	intel_hpd_cancel_work(dev_priv);
> +	intel_hpd_suspend(dev_priv);
>  
>  	intel_suspend_encoders(dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d25647be25d18b..dc1562b95d7ade 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -106,6 +106,8 @@ struct vlv_s0ix_state;
>  #define HPD_STORM_DEFAULT_THRESHOLD 50
>  
>  struct i915_hotplug {
> +	bool suspended;
> +
>  	struct delayed_work hotplug_work;
>  
>  	const u32 *hpd, *pch_hpd;
> -- 
> 2.25.1
>