On Tue, Mar 06, 2018 at 04:30:29PM +0530, Shreyas NC wrote:
> This adds support for Multi CPU DAIs in PCM ops and stream
> handling functions.
>
> Signed-off-by: Shreyas NC <shreyas.nc@xxxxxxxxx>
> ---
> @@ -313,13 +333,17 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
> static bool soc_pcm_has_symmetry(struct snd_pcm_substream *substream)
> {
> struct snd_soc_pcm_runtime *rtd = substream->private_data;
> - struct snd_soc_dai_driver *cpu_driver = rtd->cpu_dai->driver;
> struct snd_soc_dai_link *link = rtd->dai_link;
> - unsigned int symmetry, i;
> + unsigned int symmetry = 0, i;
>
> - symmetry = cpu_driver->symmetric_rates || link->symmetric_rates ||
> - cpu_driver->symmetric_channels || link->symmetric_channels ||
> - cpu_driver->symmetric_samplebits || link->symmetric_samplebits;
> + /* Apply symmetery for multiple cpu dais */
> + for (i = 0; i < rtd->num_cpu_dai; i++)
> + symmetry = rtd->cpu_dais[i]->driver->symmetric_rates ||
> + link->symmetric_rates ||
> + rtd->cpu_dais[i]->driver->symmetric_channels ||
> + link->symmetric_channels ||
> + rtd->cpu_dais[i]->driver->symmetric_samplebits ||
> + link->symmetric_samplebits;
No need to bring the link-> stuff into the loop, it won't change
on each iteration. Would also make the code look neater to leave
it before the loop.
>
> for (i = 0; i < rtd->num_codecs; i++)
> symmetry = symmetry ||
> @@ -347,10 +371,10 @@ static void soc_pcm_set_msb(struct snd_pcm_substream *substream, int bits)
> static void soc_pcm_apply_msb(struct snd_pcm_substream *substream)
> {
> struct snd_soc_pcm_runtime *rtd = substream->private_data;
> - struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> + struct snd_soc_dai *cpu_dai;
> struct snd_soc_dai *codec_dai;
> int i;
> - unsigned int bits = 0, cpu_bits;
> + unsigned int bits = 0, cpu_bits = 0;
>
> if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> for (i = 0; i < rtd->num_codecs; i++) {
> @@ -361,7 +385,17 @@ static void soc_pcm_apply_msb(struct snd_pcm_substream *substream)
> }
> bits = max(codec_dai->driver->playback.sig_bits, bits);
> }
> - cpu_bits = cpu_dai->driver->playback.sig_bits;
> +
> + for (i = 0; i < rtd->num_cpu_dai; i++) {
> + cpu_dai = rtd->cpu_dais[i];
> + if (cpu_dai->driver->playback.sig_bits == 0) {
> + cpu_bits = 0;
> + break;
> + }
> +
> + cpu_bits = max(
> + cpu_dai->driver->playback.sig_bits, bits);
Can help but feel this would look nicer if you only wrapped the
second argument down a line. Although tbf its only 1 character so
you could probably just run over the line length as well.
> @@ -427,30 +464,41 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
> rates = snd_pcm_rate_mask_intersect(codec_stream->rates, rates);
> }
>
> - /*
> - * chan min/max cannot be enforced if there are multiple CODEC DAIs
> - * connected to a single CPU DAI, use CPU DAI's directly and let
> - * channel allocation be fixed up later
> - */
> - if (rtd->num_codecs > 1) {
> - chan_min = cpu_stream->channels_min;
> - chan_max = cpu_stream->channels_max;
> - }
> + for (i = 0; i < rtd->num_cpu_dai; i++) {
> + cpu_dai_drv = rtd->cpu_dais[i]->driver;
>
> - hw->channels_min = max(chan_min, cpu_stream->channels_min);
> - hw->channels_max = min(chan_max, cpu_stream->channels_max);
> - if (hw->formats)
> - hw->formats &= formats & cpu_stream->formats;
> - else
> - hw->formats = formats & cpu_stream->formats;
> - hw->rates = snd_pcm_rate_mask_intersect(rates, cpu_stream->rates);
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> + cpu_stream = &cpu_dai_drv->playback;
> + else
> + cpu_stream = &cpu_dai_drv->capture;
>
> - snd_pcm_limit_hw_rates(runtime);
> + /*
> + * chan min/max cannot be enforced if there are multiple
> + * CODEC DAIs connected to a single CPU DAI, use CPU DAI's
> + * directly and let channel allocation be fixed up later
> + */
> + if (rtd->num_codecs > 1) {
> + chan_min = cpu_stream->channels_min;
> + chan_max = cpu_stream->channels_max;
> + }
> +
> + hw->channels_min = max(chan_min, cpu_stream->channels_min);
> + hw->channels_max = min(chan_max, cpu_stream->channels_max);
> + if (hw->formats)
> + hw->formats &= formats & cpu_stream->formats;
> + else
> + hw->formats = formats & cpu_stream->formats;
>
I don't think actually ends up with the correct values in
hw->channels_min/max. Nothing compares one iteration of the
loop to the previous one so you don't end up with the min/max for
all the CPU DAIs you just end up with the values from the last
CPU DAI.
> /*
> @@ -465,12 +513,15 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
> struct snd_soc_platform *platform = rtd->platform;
> struct snd_soc_component *component;
> struct snd_soc_rtdcom_list *rtdcom;
> - struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> + struct snd_soc_dai *cpu_dai;
> struct snd_soc_dai *codec_dai;
> const char *codec_dai_name = "multicodec";
> - int i, ret = 0, __ret;
> + const char *cpu_dai_name = "multicpu";
> + int i, ret = 0, __ret, j;
> +
> + for (i = 0; i < rtd->num_cpu_dai; i++)
> + pinctrl_pm_select_default_state(rtd->cpu_dais[i]->dev);
>
> - pinctrl_pm_select_default_state(cpu_dai->dev);
> for (i = 0; i < rtd->num_codecs; i++)
> pinctrl_pm_select_default_state(rtd->codec_dais[i]->dev);
>
> @@ -483,12 +534,15 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
> mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>
> /* startup the audio subsystem */
> - if (cpu_dai->driver->ops->startup) {
> - ret = cpu_dai->driver->ops->startup(substream, cpu_dai);
> - if (ret < 0) {
> - dev_err(cpu_dai->dev, "ASoC: can't open interface"
> - " %s: %d\n", cpu_dai->name, ret);
> - goto out;
> + for (i = 0; i < rtd->num_cpu_dai; i++) {
> + cpu_dai = rtd->cpu_dais[i];
> + if (cpu_dai->driver->ops->startup) {
> + ret = cpu_dai->driver->ops->startup(substream, cpu_dai);
> + if (ret < 0) {
> + dev_err(cpu_dai->dev, "ASoC: can't open interface %s: %d\n",
> + cpu_dai->name, ret);
> + goto out;
I don't believe this jumps to the right place anymore since you
need to shutdown any CPU DAIs you have already started up.
> @@ -1276,8 +1388,12 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream)
> break;
> }
>
> - if (cpu_dai->driver->ops->delay)
> - delay += cpu_dai->driver->ops->delay(substream, cpu_dai);
> + for (i = 0; i < rtd->num_cpu_dai; i++) {
> + cpu_dai = rtd->cpu_dais[i];
> + if (cpu_dai->driver->ops->delay)
> + delay += cpu_dai->driver->ops->delay(substream,
> + cpu_dai);
> + }
I am not clear we should be adding the delays here can't they all
run in parallel? In which case shouldn't we be taking the max?
> @@ -2998,8 +3139,13 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
> capture = 1;
> }
>
> - capture = capture && cpu_dai->driver->capture.channels_min;
> - playback = playback && cpu_dai->driver->playback.channels_min;
> + for (i = 0; i < rtd->num_cpu_dai; i++) {
> + cpu_dai = rtd->cpu_dais[i];
> + capture = capture &&
> + cpu_dai->driver->capture.channels_min;
> + playback = playback &&
> + cpu_dai->driver->playback.channels_min;
> + }
This doesn't look right either since you will end up with the
values for the last CPU DAI surely you want some sort of combined
value.
I am also a little nervous about the mapping between widgets and
DAIs in dpcm_prune_paths. But I don't think I really understand
that bit of the code well enough to provide good comments.
Hopefully someone with more understanding than me can have a look
:-)
Thanks,
Charles
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
