diff mbox series

[v3,01/23] ASoC: soc-pcm: cleanup soc_get_playback_capture()

Message ID 87frvj8g2v.wl-kuninori.morimoto.gx@renesas.com (mailing list archive)
State New, archived
Headers show
Series ASoC: Replace dpcm_playback/capture to playback/capture_assertion | expand

Commit Message

Kuninori Morimoto April 18, 2024, 4:12 a.m. UTC
Current soc_get_playback_capture() (A) is checking playback/capture
availability for DPCM (X) / Normal (Y) / Codec2Codec (Z) connections.

(A)	static int soc_get_playback_capture(...)
	{
		...
 ^		if (dai_link->dynamic || dai_link->no_pcm) {
 |			...
 |(a)			if (dai_link->dpcm_playback) {
 |				...
 | ^				for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
 |(*)					...
 | v				}
 |				...
(X)			}
 |(b)			if (dai_link->dpcm_capture) {
 |				...
 | ^				for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
 |(*)					...
 | v				}
 |				...
 v			}
		} else {
 ^ ^			/* Adapt stream for codec2codec links */
 |(Z)			int cpu_capture = ...
 | v			int cpu_playback = ...
(Y)
 | ^			for_each_rtd_ch_maps(rtd, i, ch_maps) {
 |(*)				...
 v v			}
		}
		...
	}

(*) part is checking each DAI's availability.

(X) part is for DPCM, and it checks playback/capture availability
if dai_link has dpcm_playback/capture flag (a)(b).
This availability check should be available not only for DPCM, but for
all connections. But it is not mandatory option. Let's name it as
assertion.

In case of having assertion flag, non specific side will be disabled.
For example if it has playback_assertion but not have capture_assertion,
capture will be force disabled.

	dpcm_playback -> playabck_assertion = 1

	dpcm_capture  -> capture_assertion  = 1

	playback_only -> playback_assertion = 1
	                 capture_assertion  = 0

	capture_only  -> playback_assertion = 0
	                 capture_assertion  = 1

By expanding this assertion to all connections, we can use same code
for all connections, this means code can be more cleanup.

Here, current CPU / Codec validation check relationship is like this

	DPCM
		[CPU/xxxx]-[yyyy/Codec]
		^^^^        ^^^^
	non DPCM
		[CPU/Codec]
		^^^^^^^^^^^

DPCM     part (X) is checking only CPU       DAI, and
non DPCM part (Y) is checking both CPU/Codec DAI

Current validation check on DPCM ignores Codec DAI, but there is no
reason to do it.  We should check both CPU/Codec in all connection.
This patch expands validation check to all cases.

	[CPU/xxxx]-[yyyy/Codec]
	                 *****

In many case (not all case), above [xxxx][yyyy] part are "dummy" DAI
which is always valid for both playback/capture.
But unfortunately DPCM BE Codec (**** part) had been no validation
check before, and some Codec driver doesn't have necessary settings for
it. This means all cases validation check breaks compatibility on some
vender's drivers. Thus this patch temporary uses dummy DAI at BPCM BE
Codec part, and avoid compatibility error. But it should be removed in
the future.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc.h |   9 +++
 sound/soc/soc-pcm.c | 143 +++++++++++++++++++++++++-------------------
 2 files changed, 92 insertions(+), 60 deletions(-)

Comments

Pierre-Louis Bossart April 18, 2024, 4:20 p.m. UTC | #1
> (X) part is for DPCM, and it checks playback/capture availability
> if dai_link has dpcm_playback/capture flag (a)(b).
> This availability check should be available not only for DPCM, but for
> all connections. But it is not mandatory option. Let's name it as
> assertion.

I don't follow the 'not mandatory option'. Why not make these
'assertions' mandatory? What happens in case the the option is not present?

> In case of having assertion flag, non specific side will be disabled.

Not following the wording, multiple negatives and not clear on what
'side' refers to (direction or DPCM/non-DPCM).

> For example if it has playback_assertion but not have capture_assertion,
> capture will be force disabled.
> 
> 	dpcm_playback -> playabck_assertion = 1
> 
> 	dpcm_capture  -> capture_assertion  = 1
> 
> 	playback_only -> playback_assertion = 1
> 	                 capture_assertion  = 0
> 
> 	capture_only  -> playback_assertion = 0
> 	                 capture_assertion  = 1
> 
> By expanding this assertion to all connections, we can use same code
> for all connections, this means code can be more cleanup.

I see a contradiction between "expanding the assertion to all
connections" and "not mandatory".

> Current validation check on DPCM ignores Codec DAI, but there is no
> reason to do it.  We should check both CPU/Codec in all connection.

"there's no reason to do so" ?

> This patch expands validation check to all cases.
> 
> 	[CPU/xxxx]-[yyyy/Codec]
> 	                 *****
> 
> In many case (not all case), above [xxxx][yyyy] part are "dummy" DAI
> which is always valid for both playback/capture.
> But unfortunately DPCM BE Codec (**** part) had been no validation
> check before, and some Codec driver doesn't have necessary settings for
> it. This means all cases validation check breaks compatibility on some
> vender's drivers. Thus this patch temporary uses dummy DAI at BPCM BE

vendor

> Codec part, and avoid compatibility error. But it should be removed in
> the future.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  include/sound/soc.h |   9 +++
>  sound/soc/soc-pcm.c | 143 +++++++++++++++++++++++++-------------------
>  2 files changed, 92 insertions(+), 60 deletions(-)
> 
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index 0376f7e4c15d..e604d74f6e33 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -809,6 +809,15 @@ struct snd_soc_dai_link {
>  	unsigned int dpcm_capture:1;
>  	unsigned int dpcm_playback:1;
>  
> +	/*
> +	 * Capture / Playback support assertion. Having assertion flag is not mandatory.
> +	 * In case of having assertion flag, non specific side will be disabled.

again the 'not mandatory' and 'non specific side will be disabled' are
confusing.


> +	/*
> +	 * Assertion check
> +	 *
> +	 * playback_assertion = 0	No assertion check.
> +	 * capture_assertion  = 0	ASoC will use detected playback/capture as-is.
> +	 *				No playback, No capture will be error.

did you mean "this combination will be handled as an error" ?

It's probably best to have a different presentation, to avoid
confusions. Using multiple lines without a separator isn't great.

Suggested example:

playback_assertion = 0 capture_assertion  = 0
this combination will be handled as an error

playback_assertion = 1 capture_assertion  = 0
the DAIs in this dailink must support playback.
ASoC will disable capture.
In other words "playback_only"


> +	 *
> +	 * playback_assertion = 1	DAI must playback available. ASoC will disable capture.
> +	 * capture_assertion  = 0	In other words "playback_only"
> +	 *
> +	 * playback_assertion = 0	DAI must capture available. ASoC will disable playback.
> +	 * capture_assertion  = 1	In other words "capture_only"
> +	 *
> +	 * playback_assertion = 1	DAI must both playback/capture available.
> +	 * capture_assertion  = 1

nit-pick: the 'must X available' does not read well, 'must support X' is
probably what you meant.

> +	 */
> +	if (dai_link->playback_assertion) {
> +		if (!has_playback) {
> +			dev_err(rtd->dev, "%s playback assertion check error\n", dai_link->stream_name);

"invalid playback_assertion" ?

> +			return -EINVAL;
> +		}
> +		/* makes it plyaback only */

typo: playback

> +		if (!dai_link->capture_assertion)
> +			has_capture = 0;
> +	}
> +	if (dai_link->capture_assertion) {
> +		if (!has_capture) {
> +			dev_err(rtd->dev, "%s capture assertion check error\n", dai_link->stream_name);
> +			return -EINVAL;
> +		}
> +		/* makes it capture only */
> +		if (!dai_link->playback_assertion)
> +			has_playback = 0;
> +	}

we probably want a dev_ log when the has_playback/capture is overridden?

>  
> +	/*
> +	 * Detect Mismatch
> +	 */
>  	if (!has_playback && !has_capture) {
>  		dev_err(rtd->dev, "substream %s has no playback, no capture\n",
>  			dai_link->stream_name);
> -
>  		return -EINVAL;
>  	}
>
Kuninori Morimoto April 19, 2024, 1:09 a.m. UTC | #2
Hi Pierre-Louis

> > (X) part is for DPCM, and it checks playback/capture availability
> > if dai_link has dpcm_playback/capture flag (a)(b).
> > This availability check should be available not only for DPCM, but for
> > all connections. But it is not mandatory option. Let's name it as
> > assertion.
> 
> I don't follow the 'not mandatory option'. Why not make these
> 'assertions' mandatory? What happens in case the the option is not present?

The big reason why "assertion flag" is not mandatory is that non-DPCM doesn't
have such flag before. I can't add such flags to all of non-DPCM,
because I don't know which direction (playback/capture) is available on
each DAIs.

> > In case of having assertion flag, non specific side will be disabled.
> 
> Not following the wording, multiple negatives and not clear on what
> 'side' refers to (direction or DPCM/non-DPCM).

How about this ?

	If either playback or capture assertion flag was presented,
	not presented direction will be disabled by ASoC even if
	it was available.

> I see a contradiction between "expanding the assertion to all
> connections" and "not mandatory".
(snip)
> again the 'not mandatory' and 'non specific side will be disabled' are
> confusing.
(snip)
> > +	/*
> > +	 * Assertion check
> > +	 *
> > +	 * playback_assertion = 0	No assertion check.
> > +	 * capture_assertion  = 0	ASoC will use detected playback/capture as-is.
> > +	 *				No playback, No capture will be error.
> 
> did you mean "this combination will be handled as an error" ?

Hmm... is it word selection issue ?? is "must_support" better ?

(playback_xxx, capture_xxx)

(0, 0) : Both are not must item. available direction is used as-is.
         But it will be error if nothing was available.

(1, 0) : DAI must support selected direction.
(0, 1)   Not selected direction will be disabled even though it was available

(1, 1) : Both must be supported.

> It's probably best to have a different presentation, to avoid
> confusions. Using multiple lines without a separator isn't great.
> 
> Suggested example:
> 
> playback_assertion = 0 capture_assertion  = 0
> this combination will be handled as an error
> 
> playback_assertion = 1 capture_assertion  = 0
> the DAIs in this dailink must support playback.
> ASoC will disable capture.
> In other words "playback_only"

Yeah, I agree

> > +	 * playback_assertion = 1	DAI must playback available. ASoC will disable capture.
> > +	 * capture_assertion  = 0	In other words "playback_only"
> > +	 *
> > +	 * playback_assertion = 0	DAI must capture available. ASoC will disable playback.
> > +	 * capture_assertion  = 1	In other words "capture_only"
> > +	 *
> > +	 * playback_assertion = 1	DAI must both playback/capture available.
> > +	 * capture_assertion  = 1
> 
> nit-pick: the 'must X available' does not read well, 'must support X' is
> probably what you meant.

Thanks. will fix in v4

> > +	if (dai_link->capture_assertion) {
> > +		if (!has_capture) {
> > +			dev_err(rtd->dev, "%s capture assertion check error\n", dai_link->stream_name);
> > +			return -EINVAL;
> > +		}
> > +		/* makes it capture only */
> > +		if (!dai_link->playback_assertion)
> > +			has_playback = 0;
> > +	}
> 
> we probably want a dev_ log when the has_playback/capture is overridden?

OK, will do in v4


Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
Pierre-Louis Bossart April 19, 2024, 1:17 p.m. UTC | #3
>>> (X) part is for DPCM, and it checks playback/capture availability
>>> if dai_link has dpcm_playback/capture flag (a)(b).
>>> This availability check should be available not only for DPCM, but for
>>> all connections. But it is not mandatory option. Let's name it as
>>> assertion.
>>
>> I don't follow the 'not mandatory option'. Why not make these
>> 'assertions' mandatory? What happens in case the the option is not present?
> 
> The big reason why "assertion flag" is not mandatory is that non-DPCM doesn't
> have such flag before. I can't add such flags to all of non-DPCM,
> because I don't know which direction (playback/capture) is available on
> each DAIs.

Your explanation seems to contradict the sentence above "This
availability check should be available not only for DPCM, but for all
connections."

Can we actually do this 'availability check' for non-DPCM connections.

>>> In case of having assertion flag, non specific side will be disabled.
>>
>> Not following the wording, multiple negatives and not clear on what
>> 'side' refers to (direction or DPCM/non-DPCM).
> 
> How about this ?
> 
> 	If either playback or capture assertion flag was presented,
> 	not presented direction will be disabled by ASoC even if
> 	it was available.

Did you mean

"
The playback (resp. capture) direction will be disabled by ASoC if the
playback_assertion (resp. capture) flag is false - even if this
direction was available at the DAI level
"


> (playback_xxx, capture_xxx)
> 
> (0, 0) : Both are not must item. available direction is used as-is.
>          But it will be error if nothing was available.

That new wording makes me even more confused. What does 'available'
refer to and at which level is this?

This seems also to contradict the definitions above, "available
direction is used as-is" is not aligned with "not presented direction
will be disabled by ASoC even if it was available".
> (1, 0) : DAI must support selected direction.
> (0, 1)   Not selected direction will be disabled even though it was available
> 
> (1, 1) : Both must be supported.
Kuninori Morimoto April 22, 2024, 4:46 a.m. UTC | #4
Hi Pierre-Louis
Cc Mark

> Your explanation seems to contradict the sentence above "This
> availability check should be available not only for DPCM, but for all
> connections."
>
> Can we actually do this 'availability check' for non-DPCM connections.
>
> > How about this ?
> > 
> > 	If either playback or capture assertion flag was presented,
> > 	not presented direction will be disabled by ASoC even if
> > 	it was available.
> 
> Did you mean
> 
> "
> The playback (resp. capture) direction will be disabled by ASoC if the
> playback_assertion (resp. capture) flag is false - even if this
> direction was available at the DAI level
> "
> > (0, 0) : Both are not must item. available direction is used as-is.
> >          But it will be error if nothing was available.
> 
> That new wording makes me even more confused. What does 'available'
> refer to and at which level is this?
>
> This seems also to contradict the definitions above, "available
> direction is used as-is" is not aligned with "not presented direction
> will be disabled by ASoC even if it was available".

It is complicated by the attempt to merge dpcm_xxx and xxx_only flags.
And I noticed that my one of other attemption was not indicated.

Let's cleanup what does this patch-set want to do

I still wondering why dpcm_xxx flag itself is needed.

(A) Before, it checks channels_min for DPCM, same as current non-DPCM.
This is very clear for me. Let's name this as "validation check"

	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
		if (cpu_dai->driver->playback.channels_min)
			playback = 1;
		if (cpu_dai->driver->capture.channels_min)
			capture = 1;

(B) commit 1e9de42f4324b91ce2e9da0939cab8fc6ae93893
("Explicitly set BE DAI link supported stream directions") force use to
dpcm_xxx flag

	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
		playback = rtd->dai_link->dpcm_playback;
		capture = rtd->dai_link->dpcm_capture;

(C) 9b5db059366ae2087e07892b5fc108f81f4ec189
("ASoC: soc-pcm: dpcm: Only allow playback/capture if supported")
checks channels_min (= validation check) again

	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
		cpu_dai = asoc_rtd_to_cpu(rtd, 0);
		...
		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);

(D) b73287f0b0745961b14e5ebcce92cc8ed24d4d52
("ASoC: soc-pcm: dpcm: fix playback/capture checks") expanded it to
multi connection.

So, I would say nothing has changed, but become more complicated.
Or if (B) added dpcm_xxx as "option flag", it was understandable for me.
like this

	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
		playback = (cpu_dai->driver->playback.channels_min > 0) ||
			   rtd->dai_link->dpcm_playback;
		capture  = (cpu_dai->driver->capture.channels_min  > 0) ||
			   rtd->dai_link->dpcm_capture;

So my opinion is this dpcm_xxx is unnecessary flag that only complicate
matters. I guess almost all Card don't need this flag, this means
"validation check" only is veryenough, same as current non-DPCM.

But because of these history, dpcm_xxx flag have been used as
"passage permit" or "gate way". It doesn't try to "validation check" if
dpcm_xxx flag was not set. This is the reason why I try to merge
dpcm_xxx and xxx_only flag. These are doing the same things with
dirrerent flags, IMO.

OTOH, some Card want to detect error if expected direction
(playback/capture) was not valid. I guess this is your commitment (?).

So, let's keep xxx_only flag as-is, and use dpcm_xxx as "available_check".
I'm not sure what is the good naming, but for example
"playback_available_check" flag means "owner is expecting playback is
valid/available, and want to receive error if not".

I'm not sure how many owner want this flag, thus I think "option flag"
is very enough (= not mandatory, as I mentioned in the patch-set).

If we makes these checks generalize,
For DPCM, (for example new DPCM) it can remove/ignore "available_check"
flag if it don't need, same as current non-DPCM.
And for non-DPCM, it can use "available_check" if needed,
same as current DPCM.

What do you think ? what is your opinion ?

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
Pierre-Louis Bossart April 22, 2024, 8:12 p.m. UTC | #5
Hi Morimoto-san,

> I still wondering why dpcm_xxx flag itself is needed.

That's an excellent question indeed. And since you started a historical
review, we can get a lot of information.

> (A) Before, it checks channels_min for DPCM, same as current non-DPCM.
> This is very clear for me. Let's name this as "validation check"
> 
> 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> 		if (cpu_dai->driver->playback.channels_min)
> 			playback = 1;
> 		if (cpu_dai->driver->capture.channels_min)
> 			capture = 1;
> 
> (B) commit 1e9de42f4324b91ce2e9da0939cab8fc6ae93893
> ("Explicitly set BE DAI link supported stream directions") force use to
> dpcm_xxx flag
> 
> 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> 		playback = rtd->dai_link->dpcm_playback;
> 		capture = rtd->dai_link->dpcm_capture;

The reason for this (B) addition is very clear from the commit message

"
Some BE DAIs can be "dummy" (when the DSP is controlling the DAI) and as
such wont have set a minimum number of playback or capture channels
required for BE DAI registration (to establish supported stream directions).
"

So (B) introduced these dpcm_xxx flags as override mechanisms, where the
dailink information matters more than the dai information.

> (C) 9b5db059366ae2087e07892b5fc108f81f4ec189
> ("ASoC: soc-pcm: dpcm: Only allow playback/capture if supported")
> checks channels_min (= validation check) again
> 
> 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> 		cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> 		...
> 		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);

It helps to look at the commit message in detail:

"
Normally the dpcm_playback/capture parameter should match the
capabilities of the CPU DAI. However, there is no way to set that
parameter from the device tree (e.g. with simple-audio-card or
qcom sound cards). dpcm_playback/capture are always both set to 1.
"

This is where the direction changed from "dpcm_xxx" being override of
DAI capabilities to "dpcm_xxx" being required to match DAI capabilities,
because some machine drivers incorrectly did an override that made no
sense...

(C) is essentially (A) && (B)

Clearly there's a contradiction. If (C) was the correct solution, then
it would revert the direction in (A) and report an error for dummy dais.

It's been my question from the beginning of this thread, when the
direction information can come from 2 sources, which one do we trust?

> (D) b73287f0b0745961b14e5ebcce92cc8ed24d4d52
> ("ASoC: soc-pcm: dpcm: fix playback/capture checks") expanded it to
> multi connection.

You really want to look at

(E) 4f8721542f7b75954bfad98c51aa59d683d35b50
("ASoC: core: use less strict tests for dailink capabilities")

"
This patch modifies the snd_soc_dai_link_set_capabilities() helper so
that the dpcm_playback (resp. dpcm_capture) dailink capabilities are set
if at least one dai supports playback (resp. capture).

Likewise the checks are modified so that an error is reported only
when dpcm_playback (resp. dpcm_capture) is set but none of the CPU
DAIs support playback (resp. capture).
"

This one has not fundamentally changed the reasons why (C) was
introduced, the requirement is that dpcm_xxx be aligned with at least
ONE DAI capability. It's still not clear how dummy-dais would be handled
since the number of channels may or may not be set for those dummy-dais.

> So, I would say nothing has changed, but become more complicated.

It's not really become more complicated, the open is which of (B) or (C)
are correct.

> Or if (B) added dpcm_xxx as "option flag", it was understandable for me.
> like this
> 
> 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> 		playback = (cpu_dai->driver->playback.channels_min > 0) ||
> 			   rtd->dai_link->dpcm_playback;
> 		capture  = (cpu_dai->driver->capture.channels_min  > 0) ||
> 			   rtd->dai_link->dpcm_capture;

That would essentially revert (C), since the direction would ignore the
actual capabilities of the DAI.

IMHO, we should really try with this revert and go back to the initial
definition of (A), where dpcm_xxx is an optional escape mechanism to
allow machine drivers to set the direction. I would guess that would
impact mostly DT/simple-card users and Qualcomm, if the commit message
of (C) is still relevant.

Then we can discuss about merging code and removing flags. For now we
have different directions/opinions on something that's 10 years old,
last modified 4 years ago. We will break lots of eggs if we don't first
agree on what works and what doesn't.

This 23-patch code merge/simplification is premature at this point IMHO,
we should first land in a state where everyone is happy with how
dpcm_xxx flags need to be handled. We can merge dpcm_xxx and xxx_only
flags later when we understand what they are supposed to do...

And now I need a coffee refill :-)

Regards,
-Pierre
Kuninori Morimoto April 23, 2024, 4:56 a.m. UTC | #6
Hi Pierre-Louis

Thank you for your feedback

> > (B) commit 1e9de42f4324b91ce2e9da0939cab8fc6ae93893
> > ("Explicitly set BE DAI link supported stream directions") force use to
> > dpcm_xxx flag
> > 
> > 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> > 		playback = rtd->dai_link->dpcm_playback;
> > 		capture = rtd->dai_link->dpcm_capture;
> 
> The reason for this (B) addition is very clear from the commit message
> 
> "
> Some BE DAIs can be "dummy" (when the DSP is controlling the DAI) and as
> such wont have set a minimum number of playback or capture channels
> required for BE DAI registration (to establish supported stream directions).
> "

I'm still not yet 100% understand around this "dummy" DAI, but is it
*not* soc-utils.c :: dummy_dai, but some original dummy DAI is used
somewhere ?

I know ${LINUX}/sound/soc/codecs/hda.c :: card_binder_dai is one of
the DAI which is used but doesn't have channels_min.
I think it is used as BE "Codec", but code is checking "CPU" side.

Do you know what does this "BE dummy DAI" specifically means here?

> > Or if (B) added dpcm_xxx as "option flag", it was understandable for me.
> > like this
> > 
> > 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> > 		playback = (cpu_dai->driver->playback.channels_min > 0) ||
> > 			   rtd->dai_link->dpcm_playback;
> > 		capture  = (cpu_dai->driver->capture.channels_min  > 0) ||
> > 			   rtd->dai_link->dpcm_capture;
> 
> That would essentially revert (C), since the direction would ignore the
> actual capabilities of the DAI.
> 
> IMHO, we should really try with this revert and go back to the initial
> definition of (A), where dpcm_xxx is an optional escape mechanism to
> allow machine drivers to set the direction. I would guess that would
> impact mostly DT/simple-card users and Qualcomm, if the commit message
> of (C) is still relevant.

I can agree that above code makes dpcm_xxx flag option, and allow to
machine drivers to set direction without thinking actual DAI capabilities.
I think it is effective if it was around (C) timing.

	(A) : checked CPU capabilities
	(B) : uses dpcm_xxx flag
	(C) : checks both dpcm_xxx and capabilities
	...

But *current* ASoC is checking both "actual capabilities" and "dpcm_xxx"
flag in the same time, and have no problems today.

Around (A)-(B) timing, some DAI had issue (= channels_min was not set).
Let's name it as "no_chan_DAI". It should be CPU DAI and used as BE
if my understanding was correct.

Because "no_chan_DAI" exist, (B) was added.

But after that (C) was added, and it checks channels_min again.
It continues today. This means "no_chan_DAI" today have channels_min,
otherwise it can't work today.

If my above understanding were all correct, do we still need dpcm_xxx ?
because "no_chan_DAI" is no longer exist.
Just remove dpcm_xxx seems no problem, I guess...

> Then we can discuss about merging code and removing flags. For now we
> have different directions/opinions on something that's 10 years old,
> last modified 4 years ago. We will break lots of eggs if we don't first
> agree on what works and what doesn't.
> 
> This 23-patch code merge/simplification is premature at this point IMHO,
> we should first land in a state where everyone is happy with how
> dpcm_xxx flags need to be handled. We can merge dpcm_xxx and xxx_only
> flags later when we understand what they are supposed to do...

Thank you for your help. The problem becoming more clear.
Let's focus to "revert C code" or "remove dpcm_xxx" first.

> And now I need a coffee refill :-)

Enjoy :)


Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
Kuninori Morimoto April 25, 2024, 5:32 a.m. UTC | #7
Hi Pierre-Louis, Mark

Because Japanese will dive into long vacation since next week,
I want to post mail before that. I will back at 7th May.

> > > (B) commit 1e9de42f4324b91ce2e9da0939cab8fc6ae93893
> > > ("Explicitly set BE DAI link supported stream directions") force use to
> > > dpcm_xxx flag
> > > 
> > > 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> > > 		playback = rtd->dai_link->dpcm_playback;
> > > 		capture = rtd->dai_link->dpcm_capture;
> > 
> > The reason for this (B) addition is very clear from the commit message
> > 
> > "
> > Some BE DAIs can be "dummy" (when the DSP is controlling the DAI) and as
> > such wont have set a minimum number of playback or capture channels
> > required for BE DAI registration (to establish supported stream directions).
> > "
> 
> I'm still not yet 100% understand around this "dummy" DAI, but is it
> *not* soc-utils.c :: dummy_dai, but some original dummy DAI is used
> somewhere ?
> 
> I know ${LINUX}/sound/soc/codecs/hda.c :: card_binder_dai is one of
> the DAI which is used but doesn't have channels_min.
> I think it is used as BE "Codec", but code is checking "CPU" side.
> 
> Do you know what does this "BE dummy DAI" specifically means here?

	(A) : checked CPU capabilities
	(B) : uses dpcm_xxx flag
	(C) : checks both dpcm_xxx and capabilities
	...

In my understanding, in summary, this dpcm_xxx flag was added to rescue
dummy DAI which is used on DCPM BE as CPU at (B), because some of them
might not have channels_min (This "dummy DAI" is not same as soc-utils's
dummy DAI). Let's name it as "no_chan_DAI" here.
In this patch, it was added as "mandatory flag", not "option flag",
thus all DPCM needed to use this dpcm_xxx flag.

After that (C) was added, but it was contradiction, because
it checks both dpcm_xxx and channels_min.
If my understanding was correct, original "no_chan_DAI" was supposed to
stop working, because it doesn't have channels_min. But there is no
such report after (C), during this 4 years.
We don't know which DAI is the "no_chan_DAI" (?)

Possibilities are as follows
	- No one is using "no_chan_DAI"
	- "no_chan_DAI" is no longer exist : it was removed ?
	- "no_chan_DAI" is no longer exist : it has channels_min ?

If my expectation was correct, we don't need dpcm_xxx anymore.

But because we have been used dpcm_xxx for 10 years since (B),
I understand to feel anxious to suddenly remove dpcm_xxx.
I think it should be removed anyway, but want to have grace time ?
If so, the idea is that we can use it as "option flag" instead of
"mandatory flag" for a while, like below

	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
		playback = (cpu_dai->driver->playback.channels_min > 0) ||
			   rtd->dai_link->dpcm_playback;
		capture  = (cpu_dai->driver->capture.channels_min  > 0) ||
			   rtd->dai_link->dpcm_capture;

* maybe we want to indicate message like "place re-check the flag and
  remove it" via dev_info() if dpcm_xxx flag was used ?

I think +2 kernel version or so is enough for grace time ?
After that, we can remove dpcm_xxx flag.

When we consider it very detail, above code can't 100% keep compatibility
if the user have been used this dpcm_xxx flag to limit availability,
for example in case of DAI can use both playback/capture, but it had
dpcm_playback flag only. But it can use playback_only flag, instead.
But it is very difficult to find such DAI. Each user need to check.

I personally think that remove dpcm_xxx directly is no ploblem, but what
do you think ? I'm happy to hear any opinion, and happy to create
grace time code if someone want, but if there was no comment during
Japanese long vacation, I will create patch to remove dpcm_xxx directly.


BTW, I would like to know detail things around this topic. I'm happy if
someone knows it.

* Why dummy DAI doesn't/can't have channels_min ?

* Why it checks CPU side channels_min only when DPCM ?
  I think it should check both CPU and Codec.
  I could understand if it checks FE:CPU and BE:Codec (it is assuming
  other side was dummy), but both (FE/BE) check CPU side only...

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
Pierre-Louis Bossart April 25, 2024, 3:20 p.m. UTC | #8
> Because Japanese will dive into long vacation since next week,
> I want to post mail before that. I will back at 7th May.

Enjoy!

>>>> (B) commit 1e9de42f4324b91ce2e9da0939cab8fc6ae93893
>>>> ("Explicitly set BE DAI link supported stream directions") force use to
>>>> dpcm_xxx flag
>>>>
>>>> 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
>>>> 		playback = rtd->dai_link->dpcm_playback;
>>>> 		capture = rtd->dai_link->dpcm_capture;
>>>
>>> The reason for this (B) addition is very clear from the commit message
>>>
>>> "
>>> Some BE DAIs can be "dummy" (when the DSP is controlling the DAI) and as
>>> such wont have set a minimum number of playback or capture channels
>>> required for BE DAI registration (to establish supported stream directions).
>>> "
>>
>> I'm still not yet 100% understand around this "dummy" DAI, but is it
>> *not* soc-utils.c :: dummy_dai, but some original dummy DAI is used
>> somewhere ?
>>
>> I know ${LINUX}/sound/soc/codecs/hda.c :: card_binder_dai is one of
>> the DAI which is used but doesn't have channels_min.
>> I think it is used as BE "Codec", but code is checking "CPU" side.
>>
>> Do you know what does this "BE dummy DAI" specifically means here?
> 
> 	(A) : checked CPU capabilities
> 	(B) : uses dpcm_xxx flag
> 	(C) : checks both dpcm_xxx and capabilities
> 	...
> 
> In my understanding, in summary, this dpcm_xxx flag was added to rescue
> dummy DAI which is used on DCPM BE as CPU at (B), because some of them
> might not have channels_min (This "dummy DAI" is not same as soc-utils's
> dummy DAI). Let's name it as "no_chan_DAI" here.
> In this patch, it was added as "mandatory flag", not "option flag",
> thus all DPCM needed to use this dpcm_xxx flag.
> 
> After that (C) was added, but it was contradiction, because
> it checks both dpcm_xxx and channels_min.
> If my understanding was correct, original "no_chan_DAI" was supposed to
> stop working, because it doesn't have channels_min. But there is no
> such report after (C), during this 4 years.
> We don't know which DAI is the "no_chan_DAI" (?)
> 
> Possibilities are as follows
> 	- No one is using "no_chan_DAI"
> 	- "no_chan_DAI" is no longer exist : it was removed ?
> 	- "no_chan_DAI" is no longer exist : it has channels_min ?
> 
> If my expectation was correct, we don't need dpcm_xxx anymore.

I agree with your analysis. We don't have a clear memory/understanding
of which "no_chan_DAI" platforms (B) was supposed to handle, and why no
one reported them as broken by (C).

> But because we have been used dpcm_xxx for 10 years since (B),
> I understand to feel anxious to suddenly remove dpcm_xxx.

Indeed we err on the side of paranoia with such changes!

> I think it should be removed anyway, but want to have grace time ?
> If so, the idea is that we can use it as "option flag" instead of
> "mandatory flag" for a while, like below
> 
> 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> 		playback = (cpu_dai->driver->playback.channels_min > 0) ||
> 			   rtd->dai_link->dpcm_playback;
> 		capture  = (cpu_dai->driver->capture.channels_min  > 0) ||
> 			   rtd->dai_link->dpcm_capture;
> 
> * maybe we want to indicate message like "place re-check the flag and
>   remove it" via dev_info() if dpcm_xxx flag was used ?
> 
> I think +2 kernel version or so is enough for grace time ?
> After that, we can remove dpcm_xxx flag.

I am good with that plan, but I'll need to investigate first why we had
a failure with one of the Chromebooks on this v3 patchset. That may give
us some insights on "special" uses of those flags.

> When we consider it very detail, above code can't 100% keep compatibility
> if the user have been used this dpcm_xxx flag to limit availability,
> for example in case of DAI can use both playback/capture, but it had
> dpcm_playback flag only. But it can use playback_only flag, instead.
> But it is very difficult to find such DAI. Each user need to check.
> 
> I personally think that remove dpcm_xxx directly is no ploblem, but what
> do you think ? I'm happy to hear any opinion, and happy to create
> grace time code if someone want, but if there was no comment during
> Japanese long vacation, I will create patch to remove dpcm_xxx directly.
> 
> 
> BTW, I would like to know detail things around this topic. I'm happy if
> someone knows it.
> 
> * Why dummy DAI doesn't/can't have channels_min ?
> 
> * Why it checks CPU side channels_min only when DPCM ?
>   I think it should check both CPU and Codec.
>   I could understand if it checks FE:CPU and BE:Codec (it is assuming
>   other side was dummy), but both (FE/BE) check CPU side only...
Pierre-Louis Bossart April 25, 2024, 9:59 p.m. UTC | #9
>> But because we have been used dpcm_xxx for 10 years since (B),
>> I understand to feel anxious to suddenly remove dpcm_xxx.
> 
> Indeed we err on the side of paranoia with such changes!
> 
>> I think it should be removed anyway, but want to have grace time ?
>> If so, the idea is that we can use it as "option flag" instead of
>> "mandatory flag" for a while, like below
>>
>> 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
>> 		playback = (cpu_dai->driver->playback.channels_min > 0) ||
>> 			   rtd->dai_link->dpcm_playback;
>> 		capture  = (cpu_dai->driver->capture.channels_min  > 0) ||
>> 			   rtd->dai_link->dpcm_capture;
>>
>> * maybe we want to indicate message like "place re-check the flag and
>>   remove it" via dev_info() if dpcm_xxx flag was used ?
>>
>> I think +2 kernel version or so is enough for grace time ?
>> After that, we can remove dpcm_xxx flag.
> 
> I am good with that plan, but I'll need to investigate first why we had
> a failure with one of the Chromebooks on this v3 patchset. That may give
> us some insights on "special" uses of those flags.

I found the reason why this patchset failed on Intel CI: a dailink
direction is deemed supported only if ALL cpu- and codec-dais support it.

That is a stricter criterion than in existing code. This was a good
intention based on symmetry, but in practice there are exceptions to the
rule...

On some Chromebooks, we tag a dailink as supporting capture for echo
reference, but that echo reference is generated by the Intel firmware.
The amplifiers only support playback.

see https://github.com/thesofproject/linux/pull/4937 for details, I
added a clear log:

[  807.304740] kernel:  SSP1-Codec: CPU dai SSP1 Pin supports capture
but codec DAI rt1011-aif does not

So I think for now we probably want to avoid this stricter criterion and
only check the supported direction with the cpu-dais. Or we add an
escape mechanism to let the core know that the capture support was
intentional.

Also we relax the checks to go back to (E)
4f8721542f7b75954bfad98c51aa59d683d35b50
("ASoC: core: use less strict tests for dailink capabilities")

"
This patch modifies the snd_soc_dai_link_set_capabilities() helper so
that the dpcm_playback (resp. dpcm_capture) dailink capabilities are set
if at least one [cpu-]dai supports playback (resp. capture).
"

We tried checking all cpu-dais before and found issues, so "at least one
cpu dai" seems the strictest test to apply without breaking legacy.
Kuninori Morimoto April 26, 2024, 12:24 a.m. UTC | #10
Hi Pierre-Louis

Thank you for your feedback and report

> On some Chromebooks, we tag a dailink as supporting capture for echo
> reference, but that echo reference is generated by the Intel firmware.
> The amplifiers only support playback.
> 
> see https://github.com/thesofproject/linux/pull/4937 for details, I
> added a clear log:
> 
> [  807.304740] kernel:  SSP1-Codec: CPU dai SSP1 Pin supports capture
> but codec DAI rt1011-aif does not
> 
> So I think for now we probably want to avoid this stricter criterion and
> only check the supported direction with the cpu-dais. Or we add an
> escape mechanism to let the core know that the capture support was
> intentional.

I think my patch have escape mechanism, but it was for BE Codec.
If my understand was correct, above is FE Codec ?

One question is that is it just a mistake (no one noticed) ? or is there
some reasons it can't support capture (but use it ?). If it was just a
mistake, is it possible to update driver during the grace time ?
Or do you think we need "escape mechanism" permanently ?

> I agree with your analysis. We don't have a clear memory/understanding
> of which "no_chan_DAI" platforms (B) was supposed to handle, and why no
> one reported them as broken by (C).
(snip)
> I am good with that plan, but I'll need to investigate first why we had
> a failure with one of the Chromebooks on this v3 patchset. That may give
> us some insights on "special" uses of those flags.

Thanks.
It seems the code was just complicated, and we have been missing
important check. Let's break out my patch-set into small pieces,
and go more slowly.

I think it will be like below. Can you agree about this ?

Step1
	dpcm_xxx flag will be "option flag" instead of "mandatory flag"
	for a while to keep compatibility and avoide confusion.
	But it will be removed in Step3. To indicate such things,
	it will have dev_warn() if dpcm_xxx flag was used. like below

	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
		has_playback = /* at least one of CPU DAI supports playback */
		has_capture  = ...

		if (!playback && rtd->dai_link->dpcm_playback) {
			dev_warn(dev, "Playback is requested, but CPU doesn't support it\n")
			has_playback = 1;
		}
		...

Step2 (In case of "escape mechanism" is not needed)

	Current DPCM is checking CPU side only. Indicate warning until
	Step4 if Codec is not match . warning only, not error for a while.
	
	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
		...

		if (has_playback && /* no Codec DAI supports playback */)
			dev_warn(dev, "Playback is requested, but Codec doesn't support it\n")
		...

Step3

	Step1 deadline
	remove dpcm_xxx flag

Step4
	Step2 deadline
	CPU / Codec mismatch will be error.
	It will be "at least one of CPU/Codec pair supports playback/capture"

Step5

	There is no big diff between DPCM / non-DPCM check.
	Merge these, and clean-up code (soc_get_playback_capture())


Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
Kuninori Morimoto April 26, 2024, 4 a.m. UTC | #11
Hi

> Step1
> 	dpcm_xxx flag will be "option flag" instead of "mandatory flag"
> 	for a while to keep compatibility and avoide confusion.
> 	But it will be removed in Step3. To indicate such things,
> 	it will have dev_warn() if dpcm_xxx flag was used. like below
> 
> 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> 		has_playback = /* at least one of CPU DAI supports playback */
> 		has_capture  = ...
> 
> 		if (!playback && rtd->dai_link->dpcm_playback) {
> 			dev_warn(dev, "Playback is requested, but CPU doesn't support it\n")
> 			has_playback = 1;
> 		}
> 		...

I noticed that this Step is not needed, because all existing driver has
both dpcm_xxx and availability.
Instead, We want to have warning to indicate to use xxx_only flag if
dpcm_xxx was used as limit availability.

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
diff mbox series

Patch

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 0376f7e4c15d..e604d74f6e33 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -809,6 +809,15 @@  struct snd_soc_dai_link {
 	unsigned int dpcm_capture:1;
 	unsigned int dpcm_playback:1;
 
+	/*
+	 * Capture / Playback support assertion. Having assertion flag is not mandatory.
+	 * In case of having assertion flag, non specific side will be disabled.
+	 * see details
+	 *	soc_get_playback_capture()
+	 */
+	unsigned int capture_assertion:1;
+	unsigned int playback_assertion:1;
+
 	/* DPCM used FE & BE merged format */
 	unsigned int dpcm_merged_format:1;
 	/* DPCM used FE & BE merged channel */
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index b0e1bd7f588b..412e7b7d97f5 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2794,7 +2794,12 @@  static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 				    int *playback, int *capture)
 {
 	struct snd_soc_dai_link *dai_link = rtd->dai_link;
+	struct snd_soc_dai_link_ch_map *ch_maps;
 	struct snd_soc_dai *cpu_dai;
+	struct snd_soc_dai *codec_dai;
+	struct snd_soc_dai *dummy_dai = snd_soc_find_dai(&snd_soc_dummy_dlc);
+	int cpu_playback;
+	int cpu_capture;
 	int has_playback = 0;
 	int has_capture  = 0;
 	int i;
@@ -2804,77 +2809,95 @@  static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 		return -EINVAL;
 	}
 
-	if (dai_link->dynamic || dai_link->no_pcm) {
-		int stream;
-
-		if (dai_link->dpcm_playback) {
-			stream = SNDRV_PCM_STREAM_PLAYBACK;
-
-			for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
-				if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
-					has_playback = 1;
-					break;
-				}
-			}
-			if (!has_playback) {
-				dev_err(rtd->card->dev,
-					"No CPU DAIs support playback for stream %s\n",
-					dai_link->stream_name);
-				return -EINVAL;
-			}
-		}
-		if (dai_link->dpcm_capture) {
-			stream = SNDRV_PCM_STREAM_CAPTURE;
+	/*
+	 * REMOVE ME
+	 *
+	 * dpcm_playback/capture will be used as playback/capture_assertion
+	 */
+	if (dai_link->playback_only && dai_link->capture_only) {
+		dev_err(rtd->dev, "both playback_only / capture_only are set\n");
+		return -EINVAL;
+	}
+	if (dai_link->playback_only)
+		dai_link->playback_assertion = 1;
+	if (dai_link->capture_only)
+		dai_link->capture_assertion = 1;
+	if (dai_link->dpcm_playback)
+		dai_link->playback_assertion = 1;
+	if (dai_link->dpcm_capture)
+		dai_link->capture_assertion = 1;
 
-			for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
-				if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
-					has_capture = 1;
-					break;
-				}
-			}
+	/* Adapt stream for codec2codec links */
+	cpu_playback = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_PLAYBACK);
+	cpu_capture  = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_CAPTURE);
 
-			if (!has_capture) {
-				dev_err(rtd->card->dev,
-					"No CPU DAIs support capture for stream %s\n",
-					dai_link->stream_name);
-				return -EINVAL;
-			}
-		}
-	} else {
-		struct snd_soc_dai_link_ch_map *ch_maps;
-		struct snd_soc_dai *codec_dai;
-
-		/* Adapt stream for codec2codec links */
-		int cpu_capture  = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_CAPTURE);
-		int cpu_playback = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_PLAYBACK);
+	/*
+	 * see
+	 *	soc.h :: [dai_link->ch_maps Image sample]
+	 */
+	for_each_rtd_ch_maps(rtd, i, ch_maps) {
+		cpu_dai	  = snd_soc_rtd_to_cpu(rtd,   ch_maps->cpu);
+		codec_dai = snd_soc_rtd_to_codec(rtd, ch_maps->codec);
 
 		/*
-		 * see
-		 *	soc.h :: [dai_link->ch_maps Image sample]
+		 * FIXME
+		 *
+		 * DPCM BE Codec has been no checked before.
+		 * It should be checked, but it breaks compatibility.
+		 * It ignores BE Codec here, so far.
 		 */
-		for_each_rtd_ch_maps(rtd, i, ch_maps) {
-			cpu_dai	  = snd_soc_rtd_to_cpu(rtd,   ch_maps->cpu);
-			codec_dai = snd_soc_rtd_to_codec(rtd, ch_maps->codec);
-
-			if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) &&
-			    snd_soc_dai_stream_valid(cpu_dai,   cpu_playback))
-				has_playback = 1;
-			if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) &&
-			    snd_soc_dai_stream_valid(cpu_dai,   cpu_capture))
-				has_capture = 1;
-		}
-	}
+		if (dai_link->no_pcm)
+			codec_dai = dummy_dai;
 
-	if (dai_link->playback_only)
-		has_capture = 0;
+		if (snd_soc_dai_stream_valid(cpu_dai,   cpu_playback) &&
+		    snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK))
+			has_playback = 1;
+		if (snd_soc_dai_stream_valid(cpu_dai,   cpu_capture) &&
+		    snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE))
+			has_capture = 1;
+	}
 
-	if (dai_link->capture_only)
-		has_playback = 0;
+	/*
+	 * Assertion check
+	 *
+	 * playback_assertion = 0	No assertion check.
+	 * capture_assertion  = 0	ASoC will use detected playback/capture as-is.
+	 *				No playback, No capture will be error.
+	 *
+	 * playback_assertion = 1	DAI must playback available. ASoC will disable capture.
+	 * capture_assertion  = 0	In other words "playback_only"
+	 *
+	 * playback_assertion = 0	DAI must capture available. ASoC will disable playback.
+	 * capture_assertion  = 1	In other words "capture_only"
+	 *
+	 * playback_assertion = 1	DAI must both playback/capture available.
+	 * capture_assertion  = 1
+	 */
+	if (dai_link->playback_assertion) {
+		if (!has_playback) {
+			dev_err(rtd->dev, "%s playback assertion check error\n", dai_link->stream_name);
+			return -EINVAL;
+		}
+		/* makes it plyaback only */
+		if (!dai_link->capture_assertion)
+			has_capture = 0;
+	}
+	if (dai_link->capture_assertion) {
+		if (!has_capture) {
+			dev_err(rtd->dev, "%s capture assertion check error\n", dai_link->stream_name);
+			return -EINVAL;
+		}
+		/* makes it capture only */
+		if (!dai_link->playback_assertion)
+			has_playback = 0;
+	}
 
+	/*
+	 * Detect Mismatch
+	 */
 	if (!has_playback && !has_capture) {
 		dev_err(rtd->dev, "substream %s has no playback, no capture\n",
 			dai_link->stream_name);
-
 		return -EINVAL;
 	}