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

[Andrzej Hajda <andrzej.hajda@intel.com>]

On 22.08.2022 19:27, Imre Deak wrote:
> 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.

I am not familiar with MST but as I understand from earlier discussion 
MST framework can be called during driver removal code even after 
intel_dp_mst_suspend. And since MST transfer can timeout it can trigger 
drm_dp_mst_wait_tx_reply --> mgr->cbs->poll_hpd_irq(mgr) --> 
intel_dp_mst_poll_hpd_irq --> intel_hpd_trigger_irq.

So actually this patch supersedes the 1st patch.

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

So it is slightly different, this patch introduces flag indicating if 
HPD is enabled, we can have IRQs not related to HPD, and HPD events not 
related to IRQs.

Regards
Andrzej

>
>> 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
>>