This patch change the judge timing about playing/capturing PCM rate.
Original code set constraints list of PCM rate limits at set_sysclk.
This strategy works well if system is using fixed rate clock.
But some boards and SoC (such as RockPro64 and RockChip I2S) has
connected SoC MCLK out to ES8316 MCLK in. In this case, SoC side I2S
will choose suitable frequency of MCLK such as fs * mclk-fs when
user starts playing or capturing.
Bad scenario as follows (mclk-fs = 256):
- Initialize sysclk by correct value (Ex. 12.288MHz)
- ES8316 set constraints of PCM rate by sysclk
48kHz (1/256), 32kHz (1/384), 30.720kHz (1/400),
24kHz (1/512), 16kHz (1/768), 12kHz (1/1024)
- Play 48kHz sound, it's acceptable
- Sysclk is not changed
- Play 32kHz sound, it's acceptable
- Set sysclk by 8.192MHz (= fs * mclk-fs = 32k * 256)
- ES8316 set constraints of PCM rate by sysclk
32kHz (1/256), 21.33kHz (1/384), 20.48kHz (1/400),
16kHz (1/512), 10.66kHz (1/768), 8kHz (1/1024)
- Play 48kHz again, but it's NOT acceptable because constraints
list does not allow 48kHz
Root cause of this strange behavior is changing constraints list at
set_sysclk timing. It seems that is too early to determine. So this
patch does not use constraints list and check PCM rate limit more
later timing at hw_params.
Signed-off-by: Katsuhiro Suzuki <[email protected]>
---
sound/soc/codecs/es8316.c | 37 +++++++++++++++----------------------
1 file changed, 15 insertions(+), 22 deletions(-)
diff --git a/sound/soc/codecs/es8316.c b/sound/soc/codecs/es8316.c
index ed2959dbe1fb..229808fa627c 100644
--- a/sound/soc/codecs/es8316.c
+++ b/sound/soc/codecs/es8316.c
@@ -363,27 +363,12 @@ static int es8316_set_dai_sysclk(struct snd_soc_dai *codec_dai,
{
struct snd_soc_component *component = codec_dai->component;
struct es8316_priv *es8316 = snd_soc_component_get_drvdata(component);
- int i;
- int count = 0;
es8316->sysclk = freq;
if (freq == 0)
return 0;
- /* Limit supported sample rates to ones that can be autodetected
- * by the codec running in slave mode.
- */
- for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) {
- const unsigned int ratio = supported_mclk_lrck_ratios[i];
-
- if (freq % ratio == 0)
- es8316->allowed_rates[count++] = freq / ratio;
- }
-
- es8316->sysclk_constraints.list = es8316->allowed_rates;
- es8316->sysclk_constraints.count = count;
-
return 0;
}
@@ -449,13 +434,6 @@ static int es8316_pcm_startup(struct snd_pcm_substream *substream,
return -EINVAL;
}
- /* The set of sample rates that can be supported depends on the
- * MCLK supplied to the CODEC.
- */
- snd_pcm_hw_constraint_list(substream->runtime, 0,
- SNDRV_PCM_HW_PARAM_RATE,
- &es8316->sysclk_constraints);
-
return 0;
}
@@ -466,12 +444,27 @@ static int es8316_pcm_hw_params(struct snd_pcm_substream *substream,
struct snd_soc_component *component = dai->component;
struct es8316_priv *es8316 = snd_soc_component_get_drvdata(component);
u8 wordlen = 0;
+ int i;
if (!es8316->sysclk) {
dev_err(component->dev, "No MCLK configured\n");
return -EINVAL;
}
+ /* Limit supported sample rates to ones that can be autodetected
+ * by the codec running in slave mode.
+ */
+ for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) {
+ const unsigned int ratio = supported_mclk_lrck_ratios[i];
+
+ if (es8316->sysclk % ratio != 0)
+ continue;
+ if (es8316->sysclk / ratio == params_rate(params))
+ break;
+ }
+ if (i == NR_SUPPORTED_MCLK_LRCK_RATIOS)
+ return -EINVAL;
+
switch (params_format(params)) {
case SNDRV_PCM_FORMAT_S16_LE:
wordlen = ES8316_SERDATA2_LEN_16;
--
2.23.0.rc1
On Wed, Sep 04, 2019 at 01:53:19AM +0900, Katsuhiro Suzuki wrote:
> Root cause of this strange behavior is changing constraints list at
> set_sysclk timing. It seems that is too early to determine. So this
> patch does not use constraints list and check PCM rate limit more
> later timing at hw_params.
hw_params is a bit late to impose constraints, you want them to be
available to be available to the application before it gets as far as
picking the parameters it wants so that you don't get hw_params failing
due to an invalid configuration. That makes everything run more
smoothly, applications should be able to trust the constraints they got
and some will not handle failures well.
The way this works with the variable MCLKs is that you end up in one of
two cases (wm8731 and wm8741 do this):
1. The system is idle, MCLK is set to 0. In this case no constraints
are set and we just set MCLK to whatever is required in hw_params()
in the machine driver.
2. One direction is active, MCLK is set to whatever that needed. In
this case startup() sets constraints derived from the MCLK.
There are races in this if streams are being started and torn down
simultaneously, there's not much we can do about them with the API the
way it is so we do have to validate in hw_params() anyway but it should
be validation not constraint imposition.
If the system has a fixed MCLK it just sets that on probe then we always
get the constraints applied on startup through the same code that
handles case 2.
Hello Mark,
On 2019/09/04 2:48, Mark Brown wrote:
> On Wed, Sep 04, 2019 at 01:53:19AM +0900, Katsuhiro Suzuki wrote:
>
>> Root cause of this strange behavior is changing constraints list at
>> set_sysclk timing. It seems that is too early to determine. So this
>> patch does not use constraints list and check PCM rate limit more
>> later timing at hw_params.
>
> hw_params is a bit late to impose constraints, you want them to be
> available to be available to the application before it gets as far as
> picking the parameters it wants so that you don't get hw_params failing
> due to an invalid configuration. That makes everything run more
> smoothly, applications should be able to trust the constraints they got
> and some will not handle failures well.
>
> The way this works with the variable MCLKs is that you end up in one of
> two cases (wm8731 and wm8741 do this):
>
> 1. The system is idle, MCLK is set to 0. In this case no constraints
> are set and we just set MCLK to whatever is required in hw_params()
> in the machine driver.
> 2. One direction is active, MCLK is set to whatever that needed. In
> this case startup() sets constraints derived from the MCLK.
>
> There are races in this if streams are being started and torn down
> simultaneously, there's not much we can do about them with the API the
> way it is so we do have to validate in hw_params() anyway but it should
> be validation not constraint imposition.
>
> If the system has a fixed MCLK it just sets that on probe then we always
> get the constraints applied on startup through the same code that
> handles case 2.
>
Thank you for explanation. I agree with apply no constraints if MCLK is
set to 0, and suitable constraints if MCLK is set to other values like
as wm8731 and wm8741 drivers.
I'll change my patch set and send.
Would you tell me one more thing. I don't understand who sets MCLK to 0.
Is it needed original machine driver instead of audio-graph-card?
On my test environment (audio-graph-card + Rockchip I2S + ES8316), it
seems audio-graph-card has never called set_sysclk() with freq = 0 after
stop play/capture sound. So my env will go to bad scenario as I
described in this patch.
Best Regards,
Katsuhiro Suzuki
On Thu, Sep 05, 2019 at 12:06:23AM +0900, Katsuhiro Suzuki wrote:
> Would you tell me one more thing. I don't understand who sets MCLK to 0.
> Is it needed original machine driver instead of audio-graph-card?
> On my test environment (audio-graph-card + Rockchip I2S + ES8316), it
> seems audio-graph-card has never called set_sysclk() with freq = 0 after
> stop play/capture sound. So my env will go to bad scenario as I described in
> this patch.
You shouldn't need a custom machine driver - you'll just be the first
person who ran into this with audio-graph-card. I'd just add this
support to the audio-graph-card, either with custom startup and shutdown
callbacks or using a set_bias_level() callback (both get used, I'd guess
the set_bias_level() is easier since you don't need to reference count
anything).
On 2019/09/05 0:30, Mark Brown wrote:
> On Thu, Sep 05, 2019 at 12:06:23AM +0900, Katsuhiro Suzuki wrote:
>
>> Would you tell me one more thing. I don't understand who sets MCLK to 0.
>> Is it needed original machine driver instead of audio-graph-card?
>
>> On my test environment (audio-graph-card + Rockchip I2S + ES8316), it
>> seems audio-graph-card has never called set_sysclk() with freq = 0 after
>> stop play/capture sound. So my env will go to bad scenario as I described in
>> this patch.
>
> You shouldn't need a custom machine driver - you'll just be the first
> person who ran into this with audio-graph-card. I'd just add this
> support to the audio-graph-card, either with custom startup and shutdown
> callbacks or using a set_bias_level() callback (both get used, I'd guess
> the set_bias_level() is easier since you don't need to reference count
> anything).
>
Oh, I understand current situation. I'll try it.
Thanks for your support!
Best Regards,
Katsuhiro Suzuki