| Message ID | 20220722125143.1604709-4-andrzej.hajda@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: stop HPD workers before display driver unregister | expand |
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 >
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 >>
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. > 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@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 > > > >
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@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 >>>> >>
On Thu, Aug 25, 2022 at 01:24:04PM +0200, Andrzej Hajda wrote: > 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? In theory all these should be blocked already, after i915_driver_unregister() has returned; for the above cases in particular via drm_connector_unregister_all(). > - maybe some mode_config disabling code (for example intel_mst_disable_dp) - This should be called already via i915_driver_unregister() -> intel_display_driver_unregister() -> drm_atomic_helper_shutdown(). > intel_mode_config_cleanup is called after intel_dp_mst_suspend. This should only free the allocated objects etc, but not do any transactions. > 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. Ok. I suppose blocking detection based on a new flag would mean avoiding scheduling and flushing hotplug.hotplug_work already as the first step in intel_display_driver_unregister() and somewhere early during system suspend. > 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@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 > > > > > > > > >
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;