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