Re: [PATCH] drm/i915: delay hotplug scheduling

[Guang Bai <guang.bai@intel.com>]

On Thu, 27 Sep 2018 09:45:53 +0300
Jani Nikula <jani.nikula@intel.com> wrote:

> On Wed, 26 Sep 2018, Guang Bai <guang.bai@intel.com> wrote:
> > On Wed, 26 Sep 2018 16:44:16 -0700
> > Guang Bai <guang.bai@intel.com> wrote:
> >  
> >> On Tue, 25 Sep 2018 10:18:36 +0300
> >> Jani Nikula <jani.nikula@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@endlessm.com> Tested-by: Chris Chiu
> >> > <chiu@endlessm.com> Cc: Chris Chiu <chiu@endlessm.com>
> >> > Cc: Guang Bai <guang.bai@intel.com>
> >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > Signed-off-by: Jani Nikula <jani.nikula@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?
> 
> 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.
Let me know if you would like to see my hotplug live status checking
based patch.
Thanks,
Guang
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx