[Intel-gfx] [PATCH v6 3/4] drm/i915/display: add hotplug.suspended flag
Andrzej Hajda
andrzej.hajda at intel.com
Thu Aug 25 11:24:04 UTC 2022
On 24.08.2022 13:22, Imre Deak wrote:
> On Tue, Aug 23, 2022 at 11:48:01PM +0200, Andrzej Hajda wrote:
>>
>>
>> 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.
>
> Not sure how that happens, but it looks wrong. One way I can imagine is
> that connector detection re-enables MST, which should be prevented then.
I am not MST expert and atm I have no access to the machine on which I
could look for real prove.
As I understand intel_dp_mst_suspend prevents only downstream devices to
initiate transactions, it does not prevent transactions initiated from
i915 driver.
My guesses for such transactions are:
- any ioctl/sysfs/drm_property access initiated by user which can
involve MST tranaction (set brightness, read EDID, reading capabilities,
statuses, ....), unless they are already blocked, how?
- maybe some mode_config disabling code (for example
intel_mst_disable_dp) - intel_mode_config_cleanup is called 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.
If such situation happens after intel_dp_mst_suspend
i915->hotplug.dig_port_work will be queued, and we have risk of
use-after-free.
If this analysis looks incorrect I can send patches 1, 2, 4 with your
comments addressed. CI probably will verify this anyway.
Regards
Andrzej
>
>> 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.
>
> In its current form I can't see a difference. What would make sense is
> to add a flag that prevents connector detection (which would
> incorrectly enable MST for instace), but leave the handling of other
> interrupts enabled. That flag would be set already before suspending
> MST.
>
>> Regards
>> Andrzej
>>
>>>
>>>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5950
>>>> Signed-off-by: Andrzej Hajda <andrzej.hajda at intel.com>
>>>> Reviewed-by: Arun R Murthy <arun.r.murthy at 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
>>>>
>>
More information about the Intel-gfx
mailing list