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@xxxxxxxxx>
> ---
> 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);
>
