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