Re: [PATCH 2/2] ASoC: pcm: Always honor DAI min and max sample rate constraints

[Lars-Peter Clausen <lars@metafoo.de>]

On 11/27/2013 11:31 AM, Takashi Iwai wrote:
> At Wed, 27 Nov 2013 09:58:18 +0100,
> Lars-Peter Clausen wrote:
>>
>> snd_pcm_limit_hw_rates() will initialize the minimum and maximum sample rate for
>> the PCM stream based on the rates specified in the rates field. Since we call
>> snd_pcm_limit_hw_rates() after soc_pcm_init_runtime_hw() it will essentially
>> overwrite the min and max rate set in soc_pcm_init_runtime_hw(). This may cause
>> the minimum or maximum rate to be set to a value outside the range of one of the
>> components if one of the components sets either SNDRV_PCM_RATE_CONTINUOUS or
>> SNDRV_PCM_RATE_KNOT and the other component specified a discrete rate via
>> SNDRV_PCM_RATE_[0-9]* that is outside of the first component's rate range. To
>> fix this first calculate the minimum and maximum rates using
>> snd_pcm_limit_hw_rates() and then on top of that apply the contraints specified
>> in the snd_soc_pcm_stream structs.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>>  sound/soc/soc-pcm.c | 18 ++++++++++++------
>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
>> index c4ef880..22af038 100644
>> --- a/sound/soc/soc-pcm.c
>> +++ b/sound/soc/soc-pcm.c
>> @@ -215,12 +215,12 @@ static void soc_pcm_apply_msb(struct snd_pcm_substream *substream,
>>  	}
>>  }
>>  
>> -static void soc_pcm_init_runtime_hw(struct snd_pcm_hardware *hw,
>> +static void soc_pcm_init_runtime_hw(struct snd_pcm_runtime *runtime,
>>  	struct snd_soc_pcm_stream *codec_stream,
>>  	struct snd_soc_pcm_stream *cpu_stream)
>>  {
>> -	hw->rate_min = max(codec_stream->rate_min, cpu_stream->rate_min);
>> -	hw->rate_max = min_not_zero(codec_stream->rate_max, cpu_stream->rate_max);
>> +	struct snd_pcm_hardware *hw = &runtime->hw;
>> +
>>  	hw->channels_min = max(codec_stream->channels_min,
>>  		cpu_stream->channels_min);
>>  	hw->channels_max = min(codec_stream->channels_max,
>> @@ -233,6 +233,13 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_hardware *hw,
>>  	if (cpu_stream->rates
>>  		& (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS))
>>  		hw->rates |= codec_stream->rates;
>> +
>> +	snd_pcm_limit_hw_rates(runtime);
> 
> Strictly speaking, snd_pcm_limit_hw_rates() should be applied only in
> case without KNOT nor CONTINUOUS flag.  But, I guess this would work
> better as is since there might be drivers that don't give proper
> rate_min and rate_max but rely on the rate bits, and min_not_zero()
> does the right thing in the code below.

My thinking as well. There are some driver which do set CONTINUOUS or KNOT,
but don't specify a minimum and maximum rate and probably rely on the fact
that we always call snd_pcm_limit_hw_rates() (E.g. the kirkwood drivers do
this). In my opinion those drivers should be fixed and once that is done we
can add a check that skips snd_pcm_limit_hw_rates() if KNOT or CONTINUOUS is
set. The current behavior isn't optimal e.g. if a driver sets
SNDRV_PCM_RATE_CONTINUOUS and SNDRV_PCM_RATE_8000_192000 and rate_max to
384000, we'd still end up with a maximum rate of 192000 because of
snd_pcm_limit_hw_rates().

> 
> The only missing thing is the conflict between CONTINUOUS and KNOT.
> Could you put the fix I suggested into this series, too?

ok.

- Lars