drm/i915: hotplug cleanup saga continues

The hotplug work function has two loops iterating over connectors, the first for handling hotplug disabling due to irq storms and the second for actually handling the hotplug events. Move the debug printing into the second one, so we can abstract the storm handling better. This may change the output ordering slightly when there are multiple simultaneous hotplug events.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_irq.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +0 -4

index 56db9e747464..d64d6895a2e5 100644
@@ -917,10 +917,6 @@ static void i915_hotplug_work_func(struct work_struct *work)
 				| DRM_CONNECTOR_POLL_DISCONNECT;
 			hpd_disabled = true;
 		}
-		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
-			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
-				      connector->name, intel_encoder->hpd_pin);
-		}
 	}
 	 /* if there were no outputs to poll, poll was disabled,
 	  * therefore make sure it's enabled when disabling HPD on

Subject s/even/event/

Quoted 29 lines

The hotplug work function has two loops iterating over connectors, the first for handling hotplug disabling due to irq storms and the second for actually handling the hotplug events. Move the debug printing into the second one, so we can abstract the storm handling better. This may change the output ordering slightly when there are multiple simultaneous hotplug events.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_irq.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +0 -4

index 56db9e747464..d64d6895a2e5 100644
@@ -917,10 +917,6 @@ static void i915_hotplug_work_func(struct work_struct *work)
 				| DRM_CONNECTOR_POLL_DISCONNECT;
 			hpd_disabled = true;
 		}
-		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
-			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
-				      connector->name, intel_encoder->hpd_pin);
-		}
 	}
 	 /* if there were no outputs to poll, poll was disabled,
 	  * therefore make sure it's enabled when disabling HPD on

@@ -939,6 +935,8 @@ static void i915_hotplug_work_func(struct work_struct *work)
 			continue;

drivers/gpu/drm/i915/i915_irq.c static void i915_hotplug_work_func(struct work_struct *work) · line 939

Quoted 4 lines

drivers/gpu/drm/i915/i915_irq.c +2 -0

 		intel_encoder = intel_connector->encoder;
 		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
+			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
+				      connector->name, intel_encoder->hpd_pin);

In light of other conversations, is reporting an irq (even a hotplug one) directly related to KMS? I think DRIVER is more specific in this case.

-Chris

Signature · 1 line

Chris Wilson, Intel Open Source Technology Centre

Quoted 1 line · Chris Wilson

Subject s/even/event/

Oops.

Quoted 30 lines · Jani Nikula

Quoted 29 lines

The hotplug work function has two loops iterating over connectors, the first for handling hotplug disabling due to irq storms and the second for actually handling the hotplug events. Move the debug printing into the second one, so we can abstract the storm handling better. This may change the output ordering slightly when there are multiple simultaneous hotplug events.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_irq.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +0 -4

index 56db9e747464..d64d6895a2e5 100644
@@ -917,10 +917,6 @@ static void i915_hotplug_work_func(struct work_struct *work)
 				| DRM_CONNECTOR_POLL_DISCONNECT;
 			hpd_disabled = true;
 		}
-		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
-			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
-				      connector->name, intel_encoder->hpd_pin);
-		}
 	}
 	 /* if there were no outputs to poll, poll was disabled,
 	  * therefore make sure it's enabled when disabling HPD on

@@ -939,6 +935,8 @@ static void i915_hotplug_work_func(struct work_struct *work)
 			continue;

Quoted 8 lines

drivers/gpu/drm/i915/i915_irq.c static void i915_hotplug_work_func(struct work_struct *work) · line 939

Quoted 4 lines

drivers/gpu/drm/i915/i915_irq.c +2 -0

 		intel_encoder = intel_connector->encoder;
 		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
+			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
+				      connector->name, intel_encoder->hpd_pin);

In light of other conversations, is reporting an irq (even a hotplug one) directly related to KMS? I think DRIVER is more specific in this case.

Perhaps; this patch was just about moving the call around. A more thorough cleanup of the debugging could be a follow-up.

Continue abstracting hotplug storm related functions to clarify the code. This time, abstract hotplug irq storm related hotplug disabling. While at it, clean up the loop iterating over connectors for readability.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_irq.c | 77 ++++++++++++++++++++++++++---------------
 1 file changed, 50 insertions(+), 27 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +1 -2

index d64d6895a2e5..bf4c15d0ea2b 100644
@@ -879,7 +879,7 @@ static void i915_digport_work_func(struct work_struct *work)
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
-#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
+static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv);
 
 static void i915_hotplug_work_func(struct work_struct *work)
 {

@@ -890,7 +890,6 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	struct intel_connector *intel_connector;
 	struct intel_encoder *intel_encoder;
 	struct drm_connector *connector;
-	bool hpd_disabled = false;
 	bool changed = false;
 	u32 hpd_event_bits;
 

Quoted 32 lines · Jani Nikula

Continue abstracting hotplug storm related functions to clarify the code. This time, abstract hotplug irq storm related hotplug disabling. While at it, clean up the loop iterating over connectors for readability.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_irq.c | 77 ++++++++++++++++++++++++++---------------
 1 file changed, 50 insertions(+), 27 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +1 -2

index d64d6895a2e5..bf4c15d0ea2b 100644
@@ -879,7 +879,7 @@ static void i915_digport_work_func(struct work_struct *work)
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
-#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
+static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv);
 
 static void i915_hotplug_work_func(struct work_struct *work)
 {

@@ -890,7 +890,6 @@ static void i915_hotplug_work_func(struct work_struct *work)
      struct intel_connector *intel_connector;
      struct intel_encoder *intel_encoder;
      struct drm_connector *connector;
-     bool hpd_disabled = false;
      bool changed = false;
      u32 hpd_event_bits;
 

drivers/gpu/drm/i915/i915_irq.c static void i915_hotplug_work_func(struct work_struct *work) · line 904

Quoted 4 lines

drivers/gpu/drm/i915/i915_irq.c +0 -1

@@ -901,31 +900,9 @@ static void i915_hotplug_work_func(struct work_struct *work)
 
      hpd_event_bits = dev_priv->hotplug.event_bits;
      dev_priv->hotplug.event_bits = 0;
-     list_for_each_entry(connector, &mode_config->connector_list, head) {

Random comment: We have piles of connector_list walking in probe codde, and DP MST adds/removes them without much thought really users of these. Dave? Do we need a connector_list spinlock?

Just grabbing one of the modeset locks won't cut it I think since it'll serialize way too much.

Quoted 42 lines · Daniel Vetter

Quoted 32 lines · Jani Nikula

Continue abstracting hotplug storm related functions to clarify the code. This time, abstract hotplug irq storm related hotplug disabling. While at it, clean up the loop iterating over connectors for readability.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_irq.c | 77 ++++++++++++++++++++++++++---------------
 1 file changed, 50 insertions(+), 27 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +1 -2

index d64d6895a2e5..bf4c15d0ea2b 100644
@@ -879,7 +879,7 @@ static void i915_digport_work_func(struct work_struct *work)
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
-#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
+static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv);
 
 static void i915_hotplug_work_func(struct work_struct *work)
 {

@@ -890,7 +890,6 @@ static void i915_hotplug_work_func(struct work_struct *work)
      struct intel_connector *intel_connector;
      struct intel_encoder *intel_encoder;
      struct drm_connector *connector;
-     bool hpd_disabled = false;
      bool changed = false;
      u32 hpd_event_bits;
 

drivers/gpu/drm/i915/i915_irq.c static void i915_hotplug_work_func(struct work_struct *work) · line 904

Quoted 4 lines

drivers/gpu/drm/i915/i915_irq.c +0 -1

@@ -901,31 +900,9 @@ static void i915_hotplug_work_func(struct work_struct *work)
 
      hpd_event_bits = dev_priv->hotplug.event_bits;
      dev_priv->hotplug.event_bits = 0;
-     list_for_each_entry(connector, &mode_config->connector_list, head) {

Random comment: We have piles of connector_list walking in probe codde, and DP MST adds/removes them without much thought really users of these. Dave? Do we need a connector_list spinlock?

Quoted 2 lines

Just grabbing one of the modeset locks won't cut it I think since it'll serialize way too much.

In my tree currently this code grabs mode_config->mutex, so it should be fine wrt
the connector list disappearing, as removing connectors is protected my mode_config->mutex
as well.

if we do add a spinlock, we need to be careful about dropping it and restarting the loops etc, as I would guess there are a fair few paths we don't want to descend
holding the spin lock, due to possible sleeping/scheduling etc.

I think the intel_hpd_irq_reenable_work is the one that stands out to me, the short
hotplug paths don't traverse connector lists deliberately to avoid the problem.

Make Paulo happier.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_irq.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +18 -11

index 2d3b2ccf9ce4..a6fbe6443d63 100644
@@ -1257,8 +1257,10 @@ static bool i9xx_port_hotplug_long_detect(enum port port, u32 val)
 
 /* 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])
+			     u32 hotplug_trigger, u32 dig_hotplug_reg,
+			     const u32 hpd[HPD_NUM_PINS])
 {
+	enum port port;
 	int i;
 
 	*pin_mask = 0;

@@ -1268,12 +1270,14 @@ static void pch_get_hpd_pins(u32 *pin_mask, u32 *long_mask,
 		return;
 
 	for_each_hpd_pin(i) {
-		if (hpd[i] & hotplug_trigger) {
-			*pin_mask |= BIT(i);
+		if ((hpd[i] & hotplug_trigger) == 0)
+			continue;
 
-			if (pch_port_hotplug_long_detect(intel_hpd_pin_to_port(i), dig_hotplug_reg))
-				*long_mask |= BIT(i);
-		}
+		*pin_mask |= BIT(i);
+
+		port = intel_hpd_pin_to_port(i);
+		if (pch_port_hotplug_long_detect(port, dig_hotplug_reg))
+			*long_mask |= BIT(i);
 	}
 
 	DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x, dig 0x%08x, pins 0x%08x\n",

@@ -1285,6 +1289,7 @@ static void pch_get_hpd_pins(u32 *pin_mask, u32 *long_mask,
 static void i9xx_get_hpd_pins(u32 *pin_mask, u32 *long_mask,
 			      u32 hotplug_trigger, const u32 hpd[HPD_NUM_PINS])
 {
+	enum port port;
 	int i;
 
 	*pin_mask = 0;

@@ -1294,12 +1299,14 @@ static void i9xx_get_hpd_pins(u32 *pin_mask, u32 *long_mask,
 		return;
 
 	for_each_hpd_pin(i) {
-		if (hpd[i] & hotplug_trigger) {
-			*pin_mask |= BIT(i);
+		if ((hpd[i] & hotplug_trigger) == 0)
+			continue;
 
-			if (i9xx_port_hotplug_long_detect(intel_hpd_pin_to_port(i), hotplug_trigger))
-				*long_mask |= BIT(i);
-		}
+		*pin_mask |= BIT(i);
+
+		port = intel_hpd_pin_to_port(i);
+		if (i9xx_port_hotplug_long_detect(port, hotplug_trigger))
+			*long_mask |= BIT(i);
 	}
 
 	DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x, pins 0x%08x\n",

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Some cleanups, but mostly move all platform independent, generic hotplug code to new intel_hotplug.c.

The expected follow-up would be to document hotplug with a DOC: comment and add kerneldoc for the most important functions.