[alsa-devel] [PATCH] hda_intel: Fixes distorted recording on US15W chipset
The HDA controller in US15W (Poulsbo) reports inaccurate position values for capture streams when using the LPIB read method, resulting in distorted recordings. However, using the position buffer is broken for playback streams, resulting in a fallback to the LPIB method with the current driver. This patch works around the issue by independently detecting the read position method for capture and playback streams. The patch will not have any effect if the position fix method is explicitly set. Signed-off-by: Shahin Ghazinouri shahin.ghazinouri@pelagicore.com --- --- linux-2.6.34-rc6/sound/pci/hda/hda_intel.c 2010-04-30 05:02:05.000000000 +0200 +++ linux-2.6.34-rc6_patch/sound/pci/hda/hda_intel.c 2010-04-30 11:31:57.000000000 +0200 @@ -425,7 +425,8 @@ struct snd_dma_buffer posbuf;
/* flags */ - int position_fix; + int playback_position_fix; + int capture_position_fix; int poll_count; unsigned int running :1; unsigned int initialized :1; @@ -1302,8 +1303,10 @@ azx_sd_writel(azx_dev, SD_BDLPU, upper_32_bits(azx_dev->bdl.addr));
/* enable the position buffer */ - if (chip->position_fix == POS_FIX_POSBUF || - chip->position_fix == POS_FIX_AUTO || + if (chip->capture_position_fix == POS_FIX_POSBUF || + chip->capture_position_fix == POS_FIX_AUTO || + chip->playback_position_fix == POS_FIX_POSBUF || + chip->playback_position_fix == POS_FIX_AUTO || chip->via_dmapos_patch) { if (!(azx_readl(chip, DPLBASE) & ICH6_DPLBASE_ENABLE)) azx_writel(chip, DPLBASE, @@ -1843,13 +1846,24 @@
if (chip->via_dmapos_patch) pos = azx_via_get_position(chip, azx_dev); - else if (chip->position_fix == POS_FIX_POSBUF || - chip->position_fix == POS_FIX_AUTO) { - /* use the position buffer */ - pos = le32_to_cpu(*azx_dev->posbuf); + else if (azx_dev->index < chip->playback_index_offset) { + /* Capture stream */ + if (chip->capture_position_fix == POS_FIX_POSBUF || + chip->capture_position_fix == POS_FIX_AUTO) + /* use the position buffer */ + pos = le32_to_cpu(*azx_dev->posbuf); + else + /* read LPIB */ + pos = azx_sd_readl(azx_dev, SD_LPIB); } else { - /* read LPIB */ - pos = azx_sd_readl(azx_dev, SD_LPIB); + /* Playback stream */ + if (chip->playback_position_fix == POS_FIX_POSBUF || + chip->playback_position_fix == POS_FIX_AUTO) + /* use the position buffer */ + pos = le32_to_cpu(*azx_dev->posbuf); + else + /* read LPIB */ + pos = azx_sd_readl(azx_dev, SD_LPIB); } if (pos >= azx_dev->bufsize) pos = 0; @@ -1884,15 +1898,30 @@ azx_dev->start_flag = 0;
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_LPIB; - pos = azx_get_position(chip, azx_dev); - } else - chip->position_fix = POS_FIX_POSBUF; + if (azx_dev->index < chip->playback_index_offset) { + /* Capture stream */ + if (chip->capture_position_fix == POS_FIX_AUTO) { + if (!pos) { + printk(KERN_WARNING + "hda-intel: Invalid position buffer, " + "using LPIB read method instead.\n"); + chip->capture_position_fix = POS_FIX_LPIB; + pos = azx_get_position(chip, azx_dev); + } else + chip->capture_position_fix = POS_FIX_POSBUF; + } + } else { + /* Playback stream */ + if (chip->playback_position_fix == POS_FIX_AUTO) { + if (!pos) { + printk(KERN_WARNING + "hda-intel: Invalid position buffer, " + "using LPIB read method instead.\n"); + chip->playback_position_fix = POS_FIX_LPIB; + pos = azx_get_position(chip, azx_dev); + } else + chip->playback_position_fix = POS_FIX_POSBUF; + } }
if (!bdl_pos_adj[chip->dev_index]) @@ -2431,7 +2460,10 @@ chip->dev_index = dev; INIT_WORK(&chip->irq_pending_work, azx_irq_pending_work);
- chip->position_fix = check_position_fix(chip, position_fix[dev]); + chip->capture_position_fix = + check_position_fix(chip, position_fix[dev]); + chip->playback_position_fix = + check_position_fix(chip, position_fix[dev]); check_probe_mask(chip, dev);
chip->single_cmd = single_cmd;
At Mon, 3 May 2010 16:46:56 +0200, Shahin Ghazinouri wrote:
The HDA controller in US15W (Poulsbo) reports inaccurate position values for capture streams when using the LPIB read method, resulting in distorted recordings. However, using the position buffer is broken for playback streams, resulting in a fallback to the LPIB method with the current driver. This patch works around the issue by independently detecting the read position method for capture and playback streams. The patch will not have any effect if the position fix method is explicitly set. Signed-off-by: Shahin Ghazinouri shahin.ghazinouri@pelagicore.com
The changes look OK, but the code can be a bit more simplified. Could you check whether the patch below works?
thanks,
Takashi
--- diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 236b4ca..cad9b70 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -425,7 +425,7 @@ struct azx { struct snd_dma_buffer posbuf;
/* flags */ - int position_fix; + int position_fix[2]; /* for both playback/capture streams */ int poll_count; unsigned int running :1; unsigned int initialized :1; @@ -1306,8 +1306,10 @@ static int azx_setup_controller(struct azx *chip, struct azx_dev *azx_dev) azx_sd_writel(azx_dev, SD_BDLPU, upper_32_bits(azx_dev->bdl.addr));
/* enable the position buffer */ - if (chip->position_fix == POS_FIX_POSBUF || - chip->position_fix == POS_FIX_AUTO || + if (chip->position_fix[0] == POS_FIX_POSBUF || + chip->position_fix[0] == POS_FIX_AUTO || + chip->position_fix[1] == POS_FIX_POSBUF || + chip->position_fix[1] == POS_FIX_AUTO || chip->via_dmapos_patch) { if (!(azx_readl(chip, DPLBASE) & ICH6_DPLBASE_ENABLE)) azx_writel(chip, DPLBASE, @@ -1847,13 +1849,16 @@ static unsigned int azx_get_position(struct azx *chip,
if (chip->via_dmapos_patch) pos = azx_via_get_position(chip, azx_dev); - else if (chip->position_fix == POS_FIX_POSBUF || - chip->position_fix == POS_FIX_AUTO) { - /* use the position buffer */ - pos = le32_to_cpu(*azx_dev->posbuf); - } else { - /* read LPIB */ - pos = azx_sd_readl(azx_dev, SD_LPIB); + else { + int stream = azx_dev->substream->stream; + if (chip->position_fix[stream] == POS_FIX_POSBUF || + chip->position_fix[stream] == POS_FIX_AUTO) { + /* use the position buffer */ + pos = le32_to_cpu(*azx_dev->posbuf); + } else { + /* read LPIB */ + pos = azx_sd_readl(azx_dev, SD_LPIB); + } } if (pos >= azx_dev->bufsize) pos = 0; @@ -1881,22 +1886,24 @@ static snd_pcm_uframes_t azx_pcm_pointer(struct snd_pcm_substream *substream) static int azx_position_ok(struct azx *chip, struct azx_dev *azx_dev) { unsigned int pos; + int stream;
if (azx_dev->start_flag && time_before_eq(jiffies, azx_dev->start_jiffies)) return -1; /* bogus (too early) interrupt */ azx_dev->start_flag = 0;
+ stream = azx_dev->substream->stream; pos = azx_get_position(chip, azx_dev); - if (chip->position_fix == POS_FIX_AUTO) { + if (chip->position_fix[stream] == POS_FIX_AUTO) { if (!pos) { printk(KERN_WARNING "hda-intel: Invalid position buffer, " "using LPIB read method instead.\n"); - chip->position_fix = POS_FIX_LPIB; + chip->position_fix[stream] = POS_FIX_LPIB; pos = azx_get_position(chip, azx_dev); } else - chip->position_fix = POS_FIX_POSBUF; + chip->position_fix[stream] = POS_FIX_POSBUF; }
if (!bdl_pos_adj[chip->dev_index]) @@ -2435,7 +2442,8 @@ static int __devinit azx_create(struct snd_card *card, struct pci_dev *pci, chip->dev_index = dev; INIT_WORK(&chip->irq_pending_work, azx_irq_pending_work);
- chip->position_fix = check_position_fix(chip, position_fix[dev]); + chip->position_fix[0] = chip->position_fix[1] = + check_position_fix(chip, position_fix[dev]); check_probe_mask(chip, dev);
chip->single_cmd = single_cmd;
Hi,
I've now verified the patch below works and it appears functionally identical.
Regards, Shahin
On 10 maj 2010, at 10.37, Takashi Iwai wrote:
The changes look OK, but the code can be a bit more simplified. Could you check whether the patch below works?
thanks,
Takashi
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 236b4ca..cad9b70 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -425,7 +425,7 @@ struct azx { struct snd_dma_buffer posbuf;
/* flags */
- int position_fix;
- int position_fix[2]; /* for both playback/capture streams */ int poll_count; unsigned int running :1; unsigned int initialized :1;
@@ -1306,8 +1306,10 @@ static int azx_setup_controller(struct azx *chip, struct azx_dev *azx_dev) azx_sd_writel(azx_dev, SD_BDLPU, upper_32_bits(azx_dev->bdl.addr));
/* enable the position buffer */
- if (chip->position_fix == POS_FIX_POSBUF ||
chip->position_fix == POS_FIX_AUTO ||
- if (chip->position_fix[0] == POS_FIX_POSBUF ||
chip->position_fix[0] == POS_FIX_AUTO ||
chip->position_fix[1] == POS_FIX_POSBUF ||
if (!(azx_readl(chip, DPLBASE) & ICH6_DPLBASE_ENABLE)) azx_writel(chip, DPLBASE,chip->position_fix[1] == POS_FIX_AUTO || chip->via_dmapos_patch) {
@@ -1847,13 +1849,16 @@ static unsigned int azx_get_position(struct azx *chip,
if (chip->via_dmapos_patch) pos = azx_via_get_position(chip, azx_dev);
- else if (chip->position_fix == POS_FIX_POSBUF ||
chip->position_fix == POS_FIX_AUTO) {
/* use the position buffer */
pos = le32_to_cpu(*azx_dev->posbuf);
- } else {
/* read LPIB */
pos = azx_sd_readl(azx_dev, SD_LPIB);
- else {
int stream = azx_dev->substream->stream;
if (chip->position_fix[stream] == POS_FIX_POSBUF ||
chip->position_fix[stream] == POS_FIX_AUTO) {
/* use the position buffer */
pos = le32_to_cpu(*azx_dev->posbuf);
} else {
/* read LPIB */
pos = azx_sd_readl(azx_dev, SD_LPIB);
} if (pos >= azx_dev->bufsize) pos = 0;}
@@ -1881,22 +1886,24 @@ static snd_pcm_uframes_t azx_pcm_pointer(struct snd_pcm_substream *substream) static int azx_position_ok(struct azx *chip, struct azx_dev *azx_dev) { unsigned int pos;
int stream;
if (azx_dev->start_flag && time_before_eq(jiffies, azx_dev->start_jiffies)) return -1; /* bogus (too early) interrupt */ azx_dev->start_flag = 0;
stream = azx_dev->substream->stream; pos = azx_get_position(chip, azx_dev);
- if (chip->position_fix == POS_FIX_AUTO) {
- if (chip->position_fix[stream] == POS_FIX_AUTO) { if (!pos) { printk(KERN_WARNING "hda-intel: Invalid position buffer, " "using LPIB read method instead.\n");
chip->position_fix = POS_FIX_LPIB;
} elsechip->position_fix[stream] = POS_FIX_LPIB; pos = azx_get_position(chip, azx_dev);
chip->position_fix = POS_FIX_POSBUF;
chip->position_fix[stream] = POS_FIX_POSBUF;
}
if (!bdl_pos_adj[chip->dev_index])
@@ -2435,7 +2442,8 @@ static int __devinit azx_create(struct snd_card *card, struct pci_dev *pci, chip->dev_index = dev; INIT_WORK(&chip->irq_pending_work, azx_irq_pending_work);
- chip->position_fix = check_position_fix(chip, position_fix[dev]);
chip->position_fix[0] = chip->position_fix[1] =
check_position_fix(chip, position_fix[dev]);
check_probe_mask(chip, dev);
chip->single_cmd = single_cmd;
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
At Mon, 10 May 2010 19:40:27 +0200, Shahin Ghazinouri wrote:
Hi,
I've now verified the patch below works and it appears functionally identical.
OK, now applied. Thanks!
Takashi
Regards, Shahin
On 10 maj 2010, at 10.37, Takashi Iwai wrote:
The changes look OK, but the code can be a bit more simplified. Could you check whether the patch below works?
thanks,
Takashi
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 236b4ca..cad9b70 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -425,7 +425,7 @@ struct azx { struct snd_dma_buffer posbuf;
/* flags */
- int position_fix;
- int position_fix[2]; /* for both playback/capture streams */ int poll_count; unsigned int running :1; unsigned int initialized :1;
@@ -1306,8 +1306,10 @@ static int azx_setup_controller(struct azx *chip, struct azx_dev *azx_dev) azx_sd_writel(azx_dev, SD_BDLPU, upper_32_bits(azx_dev->bdl.addr));
/* enable the position buffer */
- if (chip->position_fix == POS_FIX_POSBUF ||
chip->position_fix == POS_FIX_AUTO ||
- if (chip->position_fix[0] == POS_FIX_POSBUF ||
chip->position_fix[0] == POS_FIX_AUTO ||
chip->position_fix[1] == POS_FIX_POSBUF ||
if (!(azx_readl(chip, DPLBASE) & ICH6_DPLBASE_ENABLE)) azx_writel(chip, DPLBASE,chip->position_fix[1] == POS_FIX_AUTO || chip->via_dmapos_patch) {
@@ -1847,13 +1849,16 @@ static unsigned int azx_get_position(struct azx *chip,
if (chip->via_dmapos_patch) pos = azx_via_get_position(chip, azx_dev);
- else if (chip->position_fix == POS_FIX_POSBUF ||
chip->position_fix == POS_FIX_AUTO) {
/* use the position buffer */
pos = le32_to_cpu(*azx_dev->posbuf);
- } else {
/* read LPIB */
pos = azx_sd_readl(azx_dev, SD_LPIB);
- else {
int stream = azx_dev->substream->stream;
if (chip->position_fix[stream] == POS_FIX_POSBUF ||
chip->position_fix[stream] == POS_FIX_AUTO) {
/* use the position buffer */
pos = le32_to_cpu(*azx_dev->posbuf);
} else {
/* read LPIB */
pos = azx_sd_readl(azx_dev, SD_LPIB);
} if (pos >= azx_dev->bufsize) pos = 0;}
@@ -1881,22 +1886,24 @@ static snd_pcm_uframes_t azx_pcm_pointer(struct snd_pcm_substream *substream) static int azx_position_ok(struct azx *chip, struct azx_dev *azx_dev) { unsigned int pos;
int stream;
if (azx_dev->start_flag && time_before_eq(jiffies, azx_dev->start_jiffies)) return -1; /* bogus (too early) interrupt */ azx_dev->start_flag = 0;
stream = azx_dev->substream->stream; pos = azx_get_position(chip, azx_dev);
- if (chip->position_fix == POS_FIX_AUTO) {
- if (chip->position_fix[stream] == POS_FIX_AUTO) { if (!pos) { printk(KERN_WARNING "hda-intel: Invalid position buffer, " "using LPIB read method instead.\n");
chip->position_fix = POS_FIX_LPIB;
} elsechip->position_fix[stream] = POS_FIX_LPIB; pos = azx_get_position(chip, azx_dev);
chip->position_fix = POS_FIX_POSBUF;
chip->position_fix[stream] = POS_FIX_POSBUF;
}
if (!bdl_pos_adj[chip->dev_index])
@@ -2435,7 +2442,8 @@ static int __devinit azx_create(struct snd_card *card, struct pci_dev *pci, chip->dev_index = dev; INIT_WORK(&chip->irq_pending_work, azx_irq_pending_work);
- chip->position_fix = check_position_fix(chip, position_fix[dev]);
chip->position_fix[0] = chip->position_fix[1] =
check_position_fix(chip, position_fix[dev]);
check_probe_mask(chip, dev);
chip->single_cmd = single_cmd;
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
participants (2)
-
Shahin Ghazinouri
-
Takashi Iwai