ASoC dailink capture/playback flags

ASoC dailink capture/playback flags

[Pierre-Louis Bossart]

Hi,

Some of us at Intel are confused with multiple definitions of 
capture/playback dailink flags (credits to Rander Wang for reporting 
this in https://github.com/thesofproject/linux/pull/2070).

struct snd_soc_dai_link {
[...]

     /* For unidirectional dai links */
     unsigned int playback_only:1;
     unsigned int capture_only:1;

[...]

     /* DPCM capture and Playback support */
     unsigned int dpcm_capture:1;
     unsigned int dpcm_playback:1;
[...]

};

And of course when you start looking at some machine drivers, there are 
confusions, e.g. below with DPCM front-ends using one or the other set, 
and with copy-paste and partial diffs, this propagates without being 
consistent or being noticed:

     [GLK_DPCM_AUDIO_HS_PB] = {
         .name = "Glk Audio Headset Playback",
         .stream_name = "Headset Audio",
         .dpcm_playback = 1, <<<<
         .nonatomic = 1,
         .dynamic = 1,
         SND_SOC_DAILINK_REG(system2, dummy, platform),
     },
     [GLK_DPCM_AUDIO_ECHO_REF_CP] = {
         .name = "Glk Audio Echo Reference cap",
         .stream_name = "Echoreference Capture",
         .init = NULL,
         .capture_only = 1, <<<< should this be .dpcm_capture = 1?
         .nonatomic = 1,
         .dynamic = 1,
         SND_SOC_DAILINK_REG(echoref, dummy, platform),
     },
     [GLK_DPCM_AUDIO_REF_CP] = {
         .name = "Glk Audio Reference cap",
         .stream_name = "Refcap",
         .init = NULL,
         .dpcm_capture = 1, <<<<
         .nonatomic = 1,
         .dynamic = 1,
         .ops = &geminilake_refcap_ops,
         SND_SOC_DAILINK_REG(reference, dummy, platform),
     },

So here are the questions:

- when using DPCM, is there an expectation to use dpcm_ flags only?

- should we instead use playback/capture_only when only one of the two 
dpcm_ flags is set?

- should we flags errors if we ever encounter cases with e.g. 
dpcm_playback = true and capture_only = true?

- do we actually need two sets of definitions? There are very few users 
of the .playback_only and .capture_only flags and only a single place 
where it's checked in soc-pcm.c

Thanks for your feedback

-Pierre

Re: ASoC dailink capture/playback flags

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

On Tue, May 19, 2020 at 09:25:31PM -0500, Pierre-Louis Bossart wrote:

> So here are the questions:

> - when using DPCM, is there an expectation to use dpcm_ flags only?

> - should we instead use playback/capture_only when only one of the two dpcm_
> flags is set?

> - should we flags errors if we ever encounter cases with e.g. dpcm_playback
> = true and capture_only = true?

TBH I'm not convinced DPCM is well enough tied together to have clear
answers on this.  I don't really know what those DPCM level flags are
doing.

> - do we actually need two sets of definitions? There are very few users of
> the .playback_only and .capture_only flags and only a single place where
> it's checked in soc-pcm.c

We could probably do a driver only thing for the i.MX stuff if we had
to.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: ASoC dailink capture/playback flags

[Kuninori Morimoto]

Hi Pierre-Louis

> struct snd_soc_dai_link {
> [...]
> 
>     /* For unidirectional dai links */
>     unsigned int playback_only:1;
>     unsigned int capture_only:1;
> 
> [...]
> 
>     /* DPCM capture and Playback support */
>     unsigned int dpcm_capture:1;
>     unsigned int dpcm_playback:1;
> [...]
> 
> };

I confirmed it.
If my understand was correct...

dpcm_xxx were added to distinguish "dummy" DAI or not, by

	1e9de42f4324b91ce2e9da0939cab8fc6ae93893
	ASoC: dpcm: Explicitly set BE DAI link supported stream directions

xxx_only were added to handle unidirectional such as Freescale MX28, by

	d6bead020d8f8bcaca5cdcb035250c44b21c93e7
	ASoC: soc-pcm: Allow to specify unidirectional dai_link

> - when using DPCM, is there an expectation to use dpcm_ flags only?

I think it is needed if it was not dummy DAI.

> - should we instead use playback/capture_only when only one of the two
> dpcm_ flags is set?
> 
> - should we flags errors if we ever encounter cases with
> e.g. dpcm_playback = true and capture_only = true?
> 
> - do we actually need two sets of definitions? There are very few
> users of the .playback_only and .capture_only flags and only a single
> place where it's checked in soc-pcm.c

I wonder why xxx_only were needed when it was unidirection ?
I guess one of playback/capture will be (should be ?)
automatically 0 in such case at soc_new_pcm() ??

...? dummy DAI was the reason ??
Do we need below or similar patch ?
(not tested)

If so, and if we can handle dummy DAI correctly at soc_new_pcm() somehow,
we can automatically judge dpcm_xxx and xxx_only and is able to remove them?

----------------
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 80dd3cf6200c..dc89ead4ec62 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2811,6 +2811,9 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 			SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
 
 		for_each_rtd_codec_dais(rtd, i, codec_dai) {
+			if (!strcmp(codec_dai->component->name, "snd-soc-dummy"))
+				continue;
+
 			if (rtd->num_cpus == 1) {
 				cpu_dai = asoc_rtd_to_cpu(rtd, 0);
 			} else if (rtd->num_cpus == rtd->num_codecs) {
----------------


Thank you for your help !!

Best regards
---
Kuninori Morimoto