[Intel-gfx] [PATCH] drm/i915/irq: wait a little before queuing the hotplug work function

[Jani Nikula]

Currently it's possible this happens when a (non-DP) cable is unplugged:

- user starts unplugging cable
- hotplug irq fires
- hotplug work function runs
- connector detect function runs
- ddc pin is still connected, edid read succeeds
  -> we decide nothing changed, no uevent
- cable completely unplugged
  -> our state is inconsistent with reality, no power save

The user plugs cable back in:

- most of the same at first
- ddc pin connected, edid read succeeds
  -> we decide nothing changed because we thought the cable was plugged
     in all along, no uevent
- cable completely plugged
  -> our state is somewhat consistent, however monitor might be
     different, the link might not recover?

With our current implementation we rely on the hotplug pin being *both*
the last to be connected on plug *and* last to be disconnected on
unplug. The educated guess is that this is not the case.

Per the logs in the case of the referenced bug, the hdmi detect code
runs within a few *microseconds* after the hotplug irq, and the EDID has
been successfully read within 25 ms of the irq. If the DDC lines are
still connected when the hotplug irq fires, the user has to be blazingly
fast to complete the unplug before the detect code runs.

We can afford to wait a little before queuing the work function for a
bit more reliability. Obviously it's still possible for the user to
unplug the cable really slowly, but let's at least give the user a
fighting chance to unplug fast enough.

I'd love to claim the proposed delay of 400 ms is based on real life
measured data and rigorous analysis, but in truth it is just a gut
feeling based compromise between solving the issue and meeting some
vague real time human interaction deadline for having a picture on
screen. (However I expect any criticism to be more substantiated than
"my gut feeling is better than yours".)

An alternative would be to check the hotplug state in the irq handler,
but there have been reliability problems with it in the past, and we've
opted not to use it. [citation needed]

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82593
Tested-by: Hugh Greenberg <hugegreenbug at gmail.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.c |  2 +-
 drivers/gpu/drm/i915/i915_drv.h |  2 +-
 drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++----
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 78ef0bb53c36..002767ddfe06 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -552,7 +552,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_delayed_work_sync(&dev_priv->hotplug.reenable_work);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 611fbd86c1cc..a8a99f658772 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -221,7 +221,7 @@ enum hpd_pin {
 	for ((__pin) = (HPD_NONE + 1); (__pin) < HPD_NUM_PINS; (__pin)++)
 
 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/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 56db9e747464..bab67c279c22 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -828,6 +828,8 @@ static bool intel_hpd_irq_event(struct drm_device *dev,
 	return true;
 }
 
+#define HOTPLUG_DELAY_MS	400
+
 static void i915_digport_work_func(struct work_struct *work)
 {
 	struct drm_i915_private *dev_priv =
@@ -872,7 +874,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);
+		mod_delayed_work(system_wq, &dev_priv->hotplug.hotplug_work,
+				 msecs_to_jiffies(HOTPLUG_DELAY_MS));
 	}
 }
 
@@ -884,7 +887,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->dev;
 	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct intel_connector *intel_connector;
@@ -1601,7 +1604,8 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
 	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);
+		mod_delayed_work(system_wq, &dev_priv->hotplug.hotplug_work,
+				 msecs_to_jiffies(HOTPLUG_DELAY_MS));
 }
 
 static void gmbus_irq_handler(struct drm_device *dev)
@@ -4397,7 +4401,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 
-	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->rps.work, gen6_pm_rps_work);
 	INIT_WORK(&dev_priv->l3_parity.error_work, ivybridge_parity_work);
-- 
2.1.4