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