[Intel-gfx] [PATCH 2/3] drm/i915: grab the audio power domain when enabling audio on HSW+

2014-05-21 Thread Paulo Zanoni 
From: Paulo Zanoni paulo.r.zan...@intel.com

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 daniel.vet...@ffwll.ch
Date:   Thu Apr 24 23:54:51 2014 +0200
drm/i915: Simplify audio handling on DDI ports

Credits to Daniel for suggesting this implementation.

Cc: Daniel Vetter daniel.vet...@ffwll.ch
Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
---
 drivers/gpu/drm/i915/intel_ddi.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 355f569..b17b9c7 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -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

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

Re: [Intel-gfx] [PATCH 2/3] drm/i915: grab the audio power domain when enabling audio on HSW+

2014-05-21 Thread Daniel Vetter 
On Wed, May 21, 2014 at 05:29:31PM -0300, Paulo Zanoni wrote:
 From: Paulo Zanoni paulo.r.zan...@intel.com
 
 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 daniel.vet...@ffwll.ch
 Date:   Thu Apr 24 23:54:51 2014 +0200
 drm/i915: Simplify audio handling on DDI ports
 
 Credits to Daniel for suggesting this implementation.
 
 Cc: Daniel Vetter daniel.vet...@ffwll.ch
 Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com

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

 ---
  drivers/gpu/drm/i915/intel_ddi.c | 14 ++
  1 file changed, 10 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
 b/drivers/gpu/drm/i915/intel_ddi.c
 index 355f569..b17b9c7 100644
 --- a/drivers/gpu/drm/i915/intel_ddi.c
 +++ b/drivers/gpu/drm/i915/intel_ddi.c
 @@ -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
 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx