[alsa-devel] Question about snd_pcm_limit_hw_rates() call timing

[Lars-Peter Clausen]

On 1/21/20 7:58 AM, Takashi Iwai wrote:
> On Tue, 21 Jan 2020 02:54:26 +0100,
> Kuninori Morimoto wrote:
>>
>>
>> Hi ALSA ML
>>
>> soc-pcm has snd_pcm_limit_hw_rates() which determine rate_min/rate_max fields.
>> It updates runtime->hw.rate_min/max (A) based on hw->rates (B).
>>
>> 	int snd_pcm_limit_hw_rates(struct snd_pcm_runtime *runtime)
>> 	{
>> 		int i;
>> 		for (i = 0; i < (int)snd_pcm_known_rates.count; i++) {
>> (B)			if (runtime->hw.rates & (1 << i)) {
>> (A)				runtime->hw.rate_min = snd_pcm_known_rates.list[i];
>> 				break;
>> 			}
>> 		}
>> 		for (i = (int)snd_pcm_known_rates.count - 1; i >= 0; i--) {
>> (B)			if (runtime->hw.rates & (1 << i)) {
>> (A)				runtime->hw.rate_max = snd_pcm_known_rates.list[i];
>> 				break;
>> 			}
>> 		}
>> 		return 0;
>> 	}
>>
>> I guess the calling timing is
>>
>> 	1) set hw->rates
>> 	2) call snd_pcm_limit_hw_rates()
>> 	3) update hw->rate_min/max
>>
>> soc_pcm_init_runtime_hw() is calling it as this order
>>
>> 	static void soc_pcm_init_runtime_hw(xxx)
>> 	{
>> 		...
>> 1)		hw->rates = snd_pcm_rate_mask_intersect(rates, cpu_stream->rates);
>>
>> 2)		snd_pcm_limit_hw_rates(runtime);
>>
>> 3)		hw->rate_min = max(hw->rate_min, cpu_stream->rate_min);
>> 		hw->rate_min = max(hw->rate_min, rate_min);
>> 		hw->rate_max = min_not_zero(hw->rate_max, cpu_stream->rate_max);
>> 		hw->rate_max = min_not_zero(hw->rate_max, rate_max);
>> 	}
>>
>> But, dpcm_fe_dai_startup() are different.
>>
>> 	static int dpcm_fe_dai_startup(xxx)
>> 	{
>> 		...
>> 		/*
>> 		 * dpcm_set_fe_runtime() updates runtime->hw.xxx
>> 		 */
>> 1) 3)		dpcm_set_fe_runtime(fe_substream);
>> 2)		snd_pcm_limit_hw_rates(runtime);
>> 		...
>> 	}
>>
>> I guess we need fixup dpcm_fe_dai_startup() ?
> 
> A good catch.
> 
> Actually the question is whether we need snd_pcm_limit_hw_rates() call
> or not.  The current code in soc_pcm_init_runtime_hw() assumes that
> each cpu and codec dais already set the proper rate_min and rate_max,
> and narrow the range accordingly.  So basically the call there is
> superfluous.  OTOH, in dpcm_fe_dai_startup(), the call overrides the
> existing rate_min/max setup as you mentioned, so it may be wrong.
> 
> Or, better to ask -- is there any case that snd_pcm_limit_hw_rates()
> is mandatory in ASoC?  The snd_pcm_limit_hw_rates() is for setting up
> rates_min and rates_max from rates bits.  It's a function to be called
> only when we know that rates bits contain the full information and
> rates_min and rates_max are bogus.  That is, its overriding behavior
> is designed.
> 
> OTOH, if the driver sets up already valid rates_min and rates_max
> values, there is no need to call this function at all.

Usually a driver either sets rate_min/rate_max or it provides a discrete 
rate mask. Most drivers typically only provide the rate mask since they 
only support a discrete set of rates.

The generic code is supposed to support both cases, and even the rare 
case were both are set.

- Lars