| Message ID | 20240104083008.2715733-3-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix HPD handling during driver init/shutdown | expand |
On Thu, 2024-01-04 at 10:29 +0200, Imre Deak wrote: > If an HPD IRQ storm is detected on a connector during driver loading > or > system suspend/resume - disabling the IRQ and switching to polling - > the > polling may get disabled too early - before the intended 2 minute > HPD_STORM_REENABLE_DELAY - with the HPD IRQ staying disabled for this > duration. One such sequence is: > > Thread#1 Thread#2 > intel_display_driver_probe()-> > intel_hpd_init()-> > (HPD IRQ gets enabled) > . intel_hpd_irq_handler()-> > . > intel_hpd_irq_storm_detect() > . intel_hpd_irq_setup()- > > > . (HPD IRQ gets > disabled) > . > queue_delayed_work(hotplug.hotplug_work) > . ... > . i915_hotplug_work_func()- > > > . > intel_hpd_irq_storm_switch_to_polling()-> > . (polling enabled) > . > intel_hpd_poll_disable()-> > queue_work(hotplug.poll_init_work) > ... > i915_hpd_poll_init_work()-> > (polling gets disabled, > HPD IRQ is still disabled) > ... > > (Connector is neither polled or > detected via HPD IRQs for 2 minutes) > > intel_hpd_irq_storm_reenable_work()-> > (HPD IRQ gets enabled) > > To avoid the above 2 minute state without either polling or enabled > HPD > IRQ, leave the connector's polling mode unchanged in > i915_hpd_poll_init_work() if its HPD IRQ got disabled after an IRQ > storm > indicated by the connector's HPD_DISABLED pin state. Is it actually order which needs to be ensured here? I.e. ensure that polling is disabled before hpd interrupt gets enabled? Why disabling polling is queued work and not just done during init/resume? BR, Jouni Högander > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/display/intel_hotplug.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c > b/drivers/gpu/drm/i915/display/intel_hotplug.c > index 0c0700c6ec66d..74513c3d3690b 100644 > --- a/drivers/gpu/drm/i915/display/intel_hotplug.c > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c > @@ -710,6 +710,8 @@ static void i915_hpd_poll_init_work(struct > work_struct *work) > cancel_work(&dev_priv- > >display.hotplug.poll_init_work); > } > > + spin_lock_irq(&dev_priv->irq_lock); > + > drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > for_each_intel_connector_iter(connector, &conn_iter) { > enum hpd_pin pin; > @@ -718,6 +720,9 @@ static void i915_hpd_poll_init_work(struct > work_struct *work) > if (pin == HPD_NONE) > continue; > > + if (dev_priv->display.hotplug.stats[pin].state == > HPD_DISABLED) > + continue; > + > connector->base.polled = connector->polled; > > if (enabled && connector->base.polled == > DRM_CONNECTOR_POLL_HPD) > @@ -726,6 +731,8 @@ static void i915_hpd_poll_init_work(struct > work_struct *work) > } > drm_connector_list_iter_end(&conn_iter); > > + spin_unlock_irq(&dev_priv->irq_lock); > + > if (enabled) > drm_kms_helper_poll_reschedule(&dev_priv->drm); >
On Fri, Jan 05, 2024 at 03:23:49PM +0200, Hogander, Jouni wrote: > On Thu, 2024-01-04 at 10:29 +0200, Imre Deak wrote: > > If an HPD IRQ storm is detected on a connector during driver loading or > > system suspend/resume - disabling the IRQ and switching to polling - the > > polling may get disabled too early - before the intended 2 minute > > HPD_STORM_REENABLE_DELAY - with the HPD IRQ staying disabled for this > > duration. One such sequence is: > > > > Thread#1 Thread#2 > > intel_display_driver_probe()-> > > intel_hpd_init()-> > > (HPD IRQ gets enabled) > > . intel_hpd_irq_handler()-> > > . intel_hpd_irq_storm_detect() > > . intel_hpd_irq_setup()-> > > . (HPD IRQ gets disabled) > > . queue_delayed_work(hotplug.hotplug_work) > > . ... > > . i915_hotplug_work_func()-> > > . intel_hpd_irq_storm_switch_to_polling()-> > > . (polling enabled) > > . > > intel_hpd_poll_disable()-> > > queue_work(hotplug.poll_init_work) > > ... > > i915_hpd_poll_init_work()-> > > (polling gets disabled, > > HPD IRQ is still disabled) > > ... > > > > (Connector is neither polled or > > detected via HPD IRQs for 2 minutes) > > > > intel_hpd_irq_storm_reenable_work()-> > > (HPD IRQ gets enabled) > > > > To avoid the above 2 minute state without either polling or enabled HPD > > IRQ, leave the connector's polling mode unchanged in > > i915_hpd_poll_init_work() if its HPD IRQ got disabled after an IRQ storm > > indicated by the connector's HPD_DISABLED pin state. > > Is it actually order which needs to be ensured here? I.e. ensure that > polling is disabled before hpd interrupt gets enabled? Disabling the polling also means that there is an explicit detection of the connectors. This explicit detection at boot-up and resume must happen _after_ the HPD interrupts are enabled, otherwise you could miss an HPD connect/disconnect interrupt and leave the connector in a stale connected state. > Why disabling polling is queued work and not just done during init/resume? To reduce the latency of boot-up and resume. > BR, > > Jouni Högander > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_hotplug.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c > > b/drivers/gpu/drm/i915/display/intel_hotplug.c > > index 0c0700c6ec66d..74513c3d3690b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_hotplug.c > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c > > @@ -710,6 +710,8 @@ static void i915_hpd_poll_init_work(struct > > work_struct *work) > > cancel_work(&dev_priv- > > >display.hotplug.poll_init_work); > > } > > > > + spin_lock_irq(&dev_priv->irq_lock); > > + > > drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > > for_each_intel_connector_iter(connector, &conn_iter) { > > enum hpd_pin pin; > > @@ -718,6 +720,9 @@ static void i915_hpd_poll_init_work(struct > > work_struct *work) > > if (pin == HPD_NONE) > > continue; > > > > + if (dev_priv->display.hotplug.stats[pin].state == > > HPD_DISABLED) > > + continue; > > + > > connector->base.polled = connector->polled; > > > > if (enabled && connector->base.polled == > > DRM_CONNECTOR_POLL_HPD) > > @@ -726,6 +731,8 @@ static void i915_hpd_poll_init_work(struct > > work_struct *work) > > } > > drm_connector_list_iter_end(&conn_iter); > > > > + spin_unlock_irq(&dev_priv->irq_lock); > > + > > if (enabled) > > drm_kms_helper_poll_reschedule(&dev_priv->drm); > > >
On Fri, 2024-01-05 at 15:38 +0200, Imre Deak wrote: > On Fri, Jan 05, 2024 at 03:23:49PM +0200, Hogander, Jouni wrote: > > On Thu, 2024-01-04 at 10:29 +0200, Imre Deak wrote: > > > If an HPD IRQ storm is detected on a connector during driver > > > loading or > > > system suspend/resume - disabling the IRQ and switching to > > > polling - the > > > polling may get disabled too early - before the intended 2 minute > > > HPD_STORM_REENABLE_DELAY - with the HPD IRQ staying disabled for > > > this > > > duration. One such sequence is: > > > > > > Thread#1 Thread#2 > > > intel_display_driver_probe()-> > > > intel_hpd_init()-> > > > (HPD IRQ gets enabled) > > > . > > > intel_hpd_irq_handler()-> > > > . > > > intel_hpd_irq_storm_detect() > > > . > > > intel_hpd_irq_setup()-> > > > . (HPD IRQ gets > > > disabled) > > > . > > > queue_delayed_work(hotplug.hotplug_work) > > > . ... > > > . > > > i915_hotplug_work_func()-> > > > . > > > intel_hpd_irq_storm_switch_to_polling()-> > > > . (polling enabled) > > > . > > > intel_hpd_poll_disable()-> > > > queue_work(hotplug.poll_init_work) > > > ... > > > i915_hpd_poll_init_work()-> > > > (polling gets disabled, > > > HPD IRQ is still disabled) > > > ... > > > > > > (Connector is neither polled or > > > detected via HPD IRQs for 2 minutes) > > > > > > intel_hpd_irq_storm_reenable_work()-> > > > (HPD IRQ gets enabled) > > > > > > To avoid the above 2 minute state without either polling or > > > enabled HPD > > > IRQ, leave the connector's polling mode unchanged in > > > i915_hpd_poll_init_work() if its HPD IRQ got disabled after an > > > IRQ storm > > > indicated by the connector's HPD_DISABLED pin state. > > > > Is it actually order which needs to be ensured here? I.e. ensure > > that > > polling is disabled before hpd interrupt gets enabled? > > Disabling the polling also means that there is an explicit detection > of > the connectors. This explicit detection at boot-up and resume must > happen _after_ the HPD interrupts are enabled, otherwise you could > miss > an HPD connect/disconnect interrupt and leave the connector in a > stale > connected state. For that purpose i915_hpd_poll_detect_connectors could be used. Anyways I'm not sure if that would be any better. To me it looks like simplest/cleanest way to tackle the issue described in the commit message is in the patch: Reviewed-by: Jouni Högander <jouni.hogander@intel.com> > > Why disabling polling is queued work and not just done during > > init/resume? > > To reduce the latency of boot-up and resume. > > > BR, > > > > Jouni Högander > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_hotplug.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c > > > b/drivers/gpu/drm/i915/display/intel_hotplug.c > > > index 0c0700c6ec66d..74513c3d3690b 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_hotplug.c > > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c > > > @@ -710,6 +710,8 @@ static void i915_hpd_poll_init_work(struct > > > work_struct *work) > > > cancel_work(&dev_priv- > > > > display.hotplug.poll_init_work); > > > } > > > > > > + spin_lock_irq(&dev_priv->irq_lock); > > > + > > > drm_connector_list_iter_begin(&dev_priv->drm, > > > &conn_iter); > > > for_each_intel_connector_iter(connector, &conn_iter) { > > > enum hpd_pin pin; > > > @@ -718,6 +720,9 @@ static void i915_hpd_poll_init_work(struct > > > work_struct *work) > > > if (pin == HPD_NONE) > > > continue; > > > > > > + if (dev_priv->display.hotplug.stats[pin].state == > > > HPD_DISABLED) > > > + continue; > > > + > > > connector->base.polled = connector->polled; > > > > > > if (enabled && connector->base.polled == > > > DRM_CONNECTOR_POLL_HPD) > > > @@ -726,6 +731,8 @@ static void i915_hpd_poll_init_work(struct > > > work_struct *work) > > > } > > > drm_connector_list_iter_end(&conn_iter); > > > > > > + spin_unlock_irq(&dev_priv->irq_lock); > > > + > > > if (enabled) > > > drm_kms_helper_poll_reschedule(&dev_priv->drm); > > > > >
On Fri, Jan 05, 2024 at 04:08:31PM +0200, Hogander, Jouni wrote: > On Fri, 2024-01-05 at 15:38 +0200, Imre Deak wrote: > > On Fri, Jan 05, 2024 at 03:23:49PM +0200, Hogander, Jouni wrote: > > > On Thu, 2024-01-04 at 10:29 +0200, Imre Deak wrote: > > > > If an HPD IRQ storm is detected on a connector during driver > > > > loading or > > > > system suspend/resume - disabling the IRQ and switching to > > > > polling - the > > > > polling may get disabled too early - before the intended 2 minute > > > > HPD_STORM_REENABLE_DELAY - with the HPD IRQ staying disabled for > > > > this > > > > duration. One such sequence is: > > > > > > > > Thread#1 Thread#2 > > > > intel_display_driver_probe()-> > > > > intel_hpd_init()-> > > > > (HPD IRQ gets enabled) > > > > . intel_hpd_irq_handler()-> > > > > . intel_hpd_irq_storm_detect() > > > > . intel_hpd_irq_setup()-> > > > > . (HPD IRQ gets disabled) > > > > . queue_delayed_work(hotplug.hotplug_work) > > > > . ... > > > > . i915_hotplug_work_func()-> > > > > . intel_hpd_irq_storm_switch_to_polling()-> > > > > . (polling enabled) > > > > . > > > > intel_hpd_poll_disable()-> > > > > queue_work(hotplug.poll_init_work) > > > > ... > > > > i915_hpd_poll_init_work()-> > > > > (polling gets disabled, > > > > HPD IRQ is still disabled) > > > > ... > > > > > > > > (Connector is neither polled or > > > > detected via HPD IRQs for 2 minutes) > > > > > > > > intel_hpd_irq_storm_reenable_work()-> > > > > (HPD IRQ gets enabled) > > > > > > > > To avoid the above 2 minute state without either polling or > > > > enabled HPD IRQ, leave the connector's polling mode unchanged in > > > > i915_hpd_poll_init_work() if its HPD IRQ got disabled after an > > > > IRQ storm indicated by the connector's HPD_DISABLED pin state. > > > > > > Is it actually order which needs to be ensured here? I.e. ensure > > > that polling is disabled before hpd interrupt gets enabled? > > > > Disabling the polling also means that there is an explicit detection > > of the connectors. This explicit detection at boot-up and resume must > > happen _after_ the HPD interrupts are enabled, otherwise you could > > miss an HPD connect/disconnect interrupt and leave the connector in a > > stale connected state. > > For that purpose i915_hpd_poll_detect_connectors could be used. Yes, that's the only purpose of intel_hpd_poll_disable() during boot-up and resume (as I mention in a later patch in the patchset). Maybe the function name is not the best (since at those points polling is anyway disabled), but the point is to have an explicit asynchronous detect (to avoid the overhead), which is the same thing happening during runtime whenever polling needs to be disabled (due to power well/rpm toggling). > Anyways I'm not sure if that would be any better. > To me it looks like simplest/cleanest way to tackle the issue described > in the commit message is in the patch: > > Reviewed-by: Jouni Högander <jouni.hogander@intel.com> > > > > Why disabling polling is queued work and not just done during > > > init/resume? > > > > To reduce the latency of boot-up and resume. > > > > > BR, > > > > > > Jouni Högander > > > > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_hotplug.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c > > > > b/drivers/gpu/drm/i915/display/intel_hotplug.c > > > > index 0c0700c6ec66d..74513c3d3690b 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_hotplug.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c > > > > @@ -710,6 +710,8 @@ static void i915_hpd_poll_init_work(struct > > > > work_struct *work) > > > > cancel_work(&dev_priv- > > > > > display.hotplug.poll_init_work); > > > > } > > > > > > > > + spin_lock_irq(&dev_priv->irq_lock); > > > > + > > > > drm_connector_list_iter_begin(&dev_priv->drm, > > > > &conn_iter); > > > > for_each_intel_connector_iter(connector, &conn_iter) { > > > > enum hpd_pin pin; > > > > @@ -718,6 +720,9 @@ static void i915_hpd_poll_init_work(struct > > > > work_struct *work) > > > > if (pin == HPD_NONE) > > > > continue; > > > > > > > > + if (dev_priv->display.hotplug.stats[pin].state == > > > > HPD_DISABLED) > > > > + continue; > > > > + > > > > connector->base.polled = connector->polled; > > > > > > > > if (enabled && connector->base.polled == > > > > DRM_CONNECTOR_POLL_HPD) > > > > @@ -726,6 +731,8 @@ static void i915_hpd_poll_init_work(struct > > > > work_struct *work) > > > > } > > > > drm_connector_list_iter_end(&conn_iter); > > > > > > > > + spin_unlock_irq(&dev_priv->irq_lock); > > > > + > > > > if (enabled) > > > > drm_kms_helper_poll_reschedule(&dev_priv->drm); > > > > > > > >
diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c index 0c0700c6ec66d..74513c3d3690b 100644 --- a/drivers/gpu/drm/i915/display/intel_hotplug.c +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c @@ -710,6 +710,8 @@ static void i915_hpd_poll_init_work(struct work_struct *work) cancel_work(&dev_priv->display.hotplug.poll_init_work); } + spin_lock_irq(&dev_priv->irq_lock); + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); for_each_intel_connector_iter(connector, &conn_iter) { enum hpd_pin pin; @@ -718,6 +720,9 @@ static void i915_hpd_poll_init_work(struct work_struct *work) if (pin == HPD_NONE) continue; + if (dev_priv->display.hotplug.stats[pin].state == HPD_DISABLED) + continue; + connector->base.polled = connector->polled; if (enabled && connector->base.polled == DRM_CONNECTOR_POLL_HPD) @@ -726,6 +731,8 @@ static void i915_hpd_poll_init_work(struct work_struct *work) } drm_connector_list_iter_end(&conn_iter); + spin_unlock_irq(&dev_priv->irq_lock); + if (enabled) drm_kms_helper_poll_reschedule(&dev_priv->drm);
If an HPD IRQ storm is detected on a connector during driver loading or system suspend/resume - disabling the IRQ and switching to polling - the polling may get disabled too early - before the intended 2 minute HPD_STORM_REENABLE_DELAY - with the HPD IRQ staying disabled for this duration. One such sequence is: Thread#1 Thread#2 intel_display_driver_probe()-> intel_hpd_init()-> (HPD IRQ gets enabled) . intel_hpd_irq_handler()-> . intel_hpd_irq_storm_detect() . intel_hpd_irq_setup()-> . (HPD IRQ gets disabled) . queue_delayed_work(hotplug.hotplug_work) . ... . i915_hotplug_work_func()-> . intel_hpd_irq_storm_switch_to_polling()-> . (polling enabled) . intel_hpd_poll_disable()-> queue_work(hotplug.poll_init_work) ... i915_hpd_poll_init_work()-> (polling gets disabled, HPD IRQ is still disabled) ... (Connector is neither polled or detected via HPD IRQs for 2 minutes) intel_hpd_irq_storm_reenable_work()-> (HPD IRQ gets enabled) To avoid the above 2 minute state without either polling or enabled HPD IRQ, leave the connector's polling mode unchanged in i915_hpd_poll_init_work() if its HPD IRQ got disabled after an IRQ storm indicated by the connector's HPD_DISABLED pin state. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_hotplug.c | 7 +++++++ 1 file changed, 7 insertions(+)