From: Sheetal <sheetal@nvidia.com>

The hw_params() function for BE DAI was being called multiple times due
to an unnecessary SND_SOC_DPCM_STATE_HW_PARAMS state check.

Remove the redundant state check to ensure hw_params() is called only once
per BE DAI configuration.

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;
 
-- 
2.17.1

On 08/04/2025 11:30, Sheetal . wrote:
> From: Sheetal <sheetal@nvidia.com>
> 
> The hw_params() function for BE DAI was being called multiple times due
> to an unnecessary SND_SOC_DPCM_STATE_HW_PARAMS state check.
> 
> Remove the redundant state check to ensure hw_params() is called only once
> per BE DAI configuration.

The first sentence tells that the hw_params() of the BE is called
multiple times.

The second sentence states that the check is redundant then tells that
it is removed to not call the hw_params() of the BE, so the check was
not redundant, it got exercised.

Which one is true?

Under what circumstance the __soc_pcm_hw_params() got called multiple
times? Was it normal or was it error? What causes it?

> 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;
>  

-- 
Péter

On 5/13/25 13:10, Péter Ujfalusi wrote:
> 
> 
> On 08/04/2025 11:30, Sheetal . wrote:
>> From: Sheetal <sheetal@nvidia.com>
>>
>> The hw_params() function for BE DAI was being called multiple times due
>> to an unnecessary SND_SOC_DPCM_STATE_HW_PARAMS state check.
>>
>> Remove the redundant state check to ensure hw_params() is called only once
>> per BE DAI configuration.
> 
> The first sentence tells that the hw_params() of the BE is called
> multiple times.
> 
> The second sentence states that the check is redundant then tells that
> it is removed to not call the hw_params() of the BE, so the check was
> not redundant, it got exercised.
> 
> Which one is true?
> 
> Under what circumstance the __soc_pcm_hw_params() got called multiple
> times? Was it normal or was it error? What causes it?

I share Peter's question. The code has been around since 2016, in what case would the existing logic need to be updated?

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.

>> 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;
>>  
> 

On 23-05-2025 15:25, Pierre-Louis Bossart wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 5/13/25 13:10, Péter Ujfalusi wrote:
>>
>>
>> On 08/04/2025 11:30, Sheetal . wrote:
>>> From: Sheetal <sheetal@nvidia.com>
>>>
>>> The hw_params() function for BE DAI was being called multiple times due
>>> to an unnecessary SND_SOC_DPCM_STATE_HW_PARAMS state check.
>>>
>>> Remove the redundant state check to ensure hw_params() is called only once
>>> per BE DAI configuration.
>>
>> The first sentence tells that the hw_params() of the BE is called
>> multiple times.
>>
>> The second sentence states that the check is redundant then tells that
>> it is removed to not call the hw_params() of the BE, so the check was
>> not redundant, it got exercised.
>>
>> Which one is true?
>>
>> Under what circumstance the __soc_pcm_hw_params() got called multiple
>> times? Was it normal or was it error? What causes it?
> 
> 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.


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

> 
>>> 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;
>>>
>>
> 

>> 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;
>>>>
>>>
>>
> 
> 

On 08-04-2025 14:00, Sheetal . wrote:
> From: Sheetal <sheetal@nvidia.com>
>
> The hw_params() function for BE DAI was being called multiple times due
> to an unnecessary SND_SOC_DPCM_STATE_HW_PARAMS state check.
>
> Remove the redundant state check to ensure hw_params() is called only once
> per BE DAI configuration.
>
> 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;
>   

Reviewed-by: Sameer Pujar <spujar@nvidia.com>


Earlier Intel systems needed multiple hw_params() call and I am not sure 
if that still holds good. Given https://lkml.org/lkml/2021/9/28/1267, it 
would be good to get feedback from Intel and I have added few people 
based on the earlier discussion.

On 12/05/2025 15:01, Sameer Pujar wrote:
> 
> 
> 
> On 08-04-2025 14:00, Sheetal . wrote:
>> From: Sheetal <sheetal@nvidia.com>
>>
>> The hw_params() function for BE DAI was being called multiple times due
>> to an unnecessary SND_SOC_DPCM_STATE_HW_PARAMS state check.
>>
>> Remove the redundant state check to ensure hw_params() is called only
>> once
>> per BE DAI configuration.
>>
>> 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;
>>   
> 
> Reviewed-by: Sameer Pujar <spujar@nvidia.com>
> 
> 
> Earlier Intel systems needed multiple hw_params() call and I am not sure
> if that still holds good. Given https://lkml.org/lkml/2021/9/28/1267, it
> would be good to get feedback from Intel and I have added few people
> based on the earlier discussion.

Picked the patch to run it through our CI:
https://github.com/thesofproject/linux/pull/5414

-- 
Péter

On 13-05-2025 11:45, Péter Ujfalusi wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 12/05/2025 15:01, Sameer Pujar wrote:
>>
>>
>>
>> On 08-04-2025 14:00, Sheetal . wrote:
>>> From: Sheetal <sheetal@nvidia.com>
>>>
>>> The hw_params() function for BE DAI was being called multiple times due
>>> to an unnecessary SND_SOC_DPCM_STATE_HW_PARAMS state check.
>>>
>>> Remove the redundant state check to ensure hw_params() is called only
>>> once
>>> per BE DAI configuration.
>>>
>>> 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;
>>>
>>
>> Reviewed-by: Sameer Pujar <spujar@nvidia.com>
>>
>>
>> Earlier Intel systems needed multiple hw_params() call and I am not sure
>> if that still holds good. Given https://lkml.org/lkml/2021/9/28/1267, it
>> would be good to get feedback from Intel and I have added few people
>> based on the earlier discussion.
> 
> Picked the patch to run it through our CI:
> https://github.com/thesofproject/linux/pull/5414

Please share feedback or comments if any.

> 
> --
> Péter
> 

On 21/05/2025 08:25, Sheetal . wrote:
>>
>> Picked the patch to run it through our CI:
>> https://github.com/thesofproject/linux/pull/5414
> 
> Please share feedback or comments if any.

sorry fort he delay, but we have CI maintenance and movement.

The patch looks good and makes sense, it was just hard to tell from the
commit message what is the reason for it.

Reviewed-by: Peter Ujfalusi <peter.ujflausi@linux.intel.com>

On 21-05-2025 13:47, Péter Ujfalusi wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 21/05/2025 08:25, Sheetal . wrote:
>>>
>>> Picked the patch to run it through our CI:
>>> https://github.com/thesofproject/linux/pull/5414
>>
>> Please share feedback or comments if any.
> 
> sorry fort he delay, but we have CI maintenance and movement.
> 
> The patch looks good and makes sense, it was just hard to tell from the
> commit message what is the reason for it.

Thanks for the feedback. I will send [RFC PATCH v3] with the refined 
commit message.

> 
> Reviewed-by: Peter Ujfalusi <peter.ujflausi@linux.intel.com>
> 

Hi,

This patch should be a V1 and the "changes in v2" were based upon an 
initial internal review.

Please let me know if the change should be re-sent as V1.


On 08-04-2025 14:00, Sheetal . wrote:
> From: Sheetal <sheetal@nvidia.com>
>
> The hw_params() function for BE DAI was being called multiple times due
> to an unnecessary SND_SOC_DPCM_STATE_HW_PARAMS state check.
>
> Remove the redundant state check to ensure hw_params() is called only once
> per BE DAI configuration.
>
> 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;
>