[alsa-devel] [PATCH v5 2/3] ASoC: Add multiple CPU DAI support for PCM ops
Pierre-Louis Bossart
pierre-louis.bossart at linux.intel.com
Thu May 24 23:29:45 CEST 2018
This one is also quite dense, I could use clarifications on how channels
will be handled in a multi-cpu context. I believe for the multi-codec
case there was an assumption of symmetry, not sure this works or is
required in a multi-cpu case, see below.
>
> - symmetry = cpu_dai->driver->symmetric_channels ||
> - rtd->dai_link->symmetric_channels;
> + symmetry = rtd->dai_link->symmetric_channels;
> +
> + for (i = 0; i < rtd->num_cpu_dai; i++)
> + symmetry |= rtd->cpu_dais[i]->driver->symmetric_channels;
>
> for (i = 0; i < rtd->num_codecs; i++)
> symmetry |= rtd->codec_dais[i]->driver->symmetric_channels;
>
> - if (symmetry && cpu_dai->channels && cpu_dai->channels != channels) {
> - dev_err(rtd->dev, "ASoC: unmatched channel symmetry: %d - %d\n",
> - cpu_dai->channels, channels);
> - return -EINVAL;
> - }
> + for (i = 0; i < rtd->num_cpu_dai; i++)
> + if (symmetry && rtd->cpu_dais[i]->channels &&
> + rtd->cpu_dais[i]->channels != channels) {
> + dev_err(rtd->dev, "ASoC: unmatched channel symmetry: %d - %d\n",
> + rtd->cpu_dais[i]->channels, channels);
> + return -EINVAL;
> + }
I am not sure I get this part - but maybe I am connecting too many dots
with the SoundWire 'stream' patches.
This code is assuming all cpu_dais have the same number of channels,
defined by the hw_params.
Is this right? In the SoundWire case, we can have one port with 2
channels and another with 4, for a total of 6 channels for the stream.
Am I missing something or how should I reconcile the concepts?
making the assumption that the rates and sample_bits are identical is ok.
>
> - symmetry = cpu_dai->driver->symmetric_samplebits ||
> - rtd->dai_link->symmetric_samplebits;
> + symmetry = rtd->dai_link->symmetric_samplebits;
> +
> + for (i = 0; i < rtd->num_cpu_dai; i++)
> + symmetry |= rtd->cpu_dais[i]->driver->symmetric_samplebits;
>
> for (i = 0; i < rtd->num_codecs; i++)
> symmetry |= rtd->codec_dais[i]->driver->symmetric_samplebits;
>
> - if (symmetry && cpu_dai->sample_bits && cpu_dai->sample_bits != sample_bits) {
> - dev_err(rtd->dev, "ASoC: unmatched sample bits symmetry: %d - %d\n",
> - cpu_dai->sample_bits, sample_bits);
> - return -EINVAL;
> - }
> + for (i = 0; i < rtd->num_cpu_dai; i++)
> + if (symmetry && rtd->cpu_dais[i]->sample_bits &&
> + rtd->cpu_dais[i]->sample_bits != sample_bits) {
> + dev_err(rtd->dev, "ASoC: unmatched sample bits symmetry: %d - %d\n",
> + rtd->cpu_dais[i]->sample_bits,
> + sample_bits);
> + return -EINVAL;
> + }
>
> return 0;
> }
> @@ -308,13 +328,18 @@ 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;
>
> - symmetry = cpu_driver->symmetric_rates || link->symmetric_rates ||
> - cpu_driver->symmetric_channels || link->symmetric_channels ||
> - cpu_driver->symmetric_samplebits || link->symmetric_samplebits;
> + symmetry = link->symmetric_rates || link->symmetric_channels ||
> + link->symmetric_samplebits;
> +
> + /* Apply symmetery for multiple cpu dais */
I've never seen this spelling for cemetery :-)
[...]
>
> + for (i = 0; i < rtd->num_cpu_dai; i++) {
> + cpu_dai_drv = rtd->cpu_dais[i]->driver;
> +
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> + cpu_stream = &cpu_dai_drv->playback;
> + else
> + cpu_stream = &cpu_dai_drv->capture;
> +
> + cpu_chan_min = max(cpu_chan_min,
> + cpu_stream->channels_min);
> + cpu_chan_max = min(cpu_chan_max,
> + cpu_stream->channels_max);
> +
> + if (hw->formats)
> + hw->formats &= cpu_stream->formats;
> + else
> + hw->formats = cpu_stream->formats;
> +
> + cpu_rates = snd_pcm_rate_mask_intersect(cpu_rates,
> + cpu_stream->rates);
> +
> + cpu_rate_min = max(cpu_rate_min, cpu_stream->rate_min);
> + cpu_rate_max = min_not_zero(cpu_rate_max, cpu_stream->rate_max);
> + }
> +
> /*
> - * 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
> + * chan min/max cannot be enforced if there are multiple
> + * CODEC DAIs connected to CPU DAI(s), use CPU DAI's
> + * directly and let channel allocation be fixed up later
What does 'later' mean?
I guess I don't quite get the channel management, same issue as my
feedback above.
[...]
>
> @@ -963,11 +1070,14 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
> if (ret < 0)
> goto component_err;
>
> - /* store the parameters for each DAIs */
> - cpu_dai->rate = params_rate(params);
> - cpu_dai->channels = params_channels(params);
> - cpu_dai->sample_bits =
> - snd_pcm_format_physical_width(params_format(params));
> + for (i = 0; i < rtd->num_cpu_dai; i++) {
> + /* store the parameters for each DAIs */
> + cpu_dai = rtd->cpu_dais[i];
> + cpu_dai->rate = params_rate(params);
> + cpu_dai->channels = params_channels(params);
same here, are we again making the assumption that all cpu_dais can
transmit the same number of channels?
[...]
>
> @@ -1107,10 +1229,14 @@ static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> return ret;
> }
>
> - if (cpu_dai->driver->ops->trigger) {
> - ret = cpu_dai->driver->ops->trigger(substream, cmd, cpu_dai);
> - if (ret < 0)
> - return ret;
> + for (i = 0; i < rtd->num_cpu_dai; i++) {
> + cpu_dai = rtd->cpu_dais[i];
> + if (cpu_dai->driver->ops->trigger) {
> + ret = cpu_dai->driver->ops->trigger(substream,
> + cmd, cpu_dai);
How do I reconcile this sequential trigger with the notion of
bank-switch in SoundWire? It seems we are missing a global trigger for
all cpu_dais who are part of the same dailink? Or am I in the weeds again?
[...]
> @@ -1157,12 +1287,13 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream)
> struct snd_soc_pcm_runtime *rtd = substream->private_data;
> 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;
> struct snd_pcm_runtime *runtime = substream->runtime;
> snd_pcm_uframes_t offset = 0;
> snd_pcm_sframes_t delay = 0;
> snd_pcm_sframes_t codec_delay = 0;
> + snd_pcm_sframes_t cpu_delay = 0;
> int i;
>
> for_each_rtdcom(rtd, rtdcom) {
> @@ -1177,8 +1308,15 @@ 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)
> + cpu_delay = max(cpu_delay,
> + cpu_dai->driver->ops->delay(substream,
> + cpu_dai));
> + }
> +
> + delay += cpu_delay;
Oh, this is weird. If you are checking the delay sequentially for each
cpu_dai, what are the odds that you get a consistent reply? I think it's
fundamentally different from the codec side since you will in theory be
able to check delays on each cpu_dai fairly quickly over IPC, whereas
for codecs the delay is likely to be a long-term estimate, not an
immediate value. In addition you would probably expect that all cpu_dais
are triggered at the same time and hence have the same delay, so you
could use the cpu_dais[0] instead of querying the values multiple times.
More information about the Alsa-devel
mailing list