[PATCH] drm/i915: Acquire audio powerwell for HD-Audio registers

[PATCH] drm/i915: Acquire audio powerwell for HD-Audio registers

[Chris Wilson]

On Haswell/Broadwell, the HD-Audio block is inside the HDMI/display
power well and so the sna-hda audio codec acquires the display power
well while it is operational. However, Skylake separates the powerwells
again, but yet we still need the audio powerwell to setup the registers.
(But then the hardware uses those registers even while powered off???)

v2: Grab both rpm wakelock and audio wakelock

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96214
Fixes: 03b135cebc47 "ALSA: hda - remove dependency on i915 power well for SKL")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Libin Yang <libin.yang@intel.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Marius Vlad <marius.c.vlad@intel.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_audio.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 5d5f6bc10e85..948a7a52e3f8 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -600,6 +600,8 @@ static void i915_audio_component_codec_wake_override(struct device *dev,
 	if (!IS_SKYLAKE(dev_priv) && !IS_KABYLAKE(dev_priv))
 		return;
 
+	i915_audio_component_get_power(dev);
+
 	/*
 	 * Enable/disable generating the codec wake signal, overriding the
 	 * internal logic to generate the codec wake to controller.
@@ -615,6 +617,8 @@ static void i915_audio_component_codec_wake_override(struct device *dev,
 		I915_WRITE(HSW_AUD_CHICKENBIT, tmp);
 		usleep_range(1000, 1500);
 	}
+
+	i915_audio_component_put_power(dev);
 }
 
 /* Get CDCLK in kHz  */
@@ -648,6 +652,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
 	    !IS_HASWELL(dev_priv))
 		return 0;
 
+	i915_audio_component_get_power(dev);
 	mutex_lock(&dev_priv->av_mutex);
 	/* 1. get the pipe */
 	intel_encoder = dev_priv->dig_port_map[port];
@@ -698,6 +703,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
 
  unlock:
 	mutex_unlock(&dev_priv->av_mutex);
+	i915_audio_component_put_power(dev);
 	return err;
 }
 
-- 
2.8.1

Re: [Intel-gfx] [PATCH] drm/i915: Acquire audio powerwell for HD-Audio registers

[Daniel Vetter]

On Wed, Aug 03, 2016 at 05:09:00PM +0100, Chris Wilson wrote:
> On Haswell/Broadwell, the HD-Audio block is inside the HDMI/display
> power well and so the sna-hda audio codec acquires the display power
> well while it is operational. However, Skylake separates the powerwells
> again, but yet we still need the audio powerwell to setup the registers.
> (But then the hardware uses those registers even while powered off???)

Yeah feels fishy, but will at least duct-tape over the breakage from the
audio side. Most likely the reg writes go exactly nowhere and there's a
bug on the audio side. And this patch doesn't fix that.

But it does what it says on the tin, and it gets the job done.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> v2: Grab both rpm wakelock and audio wakelock
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96214
> Fixes: 03b135cebc47 "ALSA: hda - remove dependency on i915 power well for SKL")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Libin Yang <libin.yang@intel.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Marius Vlad <marius.c.vlad@intel.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 5d5f6bc10e85..948a7a52e3f8 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -600,6 +600,8 @@ static void i915_audio_component_codec_wake_override(struct device *dev,
>  	if (!IS_SKYLAKE(dev_priv) && !IS_KABYLAKE(dev_priv))
>  		return;
>  
> +	i915_audio_component_get_power(dev);
> +
>  	/*
>  	 * Enable/disable generating the codec wake signal, overriding the
>  	 * internal logic to generate the codec wake to controller.
> @@ -615,6 +617,8 @@ static void i915_audio_component_codec_wake_override(struct device *dev,
>  		I915_WRITE(HSW_AUD_CHICKENBIT, tmp);
>  		usleep_range(1000, 1500);
>  	}
> +
> +	i915_audio_component_put_power(dev);
>  }
>  
>  /* Get CDCLK in kHz  */
> @@ -648,6 +652,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>  	    !IS_HASWELL(dev_priv))
>  		return 0;
>  
> +	i915_audio_component_get_power(dev);
>  	mutex_lock(&dev_priv->av_mutex);
>  	/* 1. get the pipe */
>  	intel_encoder = dev_priv->dig_port_map[port];
> @@ -698,6 +703,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>  
>   unlock:
>  	mutex_unlock(&dev_priv->av_mutex);
> +	i915_audio_component_put_power(dev);
>  	return err;
>  }
>  
> -- 
> 2.8.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm/i915: Acquire audio powerwell for HD-Audio registers

[Takashi Iwai]

On Thu, 04 Aug 2016 18:44:11 +0200,
Daniel Vetter wrote:
> 
> On Wed, Aug 03, 2016 at 05:09:00PM +0100, Chris Wilson wrote:
> > On Haswell/Broadwell, the HD-Audio block is inside the HDMI/display
> > power well and so the sna-hda audio codec acquires the display power
> > well while it is operational. However, Skylake separates the powerwells
> > again, but yet we still need the audio powerwell to setup the registers.
> > (But then the hardware uses those registers even while powered off???)
> 
> Yeah feels fishy, but will at least duct-tape over the breakage from the
> audio side. Most likely the reg writes go exactly nowhere and there's a
> bug on the audio side. And this patch doesn't fix that.

Well, if the relevant code paths are only over these callbacks, I
guess the following fix would work instead.

Can anyone check?


Takashi

---
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 89dacf9b4e6c..88ad391452ae 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1021,8 +1021,8 @@ static int azx_runtime_resume(struct device *dev)
 			snd_hdac_i915_set_bclk(bus);
 		} else {
 			/* toggle codec wakeup bit for STATESTS read */
-			snd_hdac_set_codec_wakeup(bus, true);
-			snd_hdac_set_codec_wakeup(bus, false);
+			snd_hdac_display_power(bus, true);
+			snd_hdac_display_power(bus, false);
 		}
 	}