Re: [PATCH 02/12] drm/i915: Keep the connector polled state disabled after storm

[Imre Deak <imre.deak@intel.com>]

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