Re: [RFC PATCH v2] ASoC: soc-pcm: Optimize hw_params() BE DAI call

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

>> I share Peter's question. The code has been around since 2016, in what 
>> case would the existing logic need to be updated?
> 
> As such it is not causing any issue. But I think if the BE DAI state is 
> already SND_SOC_DPCM_STATE_HW_PARAMS, there is no need to call it again. 
> Similar to how dpcm_be_dai_prepare() and dpcm_be_dai_startup() won't 
> call BE DAI prepare() / open() callbacks if the state is already 
> SND_SOC_DPCM_STATE_PREPARE or SND_SOC_DPCM_STATE_OPEN respectively.
> 
> For example:
> 
> When two or more streams sharing a common BE DAI are started 
> consecutively, the following sequence can cause multiple unnecessary 
> hw_params() calls for the BE DAI:
> (1) Stream1 hw_params() called for BE DAI and updates state to
>     SND_SOC_DPCM_STATE_HW_PARAMS
> (2) Before Stream1 BE DAI prepare() call triggered,
>     Stream2 dpcm_be_dai_hw_params() call happened and now BE DAI 
> hw_params() callback happen again unnecessarily as the state of BE DAI 
> is still SND_SOC_DPCM_STATE_HW_PARAMS.

It's possible that the second call uses different parameters. Don't make 
the assumption that the parameters are exactly the same between FEs, see 
more below.

>> One key point is that hw_params() may be called multiple times with 
>> different parameters, it's a feature and not a bug. If a call to 
>> hw_params() changes an internal state, a follow-up call to hw_params() 
>> shall undo the initial state change and rerun the appropriate 
>> configuration.
> 
> As per my understanding, after hw_params() callback, prepare() will be 
> called and the BE DAI state will be updated to PREPARE now, then 
> hw_params() callback for same BE DAI will not occur as condition will be 
> unmet. Not sure how the different parameters will be configured in this 
> case? Please clarify.

prepare() is a different stage, you shouldn't assume that the transition 
between hw_params() and prepare() is immediate.

You could have two FEs that try to configure different parameters, i.e. 
S16LE or S32LE before the prepare stage is reached. Not to mention that 
sound servers such as PulseAudio or PipeWire do make repeated calls to 
hw_params() with different configurations to figure out what is 
supported, i.e. prepare() and trigger() stages are not reached.

Note that the DPCM logic is far from perfect, in the case of shared BEs 
the hw_params should not be propagated from FEs. In the case of a mixer, 
it would be very useful to have a boundary between FE and BE, with the 
mixer pre-configured to e.g. 48kHz S32LE. The existing propagation from 
FE to BE can be problematic in that the last configuration will be 
retained, and that's not necessarily the highest resolution. We had 
similar problems in the past when we tried to be smart with PulseAudio 
and automatically select 44.1kHz or 48 kHz. That causes all kinds of 
issues where the change of configuration only happens when all streams 
are idle, and while a stream is active the configuration cannot be 
reconfigured. It's much simpler to decide what the BE settings are once 
and not try to adapt.

In other words, the existing logic is fine, only redundant in the case 
of *identical* hw_params for all streams feeding into a common BE. I 
don't feel it's worth changing for now, if we change something in the 
logic that should be a bigger change to add constraints between domains, 
and allow for more than two domains.

>>>> Signed-off-by: Sheetal <sheetal@nvidia.com>
>>>> ---
>>>> Changes in v2:
>>>> - Update commit message as its not a fix.
>>>> - Marked as RFC patch as it requires feedback from other users
>>>>    perspective as well.
>>>> - The patch is being sent separately as other patch is not RFC.
>>>>
>>>>   sound/soc/soc-pcm.c | 1 -
>>>>   1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
>>>> index d7f6d3a6d312..c73be27c4ecb 100644
>>>> --- a/sound/soc/soc-pcm.c
>>>> +++ b/sound/soc/soc-pcm.c
>>>> @@ -2123,7 +2123,6 @@ int dpcm_be_dai_hw_params(struct 
>>>> snd_soc_pcm_runtime *fe, int stream)
>>>>                       continue;
>>>>
>>>>               if ((be->dpcm[stream].state != 
>>>> SND_SOC_DPCM_STATE_OPEN) &&
>>>> -                (be->dpcm[stream].state != 
>>>> SND_SOC_DPCM_STATE_HW_PARAMS) &&
>>>>                   (be->dpcm[stream].state != 
>>>> SND_SOC_DPCM_STATE_HW_FREE))
>>>>                       continue;
>>>>
>>>
>>
> 
>