Re: [PATCH v2 01/16] ASoC: soc-pcm.c: cleanup soc_get_playback_capture()

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 4/3/24 20:53, Kuninori Morimoto wrote:
> 
> Hi Pierre-Louis
> 
> Thank you for your feedback.
> I could understand your comment 80%, but not yet 100%
> 
>> With the older code, if the dpcm_playback was set for the dailink but
>> there isn't any dai connected to support playback, an error was thrown.
>>
>> With the new code, if playback_only is set but there isn't any dai
>> connected, there is no error thrown, is there?
> (snip)
>> Again we had a verification that if the dpcm_playback was set at the
>> dailink level, it was actually supported by the dais.
>>
>> We seem to have lost this verification. We only have an error when there
>> are no settings at all.
> 
> Pseudo code of new soc_get_playback_capture() is like this
> 
> 	soc_get_playback_capture(...)
> 	{
> 		...
>  ^		for_each_rtd_ch_maps(...) {
>  |			...
> (A)			has_playback = xxx;
>  |			has_capture  = xxx;
>  v		}
> 
>  ^		if (dai_link->playback_only)
>  |			has_capture = 0;
> (B)
>  |		if (dai_link->capture_only)
>  v			has_playback = 0;
> 
>  ^		if (!has_playback && !has_capture) {
> (C)			dev_err(...);
>  v			return -EINVAL;
> 		}
> 		...
> 	}
> 
> In old/new soc_get_playback_capture(), has_xxx will be set at least
> if one of rtd connected DAI can handle playback/capture.
> In new code, it will be handled at (A).
> 
> And unneeded has_xxx will be removed if xxx_only was set (B)

The problem is that we have two sources of information

1) the dais included in the dailink (the (A) part above)
2) the dailink itself (the (B) part above)

the code in A) constructs the information from the ground-up, but it's
overridden by B).

You can view it as 'removing unneeded has_xxx' flags, but it's also a
problem is the dailink information is incorrect...

In the past we would report an error if the dailink was not aligned with
the dais. Now we just ignore the dai information.

That's the concern, we're changing the behavior.

> Then, if neither has_xxx was set, it will be error (C)

That's not the concern. The concern is a discrepancy between A) and B).

> 
> 	In new code, if playback_only is set but there isn't any dai
> 	connected, there is no error thrown, is there?
> 
> If playback_only was set, has_capture will be removed at (B).
> And if DAI was not playback-able, this means has_playback was not set at (A).
> In such case, (C) will indicate error. Same things happen if capture_only too.
> 
> So, old functions validation still exist in my opinion, but am I
> misunderstanding ?
> 
> One note here is that in DPCM case, old function checks CPU only,
> but new function checks both CPU and Codec.
> 
> 2nd note is that in current version of patch-set, if dai_link doesn't
> have xxx_only settings (= it should have both playback/capture), but if
> DAI has has_playback or has_capture only, it can't detect about it.
> I suggested it in previous mail, and will fix in v3
> 
>> The point is that these flags are sometimes set in the machine driver,
>> sometimes set in the framework, and the open is which one has the priority.
> 
> I couldn't understand this.
> 
> I think "machine driver" = CPU/Codec driver, but what is "these flags"
> which is sometimes set in machine driver, and sometimes set in framework ??
> dpcm_xxx ? xxx_only ?? I don't think framework set these...

The has_xxx flag is set based on dai capabilities in (A) - which I call
"the framework" OR by the machine driver setting the
playback_only/capture_only flags, then used in (B) to override (A).

When you have two sources of information competing to set a state, we
have to be really careful on which one has priority/precedence.