[PATCH] drm/i915: only enable sdvo hotplug irq if needed

[PATCH] drm/i915: only enable sdvo hotplug irq if needed

[Jani Nikula]

Avoid constant wakeups caused by noisy irq lines when we don't even care
about the irq. This should be particularly useful for i945g/gm where the
hotplug has been disabled:

commit 768b107e4b3be0acf6f58e914afe4f337c00932b
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Fri May 4 11:29:56 2012 +0200

    drm/i915: disable sdvo hotplug on i945g/gm

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=38442
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

N.B. This really needs a Tested-by from the bug reporter.
---
 drivers/gpu/drm/i915/intel_sdvo.c |   32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index f3762f8..14379b5 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2529,7 +2529,6 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *intel_encoder;
 	struct intel_sdvo *intel_sdvo;
-	u32 hotplug_mask;
 	int i;
 
 	intel_sdvo = kzalloc(sizeof(struct intel_sdvo), GFP_KERNEL);
@@ -2561,19 +2560,6 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 		}
 	}
 
-	hotplug_mask = 0;
-	if (IS_G4X(dev)) {
-		hotplug_mask = intel_sdvo->is_sdvob ?
-			SDVOB_HOTPLUG_INT_STATUS_G4X : SDVOC_HOTPLUG_INT_STATUS_G4X;
-	} else if (IS_GEN4(dev)) {
-		hotplug_mask = intel_sdvo->is_sdvob ?
-			SDVOB_HOTPLUG_INT_STATUS_I965 : SDVOC_HOTPLUG_INT_STATUS_I965;
-	} else {
-		hotplug_mask = intel_sdvo->is_sdvob ?
-			SDVOB_HOTPLUG_INT_STATUS_I915 : SDVOC_HOTPLUG_INT_STATUS_I915;
-	}
-	dev_priv->hotplug_supported_mask |= hotplug_mask;
-
 	drm_encoder_helper_add(&intel_encoder->base, &intel_sdvo_helper_funcs);
 
 	/* In default case sdvo lvds is false */
@@ -2595,6 +2581,24 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 		goto err;
 	}
 
+	/* Only enable the hotplug irq if we need it, to work around noisy
+	 * hotplug lines.
+	 */
+	if (intel_sdvo->hotplug_active[0] & 0x3) {
+		u32 hotplug_mask;
+		if (IS_G4X(dev)) {
+			hotplug_mask = intel_sdvo->is_sdvob ?
+				SDVOB_HOTPLUG_INT_STATUS_G4X : SDVOC_HOTPLUG_INT_STATUS_G4X;
+		} else if (IS_GEN4(dev)) {
+			hotplug_mask = intel_sdvo->is_sdvob ?
+				SDVOB_HOTPLUG_INT_STATUS_I965 : SDVOC_HOTPLUG_INT_STATUS_I965;
+		} else {
+			hotplug_mask = intel_sdvo->is_sdvob ?
+				SDVOB_HOTPLUG_INT_STATUS_I915 : SDVOC_HOTPLUG_INT_STATUS_I915;
+		}
+		dev_priv->hotplug_supported_mask |= hotplug_mask;
+	}
+
 	intel_sdvo_select_ddc_bus(dev_priv, intel_sdvo, sdvo_reg);
 
 	/* Set the input timing to the screen. Assume always input 0. */
-- 
1.7.9.5

[PATCH v2] drm/i915: only enable sdvo hotplug irq if needed

[Jani Nikula]

Avoid constant wakeups caused by noisy irq lines when we don't even care
about the irq. This should be particularly useful for i945g/gm where the
hotplug has been disabled:

commit 768b107e4b3be0acf6f58e914afe4f337c00932b
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Fri May 4 11:29:56 2012 +0200

    drm/i915: disable sdvo hotplug on i945g/gm

v2: While at it, remove the bogus hotplug_active read, and do not mask
hotplug_active[0] before checking whether the irq is needed, per discussion
with Daniel on IRC.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=38442
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

N.B. This really needs a Tested-by from the bug reporter.
---
 drivers/gpu/drm/i915/intel_sdvo.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index d81bb0b..123afd3 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2573,7 +2573,6 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 		hotplug_mask = intel_sdvo->is_sdvob ?
 			SDVOB_HOTPLUG_INT_STATUS_I915 : SDVOC_HOTPLUG_INT_STATUS_I915;
 	}
-	dev_priv->hotplug_supported_mask |= hotplug_mask;
 
 	drm_encoder_helper_add(&intel_encoder->base, &intel_sdvo_helper_funcs);
 
@@ -2581,14 +2580,6 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 	if (!intel_sdvo_get_capabilities(intel_sdvo, &intel_sdvo->caps))
 		goto err;
 
-	/* Set up hotplug command - note paranoia about contents of reply.
-	 * We assume that the hardware is in a sane state, and only touch
-	 * the bits we think we understand.
-	 */
-	intel_sdvo_get_value(intel_sdvo, SDVO_CMD_GET_ACTIVE_HOT_PLUG,
-			     &intel_sdvo->hotplug_active, 2);
-	intel_sdvo->hotplug_active[0] &= ~0x3;
-
 	if (intel_sdvo_output_setup(intel_sdvo,
 				    intel_sdvo->caps.output_flags) != true) {
 		DRM_DEBUG_KMS("SDVO output failed to setup on %s\n",
@@ -2596,6 +2587,12 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 		goto err;
 	}
 
+	/* Only enable the hotplug irq if we need it, to work around noisy
+	 * hotplug lines.
+	 */
+	if (intel_sdvo->hotplug_active[0])
+		dev_priv->hotplug_supported_mask |= hotplug_mask;
+
 	intel_sdvo_select_ddc_bus(dev_priv, intel_sdvo, sdvo_reg);
 
 	/* Set the input timing to the screen. Assume always input 0. */
-- 
1.7.9.5

[PATCH] drm/i915: fix sdvo hotplug support check and activation

[Jani Nikula]

The sdvo hotplug support check and activation has worked by coincidence for
TMDS0. The boolean value returned by intel_sdvo_supports_hotplug() was
masked with a bit shifted by device number, which also should have been one
of SDVO_OUTPUT_* bits instead. Boolean true masked with 1 shifted by 0 just
happened to match SDVO_OUTPUT_TMDS0...

Get hotplug support as a bit mask, check the correct bits for support, and
use the correct bits for activating hotplug support.

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

---

In reply to the other patch as this depends on it.
---
 drivers/gpu/drm/i915/intel_sdvo.c |   29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 80552de..6b4f43f 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -97,7 +97,7 @@ struct intel_sdvo {
 	/*
 	 * Hotplug activation bits for this device
 	 */
-	uint8_t hotplug_active[2];
+	uint16_t hotplug_active;
 
 	/**
 	 * This is used to select the color range of RBG outputs in HDMI mode.
@@ -1251,25 +1251,29 @@ static bool intel_sdvo_get_capabilities(struct intel_sdvo *intel_sdvo, struct in
 	return true;
 }
 
-static int intel_sdvo_supports_hotplug(struct intel_sdvo *intel_sdvo)
+static uint16_t intel_sdvo_get_hotplug_support(struct intel_sdvo *intel_sdvo)
 {
 	struct drm_device *dev = intel_sdvo->base.base.dev;
-	u8 response[2];
+	uint16_t hotplug;
 
 	/* HW Erratum: SDVO Hotplug is broken on all i945G chips, there's noise
 	 * on the line. */
 	if (IS_I945G(dev) || IS_I945GM(dev))
-		return false;
+		return 0;
+
+	if (!intel_sdvo_get_value(intel_sdvo, SDVO_CMD_GET_HOT_PLUG_SUPPORT,
+					&hotplug, sizeof(hotplug)))
+		return 0;
 
-	return intel_sdvo_get_value(intel_sdvo, SDVO_CMD_GET_HOT_PLUG_SUPPORT,
-				    &response, 2) && response[0];
+	return hotplug;
 }
 
 static void intel_sdvo_enable_hotplug(struct intel_encoder *encoder)
 {
 	struct intel_sdvo *intel_sdvo = to_intel_sdvo(&encoder->base);
 
-	intel_sdvo_write_cmd(intel_sdvo, SDVO_CMD_SET_ACTIVE_HOT_PLUG, &intel_sdvo->hotplug_active, 2);
+	intel_sdvo_write_cmd(intel_sdvo, SDVO_CMD_SET_ACTIVE_HOT_PLUG,
+			&intel_sdvo->hotplug_active, 2);
 }
 
 static bool
@@ -2063,17 +2067,18 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 
 	intel_connector = &intel_sdvo_connector->base;
 	connector = &intel_connector->base;
-	if (intel_sdvo_supports_hotplug(intel_sdvo) & (1 << device)) {
+	if (intel_sdvo_get_hotplug_support(intel_sdvo) &
+		intel_sdvo_connector->output_flag) {
 		connector->polled = DRM_CONNECTOR_POLL_HPD;
-		intel_sdvo->hotplug_active[0] |= 1 << device;
+		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.
 		 */
 		intel_encoder->hot_plug = intel_sdvo_enable_hotplug;
 		intel_sdvo_enable_hotplug(intel_encoder);
-	}
-	else
+	} else {
 		connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
+	}
 	encoder->encoder_type = DRM_MODE_ENCODER_TMDS;
 	connector->connector_type = DRM_MODE_CONNECTOR_DVID;
 
@@ -2589,7 +2594,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 	/* Only enable the hotplug irq if we need it, to work around noisy
 	 * hotplug lines.
 	 */
-	if (intel_sdvo->hotplug_active[0])
+	if (intel_sdvo->hotplug_active)
 		dev_priv->hotplug_supported_mask |= hotplug_mask;
 
 	intel_sdvo_select_ddc_bus(dev_priv, intel_sdvo, sdvo_reg);
-- 
1.7.9.5

Re: [PATCH] drm/i915: fix sdvo hotplug support check and activation

[Simon Farnsworth]

[-- Attachment #1.1: Type: text/plain, Size: 986 bytes --]

(apologies to Daniel and Jani, who've seen this twice - I accidentally
sent it from the wrong address originally).

On Wednesday 29 August 2012 16:43:58 Jani Nikula wrote:
> The sdvo hotplug support check and activation has worked by coincidence for
> TMDS0. The boolean value returned by intel_sdvo_supports_hotplug() was
> masked with a bit shifted by device number, which also should have been one
> of SDVO_OUTPUT_* bits instead. Boolean true masked with 1 shifted by 0 just
> happened to match SDVO_OUTPUT_TMDS0...
> 
> Get hotplug support as a bit mask, check the correct bits for support, and
> use the correct bits for activating hotplug support.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
Reviewed-by: Simon Farnsworth <simon.farnsworth@onelan.com>

The code you're fixing was written with the aid of ajax's educated guesses - 
it's nice to see an Intel employee fix it to match the spec.
-- 
Simon Farnsworth
Software Engineer
ONELAN Ltd
http://www.onelan.com

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: fix sdvo hotplug support check and activation

[Daniel Vetter]

On Wed, Aug 29, 2012 at 03:12:37PM +0100, Simon Farnsworth wrote:
> On Wednesday 29 August 2012 16:43:58 Jani Nikula wrote:
> > The sdvo hotplug support check and activation has worked by coincidence for
> > TMDS0. The boolean value returned by intel_sdvo_supports_hotplug() was
> > masked with a bit shifted by device number, which also should have been one
> > of SDVO_OUTPUT_* bits instead. Boolean true masked with 1 shifted by 0 just
> > happened to match SDVO_OUTPUT_TMDS0...
> > 
> > Get hotplug support as a bit mask, check the correct bits for support, and
> > use the correct bits for activating hotplug support.
> > 
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > 
> Reviewed-by: Simon Farnsworth <simon.farnsworth@onelan.com>
Queued for -next (together with a duplicate of the bugfix to avoid merge
hassles), thanks for the patch.
-Daniel
> 
> The code you're fixing was written with the aid of ajax's educated guesses - 
> it's nice to see an Intel employee fix it to match the spec.
> -- 
> Simon Farnsworth
> Software Engineer
> ONELAN Ltd
> http://www.onelan.com

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH v2] drm/i915: only enable sdvo hotplug irq if needed

[Daniel Vetter]

On Wed, Aug 29, 2012 at 02:08:42PM +0300, Jani Nikula wrote:
> Avoid constant wakeups caused by noisy irq lines when we don't even care
> about the irq. This should be particularly useful for i945g/gm where the
> hotplug has been disabled:
> 
> commit 768b107e4b3be0acf6f58e914afe4f337c00932b
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Fri May 4 11:29:56 2012 +0200
> 
>     drm/i915: disable sdvo hotplug on i945g/gm
> 
> v2: While at it, remove the bogus hotplug_active read, and do not mask
> hotplug_active[0] before checking whether the irq is needed, per discussion
> with Daniel on IRC.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=38442
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> N.B. This really needs a Tested-by from the bug reporter.
Picked up for -fixes (with the tested-by + cc: stable added), thanks for
the patch.
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_sdvo.c |   15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index d81bb0b..123afd3 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -2573,7 +2573,6 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
>  		hotplug_mask = intel_sdvo->is_sdvob ?
>  			SDVOB_HOTPLUG_INT_STATUS_I915 : SDVOC_HOTPLUG_INT_STATUS_I915;
>  	}
> -	dev_priv->hotplug_supported_mask |= hotplug_mask;
>  
>  	drm_encoder_helper_add(&intel_encoder->base, &intel_sdvo_helper_funcs);
>  
> @@ -2581,14 +2580,6 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
>  	if (!intel_sdvo_get_capabilities(intel_sdvo, &intel_sdvo->caps))
>  		goto err;
>  
> -	/* Set up hotplug command - note paranoia about contents of reply.
> -	 * We assume that the hardware is in a sane state, and only touch
> -	 * the bits we think we understand.
> -	 */
> -	intel_sdvo_get_value(intel_sdvo, SDVO_CMD_GET_ACTIVE_HOT_PLUG,
> -			     &intel_sdvo->hotplug_active, 2);
> -	intel_sdvo->hotplug_active[0] &= ~0x3;
> -
>  	if (intel_sdvo_output_setup(intel_sdvo,
>  				    intel_sdvo->caps.output_flags) != true) {
>  		DRM_DEBUG_KMS("SDVO output failed to setup on %s\n",
> @@ -2596,6 +2587,12 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
>  		goto err;
>  	}
>  
> +	/* Only enable the hotplug irq if we need it, to work around noisy
> +	 * hotplug lines.
> +	 */
> +	if (intel_sdvo->hotplug_active[0])
> +		dev_priv->hotplug_supported_mask |= hotplug_mask;
> +
>  	intel_sdvo_select_ddc_bus(dev_priv, intel_sdvo, sdvo_reg);
>  
>  	/* Set the input timing to the screen. Assume always input 0. */
> -- 
> 1.7.9.5
> 

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48