Should we really use POS_FIX_VIACOMBO for AMD/ATI chipsets?

Should we really use POS_FIX_VIACOMBO for AMD/ATI chipsets?

[David Henningsson]

I'm far from sure, but I could be on to something...

Looking at the code for azx_via_get_position, it seems that for 
recording streams, it accesses a register at position 
VIA_IN_STREAM0_FIFO_SIZE_OFFSET = 0x90. Looking at the datasheet for 
SB700, that register is not even present. This does not seem right to me.

Could it be that we should use POS_FIX_LPIB by default for AMD/ATI 
chipsets instead? It seems more reasonable to me.
In addition, I looked through some of the commits for quirking to 
POS_FIX_LPIB and the four I checked were all ATI chipsets.

So; I've tried adding:
options snd-hda-intel position_fix=1
to /etc/modprobe.d/alsa-base.conf

...to my machine with the infamous [1002:4383] controller (and 
rebooted), and the few times I've tested, recording worked successfully 
with pulseaudio. So could you please follow this up with testing on your 
machines to see if we have actually managed to solve this long-standing bug?

The only annoying thing is that I didn't realise this earlier :-/

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

Re: Should we really use POS_FIX_VIACOMBO for AMD/ATI chipsets?

[Takashi Iwai]

At Fri, 20 May 2011 15:33:56 +0200,
David Henningsson wrote:
> 
> I'm far from sure, but I could be on to something...
> 
> Looking at the code for azx_via_get_position, it seems that for 
> recording streams, it accesses a register at position 
> VIA_IN_STREAM0_FIFO_SIZE_OFFSET = 0x90. Looking at the datasheet for 
> SB700, that register is not even present. This does not seem right to me.
> 
> Could it be that we should use POS_FIX_LPIB by default for AMD/ATI 
> chipsets instead? It seems more reasonable to me.
> In addition, I looked through some of the commits for quirking to 
> POS_FIX_LPIB and the four I checked were all ATI chipsets.
> 
> So; I've tried adding:
> options snd-hda-intel position_fix=1
> to /etc/modprobe.d/alsa-base.conf
> 
> ...to my machine with the infamous [1002:4383] controller (and 
> rebooted), and the few times I've tested, recording worked successfully 
> with pulseaudio. So could you please follow this up with testing on your 
> machines to see if we have actually managed to solve this long-standing bug?
> 
> The only annoying thing is that I didn't realise this earlier :-/

Hmm, I took this as default as it seems fixing the issue on machines,
indeed.  Actually I had to use this option first for removing
recording noises on AMD Hudson.  But, maybe I need to double-check
again after the snoop bit and other workarounds.

The FIFO size might got wrong on AMD.  In that case, the driver
calculates as if FIFO = 0.  But, it can still help for correcting the
position via position-buffer vs LPIB comparison.

For the real check, we should put some debug prints on real machines.


Takashi

Re: Should we really use POS_FIX_VIACOMBO for AMD/ATI chipsets?

[Takashi Iwai]

At Fri, 20 May 2011 15:46:37 +0200,
Takashi Iwai wrote:
> 
> At Fri, 20 May 2011 15:33:56 +0200,
> David Henningsson wrote:
> > 
> > I'm far from sure, but I could be on to something...
> > 
> > Looking at the code for azx_via_get_position, it seems that for 
> > recording streams, it accesses a register at position 
> > VIA_IN_STREAM0_FIFO_SIZE_OFFSET = 0x90. Looking at the datasheet for 
> > SB700, that register is not even present. This does not seem right to me.
> > 
> > Could it be that we should use POS_FIX_LPIB by default for AMD/ATI 
> > chipsets instead? It seems more reasonable to me.
> > In addition, I looked through some of the commits for quirking to 
> > POS_FIX_LPIB and the four I checked were all ATI chipsets.
> > 
> > So; I've tried adding:
> > options snd-hda-intel position_fix=1
> > to /etc/modprobe.d/alsa-base.conf
> > 
> > ...to my machine with the infamous [1002:4383] controller (and 
> > rebooted), and the few times I've tested, recording worked successfully 
> > with pulseaudio. So could you please follow this up with testing on your 
> > machines to see if we have actually managed to solve this long-standing bug?
> > 
> > The only annoying thing is that I didn't realise this earlier :-/
> 
> Hmm, I took this as default as it seems fixing the issue on machines,
> indeed.  Actually I had to use this option first for removing
> recording noises on AMD Hudson.  But, maybe I need to double-check
> again after the snoop bit and other workarounds.
> 
> The FIFO size might got wrong on AMD.  In that case, the driver
> calculates as if FIFO = 0.  But, it can still help for correcting the
> position via position-buffer vs LPIB comparison.
> 
> For the real check, we should put some debug prints on real machines.

Actually the values read there look not working with posbuf.
Let's fix them to use POS_FIX_LPIB now.

Thanks for remark!


Takashi

Re: Should we really use POS_FIX_VIACOMBO for AMD/ATI chipsets?

[Takashi Iwai]

At Fri, 20 May 2011 16:19:57 +0200,
Takashi Iwai wrote:
> 
> At Fri, 20 May 2011 15:46:37 +0200,
> Takashi Iwai wrote:
> > 
> > At Fri, 20 May 2011 15:33:56 +0200,
> > David Henningsson wrote:
> > > 
> > > I'm far from sure, but I could be on to something...
> > > 
> > > Looking at the code for azx_via_get_position, it seems that for 
> > > recording streams, it accesses a register at position 
> > > VIA_IN_STREAM0_FIFO_SIZE_OFFSET = 0x90. Looking at the datasheet for 
> > > SB700, that register is not even present. This does not seem right to me.
> > > 
> > > Could it be that we should use POS_FIX_LPIB by default for AMD/ATI 
> > > chipsets instead? It seems more reasonable to me.
> > > In addition, I looked through some of the commits for quirking to 
> > > POS_FIX_LPIB and the four I checked were all ATI chipsets.
> > > 
> > > So; I've tried adding:
> > > options snd-hda-intel position_fix=1
> > > to /etc/modprobe.d/alsa-base.conf
> > > 
> > > ...to my machine with the infamous [1002:4383] controller (and 
> > > rebooted), and the few times I've tested, recording worked successfully 
> > > with pulseaudio. So could you please follow this up with testing on your 
> > > machines to see if we have actually managed to solve this long-standing bug?
> > > 
> > > The only annoying thing is that I didn't realise this earlier :-/
> > 
> > Hmm, I took this as default as it seems fixing the issue on machines,
> > indeed.  Actually I had to use this option first for removing
> > recording noises on AMD Hudson.  But, maybe I need to double-check
> > again after the snoop bit and other workarounds.
> > 
> > The FIFO size might got wrong on AMD.  In that case, the driver
> > calculates as if FIFO = 0.  But, it can still help for correcting the
> > position via position-buffer vs LPIB comparison.
> > 
> > For the real check, we should put some debug prints on real machines.
> 
> Actually the values read there look not working with posbuf.
> Let's fix them to use POS_FIX_LPIB now.

The patch is below, commit 50e3bbf9898840eead86f90a43b3625a2b2f4112
as below.


Takashi

---
Subject: [PATCH] ALSA: hda - Use LPIB for ATI/AMD chipsets as default

ATI and AMD chipsets seem not providing the proper position-buffer
information, and it also doesn't provide FIFO register required by
VIACOMBO fix.  It's better to use LPIB for these.

Reported-by: David Henningsson <david.henningsson@canonical.com>
Cc: <stable@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_intel.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 0c1996d..43a0367 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2367,9 +2367,16 @@ static int __devinit check_position_fix(struct azx *chip, int fix)
 	/* Check VIA/ATI HD Audio Controller exist */
 	switch (chip->driver_type) {
 	case AZX_DRIVER_VIA:
-	case AZX_DRIVER_ATI:
 		/* Use link position directly, avoid any transfer problem. */
 		return POS_FIX_VIACOMBO;
+	case AZX_DRIVER_ATI:
+		/* ATI chipsets don't work well with position-buffer */
+		return POS_FIX_LPIB;
+	case AZX_DRIVER_GENERIC:
+		/* AMD chipsets also don't work with position-buffer */
+		if (chip->pci->vendor == PCI_VENDOR_ID_AMD)
+			return POS_FIX_LPIB;
+		break;
 	}
 
 	return POS_FIX_AUTO;
-- 
1.7.4.2

Re: Should we really use POS_FIX_VIACOMBO for AMD/ATI chipsets?

[David Henningsson]

On 2011-05-20 15:46, Takashi Iwai wrote:
> At Fri, 20 May 2011 15:33:56 +0200,
> David Henningsson wrote:
>>
>> I'm far from sure, but I could be on to something...
>>
>> Looking at the code for azx_via_get_position, it seems that for
>> recording streams, it accesses a register at position
>> VIA_IN_STREAM0_FIFO_SIZE_OFFSET = 0x90. Looking at the datasheet for
>> SB700, that register is not even present. This does not seem right to me.
>>
>> Could it be that we should use POS_FIX_LPIB by default for AMD/ATI
>> chipsets instead? It seems more reasonable to me.
>> In addition, I looked through some of the commits for quirking to
>> POS_FIX_LPIB and the four I checked were all ATI chipsets.
>>
>> So; I've tried adding:
>> options snd-hda-intel position_fix=1
>> to /etc/modprobe.d/alsa-base.conf
>>
>> ...to my machine with the infamous [1002:4383] controller (and
>> rebooted), and the few times I've tested, recording worked successfully
>> with pulseaudio. So could you please follow this up with testing on your
>> machines to see if we have actually managed to solve this long-standing bug?
>>
>> The only annoying thing is that I didn't realise this earlier :-/
>
> Hmm, I took this as default as it seems fixing the issue on machines,
> indeed.  Actually I had to use this option first for removing
> recording noises on AMD Hudson.  But, maybe I need to double-check
> again after the snoop bit and other workarounds.
>
> The FIFO size might got wrong on AMD.  In that case, the driver
> calculates as if FIFO = 0.  But, it can still help for correcting the
> position via position-buffer vs LPIB comparison.
>
> For the real check, we should put some debug prints on real machines.

Hey, you're faster than I am :-) Ok, so there actually is a FIFO 
register (I read the wrong table). But the position buffer is dead and 
always read as zero.

I did some debug print which is pasted here: http://paste.ubuntu.com/610637/

If LPIB only gives you occasional recording noises, perhaps we need a 
new type of position-fix that adds some margin (e g round down to 
nearest multiple of fifo-size, then subtract one fifo-size or something)?

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

Re: Should we really use POS_FIX_VIACOMBO for AMD/ATI chipsets?

[Takashi Iwai]

At Fri, 20 May 2011 16:51:13 +0200,
David Henningsson wrote:
> 
> On 2011-05-20 15:46, Takashi Iwai wrote:
> > At Fri, 20 May 2011 15:33:56 +0200,
> > David Henningsson wrote:
> >>
> >> I'm far from sure, but I could be on to something...
> >>
> >> Looking at the code for azx_via_get_position, it seems that for
> >> recording streams, it accesses a register at position
> >> VIA_IN_STREAM0_FIFO_SIZE_OFFSET = 0x90. Looking at the datasheet for
> >> SB700, that register is not even present. This does not seem right to me.
> >>
> >> Could it be that we should use POS_FIX_LPIB by default for AMD/ATI
> >> chipsets instead? It seems more reasonable to me.
> >> In addition, I looked through some of the commits for quirking to
> >> POS_FIX_LPIB and the four I checked were all ATI chipsets.
> >>
> >> So; I've tried adding:
> >> options snd-hda-intel position_fix=1
> >> to /etc/modprobe.d/alsa-base.conf
> >>
> >> ...to my machine with the infamous [1002:4383] controller (and
> >> rebooted), and the few times I've tested, recording worked successfully
> >> with pulseaudio. So could you please follow this up with testing on your
> >> machines to see if we have actually managed to solve this long-standing bug?
> >>
> >> The only annoying thing is that I didn't realise this earlier :-/
> >
> > Hmm, I took this as default as it seems fixing the issue on machines,
> > indeed.  Actually I had to use this option first for removing
> > recording noises on AMD Hudson.  But, maybe I need to double-check
> > again after the snoop bit and other workarounds.
> >
> > The FIFO size might got wrong on AMD.  In that case, the driver
> > calculates as if FIFO = 0.  But, it can still help for correcting the
> > position via position-buffer vs LPIB comparison.
> >
> > For the real check, we should put some debug prints on real machines.
> 
> Hey, you're faster than I am :-) Ok, so there actually is a FIFO 
> register (I read the wrong table). But the position buffer is dead and 
> always read as zero.
> 
> I did some debug print which is pasted here: http://paste.ubuntu.com/610637/
> 
> If LPIB only gives you occasional recording noises, perhaps we need a 
> new type of position-fix that adds some margin (e g round down to 
> nearest multiple of fifo-size, then subtract one fifo-size or something)?

The latter sounds more appropriate (although it's just my wild guess).

We are also checking the position via jiffies, so big jumps should
have been filtered out.  The problem is more about the small amount of
difference from the real position.


thanks,

Takashi