diff mbox

ALSA: hda - add AZX_DCAPS_I915_POWERWELL to Baytrail

Message ID e8074c2236ff5b224dbc8a92cca40f348ca5032c.1429592999.git.mengdong.lin@intel.com (mailing list archive)
State Accepted
Commit 40cc2392f4b144197d05eec73c1560f42fc25def
Headers show

Commit Message

Lin, Mengdong April 21, 2015, 5:12 a.m. UTC 
From: Mengdong Lin <mengdong.lin@intel.com>

This patch addes AZX_DCAPS_I915_POWERWELL to BYT (Baytrail).

Like Braswell and Skylake, the HDMI codec on Bytrail is also in the shared
power well with GPU. This power well must be turned on before we reset link
to probe the codec, to avoid communication failure with the codec.

The side effect is that this power is always ON in S0 because the BYT HDMI
codec does not support EPSS or D3ClkStop and so the controller doesn't enter
D3 at runtime, and the HDMI codec and analog codec share a single physical
HD-A link and so we cannot reset the HD-A link freely when we re-enable the
power to use the HDMI codec.

Next step is to test if an AGP reset or double AGP reset on BYT HDMI codec is
okay to bring the HDMI codec back to a functional state after restoring the
power. If okay, we can bind the power on/off with the HDMI codec PM without
interrupting the analog audio.

Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

Comments

Takashi Iwai April 21, 2015, 6 a.m. UTC | #1 
At Tue, 21 Apr 2015 13:12:23 +0800,
mengdong.lin@intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin@intel.com>
> 
> This patch addes AZX_DCAPS_I915_POWERWELL to BYT (Baytrail).
> 
> Like Braswell and Skylake, the HDMI codec on Bytrail is also in the shared
> power well with GPU. This power well must be turned on before we reset link
> to probe the codec, to avoid communication failure with the codec.
> 
> The side effect is that this power is always ON in S0 because the BYT HDMI
> codec does not support EPSS or D3ClkStop and so the controller doesn't enter
> D3 at runtime, and the HDMI codec and analog codec share a single physical
> HD-A link and so we cannot reset the HD-A link freely when we re-enable the
> power to use the HDMI codec.
> 
> Next step is to test if an AGP reset or double AGP reset on BYT HDMI codec is
> okay to bring the HDMI codec back to a functional state after restoring the
> power. If okay, we can bind the power on/off with the HDMI codec PM without
> interrupting the analog audio.
> 
> Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

Applied, thanks.


Takashi

> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index e1c2105..34040d2 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -297,6 +297,9 @@ enum {
>  	 AZX_DCAPS_PM_RUNTIME | AZX_DCAPS_I915_POWERWELL |\
>  	 AZX_DCAPS_SNOOP_TYPE(SCH))
>  
> +#define AZX_DCAPS_INTEL_BAYTRAIL \
> +	(AZX_DCAPS_INTEL_PCH_NOPM | AZX_DCAPS_I915_POWERWELL)
> +
>  #define AZX_DCAPS_INTEL_BRASWELL \
>  	(AZX_DCAPS_INTEL_PCH | AZX_DCAPS_I915_POWERWELL)
>  
> @@ -1992,7 +1995,7 @@ static const struct pci_device_id azx_ids[] = {
>  	  .driver_data = AZX_DRIVER_SCH | AZX_DCAPS_INTEL_PCH_NOPM },
>  	/* BayTrail */
>  	{ PCI_DEVICE(0x8086, 0x0f04),
> -	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH_NOPM },
> +	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_BAYTRAIL },
>  	/* Braswell */
>  	{ PCI_DEVICE(0x8086, 0x2284),
>  	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_BRASWELL },
> -- 
> 1.9.1
>
David Henningsson April 21, 2015, 9:29 a.m. UTC | #2 
Hi,

Baytrail is very much released and out there already - should this patch 
also go to stable?

Are there current problems (like HDMI codec not being detected) that are 
fixed by this patch?

// David

On 2015-04-21 07:12, mengdong.lin@intel.com wrote:
> From: Mengdong Lin <mengdong.lin@intel.com>
>
> This patch addes AZX_DCAPS_I915_POWERWELL to BYT (Baytrail).
>
> Like Braswell and Skylake, the HDMI codec on Bytrail is also in the shared
> power well with GPU. This power well must be turned on before we reset link
> to probe the codec, to avoid communication failure with the codec.
>
> The side effect is that this power is always ON in S0 because the BYT HDMI
> codec does not support EPSS or D3ClkStop and so the controller doesn't enter
> D3 at runtime, and the HDMI codec and analog codec share a single physical
> HD-A link and so we cannot reset the HD-A link freely when we re-enable the
> power to use the HDMI codec.
>
> Next step is to test if an AGP reset or double AGP reset on BYT HDMI codec is
> okay to bring the HDMI codec back to a functional state after restoring the
> power. If okay, we can bind the power on/off with the HDMI codec PM without
> interrupting the analog audio.
>
> Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
>
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index e1c2105..34040d2 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -297,6 +297,9 @@ enum {
>   	 AZX_DCAPS_PM_RUNTIME | AZX_DCAPS_I915_POWERWELL |\
>   	 AZX_DCAPS_SNOOP_TYPE(SCH))
>
> +#define AZX_DCAPS_INTEL_BAYTRAIL \
> +	(AZX_DCAPS_INTEL_PCH_NOPM | AZX_DCAPS_I915_POWERWELL)
> +
>   #define AZX_DCAPS_INTEL_BRASWELL \
>   	(AZX_DCAPS_INTEL_PCH | AZX_DCAPS_I915_POWERWELL)
>
> @@ -1992,7 +1995,7 @@ static const struct pci_device_id azx_ids[] = {
>   	  .driver_data = AZX_DRIVER_SCH | AZX_DCAPS_INTEL_PCH_NOPM },
>   	/* BayTrail */
>   	{ PCI_DEVICE(0x8086, 0x0f04),
> -	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH_NOPM },
> +	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_BAYTRAIL },
>   	/* Braswell */
>   	{ PCI_DEVICE(0x8086, 0x2284),
>   	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_BRASWELL },
>
Takashi Iwai April 21, 2015, 9:48 a.m. UTC | #3 
At Tue, 21 Apr 2015 11:29:47 +0200,
David Henningsson wrote:
> 
> Hi,
> 
> Baytrail is very much released and out there already - should this patch 
> also go to stable?

I thought of that, but this patch also has a drawback, thus I
hesitated to make it blindly.

> Are there current problems (like HDMI codec not being detected) that are 
> fixed by this patch?

If there is a known bug (report) and this fixes indeed, it deserves
the stable kernel, indeed.


Takashi


> // David
> 
> On 2015-04-21 07:12, mengdong.lin@intel.com wrote:
> > From: Mengdong Lin <mengdong.lin@intel.com>
> >
> > This patch addes AZX_DCAPS_I915_POWERWELL to BYT (Baytrail).
> >
> > Like Braswell and Skylake, the HDMI codec on Bytrail is also in the shared
> > power well with GPU. This power well must be turned on before we reset link
> > to probe the codec, to avoid communication failure with the codec.
> >
> > The side effect is that this power is always ON in S0 because the BYT HDMI
> > codec does not support EPSS or D3ClkStop and so the controller doesn't enter
> > D3 at runtime, and the HDMI codec and analog codec share a single physical
> > HD-A link and so we cannot reset the HD-A link freely when we re-enable the
> > power to use the HDMI codec.
> >
> > Next step is to test if an AGP reset or double AGP reset on BYT HDMI codec is
> > okay to bring the HDMI codec back to a functional state after restoring the
> > power. If okay, we can bind the power on/off with the HDMI codec PM without
> > interrupting the analog audio.
> >
> > Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index e1c2105..34040d2 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -297,6 +297,9 @@ enum {
> >   	 AZX_DCAPS_PM_RUNTIME | AZX_DCAPS_I915_POWERWELL |\
> >   	 AZX_DCAPS_SNOOP_TYPE(SCH))
> >
> > +#define AZX_DCAPS_INTEL_BAYTRAIL \
> > +	(AZX_DCAPS_INTEL_PCH_NOPM | AZX_DCAPS_I915_POWERWELL)
> > +
> >   #define AZX_DCAPS_INTEL_BRASWELL \
> >   	(AZX_DCAPS_INTEL_PCH | AZX_DCAPS_I915_POWERWELL)
> >
> > @@ -1992,7 +1995,7 @@ static const struct pci_device_id azx_ids[] = {
> >   	  .driver_data = AZX_DRIVER_SCH | AZX_DCAPS_INTEL_PCH_NOPM },
> >   	/* BayTrail */
> >   	{ PCI_DEVICE(0x8086, 0x0f04),
> > -	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH_NOPM },
> > +	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_BAYTRAIL },
> >   	/* Braswell */
> >   	{ PCI_DEVICE(0x8086, 0x2284),
> >   	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_BRASWELL },
> >
> 
> -- 
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
>
Lin, Mengdong April 22, 2015, 9:54 a.m. UTC | #4 
> -----Original Message-----
> From: Lin, Mengdong
> Sent: Tuesday, April 21, 2015 1:12 PM
> To: alsa-devel@alsa-project.org; tiwai@suse.de
> Cc: Lin, Mengdong
> Subject: [PATCH] ALSA: hda - add AZX_DCAPS_I915_POWERWELL to Baytrail
> 
> From: Mengdong Lin <mengdong.lin@intel.com>
> 
> This patch addes AZX_DCAPS_I915_POWERWELL to BYT (Baytrail).
> 
> Like Braswell and Skylake, the HDMI codec on Bytrail is also in the shared
> power well with GPU. This power well must be turned on before we reset link
> to probe the codec, to avoid communication failure with the codec.
> 
> The side effect is that this power is always ON in S0 because the BYT HDMI
> codec does not support EPSS or D3ClkStop and so the controller doesn't enter
> D3 at runtime, and the HDMI codec and analog codec share a single physical
> HD-A link and so we cannot reset the HD-A link freely when we re-enable the
> power to use the HDMI codec.
> 
> Next step is to test if an AGP reset or double AGP reset on BYT HDMI codec is
> okay to bring the HDMI codec back to a functional state after restoring the
> power. If okay, we can bind the power on/off with the HDMI codec PM without
> interrupting the analog audio.

Test on Baytrail is positive. There is no need to use AGP reset. We can
-  release the shared i915 power at the end of azx_probe_continue()
-  request the power in snd_hda_codec_register(), since the codec initial power state is D0.
-  request/release the power in hda_codec_runtime_resume/suspend.

We'll do more stress test on Baytrail and Braswell. These two platforms have similar silicon implementation. 
And finally we'll test Skylake.

Thanks
Mengdong
Lin, Mengdong April 23, 2015, 3:01 a.m. UTC | #5 
> At Tue, 21 Apr 2015 11:29:47 +0200,
> David Henningsson wrote:
> >
> > Hi,
> >
> > Baytrail is very much released and out there already - should this
> > patch also go to stable?
> 
> I thought of that, but this patch also has a drawback, thus I hesitated to make
> it blindly.

Hi Takashi and David,

Without this patch, we observed communication dead on Baytrail between the HD-A controller and HDMI codec, with mainline kernel 4.0.
It's because the i915 driver can disable this power well without sync with audio driver. 
Please see bug https://bugzilla.kernel.org/show_bug.cgi?id=93881

This fix up is not so power saving and so we're trying improve it.

> > Are there current problems (like HDMI codec not being detected) that
> > are fixed by this patch?
> 
> If there is a known bug (report) and this fixes indeed, it deserves the stable
> kernel, indeed.

The issue that HDMI codec is not always being detected is only reported on Skylake (although theoretically it can happen on Baytrail/Braswell without power sync but we never observed this).
SKL HW implementation has some change from its predecessors and need extra fixing up in i915 driver, Libin is working on this and we're close to solve this.

Thanks
Mengdong
diff mbox

Patch

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index e1c2105..34040d2 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -297,6 +297,9 @@  enum {
 	 AZX_DCAPS_PM_RUNTIME | AZX_DCAPS_I915_POWERWELL |\
 	 AZX_DCAPS_SNOOP_TYPE(SCH))
 
+#define AZX_DCAPS_INTEL_BAYTRAIL \
+	(AZX_DCAPS_INTEL_PCH_NOPM | AZX_DCAPS_I915_POWERWELL)
+
 #define AZX_DCAPS_INTEL_BRASWELL \
 	(AZX_DCAPS_INTEL_PCH | AZX_DCAPS_I915_POWERWELL)
 
@@ -1992,7 +1995,7 @@  static const struct pci_device_id azx_ids[] = {
 	  .driver_data = AZX_DRIVER_SCH | AZX_DCAPS_INTEL_PCH_NOPM },
 	/* BayTrail */
 	{ PCI_DEVICE(0x8086, 0x0f04),
-	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH_NOPM },
+	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_BAYTRAIL },
 	/* Braswell */
 	{ PCI_DEVICE(0x8086, 0x2284),
 	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_BRASWELL },