On Tue, 11 May 2010, Takashi Iwai wrote:
At Tue, 11 May 2010 10:44:17 +0200 (CEST), Jaroslav Kysela wrote:
On Tue, 11 May 2010, Takashi Iwai wrote:
At Tue, 11 May 2010 10:28:50 +0200 (CEST), Jaroslav Kysela wrote:
On Mon, 10 May 2010, Jaroslav Kysela wrote:
Hi,
I would like to ask HDA gurus to check this patch (I will include it to my tree once acked). The patch uses WALLCLK from HDA chips (marked as required in the HDA specification) to check for bogus - too early - interrups which confuses the upper PCM layer (sound skipping issues).
More details about patch testing on problematic hardware can be found at:
No objections received, so I pushed this code to my master/devel branches.
Heh, that was quick, I had no time to review :)
The change looks good to me. Thanks for your fix (and rebasing).
One thing is that you removed bdl_pos_adj=0 check. This was there explicitly to avoid the delayed irq handling no matter whether the value is correct or not. I guess it's safer to take it back, or give any other way for user to avoid the delayed irq handling.
I see. But I need the pos % period_bytes check too. What about this change?
With bdl_pos_adj==0, it should return 1 (non-error), but otherwise this should be OK, I guess.
The problem with returning value 1 -> call snd_pcm_elapsed() - is that the upper layer will be most probably confused on broken hw. From logs in kernel bz#15912, the bad irq can be triggered at any time (I really don't know why INTSTS register is set).
The first wallclk check uses to check for first 2/3 of period and the pos % period_bytes check checks for second half of period. Writing this, I think that the extra check can be added to the second condition to do position related check for first next period only:
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 0a6c55b..d4d532a 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1890,9 +1890,8 @@ static int azx_position_ok(struct azx *chip, struct azx_dev *azx_dev) unsigned int pos; int stream;
- wallclk = azx_readl(chip, WALLCLK); - if ((wallclk - azx_dev->start_wallclk) < - (azx_dev->period_wallclk * 2) / 3) + wallclk = azx_readl(chip, WALLCLK) - azx_dev->start_wallclk; + if (wallclk < (azx_dev->period_wallclk * 2) / 3) return -1; /* bogus (too early) interrupt */
stream = azx_dev->substream->stream; @@ -1910,9 +1909,12 @@ static int azx_position_ok(struct azx *chip, struct azx_dev *azx_dev)
if (WARN_ONCE(!azx_dev->period_bytes, "hda-intel: zero azx_dev->period_bytes")) - return 0; /* this shouldn't happen! */ - if (pos % azx_dev->period_bytes > azx_dev->period_bytes / 2) - return 0; /* NG - it's below the period boundary */ + /* this shouldn't happen! */ + return bdl_pos_adj[chip->dev_index] ? 0 : -1; + if (wallclk <= azx_dev->period_wallclk && + pos % azx_dev->period_bytes > azx_dev->period_bytes / 2) + /* NG - it's below the first next period boundary */ + return bdl_pos_adj[chip->dev_index] ? 0 : -1; azx_dev->start_wallclk = wallclk; return 1; /* OK, it's fine */ }
I don't see any reason to call snd_pcm_elapsed() in bad time. It's better to return -1 (ignore) and wait for next interrupt.
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.