From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lars-Peter Clausen Subject: Re: [PATCH 2/2] ASoC: pcm: Always honor DAI min and max sample rate constraints Date: Wed, 27 Nov 2013 12:18:24 +0100 Message-ID: <5295D500.50607@metafoo.de> References: <1385542698-13362-1-git-send-email-lars@metafoo.de> <1385542698-13362-2-git-send-email-lars@metafoo.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from smtp-out-025.synserver.de (smtp-out-121.synserver.de [212.40.185.121]) by alsa0.perex.cz (Postfix) with ESMTP id AB52D2625C2 for ; Wed, 27 Nov 2013 12:17:50 +0100 (CET) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: Jean-Francois Moine , alsa-devel@alsa-project.org, Mark Brown , Liam Girdwood List-Id: alsa-devel@alsa-project.org 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 >> --- >> 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