[alsa-devel] [PATCH 1/2] ASoC: soc_pcm_init_runtime_hw: Fix rate_max calculation
In order to make sure that the sample rate is in the supported range of both components the maximum rate of the card should be the minimum of the maximum rate of each components. There is one special case to consider though, if max_rate is set to 0 this means there is no maximum specified, so use min_not_zero() macro which will give use the desired result.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de --- sound/soc/soc-pcm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 170ff96..c4ef880 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -220,7 +220,7 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_hardware *hw, struct snd_soc_pcm_stream *cpu_stream) { hw->rate_min = max(codec_stream->rate_min, cpu_stream->rate_min); - hw->rate_max = max(codec_stream->rate_max, cpu_stream->rate_max); + hw->rate_max = min_not_zero(codec_stream->rate_max, cpu_stream->rate_max); hw->channels_min = max(codec_stream->channels_min, cpu_stream->channels_min); hw->channels_max = min(codec_stream->channels_max,
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); + + hw->rate_min = max(hw->rate_min, cpu_stream->rate_min); + hw->rate_min = max(hw->rate_min, codec_stream->rate_min); + hw->rate_max = min_not_zero(hw->rate_max, cpu_stream->rate_max); + hw->rate_max = min_not_zero(hw->rate_max, codec_stream->rate_max); }
/* @@ -302,15 +309,14 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
/* Check that the codec and cpu DAIs are compatible */ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { - soc_pcm_init_runtime_hw(&runtime->hw, &codec_dai_drv->playback, + soc_pcm_init_runtime_hw(runtime, &codec_dai_drv->playback, &cpu_dai_drv->playback); } else { - soc_pcm_init_runtime_hw(&runtime->hw, &codec_dai_drv->capture, + soc_pcm_init_runtime_hw(runtime, &codec_dai_drv->capture, &cpu_dai_drv->capture); }
ret = -EINVAL; - snd_pcm_limit_hw_rates(runtime); if (!runtime->hw.rates) { printk(KERN_ERR "ASoC: %s <-> %s No matching rates\n", codec_dai->name, cpu_dai->name);
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.
The only missing thing is the conflict between CONTINUOUS and KNOT. Could you put the fix I suggested into this series, too?
In anyway, feel free to take my ack for both patches Acked-by: Takashi iwai tiwai@suse.de
thanks,
Takashi
- hw->rate_min = max(hw->rate_min, cpu_stream->rate_min);
- hw->rate_min = max(hw->rate_min, codec_stream->rate_min);
- hw->rate_max = min_not_zero(hw->rate_max, cpu_stream->rate_max);
- hw->rate_max = min_not_zero(hw->rate_max, codec_stream->rate_max);
}
/* @@ -302,15 +309,14 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
/* Check that the codec and cpu DAIs are compatible */ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
soc_pcm_init_runtime_hw(&runtime->hw, &codec_dai_drv->playback,
soc_pcm_init_runtime_hw(runtime, &codec_dai_drv->playback, &cpu_dai_drv->playback);
soc_pcm_init_runtime_hw(&runtime->hw, &codec_dai_drv->capture,
soc_pcm_init_runtime_hw(runtime, &codec_dai_drv->capture, &cpu_dai_drv->capture);}
ret = -EINVAL;
- snd_pcm_limit_hw_rates(runtime); if (!runtime->hw.rates) { printk(KERN_ERR "ASoC: %s <-> %s No matching rates\n", codec_dai->name, cpu_dai->name);
-- 1.8.0
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
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@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
On Wed, Nov 27, 2013 at 12:26:29PM +0100, Takashi Iwai wrote:
Lars-Peter Clausen wrote:
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
It seems neater if snd_pcm_limit_hw_rates() does the right thing no matter what the rates are - this centralises the logic for handling this which reduces the complexity for drivers.
WARN_ON((rates & SNDRV_PCM_RATE_KNOT) && \ (rates & ~SNDRV_PCM_RATE_KNOT)); WARN_ON((rates & SNDRV_PCM_RATE_CONTINUOUS) && \ (rates & ~SNDRV_PCM_RATE_CONTINUOUS));
This does make sense, though we'll need to change the constraint merging code in ASoC since right now it assumes that combining the masks will do the merge which won't be the case if you have devices with one of these flags together with any other flags (either specific rates or the other non-bitmask one). It would be nice if we were just throwing the device constraints into the core constraint code and letting it merge them together.
At Wed, 27 Nov 2013 12:21:53 +0000, Mark Brown wrote:
On Wed, Nov 27, 2013 at 12:26:29PM +0100, Takashi Iwai wrote:
Lars-Peter Clausen wrote:
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
It seems neater if snd_pcm_limit_hw_rates() does the right thing no matter what the rates are - this centralises the logic for handling this which reduces the complexity for drivers.
Well, the difficult point is to decide what is right, if the given condition is already wrong (e.g. rate_min/max don't match with the RATE bits). We can accept it somehow silently (as of now), but I think it's better to warn (and/or refuse).
WARN_ON((rates & SNDRV_PCM_RATE_KNOT) && \ (rates & ~SNDRV_PCM_RATE_KNOT)); WARN_ON((rates & SNDRV_PCM_RATE_CONTINUOUS) && \ (rates & ~SNDRV_PCM_RATE_CONTINUOUS));
This does make sense, though we'll need to change the constraint merging code in ASoC since right now it assumes that combining the masks will do the merge which won't be the case if you have devices with one of these flags together with any other flags (either specific rates or the other non-bitmask one). It would be nice if we were just throwing the device constraints into the core constraint code and letting it merge them together.
I think Lars' fix will handle such a case nicely now; at least, merging between RATE bits and rate_min/max is solved, as long as the given conditions are consistent.
But yes, we can certainly put some sanity checks and more handling code into a core stuff. But the drivers giving wrong parameters must be fixed in anyway. IIRC, even ac97_codec.c does something misaligned. Warnings would be a help for hunting such ones, and I didn't intend it being more than that in WARN_ON() above.
thanks,
Takashi
On Wed, Nov 27, 2013 at 02:13:21PM +0100, Takashi Iwai wrote:
Mark Brown wrote:
It seems neater if snd_pcm_limit_hw_rates() does the right thing no matter what the rates are - this centralises the logic for handling this which reduces the complexity for drivers.
Well, the difficult point is to decide what is right, if the given condition is already wrong (e.g. rate_min/max don't match with the RATE bits). We can accept it somehow silently (as of now), but I think it's better to warn (and/or refuse).
I agree it's better to warn if it's wrong, I was more responding to the comment about it not being quite correct to call snd_pcm_limit_hw_rates() with _KNOT or _CONTINUOUS.
I think Lars' fix will handle such a case nicely now; at least, merging between RATE bits and rate_min/max is solved, as long as the given conditions are consistent.
Yes, it should work for the time being. It just seems silly to have more than one set of merging code, it just increases the chances we'll get stuff wrong - switching to use the core constraint merging code should avoid the possibility of such errors in future.
But yes, we can certainly put some sanity checks and more handling code into a core stuff. But the drivers giving wrong parameters must be fixed in anyway. IIRC, even ac97_codec.c does something misaligned. Warnings would be a help for hunting such ones, and I didn't intend it being more than that in WARN_ON() above.
I'm talking about the code in ASoC that combines the constraints from the devices on the link here rather than the drivers, as you say the drivers ought to have correct constraints standalone.
On 11/27/2013 03:49 PM, Mark Brown wrote: [...]
I think Lars' fix will handle such a case nicely now; at least, merging between RATE bits and rate_min/max is solved, as long as the given conditions are consistent.
Yes, it should work for the time being. It just seems silly to have more than one set of merging code, it just increases the chances we'll get stuff wrong - switching to use the core constraint merging code should avoid the possibility of such errors in future.
There isn't such code in the ALSA core yet, but I guess it makes sense to add it.
- Lars
On Wed, Nov 27, 2013 at 09:58:18AM +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
Applied, thanks.
On Wed, Nov 27, 2013 at 09:58:17AM +0100, Lars-Peter Clausen wrote:
In order to make sure that the sample rate is in the supported range of both components the maximum rate of the card should be the minimum of the maximum rate of each components. There is one special case to consider though, if max_rate is set to 0 this means there is no maximum specified, so use min_not_zero() macro which will give use the desired result.
Applied, thanks.
participants (3)
-
Lars-Peter Clausen -
Mark Brown -
Takashi Iwai