[PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Peter Rosin]

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(+)

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));
-- 
2.1.4

[PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Peter Rosin]

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(+)

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));
-- 
2.1.4

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1164 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1164 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Peter Rosin]

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

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Peter Rosin]

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

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1895 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1895 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Takashi Iwai]

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

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Takashi Iwai]

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

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Takashi Iwai]

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

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

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 :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 937 bytes --]

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 :)

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Peter Rosin]

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

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Peter Rosin]

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

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [alsa-devel] [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Takashi Iwai]

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

Re: [PATCH] ASoC: pcm: allow changing the playback/capture rates for symmetric links

[Takashi Iwai]

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