Re: [alsa-devel] Fixing snd_pcm_indirect_playback_transfer to support rewinds?

2 Jan 2018

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 |

    

Re: [alsa-devel] Fixing snd_pcm_indirect_playback_transfer to support rewinds?

Takashi Iwai

+ return -EINVAL;