| Message ID | 1314172640-26972-1-git-send-email-b29396@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 24, 2011 at 03:57:20PM +0800, Dong Aisheng wrote: > Now we only check rate for symmetry when the pcm is really ongoing > which seems more sensible. There's a genuine issue here due to the raciness of the ALSA APIs - we're not enforcing constraints and we should be. If the two programs end up setting incompatible rates we need to handle that. > /* Symmetry data - only valid if symmetry is being enforced */ > unsigned int rate; > + unsigned int ongoing; This isn't a clear name, I don't understand it. > @@ -209,6 +212,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) > cpu_dai->active++; > codec_dai->active++; > rtd->codec->active++; > + rtd->ongoing = 1; > mutex_unlock(&rtd->pcm_mutex); > return 0; > > @@ -322,6 +326,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream) > SND_SOC_DAPM_STREAM_STOP); > } > > + rtd->ongoing = 0; What happens when the stream in one direction is stopped but not the other? For example, capture stops but playback continues.
On 08/24/2011 09:57 AM, Dong Aisheng wrote: > For the playback and record using different dai links, > checking !rtd->rate for symmetry may not be accurate because that > pcm may be acutually not running and the default new open rate is 0, > then the warning message "Not enforcing symmetric_rates" will > happen each time with running arecord | aplay. > > Now we only check rate for symmetry when the pcm is really ongoing > which seems more sensible. This will break the common case where playback and record are using the same link. You'll need at least ref-counting for whether the pcm is active or not too. But what is your setup anyway. You have two dai links and both refer to the same dais? Or how does it come that the dai is active while rtd->rate is not set while you are using different links? > > Signed-off-by: Dong Aisheng <b29396@freescale.com> > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> > Cc: Liam Girdwood <lrg@ti.com> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > --- > include/sound/soc.h | 1 + > sound/soc/soc-pcm.c | 5 +++++ > 2 files changed, 6 insertions(+), 0 deletions(-) > > diff --git a/include/sound/soc.h b/include/sound/soc.h > index 3fe658e..8e9931c 100644 > --- a/include/sound/soc.h > +++ b/include/sound/soc.h > @@ -851,6 +851,7 @@ struct snd_soc_pcm_runtime { > > /* Symmetry data - only valid if symmetry is being enforced */ > unsigned int rate; > + unsigned int ongoing; > long pmdown_time; > > /* runtime devices */ > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > index 1aee9fc..f3c1a66 100644 > --- a/sound/soc/soc-pcm.c > +++ b/sound/soc/soc-pcm.c > @@ -39,6 +39,9 @@ static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream) > !rtd->dai_link->symmetric_rates) > return 0; > > + if (!rtd->ongoing) > + return 0; > + > /* This can happen if multiple streams are starting simultaneously - > * the second can need to get its constraints before the first has > * picked a rate. Complain and allow the application to carry on. > @@ -209,6 +212,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) > cpu_dai->active++; > codec_dai->active++; > rtd->codec->active++; > + rtd->ongoing = 1; > mutex_unlock(&rtd->pcm_mutex); > return 0; > > @@ -322,6 +326,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream) > SND_SOC_DAPM_STREAM_STOP); > } > > + rtd->ongoing = 0; > mutex_unlock(&rtd->pcm_mutex); > return 0; > }
> -----Original Message----- > From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] > Sent: Wednesday, August 24, 2011 5:19 PM > To: Dong Aisheng-B29396 > Cc: alsa-devel@alsa-project.org; linux-arm-kernel@lists.infradead.org; > lrg@ti.com; s.hauer@pengutronix.de; w.sang@pengutronix.de > Subject: Re: [PATCH 1/1] ASoC: soc-core: check rate for symmetry only > when pcm is ongoing > > On Wed, Aug 24, 2011 at 03:57:20PM +0800, Dong Aisheng wrote: > > > Now we only check rate for symmetry when the pcm is really ongoing > > which seems more sensible. > > There's a genuine issue here due to the raciness of the ALSA APIs - we're > not enforcing constraints and we should be. If the two programs end up > setting incompatible rates we need to handle that. Yes, I also noticed that. I did not find a proper fix since we may not imagine the execute sequence of user level. Anyway, this patch is for fixing the wrong warning message. > > /* Symmetry data - only valid if symmetry is being enforced */ > > unsigned int rate; > > + unsigned int ongoing; > > This isn't a clear name, I don't understand it. How about change to active? > > @@ -209,6 +212,7 @@ static int soc_pcm_open(struct snd_pcm_substream > *substream) > > cpu_dai->active++; > > codec_dai->active++; > > rtd->codec->active++; > > + rtd->ongoing = 1; > > mutex_unlock(&rtd->pcm_mutex); > > return 0; > > > > @@ -322,6 +326,7 @@ static int soc_pcm_close(struct snd_pcm_substream > *substream) > > SND_SOC_DAPM_STREAM_STOP); > > } > > > > + rtd->ongoing = 0; > > What happens when the stream in one direction is stopped but not the > other? For example, capture stops but playback continues. Yes, that's an issue which I neglected. Thanks for the reminder. I'm going to use a ref-counter as cpu_dai->active. So the code could be like when open pcm, rtd->active++ and when close pcm, rtd->active--. Regards Dong Aisheng
> -----Original Message----- > From: Lars-Peter Clausen [mailto:lars@metafoo.de] > Sent: Wednesday, August 24, 2011 5:25 PM > To: Dong Aisheng-B29396 > Cc: alsa-devel@alsa-project.org; s.hauer@pengutronix.de; > broonie@opensource.wolfsonmicro.com; lrg@ti.com; linux-arm- > kernel@lists.infradead.org; w.sang@pengutronix.de > Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: soc-core: check rate for > symmetry only when pcm is ongoing > > On 08/24/2011 09:57 AM, Dong Aisheng wrote: > > For the playback and record using different dai links, checking > > !rtd->rate for symmetry may not be accurate because that pcm may be > > acutually not running and the default new open rate is 0, then the > > warning message "Not enforcing symmetric_rates" will happen each time > > with running arecord | aplay. > > > > Now we only check rate for symmetry when the pcm is really ongoing > > which seems more sensible. > > > This will break the common case where playback and record are using the > same link. You'll need at least ref-counting for whether the pcm is > active or not too. Yes, I will use ref-counting. > But what is your setup anyway. You have two dai links and both refer to > the same dais? Or how does it come that the dai is active while rtd->rate > is not set while you are using different links? My setup is different dai link for playback and record, however, the codec dai is the same one. Since codec dai is active which will cause to do symmetry check, However, the new open rtd->rate of a different dai link is not set by default. Below is the detailed setup: static struct snd_soc_dai_link mxs_sgtl5000_dai[] = { { .name = "HiFi Tx", .stream_name = "HiFi Playback", .codec_dai_name = "sgtl5000", .codec_name = "sgtl5000.0-000a", .cpu_dai_name = "mxs-saif.0", .platform_name = "mxs-pcm-audio.0", .ops = &mxs_sgtl5000_hifi_ops, }, { .name = "HiFi Rx", .stream_name = "HiFi Capture", .codec_dai_name = "sgtl5000", .codec_name = "sgtl5000.0-000a", .cpu_dai_name = "mxs-saif.1", .platform_name = "mxs-pcm-audio.1", .ops = &mxs_sgtl5000_hifi_ops, }, }; > > > > Signed-off-by: Dong Aisheng <b29396@freescale.com> > > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> > > Cc: Liam Girdwood <lrg@ti.com> > > Cc: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > include/sound/soc.h | 1 + > > sound/soc/soc-pcm.c | 5 +++++ > > 2 files changed, 6 insertions(+), 0 deletions(-) > > > > diff --git a/include/sound/soc.h b/include/sound/soc.h index > > 3fe658e..8e9931c 100644 > > --- a/include/sound/soc.h > > +++ b/include/sound/soc.h > > @@ -851,6 +851,7 @@ struct snd_soc_pcm_runtime { > > > > /* Symmetry data - only valid if symmetry is being enforced */ > > unsigned int rate; > > + unsigned int ongoing; > > long pmdown_time; > > > > /* runtime devices */ > > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index > > 1aee9fc..f3c1a66 100644 > > --- a/sound/soc/soc-pcm.c > > +++ b/sound/soc/soc-pcm.c > > @@ -39,6 +39,9 @@ static int soc_pcm_apply_symmetry(struct > snd_pcm_substream *substream) > > !rtd->dai_link->symmetric_rates) > > return 0; > > > > + if (!rtd->ongoing) > > + return 0; > > + > > /* This can happen if multiple streams are starting simultaneously > - > > * the second can need to get its constraints before the first has > > * picked a rate. Complain and allow the application to carry on. > > @@ -209,6 +212,7 @@ static int soc_pcm_open(struct snd_pcm_substream > *substream) > > cpu_dai->active++; > > codec_dai->active++; > > rtd->codec->active++; > > + rtd->ongoing = 1; > > mutex_unlock(&rtd->pcm_mutex); > > return 0; > > > > @@ -322,6 +326,7 @@ static int soc_pcm_close(struct snd_pcm_substream > *substream) > > SND_SOC_DAPM_STREAM_STOP); > > } > > > > + rtd->ongoing = 0; > > mutex_unlock(&rtd->pcm_mutex); > > return 0; > > } >
On 08/24/2011 12:39 PM, Dong Aisheng-B29396 wrote: >> -----Original Message----- >> From: Lars-Peter Clausen [mailto:lars@metafoo.de] >> Sent: Wednesday, August 24, 2011 5:25 PM >> To: Dong Aisheng-B29396 >> Cc: alsa-devel@alsa-project.org; s.hauer@pengutronix.de; >> broonie@opensource.wolfsonmicro.com; lrg@ti.com; linux-arm- >> kernel@lists.infradead.org; w.sang@pengutronix.de >> Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: soc-core: check rate for >> symmetry only when pcm is ongoing >> >> On 08/24/2011 09:57 AM, Dong Aisheng wrote: >>> For the playback and record using different dai links, checking >>> !rtd->rate for symmetry may not be accurate because that pcm may be >>> acutually not running and the default new open rate is 0, then the >>> warning message "Not enforcing symmetric_rates" will happen each time >>> with running arecord | aplay. >>> >>> Now we only check rate for symmetry when the pcm is really ongoing >>> which seems more sensible. >> >> >> This will break the common case where playback and record are using the >> same link. You'll need at least ref-counting for whether the pcm is >> active or not too. > Yes, I will use ref-counting. > >> But what is your setup anyway. You have two dai links and both refer to >> the same dais? Or how does it come that the dai is active while rtd->rate >> is not set while you are using different links? > > My setup is different dai link for playback and record, however, > the codec dai is the same one. > > Since codec dai is active which will cause to do symmetry check, > However, the new open rtd->rate of a different dai link > is not set by default. > > Below is the detailed setup: > static struct snd_soc_dai_link mxs_sgtl5000_dai[] = { > { > .name = "HiFi Tx", > .stream_name = "HiFi Playback", > .codec_dai_name = "sgtl5000", > .codec_name = "sgtl5000.0-000a", > .cpu_dai_name = "mxs-saif.0", > .platform_name = "mxs-pcm-audio.0", > .ops = &mxs_sgtl5000_hifi_ops, > }, { > .name = "HiFi Rx", > .stream_name = "HiFi Capture", > .codec_dai_name = "sgtl5000", > .codec_name = "sgtl5000.0-000a", > .cpu_dai_name = "mxs-saif.1", > .platform_name = "mxs-pcm-audio.1", > .ops = &mxs_sgtl5000_hifi_ops, > }, > }; > This is an interesting case, since you still want to use the same rate for capture and playback, since the codec needs this. So the warning is actually correct, you want symmetry but it was not enforced. Though it is not really due to an race. For this kind of setup we'd have to store the current rate in the codec and cpu_dais themselves and then have something along the lines of this in soc_pcm_apply_symmetry: if (codec_dai->active) { if (codec_dai->rate != 0) { ret = snd_pcm_hw_constraint_minmax(substream->runtime, SNDRV_PCM_HW_PARAM_RATE, codec_dai->rate, codec_dai->rate); } else { race = true; } } if (cpu_dai->active) { if (cpu_dai->rate != 0) { ret = snd_pcm_hw_constraint_minmax(substream->runtime, SNDRV_PCM_HW_PARAM_RATE, cpu_dai->rate, cpu_dai->rate); } else { race = true; } } if (race) dev_warn(&rtd->dev, "Not enforcing symmetric_rates due to race\n");
diff --git a/include/sound/soc.h b/include/sound/soc.h index 3fe658e..8e9931c 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -851,6 +851,7 @@ struct snd_soc_pcm_runtime { /* Symmetry data - only valid if symmetry is being enforced */ unsigned int rate; + unsigned int ongoing; long pmdown_time; /* runtime devices */ diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 1aee9fc..f3c1a66 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -39,6 +39,9 @@ static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream) !rtd->dai_link->symmetric_rates) return 0; + if (!rtd->ongoing) + return 0; + /* This can happen if multiple streams are starting simultaneously - * the second can need to get its constraints before the first has * picked a rate. Complain and allow the application to carry on. @@ -209,6 +212,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) cpu_dai->active++; codec_dai->active++; rtd->codec->active++; + rtd->ongoing = 1; mutex_unlock(&rtd->pcm_mutex); return 0; @@ -322,6 +326,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream) SND_SOC_DAPM_STREAM_STOP); } + rtd->ongoing = 0; mutex_unlock(&rtd->pcm_mutex); return 0; }
For the playback and record using different dai links, checking !rtd->rate for symmetry may not be accurate because that pcm may be acutually not running and the default new open rate is 0, then the warning message "Not enforcing symmetric_rates" will happen each time with running arecord | aplay. Now we only check rate for symmetry when the pcm is really ongoing which seems more sensible. Signed-off-by: Dong Aisheng <b29396@freescale.com> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: Liam Girdwood <lrg@ti.com> Cc: Sascha Hauer <s.hauer@pengutronix.de> --- include/sound/soc.h | 1 + sound/soc/soc-pcm.c | 5 +++++ 2 files changed, 6 insertions(+), 0 deletions(-)