[alsa-devel] Fixing snd_pcm_indirect_playback_transfer to support rewinds?
Hi,
I am been having problems using Pulse Audio on the Raspberry Pi for quite a while, in which audio stops playing if two programs want to play through Pulse simultaneously.
Or if one program already has finished playing, but you start the second within 5 seconds.
Thinking that was a problem in the snd_bcm2835 module, I created an issue in the github repo that houses the Raspberry Pi kernel fork about it in 2014: https://github.com/raspberrypi/linux/issues/688
However I now have the impression the problem is not in their module specific code, but that snd_pcm_indirect_playback_transfer() which the module uses, doesn't deal with rewinds correctly, which Pulse uses extensively.
Is there any effort being done to fix this?
I came across the "ALSA: pcm: Fix negative appl_ptr handling in pcm-indirect helpers" patch in this mailing list, so apparently the problem does is known to you guys.
However looking at this fragment of the patch:
==
+static inline int snd_pcm_indirect_playback_transfer(struct snd_pcm_substream *substream, struct snd_pcm_indirect *rec, snd_pcm_indirect_copy_t copy) @@ -56,6 +56,8 @@ snd_pcm_indirect_playback_transfer(struct snd_pcm_substream *substream, if (diff) { if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2)) diff += runtime->boundary; + if (diff < 0) + return -EINVAL; ==
It seems it is simply denying rewinds instead of making them work? Isn't there any way to make them work, instead of disabling functionality userspace seems to be expecting to have?
Yours sincerely,
Floris Bos
On Tue, 02 Jan 2018 13:10:14 +0100, Floris Bos wrote:
Hi,
I am been having problems using Pulse Audio on the Raspberry Pi for quite a while, in which audio stops playing if two programs want to play through Pulse simultaneously.
Or if one program already has finished playing, but you start the second within 5 seconds.
Thinking that was a problem in the snd_bcm2835 module, I created an issue in the github repo that houses the Raspberry Pi kernel fork about it in 2014: https://github.com/raspberrypi/linux/issues/688
However I now have the impression the problem is not in their module specific code, but that snd_pcm_indirect_playback_transfer() which the module uses, doesn't deal with rewinds correctly, which Pulse uses extensively.
Is there any effort being done to fix this?
I came across the "ALSA: pcm: Fix negative appl_ptr handling in pcm-indirect helpers" patch in this mailing list, so apparently the problem does is known to you guys.
However looking at this fragment of the patch:
==
+static inline int snd_pcm_indirect_playback_transfer(struct snd_pcm_substream *substream, struct snd_pcm_indirect *rec, snd_pcm_indirect_copy_t copy) @@ -56,6 +56,8 @@ snd_pcm_indirect_playback_transfer(struct snd_pcm_substream *substream, if (diff) { if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2)) diff += runtime->boundary; + if (diff < 0)
+ return -EINVAL;
It seems it is simply denying rewinds instead of making them work? Isn't there any way to make them work, instead of disabling functionality userspace seems to be expecting to have?
The rewind can't work with the indirect PCM mode due to its nature. In the indirect PCM operation, the data has been already copied, thus you can't go back beyond the point.
In general, the application can't expect that rewind always works on such a device. That said, it's rather a bug of PulseAudio, I'd say.
Maybe an easy workaround would be to disable the timer-based mode on PA, e.g. by adding SNDRV_PCM_INFO_BATCH flag to the PCM stream, a patch like below. You'd need a similar change for the downstream drivers.
Not tested at all, and it might not work. Let me know if this workaround really helps, so that I can upstream the changes.
thanks,
Takashi
--- diff --git a/sound/drivers/ml403-ac97cr.c b/sound/drivers/ml403-ac97cr.c index 18fd12996cf7..a33099a17e2c 100644 --- a/sound/drivers/ml403-ac97cr.c +++ b/sound/drivers/ml403-ac97cr.c @@ -376,6 +376,7 @@ struct snd_ml403_ac97cr { static const struct snd_pcm_hardware snd_ml403_ac97cr_playback = { .info = (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED | + SNDRV_PCM_INFO_BATCH | /* indirect PCM */ SNDRV_PCM_INFO_MMAP_VALID), .formats = SNDRV_PCM_FMTBIT_S16_BE, .rates = (SNDRV_PCM_RATE_CONTINUOUS | diff --git a/sound/mips/hal2.c b/sound/mips/hal2.c index 37d378a26a50..29d9bef2e1fd 100644 --- a/sound/mips/hal2.c +++ b/sound/mips/hal2.c @@ -500,6 +500,7 @@ static const struct snd_pcm_hardware hal2_pcm_hw = { .info = (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_INTERLEAVED | + SNDRV_PCM_INFO_BATCH | /* indirect PCM */ SNDRV_PCM_INFO_BLOCK_TRANSFER), .formats = SNDRV_PCM_FMTBIT_S16_BE, .rates = SNDRV_PCM_RATE_8000_48000, diff --git a/sound/pci/cs46xx/cs46xx_lib.c b/sound/pci/cs46xx/cs46xx_lib.c index 0020fd0efc46..f170494630e2 100644 --- a/sound/pci/cs46xx/cs46xx_lib.c +++ b/sound/pci/cs46xx/cs46xx_lib.c @@ -1442,6 +1442,7 @@ static const struct snd_pcm_hardware snd_cs46xx_playback = { .info = (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED | + SNDRV_PCM_INFO_BATCH | /* maybe indirect PCM */ SNDRV_PCM_INFO_BLOCK_TRANSFER /*|*/ /*SNDRV_PCM_INFO_RESUME*/), .formats = (SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_U8 | @@ -1464,6 +1465,7 @@ static const struct snd_pcm_hardware snd_cs46xx_capture = { .info = (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED | + SNDRV_PCM_INFO_BATCH | /* maybe indirect PCM */ SNDRV_PCM_INFO_BLOCK_TRANSFER /*|*/ /*SNDRV_PCM_INFO_RESUME*/), .formats = SNDRV_PCM_FMTBIT_S16_LE, diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c index 2683b9717215..ad08a528833d 100644 --- a/sound/pci/emu10k1/emupcm.c +++ b/sound/pci/emu10k1/emupcm.c @@ -1746,6 +1746,7 @@ static const struct snd_pcm_hardware snd_emu10k1_fx8010_playback = { .info = (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_RESUME | + SNDRV_PCM_INFO_BATCH | /* indirect PCM */ /* SNDRV_PCM_INFO_MMAP_VALID | */ SNDRV_PCM_INFO_PAUSE), .formats = SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S16_LE, .rates = SNDRV_PCM_RATE_48000, diff --git a/sound/pci/rme32.c b/sound/pci/rme32.c index f0906ba416d4..9e60fdc6d9ed 100644 --- a/sound/pci/rme32.c +++ b/sound/pci/rme32.c @@ -370,6 +370,7 @@ static const struct snd_pcm_hardware snd_rme32_spdif_fd_info = { SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_PAUSE | + SNDRV_PCM_INFO_BATCH | /* indirect PCM */ SNDRV_PCM_INFO_SYNC_START), .formats = (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE), @@ -397,6 +398,7 @@ static const struct snd_pcm_hardware snd_rme32_adat_fd_info = SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_PAUSE | + SNDRV_PCM_INFO_BATCH | /* indirect PCM */ SNDRV_PCM_INFO_SYNC_START), .formats= SNDRV_PCM_FMTBIT_S16_LE, .rates = (SNDRV_PCM_RATE_44100 |
On 01/02/2018 02:49 PM, Takashi Iwai wrote:
It seems it is simply denying rewinds instead of making them work? Isn't there any way to make them work, instead of disabling functionality userspace seems to be expecting to have?
The rewind can't work with the indirect PCM mode due to its nature. In the indirect PCM operation, the data has been already copied, thus you can't go back beyond the point.
Ah, didn't know it consumed all data right away inside the function. The construct with sw_ready/hw_ready/hw_queue_size variables gave me the impression it could also only process a part and leave the rest for a next call.
BTW another thing that I noticed, is that it currently is only possible for a sound driver to report either success or failure from the ack callback. While the userspace documentation seems to suggest that partial success -in which a lower number of frames than requested is rewinded- is also a possibility.
https://www.alsa-project.org/alsa-doc/alsa-lib/group___p_c_m.html#ga6c66040d...
== Returns: a positive number for actual displacement otherwise a negative error code ==
In general, the application can't expect that rewind always works on such a device. That said, it's rather a bug of PulseAudio, I'd say.
Maybe an easy workaround would be to disable the timer-based mode on PA, e.g. by adding SNDRV_PCM_INFO_BATCH flag to the PCM stream, a patch like below. You'd need a similar change for the downstream drivers.
Will have a look at that later when I have more time.
Yours sincerely,
Floris Bos
On Tue, 02 Jan 2018 22:46:16 +0100, Floris Bos wrote:
On 01/02/2018 02:49 PM, Takashi Iwai wrote:
It seems it is simply denying rewinds instead of making them work? Isn't there any way to make them work, instead of disabling functionality userspace seems to be expecting to have?
The rewind can't work with the indirect PCM mode due to its nature. In the indirect PCM operation, the data has been already copied, thus you can't go back beyond the point.
Ah, didn't know it consumed all data right away inside the function. The construct with sw_ready/hw_ready/hw_queue_size variables gave me the impression it could also only process a part and leave the rest for a next call.
It's not always copying the whole, but in principle, the indirect PCM tries to copy the data from sw buffer to hw buffer as much as possible. That, of course, limits the availability of rewind in most cases.
BTW another thing that I noticed, is that it currently is only possible for a sound driver to report either success or failure from the ack callback. While the userspace documentation seems to suggest that partial success -in which a lower number of frames than requested is rewinded- is also a possibility.
Yes, but it's far more difficult to code in the current design. The whole rewind procedure is done only in appl_ptr and hw_ptr manipulation, and the only thing we can perform in addition is the call of ack callback, and it can give only success or failure.
Basically, if the error from rewind doesn't work, it's clearly an application bug that heavily relies on the feature that is never guaranteed to work. So the best would be to fix the application.
Meanwhile, PulseAudio is almost a single application that is actually using the rewind. Breaking it doesn't sound like a good idea.
If the workaround with SNDRV_PCM_INFO_BATCH doesn't seem working, maybe the patch like below would be a compromise.
Let me know if it works.
thanks,
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: pcm: Workaround for weird PulseAudio behavior on rewind error
The commit 9027c4639ef1 ("ALSA: pcm: Call ack() whenever appl_ptr is updated") introduced the possible error code returned from the PCM rewind ioctl. Basically the change was for handling the indirect PCM more correctly, but ironically, it caused rather a side-effect: PulseAudio gets pissed off when receiving an error from rewind, throws everything away and stops processing further, resulting in the silence.
It's clearly a failure in the application side, so the best would be to fix that bug in PA. OTOH, PA is mostly the only user of the rewind feature, so it's not good to slap the sole customer.
This patch tries to mitigate the situation: instead of returning an error, now the rewind ioctl returns zero when the driver can't rewind. It indicates that no rewind was performed, so the behavior is consistent, at least.
Fixes: 9027c4639ef1 ("ALSA: pcm: Call ack() whenever appl_ptr is updated") Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_native.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index a4d92e46c459..f08772568c17 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2580,7 +2580,7 @@ static snd_pcm_sframes_t forward_appl_ptr(struct snd_pcm_substream *substream, return ret < 0 ? ret : frames; }
-/* decrease the appl_ptr; returns the processed frames or a negative error */ +/* decrease the appl_ptr; returns the processed frames or zero for error */ static snd_pcm_sframes_t rewind_appl_ptr(struct snd_pcm_substream *substream, snd_pcm_uframes_t frames, snd_pcm_sframes_t avail) @@ -2597,7 +2597,12 @@ static snd_pcm_sframes_t rewind_appl_ptr(struct snd_pcm_substream *substream, if (appl_ptr < 0) appl_ptr += runtime->boundary; ret = pcm_lib_apply_appl_ptr(substream, appl_ptr); - return ret < 0 ? ret : frames; + /* NOTE: we return zero for errors because PulseAudio gets depressed + * upon receiving an error from rewind ioctl and stops processing + * any longer. Returning zero means that no rewind is done, so + * it's not absolutely wrong to answer like that. + */ + return ret < 0 ? 0 : frames; }
static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *substream,
participants (2)
-
Floris Bos -
Takashi Iwai