[Intel-gfx] [PATCH] drm/i915: delay hotplug scheduling

Fri Sep 28 15:36:20 UTC 2018

On Fri, 28 Sep 2018 09:26:48 +0300
Jani Nikula <jani.nikula at intel.com> wrote:

> On Thu, 27 Sep 2018, Guang Bai <guang.bai at intel.com> wrote:
> > On Thu, 27 Sep 2018 09:45:53 +0300
> > Jani Nikula <jani.nikula at intel.com> wrote:
> >  
> >> On Wed, 26 Sep 2018, Guang Bai <guang.bai at intel.com> wrote:  
> >> > On Wed, 26 Sep 2018 16:44:16 -0700
> >> > Guang Bai <guang.bai at intel.com> wrote:
> >> >    
> >> >> On Tue, 25 Sep 2018 10:18:36 +0300
> >> >> Jani Nikula <jani.nikula at intel.com> wrote:
> >> >>     
> >> >> > On some systems we get the hotplug irq on unplug before the
> >> >> > DDC pins have been disconnected, resulting in connector status
> >> >> > remaining connected. Since previous attempts at using hotplug
> >> >> > live status before DDC have failed, the only viable option is
> >> >> > to reduce the window for mistakes. Delay the hotplug
> >> >> > processing.
> >> >> > 
> >> >> > Reference:
> >> >> > http://mid.mail-archive.com/CAB4CAwcfj32FovZwpTbekvHpoc3gG0sVbNtW8jeXRcp5qwEsmw@mail.gmail.com
> >> >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107125
> >> >> > Reported-by: Chris Chiu <chiu at endlessm.com> Tested-by: Chris
> >> >> > Chiu <chiu at endlessm.com> Cc: Chris Chiu <chiu at endlessm.com>
> >> >> > Cc: Guang Bai <guang.bai at intel.com>
> >> >> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >> >> > Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> >> >> > ---
> >> >> >  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
> >> >> >  drivers/gpu/drm/i915/intel_hotplug.c | 16 +++++++++++-----
> >> >> >  2 files changed, 12 insertions(+), 6 deletions(-)
> >> >> > 
> >> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> >> > b/drivers/gpu/drm/i915/i915_drv.h index
> >> >> > 8624b4bdc242..4378be7a20d5 100644 ---
> >> >> > a/drivers/gpu/drm/i915/i915_drv.h +++
> >> >> > b/drivers/gpu/drm/i915/i915_drv.h @@ -286,7 +286,7 @@ enum
> >> >> > hpd_pin { #define HPD_STORM_DEFAULT_THRESHOLD 5
> >> >> >  
> >> >> >  struct i915_hotplug {
> >> >> > -	struct work_struct hotplug_work;
> >> >> > +	struct delayed_work hotplug_work;
> >> >> >  
> >> >> >  	struct {
> >> >> >  		unsigned long last_jiffies;
> >> >> > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c
> >> >> > b/drivers/gpu/drm/i915/intel_hotplug.c index
> >> >> > 648a13c6043c..ce9bedbedcd1 100644 ---
> >> >> > a/drivers/gpu/drm/i915/intel_hotplug.c +++
> >> >> > b/drivers/gpu/drm/i915/intel_hotplug.c @@ -110,6 +110,9 @@
> >> >> > enum hpd_pin intel_hpd_pin_default(struct drm_i915_private
> >> >> > *dev_priv, } }
> >> >> >  
> >> >> > +/* Delay between irq and hotplug detect processing */
> >> >> > +#define HOTPLUG_DELAY_MS		300
> >> >> > +
> >> >> >  #define HPD_STORM_DETECT_PERIOD		1000
> >> >> >  #define HPD_STORM_REENABLE_DELAY	(2 * 60 * 1000)
> >> >> >  
> >> >> > @@ -319,7 +322,8 @@ static void i915_digport_work_func(struct
> >> >> > work_struct *work) spin_lock_irq(&dev_priv->irq_lock);
> >> >> >  		dev_priv->hotplug.event_bits |= old_bits;
> >> >> >  		spin_unlock_irq(&dev_priv->irq_lock);
> >> >> > -
> >> >> > schedule_work(&dev_priv->hotplug.hotplug_work); +
> >> >> > schedule_delayed_work(&dev_priv->hotplug.hotplug_work,
> >> >> > +
> >> >> > msecs_to_jiffies(HOTPLUG_DELAY_MS)); }
> >> >> >  }
> >> >> >  
> >> >> > @@ -329,7 +333,7 @@ static void i915_digport_work_func(struct
> >> >> > work_struct *work) static void i915_hotplug_work_func(struct
> >> >> > work_struct *work) {
> >> >> >  	struct drm_i915_private *dev_priv =
> >> >> > -		container_of(work, struct drm_i915_private,
> >> >> > hotplug.hotplug_work);
> >> >> > +		container_of(work, struct drm_i915_private,
> >> >> > hotplug.hotplug_work.work); struct drm_device *dev =
> >> >> > &dev_priv->drm; struct intel_connector *intel_connector;
> >> >> >  	struct intel_encoder *intel_encoder;
> >> >> > @@ -466,7 +470,8 @@ void intel_hpd_irq_handler(struct
> >> >> > drm_i915_private *dev_priv, if (queue_dig)
> >> >> >  		queue_work(dev_priv->hotplug.dp_wq,
> >> >> > &dev_priv->hotplug.dig_port_work); if (queue_hp)
> >> >> > -
> >> >> > schedule_work(&dev_priv->hotplug.hotplug_work); +
> >> >> > schedule_delayed_work(&dev_priv->hotplug.hotplug_work,
> >> >> > +
> >> >> > msecs_to_jiffies(HOTPLUG_DELAY_MS)); }
> >> >> >  
> >> >> >  /**
> >> >> > @@ -586,7 +591,8 @@ void intel_hpd_poll_init(struct
> >> >> > drm_i915_private *dev_priv) 
> >> >> >  void intel_hpd_init_work(struct drm_i915_private *dev_priv)
> >> >> >  {
> >> >> > -	INIT_WORK(&dev_priv->hotplug.hotplug_work,
> >> >> > i915_hotplug_work_func);
> >> >> > +	INIT_DELAYED_WORK(&dev_priv->hotplug.hotplug_work,
> >> >> > +			  i915_hotplug_work_func);
> >> >> >  	INIT_WORK(&dev_priv->hotplug.dig_port_work,
> >> >> > i915_digport_work_func);
> >> >> > INIT_WORK(&dev_priv->hotplug.poll_init_work,
> >> >> > i915_hpd_poll_init_work);
> >> >> > INIT_DELAYED_WORK(&dev_priv->hotplug.reenable_work, @@ -604,7
> >> >> > +610,7 @@ void intel_hpd_cancel_work(struct drm_i915_private
> >> >> > *dev_priv) spin_unlock_irq(&dev_priv->irq_lock);
> >> >> > cancel_work_sync(&dev_priv->hotplug.dig_port_work);
> >> >> > -	cancel_work_sync(&dev_priv->hotplug.hotplug_work);
> >> >> > +
> >> >> > cancel_delayed_work_sync(&dev_priv->hotplug.hotplug_work);
> >> >> > cancel_work_sync(&dev_priv->hotplug.poll_init_work);
> >> >> > cancel_delayed_work_sync(&dev_priv->hotplug.reenable_work); }      
> >> >> Jani - With these changes, my customer platform still fails the
> >> >> HDMI hot-plug test - slowly unplug the HDMI cable,
> >> >> the /sys/class/drm/card0/card0-HDMI-A-1/status still shows
> >> >> connected state. My previous experiments indicated adding bigger
> >> >> delay before calling "drm_kms_helper_hotplug_event(dev)", which
> >> >> lead to reading the EDID, didn't have solid passing test
> >> >> results. The POC changes with passing results from onsite
> >> >> utilize following algorithm - 1. in the hot-plug worker
> >> >> function, check the PCH hot-plug status pin to see it's
> >> >> disconnected or not in a 3 x 100ms loop 2. if "disconnected" is
> >> >> confirmed after this loop, honor it and do not read the EDID
> >> >> 3. if "connected" is confirmed after this loop, honor it after
> >> >> EDID read is successful
> >> >> The overhead for this algorithm is to access the HPD status from
> >> >> all platform PCHs and some hot-plug code refactoring would be
> >> >> needed. (sorry for the late reply as of customer platform
> >> >> preparations, etc)   
> >> > Double checked my archives - the EDID reading is actually done in
> >> > the "intel_encoder->hotplug()" via "drm_helper_probe_detect()"
> >> > sorry for the typo.    
> >> 
> >> Earlier attempts at utilizing the hotplug live status in the work
> >> function have failed; it's not universally reliable.  
> > These are hotplug live status checking *only* or the EDID/DDC
> > readings are still followed?  
> 
> Cc: Ville for the details.
> 
> >> With that, you basically need to have a time threshold between
> >> hotplug irq and DDC disconnect. Slower than that, it will fail.
> >> How slow is acceptable?
> >> 
> >> BR,
> >> Jani.
> >> 
> >>   
> > 1000ms (1.0 second) was set as the threshold but failures were still
> > seen from onsite testing. Thus hotplug live status checking in a
> > loop of 100ms interval (>= 3 times), skipped reading DDC for the
> > disconnect case, was accepted - We did communicate our concerns on
> > the slowness of this "slowly unplug HDMI cable" test case with the
> > customer and had no positive feedback. It seems times between
> > "HOTPLUG_DET" and DDC line disconnections vary a lot per platform
> > and test cases when the HMDI cable is unplugged.  
> 
> Obviously it depends on the physical connectors on both the chassis
> and the cable. Still, 1 second not being enough to move the cable
> from HPD disconnect to DDC pin disconnect, presumably way less than
> 10 mm, seems excessive. What's the test case? What is the actual
> requirement? Is there something special about the hardware? Are you
> otherwise running an upstream kernel?
The test case is very straight forward - unplug the HDMI cable
(wiggling) and make sure the dualview and HDMI kernel connection
states are correct.
This is the KBL laptop running 4.15.7. The issue happens on both
4.15.7 and upstream latest kernel. The POC was tested with both 4.15.7
and upstream latest kernel, showing positive results.
> 
> > Let me know if you would like to see my hotplug live status checking
> > based patch.  
> 
> You're welcome to share it, but I think we've added and reverted live
> status checking at least twice now. It fixes some machines, and
> breaks a bunch of others.
We had local discussions (Bob, Matt) and indeed have some concerns on
live state checking. Anyway, I'll post the patch for you and other
experts to review it (Now double checking if display DE port or PCH
should be accessed for live state reads).
Thanks,
Guang
> 
> 
> BR,
> Jani.
>