[PATCH 1/4] ASoC: soc-pcm: dpcm: fix playback/capture checks

[Pierre-Louis Bossart]

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

> 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.