[PATCH] drm/i915: implement ibx_hpd_irq_setup

[PATCH] drm/i915: implement ibx_hpd_irq_setup

[Daniel Vetter]

Due to the irq setup rework in 3.9 Egbert's hpd rework blows up on
pch-split platforms. The new init sequence is:

- irq enabling
- modeset init
- hpd setup

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

This needs to be squashed into a commit on dinq.

Cc: Egbert Eich <eich@suse.de>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 43436e0..1279a44 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2084,7 +2084,7 @@ static void ibx_enable_hotplug(struct drm_device *dev)
 	I915_WRITE(PCH_PORT_HOTPLUG, hotplug);
 }
 
-static void ibx_irq_postinstall(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;
 	struct drm_mode_config *mode_config = &dev->mode_config;
@@ -2095,12 +2095,10 @@ static void ibx_irq_postinstall(struct drm_device *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;
 	}
 	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
 	I915_WRITE(SDEIMR, ~mask);
@@ -2110,6 +2108,21 @@ static void ibx_irq_postinstall(struct drm_device *dev)
 	ibx_enable_hotplug(dev);
 }
 
+static void ibx_irq_postinstall(struct drm_device *dev)
+{
+	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+	u32 mask = I915_READ(SDEIER);
+
+	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);
+}
+
 static int ironlake_irq_postinstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
@@ -2960,6 +2973,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 +2981,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

Re: [PATCH] drm/i915: implement ibx_hpd_irq_setup

[Ville Syrjälä]

On Wed, Mar 27, 2013 at 12:44:30AM +0100, Daniel Vetter wrote:
> Due to the irq setup rework in 3.9 Egbert's hpd rework blows up on
> pch-split platforms. The new init sequence is:
> 
> - irq enabling
> - modeset init
> - hpd setup
> 
> We need to move around the ibx setup a bit to fix this.
> 
> This needs to be squashed into a commit on dinq.
> 
> Cc: Egbert Eich <eich@suse.de>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 43436e0..1279a44 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2084,7 +2084,7 @@ static void ibx_enable_hotplug(struct drm_device *dev)
>  	I915_WRITE(PCH_PORT_HOTPLUG, hotplug);
>  }
>  
> -static void ibx_irq_postinstall(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;
>  	struct drm_mode_config *mode_config = &dev->mode_config;
> @@ -2095,12 +2095,10 @@ static void ibx_irq_postinstall(struct drm_device *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;
>  	}
>  	I915_WRITE(SDEIIR, I915_READ(SDEIIR));


>  	I915_WRITE(SDEIMR, ~mask);
> @@ -2110,6 +2108,21 @@ static void ibx_irq_postinstall(struct drm_device *dev)
>  	ibx_enable_hotplug(dev);
>  }
>  
> +static void ibx_irq_postinstall(struct drm_device *dev)
> +{
> +	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> +	u32 mask = I915_READ(SDEIER);
> +
> +	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);
> +}

Should we clear just the HPD bits here? Then again, I suppose
enable_hotplug_processing should make sure we don't do aux/gmbus
transfers at the same time, so maybe it doesn't matter.

But now I started to wondee what are chances that we'd get
some other interrupt while executing this function. That
could lead to a corrupted SDEIER now that the irq handler
touches it as well.

> +
>  static int ironlake_irq_postinstall(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> @@ -2960,6 +2973,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 +2981,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

-- 
Ville Syrjälä
Intel OTC

[PATCH] drm/i915: clear crt hotplug compare voltage field before setting

[Daniel Vetter]

Noticed while reviewing the hotplug irq setup code. Just looks better.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index c697580..be2d6dd 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2776,6 +2776,7 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
 		*/
 		if (IS_G4X(dev))
 			hotplug_en |= CRT_HOTPLUG_ACTIVATION_PERIOD_64;
+		hotplug_en &= ~CRT_HOTPLUG_VOLTAGE_COMPARE_MASK;
 		hotplug_en |= CRT_HOTPLUG_VOLTAGE_COMPARE_50;
 
 		/* Ignore TV since it's buggy */
-- 
1.7.11.7

[PATCH] drm/i915: implement ibx_hpd_irq_setup

[Daniel Vetter]

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

Re: [PATCH] drm/i915: clear crt hotplug compare voltage field before setting

[Ville Syrjälä]

On Wed, Mar 27, 2013 at 03:47:11PM +0100, Daniel Vetter wrote:
> Noticed while reviewing the hotplug irq setup code. Just looks better.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

What about the activation period? Do we want to leave it alone on non
g4x platforms, or should it also get cleared?

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index c697580..be2d6dd 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2776,6 +2776,7 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
>  		*/
>  		if (IS_G4X(dev))
>  			hotplug_en |= CRT_HOTPLUG_ACTIVATION_PERIOD_64;
> +		hotplug_en &= ~CRT_HOTPLUG_VOLTAGE_COMPARE_MASK;
>  		hotplug_en |= CRT_HOTPLUG_VOLTAGE_COMPARE_50;
>  
>  		/* Ignore TV since it's buggy */
> -- 
> 1.7.11.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: implement ibx_hpd_irq_setup

[Ville Syrjälä]

On Wed, Mar 27, 2013 at 03:55:01PM +0100, Daniel Vetter wrote:
> 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>

Makes sense to me.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

<snip>

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: clear crt hotplug compare voltage field before setting

[Daniel Vetter]

On Wed, Mar 27, 2013 at 05:02:43PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 27, 2013 at 03:47:11PM +0100, Daniel Vetter wrote:
> > Noticed while reviewing the hotplug irq setup code. Just looks better.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Queued for -next, thanks for the review.

> What about the activation period? Do we want to leave it alone on non
> g4x platforms, or should it also get cleared?

The activation period is just one bit, so setting it will dtrt. And there
are other values to tune the crt hotplug detection, so I've figured I'll
leave things as-is otherwise. The lack of proper masking just annoyed my
OCD a bit ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: implement ibx_hpd_irq_setup

[Daniel Vetter]

On Wed, Mar 27, 2013 at 05:05:54PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 27, 2013 at 03:55:01PM +0100, Daniel Vetter wrote:
> > 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>
> 
> Makes sense to me.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Queued for -next, thanks for the patch. I've just pushed down the patch a
bit to keep the bisect regression window small, but not squashed - keeping
a record of our discussion seemed beneficial.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: implement ibx_hpd_irq_setup

[Egbert Eich]

Sorry for replying so late, I wasn't able to task switch my brain
towards this when it was discussed:

Daniel Vetter writes:
 > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
 > index 43436e0..1279a44 100644
 > --- a/drivers/gpu/drm/i915/i915_irq.c
 > +++ b/drivers/gpu/drm/i915/i915_irq.c
 > @@ -2084,7 +2084,7 @@ static void ibx_enable_hotplug(struct drm_device *dev)
 >  	I915_WRITE(PCH_PORT_HOTPLUG, hotplug);
 >  }
 >  
 > -static void ibx_irq_postinstall(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;
 >  	struct drm_mode_config *mode_config = &dev->mode_config;
 > @@ -2095,12 +2095,10 @@ static void ibx_irq_postinstall(struct drm_device *dev)
 >  		mask &= ~SDE_HOTPLUG_MASK;
                ^^^^^^^^^^^^^^^^^^^^^^^
I'm missing those lines in the committed version of the patch.

 >  		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;
 >  	}
 >  	I915_WRITE(SDEIIR, I915_READ(SDEIIR));

These are not really relevant in the present code, however they are
important once I've got the hotplug stuff refitted as one needs to be
able to turn off individual interrupts.
I'm going to prepare a commit for this and will send it with the hpd 
irq storm patches.

Cheers,
	Egbert.

Re: [PATCH] drm/i915: implement ibx_hpd_irq_setup

[Daniel Vetter]

On Fri, Mar 29, 2013 at 5:35 PM, Egbert Eich <eich@suse.com> wrote:
> Daniel Vetter writes:
>  > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>  > index 43436e0..1279a44 100644
>  > --- a/drivers/gpu/drm/i915/i915_irq.c
>  > +++ b/drivers/gpu/drm/i915/i915_irq.c
>  > @@ -2084,7 +2084,7 @@ static void ibx_enable_hotplug(struct drm_device *dev)
>  >      I915_WRITE(PCH_PORT_HOTPLUG, hotplug);
>  >  }
>  >
>  > -static void ibx_irq_postinstall(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;
>  >      struct drm_mode_config *mode_config = &dev->mode_config;
>  > @@ -2095,12 +2095,10 @@ static void ibx_irq_postinstall(struct drm_device *dev)
>  >              mask &= ~SDE_HOTPLUG_MASK;
>                 ^^^^^^^^^^^^^^^^^^^^^^^
> I'm missing those lines in the committed version of the patch.
>
>  >              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;
>  >      }
>  >      I915_WRITE(SDEIIR, I915_READ(SDEIIR));
>
> These are not really relevant in the present code, however they are
> important once I've got the hotplug stuff refitted as one needs to be
> able to turn off individual interrupts.
> I'm going to prepare a commit for this and will send it with the hpd
> irq storm patches.

Yeah, makes sense now that I think about it - I've simply didn't look
ahead in your patch series while writing this little fixup ;-) Can you
just re-add this when resending your patches again please?

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

Re: [PATCH] drm/i915: implement ibx_hpd_irq_setup

[Egbert Eich]

Hi Daniel,

Daniel Vetter writes:
 > On Fri, Mar 29, 2013 at 5:35 PM, Egbert Eich <eich@suse.com> wrote:
 > 
 > Yeah, makes sense now that I think about it - I've simply didn't look
 > ahead in your patch series while writing this little fixup ;-) Can you
 > just re-add this when resending your patches again please?
 > 
Sure, I have prepared all the patches. I just wanted to give them
a try before sending them. 
Unfortunately I did not get around to do so over the Easter holidays.

Cheers,
	Egbert.