[PATCH] ASoC: soc-dai: pull out be_hw_params_fixup from snd_soc_dai_hw_params

[Pierre-Louis Bossart]

On 5/11/20 11:47 PM, Gyeongtaek Lee wrote:
> On 5/12/20 03:47 AM, Pierre-Louis Bossart wrote:
>> On 5/10/20 10:31 PM, Gyeongtaek Lee wrote:
>>> When dpcm_be_dai_hw_params() called, be_hw_params_fixup() callback is
>>> called with same arguments 3times.
>>> It is called in be_hw_params_fixup() itself and in soc_pcm_hw_params()
>>> for cpu dai and codec dai.
>>> Tested in 5.4.
>>>
>>> Signed-off-by: Gyeongtaek Lee <gt82.lee at samsung.com>
>>> Cc: stable at vger.kernel.org
>>> ---
>>>    sound/soc/soc-dai.c  | 12 ------------
>>>    sound/soc/soc-dapm.c | 11 +++++++++++
>>>    2 files changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c
>>> index 31c41559034b..4785cb6b336f 100644
>>> --- a/sound/soc/soc-dai.c
>>> +++ b/sound/soc/soc-dai.c
>>> @@ -257,20 +257,8 @@ int snd_soc_dai_hw_params(struct snd_soc_dai *dai,
>>>    			  struct snd_pcm_substream *substream,
>>>    			  struct snd_pcm_hw_params *params)
>>>    {
>>> -	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>>>    	int ret;
>>>    
>>> -	/* perform any topology hw_params fixups before DAI  */
>>> -	if (rtd->dai_link->be_hw_params_fixup) {
>>> -		ret = rtd->dai_link->be_hw_params_fixup(rtd, params);
>>> -		if (ret < 0) {
>>> -			dev_err(rtd->dev,
>>> -				"ASoC: hw_params topology fixup failed %d\n",
>>> -				ret);
>>> -			return ret;
>>> -		}
>>> -	}
>>> -
>>
>> Sorry I don't get this change.
>>
>> If the be_hw_params_fixup() callback is called three times, it's because
>> the soc_soc_dai_hw_params() routine is called three times, so what is
>> the problem here?
>>
>> Also the comment is explicit about doing fixups before calling the dai
>> hw_params() callback, so that is not longer the case with this change?
>> Even if the change was legit, the comment is no longer relevant and
>> should be updated.
>>
> Sorry, the comment was too short and inexact to explain the intention of the patch.
> When dpcm_be_dai_hw_params() called, be_hw_params_fixup() is called three times
> with same substream and params in dpcm_be_dai_hw_params() and
> snd_soc_dai_hw_params() in soc_pcm_hw_params() for cpu dai and codec dai.
> Calling same code three times may not be a problem in most systems, but in some
> system which has limited computing power and changes audio routing frequently,
> couple of milliseconds are consumed and make the system a little bit slower to
> audio response.
> If the be_hw_params_fixup() could be pull out from soc_soc_dai_hw_params(),
> and make it call once at pcm start or routing change, response time can be increased.

I accept the argument that be_hw_params_fixup() is called from two 
locations, and in the second one there is a iteration to deal with 
capture and playback. We should indeed check if calling the same 
function from two places is legit or not, you pointed to a legitimate 
issue. We currently use this fixup in SOF to make sure the BE 
configuration matches what the topology provides, and doing this match 3 
times is not very useful indeed.

That said, this is supposed to be a fixup, meaning just a change to the 
hw_params structure with no access to hardware. I believe that even if 
we reduce the number of calls you are not going to see any improvement 
in responses time, the actual time is still going to be spend in the 
hw_params() itself.

> In my search, the only point that calls snd_soc_dai_hw_params() without
> calling be_hw_params_fixup() callback directly is snd_soc_dai_link_event_pre_pmu().
> So, I proposed pulling out be_hw_params_fixup() from snd_soc_dai_hw_params() and then
> add direct call to be_hw_params_fixup() callback in snd_soc_dai_link_event_pre_pmu()
> not to harm current working process.

This change leaves the calls in soc-pcm, so you still have 3 calls to 
the same callback - and the same odd pattern where in one case the fixup 
is called in a direction-agnostic manner while in the two others it is 
called in a loop that's direction-based.

In other words my take on this is: valid problem, but let's try to see 
if we can remove redundant calls rather than move one around.

>>>    	if (dai->driver->ops->hw_params) {
>>>    		ret = dai->driver->ops->hw_params(substream, params, dai);
>>>    		if (ret < 0) {
>>> diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
>>> index e2632841b321..d86c1cd4e8fa 100644
>>> --- a/sound/soc/soc-dapm.c
>>> +++ b/sound/soc/soc-dapm.c
>>> @@ -3886,6 +3886,17 @@ snd_soc_dai_link_event_pre_pmu(struct snd_soc_dapm_widget *w,
>>>    	hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS)->max
>>>    		= config->channels_max;
>>>    
>>> +	/* perform any topology hw_params fixups before DAI  */
>>> +	if (rtd->dai_link->be_hw_params_fixup) {
>>> +		ret = rtd->dai_link->be_hw_params_fixup(rtd, params);
>>> +		if (ret < 0) {
>>> +			dev_err(rtd->dev,
>>> +				"ASoC: hw_params topology fixup failed %d\n",
>>> +				ret);
>>> +			return ret;
>>> +		}
>>> +	}
>>> +
>>>    	substream->stream = SNDRV_PCM_STREAM_CAPTURE;
>>>    	snd_soc_dapm_widget_for_each_source_path(w, path) {
>>>    		source = path->source->priv;
>>>
>>> base-commit: f3643491bd079c973ac6c693da7966cd17506ca3
>>>
>>
>