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

[Stephan Gerhold <stephan@gerhold.net>]

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

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