On Thu, 27 Sep 2018 09:45:53 +0300 Jani Nikula <jani.nikula@xxxxxxxxx> wrote: > On Wed, 26 Sep 2018, Guang Bai <guang.bai@xxxxxxxxx> wrote: > > On Wed, 26 Sep 2018 16:44:16 -0700 > > Guang Bai <guang.bai@xxxxxxxxx> wrote: > > > >> On Tue, 25 Sep 2018 10:18:36 +0300 > >> Jani Nikula <jani.nikula@xxxxxxxxx> 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@xxxxxxxxxxxxxx > >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107125 > >> > Reported-by: Chris Chiu <chiu@xxxxxxxxxxxx> Tested-by: Chris Chiu > >> > <chiu@xxxxxxxxxxxx> Cc: Chris Chiu <chiu@xxxxxxxxxxxx> > >> > Cc: Guang Bai <guang.bai@xxxxxxxxx> > >> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >> > Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> > >> > --- > >> > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
