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

On Tue, 21 Jan 2020 09:11:06 +0100, Kuninori Morimoto wrote:

Hi Takashi

Thank you for feedback

...

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.

OK, it is same as my understanding. I want to do (so far) is just cleanup. I will fixup/cleanup it.

I'm posting ALSA SoC cleanup patches to ML. This fixup/cleanup will be added to my ALSA-SoC-cleanup-patch-queue

Actually as Lars pointed out in another reply, the current code of soc_pcm_init_runtime_hw() is correct. It's a bit tricky but smartly handling the cases: when rate_min/max are to be overridden by rates bits, they are supposed to be zero, and min_not_zero() does the trick. Also, snd_pcm_rate_mask_intersect() sanitizes the bits when CONTINUOUS or KNOT is set, so the spurious rate bits won't be reflected in snd_pcm_limit_hw_rates(), too.

But still the dpcm_fe_dai_startup() needs to be fixed as you mentioned. Though, I guess we need to fix not there but rather other places (e.g. dpcm_set_fe_runtime() itself).

thanks,

Takashi