[PATCH] Fix buffer position for ATI SB4x0

[PATCH] Fix buffer position for ATI SB4x0

[Mauro Carvalho Chehab]

ATI SB4x0 doesn't need any fix at position.

This patch solves the issue of receiving several clicks during capture on those
devices.

Tested with a Gateway Notebook MX-6453.

Signed-off-by: Mauro Carvalho Chehab <mchehab@infradead.org>

diff -r 33d7308ade64 pci/hda/hda_intel.c
--- a/pci/hda/hda_intel.c	Fri May 23 17:36:59 2008 -0300
+++ b/pci/hda/hda_intel.c	Thu May 29 11:04:56 2008 -0300
@@ -1860,6 +1860,7 @@
 	SND_PCI_QUIRK(0x1028, 0x01cc, "Dell D820", POS_FIX_NONE),
 	SND_PCI_QUIRK(0x1028, 0x01de, "Dell Precision 390", POS_FIX_NONE),
 	SND_PCI_QUIRK(0x1043, 0x813d, "ASUS P5AD2", POS_FIX_NONE),
+	SND_PCI_QUIRK(0x1002, 0x437b, "ATI SB4x0 HDA", POS_FIX_NONE),
 	{}
 };

Re: [PATCH] Fix buffer position for ATI SB4x0

[Takashi Iwai]

At Thu, 29 May 2008 11:10:22 -0300,
Mauro Carvalho Chehab wrote:
> 
> ATI SB4x0 doesn't need any fix at position.

It's not about the position fixing but whether to use the
position-buffer.  The devices on the blacklist are the ones that have
no position buffer.  So, it would fall into LPIB mode, and this list
avoids it from the beginning.

> This patch solves the issue of receiving several clicks during capture on those
> devices.
> 
> Tested with a Gateway Notebook MX-6453.

The click noise is often a different problem.  Did you already try
the patch below?


thanks,

Takashi

===

commit 6bf70d9d25f35b41d7d04274aa22baebca5496ea
Author: Takashi Iwai <tiwai@suse.de>
Date:   Fri May 16 12:34:47 2008 +0200

    [ALSA] hda - Fix DMA position inaccuracy
    
    Many HD-audio controllers seem inaccurate about the IRQ timing of
    PCM period updates.  This has caused problems on audio quality; e.g.
    JACK doesn't work with two periods.
    
    This patch fixes the problem by checking the current DMA position
    at IRQ handler and delays the period-update via a workq if it's
    inaccurate.
    
    Signed-off-by: Takashi Iwai <tiwai@suse.de>

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index b3a618e..6ba7ac0 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -285,6 +285,7 @@ struct azx_dev {
 	u32 *posbuf;		/* position buffer pointer */
 
 	unsigned int bufsize;	/* size of the play buffer in bytes */
+	unsigned int period_bytes; /* size of the period in bytes */
 	unsigned int frags;	/* number for period in the play buffer */
 	unsigned int fifo_size;	/* FIFO size */
 
@@ -301,11 +302,10 @@ struct azx_dev {
 					 */
 	unsigned char stream_tag;	/* assigned stream */
 	unsigned char index;		/* stream index */
-	/* for sanity check of position buffer */
-	unsigned int period_intr;
 
 	unsigned int opened :1;
 	unsigned int running :1;
+	unsigned int irq_pending: 1;
 };
 
 /* CORB/RIRB */
@@ -369,6 +369,9 @@ struct azx {
 
 	/* for debugging */
 	unsigned int last_cmd;	/* last issued command (to sync) */
+
+	/* for pending irqs */
+	struct work_struct irq_pending_work;
 };
 
 /* driver types */
@@ -908,6 +911,8 @@ static void azx_init_pci(struct azx *chip)
 }
 
 
+static int azx_position_ok(struct azx *chip, struct azx_dev *azx_dev);
+
 /*
  * interrupt handler
  */
@@ -930,11 +935,18 @@ static irqreturn_t azx_interrupt(int irq, void *dev_id)
 		azx_dev = &chip->azx_dev[i];
 		if (status & azx_dev->sd_int_sta_mask) {
 			azx_sd_writeb(azx_dev, SD_STS, SD_INT_MASK);
-			if (azx_dev->substream && azx_dev->running) {
-				azx_dev->period_intr++;
+			if (!azx_dev->substream || !azx_dev->running)
+				continue;
+			/* check whether this IRQ is really acceptable */
+			if (azx_position_ok(chip, azx_dev)) {
+				azx_dev->irq_pending = 0;
 				spin_unlock(&chip->reg_lock);
 				snd_pcm_period_elapsed(azx_dev->substream);
 				spin_lock(&chip->reg_lock);
+			} else {
+				/* bogus IRQ, process it later */
+				azx_dev->irq_pending = 1;
+				schedule_work(&chip->irq_pending_work);
 			}
 		}
 	}
@@ -973,6 +985,7 @@ static int azx_setup_periods(struct snd_pcm_substream *substream,
 	azx_sd_writel(azx_dev, SD_BDLPU, 0);
 
 	period_bytes = snd_pcm_lib_period_bytes(substream);
+	azx_dev->period_bytes = period_bytes;
 	periods = azx_dev->bufsize / period_bytes;
 
 	/* program the initial BDL entries */
@@ -1421,27 +1434,16 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	return 0;
 }
 
-static snd_pcm_uframes_t azx_pcm_pointer(struct snd_pcm_substream *substream)
+static unsigned int azx_get_position(struct azx *chip,
+				     struct azx_dev *azx_dev)
 {
-	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
-	struct azx *chip = apcm->chip;
-	struct azx_dev *azx_dev = get_azx_dev(substream);
 	unsigned int pos;
 
 	if (chip->position_fix == POS_FIX_POSBUF ||
 	    chip->position_fix == POS_FIX_AUTO) {
 		/* use the position buffer */
 		pos = le32_to_cpu(*azx_dev->posbuf);
-		if (chip->position_fix == POS_FIX_AUTO &&
-		    azx_dev->period_intr == 1 && !pos) {
-			printk(KERN_WARNING
-			       "hda-intel: Invalid position buffer, "
-			       "using LPIB read method instead.\n");
-			chip->position_fix = POS_FIX_NONE;
-			goto read_lpib;
-		}
 	} else {
-	read_lpib:
 		/* read LPIB */
 		pos = azx_sd_readl(azx_dev, SD_LPIB);
 		if (chip->position_fix == POS_FIX_FIFO)
@@ -1449,7 +1451,90 @@ static snd_pcm_uframes_t azx_pcm_pointer(struct snd_pcm_substream *substream)
 	}
 	if (pos >= azx_dev->bufsize)
 		pos = 0;
-	return bytes_to_frames(substream->runtime, pos);
+	return pos;
+}
+
+static snd_pcm_uframes_t azx_pcm_pointer(struct snd_pcm_substream *substream)
+{
+	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
+	struct azx *chip = apcm->chip;
+	struct azx_dev *azx_dev = get_azx_dev(substream);
+	return bytes_to_frames(substream->runtime,
+			       azx_get_position(chip, azx_dev));
+}
+
+/*
+ * Check whether the current DMA position is acceptable for updating
+ * periods.  Returns non-zero if it's OK.
+ *
+ * Many HD-audio controllers appear pretty inaccurate about
+ * the update-IRQ timing.  The IRQ is issued before actually the
+ * data is processed.  So, we need to process it afterwords in a
+ * workqueue.
+ */
+static int azx_position_ok(struct azx *chip, struct azx_dev *azx_dev)
+{
+	unsigned int pos;
+
+	pos = azx_get_position(chip, azx_dev);
+	if (chip->position_fix == POS_FIX_AUTO) {
+		if (!pos) {
+			printk(KERN_WARNING
+			       "hda-intel: Invalid position buffer, "
+			       "using LPIB read method instead.\n");
+			chip->position_fix = POS_FIX_NONE;
+			pos = azx_get_position(chip, azx_dev);
+		} else
+			chip->position_fix = POS_FIX_POSBUF;
+	}
+
+	if (pos % azx_dev->period_bytes > azx_dev->period_bytes / 2)
+		return 0; /* NG - it's below the period boundary */
+	return 1; /* OK, it's fine */
+}
+
+/*
+ * The work for pending PCM period updates.
+ */
+static void azx_irq_pending_work(struct work_struct *work)
+{
+	struct azx *chip = container_of(work, struct azx, irq_pending_work);
+	int i, pending;
+
+	for (;;) {
+		pending = 0;
+		spin_lock_irq(&chip->reg_lock);
+		for (i = 0; i < chip->num_streams; i++) {
+			struct azx_dev *azx_dev = &chip->azx_dev[i];
+			if (!azx_dev->irq_pending ||
+			    !azx_dev->substream ||
+			    !azx_dev->running)
+				continue;
+			if (azx_position_ok(chip, azx_dev)) {
+				azx_dev->irq_pending = 0;
+				spin_unlock(&chip->reg_lock);
+				snd_pcm_period_elapsed(azx_dev->substream);
+				spin_lock(&chip->reg_lock);
+			} else
+				pending++;
+		}
+		spin_unlock_irq(&chip->reg_lock);
+		if (!pending)
+			return;
+		cond_resched();
+	}
+}
+
+/* clear irq_pending flags and assure no on-going workq */
+static void azx_clear_irq_pending(struct azx *chip)
+{
+	int i;
+
+	spin_lock_irq(&chip->reg_lock);
+	for (i = 0; i < chip->num_streams; i++)
+		chip->azx_dev[i].irq_pending = 0;
+	spin_unlock_irq(&chip->reg_lock);
+	flush_scheduled_work();
 }
 
 static struct snd_pcm_ops azx_pcm_ops = {
@@ -1676,6 +1761,7 @@ static int azx_suspend(struct pci_dev *pci, pm_message_t state)
 	int i;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
+	azx_clear_irq_pending(chip);
 	for (i = 0; i < AZX_MAX_PCMS; i++)
 		snd_pcm_suspend_all(chip->pcm[i]);
 	if (chip->initialized)
@@ -1732,6 +1818,7 @@ static int azx_free(struct azx *chip)
 	int i;
 
 	if (chip->initialized) {
+		azx_clear_irq_pending(chip);
 		for (i = 0; i < chip->num_streams; i++)
 			azx_stream_stop(chip, &chip->azx_dev[i]);
 		azx_stop_chip(chip);
@@ -1857,6 +1944,7 @@ static int __devinit azx_create(struct snd_card *card, struct pci_dev *pci,
 	chip->irq = -1;
 	chip->driver_type = driver_type;
 	chip->msi = enable_msi;
+	INIT_WORK(&chip->irq_pending_work, azx_irq_pending_work);
 
 	chip->position_fix = check_position_fix(chip, position_fix[dev]);
 	check_probe_mask(chip, dev);

Re: [PATCH] Fix buffer position for ATI SB4x0

[Mauro Carvalho Chehab]

On Thu, 29 May 2008 16:20:21 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> At Thu, 29 May 2008 11:10:22 -0300,
> Mauro Carvalho Chehab wrote:
> > 
> > ATI SB4x0 doesn't need any fix at position.
> 
> It's not about the position fixing but whether to use the
> position-buffer.  The devices on the blacklist are the ones that have
> no position buffer.  So, it would fall into LPIB mode, and this list
> avoids it from the beginning.

Sometimes, I get this message:

[  857.014011] hda-intel: Invalid position buffer, using LPIB read method instead.

It is weird, since this line doesn't appear always.

> > This patch solves the issue of receiving several clicks during capture on those
> > devices.
> > 
> > Tested with a Gateway Notebook MX-6453.
> 
> The click noise is often a different problem.  Did you already try
> the patch below?

This patch is already here on my testing environment.

There are a few reports I found at the net about Gateway devices with SB450 ATI
chipset. Some suggestions for working with MIC pointed the need of using
position_fix=0 to avoid the clicks on those devices.


One weird thing is that I've did this procedure, when I generated the patch:
	- tested without the patch - clicks at mic;
	- applied the patch I sent - no clicks;
	- removed the patch after your email - no clicks;
	- reapplied/removed several times - no clicks.

I'll try today to test with and without the position fix after cold boot/soft
boot/resume. Maybe there are some registers that aren't properly initialized on
some cases.

Btw, after a suspend/resume, some widgets aren't properly restored (at
mercurial tree version). For example, if I suspend with master Mute on, after
resume, mute is off (but the kmix still shows it as muted). I'm not sure if the
clicks happen after resume. I'll double check it.

Cheers,
Mauro

Re: [PATCH] Fix buffer position for ATI SB4x0

[Mauro Carvalho Chehab]

On Thu, 29 May 2008 16:20:21 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> At Thu, 29 May 2008 11:10:22 -0300,
> Mauro Carvalho Chehab wrote:
> > 
> > ATI SB4x0 doesn't need any fix at position.
> 
> It's not about the position fixing but whether to use the
> position-buffer.  The devices on the blacklist are the ones that have
> no position buffer.  So, it would fall into LPIB mode, and this list
> avoids it from the beginning.
> 
> > This patch solves the issue of receiving several clicks during capture on those
> > devices.
> > 
> > Tested with a Gateway Notebook MX-6453.
> 
> The click noise is often a different problem.  Did you already try
> the patch below?

The click seems associated to some residual samples inside the buffer. Here it
is a sample of the king of click noise I'm hearing here (captured from CD input entry):
	http://linuxtv.org/~mchehab/snd.ogg

I've started/stopped the CD some times (with cdplay). When volume goes to zero,
after an "echoed click", it disappears.

I've did some trials with position_fix changes, but nothing solved. So, my
previous patch were wrong. The click is somewhere else (still, I suspect that
it is linked to the buffer handling).

Cheers,
Mauro

Re: [PATCH] Fix buffer position for ATI SB4x0

[Takashi Iwai]

At Fri, 6 Jun 2008 11:52:32 -0300,
Mauro Carvalho Chehab wrote:
> 
> On Thu, 29 May 2008 16:20:21 +0200
> Takashi Iwai <tiwai@suse.de> wrote:
> 
> > At Thu, 29 May 2008 11:10:22 -0300,
> > Mauro Carvalho Chehab wrote:
> > > 
> > > ATI SB4x0 doesn't need any fix at position.
> > 
> > It's not about the position fixing but whether to use the
> > position-buffer.  The devices on the blacklist are the ones that have
> > no position buffer.  So, it would fall into LPIB mode, and this list
> > avoids it from the beginning.
> > 
> > > This patch solves the issue of receiving several clicks during capture on those
> > > devices.
> > > 
> > > Tested with a Gateway Notebook MX-6453.
> > 
> > The click noise is often a different problem.  Did you already try
> > the patch below?
> 
> The click seems associated to some residual samples inside the buffer. Here it
> is a sample of the king of click noise I'm hearing here (captured from CD input entry):
> 	http://linuxtv.org/~mchehab/snd.ogg

(I didn't check the ogg file yet, and just a wild guess)

Is it with dsnoop plugin?  With "default" PCM, ALSA uses dsnoop for
capture to allow multiplexing for HD-audio.  Does it happen with "hw"
or "plughw" PCM?


Takashi

Re: [PATCH] Fix buffer position for ATI SB4x0

[Mauro Carvalho Chehab]

On Fri, 06 Jun 2008 18:49:28 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> At Fri, 6 Jun 2008 11:52:32 -0300,
> Mauro Carvalho Chehab wrote:
> > 
> > On Thu, 29 May 2008 16:20:21 +0200
> > Takashi Iwai <tiwai@suse.de> wrote:
> > 
> > > At Thu, 29 May 2008 11:10:22 -0300,
> > > Mauro Carvalho Chehab wrote:
> > > > 
> > > > ATI SB4x0 doesn't need any fix at position.
> > > 
> > > It's not about the position fixing but whether to use the
> > > position-buffer.  The devices on the blacklist are the ones that have
> > > no position buffer.  So, it would fall into LPIB mode, and this list
> > > avoids it from the beginning.
> > > 
> > > > This patch solves the issue of receiving several clicks during capture on those
> > > > devices.
> > > > 
> > > > Tested with a Gateway Notebook MX-6453.
> > > 
> > > The click noise is often a different problem.  Did you already try
> > > the patch below?
> > 
> > The click seems associated to some residual samples inside the buffer. Here it
> > is a sample of the king of click noise I'm hearing here (captured from CD input entry):
> > 	http://linuxtv.org/~mchehab/snd.ogg
> 
> (I didn't check the ogg file yet, and just a wild guess)
> 
> Is it with dsnoop plugin?  With "default" PCM, ALSA uses dsnoop for
> capture to allow multiplexing for HD-audio.  Does it happen with "hw"
> or "plughw" PCM?

Results with the cdplay:

With "default" PCM (both with and without mmap):
	several clicks per second, very high clicks;
	(like a very risky analog disk)

With "hw" or "plughw" (no mmap):
	less clicks (something like two clicks or three per second), with less volume.
	both hw and plughw produces the same effect.

With "hw" or "plughw" and mmap (-M):
	high quality. no noticeable clicks.

So, it seems that there are two different issues: one with dsnoop and another
with non-mmapped captures.

Cheers,
Mauro

Re: [PATCH] Fix buffer position for ATI SB4x0

[Takashi Iwai]

At Fri, 6 Jun 2008 15:31:45 -0300,
Mauro Carvalho Chehab wrote:
> 
> On Fri, 06 Jun 2008 18:49:28 +0200
> Takashi Iwai <tiwai@suse.de> wrote:
> 
> > At Fri, 6 Jun 2008 11:52:32 -0300,
> > Mauro Carvalho Chehab wrote:
> > > 
> > > On Thu, 29 May 2008 16:20:21 +0200
> > > Takashi Iwai <tiwai@suse.de> wrote:
> > > 
> > > > At Thu, 29 May 2008 11:10:22 -0300,
> > > > Mauro Carvalho Chehab wrote:
> > > > > 
> > > > > ATI SB4x0 doesn't need any fix at position.
> > > > 
> > > > It's not about the position fixing but whether to use the
> > > > position-buffer.  The devices on the blacklist are the ones that have
> > > > no position buffer.  So, it would fall into LPIB mode, and this list
> > > > avoids it from the beginning.
> > > > 
> > > > > This patch solves the issue of receiving several clicks during capture on those
> > > > > devices.
> > > > > 
> > > > > Tested with a Gateway Notebook MX-6453.
> > > > 
> > > > The click noise is often a different problem.  Did you already try
> > > > the patch below?
> > > 
> > > The click seems associated to some residual samples inside the buffer. Here it
> > > is a sample of the king of click noise I'm hearing here (captured from CD input entry):
> > > 	http://linuxtv.org/~mchehab/snd.ogg
> > 
> > (I didn't check the ogg file yet, and just a wild guess)
> > 
> > Is it with dsnoop plugin?  With "default" PCM, ALSA uses dsnoop for
> > capture to allow multiplexing for HD-audio.  Does it happen with "hw"
> > or "plughw" PCM?
> 
> Results with the cdplay:
> 
> With "default" PCM (both with and without mmap):
> 	several clicks per second, very high clicks;
> 	(like a very risky analog disk)

Hm, it's obviously a problem.  I couldn't reproduce it on my machine
with HD-audio, so it might be controller/codec-specific, though.

> With "hw" or "plughw" (no mmap):
> 	less clicks (something like two clicks or three per second), with less volume.
> 	both hw and plughw produces the same effect.

If it works with hw, plughw should have no effect (i.e. no conversion
is done).  Thus it's logical that both hw and plughw have the same
result.

> With "hw" or "plughw" and mmap (-M):
> 	high quality. no noticeable clicks.
> 
> So, it seems that there are two different issues: one with dsnoop and another
> with non-mmapped captures.

Interesting.  The fact that even "hw" without mmap causes occasional
click noises implies that there is certainly a problem with the DMA
position calculation.  The dsnoop has more problems likely because it
uses smaller period size and more periods than hw, I guess.
Still not sure why the mmap mode works.

Anyway, it means that the capture position is wrongly reported, maybe
just in a reverse way of the playback position -- a few samples ahead
than the real position.  Sigh, another workaround is needed.


thanks,

Takashi

Re: [PATCH] Fix buffer position for ATI SB4x0

[Takashi Iwai]

At Sat, 07 Jun 2008 10:27:54 +0200,
I wrote:
> 
> At Fri, 6 Jun 2008 15:31:45 -0300,
> Mauro Carvalho Chehab wrote:
> > 
> > On Fri, 06 Jun 2008 18:49:28 +0200
> > Takashi Iwai <tiwai@suse.de> wrote:
> > 
> > > At Fri, 6 Jun 2008 11:52:32 -0300,
> > > Mauro Carvalho Chehab wrote:
> > > > 
> > > > On Thu, 29 May 2008 16:20:21 +0200
> > > > Takashi Iwai <tiwai@suse.de> wrote:
> > > > 
> > > > > At Thu, 29 May 2008 11:10:22 -0300,
> > > > > Mauro Carvalho Chehab wrote:
> > > > > > 
> > > > > > ATI SB4x0 doesn't need any fix at position.
> > > > > 
> > > > > It's not about the position fixing but whether to use the
> > > > > position-buffer.  The devices on the blacklist are the ones that have
> > > > > no position buffer.  So, it would fall into LPIB mode, and this list
> > > > > avoids it from the beginning.
> > > > > 
> > > > > > This patch solves the issue of receiving several clicks during capture on those
> > > > > > devices.
> > > > > > 
> > > > > > Tested with a Gateway Notebook MX-6453.
> > > > > 
> > > > > The click noise is often a different problem.  Did you already try
> > > > > the patch below?
> > > > 
> > > > The click seems associated to some residual samples inside the buffer. Here it
> > > > is a sample of the king of click noise I'm hearing here (captured from CD input entry):
> > > > 	http://linuxtv.org/~mchehab/snd.ogg
> > > 
> > > (I didn't check the ogg file yet, and just a wild guess)
> > > 
> > > Is it with dsnoop plugin?  With "default" PCM, ALSA uses dsnoop for
> > > capture to allow multiplexing for HD-audio.  Does it happen with "hw"
> > > or "plughw" PCM?
> > 
> > Results with the cdplay:
> > 
> > With "default" PCM (both with and without mmap):
> > 	several clicks per second, very high clicks;
> > 	(like a very risky analog disk)
> 
> Hm, it's obviously a problem.  I couldn't reproduce it on my machine
> with HD-audio, so it might be controller/codec-specific, though.
> 
> > With "hw" or "plughw" (no mmap):
> > 	less clicks (something like two clicks or three per second), with less volume.
> > 	both hw and plughw produces the same effect.
> 
> If it works with hw, plughw should have no effect (i.e. no conversion
> is done).  Thus it's logical that both hw and plughw have the same
> result.
> 
> > With "hw" or "plughw" and mmap (-M):
> > 	high quality. no noticeable clicks.
> > 
> > So, it seems that there are two different issues: one with dsnoop and another
> > with non-mmapped captures.
> 
> Interesting.  The fact that even "hw" without mmap causes occasional
> click noises implies that there is certainly a problem with the DMA
> position calculation.  The dsnoop has more problems likely because it
> uses smaller period size and more periods than hw, I guess.
> Still not sure why the mmap mode works.
> 
> Anyway, it means that the capture position is wrongly reported, maybe
> just in a reverse way of the playback position -- a few samples ahead
> than the real position.  Sigh, another workaround is needed.

The patch below adds another workaround for the DMA buffer position.
Could you give it a try?  It adds as default one sample delay for
issuing the interrupt so that the DMA pointer gets the right
position.  The delay can be changed (even dynamically) via bdl_pos_adj
module option.


thanks,

Takashi


diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index dc68709..401a4fd 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -58,6 +58,7 @@ static int position_fix[SNDRV_CARDS];
 static int probe_mask[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] = -1};
 static int single_cmd;
 static int enable_msi;
+static int bdl_pos_adj = 1;
 
 module_param_array(index, int, NULL, 0444);
 MODULE_PARM_DESC(index, "Index value for Intel HD audio interface.");
@@ -77,6 +78,8 @@ MODULE_PARM_DESC(single_cmd, "Use single command to communicate with codecs "
 		 "(for debugging only).");
 module_param(enable_msi, int, 0444);
 MODULE_PARM_DESC(enable_msi, "Enable Message Signaled Interrupt (MSI)");
+module_param(bdl_pos_adj, int, 0644);
+MODULE_PARM_DESC(bdl_pos_adj, "BDL position adjustment offset");
 
 #ifdef CONFIG_SND_HDA_POWER_SAVE
 /* power_save option is defined in hda_codec.c */
@@ -310,6 +313,7 @@ struct azx_dev {
 	unsigned int opened :1;
 	unsigned int running :1;
 	unsigned int irq_pending: 1;
+	unsigned int irq_ignore: 1;
 };
 
 /* CORB/RIRB */
@@ -943,6 +947,11 @@ static irqreturn_t azx_interrupt(int irq, void *dev_id)
 			azx_sd_writeb(azx_dev, SD_STS, SD_INT_MASK);
 			if (!azx_dev->substream || !azx_dev->running)
 				continue;
+			/* ignore the first dummy IRQ (due to pos_adj) */
+			if (azx_dev->irq_ignore) {
+				azx_dev->irq_ignore = 0;
+				continue;
+			}
 			/* check whether this IRQ is really acceptable */
 			if (azx_position_ok(chip, azx_dev)) {
 				azx_dev->irq_pending = 0;
@@ -977,14 +986,53 @@ static irqreturn_t azx_interrupt(int irq, void *dev_id)
 
 
 /*
+ * set up a BDL entry
+ */
+static int setup_bdle(struct snd_pcm_substream *substream,
+		      struct azx_dev *azx_dev, u32 **bdlp,
+		      int ofs, int size, int with_ioc)
+{
+	struct snd_sg_buf *sgbuf = snd_pcm_substream_sgbuf(substream);
+	u32 *bdl = *bdlp;
+
+	while (size > 0) {
+		dma_addr_t addr;
+		int chunk;
+
+		if (azx_dev->frags >= AZX_MAX_BDL_ENTRIES)
+			return -EINVAL;
+
+		addr = snd_pcm_sgbuf_get_addr(sgbuf, ofs);
+		/* program the address field of the BDL entry */
+		bdl[0] = cpu_to_le32((u32)addr);
+		bdl[1] = cpu_to_le32(upper_32bit(addr));
+		/* program the size field of the BDL entry */
+		chunk = PAGE_SIZE - (ofs % PAGE_SIZE);
+		if (size < chunk)
+			chunk = size;
+		bdl[2] = cpu_to_le32(chunk);
+		/* program the IOC to enable interrupt
+		 * only when the whole fragment is processed
+		 */
+		size -= chunk;
+		bdl[3] = (size || !with_ioc) ? 0 : cpu_to_le32(0x01);
+		bdl += 4;
+		azx_dev->frags++;
+		ofs += chunk;
+	}
+	*bdlp = bdl;
+	return ofs;
+}
+
+/*
  * set up BDL entries
  */
 static int azx_setup_periods(struct snd_pcm_substream *substream,
 			     struct azx_dev *azx_dev)
 {
-	struct snd_sg_buf *sgbuf = snd_pcm_substream_sgbuf(substream);
 	u32 *bdl;
 	int i, ofs, periods, period_bytes;
+	int pos_adj = 0;
 
 	/* reset BDL address */
 	azx_sd_writel(azx_dev, SD_BDLPL, 0);
@@ -998,39 +1046,44 @@ static int azx_setup_periods(struct snd_pcm_substream *substream,
 	bdl = (u32 *)azx_dev->bdl.area;
 	ofs = 0;
 	azx_dev->frags = 0;
-	for (i = 0; i < periods; i++) {
-		int size, rest;
-		if (i >= AZX_MAX_BDL_ENTRIES) {
-			snd_printk(KERN_ERR "Too many BDL entries: "
-				   "buffer=%d, period=%d\n",
-				   azx_dev->bufsize, period_bytes);
-			/* reset */
-			azx_sd_writel(azx_dev, SD_BDLPL, 0);
-			azx_sd_writel(azx_dev, SD_BDLPU, 0);
-			return -EINVAL;
+	azx_dev->irq_ignore = 0;
+	if (bdl_pos_adj > 0) {
+		struct snd_pcm_runtime *runtime = substream->runtime;
+		pos_adj = (bdl_pos_adj * runtime->rate + 47999) / 48000;
+		if (!pos_adj)
+			pos_adj = 1;
+		pos_adj = frames_to_bytes(runtime, pos_adj);
+		if (pos_adj >= period_bytes) {
+			snd_printk(KERN_WARNING "Too big adjustment %d\n",
+				   bdl_pos_adj);
+			pos_adj = 0;
+		} else {
+			ofs = setup_bdle(substream, azx_dev,
+					 &bdl, ofs, pos_adj, 1);
+			if (ofs < 0)
+				goto error;
+			azx_dev->irq_ignore = 1;
 		}
-		rest = period_bytes;
-		do {
-			dma_addr_t addr = snd_pcm_sgbuf_get_addr(sgbuf, ofs);
-			/* program the address field of the BDL entry */
-			bdl[0] = cpu_to_le32((u32)addr);
-			bdl[1] = cpu_to_le32(upper_32bit(addr));
-			/* program the size field of the BDL entry */
-			size = PAGE_SIZE - (ofs % PAGE_SIZE);
-			if (rest < size)
-				size = rest;
-			bdl[2] = cpu_to_le32(size);
-			/* program the IOC to enable interrupt
-			 * only when the whole fragment is processed
-			 */
-			rest -= size;
-			bdl[3] = rest ? 0 : cpu_to_le32(0x01);
-			bdl += 4;
-			azx_dev->frags++;
-			ofs += size;
-		} while (rest > 0);
+	}
+	for (i = 0; i < periods; i++) {
+		if (i == periods - 1 && pos_adj)
+			ofs = setup_bdle(substream, azx_dev, &bdl, ofs,
+					 period_bytes - pos_adj, 0);
+		else
+			ofs = setup_bdle(substream, azx_dev, &bdl, ofs,
+					 period_bytes, 1);
+		if (ofs < 0)
+			goto error;
 	}
 	return 0;
+
+ error:
+	snd_printk(KERN_ERR "Too many BDL entries: buffer=%d, period=%d\n",
+		   azx_dev->bufsize, period_bytes);
+	/* reset */
+	azx_sd_writel(azx_dev, SD_BDLPL, 0);
+	azx_sd_writel(azx_dev, SD_BDLPU, 0);
+	return -EINVAL;
 }
 
 /*

Re: [PATCH] Fix buffer position for ATI SB4x0

[Mauro Carvalho Chehab]

On Mon, 09 Jun 2008 15:17:41 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> At Sat, 07 Jun 2008 10:27:54 +0200,
> I wrote:
> > 
> > At Fri, 6 Jun 2008 15:31:45 -0300,
> > Mauro Carvalho Chehab wrote:
> > > 
> > > On Fri, 06 Jun 2008 18:49:28 +0200
> > > Takashi Iwai <tiwai@suse.de> wrote:
> > > 
> > > > At Fri, 6 Jun 2008 11:52:32 -0300,
> > > > Mauro Carvalho Chehab wrote:
> > > > > 
> > > > > On Thu, 29 May 2008 16:20:21 +0200
> > > > > Takashi Iwai <tiwai@suse.de> wrote:
> > > > > 
> > > > > > At Thu, 29 May 2008 11:10:22 -0300,
> > > > > > Mauro Carvalho Chehab wrote:
> > > > > > > 
> > > > > > > ATI SB4x0 doesn't need any fix at position.
> > > > > > 
> > > > > > It's not about the position fixing but whether to use the
> > > > > > position-buffer.  The devices on the blacklist are the ones that have
> > > > > > no position buffer.  So, it would fall into LPIB mode, and this list
> > > > > > avoids it from the beginning.
> > > > > > 
> > > > > > > This patch solves the issue of receiving several clicks during capture on those
> > > > > > > devices.
> > > > > > > 
> > > > > > > Tested with a Gateway Notebook MX-6453.
> > > > > > 
> > > > > > The click noise is often a different problem.  Did you already try
> > > > > > the patch below?
> > > > > 
> > > > > The click seems associated to some residual samples inside the buffer. Here it
> > > > > is a sample of the king of click noise I'm hearing here (captured from CD input entry):
> > > > > 	http://linuxtv.org/~mchehab/snd.ogg
> > > > 
> > > > (I didn't check the ogg file yet, and just a wild guess)
> > > > 
> > > > Is it with dsnoop plugin?  With "default" PCM, ALSA uses dsnoop for
> > > > capture to allow multiplexing for HD-audio.  Does it happen with "hw"
> > > > or "plughw" PCM?
> > > 
> > > Results with the cdplay:
> > > 
> > > With "default" PCM (both with and without mmap):
> > > 	several clicks per second, very high clicks;
> > > 	(like a very risky analog disk)
> > 
> > Hm, it's obviously a problem.  I couldn't reproduce it on my machine
> > with HD-audio, so it might be controller/codec-specific, though.
> > 
> > > With "hw" or "plughw" (no mmap):
> > > 	less clicks (something like two clicks or three per second), with less volume.
> > > 	both hw and plughw produces the same effect.
> > 
> > If it works with hw, plughw should have no effect (i.e. no conversion
> > is done).  Thus it's logical that both hw and plughw have the same
> > result.
> > 
> > > With "hw" or "plughw" and mmap (-M):
> > > 	high quality. no noticeable clicks.
> > > 
> > > So, it seems that there are two different issues: one with dsnoop and another
> > > with non-mmapped captures.
> > 
> > Interesting.  The fact that even "hw" without mmap causes occasional
> > click noises implies that there is certainly a problem with the DMA
> > position calculation.  The dsnoop has more problems likely because it
> > uses smaller period size and more periods than hw, I guess.
> > Still not sure why the mmap mode works.
> > 
> > Anyway, it means that the capture position is wrongly reported, maybe
> > just in a reverse way of the playback position -- a few samples ahead
> > than the real position.  Sigh, another workaround is needed.
> 
> The patch below adds another workaround for the DMA buffer position.
> Could you give it a try?  It adds as default one sample delay for
> issuing the interrupt so that the DMA pointer gets the right
> position.  The delay can be changed (even dynamically) via bdl_pos_adj
> module option.

I tried it here, but I didn't noticed any results. I've ranged the value from 1
to 317 (I had to stop/start record each time). I tried with the default PCM input,
since this way is easier to notice the click.

Cheers,
Mauro