diff mbox

ALSA: hda - Don't access stereo amps for mono channel widgets

Message ID s5hsid547lc.wl-tiwai@suse.de (mailing list archive)
State Accepted
Commit cc261738add93947d138d2fabad9f4dbed4e5c00
Headers show

Commit Message

Takashi Iwai March 16, 2015, 9:25 a.m. UTC 
At Mon, 16 Mar 2015 08:42:56 +0100,
Takashi Iwai wrote:
> 
> At Mon, 16 Mar 2015 12:02:36 +0800,
> Raymond Yau wrote:
> > 
> > >
> > > The current HDA generic parser initializes / modifies the amp values
> > > always in stereo, but this seems causing the problem on ALC3229 codec
> > > that has a few mono channel widgets: namely, these mono widgets react
> > > to actions for both channels equally.
> > >
> > > In the driver code, we do care the mono channel and create a control
> > > only for the left channel (as defined in HD-audio spec) for such a
> > > node.  When the control is updated, only the left channel value is
> > > changed.  However, in the resume, the right channel value is also
> > > restored from the initial value we took as stereo, and this overwrites
> > > the left channel value.  This ends up being the silent output as the
> > > right channel has been never touched and remains muted.
> > >
> > > This patch covers the places where unconditional stereo amp accesses
> > > are done and converts to the conditional accesses.
> > >
> > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=94581
> > 
> > Seem the number of amp-in vals is not only depend on widget cap mono
> > 
> > In your example, node 0xf is mono and has only one connection
> > 
> > Node 0x0f [Audio Mixer] wcaps 0x20010a: Mono Amp-In
> >   Amp-In caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1
> >   Amp-In vals:  [0x00]
> >   Connection: 1
> >      0x0d
> > 
> > Refer to block diagram in alc272 datasheet
> > 
> > Node 0xf has two mute switch
> > 
> > https://launchpadlibrarian.net/200283132/AlsaInfo.txt
> > 
> > Node 0x0f [Audio Mixer] wcaps 0x20010a: Mono Amp-In
> >   Amp-In caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1
> >   Amp-In vals:  [0x00] [0x00]
> >   Connection: 2
> >      0x02 0x0b
> > 
> > Refer to HDA Specification
> > 
> > 7.1.3 Widget Interconnection Rules
> > 
> > 5. When a stereo widget is an input to a mono widget, only the L-channel of
> > the source drives the sink.
> > 
> > There is one exception to this rule, which allows a mono Mixer Widget to
> > mix the L- and R-channels of a single stereo widget into a mono channel.
> > 
> > This exception occurs only when the sink widget is: A Mixer Widget, and Is
> > identified as “mono,” and Contains exactly one entry in its connection
> > list, and The single widget identified in its connection list (source) is
> > “stereo.”
> > 
> > In this case, this mixer is required to have two inputs, the first being
> > driven by the L-channel of the stereo source and the second by the
> > R-channel.
> 
> The signals are summed up in that case, indeed, but what we do care
> here is about which amp channel value to control.  So, it's irrelevant
> from the code change.

After reconsidering this, I see your point now.  The widget 0x0f in
the first example above needs to have stereo (left and right) amp
values.  What's confusing is that the second case above does *not*
have stereo amp values.  That is, this exception is applied only for a
single stereo-to-mono mix route, not for M-to-N mixing.

The patch below is the fix.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: hda - Treat stereo-to-mono mix properly

The commit [ef403edb7558: ALSA: hda - Don't access stereo amps for
mono channel widgets] fixed the handling of mono widgets in general,
but it still misses an exceptional case: namely, a mono mixer widget
taking a single stereo input.  In this case, it has stereo volumes
although it's a mono widget, and thus we have to take care of both
left and right input channels.

This patch covers this missing piece by adding proper checks of stereo
amps in both the generic parser and the proc output codes.

Reported-by: Raymond Yau <superquad.vortex2@gmail.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_generic.c | 21 +++++++++++++++++++--
 sound/pci/hda/hda_proc.c    | 38 ++++++++++++++++++++++++++++++--------
 2 files changed, 49 insertions(+), 10 deletions(-)

Comments

Raymond Yau March 16, 2015, 9:57 a.m. UTC | #1 
> > >
> > > >
> > > > The current HDA generic parser initializes / modifies the amp values
> > > > always in stereo, but this seems causing the problem on ALC3229
codec
> > > > that has a few mono channel widgets: namely, these mono widgets
react
> > > > to actions for both channels equally.
> > > >
> > > > In the driver code, we do care the mono channel and create a control
> > > > only for the left channel (as defined in HD-audio spec) for such a
> > > > node.  When the control is updated, only the left channel value is
> > > > changed.  However, in the resume, the right channel value is also
> > > > restored from the initial value we took as stereo, and this
overwrites
> > > > the left channel value.  This ends up being the silent output as the
> > > > right channel has been never touched and remains muted.
> > > >
> > > > This patch covers the places where unconditional stereo amp accesses
> > > > are done and converts to the conditional accesses.
> > > >
> > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=94581
> > >
> > > Seem the number of amp-in vals is not only depend on widget cap mono
> > >
> > > In your example, node 0xf is mono and has only one connection
> > >
> > > Node 0x0f [Audio Mixer] wcaps 0x20010a: Mono Amp-In
> > >   Amp-In caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1
> > >   Amp-In vals:  [0x00]
> > >   Connection: 1
> > >      0x0d
> > >
> > > Refer to block diagram in alc272 datasheet
> > >
> > > Node 0xf has two mute switch
> > >
> > > https://launchpadlibrarian.net/200283132/AlsaInfo.txt
> > >
> > > Node 0x0f [Audio Mixer] wcaps 0x20010a: Mono Amp-In
> > >   Amp-In caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1
> > >   Amp-In vals:  [0x00] [0x00]
> > >   Connection: 2
> > >      0x02 0x0b
> > >
> > > Refer to HDA Specification
> > >
> > > 7.1.3 Widget Interconnection Rules
> > >
> > > 5. When a stereo widget is an input to a mono widget, only the
L-channel of
> > > the source drives the sink.
> > >
> > > There is one exception to this rule, which allows a mono Mixer Widget
to
> > > mix the L- and R-channels of a single stereo widget into a mono
channel.
> > >
> > > This exception occurs only when the sink widget is: A Mixer Widget,
and Is
> > > identified as “mono,” and Contains exactly one entry in its connection
> > > list, and The single widget identified in its connection list
(source) is
> > > “stereo.”
> > >
> > > In this case, this mixer is required to have two inputs, the first
being
> > > driven by the L-channel of the stereo source and the second by the
> > > R-channel.
> >
> > The signals are summed up in that case, indeed, but what we do care
> > here is about which amp channel value to control.  So, it's irrelevant
> > from the code change.
>
> After reconsidering this, I see your point now.  The widget 0x0f in
> the first example above needs to have stereo (left and right) amp
> values.  What's confusing is that the second case above does *not*
> have stereo amp values.  That is, this exception is applied only for a
> single stereo-to-mono mix route, not for M-to-N mixing.
>

It seem that I am wrong after reconsidering this case

https://bugs.launchpad.net/ubuntu/+source/pulseaudio/+bug/1432347

It is strange that a four channels codec has three audio outputs as the
datasheet only have two DAC

Codec: Realtek ALC272
Address: 1
AFG Function Id: 0x1 (unsol 1)
Vendor Id: 0x10ec0272
Subsystem Id: 0x17aac004
Revision Id: 0x100001
No Modem Function Group found
Default PCM:
    rates [0x560]: 44100 48000 96000 192000
    bits [0xe]: 16 20 24
    formats [0x1]: PCM
Default Amp-In caps: N/A
Default Amp-Out caps: N/A
State of AFG node 0x01:
  Power states:  D0 D1 D2 D3 CLKSTOP
  Power: setting=D0, actual=D0
GPIO: io=2, o=0, i=0, unsolicited=1, wake=0
  IO[0]: enable=1, dir=1, wake=0, sticky=0, data=1, unsol=0
  IO[1]: enable=0, dir=0, wake=0, sticky=0, data=0, unsol=0
Node 0x02 [Audio Output] wcaps 0x41d: Stereo Amp-Out
  Control: name="Bass Speaker Playback Volume", index=0, device=0
    ControlAmp: chs=3, dir=Out, idx=0, ofs=0
  Amp-Out caps: ofs=0x40, nsteps=0x40, stepsize=0x03, mute=0
  Amp-Out vals:  [0x2c 0x2c]
  Converter: stream=0, channel=0
  PCM:
    rates [0x560]: 44100 48000 96000 192000
    bits [0xe]: 16 20 24
    formats [0x1]: PCM
  Power states:  D0 D1 D2 D3 EPSS
  Power: setting=D0, actual=D0
Node 0x03 [Audio Output] wcaps 0x41d: Stereo Amp-Out
  Control: name="Speaker Playback Volume", index=0, device=0
    ControlAmp: chs=3, dir=Out, idx=0, ofs=0
  Device: name="ALC272 Analog", type="Audio", device=0
  Amp-Out caps: ofs=0x40, nsteps=0x40, stepsize=0x03, mute=0
  Amp-Out vals:  [0x2c 0x2c]
  Converter: stream=0, channel=0
  PCM:
    rates [0x560]: 44100 48000 96000 192000
    bits [0xe]: 16 20 24
    formats [0x1]: PCM
  Power states:  D0 D1 D2 D3 EPSS
  Power: setting=D0, actual=D0
Node 0x04 [Audio Output] wcaps 0x41d: Stereo Amp-Out
  Control: name="Headphone Playback Volume", index=0, device=0
    ControlAmp: chs=3, dir=Out, idx=0, ofs=0
  Amp-Out caps: ofs=0x40, nsteps=0x40, stepsize=0x03, mute=0
  Amp-Out vals:  [0x2c 0x2c]
  Converter: stream=0, channel=0
  PCM:
    rates [0x560]: 44100 48000 96000 192000
    bits [0xe]: 16 20 24
    formats [0x1]: PCM
  Power states:  D0 D1 D2 D3 EPSS
  Power: setting=D0, actual=D0
diff mbox

Patch

diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index fe18071bf93a..8ec5289f8e05 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -687,13 +687,30 @@  static int get_amp_val_to_activate(struct hda_codec *codec, hda_nid_t nid,
 	return val;
 }
 
+/* is this a stereo widget or a stereo-to-mono mix? */
+static bool is_stereo_amps(struct hda_codec *codec, hda_nid_t nid, int dir)
+{
+	unsigned int wcaps = get_wcaps(codec, nid);
+	hda_nid_t conn;
+
+	if (wcaps & AC_WCAP_STEREO)
+		return true;
+	if (dir != HDA_INPUT || get_wcaps_type(wcaps) != AC_WID_AUD_MIX)
+		return false;
+	if (snd_hda_get_num_conns(codec, nid) != 1)
+		return false;
+	if (snd_hda_get_connections(codec, nid, &conn, 1) < 0)
+		return false;
+	return !!(get_wcaps(codec, conn) & AC_WCAP_STEREO);
+}
+
 /* initialize the amp value (only at the first time) */
 static void init_amp(struct hda_codec *codec, hda_nid_t nid, int dir, int idx)
 {
 	unsigned int caps = query_amp_caps(codec, nid, dir);
 	int val = get_amp_val_to_activate(codec, nid, dir, caps, false);
 
-	if (get_wcaps(codec, nid) & AC_WCAP_STEREO)
+	if (is_stereo_amps(codec, nid, dir))
 		snd_hda_codec_amp_init_stereo(codec, nid, dir, idx, 0xff, val);
 	else
 		snd_hda_codec_amp_init(codec, nid, 0, dir, idx, 0xff, val);
@@ -703,7 +720,7 @@  static void init_amp(struct hda_codec *codec, hda_nid_t nid, int dir, int idx)
 static int update_amp(struct hda_codec *codec, hda_nid_t nid, int dir, int idx,
 		      unsigned int mask, unsigned int val)
 {
-	if (get_wcaps(codec, nid) & AC_WCAP_STEREO)
+	if (is_stereo_amps(codec, nid, dir))
 		return snd_hda_codec_amp_stereo(codec, nid, dir, idx,
 						mask, val);
 	else
diff --git a/sound/pci/hda/hda_proc.c b/sound/pci/hda/hda_proc.c
index ce5a6da83419..05e19f78b4cb 100644
--- a/sound/pci/hda/hda_proc.c
+++ b/sound/pci/hda/hda_proc.c
@@ -134,13 +134,38 @@  static void print_amp_caps(struct snd_info_buffer *buffer,
 		    (caps & AC_AMPCAP_MUTE) >> AC_AMPCAP_MUTE_SHIFT);
 }
 
+/* is this a stereo widget or a stereo-to-mono mix? */
+static bool is_stereo_amps(struct hda_codec *codec, hda_nid_t nid,
+			   int dir, unsigned int wcaps, int indices)
+{
+	hda_nid_t conn;
+
+	if (wcaps & AC_WCAP_STEREO)
+		return true;
+	/* check for a stereo-to-mono mix; it must be:
+	 * only a single connection, only for input, and only a mixer widget
+	 */
+	if (indices != 1 || dir != HDA_INPUT ||
+	    get_wcaps_type(wcaps) != AC_WID_AUD_MIX)
+		return false;
+
+	if (snd_hda_get_raw_connections(codec, nid, &conn, 1) < 0)
+		return false;
+	/* the connection source is a stereo? */
+	wcaps = snd_hda_param_read(codec, conn, AC_PAR_AUDIO_WIDGET_CAP);
+	return !!(wcaps & AC_WCAP_STEREO);
+}
+
 static void print_amp_vals(struct snd_info_buffer *buffer,
 			   struct hda_codec *codec, hda_nid_t nid,
-			   int dir, int stereo, int indices)
+			   int dir, unsigned int wcaps, int indices)
 {
 	unsigned int val;
+	bool stereo;
 	int i;
 
+	stereo = is_stereo_amps(codec, nid, dir, wcaps, indices);
+
 	dir = dir == HDA_OUTPUT ? AC_AMP_GET_OUTPUT : AC_AMP_GET_INPUT;
 	for (i = 0; i < indices; i++) {
 		snd_iprintf(buffer, " [");
@@ -757,12 +782,10 @@  static void print_codec_info(struct snd_info_entry *entry,
 			    (codec->single_adc_amp &&
 			     wid_type == AC_WID_AUD_IN))
 				print_amp_vals(buffer, codec, nid, HDA_INPUT,
-					       wid_caps & AC_WCAP_STEREO,
-					       1);
+					       wid_caps, 1);
 			else
 				print_amp_vals(buffer, codec, nid, HDA_INPUT,
-					       wid_caps & AC_WCAP_STEREO,
-					       conn_len);
+					       wid_caps, conn_len);
 		}
 		if (wid_caps & AC_WCAP_OUT_AMP) {
 			snd_iprintf(buffer, "  Amp-Out caps: ");
@@ -771,11 +794,10 @@  static void print_codec_info(struct snd_info_entry *entry,
 			if (wid_type == AC_WID_PIN &&
 			    codec->pin_amp_workaround)
 				print_amp_vals(buffer, codec, nid, HDA_OUTPUT,
-					       wid_caps & AC_WCAP_STEREO,
-					       conn_len);
+					       wid_caps, conn_len);
 			else
 				print_amp_vals(buffer, codec, nid, HDA_OUTPUT,
-					       wid_caps & AC_WCAP_STEREO, 1);
+					       wid_caps, 1);
 		}
 
 		switch (wid_type) {