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

[Takashi Iwai]

At Wed, 27 Nov 2013 12:18:24 +0100,
Lars-Peter Clausen wrote:
> 
> 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 at 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().

Right.  We should catch all them once, then add a sanity check
something like
	WARN_ON((rates & SNDRV_PCM_RATE_KNOT) && \
		(rates & ~SNDRV_PCM_RATE_KNOT));
	WARN_ON((rates & SNDRV_PCM_RATE_CONTINUOUS) && \
		(rates & ~SNDRV_PCM_RATE_CONTINUOUS));

Takashi