On Wed, Jun 17, 2020 at 09:33:40AM -0500, Pierre-Louis Bossart wrote: > > > On 6/17/20 4:01 AM, Stephan Gerhold wrote: > > 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/sound/soc/soc-core.c#n2988) > > > > > > > > > > > > > > > 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). > > Yes, I worked on a helper in soc-dai.c and have a tentative proposal in > https://github.com/thesofproject/linux/pull/2203 > Thanks! > > 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. > > Fully agree, but removing dpcm_playback/capture/playback_only/capture_only > should be done in a follow-up patchset. It's just too complicated to both > fix the current DPCM configurations and clean-up at the same time, it'd > prefer to do this cleanup in two steps. > > > 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. > > I don't understand what previous behavior you are referring to (it's not > something I personally changed), and these flags are also hard-coded in > static dailink descriptors for machine drivers. > At the end the question is if those machine drivers that have dpcm_playback/capture hardcoded just set it because it was required to make DPCM work, or if they actually use it to restrict the direction of a DAI link. If they just did it to make DPCM work, then dpcm_playback/capture will most likely be equal to the capabilities of the DAIs. In that case there is little reason to duplicate that information on the DAI link. (We can just get the capabilities by always iterating over the DAIs, just like your helpers do for the device tree case). IMO dpcm_playback/capture would be only needed at all if you want to restrict the direction of a DAI link, instead of using everything possible using the configured DAI links. But I agree, removing dpcm_playback/capture is way too much refactoring to fix the problem I mentioned. The previous behavior I'm referring to is that until your patch (the one I replied to), it was not required to set dpcm_playback/capture appropriately: Before your changes there was just playback = rtd->dai_link->dpcm_playback && snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK); capture = rtd->dai_link->dpcm_capture && snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_CAPTURE); so even if both dpcm_playback/capture were set to 1 (= true), it would just set playback/capture = false if the CPU DAI does not actually support those. Your patch added error conditions if dpcm_playback/capture do not match the actual capabilities of the DAI, so now I get e.g. "CPU DAI MultiMedia1 for rtd MultiMedia1 does not support capture". Something like this would restore the previous behavior: diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 39ce61c5b874..2b32c2a1fad7 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2920,31 +2920,31 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) { if (rtd->dai_link->dpcm_playback) { stream = SNDRV_PCM_STREAM_PLAYBACK; + playback = 1; for_each_rtd_cpu_dais(rtd, i, cpu_dai) if (!snd_soc_dai_stream_valid(cpu_dai, stream)) { - dev_err(rtd->card->dev, + dev_dbg(rtd->card->dev, "CPU DAI %s for rtd %s does not support playback\n", cpu_dai->name, rtd->dai_link->stream_name); - return -EINVAL; + playback = 0; } - playback = 1; } if (rtd->dai_link->dpcm_capture) { stream = SNDRV_PCM_STREAM_CAPTURE; + capture = 1; for_each_rtd_cpu_dais(rtd, i, cpu_dai) if (!snd_soc_dai_stream_valid(cpu_dai, stream)) { - dev_err(rtd->card->dev, + dev_dbg(rtd->card->dev, "CPU DAI %s for rtd %s does not support capture\n", cpu_dai->name, rtd->dai_link->stream_name); - return -EINVAL; + capture = 0; } - capture = 1; } } else { /* Adapt stream for codec2codec links */ (I just removed the error case you introduced in your patch...) I understand why you added those error checks, but as a temporary fix it might be easier to relax those error checks again for now. I'm not sure if any other drivers have dpcm_playback/capture set "incorrectly" (better: not matching the DAI capabilities) as well. Thanks, Stephan
