[PATCH 0/7] drm/i915 hotplug cleanup, part II

[PATCH 0/7] drm/i915 hotplug cleanup, part II

[Jani Nikula]

Continuing from [1], here's further hotplug code cleanup.

Unfortunately I realized I need to retract one change from the previous
series, the indentation change in intel_hpd_irq_handler, but it's not a
big deal. The main change here is patch 6/7 which splits platform
specific parts from platform agnostic parts.

BR,
Jani.


[1] http://mid.gmane.org/cover.1432728144.git.jani.nikula@intel.com


Jani Nikula (7):
  drm/i915: add for_each_hpd_pin to iterate over hotplug pins
  drm/i915: simplify conditions for skipping the 2nd hpd loop iterations
  drm/i915: put back the indent in intel_hpd_irq_handler
  drm/i915: merge the two hpd loops in intel_hpd_irq_handler to one
  drm/i915: simplify condition for digital port
  drm/i915: abstract away platform specific parts from hpd handling
  drm/i915/bxt: clear hpd status sticky bits earlier

 drivers/gpu/drm/i915/i915_drv.h |   3 +
 drivers/gpu/drm/i915/i915_irq.c | 210 ++++++++++++++++++++++++----------------
 2 files changed, 128 insertions(+), 85 deletions(-)

-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/7] drm/i915: add for_each_hpd_pin to iterate over hotplug pins

[Jani Nikula]

No functional changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 3 +++
 drivers/gpu/drm/i915/i915_irq.c | 8 ++++----
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 173c9051bf86..60aa9626f91f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -217,6 +217,9 @@ enum hpd_pin {
 	HPD_NUM_PINS
 };
 
+#define for_each_hpd_pin(__pin) \
+	for ((__pin) = (HPD_NONE + 1); (__pin) < HPD_NUM_PINS; (__pin)++)
+
 struct i915_hotplug {
 	struct work_struct hotplug_work;
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 91cb0b6ec47b..ad8897828b0c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1441,7 +1441,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
 			 hotplug_trigger, dig_hotplug_reg);
 
 	spin_lock(&dev_priv->irq_lock);
-	for (i = 1; i < HPD_NUM_PINS; i++) {
+	for_each_hpd_pin(i) {
 		bool long_hpd;
 
 		if (!(hpd[i] & hotplug_trigger))
@@ -1477,7 +1477,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
 		queue_dig = true;
 	}
 
-	for (i = 1; i < HPD_NUM_PINS; i++) {
+	for_each_hpd_pin(i) {
 		if (hpd[i] & hotplug_trigger &&
 		    dev_priv->hotplug.stats[i].state == HPD_DISABLED) {
 			/*
@@ -4292,7 +4292,7 @@ static void intel_hpd_irq_reenable_work(struct work_struct *work)
 	intel_runtime_pm_get(dev_priv);
 
 	spin_lock_irq(&dev_priv->irq_lock);
-	for (i = (HPD_NONE + 1); i < HPD_NUM_PINS; i++) {
+	for_each_hpd_pin(i) {
 		struct drm_connector *connector;
 
 		if (dev_priv->hotplug.stats[i].state != HPD_DISABLED)
@@ -4450,7 +4450,7 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
 	struct drm_connector *connector;
 	int i;
 
-	for (i = 1; i < HPD_NUM_PINS; i++) {
+	for_each_hpd_pin(i) {
 		dev_priv->hotplug.stats[i].count = 0;
 		dev_priv->hotplug.stats[i].state = HPD_ENABLED;
 	}
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/7] drm/i915: simplify conditions for skipping the 2nd hpd loop iterations

[Jani Nikula]

Multiple positive and negative checks for hpd[i] & hotplug_trigger gets
hard to read. Simplify. This should make follow-up patches merging the
two loops easier. No functional changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ad8897828b0c..536e97381cef 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1478,8 +1478,10 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
 	}
 
 	for_each_hpd_pin(i) {
-		if (hpd[i] & hotplug_trigger &&
-		    dev_priv->hotplug.stats[i].state == HPD_DISABLED) {
+		if (!(hpd[i] & hotplug_trigger))
+			continue;
+
+		if (dev_priv->hotplug.stats[i].state == HPD_DISABLED) {
 			/*
 			 * On GMCH platforms the interrupt mask bits only
 			 * prevent irq generation, not the setting of the
@@ -1493,8 +1495,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
 			continue;
 		}
 
-		if (!(hpd[i] & hotplug_trigger) ||
-		    dev_priv->hotplug.stats[i].state != HPD_ENABLED)
+		if (dev_priv->hotplug.stats[i].state != HPD_ENABLED)
 			continue;
 
 		if (!(dig_port_mask & hpd[i])) {
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/7] drm/i915: put back the indent in intel_hpd_irq_handler

[Jani Nikula]

In an unfortunate back and forth stepping, retract the earlier change to
reduce indent. This is to make merging the two loops easier. No
functional changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 51 ++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 536e97381cef..4840b21c1869 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1442,39 +1442,38 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
 
 	spin_lock(&dev_priv->irq_lock);
 	for_each_hpd_pin(i) {
-		bool long_hpd;
-
 		if (!(hpd[i] & hotplug_trigger))
 			continue;
 
 		port = get_port_from_pin(i);
-		if (!port || !dev_priv->hotplug.irq_port[port])
-			continue;
+		if (port && dev_priv->hotplug.irq_port[port]) {
+			bool long_hpd;
 
-		if (!HAS_GMCH_DISPLAY(dev_priv)) {
-			dig_shift = pch_port_to_hotplug_shift(port);
-			long_hpd = (dig_hotplug_reg >> dig_shift) & PORTB_HOTPLUG_LONG_DETECT;
-		} else {
-			dig_shift = i915_port_to_hotplug_shift(port);
-			long_hpd = (hotplug_trigger >> dig_shift) & PORTB_HOTPLUG_LONG_DETECT;
-		}
+			if (!HAS_GMCH_DISPLAY(dev_priv)) {
+				dig_shift = pch_port_to_hotplug_shift(port);
+				long_hpd = (dig_hotplug_reg >> dig_shift) & PORTB_HOTPLUG_LONG_DETECT;
+			} else {
+				dig_shift = i915_port_to_hotplug_shift(port);
+				long_hpd = (hotplug_trigger >> dig_shift) & PORTB_HOTPLUG_LONG_DETECT;
+			}
 
-		DRM_DEBUG_DRIVER("digital hpd port %c - %s\n", port_name(port),
-				 long_hpd ? "long" : "short");
-		/*
-		 * For long HPD pulses we want to have the digital queue happen,
-		 * but we still want HPD storm detection to function.
-		 */
-		if (long_hpd) {
-			dev_priv->hotplug.long_port_mask |= (1 << port);
-			dig_port_mask |= hpd[i];
-		} else {
-			/* for short HPD just trigger the digital queue */
-			dev_priv->hotplug.short_port_mask |= (1 << port);
-			hotplug_trigger &= ~hpd[i];
-		}
+			DRM_DEBUG_DRIVER("digital hpd port %c - %s\n", port_name(port),
+					 long_hpd ? "long" : "short");
+			/*
+			 * For long HPD pulses we want to have the digital queue happen,
+			 * but we still want HPD storm detection to function.
+			 */
+			if (long_hpd) {
+				dev_priv->hotplug.long_port_mask |= (1 << port);
+				dig_port_mask |= hpd[i];
+			} else {
+				/* for short HPD just trigger the digital queue */
+				dev_priv->hotplug.short_port_mask |= (1 << port);
+				hotplug_trigger &= ~hpd[i];
+			}
 
-		queue_dig = true;
+			queue_dig = true;
+		}
 	}
 
 	for_each_hpd_pin(i) {
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/7] drm/i915: merge the two hpd loops in intel_hpd_irq_handler to one

[Jani Nikula]

Nothing in the two consecutive loops over hpd pins depends on state in a
larger context than the single hpd pin. If we skip the rest of the loop
on short hpd pulses, we can merge the two loops into one.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4840b21c1869..b53b91744a17 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1463,22 +1463,17 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
 			 * For long HPD pulses we want to have the digital queue happen,
 			 * but we still want HPD storm detection to function.
 			 */
+			queue_dig = true;
 			if (long_hpd) {
 				dev_priv->hotplug.long_port_mask |= (1 << port);
+				/* FIXME: this can be simplified. */
 				dig_port_mask |= hpd[i];
 			} else {
 				/* for short HPD just trigger the digital queue */
 				dev_priv->hotplug.short_port_mask |= (1 << port);
-				hotplug_trigger &= ~hpd[i];
+				continue;
 			}
-
-			queue_dig = true;
 		}
-	}
-
-	for_each_hpd_pin(i) {
-		if (!(hpd[i] & hotplug_trigger))
-			continue;
 
 		if (dev_priv->hotplug.stats[i].state == HPD_DISABLED) {
 			/*
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/7] drm/i915: simplify condition for digital port

[Jani Nikula]

As the hpd loops have been merged together, we don't have to maintain
state for all hpd triggers.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b53b91744a17..6fffbfd3121a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1432,7 +1432,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
 	bool storm_detected = false;
 	bool queue_dig = false, queue_hp = false;
 	u32 dig_shift;
-	u32 dig_port_mask = 0;
+	bool is_dig_port;
 
 	if (!hotplug_trigger)
 		return;
@@ -1446,7 +1446,9 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
 			continue;
 
 		port = get_port_from_pin(i);
-		if (port && dev_priv->hotplug.irq_port[port]) {
+		is_dig_port = port && dev_priv->hotplug.irq_port[port];
+
+		if (is_dig_port) {
 			bool long_hpd;
 
 			if (!HAS_GMCH_DISPLAY(dev_priv)) {
@@ -1466,8 +1468,6 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
 			queue_dig = true;
 			if (long_hpd) {
 				dev_priv->hotplug.long_port_mask |= (1 << port);
-				/* FIXME: this can be simplified. */
-				dig_port_mask |= hpd[i];
 			} else {
 				/* for short HPD just trigger the digital queue */
 				dev_priv->hotplug.short_port_mask |= (1 << port);
@@ -1492,7 +1492,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
 		if (dev_priv->hotplug.stats[i].state != HPD_ENABLED)
 			continue;
 
-		if (!(dig_port_mask & hpd[i])) {
+		if (!is_dig_port) {
 			dev_priv->hotplug.event_bits |= (1 << i);
 			queue_hp = true;
 		}
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/7] drm/i915: abstract away platform specific parts from hpd handling

[Jani Nikula]

Split intel_hpd_irq_handler into platforms specific and platform
agnostic parts. The platform specific parts decode the registers into
information about which hpd pins triggered, and if they were long
pulses. The platform agnostic parts do further processing, such as
interrupt storm mitigation and scheduling bottom halves.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 147 +++++++++++++++++++++++++++-------------
 1 file changed, 101 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6fffbfd3121a..d401c863aeee 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1375,35 +1375,31 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
 #define HPD_STORM_DETECT_PERIOD 1000
 #define HPD_STORM_THRESHOLD 5
 
-static int pch_port_to_hotplug_shift(enum port port)
+static bool pch_port_hotplug_long_detect(enum port port, u32 val)
 {
 	switch (port) {
-	case PORT_A:
-	case PORT_E:
-	default:
-		return -1;
 	case PORT_B:
-		return 0;
+		return val & PORTB_HOTPLUG_LONG_DETECT;
 	case PORT_C:
-		return 8;
+		return val & PORTC_HOTPLUG_LONG_DETECT;
 	case PORT_D:
-		return 16;
+		return val & PORTD_HOTPLUG_LONG_DETECT;
+	default:
+		return false;
 	}
 }
 
-static int i915_port_to_hotplug_shift(enum port port)
+static bool i9xx_port_hotplug_long_detect(enum port port, u32 val)
 {
 	switch (port) {
-	case PORT_A:
-	case PORT_E:
-	default:
-		return -1;
 	case PORT_B:
-		return 17;
+		return val & PORTB_HOTPLUG_INT_LONG_PULSE;
 	case PORT_C:
-		return 19;
+		return val & PORTC_HOTPLUG_INT_LONG_PULSE;
 	case PORT_D:
-		return 21;
+		return val & PORTD_HOTPLUG_INT_LONG_PULSE;
+	default:
+		return false;
 	}
 }
 
@@ -1421,43 +1417,96 @@ static enum port get_port_from_pin(enum hpd_pin pin)
 	}
 }
 
+/* Get a bit mask of pins that have triggered, and which ones may be long. */
+static void pch_get_hpd_pins(u32 *pin_mask, u32 *long_mask,
+			     u32 hotplug_trigger, u32 dig_hotplug_reg, const u32 hpd[HPD_NUM_PINS])
+{
+	int i;
+
+	*pin_mask = 0;
+	*long_mask = 0;
+
+	if (!hotplug_trigger)
+		return;
+
+	for_each_hpd_pin(i) {
+		if (hpd[i] & hotplug_trigger) {
+			*pin_mask |= BIT(i);
+
+			if (pch_port_hotplug_long_detect(get_port_from_pin(i), dig_hotplug_reg))
+				*long_mask |= BIT(i);
+		}
+	}
+
+	DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x, dig 0x%08x, pins 0x%08x\n",
+			 hotplug_trigger, dig_hotplug_reg, *pin_mask);
+
+}
+
+/* Get a bit mask of pins that have triggered, and which ones may be long. */
+static void i9xx_get_hpd_pins(u32 *pin_mask, u32 *long_mask,
+			      u32 hotplug_trigger, const u32 hpd[HPD_NUM_PINS])
+{
+	int i;
+
+	*pin_mask = 0;
+	*long_mask = 0;
+
+	if (!hotplug_trigger)
+		return;
+
+	for_each_hpd_pin(i) {
+		if (hpd[i] & hotplug_trigger) {
+			*pin_mask |= BIT(i);
+
+			if (i9xx_port_hotplug_long_detect(get_port_from_pin(i), hotplug_trigger))
+				*long_mask |= BIT(i);
+		}
+	}
+
+	DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x, pins 0x%08x\n",
+			 hotplug_trigger, *pin_mask);
+}
+
+/**
+ * intel_hpd_irq_handler - main hotplug irq handler
+ * @dev: drm device
+ * @pin_mask: a mask of hpd pins that have triggered the irq
+ * @long_mask: a mask of hpd pins that may be long hpd pulses
+ *
+ * This is the main hotplug irq handler for all platforms. The platform specific
+ * irq handlers call the platform specific hotplug irq handlers, which read and
+ * decode the appropriate registers into bitmasks about hpd pins that have
+ * triggered (@pin_mask), and which of those pins may be long pulses
+ * (@long_mask). The @long_mask is ignored if the port corresponding to the pin
+ * is not a digital port.
+ *
+ * Here, we do hotplug irq storm detection and mitigation, and pass further
+ * processing to appropriate bottom halves.
+ */
 static void intel_hpd_irq_handler(struct drm_device *dev,
-				  u32 hotplug_trigger,
-				  u32 dig_hotplug_reg,
-				  const u32 hpd[HPD_NUM_PINS])
+				  u32 pin_mask, u32 long_mask)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int i;
 	enum port port;
 	bool storm_detected = false;
 	bool queue_dig = false, queue_hp = false;
-	u32 dig_shift;
 	bool is_dig_port;
 
-	if (!hotplug_trigger)
+	if (!pin_mask)
 		return;
 
-	DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x, dig 0x%08x\n",
-			 hotplug_trigger, dig_hotplug_reg);
-
 	spin_lock(&dev_priv->irq_lock);
 	for_each_hpd_pin(i) {
-		if (!(hpd[i] & hotplug_trigger))
+		if (!(BIT(i) & pin_mask))
 			continue;
 
 		port = get_port_from_pin(i);
 		is_dig_port = port && dev_priv->hotplug.irq_port[port];
 
 		if (is_dig_port) {
-			bool long_hpd;
-
-			if (!HAS_GMCH_DISPLAY(dev_priv)) {
-				dig_shift = pch_port_to_hotplug_shift(port);
-				long_hpd = (dig_hotplug_reg >> dig_shift) & PORTB_HOTPLUG_LONG_DETECT;
-			} else {
-				dig_shift = i915_port_to_hotplug_shift(port);
-				long_hpd = (hotplug_trigger >> dig_shift) & PORTB_HOTPLUG_LONG_DETECT;
-			}
+			bool long_hpd = long_mask & BIT(i);
 
 			DRM_DEBUG_DRIVER("digital hpd port %c - %s\n", port_name(port),
 					 long_hpd ? "long" : "short");
@@ -1483,9 +1532,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
 			 * interrupts on saner platforms.
 			 */
 			WARN_ONCE(INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev),
-				  "Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n",
-				  hotplug_trigger, i, hpd[i]);
-
+				  "Received HPD interrupt on pin %d although disabled\n", i);
 			continue;
 		}
 
@@ -1493,7 +1540,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
 			continue;
 
 		if (!is_dig_port) {
-			dev_priv->hotplug.event_bits |= (1 << i);
+			dev_priv->hotplug.event_bits |= BIT(i);
 			queue_hp = true;
 		}
 
@@ -1505,7 +1552,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
 			DRM_DEBUG_KMS("Received HPD interrupt on PIN %d - cnt: 0\n", i);
 		} else if (dev_priv->hotplug.stats[i].count > HPD_STORM_THRESHOLD) {
 			dev_priv->hotplug.stats[i].state = HPD_MARK_DISABLED;
-			dev_priv->hotplug.event_bits &= ~(1 << i);
+			dev_priv->hotplug.event_bits &= ~BIT(i);
 			DRM_DEBUG_KMS("HPD interrupt storm detected on PIN %d\n", i);
 			storm_detected = true;
 		} else {
@@ -1753,6 +1800,7 @@ static void i9xx_hpd_irq_handler(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
+	u32 pin_mask, long_mask;
 
 	if (!hotplug_status)
 		return;
@@ -1767,14 +1815,16 @@ static void i9xx_hpd_irq_handler(struct drm_device *dev)
 	if (IS_G4X(dev) || IS_VALLEYVIEW(dev)) {
 		u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_G4X;
 
-		intel_hpd_irq_handler(dev, hotplug_trigger, 0, hpd_status_g4x);
+		i9xx_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, hpd_status_g4x);
+		intel_hpd_irq_handler(dev, pin_mask, long_mask);
 
 		if (hotplug_status & DP_AUX_CHANNEL_MASK_INT_STATUS_G4X)
 			dp_aux_irq_handler(dev);
 	} else {
 		u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
 
-		intel_hpd_irq_handler(dev, hotplug_trigger, 0, hpd_status_i915);
+		i9xx_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, hpd_status_i915);
+		intel_hpd_irq_handler(dev, pin_mask, long_mask);
 	}
 }
 
@@ -1874,11 +1924,13 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
 	int pipe;
 	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK;
 	u32 dig_hotplug_reg;
+	u32 pin_mask, long_mask;
 
 	dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG);
 	I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg);
 
-	intel_hpd_irq_handler(dev, hotplug_trigger, dig_hotplug_reg, hpd_ibx);
+	pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_ibx);
+	intel_hpd_irq_handler(dev, pin_mask, long_mask);
 
 	if (pch_iir & SDE_AUDIO_POWER_MASK) {
 		int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK) >>
@@ -1971,11 +2023,13 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
 	int pipe;
 	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
 	u32 dig_hotplug_reg;
+	u32 pin_mask, long_mask;
 
 	dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG);
 	I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg);
 
-	intel_hpd_irq_handler(dev, hotplug_trigger, dig_hotplug_reg, hpd_cpt);
+	pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_cpt);
+	intel_hpd_irq_handler(dev, pin_mask, long_mask);
 
 	if (pch_iir & SDE_AUDIO_POWER_MASK_CPT) {
 		int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK_CPT) >>
@@ -2174,8 +2228,8 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 static void bxt_hpd_handler(struct drm_device *dev, uint32_t iir_status)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	uint32_t hp_control;
-	uint32_t hp_trigger;
+	u32 hp_control, hp_trigger;
+	u32 pin_mask, long_mask;
 
 	/* Get the status */
 	hp_trigger = iir_status & BXT_DE_PORT_HOTPLUG_MASK;
@@ -2191,7 +2245,8 @@ static void bxt_hpd_handler(struct drm_device *dev, uint32_t iir_status)
 		hp_control & BXT_HOTPLUG_CTL_MASK);
 
 	/* Check for HPD storm and schedule bottom half */
-	intel_hpd_irq_handler(dev, hp_trigger, hp_control, hpd_bxt);
+	pch_get_hpd_pins(&pin_mask, &long_mask, hp_trigger, hp_control, hpd_bxt);
+	intel_hpd_irq_handler(dev, pin_mask, long_mask);
 
 	/*
 	 * FIXME: Save the hot plug status for bottom half before
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/7] drm/i915/bxt: clear hpd status sticky bits earlier

[Jani Nikula]

The hotplug status is cached in hp_control, and will be passed on to
bottom halves through intel_hpd_irq_handler(), so we can clear the
sticky bits earlier.

While at it, drop the redundant logging of the hotplug status, which
will also be logged by pch_get_hpd_pins().

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d401c863aeee..e4260b0924f1 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2241,21 +2241,11 @@ static void bxt_hpd_handler(struct drm_device *dev, uint32_t iir_status)
 		return;
 	}
 
-	DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
-		hp_control & BXT_HOTPLUG_CTL_MASK);
+	/* Clear sticky bits in hpd status */
+	I915_WRITE(BXT_HOTPLUG_CTL, hp_control);
 
-	/* Check for HPD storm and schedule bottom half */
 	pch_get_hpd_pins(&pin_mask, &long_mask, hp_trigger, hp_control, hpd_bxt);
 	intel_hpd_irq_handler(dev, pin_mask, long_mask);
-
-	/*
-	 * FIXME: Save the hot plug status for bottom half before
-	 * clearing the sticky status bits, else the status will be
-	 * lost.
-	 */
-
-	/* Clear sticky bits in hpd status */
-	I915_WRITE(BXT_HOTPLUG_CTL, hp_control);
 }
 
 static irqreturn_t gen8_irq_handler(int irq, void *arg)
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/7] drm/i915 hotplug cleanup, part II

[Paulo Zanoni]

2015-05-28 9:43 GMT-03:00 Jani Nikula <jani.nikula@intel.com>:
> Continuing from [1], here's further hotplug code cleanup.
>
> Unfortunately I realized I need to retract one change from the previous
> series, the indentation change in intel_hpd_irq_handler, but it's not a
> big deal. The main change here is patch 6/7 which splits platform
> specific parts from platform agnostic parts.

For patches 1, 2, 3, 4 and 5:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

<insert complaint about exceeding 80 columns on P3 here>

>
> BR,
> Jani.
>
>
> [1] http://mid.gmane.org/cover.1432728144.git.jani.nikula@intel.com
>
>
> Jani Nikula (7):
>   drm/i915: add for_each_hpd_pin to iterate over hotplug pins
>   drm/i915: simplify conditions for skipping the 2nd hpd loop iterations
>   drm/i915: put back the indent in intel_hpd_irq_handler
>   drm/i915: merge the two hpd loops in intel_hpd_irq_handler to one
>   drm/i915: simplify condition for digital port
>   drm/i915: abstract away platform specific parts from hpd handling
>   drm/i915/bxt: clear hpd status sticky bits earlier
>
>  drivers/gpu/drm/i915/i915_drv.h |   3 +
>  drivers/gpu/drm/i915/i915_irq.c | 210 ++++++++++++++++++++++++----------------
>  2 files changed, 128 insertions(+), 85 deletions(-)
>
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915: abstract away platform specific parts from hpd handling

[Paulo Zanoni]

2015-05-28 9:43 GMT-03:00 Jani Nikula <jani.nikula@intel.com>:
> Split intel_hpd_irq_handler into platforms specific and platform
> agnostic parts. The platform specific parts decode the registers into
> information about which hpd pins triggered, and if they were long
> pulses. The platform agnostic parts do further processing, such as
> interrupt storm mitigation and scheduling bottom halves.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 147 +++++++++++++++++++++++++++-------------
>  1 file changed, 101 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6fffbfd3121a..d401c863aeee 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1375,35 +1375,31 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
>  #define HPD_STORM_DETECT_PERIOD 1000
>  #define HPD_STORM_THRESHOLD 5
>
> -static int pch_port_to_hotplug_shift(enum port port)
> +static bool pch_port_hotplug_long_detect(enum port port, u32 val)

We could try to think on something better than just "val". IMHO all
the variable names for the whole hotplug code on this file feel weird.


>  {
>         switch (port) {
> -       case PORT_A:
> -       case PORT_E:
> -       default:
> -               return -1;
>         case PORT_B:
> -               return 0;
> +               return val & PORTB_HOTPLUG_LONG_DETECT;
>         case PORT_C:
> -               return 8;
> +               return val & PORTC_HOTPLUG_LONG_DETECT;
>         case PORT_D:
> -               return 16;
> +               return val & PORTD_HOTPLUG_LONG_DETECT;

How about if we at least DRM_DEBUG_KMS() in case the short bit is not
1 for these "valid" ports? But I would prefer DRM_ERROR. This
can/should be a separate patch.


> +       default:
> +               return false;
>         }
>  }
>
> -static int i915_port_to_hotplug_shift(enum port port)
> +static bool i9xx_port_hotplug_long_detect(enum port port, u32 val)
>  {
>         switch (port) {
> -       case PORT_A:
> -       case PORT_E:
> -       default:
> -               return -1;
>         case PORT_B:
> -               return 17;
> +               return val & PORTB_HOTPLUG_INT_LONG_PULSE;
>         case PORT_C:
> -               return 19;
> +               return val & PORTC_HOTPLUG_INT_LONG_PULSE;
>         case PORT_D:
> -               return 21;
> +               return val & PORTD_HOTPLUG_INT_LONG_PULSE;
> +       default:
> +               return false;
>         }
>  }
>
> @@ -1421,43 +1417,96 @@ static enum port get_port_from_pin(enum hpd_pin pin)
>         }
>  }
>
> +/* Get a bit mask of pins that have triggered, and which ones may be long. */
> +static void pch_get_hpd_pins(u32 *pin_mask, u32 *long_mask,
> +                            u32 hotplug_trigger, u32 dig_hotplug_reg, const u32 hpd[HPD_NUM_PINS])

<insert even louder complaints about going past 80 columns>

> +{
> +       int i;
> +
> +       *pin_mask = 0;
> +       *long_mask = 0;
> +
> +       if (!hotplug_trigger)
> +               return;
> +
> +       for_each_hpd_pin(i) {
> +               if (hpd[i] & hotplug_trigger) {
> +                       *pin_mask |= BIT(i);
> +
> +                       if (pch_port_hotplug_long_detect(get_port_from_pin(i), dig_hotplug_reg))
> +                               *long_mask |= BIT(i);
> +               }
> +       }
> +
> +       DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x, dig 0x%08x, pins 0x%08x\n",
> +                        hotplug_trigger, dig_hotplug_reg, *pin_mask);
> +
> +}
> +
> +/* Get a bit mask of pins that have triggered, and which ones may be long. */
> +static void i9xx_get_hpd_pins(u32 *pin_mask, u32 *long_mask,
> +                             u32 hotplug_trigger, const u32 hpd[HPD_NUM_PINS])
> +{
> +       int i;
> +
> +       *pin_mask = 0;
> +       *long_mask = 0;
> +
> +       if (!hotplug_trigger)
> +               return;
> +
> +       for_each_hpd_pin(i) {
> +               if (hpd[i] & hotplug_trigger) {
> +                       *pin_mask |= BIT(i);
> +
> +                       if (i9xx_port_hotplug_long_detect(get_port_from_pin(i), hotplug_trigger))
> +                               *long_mask |= BIT(i);
> +               }
> +       }
> +
> +       DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x, pins 0x%08x\n",
> +                        hotplug_trigger, *pin_mask);

The fact that this is mostly duplicated code feels a little bit weird.
I won't be surprised if some random person writes a patch to
deduplicate this in the future. I could even give it a R-B.


> +}
> +
> +/**
> + * intel_hpd_irq_handler - main hotplug irq handler
> + * @dev: drm device
> + * @pin_mask: a mask of hpd pins that have triggered the irq
> + * @long_mask: a mask of hpd pins that may be long hpd pulses
> + *
> + * This is the main hotplug irq handler for all platforms. The platform specific
> + * irq handlers call the platform specific hotplug irq handlers, which read and
> + * decode the appropriate registers into bitmasks about hpd pins that have
> + * triggered (@pin_mask), and which of those pins may be long pulses
> + * (@long_mask). The @long_mask is ignored if the port corresponding to the pin
> + * is not a digital port.

Nice!

> + *
> + * Here, we do hotplug irq storm detection and mitigation, and pass further
> + * processing to appropriate bottom halves.

Now you can extract the code that does the storm detection and put it
on its own function :)
intel_hpd_storm_pin_update() or something else...

Besides all the bikeshedding and suggestions for "part III", the code
looks correct, and more organized, so with or without changes:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


> + */
>  static void intel_hpd_irq_handler(struct drm_device *dev,
> -                                 u32 hotplug_trigger,
> -                                 u32 dig_hotplug_reg,
> -                                 const u32 hpd[HPD_NUM_PINS])
> +                                 u32 pin_mask, u32 long_mask)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         int i;
>         enum port port;
>         bool storm_detected = false;
>         bool queue_dig = false, queue_hp = false;
> -       u32 dig_shift;
>         bool is_dig_port;
>
> -       if (!hotplug_trigger)
> +       if (!pin_mask)
>                 return;
>
> -       DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x, dig 0x%08x\n",
> -                        hotplug_trigger, dig_hotplug_reg);
> -
>         spin_lock(&dev_priv->irq_lock);
>         for_each_hpd_pin(i) {
> -               if (!(hpd[i] & hotplug_trigger))
> +               if (!(BIT(i) & pin_mask))
>                         continue;
>
>                 port = get_port_from_pin(i);
>                 is_dig_port = port && dev_priv->hotplug.irq_port[port];
>
>                 if (is_dig_port) {
> -                       bool long_hpd;
> -
> -                       if (!HAS_GMCH_DISPLAY(dev_priv)) {
> -                               dig_shift = pch_port_to_hotplug_shift(port);
> -                               long_hpd = (dig_hotplug_reg >> dig_shift) & PORTB_HOTPLUG_LONG_DETECT;
> -                       } else {
> -                               dig_shift = i915_port_to_hotplug_shift(port);
> -                               long_hpd = (hotplug_trigger >> dig_shift) & PORTB_HOTPLUG_LONG_DETECT;
> -                       }
> +                       bool long_hpd = long_mask & BIT(i);
>
>                         DRM_DEBUG_DRIVER("digital hpd port %c - %s\n", port_name(port),
>                                          long_hpd ? "long" : "short");
> @@ -1483,9 +1532,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
>                          * interrupts on saner platforms.
>                          */
>                         WARN_ONCE(INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev),
> -                                 "Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n",
> -                                 hotplug_trigger, i, hpd[i]);
> -
> +                                 "Received HPD interrupt on pin %d although disabled\n", i);
>                         continue;
>                 }
>
> @@ -1493,7 +1540,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
>                         continue;
>
>                 if (!is_dig_port) {
> -                       dev_priv->hotplug.event_bits |= (1 << i);
> +                       dev_priv->hotplug.event_bits |= BIT(i);
>                         queue_hp = true;
>                 }
>
> @@ -1505,7 +1552,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
>                         DRM_DEBUG_KMS("Received HPD interrupt on PIN %d - cnt: 0\n", i);
>                 } else if (dev_priv->hotplug.stats[i].count > HPD_STORM_THRESHOLD) {
>                         dev_priv->hotplug.stats[i].state = HPD_MARK_DISABLED;
> -                       dev_priv->hotplug.event_bits &= ~(1 << i);
> +                       dev_priv->hotplug.event_bits &= ~BIT(i);
>                         DRM_DEBUG_KMS("HPD interrupt storm detected on PIN %d\n", i);
>                         storm_detected = true;
>                 } else {
> @@ -1753,6 +1800,7 @@ static void i9xx_hpd_irq_handler(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
> +       u32 pin_mask, long_mask;
>
>         if (!hotplug_status)
>                 return;
> @@ -1767,14 +1815,16 @@ static void i9xx_hpd_irq_handler(struct drm_device *dev)
>         if (IS_G4X(dev) || IS_VALLEYVIEW(dev)) {
>                 u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_G4X;
>
> -               intel_hpd_irq_handler(dev, hotplug_trigger, 0, hpd_status_g4x);
> +               i9xx_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, hpd_status_g4x);
> +               intel_hpd_irq_handler(dev, pin_mask, long_mask);
>
>                 if (hotplug_status & DP_AUX_CHANNEL_MASK_INT_STATUS_G4X)
>                         dp_aux_irq_handler(dev);
>         } else {
>                 u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
>
> -               intel_hpd_irq_handler(dev, hotplug_trigger, 0, hpd_status_i915);
> +               i9xx_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, hpd_status_i915);
> +               intel_hpd_irq_handler(dev, pin_mask, long_mask);
>         }
>  }
>
> @@ -1874,11 +1924,13 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
>         int pipe;
>         u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK;
>         u32 dig_hotplug_reg;
> +       u32 pin_mask, long_mask;
>
>         dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG);
>         I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg);
>
> -       intel_hpd_irq_handler(dev, hotplug_trigger, dig_hotplug_reg, hpd_ibx);
> +       pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_ibx);
> +       intel_hpd_irq_handler(dev, pin_mask, long_mask);
>
>         if (pch_iir & SDE_AUDIO_POWER_MASK) {
>                 int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK) >>
> @@ -1971,11 +2023,13 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>         int pipe;
>         u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
>         u32 dig_hotplug_reg;
> +       u32 pin_mask, long_mask;
>
>         dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG);
>         I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg);
>
> -       intel_hpd_irq_handler(dev, hotplug_trigger, dig_hotplug_reg, hpd_cpt);
> +       pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_cpt);
> +       intel_hpd_irq_handler(dev, pin_mask, long_mask);
>
>         if (pch_iir & SDE_AUDIO_POWER_MASK_CPT) {
>                 int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK_CPT) >>
> @@ -2174,8 +2228,8 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  static void bxt_hpd_handler(struct drm_device *dev, uint32_t iir_status)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       uint32_t hp_control;
> -       uint32_t hp_trigger;
> +       u32 hp_control, hp_trigger;
> +       u32 pin_mask, long_mask;
>
>         /* Get the status */
>         hp_trigger = iir_status & BXT_DE_PORT_HOTPLUG_MASK;
> @@ -2191,7 +2245,8 @@ static void bxt_hpd_handler(struct drm_device *dev, uint32_t iir_status)
>                 hp_control & BXT_HOTPLUG_CTL_MASK);
>
>         /* Check for HPD storm and schedule bottom half */
> -       intel_hpd_irq_handler(dev, hp_trigger, hp_control, hpd_bxt);
> +       pch_get_hpd_pins(&pin_mask, &long_mask, hp_trigger, hp_control, hpd_bxt);
> +       intel_hpd_irq_handler(dev, pin_mask, long_mask);
>
>         /*
>          * FIXME: Save the hot plug status for bottom half before
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915/bxt: clear hpd status sticky bits earlier

[Paulo Zanoni]

2015-05-28 9:43 GMT-03:00 Jani Nikula <jani.nikula@intel.com>:
> The hotplug status is cached in hp_control, and will be passed on to
> bottom halves through intel_hpd_irq_handler(), so we can clear the
> sticky bits earlier.
>
> While at it, drop the redundant logging of the hotplug status, which
> will also be logged by pch_get_hpd_pins().
>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d401c863aeee..e4260b0924f1 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2241,21 +2241,11 @@ static void bxt_hpd_handler(struct drm_device *dev, uint32_t iir_status)
>                 return;
>         }
>
> -       DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
> -               hp_control & BXT_HOTPLUG_CTL_MASK);
> +       /* Clear sticky bits in hpd status */
> +       I915_WRITE(BXT_HOTPLUG_CTL, hp_control);
>
> -       /* Check for HPD storm and schedule bottom half */
>         pch_get_hpd_pins(&pin_mask, &long_mask, hp_trigger, hp_control, hpd_bxt);
>         intel_hpd_irq_handler(dev, pin_mask, long_mask);
> -
> -       /*
> -        * FIXME: Save the hot plug status for bottom half before
> -        * clearing the sticky status bits, else the status will be
> -        * lost.
> -        */
> -
> -       /* Clear sticky bits in hpd status */
> -       I915_WRITE(BXT_HOTPLUG_CTL, hp_control);
>  }
>
>  static irqreturn_t gen8_irq_handler(int irq, void *arg)
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915: abstract away platform specific parts from hpd handling

[Jani Nikula]

On Thu, 28 May 2015, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2015-05-28 9:43 GMT-03:00 Jani Nikula <jani.nikula@intel.com>:
>> Split intel_hpd_irq_handler into platforms specific and platform
>> agnostic parts. The platform specific parts decode the registers into
>> information about which hpd pins triggered, and if they were long
>> pulses. The platform agnostic parts do further processing, such as
>> interrupt storm mitigation and scheduling bottom halves.
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 147 +++++++++++++++++++++++++++-------------
>>  1 file changed, 101 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 6fffbfd3121a..d401c863aeee 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1375,35 +1375,31 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
>>  #define HPD_STORM_DETECT_PERIOD 1000
>>  #define HPD_STORM_THRESHOLD 5
>>
>> -static int pch_port_to_hotplug_shift(enum port port)
>> +static bool pch_port_hotplug_long_detect(enum port port, u32 val)
>
> We could try to think on something better than just "val". IMHO all
> the variable names for the whole hotplug code on this file feel weird.
>
>
>>  {
>>         switch (port) {
>> -       case PORT_A:
>> -       case PORT_E:
>> -       default:
>> -               return -1;
>>         case PORT_B:
>> -               return 0;
>> +               return val & PORTB_HOTPLUG_LONG_DETECT;
>>         case PORT_C:
>> -               return 8;
>> +               return val & PORTC_HOTPLUG_LONG_DETECT;
>>         case PORT_D:
>> -               return 16;
>> +               return val & PORTD_HOTPLUG_LONG_DETECT;
>
> How about if we at least DRM_DEBUG_KMS() in case the short bit is not
> 1 for these "valid" ports? But I would prefer DRM_ERROR. This
> can/should be a separate patch.
>
>
>> +       default:
>> +               return false;
>>         }
>>  }
>>
>> -static int i915_port_to_hotplug_shift(enum port port)
>> +static bool i9xx_port_hotplug_long_detect(enum port port, u32 val)
>>  {
>>         switch (port) {
>> -       case PORT_A:
>> -       case PORT_E:
>> -       default:
>> -               return -1;
>>         case PORT_B:
>> -               return 17;
>> +               return val & PORTB_HOTPLUG_INT_LONG_PULSE;
>>         case PORT_C:
>> -               return 19;
>> +               return val & PORTC_HOTPLUG_INT_LONG_PULSE;
>>         case PORT_D:
>> -               return 21;
>> +               return val & PORTD_HOTPLUG_INT_LONG_PULSE;
>> +       default:
>> +               return false;
>>         }
>>  }
>>
>> @@ -1421,43 +1417,96 @@ static enum port get_port_from_pin(enum hpd_pin pin)
>>         }
>>  }
>>
>> +/* Get a bit mask of pins that have triggered, and which ones may be long. */
>> +static void pch_get_hpd_pins(u32 *pin_mask, u32 *long_mask,
>> +                            u32 hotplug_trigger, u32 dig_hotplug_reg, const u32 hpd[HPD_NUM_PINS])
>
> <insert even louder complaints about going past 80 columns>
>
>> +{
>> +       int i;
>> +
>> +       *pin_mask = 0;
>> +       *long_mask = 0;
>> +
>> +       if (!hotplug_trigger)
>> +               return;
>> +
>> +       for_each_hpd_pin(i) {
>> +               if (hpd[i] & hotplug_trigger) {
>> +                       *pin_mask |= BIT(i);
>> +
>> +                       if (pch_port_hotplug_long_detect(get_port_from_pin(i), dig_hotplug_reg))
>> +                               *long_mask |= BIT(i);
>> +               }
>> +       }
>> +
>> +       DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x, dig 0x%08x, pins 0x%08x\n",
>> +                        hotplug_trigger, dig_hotplug_reg, *pin_mask);
>> +
>> +}
>> +
>> +/* Get a bit mask of pins that have triggered, and which ones may be long. */
>> +static void i9xx_get_hpd_pins(u32 *pin_mask, u32 *long_mask,
>> +                             u32 hotplug_trigger, const u32 hpd[HPD_NUM_PINS])
>> +{
>> +       int i;
>> +
>> +       *pin_mask = 0;
>> +       *long_mask = 0;
>> +
>> +       if (!hotplug_trigger)
>> +               return;
>> +
>> +       for_each_hpd_pin(i) {
>> +               if (hpd[i] & hotplug_trigger) {
>> +                       *pin_mask |= BIT(i);
>> +
>> +                       if (i9xx_port_hotplug_long_detect(get_port_from_pin(i), hotplug_trigger))
>> +                               *long_mask |= BIT(i);
>> +               }
>> +       }
>> +
>> +       DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x, pins 0x%08x\n",
>> +                        hotplug_trigger, *pin_mask);
>
> The fact that this is mostly duplicated code feels a little bit weird.
> I won't be surprised if some random person writes a patch to
> deduplicate this in the future. I could even give it a R-B.

I know, I was divided here myself. In the end I didn't want to make this
superficially platform independent with ifs inside, when the callers are
already platform specific. Also, the i9xx one does not have
dig_hotplug_reg, so not passing that in or logging adds clarity. But I
could have gone either way.

>
>
>> +}
>> +
>> +/**
>> + * intel_hpd_irq_handler - main hotplug irq handler
>> + * @dev: drm device
>> + * @pin_mask: a mask of hpd pins that have triggered the irq
>> + * @long_mask: a mask of hpd pins that may be long hpd pulses
>> + *
>> + * This is the main hotplug irq handler for all platforms. The platform specific
>> + * irq handlers call the platform specific hotplug irq handlers, which read and
>> + * decode the appropriate registers into bitmasks about hpd pins that have
>> + * triggered (@pin_mask), and which of those pins may be long pulses
>> + * (@long_mask). The @long_mask is ignored if the port corresponding to the pin
>> + * is not a digital port.
>
> Nice!
>
>> + *
>> + * Here, we do hotplug irq storm detection and mitigation, and pass further
>> + * processing to appropriate bottom halves.
>
> Now you can extract the code that does the storm detection and put it
> on its own function :)
> intel_hpd_storm_pin_update() or something else...

Yup, that's the plan, but things seem to move forward better with
smaller steps!

>
> Besides all the bikeshedding and suggestions for "part III", the code
> looks correct, and more organized, so with or without changes:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Thanks for the review and suggestions!

BR,
Jani.


>
>
>> + */
>>  static void intel_hpd_irq_handler(struct drm_device *dev,
>> -                                 u32 hotplug_trigger,
>> -                                 u32 dig_hotplug_reg,
>> -                                 const u32 hpd[HPD_NUM_PINS])
>> +                                 u32 pin_mask, u32 long_mask)
>>  {
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>>         int i;
>>         enum port port;
>>         bool storm_detected = false;
>>         bool queue_dig = false, queue_hp = false;
>> -       u32 dig_shift;
>>         bool is_dig_port;
>>
>> -       if (!hotplug_trigger)
>> +       if (!pin_mask)
>>                 return;
>>
>> -       DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x, dig 0x%08x\n",
>> -                        hotplug_trigger, dig_hotplug_reg);
>> -
>>         spin_lock(&dev_priv->irq_lock);
>>         for_each_hpd_pin(i) {
>> -               if (!(hpd[i] & hotplug_trigger))
>> +               if (!(BIT(i) & pin_mask))
>>                         continue;
>>
>>                 port = get_port_from_pin(i);
>>                 is_dig_port = port && dev_priv->hotplug.irq_port[port];
>>
>>                 if (is_dig_port) {
>> -                       bool long_hpd;
>> -
>> -                       if (!HAS_GMCH_DISPLAY(dev_priv)) {
>> -                               dig_shift = pch_port_to_hotplug_shift(port);
>> -                               long_hpd = (dig_hotplug_reg >> dig_shift) & PORTB_HOTPLUG_LONG_DETECT;
>> -                       } else {
>> -                               dig_shift = i915_port_to_hotplug_shift(port);
>> -                               long_hpd = (hotplug_trigger >> dig_shift) & PORTB_HOTPLUG_LONG_DETECT;
>> -                       }
>> +                       bool long_hpd = long_mask & BIT(i);
>>
>>                         DRM_DEBUG_DRIVER("digital hpd port %c - %s\n", port_name(port),
>>                                          long_hpd ? "long" : "short");
>> @@ -1483,9 +1532,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
>>                          * interrupts on saner platforms.
>>                          */
>>                         WARN_ONCE(INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev),
>> -                                 "Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n",
>> -                                 hotplug_trigger, i, hpd[i]);
>> -
>> +                                 "Received HPD interrupt on pin %d although disabled\n", i);
>>                         continue;
>>                 }
>>
>> @@ -1493,7 +1540,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
>>                         continue;
>>
>>                 if (!is_dig_port) {
>> -                       dev_priv->hotplug.event_bits |= (1 << i);
>> +                       dev_priv->hotplug.event_bits |= BIT(i);
>>                         queue_hp = true;
>>                 }
>>
>> @@ -1505,7 +1552,7 @@ static void intel_hpd_irq_handler(struct drm_device *dev,
>>                         DRM_DEBUG_KMS("Received HPD interrupt on PIN %d - cnt: 0\n", i);
>>                 } else if (dev_priv->hotplug.stats[i].count > HPD_STORM_THRESHOLD) {
>>                         dev_priv->hotplug.stats[i].state = HPD_MARK_DISABLED;
>> -                       dev_priv->hotplug.event_bits &= ~(1 << i);
>> +                       dev_priv->hotplug.event_bits &= ~BIT(i);
>>                         DRM_DEBUG_KMS("HPD interrupt storm detected on PIN %d\n", i);
>>                         storm_detected = true;
>>                 } else {
>> @@ -1753,6 +1800,7 @@ static void i9xx_hpd_irq_handler(struct drm_device *dev)
>>  {
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>>         u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
>> +       u32 pin_mask, long_mask;
>>
>>         if (!hotplug_status)
>>                 return;
>> @@ -1767,14 +1815,16 @@ static void i9xx_hpd_irq_handler(struct drm_device *dev)
>>         if (IS_G4X(dev) || IS_VALLEYVIEW(dev)) {
>>                 u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_G4X;
>>
>> -               intel_hpd_irq_handler(dev, hotplug_trigger, 0, hpd_status_g4x);
>> +               i9xx_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, hpd_status_g4x);
>> +               intel_hpd_irq_handler(dev, pin_mask, long_mask);
>>
>>                 if (hotplug_status & DP_AUX_CHANNEL_MASK_INT_STATUS_G4X)
>>                         dp_aux_irq_handler(dev);
>>         } else {
>>                 u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
>>
>> -               intel_hpd_irq_handler(dev, hotplug_trigger, 0, hpd_status_i915);
>> +               i9xx_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, hpd_status_i915);
>> +               intel_hpd_irq_handler(dev, pin_mask, long_mask);
>>         }
>>  }
>>
>> @@ -1874,11 +1924,13 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
>>         int pipe;
>>         u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK;
>>         u32 dig_hotplug_reg;
>> +       u32 pin_mask, long_mask;
>>
>>         dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG);
>>         I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg);
>>
>> -       intel_hpd_irq_handler(dev, hotplug_trigger, dig_hotplug_reg, hpd_ibx);
>> +       pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_ibx);
>> +       intel_hpd_irq_handler(dev, pin_mask, long_mask);
>>
>>         if (pch_iir & SDE_AUDIO_POWER_MASK) {
>>                 int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK) >>
>> @@ -1971,11 +2023,13 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>>         int pipe;
>>         u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
>>         u32 dig_hotplug_reg;
>> +       u32 pin_mask, long_mask;
>>
>>         dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG);
>>         I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg);
>>
>> -       intel_hpd_irq_handler(dev, hotplug_trigger, dig_hotplug_reg, hpd_cpt);
>> +       pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_cpt);
>> +       intel_hpd_irq_handler(dev, pin_mask, long_mask);
>>
>>         if (pch_iir & SDE_AUDIO_POWER_MASK_CPT) {
>>                 int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK_CPT) >>
>> @@ -2174,8 +2228,8 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>>  static void bxt_hpd_handler(struct drm_device *dev, uint32_t iir_status)
>>  {
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>> -       uint32_t hp_control;
>> -       uint32_t hp_trigger;
>> +       u32 hp_control, hp_trigger;
>> +       u32 pin_mask, long_mask;
>>
>>         /* Get the status */
>>         hp_trigger = iir_status & BXT_DE_PORT_HOTPLUG_MASK;
>> @@ -2191,7 +2245,8 @@ static void bxt_hpd_handler(struct drm_device *dev, uint32_t iir_status)
>>                 hp_control & BXT_HOTPLUG_CTL_MASK);
>>
>>         /* Check for HPD storm and schedule bottom half */
>> -       intel_hpd_irq_handler(dev, hp_trigger, hp_control, hpd_bxt);
>> +       pch_get_hpd_pins(&pin_mask, &long_mask, hp_trigger, hp_control, hpd_bxt);
>> +       intel_hpd_irq_handler(dev, pin_mask, long_mask);
>>
>>         /*
>>          * FIXME: Save the hot plug status for bottom half before
>> --
>> 2.1.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> -- 
> Paulo Zanoni

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