drm/i915: don't read HSW_AUD_PIN_ELD_CP_VLD when the power well is off

From: Paulo Zanoni <[email protected]>

Because this will trigger "Unclaimed register" messages. All I need to reproduce this problem is to boot my HSW machine with eDP+HDMI connected.

Regression introduced by:

commit 9ed109a7b445e3f073d8ea72f888ec80c0532465

Author: Daniel Vetter <[email protected]>
Date:   Thu Apr 24 23:54:52 2014 +0200

drm/i915: Track has_audio in the pipe config

---
 drivers/gpu/drm/i915/intel_ddi.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

index 271ce19..355f569 100644
@@ -1560,9 +1560,11 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
 		break;
 	}
 
-	temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
-	if (temp & (AUDIO_OUTPUT_ENABLE_A << (intel_crtc->pipe * 4)))
-		pipe_config->has_audio = true;
+	if (intel_display_power_enabled(dev_priv, POWER_DOMAIN_AUDIO)) {
+		temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
+		if (temp & (AUDIO_OUTPUT_ENABLE_A << (intel_crtc->pipe * 4)))
+			pipe_config->has_audio = true;
+	}
 
 	if (encoder->type == INTEL_OUTPUT_EDP && dev_priv->vbt.edp_bpp &&
 	    pipe_config->pipe_bpp > dev_priv->vbt.edp_bpp) {

1.9.0

From: Paulo Zanoni <[email protected]>

Because this will trigger "Unclaimed register" messages. This can be reproduced by running the pm_rpm tests of the test suite. It can probably be reproduced by any of the tests that do modesets too.

Regression introduced by:

commit acfa75b02e72bad7c93564ac379712e29c001432

Author: Daniel Vetter <[email protected]>
Date:   Thu Apr 24 23:54:51 2014 +0200

drm/i915: Simplify audio handling on DDI ports

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

index 355f569..54a3bb7 100644
@@ -1372,10 +1372,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t tmp;
 
-	tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
-	tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) <<
-		 (pipe * 4));
-	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
+	if (intel_display_power_enabled(dev_priv, POWER_DOMAIN_AUDIO)) {
+		tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
+		tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) <<
+			 (pipe * 4));
+		I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
+	}
 
 	if (type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);

1.9.0

From: Paulo Zanoni <[email protected]>

With the current code, we unconditionally touch HSW_AUD_PIN_ELD_CP_VLD, which means we can touch it when the power well is off, and that will trigger an "Unclaimed register" message.

Just adding the intel_crtc->config.has_audio should already avoid the unclaimed register messsages, but since we actually need the power well to make the Audio code work, it makes sense to also grab the audio power domain reference, and release it when it's not needed anymore.

I used IGT's pm_rpm to reproduce this bug, but it can probably be reproduced on other tests that do modesets. I'm using a machine with eDP+HDMI connected.

Regression introduced by:

commit acfa75b02e72bad7c93564ac379712e29c001432

Author: Daniel Vetter <[email protected]>
Date:   Thu Apr 24 23:54:51 2014 +0200

drm/i915: Simplify audio handling on DDI ports

Credits to Daniel for suggesting this implementation.

---
 drivers/gpu/drm/i915/intel_ddi.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

index 355f569..b17b9c7 100644
@@ -1355,6 +1355,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
 	}
 
 	if (intel_crtc->config.has_audio) {
+		intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
 		tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
 		tmp |= ((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
 		I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);

@@ -1372,10 +1373,15 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t tmp;
 
-	tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
-	tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) <<
-		 (pipe * 4));
-	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
+	/* We can't touch HSW_AUD_PIN_ELD_CP_VLD uncionditionally because this
+	 * register is part of the power well on Haswell. */
+	if (intel_crtc->config.has_audio) {
+		tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
+		tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) <<
+			 (pipe * 4));
+		I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
+		intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
+	}
 
 	if (type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);

1.9.0

Queued for -next, thanks for the patch.
-Daniel

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

From: Paulo Zanoni <[email protected]>

The current code only runs when we do an I915_WRITE operation. It checks if the unclaimed register flag is set before we do the operation, and then it checks it again after we do the operation. This double check allows us to find out if the I915_WRITE operation in question is the bad one, or if some previous code is the bad one. When it finds a problem, our code uses DRM_ERROR to signal it.

The good thing about the current code is that it detects the problem, so at least we can know we did something wrong. The problem is that even though we find the problem, we don't really have much information to actually debug it. So whenever I see one of these DRM_ERROR messages on my systems, the first thing I do is apply a patch to change the DRM_ERROR to a WARN and also check for unclaimed registers on I915_READ operations. This local patch makes things even slower, but it usually helps a lot in finding the bad code.

The first point here is that since the current code is only useful to detect whether we have a problem or not, but it is not really good to find the cause of the problem, I don't think we should be checking both before and after every I915_WRITE operation: just doing the check once should be enough for us to quickly detect problems. With this change, the code that runs by default for every single user will only do 1 read operation for every single I915_WRITE, instead of 2. This patch does this change.

The second point is that the local patch I have should be upstream, but since it makes things slower it should be disabled by default. So I added the i915.mmio_debug option to enable it.

So after this patch, this is what will happen:
- By default, we will try to detect unclaimed registers once after every I915_WRITE operation. Previously we tried twice for every I915_WRITE.
- When we find an unclaimed register we will still print a DRM_ERROR message, but we will now tell the user to try again with i915.mmio_debug=1.
- When we use i915.mmio_debug=1 we will try to find unclaimed registers both before and after every I915_READ and I915_WRITE operation, and we will print stack traces in case we find them. This should really help locating the exact point of the bad code (or at least finding out that i915.ko is not the problem).

This commit also opens space for really-slow register debugging operations on other platforms. In theory we can now add lots and lots of debug code behind i915.mmio_debug, enable this option on our tests, and catch more problems.

---
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/i915/i915_params.c  |  6 ++++
 drivers/gpu/drm/i915/intel_uncore.c | 69 +++++++++++++++++++++++++++++++++----
 3 files changed, 69 insertions(+), 7 deletions(-)

index 8daf4f7..60cf78f 100644
@@ -2045,6 +2045,7 @@ struct i915_params {
 	bool reset;
 	bool disable_display;
 	bool disable_vtd_wa;
+	bool mmio_debug;
 };
 extern struct i915_params i915 __read_mostly;
 

index d05a2af..d9fa00f 100644
@@ -48,6 +48,7 @@ struct i915_params i915 __read_mostly = {
 	.disable_display = 0,
 	.enable_cmd_parser = 1,
 	.disable_vtd_wa = 0,
+	.mmio_debug = 0,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);

@@ -156,3 +157,8 @@ MODULE_PARM_DESC(disable_vtd_wa, "Disable all VT-d workarounds (default: false)"
 module_param_named(enable_cmd_parser, i915.enable_cmd_parser, int, 0600);
 MODULE_PARM_DESC(enable_cmd_parser,
 		 "Enable command parsing (1=enabled [default], 0=disabled)");
+
+module_param_named(mmio_debug, i915.mmio_debug, bool, 0600);
+MODULE_PARM_DESC(disable_vtd_wa,
+	"Enable the MMIO debug code (default: false). This may negatively "
+	"affect performance.");

1.9.0

Argh, I always forget about these. Thanks for tracking this down, patch merged.
-Daniel

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