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

[Takashi Iwai]

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.


thanks,

Takashi