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

["Hogander, Jouni" <jouni.hogander@intel.com>]

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