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

From: Jani Nikula <jani.nikula@intel.com>
To: Guang Bai <guang.bai@intel.com>
Cc: intel-gfx@lists.freedesktop.org, Chris Chiu <chiu@endlessm.com>
Subject: Re: [PATCH] drm/i915: delay hotplug scheduling
Date: Thu, 27 Sep 2018 09:45:53 +0300	[thread overview]
Message-ID: <87zhw3saji.fsf@intel.com> (raw)
In-Reply-To: <20180926174345.3c6c11ca@gbai-ub1604-LTS>

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.

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.

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx