[alsa-devel] snd-hda-intel - wallclk patch
Jaroslav Kysela
perex at perex.cz
Tue May 11 11:07:05 CEST 2010
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:
>>>>>
>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=15912
>>>>
>>>> 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 at perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.
More information about the Alsa-devel
mailing list