Skip to content

Commit

Permalink
FROMLIST: ALSA: pcm: fix wait_time calculations
Browse files Browse the repository at this point in the history
... in wait_for_avail() and snd_pcm_drain().

t was calculated in seconds, so it would be pretty much always zero, to
be subsequently de-facto ignored due to being max(t, 10)'d. And then it
(i.e., 10) would be treated as secs, which doesn't seem right.

However, fixing it to properly calculate msecs would potentially cause
timeouts when using twice the period size for the default timeout (which
seems reasonable to me), so instead use the buffer size plus 10 percent
to be on the safe side ... but that still seems insufficient, presumably
because the hardware typically needs a moment to fire up. To compensate
for this, we up the minimal timeout to 100ms, which is still two orders
of magnitude less than the bogus minimum.

substream->wait_time was also misinterpreted as jiffies, despite being
documented as being in msecs. Only the soc/sof driver sets it - to 500,
which looks very much like msecs were intended.

Speaking of which, shouldn't snd_pcm_drain() also use substream->
wait_time?

As a drive-by, make the debug messages on timeout less confusing.

Tracked-On: OAM-113000
Signed-off-by: Oswald Buddenhagen <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Takashi Iwai <[email protected]>
  • Loading branch information
ossilator authored and JeevakaPrabu committed Jun 17, 2024
1 parent e8b774a commit 43c330e
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 10 deletions.
11 changes: 5 additions & 6 deletions sound/core/pcm_lib.c
Original file line number Diff line number Diff line change
Expand Up @@ -1912,15 +1912,14 @@ static int wait_for_avail(struct snd_pcm_substream *substream,
if (substream->wait_time) {
wait_time = substream->wait_time;
} else {
wait_time = 10;
wait_time = 100;

if (runtime->rate) {
long t = runtime->period_size * 2 /
runtime->rate;
long t = runtime->buffer_size * 1100 / runtime->rate;
wait_time = max(t, wait_time);
}
wait_time = msecs_to_jiffies(wait_time * 1000);
}
wait_time = msecs_to_jiffies(wait_time);
}

for (;;) {
Expand Down Expand Up @@ -1968,8 +1967,8 @@ static int wait_for_avail(struct snd_pcm_substream *substream,
}
if (!tout) {
pcm_dbg(substream->pcm,
"%s write error (DMA or IRQ trouble?)\n",
is_playback ? "playback" : "capture");
"%s timeout (DMA or IRQ trouble?)\n",
is_playback ? "playback write" : "capture read");
err = -EIO;
break;
}
Expand Down
8 changes: 4 additions & 4 deletions sound/core/pcm_native.c
Original file line number Diff line number Diff line change
Expand Up @@ -2170,12 +2170,12 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream,
if (runtime->no_period_wakeup)
tout = MAX_SCHEDULE_TIMEOUT;
else {
tout = 10;
tout = 100;
if (runtime->rate) {
long t = runtime->period_size * 2 / runtime->rate;
long t = runtime->buffer_size * 1100 / runtime->rate;
tout = max(t, tout);
}
tout = msecs_to_jiffies(tout * 1000);
tout = msecs_to_jiffies(tout);
}
tout = schedule_timeout(tout);

Expand All @@ -2198,7 +2198,7 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream,
result = -ESTRPIPE;
else {
dev_dbg(substream->pcm->card->dev,
"playback drain error (DMA or IRQ trouble?)\n");
"playback drain timeout (DMA or IRQ trouble?)\n");
snd_pcm_stop(substream, SNDRV_PCM_STATE_SETUP);
result = -EIO;
}
Expand Down

0 comments on commit 43c330e

Please sign in to comment.