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

[Guang Bai]

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)