[PATCH v3 0/7] Add HPD interrupt storm detection.

[PATCH v3 0/7] Add HPD interrupt storm detection.

[Egbert Eich]

This set of patches contains the remaining bits to add IRQ strom detection and
disabling.
Daniel has asked me to send this batch as soon as possible even though this
latest batch has not yet been retested, yet.


Egbert Eich (7):
  drm/i915: Add HPD IRQ storm detection (v4)
  drm/i915: (re)init HPD interrupt storm statistics
  drm/i915: Mask out the HPD irq bits before setting them individually.
  drm/i915: Disable HPD interrupt on pin when irq storm is detected (v2)
  drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v3)
  drm/i915: Add bit field to record which pins have received HPD events
    (v2)
  drm/i915: Only reprobe display on encoder which has received an HPD
    event

 drivers/gpu/drm/i915/i915_drv.h   |  12 ++
 drivers/gpu/drm/i915/i915_irq.c   | 224 ++++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_crt.c  |   6 +-
 drivers/gpu/drm/i915/intel_dp.c   |   1 -
 drivers/gpu/drm/i915/intel_drv.h  |   4 +
 drivers/gpu/drm/i915/intel_hdmi.c |   1 -
 drivers/gpu/drm/i915/intel_sdvo.c |   5 +-
 drivers/gpu/drm/i915/intel_tv.c   |   2 +-
 8 files changed, 221 insertions(+), 34 deletions(-)

-- 
1.8.1.4

[PATCH v3 1/7] drm/i915: Add HPD IRQ storm detection (v4)

[Egbert Eich]

From: Egbert Eich <eich@suse.de>

Add a hotplug IRQ storm detection (triggered when a hotplug interrupt
fires more than 5 times / sec).
Rationale:
Despite of the many attempts to fix the problem with noisy hotplug
interrupt lines we are still seeing systems which have issues:
Once cause of noise seems to be bad routing of the hotplug line
on the board: cross talk from other signals seems to cause erronous
hotplug interrupts. This has been documented as an erratum for the
the i945GM chipset and thus hotplug support was disabled for this
chipset model but others seem to have this problem, too.

We have seen this issue on a G35 motherboard for example:
Even different motherboards of the same model seem to behave
differently: while some only see only around 10-100 interrupts/s
others seem to see 5k or more.
We've also observed a dependency on the selected video mode.

Also on certain laptops interrupt noise seems to occur duing
battery charging when the battery is at a certain charge levels.

Thus we add a simple algorithm here that detects an 'interrupt storm'
condition.

v2: Fixed comment.
v3: Reordered drm_i915_private: moved hpd state tracking to hotplug work stuff.
v4: Followed by Jesse Barnes to use a time_..() macro.

Signed-off-by: Egbert Eich <eich@suse.de>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.h |  9 ++++++
 drivers/gpu/drm/i915/i915_irq.c | 66 +++++++++++++++++++++++++++++++++--------
 2 files changed, 63 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 44fca0b..83fc1a6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -929,6 +929,15 @@ typedef struct drm_i915_private {
 
 	struct work_struct hotplug_work;
 	bool enable_hotplug_processing;
+	struct {
+		unsigned long hpd_last_jiffies;
+		int hpd_cnt;
+		enum {
+			HPD_ENABLED = 0,
+			HPD_DISABLED = 1,
+			HPD_MARK_DISABLED = 2
+		} hpd_mark;
+	} hpd_stats[HPD_NUM_PINS];
 
 	int num_pch_pll;
 	int num_plane;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4c5bdd0..32b5527 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -582,6 +582,37 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
 	queue_work(dev_priv->wq, &dev_priv->rps.work);
 }
 
+#define HPD_STORM_DETECT_PERIOD 1000
+
+static inline void hotplug_irq_storm_detect(struct drm_device *dev,
+					    u32 hotplug_trigger,
+					    const u32 *hpd)
+{
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	unsigned long irqflags;
+	int i;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+
+	for (i = 1; i < HPD_NUM_PINS; i++) {
+		if ((hpd[i] & hotplug_trigger) &&
+		    dev_priv->hpd_stats[i].hpd_mark == HPD_ENABLED) {
+			if (!time_in_range(jiffies, dev_priv->hpd_stats[i].hpd_last_jiffies,
+					  dev_priv->hpd_stats[i].hpd_last_jiffies
+					  + msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
+				dev_priv->hpd_stats[i].hpd_last_jiffies = jiffies;
+				dev_priv->hpd_stats[i].hpd_cnt = 0;
+			} else if (dev_priv->hpd_stats[i].hpd_cnt > 5) {
+				dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
+				DRM_DEBUG_KMS("HPD interrupt storm detected on  PIN %d\n", i);
+			} else
+				dev_priv->hpd_stats[i].hpd_cnt++;
+		}
+	}
+
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
+
 static void gmbus_irq_handler(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = (drm_i915_private_t *) dev->dev_private;
@@ -650,13 +681,15 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 		/* Consume port.  Then clear IIR or we'll miss events */
 		if (iir & I915_DISPLAY_PORT_INTERRUPT) {
 			u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
+			u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
 
 			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
 					 hotplug_status);
-			if (hotplug_status & HOTPLUG_INT_STATUS_I915)
+			if (hotplug_trigger) {
+				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
 				queue_work(dev_priv->wq,
 					   &dev_priv->hotplug_work);
-
+			}
 			I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status);
 			I915_READ(PORT_HOTPLUG_STAT);
 		}
@@ -680,10 +713,12 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	int pipe;
+	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK;
 
-	if (pch_iir & SDE_HOTPLUG_MASK)
+	if (hotplug_trigger) {
+		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_ibx);
 		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
-
+	}
 	if (pch_iir & SDE_AUDIO_POWER_MASK)
 		DRM_DEBUG_DRIVER("PCH audio power change on port %d\n",
 				 (pch_iir & SDE_AUDIO_POWER_MASK) >>
@@ -726,10 +761,12 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	int pipe;
+	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
 
-	if (pch_iir & SDE_HOTPLUG_MASK_CPT)
+	if (hotplug_trigger) {
+		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_cpt);
 		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
-
+	}
 	if (pch_iir & SDE_AUDIO_POWER_MASK_CPT)
 		DRM_DEBUG_DRIVER("PCH audio power change on port %d\n",
 				 (pch_iir & SDE_AUDIO_POWER_MASK_CPT) >>
@@ -2607,13 +2644,15 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
 		if ((I915_HAS_HOTPLUG(dev)) &&
 		    (iir & I915_DISPLAY_PORT_INTERRUPT)) {
 			u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
+			u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
 
 			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
 				  hotplug_status);
-			if (hotplug_status & HOTPLUG_INT_STATUS_I915)
+			if (hotplug_trigger) {
+				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
 				queue_work(dev_priv->wq,
 					   &dev_priv->hotplug_work);
-
+			}
 			I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status);
 			POSTING_READ(PORT_HOTPLUG_STAT);
 		}
@@ -2840,15 +2879,18 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
 		/* Consume port.  Then clear IIR or we'll miss events */
 		if (iir & I915_DISPLAY_PORT_INTERRUPT) {
 			u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
+			u32 hotplug_trigger = hotplug_status & (IS_G4X(dev) ?
+								  HOTPLUG_INT_STATUS_G4X :
+								  HOTPLUG_INT_STATUS_I965);
 
 			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
 				  hotplug_status);
-			if (hotplug_status & (IS_G4X(dev) ?
-					      HOTPLUG_INT_STATUS_G4X :
-					      HOTPLUG_INT_STATUS_I965))
+			if (hotplug_trigger) {
+				hotplug_irq_storm_detect(dev, hotplug_trigger,
+							 IS_G4X(dev) ? hpd_status_gen4 : hpd_status_i965);
 				queue_work(dev_priv->wq,
 					   &dev_priv->hotplug_work);
-
+			}
 			I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status);
 			I915_READ(PORT_HOTPLUG_STAT);
 		}
-- 
1.8.1.4

[PATCH v3 2/7] drm/i915: (re)init HPD interrupt storm statistics

[Egbert Eich]

From: Egbert Eich <eich@suse.de>

When an encoder is shared on several connectors there is only
one hotplug line, thus this line needs to be shared among these
connectors.
If HPD detect only works reliably on a subset of those connectors,
we want to poll the others. Thus we need to make sure that storm
detection doesn't mess up the settings for those connectors.
Therefore we store the settings in the intel_connector struct and
restore them from there.
If nothing is set but the encoder has a hpd_pin set we assume this
connector is hotplug capable.
On init/reset we make sure the polled state of the connectors
is (re)set to the default value, the HPD interrupts are marked
enabled.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/i915_irq.c   | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_crt.c  |  6 ++----
 drivers/gpu/drm/i915/intel_dp.c   |  1 -
 drivers/gpu/drm/i915/intel_drv.h  |  4 ++++
 drivers/gpu/drm/i915/intel_hdmi.c |  1 -
 drivers/gpu/drm/i915/intel_sdvo.c |  5 ++---
 drivers/gpu/drm/i915/intel_tv.c   |  2 +-
 7 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 32b5527..5408a3a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3045,7 +3045,20 @@ void intel_irq_init(struct drm_device *dev)
 void intel_hpd_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	struct drm_connector *connector;
+	int i;
 
+	for (i = 1; i < HPD_NUM_PINS; i++) {
+		dev_priv->hpd_stats[i].hpd_cnt = 0;
+		dev_priv->hpd_stats[i].hpd_mark = HPD_ENABLED;
+	}
+	list_for_each_entry(connector, &mode_config->connector_list, head) {
+		struct intel_connector *intel_connector = to_intel_connector(connector);
+		connector->polled = intel_connector->polled;
+		if (!connector->polled && I915_HAS_HOTPLUG(dev) && intel_connector->encoder->hpd_pin > HPD_NONE)
+			connector->polled = DRM_CONNECTOR_POLL_HPD;
+	}
 	if (dev_priv->display.hpd_irq_setup)
 		dev_priv->display.hpd_irq_setup(dev);
 }
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 1ae2d7f..c063b9f 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -793,10 +793,8 @@ void intel_crt_init(struct drm_device *dev)
 
 	drm_sysfs_connector_add(connector);
 
-	if (I915_HAS_HOTPLUG(dev))
-		connector->polled = DRM_CONNECTOR_POLL_HPD;
-	else
-		connector->polled = DRM_CONNECTOR_POLL_CONNECT;
+	if (!I915_HAS_HOTPLUG(dev))
+		intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
 
 	/*
 	 * Configure the automatic hotplug detection stuff
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 482b5e5..1e9b19a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2786,7 +2786,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	drm_connector_init(dev, connector, &intel_dp_connector_funcs, type);
 	drm_connector_helper_add(connector, &intel_dp_connector_helper_funcs);
 
-	connector->polled = DRM_CONNECTOR_POLL_HPD;
 	connector->interlace_allowed = true;
 	connector->doublescan_allowed = 0;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d7bd031..a05fde7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -171,6 +171,10 @@ struct intel_connector {
 
 	/* Cached EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */
 	struct edid *edid;
+
+	/* since POLL and HPD connectors may use the same HPD line keep the native
+	   state of connector->polled in case hotplug storm detection changes it */
+	u8 polled;
 };
 
 struct intel_crtc_config {
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index ee4a8da..8912201 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -998,7 +998,6 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 			   DRM_MODE_CONNECTOR_HDMIA);
 	drm_connector_helper_add(connector, &intel_hdmi_connector_helper_funcs);
 
-	connector->polled = DRM_CONNECTOR_POLL_HPD;
 	connector->interlace_allowed = 1;
 	connector->doublescan_allowed = 0;
 
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 298dc85..64b8b40 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2276,7 +2276,6 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 	connector = &intel_connector->base;
 	if (intel_sdvo_get_hotplug_support(intel_sdvo) &
 		intel_sdvo_connector->output_flag) {
-		connector->polled = DRM_CONNECTOR_POLL_HPD;
 		intel_sdvo->hotplug_active |= intel_sdvo_connector->output_flag;
 		/* Some SDVO devices have one-shot hotplug interrupts.
 		 * Ensure that they get re-enabled when an interrupt happens.
@@ -2284,7 +2283,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 		intel_encoder->hot_plug = intel_sdvo_enable_hotplug;
 		intel_sdvo_enable_hotplug(intel_encoder);
 	} else {
-		connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
+		intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
 	}
 	encoder->encoder_type = DRM_MODE_ENCODER_TMDS;
 	connector->connector_type = DRM_MODE_CONNECTOR_DVID;
@@ -2353,7 +2352,7 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
 
 	intel_connector = &intel_sdvo_connector->base;
 	connector = &intel_connector->base;
-	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
+	intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
 	encoder->encoder_type = DRM_MODE_ENCODER_DAC;
 	connector->connector_type = DRM_MODE_CONNECTOR_VGA;
 
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 6673726..b945bc5 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1613,7 +1613,7 @@ intel_tv_init(struct drm_device *dev)
 	 *
 	 * More recent chipsets favour HDMI rather than integrated S-Video.
 	 */
-	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
+	intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
 
 	drm_connector_init(dev, connector, &intel_tv_connector_funcs,
 			   DRM_MODE_CONNECTOR_SVIDEO);
-- 
1.8.1.4

[PATCH v3 3/7] drm/i915: Mask out the HPD irq bits before setting them individually.

[Egbert Eich]

From: Egbert Eich <eich@suse.de>

To disable previously enabled HPD IRQs we need to reset them and
set the enabled ones individually.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5408a3a..a3f1ac4 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2117,9 +2117,11 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
 	u32 hotplug;
 
 	if (HAS_PCH_IBX(dev)) {
+		mask &= ~SDE_HOTPLUG_MASK;
 		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
 			mask |= hpd_ibx[intel_encoder->hpd_pin];
 	} else {
+		mask &= ~SDE_HOTPLUG_MASK_CPT;
 		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
 			mask |= hpd_cpt[intel_encoder->hpd_pin];
 	}
-- 
1.8.1.4

[PATCH v3 4/7] drm/i915: Disable HPD interrupt on pin when irq storm is detected (v2)

[Egbert Eich]

From: Egbert Eich <eich@suse.de>

This patch disables hotplug interrupts if an 'interrupt storm'
has been detected.
Noise on the interrupt line renders the hotplug interrupt useless:
each hotplug event causes the devices to be rescanned which will
will only increase the system load.
Thus disable the hotplug interrupts and fall back to periodic
device polling.

v2: Fixed cleanup typo.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/i915_irq.c | 71 +++++++++++++++++++++++++++++++----------
 1 file changed, 54 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a3f1ac4..d0e6f6d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -88,7 +88,8 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
-
+static void ibx_hpd_irq_setup(struct drm_device *dev);
+static void i915_hpd_irq_setup(struct drm_device *dev);
 
 /* For display hotplug interrupt */
 static void
@@ -342,7 +343,11 @@ static void i915_hotplug_work_func(struct work_struct *work)
 						    hotplug_work);
 	struct drm_device *dev = dev_priv->dev;
 	struct drm_mode_config *mode_config = &dev->mode_config;
-	struct intel_encoder *encoder;
+	struct intel_connector *intel_connector;
+	struct intel_encoder *intel_encoder;
+	struct drm_connector *connector;
+	unsigned long irqflags;
+	bool connector_disabled = false;
 
 	/* HPD irq before everything is fully set up. */
 	if (!dev_priv->enable_hotplug_processing)
@@ -351,9 +356,29 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	mutex_lock(&mode_config->mutex);
 	DRM_DEBUG_KMS("running encoder hotplug functions\n");
 
-	list_for_each_entry(encoder, &mode_config->encoder_list, base.head)
-		if (encoder->hot_plug)
-			encoder->hot_plug(encoder);
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+	list_for_each_entry(connector, &mode_config->connector_list, head) {
+		intel_connector = to_intel_connector(connector);
+		intel_encoder = intel_connector->encoder;
+		if (intel_encoder->hpd_pin > HPD_NONE &&
+		    dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_MARK_DISABLED &&
+		    connector->polled == DRM_CONNECTOR_POLL_HPD) {
+			pr_warn("HPD interrupt storm detected on connector %s disabling\n",
+				drm_get_connector_name(connector));
+			dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark = HPD_DISABLED;
+			connector->polled = DRM_CONNECTOR_POLL_CONNECT
+				| DRM_CONNECTOR_POLL_DISCONNECT;
+			connector_disabled = true;
+		}
+	}
+	if (connector_disabled)
+		drm_kms_helper_poll_enable(dev); /* if there were no outputs to poll, poll is disabled */
+
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+
+	list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
+		if (intel_encoder->hot_plug)
+			intel_encoder->hot_plug(intel_encoder);
 
 	mutex_unlock(&mode_config->mutex);
 
@@ -584,13 +609,14 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
 
 #define HPD_STORM_DETECT_PERIOD 1000
 
-static inline void hotplug_irq_storm_detect(struct drm_device *dev,
+static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
 					    u32 hotplug_trigger,
 					    const u32 *hpd)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	unsigned long irqflags;
 	int i;
+	bool ret = false;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 
@@ -605,12 +631,15 @@ static inline void hotplug_irq_storm_detect(struct drm_device *dev,
 			} else if (dev_priv->hpd_stats[i].hpd_cnt > 5) {
 				dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
 				DRM_DEBUG_KMS("HPD interrupt storm detected on  PIN %d\n", i);
+				ret = true;
 			} else
 				dev_priv->hpd_stats[i].hpd_cnt++;
 		}
 	}
 
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+
+	return ret;
 }
 
 static void gmbus_irq_handler(struct drm_device *dev)
@@ -686,7 +715,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
 					 hotplug_status);
 			if (hotplug_trigger) {
-				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
+				if( hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915))
+					i915_hpd_irq_setup(dev);
 				queue_work(dev_priv->wq,
 					   &dev_priv->hotplug_work);
 			}
@@ -716,7 +746,8 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
 	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK;
 
 	if (hotplug_trigger) {
-		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_ibx);
+		if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_ibx))
+			ibx_hpd_irq_setup(dev);
 		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
 	}
 	if (pch_iir & SDE_AUDIO_POWER_MASK)
@@ -764,7 +795,8 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
 	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
 
 	if (hotplug_trigger) {
-		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_cpt);
+		if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_cpt))
+			ibx_hpd_irq_setup(dev);
 		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
 	}
 	if (pch_iir & SDE_AUDIO_POWER_MASK_CPT)
@@ -2119,11 +2151,13 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
 	if (HAS_PCH_IBX(dev)) {
 		mask &= ~SDE_HOTPLUG_MASK;
 		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
-			mask |= hpd_ibx[intel_encoder->hpd_pin];
+			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
+				mask |= hpd_ibx[intel_encoder->hpd_pin];
 	} else {
 		mask &= ~SDE_HOTPLUG_MASK_CPT;
 		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
-			mask |= hpd_cpt[intel_encoder->hpd_pin];
+			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
+				mask |= hpd_cpt[intel_encoder->hpd_pin];
 	}
 
 	I915_WRITE(SDEIMR, ~mask);
@@ -2651,7 +2685,8 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
 			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
 				  hotplug_status);
 			if (hotplug_trigger) {
-				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
+				if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915))
+					i915_hpd_irq_setup(dev);
 				queue_work(dev_priv->wq,
 					   &dev_priv->hotplug_work);
 			}
@@ -2801,7 +2836,7 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	struct drm_mode_config *mode_config = &dev->mode_config;
-	struct intel_encoder *encoder;
+	struct intel_encoder *intel_encoder;
 	u32 hotplug_en;
 
 	if (I915_HAS_HOTPLUG(dev)) {
@@ -2809,8 +2844,9 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
 		hotplug_en &= ~HOTPLUG_INT_EN_MASK;
 		/* Note HDMI and DP share hotplug bits */
 		/* enable bits are the same for all generations */
-		list_for_each_entry(encoder, &mode_config->encoder_list, base.head)
-			hotplug_en |= hpd_mask_i915[encoder->hpd_pin];
+		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
+			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
+				hotplug_en |= hpd_mask_i915[intel_encoder->hpd_pin];
 		/* Programming the CRT detection parameters tends
 		   to generate a spurious hotplug event about three
 		   seconds later.  So just do it once.
@@ -2888,8 +2924,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
 			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
 				  hotplug_status);
 			if (hotplug_trigger) {
-				hotplug_irq_storm_detect(dev, hotplug_trigger,
-							 IS_G4X(dev) ? hpd_status_gen4 : hpd_status_i965);
+				if (hotplug_irq_storm_detect(dev, hotplug_trigger,
+							    IS_G4X(dev) ? hpd_status_gen4 : hpd_status_i965))
+					i915_hpd_irq_setup(dev);
 				queue_work(dev_priv->wq,
 					   &dev_priv->hotplug_work);
 			}
-- 
1.8.1.4

[PATCH v3 5/7] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v3)

[Egbert Eich]

From: Egbert Eich <eich@suse.de>

We disable hoptplug detection when we encounter a hotplug event
storm. Still hotplug detection is required on some outputs (like
Display Port). The interrupt storm may be only temporary (on certain
Dell Laptops for instance it happens at certain charging states of
the system). Thus we enable it after a certain grace period (2 minutes).
Should the interrupt storm persist it will be detected immediately
and it will be disabled again.

v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state tracker.
v3: Clarified loop start value,
    Removed superfluous test for Ivybridge and Haswell,
    Restructured loop to avoid deep nesting (all suggested by Ville Syrjälä)

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/i915_drv.h |  2 ++
 drivers/gpu/drm/i915/i915_irq.c | 49 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 83fc1a6..a3ed2e3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -938,6 +938,8 @@ typedef struct drm_i915_private {
 			HPD_MARK_DISABLED = 2
 		} hpd_mark;
 	} hpd_stats[HPD_NUM_PINS];
+#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
+	struct timer_list hotplug_reenable_timer;
 
 	int num_pch_pll;
 	int num_plane;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d0e6f6d..1a00533 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -371,8 +371,11 @@ static void i915_hotplug_work_func(struct work_struct *work)
 			connector_disabled = true;
 		}
 	}
-	if (connector_disabled)
+	if (connector_disabled) {
 		drm_kms_helper_poll_enable(dev); /* if there were no outputs to poll, poll is disabled */
+		mod_timer(&dev_priv->hotplug_reenable_timer,
+			  jiffies + msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY));
+	}
 
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
@@ -2348,6 +2351,8 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 	if (!dev_priv)
 		return;
 
+	del_timer_sync(&dev_priv->hotplug_reenable_timer);
+
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
 
@@ -2369,6 +2374,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 	if (!dev_priv)
 		return;
 
+	del_timer_sync(&dev_priv->hotplug_reenable_timer);
+
 	I915_WRITE(HWSTAM, 0xffffffff);
 
 	I915_WRITE(DEIMR, 0xffffffff);
@@ -2745,6 +2752,8 @@ static void i915_irq_uninstall(struct drm_device * dev)
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	int pipe;
 
+	del_timer_sync(&dev_priv->hotplug_reenable_timer);
+
 	if (I915_HAS_HOTPLUG(dev)) {
 		I915_WRITE(PORT_HOTPLUG_EN, 0);
 		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
@@ -2989,6 +2998,8 @@ static void i965_irq_uninstall(struct drm_device * dev)
 	if (!dev_priv)
 		return;
 
+	del_timer_sync(&dev_priv->hotplug_reenable_timer);
+
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
@@ -3004,6 +3015,40 @@ static void i965_irq_uninstall(struct drm_device * dev)
 	I915_WRITE(IIR, I915_READ(IIR));
 }
 
+static void i915_reenable_hotplug_timer_func(unsigned long data)
+{
+	drm_i915_private_t *dev_priv = (drm_i915_private_t *)data;
+	struct drm_device *dev = dev_priv->dev;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	unsigned long irqflags;
+	int i;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+	for (i = (HPD_NONE + 1); i < HPD_NUM_PINS; i++) {
+		struct drm_connector *connector;
+
+		if (dev_priv->hpd_stats[i].hpd_mark != HPD_MARK_DISABLED)
+			continue;
+
+		dev_priv->hpd_stats[i].hpd_mark = HPD_ENABLED;
+
+		list_for_each_entry(connector, &mode_config->connector_list, head) {
+			struct intel_connector *intel_connector = to_intel_connector(connector);
+
+			if (intel_connector->encoder->hpd_pin == i) {
+				if (connector->polled != intel_connector->polled)
+					DRM_DEBUG_DRIVER("Reenabling HPD on connector %s\n",
+							 drm_get_connector_name(connector));
+				connector->polled = intel_connector->polled;
+				if (!connector->polled)
+					connector->polled = DRM_CONNECTOR_POLL_HPD;
+			}
+		}
+		dev_priv->display.hpd_irq_setup(dev);
+	}
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
+
 void intel_irq_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3016,6 +3061,8 @@ void intel_irq_init(struct drm_device *dev)
 	setup_timer(&dev_priv->gpu_error.hangcheck_timer,
 		    i915_hangcheck_elapsed,
 		    (unsigned long) dev);
+	setup_timer(&dev_priv->hotplug_reenable_timer, i915_reenable_hotplug_timer_func,
+		    (unsigned long) dev_priv);
 
 	pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
 
-- 
1.8.1.4

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

[PATCH v3 6/7] drm/i915: Add bit field to record which pins have received HPD events (v2)

[Egbert Eich]

From: Egbert Eich <eich@suse.de>

This way it is possible to limit 're'-detect() of displays to connectors
which have received an HPD event.

v2: Reordered drm_i915_private: Move hpd_event_bits to hpd state tracking.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a3ed2e3..907e290 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -938,6 +938,7 @@ typedef struct drm_i915_private {
 			HPD_MARK_DISABLED = 2
 		} hpd_mark;
 	} hpd_stats[HPD_NUM_PINS];
+	u32 hpd_event_bits;
 #define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
 	struct timer_list hotplug_reenable_timer;
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1a00533..92041b9 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -348,6 +348,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	struct drm_connector *connector;
 	unsigned long irqflags;
 	bool connector_disabled = false;
+	u32 hpd_event_bits;
 
 	/* HPD irq before everything is fully set up. */
 	if (!dev_priv->enable_hotplug_processing)
@@ -357,6 +358,9 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	DRM_DEBUG_KMS("running encoder hotplug functions\n");
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+
+	hpd_event_bits = dev_priv->hpd_event_bits;
+	dev_priv->hpd_event_bits = 0;
 	list_for_each_entry(connector, &mode_config->connector_list, head) {
 		intel_connector = to_intel_connector(connector);
 		intel_encoder = intel_connector->encoder;
@@ -370,6 +374,10 @@ static void i915_hotplug_work_func(struct work_struct *work)
 				| DRM_CONNECTOR_POLL_DISCONNECT;
 			connector_disabled = true;
 		}
+		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
+			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
+				      drm_get_connector_name(connector), intel_encoder->hpd_pin);
+		}
 	}
 	if (connector_disabled) {
 		drm_kms_helper_poll_enable(dev); /* if there were no outputs to poll, poll is disabled */
@@ -626,6 +634,7 @@ static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
 	for (i = 1; i < HPD_NUM_PINS; i++) {
 		if ((hpd[i] & hotplug_trigger) &&
 		    dev_priv->hpd_stats[i].hpd_mark == HPD_ENABLED) {
+			dev_priv->hpd_event_bits |= (1 << i);
 			if (!time_in_range(jiffies, dev_priv->hpd_stats[i].hpd_last_jiffies,
 					  dev_priv->hpd_stats[i].hpd_last_jiffies
 					  + msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
@@ -633,6 +642,7 @@ static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
 				dev_priv->hpd_stats[i].hpd_cnt = 0;
 			} else if (dev_priv->hpd_stats[i].hpd_cnt > 5) {
 				dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
+				dev_priv->hpd_event_bits &= ~(1 << i);
 				DRM_DEBUG_KMS("HPD interrupt storm detected on  PIN %d\n", i);
 				ret = true;
 			} else
-- 
1.8.1.4

[PATCH v3 7/7] drm/i915: Only reprobe display on encoder which has received an HPD event

[Egbert Eich]

From: Egbert Eich <eich@suse.de>

Instead of calling into the DRM helper layer to poll all connectors for
changes in connected displays probe only those connectors which have
received a hotplug event.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/i915_irq.c | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 92041b9..7788536 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -334,6 +334,24 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 						     crtc);
 }
 
+/**
+ * drm_helper_hpd_irq_single_connector_event() - call this function with mode_config.mutex lock held
+ */
+
+static int intel_hpd_irq_single_connector_event(struct drm_device *dev, struct drm_connector *connector)
+{
+	enum drm_connector_status old_status;
+
+	old_status = connector->status;
+
+	connector->status = connector->funcs->detect(connector, false);
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n",
+		      connector->base.id,
+		      drm_get_connector_name(connector),
+		      old_status, connector->status);
+	return (old_status != connector->status);
+}
+
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
@@ -348,6 +366,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	struct drm_connector *connector;
 	unsigned long irqflags;
 	bool connector_disabled = false;
+	bool changed = false;
 	u32 hpd_event_bits;
 
 	/* HPD irq before everything is fully set up. */
@@ -387,14 +406,20 @@ static void i915_hotplug_work_func(struct work_struct *work)
 
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
-	list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
-		if (intel_encoder->hot_plug)
-			intel_encoder->hot_plug(intel_encoder);
-
+	list_for_each_entry(connector, &mode_config->connector_list, head) {
+		intel_connector = to_intel_connector(connector);
+		intel_encoder = intel_connector->encoder;
+		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
+			if (intel_encoder->hot_plug)
+				intel_encoder->hot_plug(intel_encoder);
+			if (intel_hpd_irq_single_connector_event(dev, connector))
+				changed = true;
+		}
+	}
 	mutex_unlock(&mode_config->mutex);
 
-	/* Just fire off a uevent and let userspace tell us what to do */
-	drm_helper_hpd_irq_event(dev);
+	if (changed)
+		drm_kms_helper_hotplug_event(dev);
 }
 
 static void ironlake_handle_rps_change(struct drm_device *dev)
-- 
1.8.1.4

Re: [PATCH v3 1/7] drm/i915: Add HPD IRQ storm detection (v4)

[Jani Nikula]

Hi Egbert -

Up front, I haven't been following this series or read any of the
previous review comments. Please bear with me, and feel free to direct
me to earlier comments if I'm in contradiction.

On Tue, 09 Apr 2013, Egbert Eich <eich@freedesktop.org> wrote:
> From: Egbert Eich <eich@suse.de>
>
> Add a hotplug IRQ storm detection (triggered when a hotplug interrupt
> fires more than 5 times / sec).

Okay, this is theoretical, but a display port sink could do more than
that many hpd irq requests when connected. Which leads me to wonder in
general if the storm detection should be different for hot plug
vs. unplug and hpd irq events.

> Rationale:
> Despite of the many attempts to fix the problem with noisy hotplug
> interrupt lines we are still seeing systems which have issues:
> Once cause of noise seems to be bad routing of the hotplug line
> on the board: cross talk from other signals seems to cause erronous
> hotplug interrupts. This has been documented as an erratum for the
> the i945GM chipset and thus hotplug support was disabled for this
> chipset model but others seem to have this problem, too.
>
> We have seen this issue on a G35 motherboard for example:
> Even different motherboards of the same model seem to behave
> differently: while some only see only around 10-100 interrupts/s
> others seem to see 5k or more.
> We've also observed a dependency on the selected video mode.
>
> Also on certain laptops interrupt noise seems to occur duing
> battery charging when the battery is at a certain charge levels.

Have you observed difference between hot plug/unplug?

Has this been a problem on PCH split platforms, i.e. since ilk/gen5?

> Thus we add a simple algorithm here that detects an 'interrupt storm'
> condition.
>
> v2: Fixed comment.
> v3: Reordered drm_i915_private: moved hpd state tracking to hotplug work stuff.
> v4: Followed by Jesse Barnes to use a time_..() macro.
>
> Signed-off-by: Egbert Eich <eich@suse.de>
> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  9 ++++++
>  drivers/gpu/drm/i915/i915_irq.c | 66 +++++++++++++++++++++++++++++++++--------
>  2 files changed, 63 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 44fca0b..83fc1a6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -929,6 +929,15 @@ typedef struct drm_i915_private {
>  
>  	struct work_struct hotplug_work;
>  	bool enable_hotplug_processing;
> +	struct {
> +		unsigned long hpd_last_jiffies;
> +		int hpd_cnt;
> +		enum {
> +			HPD_ENABLED = 0,
> +			HPD_DISABLED = 1,
> +			HPD_MARK_DISABLED = 2
> +		} hpd_mark;
> +	} hpd_stats[HPD_NUM_PINS];

With all the hotplug stuff being added, I think it's getting time to
group all the hotplug stuff under a struct.

>  	int num_pch_pll;
>  	int num_plane;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 4c5bdd0..32b5527 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -582,6 +582,37 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
>  	queue_work(dev_priv->wq, &dev_priv->rps.work);
>  }
>  
> +#define HPD_STORM_DETECT_PERIOD 1000
> +
> +static inline void hotplug_irq_storm_detect(struct drm_device *dev,
> +					    u32 hotplug_trigger,
> +					    const u32 *hpd)

I think you should just add the bool return status right here instead of
postponing until the later patch that needs it.

> +{
> +	drm_i915_private_t *dev_priv = dev->dev_private;
> +	unsigned long irqflags;
> +	int i;
> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +
> +	for (i = 1; i < HPD_NUM_PINS; i++) {
> +		if ((hpd[i] & hotplug_trigger) &&
> +		    dev_priv->hpd_stats[i].hpd_mark == HPD_ENABLED) {

Bikeshed: You might get a nicer layout below by negating that if and
adding continue.

> +			if (!time_in_range(jiffies, dev_priv->hpd_stats[i].hpd_last_jiffies,
> +					  dev_priv->hpd_stats[i].hpd_last_jiffies
> +					  + msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
> +				dev_priv->hpd_stats[i].hpd_last_jiffies = jiffies;
> +				dev_priv->hpd_stats[i].hpd_cnt = 0;
> +			} else if (dev_priv->hpd_stats[i].hpd_cnt > 5) {
> +				dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
> +				DRM_DEBUG_KMS("HPD interrupt storm detected on  PIN %d\n", i);

Extra space before "PIN".

> +			} else
> +				dev_priv->hpd_stats[i].hpd_cnt++;

If one branch requires braces, then all do.

The above ends up adding HPD_MARK_DISABLED only after there have been
seven interrupts within a second, not five. Maybe you should take out
the magic 5 out of there and #define it, and make it precise.

> +		}
> +	}
> +
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +}
> +
>  static void gmbus_irq_handler(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = (drm_i915_private_t *) dev->dev_private;
> @@ -650,13 +681,15 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  		/* Consume port.  Then clear IIR or we'll miss events */
>  		if (iir & I915_DISPLAY_PORT_INTERRUPT) {
>  			u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
> +			u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
>  
>  			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
>  					 hotplug_status);
> -			if (hotplug_status & HOTPLUG_INT_STATUS_I915)
> +			if (hotplug_trigger) {
> +				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
>  				queue_work(dev_priv->wq,
>  					   &dev_priv->hotplug_work);
> -
> +			}
>  			I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status);
>  			I915_READ(PORT_HOTPLUG_STAT);
>  		}
> @@ -680,10 +713,12 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
>  {
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	int pipe;
> +	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK;
>  
> -	if (pch_iir & SDE_HOTPLUG_MASK)
> +	if (hotplug_trigger) {
> +		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_ibx);
>  		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
> -
> +	}
>  	if (pch_iir & SDE_AUDIO_POWER_MASK)
>  		DRM_DEBUG_DRIVER("PCH audio power change on port %d\n",
>  				 (pch_iir & SDE_AUDIO_POWER_MASK) >>
> @@ -726,10 +761,12 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>  {
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	int pipe;
> +	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
>  
> -	if (pch_iir & SDE_HOTPLUG_MASK_CPT)
> +	if (hotplug_trigger) {
> +		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_cpt);
>  		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
> -
> +	}
>  	if (pch_iir & SDE_AUDIO_POWER_MASK_CPT)
>  		DRM_DEBUG_DRIVER("PCH audio power change on port %d\n",
>  				 (pch_iir & SDE_AUDIO_POWER_MASK_CPT) >>
> @@ -2607,13 +2644,15 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
>  		if ((I915_HAS_HOTPLUG(dev)) &&
>  		    (iir & I915_DISPLAY_PORT_INTERRUPT)) {
>  			u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
> +			u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
>  
>  			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
>  				  hotplug_status);
> -			if (hotplug_status & HOTPLUG_INT_STATUS_I915)
> +			if (hotplug_trigger) {
> +				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
>  				queue_work(dev_priv->wq,
>  					   &dev_priv->hotplug_work);
> -
> +			}
>  			I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status);
>  			POSTING_READ(PORT_HOTPLUG_STAT);
>  		}
> @@ -2840,15 +2879,18 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
>  		/* Consume port.  Then clear IIR or we'll miss events */
>  		if (iir & I915_DISPLAY_PORT_INTERRUPT) {
>  			u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
> +			u32 hotplug_trigger = hotplug_status & (IS_G4X(dev) ?
> +								  HOTPLUG_INT_STATUS_G4X :
> +								  HOTPLUG_INT_STATUS_I965);
>  
>  			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
>  				  hotplug_status);
> -			if (hotplug_status & (IS_G4X(dev) ?
> -					      HOTPLUG_INT_STATUS_G4X :
> -					      HOTPLUG_INT_STATUS_I965))
> +			if (hotplug_trigger) {
> +				hotplug_irq_storm_detect(dev, hotplug_trigger,
> +							 IS_G4X(dev) ? hpd_status_gen4 : hpd_status_i965);
>  				queue_work(dev_priv->wq,
>  					   &dev_priv->hotplug_work);
> -
> +			}
>  			I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status);
>  			I915_READ(PORT_HOTPLUG_STAT);
>  		}
> -- 
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/7] drm/i915: (re)init HPD interrupt storm statistics

[Jani Nikula]

On Tue, 09 Apr 2013, Egbert Eich <eich@freedesktop.org> wrote:
> From: Egbert Eich <eich@suse.de>
>
> When an encoder is shared on several connectors there is only
> one hotplug line, thus this line needs to be shared among these
> connectors.
> If HPD detect only works reliably on a subset of those connectors,
> we want to poll the others. Thus we need to make sure that storm
> detection doesn't mess up the settings for those connectors.
> Therefore we store the settings in the intel_connector struct and
> restore them from there.
> If nothing is set but the encoder has a hpd_pin set we assume this
> connector is hotplug capable.
> On init/reset we make sure the polled state of the connectors
> is (re)set to the default value, the HPD interrupts are marked
> enabled.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/i915/i915_irq.c   | 13 +++++++++++++
>  drivers/gpu/drm/i915/intel_crt.c  |  6 ++----
>  drivers/gpu/drm/i915/intel_dp.c   |  1 -
>  drivers/gpu/drm/i915/intel_drv.h  |  4 ++++
>  drivers/gpu/drm/i915/intel_hdmi.c |  1 -
>  drivers/gpu/drm/i915/intel_sdvo.c |  5 ++---
>  drivers/gpu/drm/i915/intel_tv.c   |  2 +-
>  7 files changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 32b5527..5408a3a 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3045,7 +3045,20 @@ void intel_irq_init(struct drm_device *dev)
>  void intel_hpd_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_mode_config *mode_config = &dev->mode_config;
> +	struct drm_connector *connector;
> +	int i;
>  
> +	for (i = 1; i < HPD_NUM_PINS; i++) {
> +		dev_priv->hpd_stats[i].hpd_cnt = 0;
> +		dev_priv->hpd_stats[i].hpd_mark = HPD_ENABLED;
> +	}
> +	list_for_each_entry(connector, &mode_config->connector_list, head) {
> +		struct intel_connector *intel_connector = to_intel_connector(connector);
> +		connector->polled = intel_connector->polled;
> +		if (!connector->polled && I915_HAS_HOTPLUG(dev) && intel_connector->encoder->hpd_pin > HPD_NONE)
> +			connector->polled = DRM_CONNECTOR_POLL_HPD;
> +	}
>  	if (dev_priv->display.hpd_irq_setup)
>  		dev_priv->display.hpd_irq_setup(dev);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 1ae2d7f..c063b9f 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -793,10 +793,8 @@ void intel_crt_init(struct drm_device *dev)
>  
>  	drm_sysfs_connector_add(connector);
>  
> -	if (I915_HAS_HOTPLUG(dev))
> -		connector->polled = DRM_CONNECTOR_POLL_HPD;
> -	else
> -		connector->polled = DRM_CONNECTOR_POLL_CONNECT;
> +	if (!I915_HAS_HOTPLUG(dev))
> +		intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
>  
>  	/*
>  	 * Configure the automatic hotplug detection stuff
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 482b5e5..1e9b19a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2786,7 +2786,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	drm_connector_init(dev, connector, &intel_dp_connector_funcs, type);
>  	drm_connector_helper_add(connector, &intel_dp_connector_helper_funcs);
>  
> -	connector->polled = DRM_CONNECTOR_POLL_HPD;
>  	connector->interlace_allowed = true;
>  	connector->doublescan_allowed = 0;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d7bd031..a05fde7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -171,6 +171,10 @@ struct intel_connector {
>  
>  	/* Cached EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */
>  	struct edid *edid;
> +
> +	/* since POLL and HPD connectors may use the same HPD line keep the native
> +	   state of connector->polled in case hotplug storm detection changes it */
> +	u8 polled;
>  };
>  
>  struct intel_crtc_config {
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index ee4a8da..8912201 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -998,7 +998,6 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  			   DRM_MODE_CONNECTOR_HDMIA);
>  	drm_connector_helper_add(connector, &intel_hdmi_connector_helper_funcs);
>  
> -	connector->polled = DRM_CONNECTOR_POLL_HPD;
>  	connector->interlace_allowed = 1;
>  	connector->doublescan_allowed = 0;
>  
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 298dc85..64b8b40 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -2276,7 +2276,6 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
>  	connector = &intel_connector->base;
>  	if (intel_sdvo_get_hotplug_support(intel_sdvo) &
>  		intel_sdvo_connector->output_flag) {
> -		connector->polled = DRM_CONNECTOR_POLL_HPD;
>  		intel_sdvo->hotplug_active |= intel_sdvo_connector->output_flag;
>  		/* Some SDVO devices have one-shot hotplug interrupts.
>  		 * Ensure that they get re-enabled when an interrupt happens.
> @@ -2284,7 +2283,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
>  		intel_encoder->hot_plug = intel_sdvo_enable_hotplug;
>  		intel_sdvo_enable_hotplug(intel_encoder);
>  	} else {
> -		connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
> +		intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
>  	}
>  	encoder->encoder_type = DRM_MODE_ENCODER_TMDS;
>  	connector->connector_type = DRM_MODE_CONNECTOR_DVID;
> @@ -2353,7 +2352,7 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
>  
>  	intel_connector = &intel_sdvo_connector->base;
>  	connector = &intel_connector->base;
> -	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
> +	intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
>  	encoder->encoder_type = DRM_MODE_ENCODER_DAC;
>  	connector->connector_type = DRM_MODE_CONNECTOR_VGA;
>  
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 6673726..b945bc5 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1613,7 +1613,7 @@ intel_tv_init(struct drm_device *dev)
>  	 *
>  	 * More recent chipsets favour HDMI rather than integrated S-Video.
>  	 */
> -	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
> +	intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
>  
>  	drm_connector_init(dev, connector, &intel_tv_connector_funcs,
>  			   DRM_MODE_CONNECTOR_SVIDEO);
> -- 
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 3/7] drm/i915: Mask out the HPD irq bits before setting them individually.

[Jani Nikula]

On Tue, 09 Apr 2013, Egbert Eich <eich@freedesktop.org> wrote:
> From: Egbert Eich <eich@suse.de>
>
> To disable previously enabled HPD IRQs we need to reset them and
> set the enabled ones individually.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 5408a3a..a3f1ac4 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2117,9 +2117,11 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
>  	u32 hotplug;
>  
>  	if (HAS_PCH_IBX(dev)) {
> +		mask &= ~SDE_HOTPLUG_MASK;
>  		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
>  			mask |= hpd_ibx[intel_encoder->hpd_pin];
>  	} else {
> +		mask &= ~SDE_HOTPLUG_MASK_CPT;
>  		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
>  			mask |= hpd_cpt[intel_encoder->hpd_pin];
>  	}
> -- 
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 4/7] drm/i915: Disable HPD interrupt on pin when irq storm is detected (v2)

[Jani Nikula]

On Tue, 09 Apr 2013, Egbert Eich <eich@freedesktop.org> wrote:
> From: Egbert Eich <eich@suse.de>
>
> This patch disables hotplug interrupts if an 'interrupt storm'
> has been detected.
> Noise on the interrupt line renders the hotplug interrupt useless:
> each hotplug event causes the devices to be rescanned which will
> will only increase the system load.
> Thus disable the hotplug interrupts and fall back to periodic
> device polling.
>
> v2: Fixed cleanup typo.
>
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 71 +++++++++++++++++++++++++++++++----------
>  1 file changed, 54 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a3f1ac4..d0e6f6d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -88,7 +88,8 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>  	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
>  };
>  
> -
> +static void ibx_hpd_irq_setup(struct drm_device *dev);
> +static void i915_hpd_irq_setup(struct drm_device *dev);
>  
>  /* For display hotplug interrupt */
>  static void
> @@ -342,7 +343,11 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  						    hotplug_work);
>  	struct drm_device *dev = dev_priv->dev;
>  	struct drm_mode_config *mode_config = &dev->mode_config;
> -	struct intel_encoder *encoder;
> +	struct intel_connector *intel_connector;
> +	struct intel_encoder *intel_encoder;
> +	struct drm_connector *connector;
> +	unsigned long irqflags;
> +	bool connector_disabled = false;

Isn't it really hpd_disabled, not connector?

>  
>  	/* HPD irq before everything is fully set up. */
>  	if (!dev_priv->enable_hotplug_processing)
> @@ -351,9 +356,29 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  	mutex_lock(&mode_config->mutex);
>  	DRM_DEBUG_KMS("running encoder hotplug functions\n");
>  
> -	list_for_each_entry(encoder, &mode_config->encoder_list, base.head)
> -		if (encoder->hot_plug)
> -			encoder->hot_plug(encoder);
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +	list_for_each_entry(connector, &mode_config->connector_list, head) {
> +		intel_connector = to_intel_connector(connector);
> +		intel_encoder = intel_connector->encoder;
> +		if (intel_encoder->hpd_pin > HPD_NONE &&
> +		    dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_MARK_DISABLED &&
> +		    connector->polled == DRM_CONNECTOR_POLL_HPD) {
> +			pr_warn("HPD interrupt storm detected on connector %s disabling\n",
> +				drm_get_connector_name(connector));

Please use some DRM message, like DRM_INFO. And perhaps make the message
something like, "Switching from hotplug detection to polling on
connector %s due to interrupt storm\n".

> +			dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark = HPD_DISABLED;
> +			connector->polled = DRM_CONNECTOR_POLL_CONNECT
> +				| DRM_CONNECTOR_POLL_DISCONNECT;
> +			connector_disabled = true;
> +		}
> +	}
> +	if (connector_disabled)
> +		drm_kms_helper_poll_enable(dev); /* if there were no outputs to poll, poll is disabled */

Please move the comment to a line of it's own. (It's okay to add braces
for clarity, since you'll add them later anyway.)

> +
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +
> +	list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> +		if (intel_encoder->hot_plug)
> +			intel_encoder->hot_plug(intel_encoder);
>  
>  	mutex_unlock(&mode_config->mutex);
>  
> @@ -584,13 +609,14 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
>  
>  #define HPD_STORM_DETECT_PERIOD 1000
>  
> -static inline void hotplug_irq_storm_detect(struct drm_device *dev,
> +static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
>  					    u32 hotplug_trigger,
>  					    const u32 *hpd)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	unsigned long irqflags;
>  	int i;
> +	bool ret = false;
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  
> @@ -605,12 +631,15 @@ static inline void hotplug_irq_storm_detect(struct drm_device *dev,
>  			} else if (dev_priv->hpd_stats[i].hpd_cnt > 5) {
>  				dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
>  				DRM_DEBUG_KMS("HPD interrupt storm detected on  PIN %d\n", i);
> +				ret = true;
>  			} else
>  				dev_priv->hpd_stats[i].hpd_cnt++;
>  		}
>  	}
>  
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +
> +	return ret;

As mentioned earlier, these hunks could be moved to the patch
introducing hotplug_irq_storm_detect() in the first place.

>  }
>  
>  static void gmbus_irq_handler(struct drm_device *dev)
> @@ -686,7 +715,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
>  					 hotplug_status);
>  			if (hotplug_trigger) {
> -				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
> +				if( hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915))

Misplaced space after (.


BR,
Jani.

> +					i915_hpd_irq_setup(dev);
>  				queue_work(dev_priv->wq,
>  					   &dev_priv->hotplug_work);
>  			}
> @@ -716,7 +746,8 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
>  	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK;
>  
>  	if (hotplug_trigger) {
> -		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_ibx);
> +		if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_ibx))
> +			ibx_hpd_irq_setup(dev);
>  		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
>  	}
>  	if (pch_iir & SDE_AUDIO_POWER_MASK)
> @@ -764,7 +795,8 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>  	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
>  
>  	if (hotplug_trigger) {
> -		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_cpt);
> +		if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_cpt))
> +			ibx_hpd_irq_setup(dev);
>  		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
>  	}
>  	if (pch_iir & SDE_AUDIO_POWER_MASK_CPT)
> @@ -2119,11 +2151,13 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
>  	if (HAS_PCH_IBX(dev)) {
>  		mask &= ~SDE_HOTPLUG_MASK;
>  		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> -			mask |= hpd_ibx[intel_encoder->hpd_pin];
> +			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
> +				mask |= hpd_ibx[intel_encoder->hpd_pin];
>  	} else {
>  		mask &= ~SDE_HOTPLUG_MASK_CPT;
>  		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> -			mask |= hpd_cpt[intel_encoder->hpd_pin];
> +			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
> +				mask |= hpd_cpt[intel_encoder->hpd_pin];
>  	}
>  
>  	I915_WRITE(SDEIMR, ~mask);
> @@ -2651,7 +2685,8 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
>  			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
>  				  hotplug_status);
>  			if (hotplug_trigger) {
> -				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
> +				if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915))
> +					i915_hpd_irq_setup(dev);
>  				queue_work(dev_priv->wq,
>  					   &dev_priv->hotplug_work);
>  			}
> @@ -2801,7 +2836,7 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	struct drm_mode_config *mode_config = &dev->mode_config;
> -	struct intel_encoder *encoder;
> +	struct intel_encoder *intel_encoder;
>  	u32 hotplug_en;
>  
>  	if (I915_HAS_HOTPLUG(dev)) {
> @@ -2809,8 +2844,9 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
>  		hotplug_en &= ~HOTPLUG_INT_EN_MASK;
>  		/* Note HDMI and DP share hotplug bits */
>  		/* enable bits are the same for all generations */
> -		list_for_each_entry(encoder, &mode_config->encoder_list, base.head)
> -			hotplug_en |= hpd_mask_i915[encoder->hpd_pin];
> +		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> +			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
> +				hotplug_en |= hpd_mask_i915[intel_encoder->hpd_pin];
>  		/* Programming the CRT detection parameters tends
>  		   to generate a spurious hotplug event about three
>  		   seconds later.  So just do it once.
> @@ -2888,8 +2924,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
>  			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
>  				  hotplug_status);
>  			if (hotplug_trigger) {
> -				hotplug_irq_storm_detect(dev, hotplug_trigger,
> -							 IS_G4X(dev) ? hpd_status_gen4 : hpd_status_i965);
> +				if (hotplug_irq_storm_detect(dev, hotplug_trigger,
> +							    IS_G4X(dev) ? hpd_status_gen4 : hpd_status_i965))
> +					i915_hpd_irq_setup(dev);
>  				queue_work(dev_priv->wq,
>  					   &dev_priv->hotplug_work);
>  			}
> -- 
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 5/7] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v3)

[Jani Nikula]

On Tue, 09 Apr 2013, Egbert Eich <eich@freedesktop.org> wrote:
> From: Egbert Eich <eich@suse.de>
>
> We disable hoptplug detection when we encounter a hotplug event
> storm. Still hotplug detection is required on some outputs (like
> Display Port). The interrupt storm may be only temporary (on certain
> Dell Laptops for instance it happens at certain charging states of
> the system). Thus we enable it after a certain grace period (2 minutes).
> Should the interrupt storm persist it will be detected immediately
> and it will be disabled again.
>
> v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state tracker.
> v3: Clarified loop start value,
>     Removed superfluous test for Ivybridge and Haswell,
>     Restructured loop to avoid deep nesting (all suggested by Ville Syrjälä)
>
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  2 ++
>  drivers/gpu/drm/i915/i915_irq.c | 49 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 83fc1a6..a3ed2e3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -938,6 +938,8 @@ typedef struct drm_i915_private {
>  			HPD_MARK_DISABLED = 2
>  		} hpd_mark;
>  	} hpd_stats[HPD_NUM_PINS];
> +#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)

It's already too crowded here, please move this #define to i915_irq.c.

> +	struct timer_list hotplug_reenable_timer;
>  
>  	int num_pch_pll;
>  	int num_plane;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d0e6f6d..1a00533 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -371,8 +371,11 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  			connector_disabled = true;
>  		}
>  	}
> -	if (connector_disabled)
> +	if (connector_disabled) {
>  		drm_kms_helper_poll_enable(dev); /* if there were no outputs to poll, poll is disabled */
> +		mod_timer(&dev_priv->hotplug_reenable_timer,
> +			  jiffies + msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY));
> +	}
>  
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
> @@ -2348,6 +2351,8 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
>  	if (!dev_priv)
>  		return;
>  
> +	del_timer_sync(&dev_priv->hotplug_reenable_timer);
> +
>  	for_each_pipe(pipe)
>  		I915_WRITE(PIPESTAT(pipe), 0xffff);
>  
> @@ -2369,6 +2374,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>  	if (!dev_priv)
>  		return;
>  
> +	del_timer_sync(&dev_priv->hotplug_reenable_timer);
> +
>  	I915_WRITE(HWSTAM, 0xffffffff);
>  
>  	I915_WRITE(DEIMR, 0xffffffff);
> @@ -2745,6 +2752,8 @@ static void i915_irq_uninstall(struct drm_device * dev)
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	int pipe;
>  
> +	del_timer_sync(&dev_priv->hotplug_reenable_timer);
> +
>  	if (I915_HAS_HOTPLUG(dev)) {
>  		I915_WRITE(PORT_HOTPLUG_EN, 0);
>  		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
> @@ -2989,6 +2998,8 @@ static void i965_irq_uninstall(struct drm_device * dev)
>  	if (!dev_priv)
>  		return;
>  
> +	del_timer_sync(&dev_priv->hotplug_reenable_timer);
> +
>  	I915_WRITE(PORT_HOTPLUG_EN, 0);
>  	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
>  
> @@ -3004,6 +3015,40 @@ static void i965_irq_uninstall(struct drm_device * dev)
>  	I915_WRITE(IIR, I915_READ(IIR));
>  }
>  
> +static void i915_reenable_hotplug_timer_func(unsigned long data)
> +{
> +	drm_i915_private_t *dev_priv = (drm_i915_private_t *)data;
> +	struct drm_device *dev = dev_priv->dev;
> +	struct drm_mode_config *mode_config = &dev->mode_config;
> +	unsigned long irqflags;
> +	int i;
> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +	for (i = (HPD_NONE + 1); i < HPD_NUM_PINS; i++) {
> +		struct drm_connector *connector;
> +
> +		if (dev_priv->hpd_stats[i].hpd_mark != HPD_MARK_DISABLED)

I think this is wrong, skipping HPD_DISABLED.

> +			continue;
> +
> +		dev_priv->hpd_stats[i].hpd_mark = HPD_ENABLED;
> +
> +		list_for_each_entry(connector, &mode_config->connector_list, head) {
> +			struct intel_connector *intel_connector = to_intel_connector(connector);
> +
> +			if (intel_connector->encoder->hpd_pin == i) {
> +				if (connector->polled != intel_connector->polled)
> +					DRM_DEBUG_DRIVER("Reenabling HPD on connector %s\n",
> +							 drm_get_connector_name(connector));
> +				connector->polled = intel_connector->polled;
> +				if (!connector->polled)
> +					connector->polled = DRM_CONNECTOR_POLL_HPD;
> +			}
> +		}
> +		dev_priv->display.hpd_irq_setup(dev);

You don't need to call this at each iteration, do you?

> +	}

In fact, couldn't you just call intel_hpd_init(), or modify it to do
what you want? Keep it simple.

> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +}
> +
>  void intel_irq_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3016,6 +3061,8 @@ void intel_irq_init(struct drm_device *dev)
>  	setup_timer(&dev_priv->gpu_error.hangcheck_timer,
>  		    i915_hangcheck_elapsed,
>  		    (unsigned long) dev);
> +	setup_timer(&dev_priv->hotplug_reenable_timer, i915_reenable_hotplug_timer_func,
> +		    (unsigned long) dev_priv);
>  
>  	pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
>  
> -- 
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/7] drm/i915: Add HPD IRQ storm detection (v4)

[Daniel Vetter]

On Thu, Apr 11, 2013 at 11:32 AM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Tue, 09 Apr 2013, Egbert Eich <eich@freedesktop.org> wrote:
>> From: Egbert Eich <eich@suse.de>
>>
>> Add a hotplug IRQ storm detection (triggered when a hotplug interrupt
>> fires more than 5 times / sec).
>
> Okay, this is theoretical, but a display port sink could do more than
> that many hpd irq requests when connected. Which leads me to wonder in
> general if the storm detection should be different for hot plug
> vs. unplug and hpd irq events.

Yeah, I've considered what to do with short pulse hotplug events. But
otoh we don't differentiate that yet in our code and worst case we
need to increase the limits a bit maybe. I guess we just need to see
what happens in reality and then act accordingly.

>> Rationale:
>> Despite of the many attempts to fix the problem with noisy hotplug
>> interrupt lines we are still seeing systems which have issues:
>> Once cause of noise seems to be bad routing of the hotplug line
>> on the board: cross talk from other signals seems to cause erronous
>> hotplug interrupts. This has been documented as an erratum for the
>> the i945GM chipset and thus hotplug support was disabled for this
>> chipset model but others seem to have this problem, too.
>>
>> We have seen this issue on a G35 motherboard for example:
>> Even different motherboards of the same model seem to behave
>> differently: while some only see only around 10-100 interrupts/s
>> others seem to see 5k or more.
>> We've also observed a dependency on the selected video mode.
>>
>> Also on certain laptops interrupt noise seems to occur duing
>> battery charging when the battery is at a certain charge levels.
>
> Have you observed difference between hot plug/unplug?
>
> Has this been a problem on PCH split platforms, i.e. since ilk/gen5?

I've discussed this with Art (iirc) and apparently the BIOS/Windows
guys have some elaborate schemes for this on all platforms. So I guess
this could happen everywhere. Note that hpd storms could also be
caused in the sink, so imo we want this everywhere.

>> Thus we add a simple algorithm here that detects an 'interrupt storm'
>> condition.
>>
>> v2: Fixed comment.
>> v3: Reordered drm_i915_private: moved hpd state tracking to hotplug work stuff.
>> v4: Followed by Jesse Barnes to use a time_..() macro.
>>
>> Signed-off-by: Egbert Eich <eich@suse.de>
>> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h |  9 ++++++
>>  drivers/gpu/drm/i915/i915_irq.c | 66 +++++++++++++++++++++++++++++++++--------
>>  2 files changed, 63 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 44fca0b..83fc1a6 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -929,6 +929,15 @@ typedef struct drm_i915_private {
>>
>>       struct work_struct hotplug_work;
>>       bool enable_hotplug_processing;
>> +     struct {
>> +             unsigned long hpd_last_jiffies;
>> +             int hpd_cnt;
>> +             enum {
>> +                     HPD_ENABLED = 0,
>> +                     HPD_DISABLED = 1,
>> +                     HPD_MARK_DISABLED = 2
>> +             } hpd_mark;
>> +     } hpd_stats[HPD_NUM_PINS];
>
> With all the hotplug stuff being added, I think it's getting time to
> group all the hotplug stuff under a struct.

Yeah, agreed. Though to avoid too much rebase hell I think it's better
to do that as a follow-up.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v3 5/7] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v3)

[Egbert Eich]

On Thu, Apr 11, 2013 at 01:44:51PM +0300, Jani Nikula wrote:
> > +
> > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > +	for (i = (HPD_NONE + 1); i < HPD_NUM_PINS; i++) {
> > +		struct drm_connector *connector;
> > +
> > +		if (dev_priv->hpd_stats[i].hpd_mark != HPD_MARK_DISABLED)
> 
> I think this is wrong, skipping HPD_DISABLED.

Right, this was indeed wrong.

> 
> > +			continue;
> > +
> > +		dev_priv->hpd_stats[i].hpd_mark = HPD_ENABLED;
> > +
> > +		list_for_each_entry(connector, &mode_config->connector_list, head) {
> > +			struct intel_connector *intel_connector = to_intel_connector(connector);
> > +
> > +			if (intel_connector->encoder->hpd_pin == i) {
> > +				if (connector->polled != intel_connector->polled)
> > +					DRM_DEBUG_DRIVER("Reenabling HPD on connector %s\n",
> > +							 drm_get_connector_name(connector));
> > +				connector->polled = intel_connector->polled;
> > +				if (!connector->polled)
> > +					connector->polled = DRM_CONNECTOR_POLL_HPD;
> > +			}
> > +		}
> > +		dev_priv->display.hpd_irq_setup(dev);
> 
> You don't need to call this at each iteration, do you?

Right, you are right here as well.
> 
> > +	}
> 
> In fact, couldn't you just call intel_hpd_init(), or modify it to do
> what you want? Keep it simple.

Well, intel_hpd_init() is initializing every to dev_priv->hpd_stats[i].hpd_mark
to HPD_ENABLED. Can we rule out that the timer runs between the interrupt
handler and the worker - as in this state this variable might me set to
HPD_MARK_DISABLED?
In fact it would probably not even hurt if we changed the HPD_MARK_DISABLED
to HPD_ENABLED as in a storm condition this condition will soon reappear but
I'd rather avoid it.
Of course we could pass an argument to the function to distinguish both
conditions. This is a simplification which can still be introduced - when
I'm in fact able to do some testing.

Thanks a lot for the review!

Cheers,
	Egbert.

Re: [PATCH v3 6/7] drm/i915: Add bit field to record which pins have received HPD events (v2)

[Jani Nikula]

On Tue, 09 Apr 2013, Egbert Eich <eich@freedesktop.org> wrote:
> From: Egbert Eich <eich@suse.de>
>
> This way it is possible to limit 're'-detect() of displays to connectors
> which have received an HPD event.

I'd like you to be more explicit that this patch doesn't in fact add
such stuff in itself.

>
> v2: Reordered drm_i915_private: Move hpd_event_bits to hpd state tracking.
>
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a3ed2e3..907e290 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -938,6 +938,7 @@ typedef struct drm_i915_private {
>  			HPD_MARK_DISABLED = 2
>  		} hpd_mark;
>  	} hpd_stats[HPD_NUM_PINS];
> +	u32 hpd_event_bits;
>  #define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
>  	struct timer_list hotplug_reenable_timer;
>  
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 1a00533..92041b9 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -348,6 +348,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  	struct drm_connector *connector;
>  	unsigned long irqflags;
>  	bool connector_disabled = false;
> +	u32 hpd_event_bits;
>  
>  	/* HPD irq before everything is fully set up. */
>  	if (!dev_priv->enable_hotplug_processing)
> @@ -357,6 +358,9 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  	DRM_DEBUG_KMS("running encoder hotplug functions\n");
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +
> +	hpd_event_bits = dev_priv->hpd_event_bits;
> +	dev_priv->hpd_event_bits = 0;
>  	list_for_each_entry(connector, &mode_config->connector_list, head) {
>  		intel_connector = to_intel_connector(connector);
>  		intel_encoder = intel_connector->encoder;
> @@ -370,6 +374,10 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  				| DRM_CONNECTOR_POLL_DISCONNECT;
>  			connector_disabled = true;
>  		}
> +		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
> +			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
> +				      drm_get_connector_name(connector), intel_encoder->hpd_pin);
> +		}

I fear this may end up being a bit excessive debug printing.

BR,
Jani.

>  	}
>  	if (connector_disabled) {
>  		drm_kms_helper_poll_enable(dev); /* if there were no outputs to poll, poll is disabled */
> @@ -626,6 +634,7 @@ static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
>  	for (i = 1; i < HPD_NUM_PINS; i++) {
>  		if ((hpd[i] & hotplug_trigger) &&
>  		    dev_priv->hpd_stats[i].hpd_mark == HPD_ENABLED) {
> +			dev_priv->hpd_event_bits |= (1 << i);
>  			if (!time_in_range(jiffies, dev_priv->hpd_stats[i].hpd_last_jiffies,
>  					  dev_priv->hpd_stats[i].hpd_last_jiffies
>  					  + msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
> @@ -633,6 +642,7 @@ static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
>  				dev_priv->hpd_stats[i].hpd_cnt = 0;
>  			} else if (dev_priv->hpd_stats[i].hpd_cnt > 5) {
>  				dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
> +				dev_priv->hpd_event_bits &= ~(1 << i);
>  				DRM_DEBUG_KMS("HPD interrupt storm detected on  PIN %d\n", i);
>  				ret = true;
>  			} else
> -- 
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3] drm/i915: Disable HPD interrupt on pin when irq storm is detected (v3)

[Egbert Eich]

This patch disables hotplug interrupts if an 'interrupt storm'
has been detected.
Noise on the interrupt line renders the hotplug interrupt useless:
each hotplug event causes the devices to be rescanned which will
will only increase the system load.
Thus disable the hotplug interrupts and fall back to periodic
device polling.

v2: Fixed cleanup typo.
v3: Fixed format issues, clarified a variable name,
    changed pr_warn() to DRM_INFO() as suggested by
    Jani Nikula <jani.nikula@linux.intel.com>.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/i915_irq.c | 75 +++++++++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a3f1ac4..834c717 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -88,7 +88,8 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
-
+static void ibx_hpd_irq_setup(struct drm_device *dev);
+static void i915_hpd_irq_setup(struct drm_device *dev);
 
 /* For display hotplug interrupt */
 static void
@@ -342,7 +343,11 @@ static void i915_hotplug_work_func(struct work_struct *work)
 						    hotplug_work);
 	struct drm_device *dev = dev_priv->dev;
 	struct drm_mode_config *mode_config = &dev->mode_config;
-	struct intel_encoder *encoder;
+	struct intel_connector *intel_connector;
+	struct intel_encoder *intel_encoder;
+	struct drm_connector *connector;
+	unsigned long irqflags;
+	bool hpd_disabled = false;
 
 	/* HPD irq before everything is fully set up. */
 	if (!dev_priv->enable_hotplug_processing)
@@ -351,9 +356,33 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	mutex_lock(&mode_config->mutex);
 	DRM_DEBUG_KMS("running encoder hotplug functions\n");
 
-	list_for_each_entry(encoder, &mode_config->encoder_list, base.head)
-		if (encoder->hot_plug)
-			encoder->hot_plug(encoder);
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+	list_for_each_entry(connector, &mode_config->connector_list, head) {
+		intel_connector = to_intel_connector(connector);
+		intel_encoder = intel_connector->encoder;
+		if (intel_encoder->hpd_pin > HPD_NONE &&
+		    dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_MARK_DISABLED &&
+		    connector->polled == DRM_CONNECTOR_POLL_HPD) {
+			DRM_INFO("HPD interrupt storm detected on connector %s: "
+				 "switching from hotplug detection to polling\n",
+				drm_get_connector_name(connector));
+			dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark = HPD_DISABLED;
+			connector->polled = DRM_CONNECTOR_POLL_CONNECT
+				| DRM_CONNECTOR_POLL_DISCONNECT;
+			hpd_disabled = true;
+		}
+	}
+	 /* if there were no outputs to poll, poll was disabled,
+	  * therefore make sure it's enabled when disabling HPD on
+	  * some connectors */
+	if (hpd_disabled) {
+		drm_kms_helper_poll_enable(dev);
+	}
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+
+	list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
+		if (intel_encoder->hot_plug)
+			intel_encoder->hot_plug(intel_encoder);
 
 	mutex_unlock(&mode_config->mutex);
 
@@ -584,13 +613,14 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
 
 #define HPD_STORM_DETECT_PERIOD 1000
 
-static inline void hotplug_irq_storm_detect(struct drm_device *dev,
+static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
 					    u32 hotplug_trigger,
 					    const u32 *hpd)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	unsigned long irqflags;
 	int i;
+	bool ret = false;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 
@@ -605,12 +635,15 @@ static inline void hotplug_irq_storm_detect(struct drm_device *dev,
 			} else if (dev_priv->hpd_stats[i].hpd_cnt > 5) {
 				dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
 				DRM_DEBUG_KMS("HPD interrupt storm detected on  PIN %d\n", i);
+				ret = true;
 			} else
 				dev_priv->hpd_stats[i].hpd_cnt++;
 		}
 	}
 
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+
+	return ret;
 }
 
 static void gmbus_irq_handler(struct drm_device *dev)
@@ -686,7 +719,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
 					 hotplug_status);
 			if (hotplug_trigger) {
-				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
+				if(hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915))
+					i915_hpd_irq_setup(dev);
 				queue_work(dev_priv->wq,
 					   &dev_priv->hotplug_work);
 			}
@@ -716,7 +750,8 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
 	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK;
 
 	if (hotplug_trigger) {
-		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_ibx);
+		if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_ibx))
+			ibx_hpd_irq_setup(dev);
 		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
 	}
 	if (pch_iir & SDE_AUDIO_POWER_MASK)
@@ -764,7 +799,8 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
 	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
 
 	if (hotplug_trigger) {
-		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_cpt);
+		if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_cpt))
+			ibx_hpd_irq_setup(dev);
 		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
 	}
 	if (pch_iir & SDE_AUDIO_POWER_MASK_CPT)
@@ -2119,11 +2155,13 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
 	if (HAS_PCH_IBX(dev)) {
 		mask &= ~SDE_HOTPLUG_MASK;
 		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
-			mask |= hpd_ibx[intel_encoder->hpd_pin];
+			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
+				mask |= hpd_ibx[intel_encoder->hpd_pin];
 	} else {
 		mask &= ~SDE_HOTPLUG_MASK_CPT;
 		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
-			mask |= hpd_cpt[intel_encoder->hpd_pin];
+			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
+				mask |= hpd_cpt[intel_encoder->hpd_pin];
 	}
 
 	I915_WRITE(SDEIMR, ~mask);
@@ -2651,7 +2689,8 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
 			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
 				  hotplug_status);
 			if (hotplug_trigger) {
-				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
+				if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915))
+					i915_hpd_irq_setup(dev);
 				queue_work(dev_priv->wq,
 					   &dev_priv->hotplug_work);
 			}
@@ -2801,7 +2840,7 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	struct drm_mode_config *mode_config = &dev->mode_config;
-	struct intel_encoder *encoder;
+	struct intel_encoder *intel_encoder;
 	u32 hotplug_en;
 
 	if (I915_HAS_HOTPLUG(dev)) {
@@ -2809,8 +2848,9 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
 		hotplug_en &= ~HOTPLUG_INT_EN_MASK;
 		/* Note HDMI and DP share hotplug bits */
 		/* enable bits are the same for all generations */
-		list_for_each_entry(encoder, &mode_config->encoder_list, base.head)
-			hotplug_en |= hpd_mask_i915[encoder->hpd_pin];
+		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
+			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
+				hotplug_en |= hpd_mask_i915[intel_encoder->hpd_pin];
 		/* Programming the CRT detection parameters tends
 		   to generate a spurious hotplug event about three
 		   seconds later.  So just do it once.
@@ -2888,8 +2928,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
 			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
 				  hotplug_status);
 			if (hotplug_trigger) {
-				hotplug_irq_storm_detect(dev, hotplug_trigger,
-							 IS_G4X(dev) ? hpd_status_gen4 : hpd_status_i965);
+				if (hotplug_irq_storm_detect(dev, hotplug_trigger,
+							    IS_G4X(dev) ? hpd_status_gen4 : hpd_status_i965))
+					i915_hpd_irq_setup(dev);
 				queue_work(dev_priv->wq,
 					   &dev_priv->hotplug_work);
 			}
-- 
1.8.1.4

[PATCH v4] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v4)

[Egbert Eich]

We disable hoptplug detection when we encounter a hotplug event
storm. Still hotplug detection is required on some outputs (like
Display Port). The interrupt storm may be only temporary (on certain
Dell Laptops for instance it happens at certain charging states of
the system). Thus we enable it after a certain grace period (2 minutes).
Should the interrupt storm persist it will be detected immediately
and it will be disabled again.

v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state tracker.
v3: Clarified loop start value,
    Removed superfluous test for Ivybridge and Haswell,
    Restructured loop to avoid deep nesting (all suggested by Ville Syrjälä)
v4: Fixed two bugs pointed out by Jani Nikula.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_irq.c | 50 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 83fc1a6..195b9fe 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -938,6 +938,7 @@ typedef struct drm_i915_private {
 			HPD_MARK_DISABLED = 2
 		} hpd_mark;
 	} hpd_stats[HPD_NUM_PINS];
+	struct timer_list hotplug_reenable_timer;
 
 	int num_pch_pll;
 	int num_plane;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 834c717..f60c643 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -337,6 +337,8 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
+#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
+
 static void i915_hotplug_work_func(struct work_struct *work)
 {
 	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
@@ -377,7 +379,10 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	  * some connectors */
 	if (hpd_disabled) {
 		drm_kms_helper_poll_enable(dev);
+		mod_timer(&dev_priv->hotplug_reenable_timer,
+			  jiffies + msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY));
 	}
+
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
@@ -2352,6 +2357,8 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 	if (!dev_priv)
 		return;
 
+	del_timer_sync(&dev_priv->hotplug_reenable_timer);
+
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
 
@@ -2373,6 +2380,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 	if (!dev_priv)
 		return;
 
+	del_timer_sync(&dev_priv->hotplug_reenable_timer);
+
 	I915_WRITE(HWSTAM, 0xffffffff);
 
 	I915_WRITE(DEIMR, 0xffffffff);
@@ -2749,6 +2758,8 @@ static void i915_irq_uninstall(struct drm_device * dev)
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	int pipe;
 
+	del_timer_sync(&dev_priv->hotplug_reenable_timer);
+
 	if (I915_HAS_HOTPLUG(dev)) {
 		I915_WRITE(PORT_HOTPLUG_EN, 0);
 		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
@@ -2993,6 +3004,8 @@ static void i965_irq_uninstall(struct drm_device * dev)
 	if (!dev_priv)
 		return;
 
+	del_timer_sync(&dev_priv->hotplug_reenable_timer);
+
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
@@ -3008,6 +3021,41 @@ static void i965_irq_uninstall(struct drm_device * dev)
 	I915_WRITE(IIR, I915_READ(IIR));
 }
 
+static void i915_reenable_hotplug_timer_func(unsigned long data)
+{
+	drm_i915_private_t *dev_priv = (drm_i915_private_t *)data;
+	struct drm_device *dev = dev_priv->dev;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	unsigned long irqflags;
+	int i;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+	for (i = (HPD_NONE + 1); i < HPD_NUM_PINS; i++) {
+		struct drm_connector *connector;
+
+		if (dev_priv->hpd_stats[i].hpd_mark != HPD_DISABLED)
+			continue;
+
+		dev_priv->hpd_stats[i].hpd_mark = HPD_ENABLED;
+
+		list_for_each_entry(connector, &mode_config->connector_list, head) {
+			struct intel_connector *intel_connector = to_intel_connector(connector);
+
+			if (intel_connector->encoder->hpd_pin == i) {
+				if (connector->polled != intel_connector->polled)
+					DRM_DEBUG_DRIVER("Reenabling HPD on connector %s\n",
+							 drm_get_connector_name(connector));
+				connector->polled = intel_connector->polled;
+				if (!connector->polled)
+					connector->polled = DRM_CONNECTOR_POLL_HPD;
+			}
+		}
+	}
+	if (dev_priv->display.hpd_irq_setup)
+		dev_priv->display.hpd_irq_setup(dev);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
+
 void intel_irq_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3020,6 +3068,8 @@ void intel_irq_init(struct drm_device *dev)
 	setup_timer(&dev_priv->gpu_error.hangcheck_timer,
 		    i915_hangcheck_elapsed,
 		    (unsigned long) dev);
+	setup_timer(&dev_priv->hotplug_reenable_timer, i915_reenable_hotplug_timer_func,
+		    (unsigned long) dev_priv);
 
 	pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
 
-- 
1.8.1.4

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

Re: [PATCH v3 6/7] drm/i915: Add bit field to record which pins have received HPD events (v2)

[Egbert Eich]

On Thu, Apr 11, 2013 at 04:21:54PM +0300, Jani Nikula wrote:
> >  		}
> > +		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
> > +			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
> > +				      drm_get_connector_name(connector), intel_encoder->hpd_pin);
> > +		}
> 
> I fear this may end up being a bit excessive debug printing.
> 

In a storm condition this is definitely true.
I still would like to keep this code in for debugging as I have 
some more ideas I would like to implement and test - however I
will wrap this code with #if 0 .. #endif so it can be enabled
easily.
Eventually I will remove it entirely.
Would this be ok with you?

Cheers,
	Egbert.

Re: [PATCH v3 7/7] drm/i915: Only reprobe display on encoder which has received an HPD event

[Jani Nikula]

On Tue, 09 Apr 2013, Egbert Eich <eich@freedesktop.org> wrote:
> From: Egbert Eich <eich@suse.de>
>
> Instead of calling into the DRM helper layer to poll all connectors for
> changes in connected displays probe only those connectors which have
> received a hotplug event.

Thanks, we've all been waiting for someone(tm) to do this.

Apart from the bikesheds below,
Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 37 +++++++++++++++++++++++++++++++------
>  1 file changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 92041b9..7788536 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -334,6 +334,24 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
>  						     crtc);
>  }
>  
> +/**
> + * drm_helper_hpd_irq_single_connector_event() - call this function with mode_config.mutex lock held
> + */

That kernel-doc line should include a one line description what the
function does, not what the preconditions are. You can achieve the same
with:

	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));

I'd also like to bikeshed the function name, because we'll be seeing it
a lot in the dmesgs through DRM_DEBUG_KMS. Just intel_hpd_irq_event()?

> +
> +static int intel_hpd_irq_single_connector_event(struct drm_device *dev, struct drm_connector *connector)
> +{
> +	enum drm_connector_status old_status;
> +
> +	old_status = connector->status;
> +
> +	connector->status = connector->funcs->detect(connector, false);
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n",
> +		      connector->base.id,
> +		      drm_get_connector_name(connector),
> +		      old_status, connector->status);

And while at it, another bikeshed about having different messages for
when old_status == connector->status vs. not. I've always found the
"status updated from 1 to 1" messages a bit dumb... ;)

> +	return (old_status != connector->status);
> +}
> +
>  /*
>   * Handle hotplug events outside the interrupt handler proper.
>   */
> @@ -348,6 +366,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  	struct drm_connector *connector;
>  	unsigned long irqflags;
>  	bool connector_disabled = false;
> +	bool changed = false;
>  	u32 hpd_event_bits;
>  
>  	/* HPD irq before everything is fully set up. */
> @@ -387,14 +406,20 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
> -	list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> -		if (intel_encoder->hot_plug)
> -			intel_encoder->hot_plug(intel_encoder);
> -
> +	list_for_each_entry(connector, &mode_config->connector_list, head) {
> +		intel_connector = to_intel_connector(connector);
> +		intel_encoder = intel_connector->encoder;
> +		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
> +			if (intel_encoder->hot_plug)
> +				intel_encoder->hot_plug(intel_encoder);
> +			if (intel_hpd_irq_single_connector_event(dev, connector))
> +				changed = true;
> +		}
> +	}
>  	mutex_unlock(&mode_config->mutex);
>  
> -	/* Just fire off a uevent and let userspace tell us what to do */
> -	drm_helper_hpd_irq_event(dev);
> +	if (changed)
> +		drm_kms_helper_hotplug_event(dev);
>  }
>  
>  static void ironlake_handle_rps_change(struct drm_device *dev)
> -- 
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3] drm/i915: Add bit field to record which pins have received HPD events (v3)

[Egbert Eich]

This way it is possible to limit 're'-detect() of displays to connectors
which have received an HPD event.

v2: Reordered drm_i915_private: Move hpd_event_bits to hpd state tracking.
v3: Fixed merge conflicts with previous patches.

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 195b9fe..c0d9fb9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -938,6 +938,7 @@ typedef struct drm_i915_private {
 			HPD_MARK_DISABLED = 2
 		} hpd_mark;
 	} hpd_stats[HPD_NUM_PINS];
+	u32 hpd_event_bits;
 	struct timer_list hotplug_reenable_timer;
 
 	int num_pch_pll;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f60c643..f7219d8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -350,6 +350,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	struct drm_connector *connector;
 	unsigned long irqflags;
 	bool hpd_disabled = false;
+	u32 hpd_event_bits;
 
 	/* HPD irq before everything is fully set up. */
 	if (!dev_priv->enable_hotplug_processing)
@@ -359,6 +360,9 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	DRM_DEBUG_KMS("running encoder hotplug functions\n");
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+
+	hpd_event_bits = dev_priv->hpd_event_bits;
+	dev_priv->hpd_event_bits = 0;
 	list_for_each_entry(connector, &mode_config->connector_list, head) {
 		intel_connector = to_intel_connector(connector);
 		intel_encoder = intel_connector->encoder;
@@ -373,6 +377,10 @@ 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",
+				      drm_get_connector_name(connector), intel_encoder->hpd_pin);
+		}
 	}
 	 /* if there were no outputs to poll, poll was disabled,
 	  * therefore make sure it's enabled when disabling HPD on
@@ -632,6 +640,7 @@ static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
 	for (i = 1; i < HPD_NUM_PINS; i++) {
 		if ((hpd[i] & hotplug_trigger) &&
 		    dev_priv->hpd_stats[i].hpd_mark == HPD_ENABLED) {
+			dev_priv->hpd_event_bits |= (1 << i);
 			if (!time_in_range(jiffies, dev_priv->hpd_stats[i].hpd_last_jiffies,
 					  dev_priv->hpd_stats[i].hpd_last_jiffies
 					  + msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
@@ -639,6 +648,7 @@ static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
 				dev_priv->hpd_stats[i].hpd_cnt = 0;
 			} else if (dev_priv->hpd_stats[i].hpd_cnt > 5) {
 				dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
+				dev_priv->hpd_event_bits &= ~(1 << i);
 				DRM_DEBUG_KMS("HPD interrupt storm detected on  PIN %d\n", i);
 				ret = true;
 			} else
-- 
1.8.1.4

[PATCH v2] drm/i915: Only reprobe display on encoder which has received an HPD event (v2)

[Egbert Eich]

Instead of calling into the DRM helper layer to poll all connectors for
changes in connected displays probe only those connectors which have
received a hotplug event.

Signed-off-by: Egbert Eich <eich@suse.de>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>

v2: Resolved conflicts with changes in previous commits.
    Renamed function and and added a WARN_ON() to warn of
    intel_hpd_irq_event() from being called without
    mode_config.mutex held - suggested by Jani Nikula.
---
 drivers/gpu/drm/i915/i915_irq.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6e643d7..54eca33 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -334,6 +334,21 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 						     crtc);
 }
 
+static int intel_hpd_irq_event(struct drm_device *dev, struct drm_connector *connector)
+{
+	enum drm_connector_status old_status;
+
+	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
+	old_status = connector->status;
+
+	connector->status = connector->funcs->detect(connector, false);
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n",
+		      connector->base.id,
+		      drm_get_connector_name(connector),
+		      old_status, connector->status);
+	return (old_status != connector->status);
+}
+
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
@@ -350,6 +365,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	struct drm_connector *connector;
 	unsigned long irqflags;
 	bool hpd_disabled = false;
+	bool changed = false;
 	u32 hpd_event_bits;
 
 	/* HPD irq before everything is fully set up. */
@@ -395,14 +411,20 @@ static void i915_hotplug_work_func(struct work_struct *work)
 
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
-	list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
-		if (intel_encoder->hot_plug)
-			intel_encoder->hot_plug(intel_encoder);
-
+	list_for_each_entry(connector, &mode_config->connector_list, head) {
+		intel_connector = to_intel_connector(connector);
+		intel_encoder = intel_connector->encoder;
+		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
+			if (intel_encoder->hot_plug)
+				intel_encoder->hot_plug(intel_encoder);
+			if (intel_hpd_irq_event(dev, connector))
+				changed = true;
+		}
+	}
 	mutex_unlock(&mode_config->mutex);
 
-	/* Just fire off a uevent and let userspace tell us what to do */
-	drm_helper_hpd_irq_event(dev);
+	if (changed)
+		drm_kms_helper_hotplug_event(dev);
 }
 
 static void ironlake_handle_rps_change(struct drm_device *dev)
-- 
1.8.1.4

[PATCH v3 Update] drm/i915: Add bit field to record which pins have received HPD events (v3)

[Egbert Eich]

This allows to add code which limits 're'-detect() of displays to connectors
that have received an HPD event.

v2: Reordered drm_i915_private: Move hpd_event_bits to hpd state tracking.
v3: Fixed merge conflicts with previous patches, removed some noisy debug
    logging as suggested by Jani Nikula.

Signed-off-by: Egbert Eich <eich@suse.de>
---
(Sorry for the spam, I had accidenty went the wrong version of this patch)

 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 195b9fe..c0d9fb9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -938,6 +938,7 @@ typedef struct drm_i915_private {
 			HPD_MARK_DISABLED = 2
 		} hpd_mark;
 	} hpd_stats[HPD_NUM_PINS];
+	u32 hpd_event_bits;
 	struct timer_list hotplug_reenable_timer;
 
 	int num_pch_pll;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f60c643..6e643d7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -350,6 +350,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	struct drm_connector *connector;
 	unsigned long irqflags;
 	bool hpd_disabled = false;
+	u32 hpd_event_bits;
 
 	/* HPD irq before everything is fully set up. */
 	if (!dev_priv->enable_hotplug_processing)
@@ -359,6 +360,9 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	DRM_DEBUG_KMS("running encoder hotplug functions\n");
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+
+	hpd_event_bits = dev_priv->hpd_event_bits;
+	dev_priv->hpd_event_bits = 0;
 	list_for_each_entry(connector, &mode_config->connector_list, head) {
 		intel_connector = to_intel_connector(connector);
 		intel_encoder = intel_connector->encoder;
@@ -373,6 +377,12 @@ static void i915_hotplug_work_func(struct work_struct *work)
 				| DRM_CONNECTOR_POLL_DISCONNECT;
 			hpd_disabled = true;
 		}
+#if 0
+		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
+			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
+				      drm_get_connector_name(connector), intel_encoder->hpd_pin);
+		}
+#endif
 	}
 	 /* if there were no outputs to poll, poll was disabled,
 	  * therefore make sure it's enabled when disabling HPD on
@@ -632,6 +642,7 @@ static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
 	for (i = 1; i < HPD_NUM_PINS; i++) {
 		if ((hpd[i] & hotplug_trigger) &&
 		    dev_priv->hpd_stats[i].hpd_mark == HPD_ENABLED) {
+			dev_priv->hpd_event_bits |= (1 << i);
 			if (!time_in_range(jiffies, dev_priv->hpd_stats[i].hpd_last_jiffies,
 					  dev_priv->hpd_stats[i].hpd_last_jiffies
 					  + msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
@@ -639,6 +650,7 @@ static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
 				dev_priv->hpd_stats[i].hpd_cnt = 0;
 			} else if (dev_priv->hpd_stats[i].hpd_cnt > 5) {
 				dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
+				dev_priv->hpd_event_bits &= ~(1 << i);
 				DRM_DEBUG_KMS("HPD interrupt storm detected on  PIN %d\n", i);
 				ret = true;
 			} else
-- 
1.8.1.4

Re: [PATCH v3] drm/i915: Disable HPD interrupt on pin when irq storm is detected (v3)

[Jani Nikula]

On Thu, 11 Apr 2013, Egbert Eich <eich@suse.de> wrote:
> This patch disables hotplug interrupts if an 'interrupt storm'
> has been detected.
> Noise on the interrupt line renders the hotplug interrupt useless:
> each hotplug event causes the devices to be rescanned which will
> will only increase the system load.
> Thus disable the hotplug interrupts and fall back to periodic
> device polling.
>
> v2: Fixed cleanup typo.
> v3: Fixed format issues, clarified a variable name,
>     changed pr_warn() to DRM_INFO() as suggested by
>     Jani Nikula <jani.nikula@linux.intel.com>.
>
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 75 +++++++++++++++++++++++++++++++----------
>  1 file changed, 58 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a3f1ac4..834c717 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -88,7 +88,8 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>  	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
>  };
>  
> -
> +static void ibx_hpd_irq_setup(struct drm_device *dev);
> +static void i915_hpd_irq_setup(struct drm_device *dev);
>  
>  /* For display hotplug interrupt */
>  static void
> @@ -342,7 +343,11 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  						    hotplug_work);
>  	struct drm_device *dev = dev_priv->dev;
>  	struct drm_mode_config *mode_config = &dev->mode_config;
> -	struct intel_encoder *encoder;
> +	struct intel_connector *intel_connector;
> +	struct intel_encoder *intel_encoder;
> +	struct drm_connector *connector;
> +	unsigned long irqflags;
> +	bool hpd_disabled = false;
>  
>  	/* HPD irq before everything is fully set up. */
>  	if (!dev_priv->enable_hotplug_processing)
> @@ -351,9 +356,33 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  	mutex_lock(&mode_config->mutex);
>  	DRM_DEBUG_KMS("running encoder hotplug functions\n");
>  
> -	list_for_each_entry(encoder, &mode_config->encoder_list, base.head)
> -		if (encoder->hot_plug)
> -			encoder->hot_plug(encoder);
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +	list_for_each_entry(connector, &mode_config->connector_list, head) {
> +		intel_connector = to_intel_connector(connector);
> +		intel_encoder = intel_connector->encoder;
> +		if (intel_encoder->hpd_pin > HPD_NONE &&
> +		    dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_MARK_DISABLED &&
> +		    connector->polled == DRM_CONNECTOR_POLL_HPD) {
> +			DRM_INFO("HPD interrupt storm detected on connector %s: "
> +				 "switching from hotplug detection to polling\n",
> +				drm_get_connector_name(connector));
> +			dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark = HPD_DISABLED;
> +			connector->polled = DRM_CONNECTOR_POLL_CONNECT
> +				| DRM_CONNECTOR_POLL_DISCONNECT;
> +			hpd_disabled = true;
> +		}
> +	}
> +	 /* if there were no outputs to poll, poll was disabled,
> +	  * therefore make sure it's enabled when disabling HPD on
> +	  * some connectors */
> +	if (hpd_disabled) {
> +		drm_kms_helper_poll_enable(dev);
> +	}
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +
> +	list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> +		if (intel_encoder->hot_plug)
> +			intel_encoder->hot_plug(intel_encoder);
>  
>  	mutex_unlock(&mode_config->mutex);
>  
> @@ -584,13 +613,14 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
>  
>  #define HPD_STORM_DETECT_PERIOD 1000
>  
> -static inline void hotplug_irq_storm_detect(struct drm_device *dev,
> +static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
>  					    u32 hotplug_trigger,
>  					    const u32 *hpd)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	unsigned long irqflags;
>  	int i;
> +	bool ret = false;
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  
> @@ -605,12 +635,15 @@ static inline void hotplug_irq_storm_detect(struct drm_device *dev,
>  			} else if (dev_priv->hpd_stats[i].hpd_cnt > 5) {
>  				dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
>  				DRM_DEBUG_KMS("HPD interrupt storm detected on  PIN %d\n", i);
> +				ret = true;
>  			} else
>  				dev_priv->hpd_stats[i].hpd_cnt++;
>  		}
>  	}
>  
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +
> +	return ret;
>  }
>  
>  static void gmbus_irq_handler(struct drm_device *dev)
> @@ -686,7 +719,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
>  					 hotplug_status);
>  			if (hotplug_trigger) {
> -				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
> +				if(hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915))
> +					i915_hpd_irq_setup(dev);

What I really meant was s/if( /if (/ there, but I'll look elsewhere now.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  				queue_work(dev_priv->wq,
>  					   &dev_priv->hotplug_work);
>  			}
> @@ -716,7 +750,8 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
>  	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK;
>  
>  	if (hotplug_trigger) {
> -		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_ibx);
> +		if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_ibx))
> +			ibx_hpd_irq_setup(dev);
>  		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
>  	}
>  	if (pch_iir & SDE_AUDIO_POWER_MASK)
> @@ -764,7 +799,8 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>  	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
>  
>  	if (hotplug_trigger) {
> -		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_cpt);
> +		if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_cpt))
> +			ibx_hpd_irq_setup(dev);
>  		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
>  	}
>  	if (pch_iir & SDE_AUDIO_POWER_MASK_CPT)
> @@ -2119,11 +2155,13 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
>  	if (HAS_PCH_IBX(dev)) {
>  		mask &= ~SDE_HOTPLUG_MASK;
>  		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> -			mask |= hpd_ibx[intel_encoder->hpd_pin];
> +			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
> +				mask |= hpd_ibx[intel_encoder->hpd_pin];
>  	} else {
>  		mask &= ~SDE_HOTPLUG_MASK_CPT;
>  		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> -			mask |= hpd_cpt[intel_encoder->hpd_pin];
> +			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
> +				mask |= hpd_cpt[intel_encoder->hpd_pin];
>  	}
>  
>  	I915_WRITE(SDEIMR, ~mask);
> @@ -2651,7 +2689,8 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
>  			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
>  				  hotplug_status);
>  			if (hotplug_trigger) {
> -				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
> +				if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915))
> +					i915_hpd_irq_setup(dev);
>  				queue_work(dev_priv->wq,
>  					   &dev_priv->hotplug_work);
>  			}
> @@ -2801,7 +2840,7 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	struct drm_mode_config *mode_config = &dev->mode_config;
> -	struct intel_encoder *encoder;
> +	struct intel_encoder *intel_encoder;
>  	u32 hotplug_en;
>  
>  	if (I915_HAS_HOTPLUG(dev)) {
> @@ -2809,8 +2848,9 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
>  		hotplug_en &= ~HOTPLUG_INT_EN_MASK;
>  		/* Note HDMI and DP share hotplug bits */
>  		/* enable bits are the same for all generations */
> -		list_for_each_entry(encoder, &mode_config->encoder_list, base.head)
> -			hotplug_en |= hpd_mask_i915[encoder->hpd_pin];
> +		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> +			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
> +				hotplug_en |= hpd_mask_i915[intel_encoder->hpd_pin];
>  		/* Programming the CRT detection parameters tends
>  		   to generate a spurious hotplug event about three
>  		   seconds later.  So just do it once.
> @@ -2888,8 +2928,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
>  			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
>  				  hotplug_status);
>  			if (hotplug_trigger) {
> -				hotplug_irq_storm_detect(dev, hotplug_trigger,
> -							 IS_G4X(dev) ? hpd_status_gen4 : hpd_status_i965);
> +				if (hotplug_irq_storm_detect(dev, hotplug_trigger,
> +							    IS_G4X(dev) ? hpd_status_gen4 : hpd_status_i965))
> +					i915_hpd_irq_setup(dev);
>  				queue_work(dev_priv->wq,
>  					   &dev_priv->hotplug_work);
>  			}
> -- 
> 1.8.1.4

Re: [PATCH v4] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v4)

[Jani Nikula]

On Thu, 11 Apr 2013, Egbert Eich <eich@suse.de> wrote:
> We disable hoptplug detection when we encounter a hotplug event
> storm. Still hotplug detection is required on some outputs (like
> Display Port). The interrupt storm may be only temporary (on certain
> Dell Laptops for instance it happens at certain charging states of
> the system). Thus we enable it after a certain grace period (2 minutes).
> Should the interrupt storm persist it will be detected immediately
> and it will be disabled again.
>
> v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state tracker.
> v3: Clarified loop start value,
>     Removed superfluous test for Ivybridge and Haswell,
>     Restructured loop to avoid deep nesting (all suggested by Ville Syrjälä)
> v4: Fixed two bugs pointed out by Jani Nikula.
>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/i915_irq.c | 50 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 83fc1a6..195b9fe 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -938,6 +938,7 @@ typedef struct drm_i915_private {
>  			HPD_MARK_DISABLED = 2
>  		} hpd_mark;
>  	} hpd_stats[HPD_NUM_PINS];
> +	struct timer_list hotplug_reenable_timer;
>  
>  	int num_pch_pll;
>  	int num_plane;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 834c717..f60c643 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -337,6 +337,8 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
>  /*
>   * Handle hotplug events outside the interrupt handler proper.
>   */
> +#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
> +
>  static void i915_hotplug_work_func(struct work_struct *work)
>  {
>  	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
> @@ -377,7 +379,10 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  	  * some connectors */
>  	if (hpd_disabled) {
>  		drm_kms_helper_poll_enable(dev);
> +		mod_timer(&dev_priv->hotplug_reenable_timer,
> +			  jiffies + msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY));
>  	}
> +
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
>  	list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> @@ -2352,6 +2357,8 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
>  	if (!dev_priv)
>  		return;
>  
> +	del_timer_sync(&dev_priv->hotplug_reenable_timer);
> +
>  	for_each_pipe(pipe)
>  		I915_WRITE(PIPESTAT(pipe), 0xffff);
>  
> @@ -2373,6 +2380,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>  	if (!dev_priv)
>  		return;
>  
> +	del_timer_sync(&dev_priv->hotplug_reenable_timer);
> +
>  	I915_WRITE(HWSTAM, 0xffffffff);
>  
>  	I915_WRITE(DEIMR, 0xffffffff);
> @@ -2749,6 +2758,8 @@ static void i915_irq_uninstall(struct drm_device * dev)
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	int pipe;
>  
> +	del_timer_sync(&dev_priv->hotplug_reenable_timer);
> +
>  	if (I915_HAS_HOTPLUG(dev)) {
>  		I915_WRITE(PORT_HOTPLUG_EN, 0);
>  		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
> @@ -2993,6 +3004,8 @@ static void i965_irq_uninstall(struct drm_device * dev)
>  	if (!dev_priv)
>  		return;
>  
> +	del_timer_sync(&dev_priv->hotplug_reenable_timer);
> +
>  	I915_WRITE(PORT_HOTPLUG_EN, 0);
>  	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
>  
> @@ -3008,6 +3021,41 @@ static void i965_irq_uninstall(struct drm_device * dev)
>  	I915_WRITE(IIR, I915_READ(IIR));
>  }
>  
> +static void i915_reenable_hotplug_timer_func(unsigned long data)
> +{
> +	drm_i915_private_t *dev_priv = (drm_i915_private_t *)data;
> +	struct drm_device *dev = dev_priv->dev;
> +	struct drm_mode_config *mode_config = &dev->mode_config;
> +	unsigned long irqflags;
> +	int i;
> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +	for (i = (HPD_NONE + 1); i < HPD_NUM_PINS; i++) {
> +		struct drm_connector *connector;
> +
> +		if (dev_priv->hpd_stats[i].hpd_mark != HPD_DISABLED)
> +			continue;
> +
> +		dev_priv->hpd_stats[i].hpd_mark = HPD_ENABLED;
> +
> +		list_for_each_entry(connector, &mode_config->connector_list, head) {
> +			struct intel_connector *intel_connector = to_intel_connector(connector);
> +
> +			if (intel_connector->encoder->hpd_pin == i) {
> +				if (connector->polled != intel_connector->polled)
> +					DRM_DEBUG_DRIVER("Reenabling HPD on connector %s\n",
> +							 drm_get_connector_name(connector));
> +				connector->polled = intel_connector->polled;
> +				if (!connector->polled)
> +					connector->polled = DRM_CONNECTOR_POLL_HPD;
> +			}
> +		}
> +	}
> +	if (dev_priv->display.hpd_irq_setup)
> +		dev_priv->display.hpd_irq_setup(dev);
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +}
> +
>  void intel_irq_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3020,6 +3068,8 @@ void intel_irq_init(struct drm_device *dev)
>  	setup_timer(&dev_priv->gpu_error.hangcheck_timer,
>  		    i915_hangcheck_elapsed,
>  		    (unsigned long) dev);
> +	setup_timer(&dev_priv->hotplug_reenable_timer, i915_reenable_hotplug_timer_func,
> +		    (unsigned long) dev_priv);
>  
>  	pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
>  
> -- 
> 1.8.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 5/7] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v3)

[Jani Nikula]

On Thu, 11 Apr 2013, Egbert Eich <eich@freedesktop.org> wrote:
> On Thu, Apr 11, 2013 at 01:44:51PM +0300, Jani Nikula wrote:
>> > +
>> > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>> > +	for (i = (HPD_NONE + 1); i < HPD_NUM_PINS; i++) {
>> > +		struct drm_connector *connector;
>> > +
>> > +		if (dev_priv->hpd_stats[i].hpd_mark != HPD_MARK_DISABLED)
>> 
>> I think this is wrong, skipping HPD_DISABLED.
>
> Right, this was indeed wrong.
>
>> 
>> > +			continue;
>> > +
>> > +		dev_priv->hpd_stats[i].hpd_mark = HPD_ENABLED;
>> > +
>> > +		list_for_each_entry(connector, &mode_config->connector_list, head) {
>> > +			struct intel_connector *intel_connector = to_intel_connector(connector);
>> > +
>> > +			if (intel_connector->encoder->hpd_pin == i) {
>> > +				if (connector->polled != intel_connector->polled)
>> > +					DRM_DEBUG_DRIVER("Reenabling HPD on connector %s\n",
>> > +							 drm_get_connector_name(connector));
>> > +				connector->polled = intel_connector->polled;
>> > +				if (!connector->polled)
>> > +					connector->polled = DRM_CONNECTOR_POLL_HPD;
>> > +			}
>> > +		}
>> > +		dev_priv->display.hpd_irq_setup(dev);
>> 
>> You don't need to call this at each iteration, do you?
>
> Right, you are right here as well.
>> 
>> > +	}
>> 
>> In fact, couldn't you just call intel_hpd_init(), or modify it to do
>> what you want? Keep it simple.
>
> Well, intel_hpd_init() is initializing every to dev_priv->hpd_stats[i].hpd_mark
> to HPD_ENABLED. Can we rule out that the timer runs between the interrupt
> handler and the worker - as in this state this variable might me set to
> HPD_MARK_DISABLED?
> In fact it would probably not even hurt if we changed the HPD_MARK_DISABLED
> to HPD_ENABLED as in a storm condition this condition will soon reappear but
> I'd rather avoid it.
> Of course we could pass an argument to the function to distinguish both
> conditions. This is a simplification which can still be introduced - when
> I'm in fact able to do some testing.

Yeah, let's go with this for now. R-b added.

One idea is to reuse the per-pin hpd_last_jiffies to check if enough
time has passed from the last storm on each pin. That could be done
here, or we could even throw out the timer, and check hpd_last_jiffies
on polling detect callbacks. Or maybe the latter is too spread out all
over the place. Perhaps you can think of something nice based on
this. ;)

> Thanks a lot for the review!

Thanks for doing this!


BR,
Jani.


>
> Cheers,
> 	Egbert.

Re: [PATCH v3 Update] drm/i915: Add bit field to record which pins have received HPD events (v3)

[Jani Nikula]

On Thu, 11 Apr 2013, Egbert Eich <eich@suse.de> wrote:
> This allows to add code which limits 're'-detect() of displays to connectors
> that have received an HPD event.
>
> v2: Reordered drm_i915_private: Move hpd_event_bits to hpd state tracking.
> v3: Fixed merge conflicts with previous patches, removed some noisy debug
>     logging as suggested by Jani Nikula.

Okay, I'm not generally enthusiastic about adding #if 0 stuff. If we're
going to remove that later anyway, we might just as well add the noisy
debug print and remove *that* later. I'll just dodge this and let Daniel
decide. Both versions get my

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
> (Sorry for the spam, I had accidenty went the wrong version of this patch)
>
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 195b9fe..c0d9fb9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -938,6 +938,7 @@ typedef struct drm_i915_private {
>  			HPD_MARK_DISABLED = 2
>  		} hpd_mark;
>  	} hpd_stats[HPD_NUM_PINS];
> +	u32 hpd_event_bits;
>  	struct timer_list hotplug_reenable_timer;
>  
>  	int num_pch_pll;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index f60c643..6e643d7 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -350,6 +350,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  	struct drm_connector *connector;
>  	unsigned long irqflags;
>  	bool hpd_disabled = false;
> +	u32 hpd_event_bits;
>  
>  	/* HPD irq before everything is fully set up. */
>  	if (!dev_priv->enable_hotplug_processing)
> @@ -359,6 +360,9 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  	DRM_DEBUG_KMS("running encoder hotplug functions\n");
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +
> +	hpd_event_bits = dev_priv->hpd_event_bits;
> +	dev_priv->hpd_event_bits = 0;
>  	list_for_each_entry(connector, &mode_config->connector_list, head) {
>  		intel_connector = to_intel_connector(connector);
>  		intel_encoder = intel_connector->encoder;
> @@ -373,6 +377,12 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  				| DRM_CONNECTOR_POLL_DISCONNECT;
>  			hpd_disabled = true;
>  		}
> +#if 0
> +		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
> +			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
> +				      drm_get_connector_name(connector), intel_encoder->hpd_pin);
> +		}
> +#endif
>  	}
>  	 /* if there were no outputs to poll, poll was disabled,
>  	  * therefore make sure it's enabled when disabling HPD on
> @@ -632,6 +642,7 @@ static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
>  	for (i = 1; i < HPD_NUM_PINS; i++) {
>  		if ((hpd[i] & hotplug_trigger) &&
>  		    dev_priv->hpd_stats[i].hpd_mark == HPD_ENABLED) {
> +			dev_priv->hpd_event_bits |= (1 << i);
>  			if (!time_in_range(jiffies, dev_priv->hpd_stats[i].hpd_last_jiffies,
>  					  dev_priv->hpd_stats[i].hpd_last_jiffies
>  					  + msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
> @@ -639,6 +650,7 @@ static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
>  				dev_priv->hpd_stats[i].hpd_cnt = 0;
>  			} else if (dev_priv->hpd_stats[i].hpd_cnt > 5) {
>  				dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
> +				dev_priv->hpd_event_bits &= ~(1 << i);
>  				DRM_DEBUG_KMS("HPD interrupt storm detected on  PIN %d\n", i);
>  				ret = true;
>  			} else
> -- 
> 1.8.1.4

Re: [PATCH v2] drm/i915: Only reprobe display on encoder which has received an HPD event (v2)

[Jani Nikula]

On Thu, 11 Apr 2013, Egbert Eich <eich@suse.de> wrote:
> Instead of calling into the DRM helper layer to poll all connectors for
> changes in connected displays probe only those connectors which have
> received a hotplug event.
>
> Signed-off-by: Egbert Eich <eich@suse.de>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Yup.

>
> v2: Resolved conflicts with changes in previous commits.
>     Renamed function and and added a WARN_ON() to warn of
>     intel_hpd_irq_event() from being called without
>     mode_config.mutex held - suggested by Jani Nikula.
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6e643d7..54eca33 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -334,6 +334,21 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
>  						     crtc);
>  }
>  
> +static int intel_hpd_irq_event(struct drm_device *dev, struct drm_connector *connector)
> +{
> +	enum drm_connector_status old_status;
> +
> +	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> +	old_status = connector->status;
> +
> +	connector->status = connector->funcs->detect(connector, false);
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n",
> +		      connector->base.id,
> +		      drm_get_connector_name(connector),
> +		      old_status, connector->status);
> +	return (old_status != connector->status);
> +}
> +
>  /*
>   * Handle hotplug events outside the interrupt handler proper.
>   */
> @@ -350,6 +365,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  	struct drm_connector *connector;
>  	unsigned long irqflags;
>  	bool hpd_disabled = false;
> +	bool changed = false;
>  	u32 hpd_event_bits;
>  
>  	/* HPD irq before everything is fully set up. */
> @@ -395,14 +411,20 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
> -	list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> -		if (intel_encoder->hot_plug)
> -			intel_encoder->hot_plug(intel_encoder);
> -
> +	list_for_each_entry(connector, &mode_config->connector_list, head) {
> +		intel_connector = to_intel_connector(connector);
> +		intel_encoder = intel_connector->encoder;
> +		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
> +			if (intel_encoder->hot_plug)
> +				intel_encoder->hot_plug(intel_encoder);
> +			if (intel_hpd_irq_event(dev, connector))
> +				changed = true;
> +		}
> +	}
>  	mutex_unlock(&mode_config->mutex);
>  
> -	/* Just fire off a uevent and let userspace tell us what to do */
> -	drm_helper_hpd_irq_event(dev);
> +	if (changed)
> +		drm_kms_helper_hotplug_event(dev);
>  }
>  
>  static void ironlake_handle_rps_change(struct drm_device *dev)
> -- 
> 1.8.1.4

Re: [PATCH v3 1/7] drm/i915: Add HPD IRQ storm detection (v4)

[Egbert Eich]

Hi Jani,

On Thu, Apr 11, 2013 at 12:32:29PM +0300, Jani Nikula wrote:
> 
> Hi Egbert -
> 
> Up front, I haven't been following this series or read any of the
> previous review comments. Please bear with me, and feel free to direct
> me to earlier comments if I'm in contradiction.

Sorry for the late reply - last week I was quite busy with other stuff,
this week i'm a bit preoccupied.

> 
> On Tue, 09 Apr 2013, Egbert Eich <eich@freedesktop.org> wrote:
> > From: Egbert Eich <eich@suse.de>
> >
> > Add a hotplug IRQ storm detection (triggered when a hotplug interrupt
> > fires more than 5 times / sec).
> 
> Okay, this is theoretical, but a display port sink could do more than
> that many hpd irq requests when connected. Which leads me to wonder in
> general if the storm detection should be different for hot plug
> vs. unplug and hpd irq events.

Agreed. 
During my tests I did not see any issues with the statistics I've 
implemented: 5/sec was an ad-hoc choice and I wanted to start with 
something simple.
I've tested it and it seemed to work, so I didn't bother to look into
this more deeply, however if you feel 5 events  / sec are too few to
really do a good distinction, we could easily increase this number.

There have been two situations where I have seen 'interrupt storms':

1. On G35: some boxes of the affected systems do not show this issue 
   at all while others see a very high load but are still usable.
   In my recollection there were in the order of some 100 interrupts/sec
   on these machines. Then there were systems which would 'stall' in
   the worker during boot due to high load. At one point the NMI 
   watchdog kicked in and stopped this mess. There the interrupts
   happened at an order of magnitude if 10k!

2. A laptop with a Sandybridge chipset where the system load went
   high at certain stages of charging levels - when the power supply
   was connected. I would assume the frequency there also was around
   some 100 / sec.

Thus if we increase the threshold frequency to some 10 / sec we
would still cover all those cases.

Some other issue I've seen is 'bouncing' during manual plugging
I've been contemplating how to address this. There are two things 
to look at:
1. multiple hotplug events due to not getting a perfect connection at first
2. EDID readout happening to early when the EDID lines are not yet fully
   and 'permanently' connected.
It might well be, that a fix for these issues might actually also adress 
the issues you are pointing out.

I have not seen them on Intel hardware - but this may be due to the 
fact that the hardware I saw it on was a separate gfx card which did 
not have the usual mounting bracket and thus the entire setup was a 
bit fragile and not really suitable for hot-plugging.
However I believe that these things might happen everywere for people
not quite used to plugging monitors too much.

> 
> Have you observed difference between hot plug/unplug?

There seems to be a difference between monitor connected/not connected:

On DVI (G35) one doesn't distinguish between plug/unplug: when the hotplug
line on the connector changes state an interrupt is sent. On this system
storms only happened when a monitor was conneted - since the state of the
HPD pin is signalled thru different frequencies on a line across SDVO (in 
my recollection it was 10 vs. 20 MHz) I believe that due to cross talk
the higher(?) frequency could not always reliably be measured.

I did not have access to the laptop system and the customer was not 
patient enough to help me to debug this further with me.

Generally I think we would still adress the 'strom condition' if we
raised the threshold to 20 or 30 /sec.

What do you think?

> 
> Has this been a problem on PCH split platforms, i.e. since ilk/gen5?

I've also observed this on Sandybridge - which would be past GEN5, 
wouldn't it?

I will address some of the other issues mentioned in a new patch.

Thanks a lot for looking at it!

Cheers,
	Egbert.

Re: [PATCH v3 1/7] drm/i915: Add HPD IRQ storm detection (v4)

[Egbert Eich]

Hi Jani,

I've rebased and regenerated all the patches now, as there 
were some other bikesheds i had not adressed. I've also
included all Reviewed-By: 
This should make it easier to integrate the patches.

Some comments below:


On Thu, Apr 11, 2013 at 12:32:29PM +0300, Jani Nikula wrote:
> > +	struct {
> > +		unsigned long hpd_last_jiffies;
> > +		int hpd_cnt;
> > +		enum {
> > +			HPD_ENABLED = 0,
> > +			HPD_DISABLED = 1,
> > +			HPD_MARK_DISABLED = 2
> > +		} hpd_mark;
> > +	} hpd_stats[HPD_NUM_PINS];
> 
> With all the hotplug stuff being added, I think it's getting time to
> group all the hotplug stuff under a struct.

I will postpone this until I address the issues that I have on my TODO.

> 
> >  	int num_pch_pll;
> >  	int num_plane;
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 4c5bdd0..32b5527 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -582,6 +582,37 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
> >  	queue_work(dev_priv->wq, &dev_priv->rps.work);
> >  }
> >  
> > +#define HPD_STORM_DETECT_PERIOD 1000
> > +
> > +static inline void hotplug_irq_storm_detect(struct drm_device *dev,
> > +					    u32 hotplug_trigger,
> > +					    const u32 *hpd)
> 
> I think you should just add the bool return status right here instead of
> postponing until the later patch that needs it.

I thought about this and left it as it is: Returning a bool status now
will raise other questions as the logic in this patch doesn't require it.
I'd rather have a bigger patch later which will however clearly explains
the reason for the change to the function.

> 
> > +{
> > +	drm_i915_private_t *dev_priv = dev->dev_private;
> > +	unsigned long irqflags;
> > +	int i;
> > +
> > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > +
> > +	for (i = 1; i < HPD_NUM_PINS; i++) {
> > +		if ((hpd[i] & hotplug_trigger) &&
> > +		    dev_priv->hpd_stats[i].hpd_mark == HPD_ENABLED) {
> 
> Bikeshed: You might get a nicer layout below by negating that if and
> adding continue.

Fixed.

> 
> > +			if (!time_in_range(jiffies, dev_priv->hpd_stats[i].hpd_last_jiffies,
> > +					  dev_priv->hpd_stats[i].hpd_last_jiffies
> > +					  + msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
> > +				dev_priv->hpd_stats[i].hpd_last_jiffies = jiffies;
> > +				dev_priv->hpd_stats[i].hpd_cnt = 0;
> > +			} else if (dev_priv->hpd_stats[i].hpd_cnt > 5) {
> > +				dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
> > +				DRM_DEBUG_KMS("HPD interrupt storm detected on  PIN %d\n", i);
> 
> Extra space before "PIN".

Fixed.
> 
> > +			} else
> > +				dev_priv->hpd_stats[i].hpd_cnt++;
> 
> If one branch requires braces, then all do.

Fixed.

Cheers,
	Egbert.

[PATCH 1/7] drm/i915: Add HPD IRQ storm detection (v5)

[Egbert Eich]

Add a hotplug IRQ storm detection (triggered when a hotplug interrupt
fires more than 5 times / sec).
Rationale:
Despite of the many attempts to fix the problem with noisy hotplug
interrupt lines we are still seeing systems which have issues:
Once cause of noise seems to be bad routing of the hotplug line
on the board: cross talk from other signals seems to cause erronous
hotplug interrupts. This has been documented as an erratum for the
the i945GM chipset and thus hotplug support was disabled for this
chipset model but others seem to have this problem, too.

We have seen this issue on a G35 motherboard for example:
Even different motherboards of the same model seem to behave
differently: while some only see only around 10-100 interrupts/s
others seem to see 5k or more.
We've also observed a dependency on the selected video mode.

Also on certain laptops interrupt noise seems to occur duing
battery charging when the battery is at a certain charge levels.

Thus we add a simple algorithm here that detects an 'interrupt storm'
condition.

v2: Fixed comment.
v3: Reordered drm_i915_private: moved hpd state tracking to hotplug work stuff.
v4: Followed by Jesse Barnes to use a time_..() macro.
v5: Fixed coding style as suggested by Jani Nikula.

Signed-off-by: Egbert Eich <eich@suse.de>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.h |  9 ++++++
 drivers/gpu/drm/i915/i915_irq.c | 69 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 66 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 44fca0b..83fc1a6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -929,6 +929,15 @@ typedef struct drm_i915_private {
 
 	struct work_struct hotplug_work;
 	bool enable_hotplug_processing;
+	struct {
+		unsigned long hpd_last_jiffies;
+		int hpd_cnt;
+		enum {
+			HPD_ENABLED = 0,
+			HPD_DISABLED = 1,
+			HPD_MARK_DISABLED = 2
+		} hpd_mark;
+	} hpd_stats[HPD_NUM_PINS];
 
 	int num_pch_pll;
 	int num_plane;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4c5bdd0..71fc238 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -582,6 +582,40 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
 	queue_work(dev_priv->wq, &dev_priv->rps.work);
 }
 
+#define HPD_STORM_DETECT_PERIOD 1000
+#define HPD_STORM_THRESHOLD 5
+
+static inline void hotplug_irq_storm_detect(struct drm_device *dev,
+					    u32 hotplug_trigger,
+					    const u32 *hpd)
+{
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	unsigned long irqflags;
+	int i;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+
+	for (i = 1; i < HPD_NUM_PINS; i++) {
+		if (!(hpd[i] & hotplug_trigger) ||
+		    dev_priv->hpd_stats[i].hpd_mark != HPD_ENABLED)
+			continue;
+
+		if (!time_in_range(jiffies, dev_priv->hpd_stats[i].hpd_last_jiffies,
+				   dev_priv->hpd_stats[i].hpd_last_jiffies
+				   + msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
+			dev_priv->hpd_stats[i].hpd_last_jiffies = jiffies;
+			dev_priv->hpd_stats[i].hpd_cnt = 0;
+		} else if (dev_priv->hpd_stats[i].hpd_cnt > HPD_STORM_THRESHOLD) {
+			dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
+			DRM_DEBUG_KMS("HPD interrupt storm detected on PIN %d\n", i);
+		} else {
+			dev_priv->hpd_stats[i].hpd_cnt++;
+		}
+	}
+
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
+
 static void gmbus_irq_handler(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = (drm_i915_private_t *) dev->dev_private;
@@ -650,13 +684,15 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 		/* Consume port.  Then clear IIR or we'll miss events */
 		if (iir & I915_DISPLAY_PORT_INTERRUPT) {
 			u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
+			u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
 
 			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
 					 hotplug_status);
-			if (hotplug_status & HOTPLUG_INT_STATUS_I915)
+			if (hotplug_trigger) {
+				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
 				queue_work(dev_priv->wq,
 					   &dev_priv->hotplug_work);
-
+			}
 			I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status);
 			I915_READ(PORT_HOTPLUG_STAT);
 		}
@@ -680,10 +716,12 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	int pipe;
+	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK;
 
-	if (pch_iir & SDE_HOTPLUG_MASK)
+	if (hotplug_trigger) {
+		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_ibx);
 		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
-
+	}
 	if (pch_iir & SDE_AUDIO_POWER_MASK)
 		DRM_DEBUG_DRIVER("PCH audio power change on port %d\n",
 				 (pch_iir & SDE_AUDIO_POWER_MASK) >>
@@ -726,10 +764,12 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	int pipe;
+	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
 
-	if (pch_iir & SDE_HOTPLUG_MASK_CPT)
+	if (hotplug_trigger) {
+		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_cpt);
 		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
-
+	}
 	if (pch_iir & SDE_AUDIO_POWER_MASK_CPT)
 		DRM_DEBUG_DRIVER("PCH audio power change on port %d\n",
 				 (pch_iir & SDE_AUDIO_POWER_MASK_CPT) >>
@@ -2607,13 +2647,15 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
 		if ((I915_HAS_HOTPLUG(dev)) &&
 		    (iir & I915_DISPLAY_PORT_INTERRUPT)) {
 			u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
+			u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
 
 			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
 				  hotplug_status);
-			if (hotplug_status & HOTPLUG_INT_STATUS_I915)
+			if (hotplug_trigger) {
+				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
 				queue_work(dev_priv->wq,
 					   &dev_priv->hotplug_work);
-
+			}
 			I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status);
 			POSTING_READ(PORT_HOTPLUG_STAT);
 		}
@@ -2840,15 +2882,18 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
 		/* Consume port.  Then clear IIR or we'll miss events */
 		if (iir & I915_DISPLAY_PORT_INTERRUPT) {
 			u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
+			u32 hotplug_trigger = hotplug_status & (IS_G4X(dev) ?
+								  HOTPLUG_INT_STATUS_G4X :
+								  HOTPLUG_INT_STATUS_I965);
 
 			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
 				  hotplug_status);
-			if (hotplug_status & (IS_G4X(dev) ?
-					      HOTPLUG_INT_STATUS_G4X :
-					      HOTPLUG_INT_STATUS_I965))
+			if (hotplug_trigger) {
+				hotplug_irq_storm_detect(dev, hotplug_trigger,
+							 IS_G4X(dev) ? hpd_status_gen4 : hpd_status_i965);
 				queue_work(dev_priv->wq,
 					   &dev_priv->hotplug_work);
-
+			}
 			I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status);
 			I915_READ(PORT_HOTPLUG_STAT);
 		}
-- 
1.8.1.4

[PATCH 2/7] drm/i915: (re)init HPD interrupt storm statistics

[Egbert Eich]

When an encoder is shared on several connectors there is only
one hotplug line, thus this line needs to be shared among these
connectors.
If HPD detect only works reliably on a subset of those connectors,
we want to poll the others. Thus we need to make sure that storm
detection doesn't mess up the settings for those connectors.
Therefore we store the settings in the intel_connector struct and
restore them from there.
If nothing is set but the encoder has a hpd_pin set we assume this
connector is hotplug capable.
On init/reset we make sure the polled state of the connectors
is (re)set to the default value, the HPD interrupts are marked
enabled.

Signed-off-by: Egbert Eich <eich@suse.de>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c   | 14 ++++++++++++++
 drivers/gpu/drm/i915/intel_crt.c  |  6 ++----
 drivers/gpu/drm/i915/intel_dp.c   |  1 -
 drivers/gpu/drm/i915/intel_drv.h  |  4 ++++
 drivers/gpu/drm/i915/intel_hdmi.c |  1 -
 drivers/gpu/drm/i915/intel_sdvo.c |  5 ++---
 drivers/gpu/drm/i915/intel_tv.c   |  2 +-
 7 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 71fc238..bc00532 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -596,6 +596,7 @@ static inline void hotplug_irq_storm_detect(struct drm_device *dev,
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 
 	for (i = 1; i < HPD_NUM_PINS; i++) {
+
 		if (!(hpd[i] & hotplug_trigger) ||
 		    dev_priv->hpd_stats[i].hpd_mark != HPD_ENABLED)
 			continue;
@@ -3048,7 +3049,20 @@ void intel_irq_init(struct drm_device *dev)
 void intel_hpd_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	struct drm_connector *connector;
+	int i;
 
+	for (i = 1; i < HPD_NUM_PINS; i++) {
+		dev_priv->hpd_stats[i].hpd_cnt = 0;
+		dev_priv->hpd_stats[i].hpd_mark = HPD_ENABLED;
+	}
+	list_for_each_entry(connector, &mode_config->connector_list, head) {
+		struct intel_connector *intel_connector = to_intel_connector(connector);
+		connector->polled = intel_connector->polled;
+		if (!connector->polled && I915_HAS_HOTPLUG(dev) && intel_connector->encoder->hpd_pin > HPD_NONE)
+			connector->polled = DRM_CONNECTOR_POLL_HPD;
+	}
 	if (dev_priv->display.hpd_irq_setup)
 		dev_priv->display.hpd_irq_setup(dev);
 }
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 1ae2d7f..c063b9f 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -793,10 +793,8 @@ void intel_crt_init(struct drm_device *dev)
 
 	drm_sysfs_connector_add(connector);
 
-	if (I915_HAS_HOTPLUG(dev))
-		connector->polled = DRM_CONNECTOR_POLL_HPD;
-	else
-		connector->polled = DRM_CONNECTOR_POLL_CONNECT;
+	if (!I915_HAS_HOTPLUG(dev))
+		intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
 
 	/*
 	 * Configure the automatic hotplug detection stuff
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 482b5e5..1e9b19a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2786,7 +2786,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	drm_connector_init(dev, connector, &intel_dp_connector_funcs, type);
 	drm_connector_helper_add(connector, &intel_dp_connector_helper_funcs);
 
-	connector->polled = DRM_CONNECTOR_POLL_HPD;
 	connector->interlace_allowed = true;
 	connector->doublescan_allowed = 0;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d7bd031..a05fde7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -171,6 +171,10 @@ struct intel_connector {
 
 	/* Cached EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */
 	struct edid *edid;
+
+	/* since POLL and HPD connectors may use the same HPD line keep the native
+	   state of connector->polled in case hotplug storm detection changes it */
+	u8 polled;
 };
 
 struct intel_crtc_config {
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index ee4a8da..8912201 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -998,7 +998,6 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 			   DRM_MODE_CONNECTOR_HDMIA);
 	drm_connector_helper_add(connector, &intel_hdmi_connector_helper_funcs);
 
-	connector->polled = DRM_CONNECTOR_POLL_HPD;
 	connector->interlace_allowed = 1;
 	connector->doublescan_allowed = 0;
 
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 298dc85..64b8b40 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2276,7 +2276,6 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 	connector = &intel_connector->base;
 	if (intel_sdvo_get_hotplug_support(intel_sdvo) &
 		intel_sdvo_connector->output_flag) {
-		connector->polled = DRM_CONNECTOR_POLL_HPD;
 		intel_sdvo->hotplug_active |= intel_sdvo_connector->output_flag;
 		/* Some SDVO devices have one-shot hotplug interrupts.
 		 * Ensure that they get re-enabled when an interrupt happens.
@@ -2284,7 +2283,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 		intel_encoder->hot_plug = intel_sdvo_enable_hotplug;
 		intel_sdvo_enable_hotplug(intel_encoder);
 	} else {
-		connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
+		intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
 	}
 	encoder->encoder_type = DRM_MODE_ENCODER_TMDS;
 	connector->connector_type = DRM_MODE_CONNECTOR_DVID;
@@ -2353,7 +2352,7 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
 
 	intel_connector = &intel_sdvo_connector->base;
 	connector = &intel_connector->base;
-	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
+	intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
 	encoder->encoder_type = DRM_MODE_ENCODER_DAC;
 	connector->connector_type = DRM_MODE_CONNECTOR_VGA;
 
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 6673726..b945bc5 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1613,7 +1613,7 @@ intel_tv_init(struct drm_device *dev)
 	 *
 	 * More recent chipsets favour HDMI rather than integrated S-Video.
 	 */
-	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
+	intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
 
 	drm_connector_init(dev, connector, &intel_tv_connector_funcs,
 			   DRM_MODE_CONNECTOR_SVIDEO);
-- 
1.8.1.4

[PATCH 3/7] drm/i915: Mask out the HPD irq bits before setting them individually.

[Egbert Eich]

To disable previously enabled HPD IRQs we need to reset them and
set the enabled ones individually.

Signed-off-by: Egbert Eich <eich@suse.de>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bc00532..4cf33b3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2121,9 +2121,11 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
 	u32 hotplug;
 
 	if (HAS_PCH_IBX(dev)) {
+		mask &= ~SDE_HOTPLUG_MASK;
 		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
 			mask |= hpd_ibx[intel_encoder->hpd_pin];
 	} else {
+		mask &= ~SDE_HOTPLUG_MASK_CPT;
 		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
 			mask |= hpd_cpt[intel_encoder->hpd_pin];
 	}
-- 
1.8.1.4

[PATCH 4/7] drm/i915: Disable HPD interrupt on pin when irq storm is detected (v3)

[Egbert Eich]

This patch disables hotplug interrupts if an 'interrupt storm'
has been detected.
Noise on the interrupt line renders the hotplug interrupt useless:
each hotplug event causes the devices to be rescanned which will
will only increase the system load.
Thus disable the hotplug interrupts and fall back to periodic
device polling.

v2: Fixed cleanup typo.
v3: Fixed format issues, clarified a variable name,
    changed pr_warn() to DRM_INFO() as suggested by
    Jani Nikula <jani.nikula@linux.intel.com>.

Signed-off-by: Egbert Eich <eich@suse.de>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 75 +++++++++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4cf33b3..d059c5d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -88,7 +88,8 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
-
+static void ibx_hpd_irq_setup(struct drm_device *dev);
+static void i915_hpd_irq_setup(struct drm_device *dev);
 
 /* For display hotplug interrupt */
 static void
@@ -342,7 +343,11 @@ static void i915_hotplug_work_func(struct work_struct *work)
 						    hotplug_work);
 	struct drm_device *dev = dev_priv->dev;
 	struct drm_mode_config *mode_config = &dev->mode_config;
-	struct intel_encoder *encoder;
+	struct intel_connector *intel_connector;
+	struct intel_encoder *intel_encoder;
+	struct drm_connector *connector;
+	unsigned long irqflags;
+	bool hpd_disabled = false;
 
 	/* HPD irq before everything is fully set up. */
 	if (!dev_priv->enable_hotplug_processing)
@@ -351,9 +356,33 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	mutex_lock(&mode_config->mutex);
 	DRM_DEBUG_KMS("running encoder hotplug functions\n");
 
-	list_for_each_entry(encoder, &mode_config->encoder_list, base.head)
-		if (encoder->hot_plug)
-			encoder->hot_plug(encoder);
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+	list_for_each_entry(connector, &mode_config->connector_list, head) {
+		intel_connector = to_intel_connector(connector);
+		intel_encoder = intel_connector->encoder;
+		if (intel_encoder->hpd_pin > HPD_NONE &&
+		    dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_MARK_DISABLED &&
+		    connector->polled == DRM_CONNECTOR_POLL_HPD) {
+			DRM_INFO("HPD interrupt storm detected on connector %s: "
+				 "switching from hotplug detection to polling\n",
+				drm_get_connector_name(connector));
+			dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark = HPD_DISABLED;
+			connector->polled = DRM_CONNECTOR_POLL_CONNECT
+				| DRM_CONNECTOR_POLL_DISCONNECT;
+			hpd_disabled = true;
+		}
+	}
+	 /* if there were no outputs to poll, poll was disabled,
+	  * therefore make sure it's enabled when disabling HPD on
+	  * some connectors */
+	if (hpd_disabled)
+		drm_kms_helper_poll_enable(dev);
+
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+
+	list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
+		if (intel_encoder->hot_plug)
+			intel_encoder->hot_plug(intel_encoder);
 
 	mutex_unlock(&mode_config->mutex);
 
@@ -585,13 +614,14 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
 #define HPD_STORM_DETECT_PERIOD 1000
 #define HPD_STORM_THRESHOLD 5
 
-static inline void hotplug_irq_storm_detect(struct drm_device *dev,
+static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
 					    u32 hotplug_trigger,
 					    const u32 *hpd)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	unsigned long irqflags;
 	int i;
+	bool ret = false;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 
@@ -609,12 +639,15 @@ static inline void hotplug_irq_storm_detect(struct drm_device *dev,
 		} else if (dev_priv->hpd_stats[i].hpd_cnt > HPD_STORM_THRESHOLD) {
 			dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
 			DRM_DEBUG_KMS("HPD interrupt storm detected on PIN %d\n", i);
+			ret = true;
 		} else {
 			dev_priv->hpd_stats[i].hpd_cnt++;
 		}
 	}
 
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+
+	return ret;
 }
 
 static void gmbus_irq_handler(struct drm_device *dev)
@@ -690,7 +723,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
 					 hotplug_status);
 			if (hotplug_trigger) {
-				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
+				if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915))
+					i915_hpd_irq_setup(dev);
 				queue_work(dev_priv->wq,
 					   &dev_priv->hotplug_work);
 			}
@@ -720,7 +754,8 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
 	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK;
 
 	if (hotplug_trigger) {
-		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_ibx);
+		if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_ibx))
+			ibx_hpd_irq_setup(dev);
 		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
 	}
 	if (pch_iir & SDE_AUDIO_POWER_MASK)
@@ -768,7 +803,8 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
 	u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
 
 	if (hotplug_trigger) {
-		hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_cpt);
+		if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_cpt))
+			ibx_hpd_irq_setup(dev);
 		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
 	}
 	if (pch_iir & SDE_AUDIO_POWER_MASK_CPT)
@@ -2123,11 +2159,13 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
 	if (HAS_PCH_IBX(dev)) {
 		mask &= ~SDE_HOTPLUG_MASK;
 		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
-			mask |= hpd_ibx[intel_encoder->hpd_pin];
+			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
+				mask |= hpd_ibx[intel_encoder->hpd_pin];
 	} else {
 		mask &= ~SDE_HOTPLUG_MASK_CPT;
 		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
-			mask |= hpd_cpt[intel_encoder->hpd_pin];
+			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
+				mask |= hpd_cpt[intel_encoder->hpd_pin];
 	}
 
 	I915_WRITE(SDEIMR, ~mask);
@@ -2655,7 +2693,8 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
 			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
 				  hotplug_status);
 			if (hotplug_trigger) {
-				hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915);
+				if (hotplug_irq_storm_detect(dev, hotplug_trigger, hpd_status_i915))
+					i915_hpd_irq_setup(dev);
 				queue_work(dev_priv->wq,
 					   &dev_priv->hotplug_work);
 			}
@@ -2805,7 +2844,7 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	struct drm_mode_config *mode_config = &dev->mode_config;
-	struct intel_encoder *encoder;
+	struct intel_encoder *intel_encoder;
 	u32 hotplug_en;
 
 	if (I915_HAS_HOTPLUG(dev)) {
@@ -2813,8 +2852,9 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
 		hotplug_en &= ~HOTPLUG_INT_EN_MASK;
 		/* Note HDMI and DP share hotplug bits */
 		/* enable bits are the same for all generations */
-		list_for_each_entry(encoder, &mode_config->encoder_list, base.head)
-			hotplug_en |= hpd_mask_i915[encoder->hpd_pin];
+		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
+			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
+				hotplug_en |= hpd_mask_i915[intel_encoder->hpd_pin];
 		/* Programming the CRT detection parameters tends
 		   to generate a spurious hotplug event about three
 		   seconds later.  So just do it once.
@@ -2892,8 +2932,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
 			DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
 				  hotplug_status);
 			if (hotplug_trigger) {
-				hotplug_irq_storm_detect(dev, hotplug_trigger,
-							 IS_G4X(dev) ? hpd_status_gen4 : hpd_status_i965);
+				if (hotplug_irq_storm_detect(dev, hotplug_trigger,
+							    IS_G4X(dev) ? hpd_status_gen4 : hpd_status_i965))
+					i915_hpd_irq_setup(dev);
 				queue_work(dev_priv->wq,
 					   &dev_priv->hotplug_work);
 			}
-- 
1.8.1.4

[PATCH 5/7] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v4)

[Egbert Eich]

We disable hoptplug detection when we encounter a hotplug event
storm. Still hotplug detection is required on some outputs (like
Display Port). The interrupt storm may be only temporary (on certain
Dell Laptops for instance it happens at certain charging states of
the system). Thus we enable it after a certain grace period (2 minutes).
Should the interrupt storm persist it will be detected immediately
and it will be disabled again.

v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state tracker.
v3: Clarified loop start value,
    Removed superfluous test for Ivybridge and Haswell,
    Restructured loop to avoid deep nesting (all suggested by Ville Syrjälä)
v4: Fixed two bugs pointed out by Jani Nikula.

Signed-off-by: Egbert Eich <eich@suse.de>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_irq.c | 52 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 83fc1a6..195b9fe 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -938,6 +938,7 @@ typedef struct drm_i915_private {
 			HPD_MARK_DISABLED = 2
 		} hpd_mark;
 	} hpd_stats[HPD_NUM_PINS];
+	struct timer_list hotplug_reenable_timer;
 
 	int num_pch_pll;
 	int num_plane;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d059c5d..ae759ac 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -337,6 +337,8 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
+#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)
+
 static void i915_hotplug_work_func(struct work_struct *work)
 {
 	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
@@ -375,8 +377,11 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	 /* if there were no outputs to poll, poll was disabled,
 	  * therefore make sure it's enabled when disabling HPD on
 	  * some connectors */
-	if (hpd_disabled)
+	if (hpd_disabled) {
 		drm_kms_helper_poll_enable(dev);
+		mod_timer(&dev_priv->hotplug_reenable_timer,
+			  jiffies + msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY));
+	}
 
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
@@ -2356,6 +2361,8 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 	if (!dev_priv)
 		return;
 
+	del_timer_sync(&dev_priv->hotplug_reenable_timer);
+
 	for_each_pipe(pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
 
@@ -2377,6 +2384,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 	if (!dev_priv)
 		return;
 
+	del_timer_sync(&dev_priv->hotplug_reenable_timer);
+
 	I915_WRITE(HWSTAM, 0xffffffff);
 
 	I915_WRITE(DEIMR, 0xffffffff);
@@ -2753,6 +2762,8 @@ static void i915_irq_uninstall(struct drm_device * dev)
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	int pipe;
 
+	del_timer_sync(&dev_priv->hotplug_reenable_timer);
+
 	if (I915_HAS_HOTPLUG(dev)) {
 		I915_WRITE(PORT_HOTPLUG_EN, 0);
 		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
@@ -2997,6 +3008,8 @@ static void i965_irq_uninstall(struct drm_device * dev)
 	if (!dev_priv)
 		return;
 
+	del_timer_sync(&dev_priv->hotplug_reenable_timer);
+
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
@@ -3012,6 +3025,41 @@ static void i965_irq_uninstall(struct drm_device * dev)
 	I915_WRITE(IIR, I915_READ(IIR));
 }
 
+static void i915_reenable_hotplug_timer_func(unsigned long data)
+{
+	drm_i915_private_t *dev_priv = (drm_i915_private_t *)data;
+	struct drm_device *dev = dev_priv->dev;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	unsigned long irqflags;
+	int i;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+	for (i = (HPD_NONE + 1); i < HPD_NUM_PINS; i++) {
+		struct drm_connector *connector;
+
+		if (dev_priv->hpd_stats[i].hpd_mark != HPD_DISABLED)
+			continue;
+
+		dev_priv->hpd_stats[i].hpd_mark = HPD_ENABLED;
+
+		list_for_each_entry(connector, &mode_config->connector_list, head) {
+			struct intel_connector *intel_connector = to_intel_connector(connector);
+
+			if (intel_connector->encoder->hpd_pin == i) {
+				if (connector->polled != intel_connector->polled)
+					DRM_DEBUG_DRIVER("Reenabling HPD on connector %s\n",
+							 drm_get_connector_name(connector));
+				connector->polled = intel_connector->polled;
+				if (!connector->polled)
+					connector->polled = DRM_CONNECTOR_POLL_HPD;
+			}
+		}
+	}
+	if (dev_priv->display.hpd_irq_setup)
+		dev_priv->display.hpd_irq_setup(dev);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
+
 void intel_irq_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3024,6 +3072,8 @@ void intel_irq_init(struct drm_device *dev)
 	setup_timer(&dev_priv->gpu_error.hangcheck_timer,
 		    i915_hangcheck_elapsed,
 		    (unsigned long) dev);
+	setup_timer(&dev_priv->hotplug_reenable_timer, i915_reenable_hotplug_timer_func,
+		    (unsigned long) dev_priv);
 
 	pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
 
-- 
1.8.1.4

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

[PATCH 6/7] drm/i915: Add bit field to record which pins have received HPD events (v3)

[Egbert Eich]

This allows to add code which limits 're'-detect() of displays to connectors
that have received an HPD event.

v2: Reordered drm_i915_private: Move hpd_event_bits to hpd state tracking.
v3: Fixed merge conflicts with previous patches, removed some noisy debug
    logging as suggested by Jani Nikula.

Signed-off-by: Egbert Eich <eich@suse.de>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_irq.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 195b9fe..c0d9fb9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -938,6 +938,7 @@ typedef struct drm_i915_private {
 			HPD_MARK_DISABLED = 2
 		} hpd_mark;
 	} hpd_stats[HPD_NUM_PINS];
+	u32 hpd_event_bits;
 	struct timer_list hotplug_reenable_timer;
 
 	int num_pch_pll;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ae759ac..e108729 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -350,6 +350,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	struct drm_connector *connector;
 	unsigned long irqflags;
 	bool hpd_disabled = false;
+	u32 hpd_event_bits;
 
 	/* HPD irq before everything is fully set up. */
 	if (!dev_priv->enable_hotplug_processing)
@@ -359,6 +360,9 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	DRM_DEBUG_KMS("running encoder hotplug functions\n");
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+
+	hpd_event_bits = dev_priv->hpd_event_bits;
+	dev_priv->hpd_event_bits = 0;
 	list_for_each_entry(connector, &mode_config->connector_list, head) {
 		intel_connector = to_intel_connector(connector);
 		intel_encoder = intel_connector->encoder;
@@ -373,6 +377,10 @@ 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",
+				      drm_get_connector_name(connector), intel_encoder->hpd_pin);
+		}
 	}
 	 /* if there were no outputs to poll, poll was disabled,
 	  * therefore make sure it's enabled when disabling HPD on
@@ -636,6 +644,8 @@ static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
 		    dev_priv->hpd_stats[i].hpd_mark != HPD_ENABLED)
 			continue;
 
+		dev_priv->hpd_event_bits |= (1 << i);
+
 		if (!time_in_range(jiffies, dev_priv->hpd_stats[i].hpd_last_jiffies,
 				   dev_priv->hpd_stats[i].hpd_last_jiffies
 				   + msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
@@ -643,6 +653,7 @@ static inline bool hotplug_irq_storm_detect(struct drm_device *dev,
 			dev_priv->hpd_stats[i].hpd_cnt = 0;
 		} else if (dev_priv->hpd_stats[i].hpd_cnt > HPD_STORM_THRESHOLD) {
 			dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
+			dev_priv->hpd_event_bits &= ~(1 << i);
 			DRM_DEBUG_KMS("HPD interrupt storm detected on PIN %d\n", i);
 			ret = true;
 		} else {
-- 
1.8.1.4

[PATCH 7/7] drm/i915: Only reprobe display on encoder which has received an HPD event (v2)

[Egbert Eich]

Instead of calling into the DRM helper layer to poll all connectors for
changes in connected displays probe only those connectors which have
received a hotplug event.

Signed-off-by: Egbert Eich <eich@suse.de>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>

v2: Resolved conflicts with changes in previous commits.
    Renamed function and and added a WARN_ON() to warn of
    intel_hpd_irq_event() from being called without
    mode_config.mutex held - suggested by Jani Nikula.
---
 drivers/gpu/drm/i915/i915_irq.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e108729..dd579d7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -334,6 +334,21 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 						     crtc);
 }
 
+static int intel_hpd_irq_event(struct drm_device *dev, struct drm_connector *connector)
+{
+	enum drm_connector_status old_status;
+
+	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
+	old_status = connector->status;
+
+	connector->status = connector->funcs->detect(connector, false);
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n",
+		      connector->base.id,
+		      drm_get_connector_name(connector),
+		      old_status, connector->status);
+	return (old_status != connector->status);
+}
+
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
@@ -350,6 +365,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	struct drm_connector *connector;
 	unsigned long irqflags;
 	bool hpd_disabled = false;
+	bool changed = false;
 	u32 hpd_event_bits;
 
 	/* HPD irq before everything is fully set up. */
@@ -393,14 +409,20 @@ static void i915_hotplug_work_func(struct work_struct *work)
 
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
-	list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
-		if (intel_encoder->hot_plug)
-			intel_encoder->hot_plug(intel_encoder);
-
+	list_for_each_entry(connector, &mode_config->connector_list, head) {
+		intel_connector = to_intel_connector(connector);
+		intel_encoder = intel_connector->encoder;
+		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
+			if (intel_encoder->hot_plug)
+				intel_encoder->hot_plug(intel_encoder);
+			if (intel_hpd_irq_event(dev, connector))
+				changed = true;
+		}
+	}
 	mutex_unlock(&mode_config->mutex);
 
-	/* Just fire off a uevent and let userspace tell us what to do */
-	drm_helper_hpd_irq_event(dev);
+	if (changed)
+		drm_kms_helper_hotplug_event(dev);
 }
 
 static void ironlake_handle_rps_change(struct drm_device *dev)
-- 
1.8.1.4

Re: [PATCH 5/7] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v4)

[Daniel Vetter]

On Tue, Apr 16, 2013 at 01:36:58PM +0200, Egbert Eich wrote:
> We disable hoptplug detection when we encounter a hotplug event
> storm. Still hotplug detection is required on some outputs (like
> Display Port). The interrupt storm may be only temporary (on certain
> Dell Laptops for instance it happens at certain charging states of
> the system). Thus we enable it after a certain grace period (2 minutes).
> Should the interrupt storm persist it will be detected immediately
> and it will be disabled again.
> 
> v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state tracker.
> v3: Clarified loop start value,
>     Removed superfluous test for Ivybridge and Haswell,
>     Restructured loop to avoid deep nesting (all suggested by Ville Syrjälä)
> v4: Fixed two bugs pointed out by Jani Nikula.
> 
> Signed-off-by: Egbert Eich <eich@suse.de>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

I've queued up patches 1-5 of this series, thanks a lot for doing all
this. Wrt bikesheds: checkpatch seemed a bit unhappy about some of them,
but I've decided that I want this more for 3.10 than care about checkpatch
;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/7] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v4)

[Egbert Eich]

On Tue, Apr 16, 2013 at 08:07:09PM +0200, Daniel Vetter wrote:
> On Tue, Apr 16, 2013 at 01:36:58PM +0200, Egbert Eich wrote:
> > We disable hoptplug detection when we encounter a hotplug event
> > storm. Still hotplug detection is required on some outputs (like
> > Display Port). The interrupt storm may be only temporary (on certain
> > Dell Laptops for instance it happens at certain charging states of
> > the system). Thus we enable it after a certain grace period (2 minutes).
> > Should the interrupt storm persist it will be detected immediately
> > and it will be disabled again.
> > 
> > v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state tracker.
> > v3: Clarified loop start value,
> >     Removed superfluous test for Ivybridge and Haswell,
> >     Restructured loop to avoid deep nesting (all suggested by Ville Syrjälä)
> > v4: Fixed two bugs pointed out by Jani Nikula.
> > 
> > Signed-off-by: Egbert Eich <eich@suse.de>
> > Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> I've queued up patches 1-5 of this series, thanks a lot for doing all
> this. Wrt bikesheds: checkpatch seemed a bit unhappy about some of them,
> but I've decided that I want this more for 3.10 than care about checkpatch
> ;-)

I've run checkpatch on every patch - here it only complains about lines longer 
than 80 characters.
Now - if you can prove that my file introduced lines with more than 80 characters
to a file that had no such lines already we can talk about fixing this ;p

Cheers,
	Egbert.

Re: [PATCH 5/7] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v4)

[Daniel Vetter]

On Tue, Apr 16, 2013 at 10:22 PM, Egbert Eich <eich@freedesktop.org> wrote:
> On Tue, Apr 16, 2013 at 08:07:09PM +0200, Daniel Vetter wrote:
>> On Tue, Apr 16, 2013 at 01:36:58PM +0200, Egbert Eich wrote:
>> > We disable hoptplug detection when we encounter a hotplug event
>> > storm. Still hotplug detection is required on some outputs (like
>> > Display Port). The interrupt storm may be only temporary (on certain
>> > Dell Laptops for instance it happens at certain charging states of
>> > the system). Thus we enable it after a certain grace period (2 minutes).
>> > Should the interrupt storm persist it will be detected immediately
>> > and it will be disabled again.
>> >
>> > v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state tracker.
>> > v3: Clarified loop start value,
>> >     Removed superfluous test for Ivybridge and Haswell,
>> >     Restructured loop to avoid deep nesting (all suggested by Ville Syrjälä)
>> > v4: Fixed two bugs pointed out by Jani Nikula.
>> >
>> > Signed-off-by: Egbert Eich <eich@suse.de>
>> > Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>>
>> I've queued up patches 1-5 of this series, thanks a lot for doing all
>> this. Wrt bikesheds: checkpatch seemed a bit unhappy about some of them,
>> but I've decided that I want this more for 3.10 than care about checkpatch
>> ;-)
>
> I've run checkpatch on every patch - here it only complains about lines longer
> than 80 characters.
> Now - if you can prove that my file introduced lines with more than 80 characters
> to a file that had no such lines already we can talk about fixing this ;p

Yeah, the nesting levels in i915_irq.c are already pretty offensive,
hence why I didn't complain louder than with a little comment after
merging ;-) But maybe someone reading this is a bit bored and looks
for an easy refactor patch ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2] drm/i915: Only reprobe display on encoder which has received an HPD event (v2)

[Daniel Vetter]

On Thu, Apr 11, 2013 at 06:06:23PM +0300, Jani Nikula wrote:
> On Thu, 11 Apr 2013, Egbert Eich <eich@suse.de> wrote:
> > Instead of calling into the DRM helper layer to poll all connectors for
> > changes in connected displays probe only those connectors which have
> > received a hotplug event.
> >
> > Signed-off-by: Egbert Eich <eich@suse.de>
> > Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> Yup.

Merged both patches to dinq, picking the first variant (i.e. without #if
0) on the first one.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch