| Message ID | 1461746959-11443-1-git-send-email-peda@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 27, 2016 at 10:49:19AM +0200, Peter Rosin wrote: > The below program fails on a dai link with symmetric rates without this > patch. The patch makes it work. You've not articulated the problem you're trying to fix here, what in concrete terms is the program trying to accomplish and why should it succeed? > if ((fd = open("/dev/dsp", O_WRONLY, 0)) == -1) { > perror("open"); > return 1; > } This is using the OSS interfaces which really haven't ever been especially supported for ASoC. > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { > + if (!cpu_dai->capture_active) > + return 0; > + } else if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) { > + if (!cpu_dai->playback_active) > + return 0; > + } > + > rate = params_rate(params); > channels = params_channels(params); > sample_bits = snd_pcm_format_physical_width(params_format(params)); This means we've opened up a race where the stream is configured but not started where the opposite direction can configure a different setup. Since starting both directions very close together is a common operation it seems likely to cause issues.
On 2016-04-27 18:15, Mark Brown wrote: > On Wed, Apr 27, 2016 at 10:49:19AM +0200, Peter Rosin wrote: > >> The below program fails on a dai link with symmetric rates without this >> patch. The patch makes it work. > > You've not articulated the problem you're trying to fix here, what in > concrete terms is the program trying to accomplish and why should it > succeed? It is opening an OSS fd, and setting some parameters, but changing the speed fails (in this case, since the codec dai has .symmetric_rates). As far as I know this is how you specify a specific speed when using OSS and it is simply not possibly when one of the involved dais is symmetric in any way. Which is silly since there is only one stream, so symmetry should not be an issue. >> if ((fd = open("/dev/dsp", O_WRONLY, 0)) == -1) { >> perror("open"); >> return 1; >> } > > This is using the OSS interfaces which really haven't ever been > especially supported for ASoC. Apparently, how should I know? >> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { >> + if (!cpu_dai->capture_active) >> + return 0; >> + } else if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) { >> + if (!cpu_dai->playback_active) >> + return 0; >> + } >> + >> rate = params_rate(params); >> channels = params_channels(params); >> sample_bits = snd_pcm_format_physical_width(params_format(params)); > > This means we've opened up a race where the stream is configured but not > started where the opposite direction can configure a different setup. > Since starting both directions very close together is a common operation > it seems likely to cause issues. Ouch, that's no good. Scrap the patch. I haven't looked really closely at the userspace side of this, but the big picture is that we're using a (closed source) library that in its documentation has an example where they open /dev/dsp like this and feeds the fd to the lib. We are simply replicating that in our code. I don't know if the library does anything OSSy with the fd, or if it would be equally happy with an ALSA fd. I'll investigate that... Cheers, Peter
On Wed, Apr 27, 2016 at 10:45:48PM +0200, Peter Rosin wrote: > On 2016-04-27 18:15, Mark Brown wrote: > > You've not articulated the problem you're trying to fix here, what in > > concrete terms is the program trying to accomplish and why should it > > succeed? > It is opening an OSS fd, and setting some parameters, but changing > the speed fails (in this case, since the codec dai has .symmetric_rates). > As far as I know this is how you specify a specific speed when using > OSS and it is simply not possibly when one of the involved dais is > symmetric in any way. Which is silly since there is only one stream, > so symmetry should not be an issue. Lots of things just don't work with OSS, we stopped caring about it years ago. The configuration mechanism just doesn't play nicely with dynamic constraints at all. > >> if ((fd = open("/dev/dsp", O_WRONLY, 0)) == -1) { > >> perror("open"); > >> return 1; > >> } > > This is using the OSS interfaces which really haven't ever been > > especially supported for ASoC. > Apparently, how should I know? OSS in general had been deprecated for 5-10 years. > I haven't looked really closely at the userspace side of this, but the > big picture is that we're using a (closed source) library that in its > documentation has an example where they open /dev/dsp like this and > feeds the fd to the lib. We are simply replicating that in our code. > I don't know if the library does anything OSSy with the fd, or if it > would be equally happy with an ALSA fd. If it's using OSS it's unlikely to work with ALSA, the two ABIs are completely different. You need to talk to your proprietary software vendor about updating their software. There are also some userspace utilities that try to do the remapping by wrapping things there but I can't remember the names off the top of my head, it's been a long time.
On Thu, 28 Apr 2016 12:38:22 +0200, Mark Brown wrote: > > On Wed, Apr 27, 2016 at 10:45:48PM +0200, Peter Rosin wrote: > > On 2016-04-27 18:15, Mark Brown wrote: > > > > You've not articulated the problem you're trying to fix here, what in > > > concrete terms is the program trying to accomplish and why should it > > > succeed? > > > It is opening an OSS fd, and setting some parameters, but changing > > the speed fails (in this case, since the codec dai has .symmetric_rates). > > As far as I know this is how you specify a specific speed when using > > OSS and it is simply not possibly when one of the involved dais is > > symmetric in any way. Which is silly since there is only one stream, > > so symmetry should not be an issue. > > Lots of things just don't work with OSS, we stopped caring about it > years ago. The configuration mechanism just doesn't play nicely with > dynamic constraints at all. True, but looking at the code there, I wonder how only OSS hits. Does ALSA-native API really work as is...? > > >> if ((fd = open("/dev/dsp", O_WRONLY, 0)) == -1) { > > >> perror("open"); > > >> return 1; > > >> } > > > > This is using the OSS interfaces which really haven't ever been > > > especially supported for ASoC. > > > Apparently, how should I know? > > OSS in general had been deprecated for 5-10 years. > > > I haven't looked really closely at the userspace side of this, but the > > big picture is that we're using a (closed source) library that in its > > documentation has an example where they open /dev/dsp like this and > > feeds the fd to the lib. We are simply replicating that in our code. > > I don't know if the library does anything OSSy with the fd, or if it > > would be equally happy with an ALSA fd. > > If it's using OSS it's unlikely to work with ALSA, the two ABIs are > completely different. You need to talk to your proprietary software > vendor about updating their software. There are also some userspace > utilities that try to do the remapping by wrapping things there but I > can't remember the names off the top of my head, it's been a long time. There are a couple of ways. A simple one is with alsa-oss LD_PRELOAD wrapper, and another is via CUSE. Also, PA has own LD_PRELOAD wrapper. Takashi
On Thu, Apr 28, 2016 at 01:03:22PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > Lots of things just don't work with OSS, we stopped caring about it > > years ago. The configuration mechanism just doesn't play nicely with > > dynamic constraints at all. > True, but looking at the code there, I wonder how only OSS hits. > Does ALSA-native API really work as is...? We've had the code for years without anyone reporting issues so... the idiomatic ALSA thing is to set everything in one call which helps a lot.
On Thu, 28 Apr 2016 13:10:01 +0200, Mark Brown wrote: > > On Thu, Apr 28, 2016 at 01:03:22PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > Lots of things just don't work with OSS, we stopped caring about it > > > years ago. The configuration mechanism just doesn't play nicely with > > > dynamic constraints at all. > > > True, but looking at the code there, I wonder how only OSS hits. > > Does ALSA-native API really work as is...? > > We've had the code for years without anyone reporting issues so... the > idiomatic ALSA thing is to set everything in one call which helps a lot. Yes, most of such problems come from the inconsistent hw constraints, and one-shot configuration helps indeed. But, in this case, it appears more like an overlooked case to me. BTW, this reminds me of another question: don't we have a dummy ASoC driver like snd-dummy? It would be convenient for a casual API testing. thanks, Takashi
On Thu, Apr 28, 2016 at 01:26:49PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > We've had the code for years without anyone reporting issues so... the > > idiomatic ALSA thing is to set everything in one call which helps a lot. > Yes, most of such problems come from the inconsistent hw constraints, > and one-shot configuration helps indeed. But, in this case, it > appears more like an overlooked case to me. If there's a useful report with the ALSA API then let's look into that - if people can only see this with the OSS API and can't articulate an actual ALSA API level problem then I'm struggling to care. Drivers that do parameter validation tend to fail with OSS anyway as it tries to set partially configured hw_params with invalid values in them and it's not clear to me that this isn't the same thing happening again. > BTW, this reminds me of another question: don't we have a dummy ASoC > driver like snd-dummy? It would be convenient for a casual API > testing. No, someone would need to write a series of dummy drivers that exercised the various API features (almost all of which are kernel internal) and then instantiated them but nobody stepped up and did that yet.
On Thu, 28 Apr 2016 13:41:50 +0200, Mark Brown wrote: > > On Thu, Apr 28, 2016 at 01:26:49PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > We've had the code for years without anyone reporting issues so... the > > > idiomatic ALSA thing is to set everything in one call which helps a lot. > > > Yes, most of such problems come from the inconsistent hw constraints, > > and one-shot configuration helps indeed. But, in this case, it > > appears more like an overlooked case to me. > > If there's a useful report with the ALSA API then let's look into that - > if people can only see this with the OSS API and can't articulate an > actual ALSA API level problem then I'm struggling to care. Drivers that > do parameter validation tend to fail with OSS anyway as it tries to set > partially configured hw_params with invalid values in them and it's not > clear to me that this isn't the same thing happening again. Yeah, that's why I asked whether this really doesn't happen in ALSA native API. > > BTW, this reminds me of another question: don't we have a dummy ASoC > > driver like snd-dummy? It would be convenient for a casual API > > testing. > > No, someone would need to write a series of dummy drivers that exercised > the various API features (almost all of which are kernel internal) and > then instantiated them but nobody stepped up and did that yet. Well, we don't support all API features there. Developer can modify the existing code and test it quickly if needed at any time once when the base code is present. So, yeah, we need a volunteer! Takashi
On Thu, Apr 28, 2016 at 02:02:21PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > On Thu, Apr 28, 2016 at 01:26:49PM +0200, Takashi Iwai wrote: > > > BTW, this reminds me of another question: don't we have a dummy ASoC > > > driver like snd-dummy? It would be convenient for a casual API > > > testing. > > No, someone would need to write a series of dummy drivers that exercised > > the various API features (almost all of which are kernel internal) and > > then instantiated them but nobody stepped up and did that yet. > Well, we don't support all API features there. Developer can modify > the existing code and test it quickly if needed at any time once when Yeah. Though the main use case would be making sure changes hadn't broken any of the more obscure DAPM features (at least, that's the main place where we seem to have issues) so... > the base code is present. So, yeah, we need a volunteer! No problem then :)
On 2016-04-28 12:38, Mark Brown wrote: > On Wed, Apr 27, 2016 at 10:45:48PM +0200, Peter Rosin wrote: >> I haven't looked really closely at the userspace side of this, but the >> big picture is that we're using a (closed source) library that in its >> documentation has an example where they open /dev/dsp like this and >> feeds the fd to the lib. We are simply replicating that in our code. >> I don't know if the library does anything OSSy with the fd, or if it >> would be equally happy with an ALSA fd. > > If it's using OSS it's unlikely to work with ALSA, the two ABIs are > completely different. You need to talk to your proprietary software > vendor about updating their software. There are also some userspace > utilities that try to do the remapping by wrapping things there but I > can't remember the names off the top of my head, it's been a long time. Ok, I managed to hook it up with ALSA instead but am now bitten by a completely different problem. The code now does this (error checks elided, there are no errors reported in the real code which does check for errors): snd_pcm_open(&pcm, "default", SND_PCM_STREAM_PLAYBACK, 0); snd_pcm_set_params(pcm, SND_PCM_FORMAT_S16_LE, SND_PCM_ACCESS_RW_INTERLEAVED, 2, 22050, 0, 1000000); while (!end) { /* code filling in stereo_buf elided */ snd_pcm_writei(pcm, stereo_buf, sample_count); } snd_pcm_drain(pcm); snd_pcm_close(pcm); There is no rate control in this, the code relies on snd_pcm_writei to block if there is no room. This part appears to work as expected. sample_count is not constant, but it is mostly 4096 and never bigger than that. The problem is in the drain/close part. If I leave out snd_pcm_drain I naturally lose the tail of the output, but if I have it I get what I guess is a repeat of the content in some circular buffer. I.e. the very last part of the played sound is played twice(ice). If I instead write the generated samples to a file and play them with aplay, all is fine, so I appear to be misunderstanding the alsa api? Anything obviously wrong in the above code? Cheers, Peter
On Mon, May 02, 2016 at 09:43:53AM +0200, Peter Rosin wrote: > The problem is in the drain/close part. If I leave out snd_pcm_drain > I naturally lose the tail of the output, but if I have it I get what > I guess is a repeat of the content in some circular buffer. I.e. the > very last part of the played sound is played twice(ice). > If I instead write the generated samples to a file and play them > with aplay, all is fine, so I appear to be misunderstanding the > alsa api? > Anything obviously wrong in the above code? Look at how aplay handles this... it's a natural consequence of the hardware being free running. You need to either make the teardown non-blocking or play some silence at the end.
On Wed, 04 May 2016 18:49:22 +0200, Mark Brown wrote: > > On Mon, May 02, 2016 at 09:43:53AM +0200, Peter Rosin wrote: > > > The problem is in the drain/close part. If I leave out snd_pcm_drain > > I naturally lose the tail of the output, but if I have it I get what > > I guess is a repeat of the content in some circular buffer. I.e. the > > very last part of the played sound is played twice(ice). > > > If I instead write the generated samples to a file and play them > > with aplay, all is fine, so I appear to be misunderstanding the > > alsa api? > > > Anything obviously wrong in the above code? > > Look at how aplay handles this... it's a natural consequence of the > hardware being free running. You need to either make the teardown > non-blocking or play some silence at the end. Right, usually the transfer is aligned with the period size, and the application is supposed to fill the period size at least. Takashi
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 35fe58f4fa86..1e876ff23524 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -237,6 +237,14 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream, struct snd_soc_dai *cpu_dai = rtd->cpu_dai; unsigned int rate, channels, sample_bits, symmetry, i; + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + if (!cpu_dai->capture_active) + return 0; + } else if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) { + if (!cpu_dai->playback_active) + return 0; + } + rate = params_rate(params); channels = params_channels(params); sample_bits = snd_pcm_format_physical_width(params_format(params));
The below program fails on a dai link with symmetric rates without this patch. The patch makes it work. #include <sys/soundcard.h> #include <unistd.h> #include <fcntl.h> #include <stropts.h> #include <stdio.h> int main(void) { int fd; int format; int channels; int speed; if ((fd = open("/dev/dsp", O_WRONLY, 0)) == -1) { perror("open"); return 1; } format = AFMT_S16_LE; if (ioctl(fd, SNDCTL_DSP_SETFMT, &format) == -1) { perror("SNDCTL_DSP_SETFMT"); return 1; } channels = 2; if (ioctl(fd, SNDCTL_DSP_CHANNELS, &channels) == -1) { perror("SNDCTL_DSP_CHANNELS"); return 1; } speed = 22050; if (ioctl(fd, SNDCTL_DSP_SPEED, &speed) == -1) { perror("SNDCTL_DSP_SPEED"); return 1; } return 0; } Signed-off-by: Peter Rosin <peda@axentia.se> --- sound/soc/soc-pcm.c | 8 ++++++++ 1 file changed, 8 insertions(+)