[PATCH] drm/i915: implement ibx_hpd_irq_setup

From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Cc: Egbert Eich <eich@suse.de>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Paulo Zanoni <paulo.r.zanoni@intel.com>
Subject: [PATCH] drm/i915: implement ibx_hpd_irq_setup
Date: Wed, 27 Mar 2013 15:55:01 +0100	[thread overview]
Message-ID: <1364396101-32056-1-git-send-email-daniel.vetter@ffwll.ch> (raw)
In-Reply-To: <20130327140316.GA4469@intel.com>

This fixes a regression introduced in

commit e5868a318d1ae28f760f77bb91ce5deb751733fd
Author: Egbert Eich <eich@suse.de>
Date:   Thu Feb 28 04:17:12 2013 -0500

    DRM/i915: Convert HPD interrupts to make use of HPD pin assignment in encode

Due to the irq setup rework in 3.9, see

commit 20afbda209d708be66944907966486d0c1331cb8
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Tue Dec 11 14:05:07 2012 +0100

    drm/i915: Fixup hpd irq register setup ordering

Egbert Eich's hpd rework blows up on pch-split platforms - it walks
the encoder list before that has been set up completely. The new init
sequence is:

1. irq enabling
2. modeset init
3. hpd setup

We need to move around the ibx setup a bit to fix this.

Ville Syrjälä pointed out in his review that we can't touch SDEIER
after the interrupt handler is set up, since that'll race with Paulo
Zanoni's PCH interrupt race fix:

commit 44498aea293b37af1d463acd9658cdce1ecdf427
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Fri Feb 22 17:05:28 2013 -0300

    drm/i915: also disable south interrupts when handling them

We fix that by unconditionally enabling all interrupts in SDEIER, but
masking them as-needed in SDEIMR. Since only the single-threaded
setup/teardown (or suspend/resume) code touches that, no further
locking is required.

While at it also simplify the mask handling - we start out with all
interrupts cleared in the postinstall hook, and never enable a hpd
interrupt before hpd_irq_setup is called.

And finally, for consistency rename the ibx hpd setup function to
ibx_hpd_irq_setup.

v2: Fix race around SDEIER writes (Ville).

v3: Remove the superflous posting read for SDEIER, spotted by Ville.

Ville also wondered whether we shouldn't clear SDEIIR, since now
SDE interrupts are enabled before we have an irq handler installed.
But the master interrupt control bit in DEIER is still cleared, so we
should be fine.

Cc: Egbert Eich <eich@suse.de>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 63 +++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 43436e0..666a0ec 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2028,7 +2028,13 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 
 	/* south display irq */
 	I915_WRITE(SDEIMR, 0xffffffff);
-	I915_WRITE(SDEIER, 0x0);
+	/*
+	 * SDEIER is also touched by the interrupt handler to work around missed
+	 * PCH interrupts. Hence we can't update it after the interrupt handler
+	 * is enabled - instead we unconditionally enable all PCH interrupt
+	 * sources here, but then only unmask them as needed with SDEIMR.
+	 */
+	I915_WRITE(SDEIER, 0xffffffff);
 	POSTING_READ(SDEIER);
 }
 
@@ -2064,18 +2070,30 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	POSTING_READ(VLV_IER);
 }
 
-/*
- * Enable digital hotplug on the PCH, and configure the DP short pulse
- * duration to 2ms (which is the minimum in the Display Port spec)
- *
- * This register is the same on all known PCH chips.
- */
-
-static void ibx_enable_hotplug(struct drm_device *dev)
+static void ibx_hpd_irq_setup(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
-	u32	hotplug;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	struct intel_encoder *intel_encoder;
+	u32 mask = ~I915_READ(SDEIMR);
+	u32 hotplug;
+
+	if (HAS_PCH_IBX(dev)) {
+		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
+			mask |= hpd_ibx[intel_encoder->hpd_pin];
+	} else {
+		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
+			mask |= hpd_cpt[intel_encoder->hpd_pin];
+	}
 
+	I915_WRITE(SDEIMR, ~mask);
+
+	/*
+	 * Enable digital hotplug on the PCH, and configure the DP short pulse
+	 * duration to 2ms (which is the minimum in the Display Port spec)
+	 *
+	 * This register is the same on all known PCH chips.
+	 */
 	hotplug = I915_READ(PCH_PORT_HOTPLUG);
 	hotplug &= ~(PORTD_PULSE_DURATION_MASK|PORTC_PULSE_DURATION_MASK|PORTB_PULSE_DURATION_MASK);
 	hotplug |= PORTD_HOTPLUG_ENABLE | PORTD_PULSE_DURATION_2ms;
@@ -2087,27 +2105,14 @@ static void ibx_enable_hotplug(struct drm_device *dev)
 static void ibx_irq_postinstall(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 *intel_encoder;
-	u32 mask = I915_READ(SDEIER);
+	u32 mask;
 
-	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];
-		mask |= SDE_GMBUS | SDE_AUX_MASK;
-	} 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];
-		mask |= SDE_GMBUS_CPT | SDE_AUX_MASK_CPT;
-	}
+	if (HAS_PCH_IBX(dev))
+		mask = SDE_GMBUS | SDE_AUX_MASK;
+	else
+		mask = SDE_GMBUS_CPT | SDE_AUX_MASK_CPT;
 	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
 	I915_WRITE(SDEIMR, ~mask);
-	I915_WRITE(SDEIER, mask);
-	POSTING_READ(SDEIER);
-
-	ibx_enable_hotplug(dev);
 }
 
 static int ironlake_irq_postinstall(struct drm_device *dev)
@@ -2960,6 +2965,7 @@ void intel_irq_init(struct drm_device *dev)
 		dev->driver->irq_uninstall = ironlake_irq_uninstall;
 		dev->driver->enable_vblank = ivybridge_enable_vblank;
 		dev->driver->disable_vblank = ivybridge_disable_vblank;
+		dev_priv->display.hpd_irq_setup = ibx_hpd_irq_setup;
 	} else if (HAS_PCH_SPLIT(dev)) {
 		dev->driver->irq_handler = ironlake_irq_handler;
 		dev->driver->irq_preinstall = ironlake_irq_preinstall;
@@ -2967,6 +2973,7 @@ void intel_irq_init(struct drm_device *dev)
 		dev->driver->irq_uninstall = ironlake_irq_uninstall;
 		dev->driver->enable_vblank = ironlake_enable_vblank;
 		dev->driver->disable_vblank = ironlake_disable_vblank;
+		dev_priv->display.hpd_irq_setup = ibx_hpd_irq_setup;
 	} else {
 		if (INTEL_INFO(dev)->gen == 2) {
 			dev->driver->irq_preinstall = i8xx_irq_preinstall;
-- 
1.7.11.7

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