On Tue, Jun 16, 2020 at 12:05:49PM -0500, Pierre-Louis Bossart wrote:
simple-card.c and audio-graph-card do hard-code but that's done with C in the driver:
ret = asoc_simple_parse_daifmt(dev, cpu_ep, codec_ep, NULL, &dai_link->dai_fmt); if (ret < 0) goto out_put_node;
dai_link->dpcm_playback = 1; dai_link->dpcm_capture = 1;
that that should be fixed based on the DAI format used in that dai_link - in other words we can make sure the capabilities of the dailink are aligned with the dais while parsing the DT blobs.
But how do you know which capabilities to set? The device tree doesn't tells us that. We could add some code to look up the snd_soc_dai_driver early, based on the references in the device tree (basically something like snd_soc_of_get_dai_name(), see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...)
At least to me that function doesn't exactly look trivial though, and that's just to properly fill in the dpcm_playback/capture parameters. Essentially those parameters only complicate the current device tree use case, where you want the DAI link to be for both playback/capture, but restricted to the capabilities of the DAI.
Just wondering if setting up dpcm_playback/capture properly is worth it at all in this case. This isn't necessary for the non-DPCM case either, there we automatically set it based on the DAI capabilities.
We can add a simple loop for each direction that relies on snd_soc_dai_stream_valid() to identify if each DAI is capable of doing playback/capture.
see below completely untested diff to show what I had in mind: we already make use of snd_soc_dai_stream_valid() in other parts of the core so we should be able to determine dpcm_playback/capture based on the same information already used.
diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c index 9ad35d9940fe..4c67f1f65eb4 100644 --- a/sound/soc/generic/audio-graph-card.c +++ b/sound/soc/generic/audio-graph-card.c @@ -215,7 +215,9 @@ static int graph_dai_link_of_dpcm(struct asoc_simple_priv *priv, struct asoc_simple_dai *dai; struct snd_soc_dai_link_component *cpus = dai_link->cpus; struct snd_soc_dai_link_component *codecs = dai_link->codecs;
int stream; int ret;int i; /* Do it all CPU endpoint, and 1st Codec endpoint */ if (!li->cpu && dup_codec)@@ -317,8 +319,34 @@ static int graph_dai_link_of_dpcm(struct asoc_simple_priv *priv, if (ret < 0) goto out_put_node;
dai_link->dpcm_playback = 1;dai_link->dpcm_capture = 1;
for_each_pcm_streams(stream) {struct snd_soc_dai_link_component *cpu;struct snd_soc_dai_link_component *codec;struct snd_soc_dai *d;bool dpcm_direction = true;for_each_link_cpus(dai_link, i, cpu) {d = snd_soc_find_dai(cpu);if (!d || !snd_soc_dai_stream_valid(d, stream)) {dpcm_direction = false;break;}}for_each_link_codecs(dai_link, i, codec) {d = snd_soc_find_dai(codec);if (!d || !snd_soc_dai_stream_valid(d, stream)) {dpcm_direction = false;break;}}if (dpcm_direction) {if (stream == SNDRV_PCM_STREAM_PLAYBACK)dai_link->dpcm_playback = 1;if (stream == SNDRV_PCM_STREAM_CAPTURE)dai_link->dpcm_capture = 1;}}dai_link->ops = &graph_ops; dai_link->init = asoc_simple_dai_init;
Thanks for the diff! I tested it for my case and it seems to work fine so far. I'm fine with this solution given that it fixes the problem I mentioned. We would need to patch it into at least simple-audio-card.c, audio-graph-card.c and soc/qcom/common.c (probably best to create a shared function in soc-core.c then).
However, personally I still think that dpcm_playback/capture essentially just duplicate the capabilities that are already exposed as part of the DAI drivers. We don't need that duplication in the non-DPCM case, so I wonder if we really need it for DPCM.
With your diff we go over all the DAIs to set dpcm_playback/capture correctly so that soc_new_pcm() can then verify that they were set correctly. IMO it would be much simpler to restore the previous behavior and just make soc_new_pcm() rely on the DAI capabilities to decide if playback/capture is supported, without producing the error.
Thanks, Stephan