[alsa-devel] [PATCH RFC 0/3] tda998x updates for DAI formats and bclk_ratio
This series addresses two issues with TDA998x that have been identified:
1) Peter found that the I2S format was not being explicitly set, and retains its value from whatever was previously running on the platform. Work around this by implementing support for setting the I2S format from the DAI format, rather than merely defaulting the register back to its power-on value.
2) Sven found that TDA998x does not work on his Freescale platform, which always uses a 64·fs bitclock. The TDA998x driver was deriving this information from the sample width, which, while it works for Beagle Bone Black, does not allow the driver to be used with other I2S sources that may have different behaviours.
To work around that, we implement support for snd_soc_dai_set_bclk_ratio() in hdmi-codec, and propagate its value to TDA998x and other HDMI codecs via a new member. However, since snd_soc_dai_set_bclk_ratio() is never called, we need to avoid breaking any existing users, so we detect the lack of call by an impossible zero value, and subsitute a value corresponding with the TDA998x's old behaviour.
It is hoped that snd_soc_dai_set_bclk_ratio() will see more adoption in ASoC, and the TDA998x specific defaulting can be removed.
drivers/gpu/drm/i2c/tda998x_drv.c | 75 ++++++++++++++++++++++++++------------- include/drm/i2c/tda998x.h | 12 +++++-- include/sound/hdmi-codec.h | 1 + sound/soc/codecs/hdmi-codec.c | 45 +++++++++++++++++++++-- 4 files changed, 104 insertions(+), 29 deletions(-)
Some HDMI codecs need to know the relationship between the I2S bit clock and the I2S word clock in order to correctly generate the CTS value for audio clock recovery on the sink.
Add support for this, but there are currently no callers of snd_soc_dai_set_bclk_ratio(), we provide a default implementation that uses the sample width to derive the ratio from the 8-bit aligned sample size. This reflects the derivation that is in TDA998x, which we are going to convert to use this new support.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- include/sound/hdmi-codec.h | 1 + sound/soc/codecs/hdmi-codec.c | 45 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index 9483c55f871b..0fca69880dc3 100644 --- a/include/sound/hdmi-codec.h +++ b/include/sound/hdmi-codec.h @@ -42,6 +42,7 @@ struct hdmi_codec_daifmt { unsigned int frame_clk_inv:1; unsigned int bit_clk_master:1; unsigned int frame_clk_master:1; + unsigned int bclk_ratio; };
/* diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index e5b6769b9797..d71a7e5a2231 100644 --- a/sound/soc/codecs/hdmi-codec.c +++ b/sound/soc/codecs/hdmi-codec.c @@ -470,6 +470,7 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); + struct hdmi_codec_daifmt fmt; struct hdmi_codec_params hp = { .iec = { .status = { 0 }, @@ -520,8 +521,43 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, hp.sample_rate = params_rate(params); hp.channels = params_channels(params);
+ fmt = hcp->daifmt[dai->id]; + + /* + * If the .set_bclk_ratio() has not been called, default it + * using the sample width for compatibility for TDA998x. + * Rather than changing this, drivers should arrange to make + * an appropriate call to snd_soc_dai_set_bclk_ratio(). + */ + if (fmt.bclk_ratio == 0) { + switch (hp.sample_width) { + case 16: + fmt.bclk_ratio = 32; + break; + case 18: + case 20: + case 24: + fmt.bclk_ratio = 48; + break; + default: + fmt.bclk_ratio = 64; + break; + } + } + return hcp->hcd.ops->hw_params(dai->dev->parent, hcp->hcd.data, - &hcp->daifmt[dai->id], &hp); + &fmt, &hp); +} + +static int hdmi_codec_set_bclk_ratio(struct snd_soc_dai *dai, + unsigned int ratio) +{ + struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); + + /* FIXME: some validation here would be good? */ + hcp->daifmt[dai->id].bclk_ratio = ratio; + + return 0; }
static int hdmi_codec_set_fmt(struct snd_soc_dai *dai, @@ -593,7 +629,11 @@ static int hdmi_codec_set_fmt(struct snd_soc_dai *dai, } }
- hcp->daifmt[dai->id] = cf; + hcp->daifmt[dai->id].fmt = cf.fmt; + hcp->daifmt[dai->id].bit_clk_inv = cf.bit_clk_inv; + hcp->daifmt[dai->id].frame_clk_inv = cf.frame_clk_inv; + hcp->daifmt[dai->id].bit_clk_master = cf.bit_clk_master; + hcp->daifmt[dai->id].frame_clk_master = cf.frame_clk_master;
return ret; } @@ -615,6 +655,7 @@ static const struct snd_soc_dai_ops hdmi_dai_ops = { .startup = hdmi_codec_startup, .shutdown = hdmi_codec_shutdown, .hw_params = hdmi_codec_hw_params, + .set_bclk_ratio = hdmi_codec_set_bclk_ratio, .set_fmt = hdmi_codec_set_fmt, .digital_mute = hdmi_codec_digital_mute, };
On 22/02/2019 23:27, Russell King wrote:
Some HDMI codecs need to know the relationship between the I2S bit clock and the I2S word clock in order to correctly generate the CTS value for audio clock recovery on the sink.
Add support for this, but there are currently no callers of snd_soc_dai_set_bclk_ratio(), we provide a default implementation that uses the sample width to derive the ratio from the 8-bit aligned sample size. This reflects the derivation that is in TDA998x, which we are going to convert to use this new support.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk
include/sound/hdmi-codec.h | 1 + sound/soc/codecs/hdmi-codec.c | 45 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index 9483c55f871b..0fca69880dc3 100644 --- a/include/sound/hdmi-codec.h +++ b/include/sound/hdmi-codec.h @@ -42,6 +42,7 @@ struct hdmi_codec_daifmt { unsigned int frame_clk_inv:1; unsigned int bit_clk_master:1; unsigned int frame_clk_master:1;
- unsigned int bclk_ratio;
};
/* diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index e5b6769b9797..d71a7e5a2231 100644 --- a/sound/soc/codecs/hdmi-codec.c +++ b/sound/soc/codecs/hdmi-codec.c @@ -470,6 +470,7 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
- struct hdmi_codec_daifmt fmt; struct hdmi_codec_params hp = { .iec = { .status = { 0 },
@@ -520,8 +521,43 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, hp.sample_rate = params_rate(params); hp.channels = params_channels(params);
- fmt = hcp->daifmt[dai->id];
- /*
* If the .set_bclk_ratio() has not been called, default it* using the sample width for compatibility for TDA998x.* Rather than changing this, drivers should arrange to make* an appropriate call to snd_soc_dai_set_bclk_ratio().*/- if (fmt.bclk_ratio == 0) {
switch (hp.sample_width) {case 16:fmt.bclk_ratio = 32;break;case 18:case 20:case 24:fmt.bclk_ratio = 48;break;
AFAIK, this is not the usual choice for 18- or 20-bit samples. Usually, the bclk_ratio is set to the exact frame length required by the sample width without any padding. That is at least the case with tlv320aic3x-driver and 20-bit sample width.
default:fmt.bclk_ratio = 64;break;}- }
- return hcp->hcd.ops->hw_params(dai->dev->parent, hcp->hcd.data,
&hcp->daifmt[dai->id], &hp);
&fmt, &hp);+}
+static int hdmi_codec_set_bclk_ratio(struct snd_soc_dai *dai,
unsigned int ratio)+{
- struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
- /* FIXME: some validation here would be good? */
- hcp->daifmt[dai->id].bclk_ratio = ratio;
- return 0;
}
static int hdmi_codec_set_fmt(struct snd_soc_dai *dai, @@ -593,7 +629,11 @@ static int hdmi_codec_set_fmt(struct snd_soc_dai *dai, } }
- hcp->daifmt[dai->id] = cf;
hcp->daifmt[dai->id].fmt = cf.fmt;
hcp->daifmt[dai->id].bit_clk_inv = cf.bit_clk_inv;
hcp->daifmt[dai->id].frame_clk_inv = cf.frame_clk_inv;
hcp->daifmt[dai->id].bit_clk_master = cf.bit_clk_master;
hcp->daifmt[dai->id].frame_clk_master = cf.frame_clk_master;
return ret;
} @@ -615,6 +655,7 @@ static const struct snd_soc_dai_ops hdmi_dai_ops = { .startup = hdmi_codec_startup, .shutdown = hdmi_codec_shutdown, .hw_params = hdmi_codec_hw_params,
- .set_bclk_ratio = hdmi_codec_set_bclk_ratio, .set_fmt = hdmi_codec_set_fmt, .digital_mute = hdmi_codec_digital_mute,
};
On Mon, Feb 25, 2019 at 03:45:44PM +0200, Jyri Sarha wrote:
On 22/02/2019 23:27, Russell King wrote:
Some HDMI codecs need to know the relationship between the I2S bit clock and the I2S word clock in order to correctly generate the CTS value for audio clock recovery on the sink.
Add support for this, but there are currently no callers of snd_soc_dai_set_bclk_ratio(), we provide a default implementation that uses the sample width to derive the ratio from the 8-bit aligned sample size. This reflects the derivation that is in TDA998x, which we are going to convert to use this new support.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk
include/sound/hdmi-codec.h | 1 + sound/soc/codecs/hdmi-codec.c | 45 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index 9483c55f871b..0fca69880dc3 100644 --- a/include/sound/hdmi-codec.h +++ b/include/sound/hdmi-codec.h @@ -42,6 +42,7 @@ struct hdmi_codec_daifmt { unsigned int frame_clk_inv:1; unsigned int bit_clk_master:1; unsigned int frame_clk_master:1;
- unsigned int bclk_ratio;
};
/* diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index e5b6769b9797..d71a7e5a2231 100644 --- a/sound/soc/codecs/hdmi-codec.c +++ b/sound/soc/codecs/hdmi-codec.c @@ -470,6 +470,7 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
- struct hdmi_codec_daifmt fmt; struct hdmi_codec_params hp = { .iec = { .status = { 0 },
@@ -520,8 +521,43 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, hp.sample_rate = params_rate(params); hp.channels = params_channels(params);
- fmt = hcp->daifmt[dai->id];
- /*
* If the .set_bclk_ratio() has not been called, default it* using the sample width for compatibility for TDA998x.* Rather than changing this, drivers should arrange to make* an appropriate call to snd_soc_dai_set_bclk_ratio().*/- if (fmt.bclk_ratio == 0) {
switch (hp.sample_width) {case 16:fmt.bclk_ratio = 32;break;case 18:case 20:case 24:fmt.bclk_ratio = 48;break;AFAIK, this is not the usual choice for 18- or 20-bit samples. Usually, the bclk_ratio is set to the exact frame length required by the sample width without any padding. That is at least the case with tlv320aic3x-driver and 20-bit sample width.
As mentioned in the commit message, this is what TDA998x does today, and I have to assume that the contributor tested this, who seems to be one Jyri Sarha in commit 95db3b255fde ("drm/i2c: tda998x: Improve tda998x_configure_audio() audio related pdata").
If we don't do it this way, then converting TDA998x to use this defaulting would change the already established behaviour. As there are no users of this by hdmi-codec implementations, and as there are no callers of snd_soc_dai_set_bclk_ratio(), the only way to maintain the current behaviour in TDA998x is to either place a default in hdmi-codec.c, or to have hdmi-codec.c pass zero into TDA998x and have that encode the above.
I would much rather every user of hdmi-codec gained support for snd_soc_dai_set_bclk_ratio() rather than relying on that default, but that is beyond what I can do - I don't have the knowledge of which sound setups would need it, and I don't have any platforms that are configured to use I2S with TDA998x.
The Dove Cubox has spdif and i2s but is setup to use spdif (since that is the most flexible, supporting compressed audio.) I've a large pile of unsubmittable patches there which makes kirkwood use DPCM, as they would end up breaking all the existing users, and from what I can see there's no way around that. That makes submitting a patch to add snd_soc_dai_set_bclk_ratio() very difficult. That said, the above defaults are not correct for kirkwood-i2s - that always uses bclk running at 64fs, as per the TDA998x code prior to your patch I mention above.
ARM Juno has two TDA998x, but the DT description lacks any audio description, so it's not clear whether these are even wired.
The ARM MPS platform has a TDA998x connected to a HDLCD, but the HDLCD does not have an interrupt - and the DRM driver requires an interrupt. The conclusion that was come to there is basically "don't even bother". Also unknown whether audio is wired up.
On 25/02/2019 16:03, Russell King - ARM Linux admin wrote:
On Mon, Feb 25, 2019 at 03:45:44PM +0200, Jyri Sarha wrote:
On 22/02/2019 23:27, Russell King wrote:
Some HDMI codecs need to know the relationship between the I2S bit clock and the I2S word clock in order to correctly generate the CTS value for audio clock recovery on the sink.
Add support for this, but there are currently no callers of snd_soc_dai_set_bclk_ratio(), we provide a default implementation that uses the sample width to derive the ratio from the 8-bit aligned sample size. This reflects the derivation that is in TDA998x, which we are going to convert to use this new support.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk
include/sound/hdmi-codec.h | 1 + sound/soc/codecs/hdmi-codec.c | 45 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index 9483c55f871b..0fca69880dc3 100644 --- a/include/sound/hdmi-codec.h +++ b/include/sound/hdmi-codec.h @@ -42,6 +42,7 @@ struct hdmi_codec_daifmt { unsigned int frame_clk_inv:1; unsigned int bit_clk_master:1; unsigned int frame_clk_master:1;
- unsigned int bclk_ratio;
};
/* diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index e5b6769b9797..d71a7e5a2231 100644 --- a/sound/soc/codecs/hdmi-codec.c +++ b/sound/soc/codecs/hdmi-codec.c @@ -470,6 +470,7 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
- struct hdmi_codec_daifmt fmt; struct hdmi_codec_params hp = { .iec = { .status = { 0 },
@@ -520,8 +521,43 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, hp.sample_rate = params_rate(params); hp.channels = params_channels(params);
- fmt = hcp->daifmt[dai->id];
- /*
* If the .set_bclk_ratio() has not been called, default it* using the sample width for compatibility for TDA998x.* Rather than changing this, drivers should arrange to make* an appropriate call to snd_soc_dai_set_bclk_ratio().*/- if (fmt.bclk_ratio == 0) {
switch (hp.sample_width) {case 16:fmt.bclk_ratio = 32;break;case 18:case 20:case 24:fmt.bclk_ratio = 48;break;AFAIK, this is not the usual choice for 18- or 20-bit samples. Usually, the bclk_ratio is set to the exact frame length required by the sample width without any padding. That is at least the case with tlv320aic3x-driver and 20-bit sample width.
As mentioned in the commit message, this is what TDA998x does today, and I have to assume that the contributor tested this, who seems to be one Jyri Sarha in commit 95db3b255fde ("drm/i2c: tda998x: Improve tda998x_configure_audio() audio related pdata").
Yes, my original implementation was a bit optimistic. I only had limited information about the chip and a somewhat working undocumented hackish driver implementation. By now it's clear that rejecting anything but 16-, 24-, or 32-bit sample widths (32, 48, or 64 bclk ratios) would have been right way to go.
If we don't do it this way, then converting TDA998x to use this defaulting would change the already established behaviour. As there are no users of this by hdmi-codec implementations, and as there are no callers of snd_soc_dai_set_bclk_ratio(), the only way to maintain the current behaviour in TDA998x is to either place a default in hdmi-codec.c, or to have hdmi-codec.c pass zero into TDA998x and have that encode the above.
I think there are other options too. The obvious one would be passing the blck_ratio callback trough HDMI-codec to HDMI-bridge-driver, but yes, I do not like that either.
The other would be changing the implicit bclk_ratio to 2 x sample-width, and accepting blck_ratios of 36 and 40 in tda998x, and set CTS_N_M = 3 and CTS_N_K = 2 for them too.
This way my bad choices would not spread to all hdmi-codec users.
Then again, I don't think anyone is using 18- or 20-bit samples with tda998x. They most likely do not even work. So simply refusing the 36 and 40 blck_ratios would probably be just fine too.
I would much rather every user of hdmi-codec gained support for snd_soc_dai_set_bclk_ratio() rather than relying on that default, but that is beyond what I can do - I don't have the knowledge of which sound setups would need it, and I don't have any platforms that are configured to use I2S with TDA998x.
The little that I know how ASoC is generally used, it is no wonder that so few drivers have implemented it. In 99% of the cases that I have encountered the bclk_ratio is sample_width*channels. I cases where something else is needed the blck_ratio is not the only missing piece.
The Dove Cubox has spdif and i2s but is setup to use spdif (since that is the most flexible, supporting compressed audio.) I've a large pile of unsubmittable patches there which makes kirkwood use DPCM, as they would end up breaking all the existing users, and from what I can see there's no way around that. That makes submitting a patch to add snd_soc_dai_set_bclk_ratio() very difficult. That said, the above defaults are not correct for kirkwood-i2s - that always uses bclk running at 64fs, as per the TDA998x code prior to your patch I mention above.
ARM Juno has two TDA998x, but the DT description lacks any audio description, so it's not clear whether these are even wired.
The ARM MPS platform has a TDA998x connected to a HDLCD, but the HDLCD does not have an interrupt - and the DRM driver requires an interrupt. The conclusion that was come to there is basically "don't even bother". Also unknown whether audio is wired up.
On Mon, Feb 25, 2019 at 10:58:34PM +0200, Jyri Sarha wrote:
On 25/02/2019 16:03, Russell King - ARM Linux admin wrote:
As mentioned in the commit message, this is what TDA998x does today, and I have to assume that the contributor tested this, who seems to be one Jyri Sarha in commit 95db3b255fde ("drm/i2c: tda998x: Improve tda998x_configure_audio() audio related pdata").
Yes, my original implementation was a bit optimistic. I only had limited information about the chip and a somewhat working undocumented hackish driver implementation. By now it's clear that rejecting anything but 16-, 24-, or 32-bit sample widths (32, 48, or 64 bclk ratios) would have been right way to go.
If we don't do it this way, then converting TDA998x to use this defaulting would change the already established behaviour. As there are no users of this by hdmi-codec implementations, and as there are no callers of snd_soc_dai_set_bclk_ratio(), the only way to maintain the current behaviour in TDA998x is to either place a default in hdmi-codec.c, or to have hdmi-codec.c pass zero into TDA998x and have that encode the above.
I think there are other options too. The obvious one would be passing the blck_ratio callback trough HDMI-codec to HDMI-bridge-driver, but yes, I do not like that either.
The other would be changing the implicit bclk_ratio to 2 x sample-width, and accepting blck_ratios of 36 and 40 in tda998x, and set CTS_N_M = 3 and CTS_N_K = 2 for them too.
This way my bad choices would not spread to all hdmi-codec users.
Then again, I don't think anyone is using 18- or 20-bit samples with tda998x. They most likely do not even work. So simply refusing the 36 and 40 blck_ratios would probably be just fine too.
Another would be keepign the existing code with an additional WARN_ON_ONCE(1) if invoked, which would have the effect of encouraging drivers to be fixed up to call snd_soc_dai_set_bclk_ratio() - which has surely to be a good thing? However, the risk is that no one reports the problem cases...
On Mon, Feb 25, 2019 at 10:58:34PM +0200, Jyri Sarha wrote:
I think there are other options too. The obvious one would be passing the blck_ratio callback trough HDMI-codec to HDMI-bridge-driver, but yes, I do not like that either.
The other would be changing the implicit bclk_ratio to 2 x sample-width, and accepting blck_ratios of 36 and 40 in tda998x, and set CTS_N_M = 3 and CTS_N_K = 2 for them too.
To put a bit of further information out there...
I really don't like that idea - what if we have a transmitter that really does use a bclk_ratio of 36 or 40? That would mess up the CTS calculation in the TDA998x.
The equation I've come up to fit what we know for TDA998x is:
k = m * bclk_ratio / 128
where k and m are parameters that we values we select for TDA998x. This reflects the entire clock regeneration system including the sink. The possible values of k are 1, 2, 3, 4, or 8. m are 1, 2, 4, or 8. I also have this equation which defines the fs regenerated at the sink:
fso = bclk_ratio * fsi * m / (128 * k)
If we did have a ratio of 36 or 40, the first equation above fails since k is not one of the possible integers. If we round down and use e.g. 2 for the 36fs case, then we'd actually end up with a sample frequency on the sink of 1.125x faster than it should be, and the sink would suffer starvation of audio samples. If we rounded up to 3, then the sample frequency will be 3/4 of it's true value, so the sink will overflow and would have to discard audio samples.
We know this happens, because that's the root of the problem at hand: wrongly setting the m and k values for the bclk_ratio being supplied to the TDA998x causes audio to be corrupted when reproduced by the sink.
Given that, we do need some way to validate the bclk_ratio when it is set, and not during hw_params which would (a) lead to ALSA devices failing when userspace is negotiating the parameters and (b) gives no way in the kernel for set_bclk_ratio() to discover whether a particular ratio is supported by the codec.
So, I think there's three possible ways around this: 1. adding a set_bclk_ratio() method to hdmi_codec_ops 2. calling hw_params() when our set_bclk_ratio() method is called (what if the rest of the format isn't set or incompatible, which may cause the hw_params() method to fail?) 3. adding a list of acceptable bclk_ratio values to hdmi_codec_pdata
On 27/02/2019 13:47, Russell King - ARM Linux admin wrote:
On Mon, Feb 25, 2019 at 10:58:34PM +0200, Jyri Sarha wrote:
I think there are other options too. The obvious one would be passing the blck_ratio callback trough HDMI-codec to HDMI-bridge-driver, but yes, I do not like that either.
The other would be changing the implicit bclk_ratio to 2 x sample-width, and accepting blck_ratios of 36 and 40 in tda998x, and set CTS_N_M = 3 and CTS_N_K = 2 for them too.
To put a bit of further information out there...
I really don't like that idea - what if we have a transmitter that really does use a bclk_ratio of 36 or 40? That would mess up the CTS calculation in the TDA998x.
The equation I've come up to fit what we know for TDA998x is:
k = m * bclk_ratio / 128
where k and m are parameters that we values we select for TDA998x. This reflects the entire clock regeneration system including the sink. The possible values of k are 1, 2, 3, 4, or 8. m are 1, 2, 4, or 8. I also have this equation which defines the fs regenerated at the sink:
fso = bclk_ratio * fsi * m / (128 * k)
If we did have a ratio of 36 or 40, the first equation above fails since k is not one of the possible integers. If we round down and use e.g. 2 for the 36fs case, then we'd actually end up with a sample frequency on the sink of 1.125x faster than it should be, and the sink would suffer starvation of audio samples. If we rounded up to 3, then the sample frequency will be 3/4 of it's true value, so the sink will overflow and would have to discard audio samples.
We know this happens, because that's the root of the problem at hand: wrongly setting the m and k values for the bclk_ratio being supplied to the TDA998x causes audio to be corrupted when reproduced by the sink.
Given that, we do need some way to validate the bclk_ratio when it is set, and not during hw_params which would (a) lead to ALSA devices failing when userspace is negotiating the parameters and (b) gives no way in the kernel for set_bclk_ratio() to discover whether a particular ratio is supported by the codec.
So, I think there's three possible ways around this:
- adding a set_bclk_ratio() method to hdmi_codec_ops
- calling hw_params() when our set_bclk_ratio() method is called (what if the rest of the format isn't set or incompatible, which may cause the hw_params() method to fail?)
- adding a list of acceptable bclk_ratio values to hdmi_codec_pdata
Or leaving the bclk_ratio field zero in struct hdmi_codec_params if set_bclk_ratio() is not called and leaving the bridge driver to decide what to do in such a situation.
And in tda998x_audio_hw_params() doing something like this:
if (audio.bclk_ratio == 0) audio.bclk_ratio = DIV_ROUND_UP(params->sample_width, 8) * 8;
But then again I think it would be quite sane option to set bclk_ration in hdmi-codec to 2*sample_width and simply refuse the ratios of 36 or 40 in tda998x.
Best regards, Jyri
On Wed, Feb 27, 2019 at 07:48:33PM +0200, Jyri Sarha wrote:
On 27/02/2019 13:47, Russell King - ARM Linux admin wrote:
On Mon, Feb 25, 2019 at 10:58:34PM +0200, Jyri Sarha wrote:
I think there are other options too. The obvious one would be passing the blck_ratio callback trough HDMI-codec to HDMI-bridge-driver, but yes, I do not like that either.
The other would be changing the implicit bclk_ratio to 2 x sample-width, and accepting blck_ratios of 36 and 40 in tda998x, and set CTS_N_M = 3 and CTS_N_K = 2 for them too.
To put a bit of further information out there...
I really don't like that idea - what if we have a transmitter that really does use a bclk_ratio of 36 or 40? That would mess up the CTS calculation in the TDA998x.
The equation I've come up to fit what we know for TDA998x is:
k = m * bclk_ratio / 128
where k and m are parameters that we values we select for TDA998x. This reflects the entire clock regeneration system including the sink. The possible values of k are 1, 2, 3, 4, or 8. m are 1, 2, 4, or 8. I also have this equation which defines the fs regenerated at the sink:
fso = bclk_ratio * fsi * m / (128 * k)
If we did have a ratio of 36 or 40, the first equation above fails since k is not one of the possible integers. If we round down and use e.g. 2 for the 36fs case, then we'd actually end up with a sample frequency on the sink of 1.125x faster than it should be, and the sink would suffer starvation of audio samples. If we rounded up to 3, then the sample frequency will be 3/4 of it's true value, so the sink will overflow and would have to discard audio samples.
We know this happens, because that's the root of the problem at hand: wrongly setting the m and k values for the bclk_ratio being supplied to the TDA998x causes audio to be corrupted when reproduced by the sink.
Given that, we do need some way to validate the bclk_ratio when it is set, and not during hw_params which would (a) lead to ALSA devices failing when userspace is negotiating the parameters and (b) gives no way in the kernel for set_bclk_ratio() to discover whether a particular ratio is supported by the codec.
So, I think there's three possible ways around this:
- adding a set_bclk_ratio() method to hdmi_codec_ops
- calling hw_params() when our set_bclk_ratio() method is called (what if the rest of the format isn't set or incompatible, which may cause the hw_params() method to fail?)
- adding a list of acceptable bclk_ratio values to hdmi_codec_pdata
Or leaving the bclk_ratio field zero in struct hdmi_codec_params if set_bclk_ratio() is not called and leaving the bridge driver to decide what to do in such a situation.
And in tda998x_audio_hw_params() doing something like this:
if (audio.bclk_ratio == 0) audio.bclk_ratio = DIV_ROUND_UP(params->sample_width, 8) * 8;
But then again I think it would be quite sane option to set bclk_ration in hdmi-codec to 2*sample_width and simply refuse the ratios of 36 or 40 in tda998x.
... which then leads to breakage of userspace if 18 or 20 bit formats were attempted, making the bridge driver's capabilities undiscoverable.
Returning an error from hw_params causes hard failures.
On 27/02/2019 20:00, Russell King - ARM Linux admin wrote:
On Wed, Feb 27, 2019 at 07:48:33PM +0200, Jyri Sarha wrote:
On 27/02/2019 13:47, Russell King - ARM Linux admin wrote:
On Mon, Feb 25, 2019 at 10:58:34PM +0200, Jyri Sarha wrote:
I think there are other options too. The obvious one would be passing the blck_ratio callback trough HDMI-codec to HDMI-bridge-driver, but yes, I do not like that either.
The other would be changing the implicit bclk_ratio to 2 x sample-width, and accepting blck_ratios of 36 and 40 in tda998x, and set CTS_N_M = 3 and CTS_N_K = 2 for them too.
To put a bit of further information out there...
I really don't like that idea - what if we have a transmitter that really does use a bclk_ratio of 36 or 40? That would mess up the CTS calculation in the TDA998x.
The equation I've come up to fit what we know for TDA998x is:
k = m * bclk_ratio / 128
where k and m are parameters that we values we select for TDA998x. This reflects the entire clock regeneration system including the sink. The possible values of k are 1, 2, 3, 4, or 8. m are 1, 2, 4, or 8. I also have this equation which defines the fs regenerated at the sink:
fso = bclk_ratio * fsi * m / (128 * k)
If we did have a ratio of 36 or 40, the first equation above fails since k is not one of the possible integers. If we round down and use e.g. 2 for the 36fs case, then we'd actually end up with a sample frequency on the sink of 1.125x faster than it should be, and the sink would suffer starvation of audio samples. If we rounded up to 3, then the sample frequency will be 3/4 of it's true value, so the sink will overflow and would have to discard audio samples.
We know this happens, because that's the root of the problem at hand: wrongly setting the m and k values for the bclk_ratio being supplied to the TDA998x causes audio to be corrupted when reproduced by the sink.
Given that, we do need some way to validate the bclk_ratio when it is set, and not during hw_params which would (a) lead to ALSA devices failing when userspace is negotiating the parameters and (b) gives no way in the kernel for set_bclk_ratio() to discover whether a particular ratio is supported by the codec.
So, I think there's three possible ways around this:
- adding a set_bclk_ratio() method to hdmi_codec_ops
- calling hw_params() when our set_bclk_ratio() method is called (what if the rest of the format isn't set or incompatible, which may cause the hw_params() method to fail?)
- adding a list of acceptable bclk_ratio values to hdmi_codec_pdata
The suggestion below would be my first choice. Packing the same information that we already have in sample_width to bclk_ratio does not make too much sense.
Or leaving the bclk_ratio field zero in struct hdmi_codec_params if set_bclk_ratio() is not called and leaving the bridge driver to decide what to do in such a situation.
And in tda998x_audio_hw_params() doing something like this:
if (audio.bclk_ratio == 0) audio.bclk_ratio = DIV_ROUND_UP(params->sample_width, 8) * 8;
But then again I think it would be quite sane option to set bclk_ration
in hdmi-codec to 2*sample_width and simply refuse the ratios of 36 or 40 in tda998x.
... which then leads to breakage of userspace if 18 or 20 bit formats were attempted, making the bridge driver's capabilities undiscoverable.
Returning an error from hw_params causes hard failures.
Knowing how limited the support to those formats is in Linux userspace, I would say that it is terribly unlikely anybody uses those formats. Before the currently worked on bclk_ratio, those formats would only have worked if the CPU DAI would implicitly use 48-bit bclk_ratio. I would say that is quite unlikely too. I such a case exists or emerges later, then they just need start using the bclk_ratio callback in their machine driver. But still, I think leaving the bclk_ratio to zero if it is not set is a better choice.
Best regards, Jyri
On Wed, Feb 27, 2019 at 6:47 AM Russell King - ARM Linux admin linux@armlinux.org.uk wrote:
Given that, we do need some way to validate the bclk_ratio when it is set, and not during hw_params which would (a) lead to ALSA devices failing when userspace is negotiating the parameters and (b) gives no way in the kernel for set_bclk_ratio() to discover whether a particular ratio is supported by the codec.
So, I think there's three possible ways around this:
- adding a set_bclk_ratio() method to hdmi_codec_ops
- calling hw_params() when our set_bclk_ratio() method is called (what if the rest of the format isn't set or incompatible, which may cause the hw_params() method to fail?)
- adding a list of acceptable bclk_ratio values to hdmi_codec_pdata
Again excuse my obvious ignorance, but would these solutions work well in the more general case?
Imagine a cpu dai that supports 16x2/16x2, 20x2/20x2, 24x2/24x2 (sample bits/frame bits -- wire format), sending audio to our tda998x hdmi xmitter. Depending on the type of samples that userspace chooses to play, one of the above formats gets selected by the ASoC core, resulting in a bclk_ratio of 16x2, 20x2 or 32x2. It's up to the card driver to call set_bclk_ratio(), right? So now this card driver needs intimate knowledge of bclk_ratios vs formats for the cpu dai. Also it needs knowledge of which bclk_ratios are supported by the hdmi xmitter, and a mechanism to filter our the 20x2 blk_ratio format. Which may not be trivial, and also prevents it from being generic, i.e. we can no longer use simple-card ?
But it gets worse. Imagine a hypothetical cpu dai that supports 20x2/20x2 and 20x2/24x2. When the dai is sending to a codec that doesn't care about bclk_ratio, it should pick 20x2/20x2, because that's most efficient, right? Except on a tda998x it should select 20x2/24x2. So how would a card driver now even begin to deal with this, given that there appears to be no mechanism to even describe these differences? Because the params_physical_width() describes the memory format, and not the wire format, correct?
So all this kind of suggests to me that the bclk_ratio could be part of the format description, or something?
static struct snd_soc_dai_driver acme_cpu_dai = { .playback = { .formats = SNDRV_PCM_FMTBIT_S20_3LE_20 | SNDRV_PCM_FMTBIT_S20_3LE_24, SNDRV_PCM_FMTBIT_S16_LE | // bclk_ratio 16 implied SNDRV_PCM_FMTBIT_S24_LE | // bclk_ratio 24 implied SNDRV_PCM_FMTBIT_S24_LE_32 }, };
static struct snd_soc_dai_driver hdmi_dai = { .playback = { .formats = SNDRV_PCM_FMTBIT_S20_3LE_24 | SNDRV_PCM_FMTBIT_S16_LE | // bclk_ratio 16 implied SNDRV_PCM_FMTBIT_S24_LE | // bclk_ratio 24 implied SNDRV_PCM_FMTBIT_S24_LE_32, }, };
In this case, the capabilities get negotiated correctly, and the tda998x's hw_params() could just call params_bclk_ratio(params) to get the ratio, right?
And the fsl_ssi could resort to a rule to filter out all non-32x2 bclk_ratio formats only in master mode.
As I said, I'm way out of my depth here. No idea how realistic or hypothetical this is, or if this would go against the grain of the existing ASoC architecture.
I really hope there's a much better solution than this that will solve the general case.
On Wed, Feb 27, 2019 at 01:01:05PM -0500, Sven Van Asbroeck wrote:
On Wed, Feb 27, 2019 at 6:47 AM Russell King - ARM Linux admin linux@armlinux.org.uk wrote:
Given that, we do need some way to validate the bclk_ratio when it is set, and not during hw_params which would (a) lead to ALSA devices failing when userspace is negotiating the parameters and (b) gives no way in the kernel for set_bclk_ratio() to discover whether a particular ratio is supported by the codec.
So, I think there's three possible ways around this:
- adding a set_bclk_ratio() method to hdmi_codec_ops
- calling hw_params() when our set_bclk_ratio() method is called (what if the rest of the format isn't set or incompatible, which may cause the hw_params() method to fail?)
- adding a list of acceptable bclk_ratio values to hdmi_codec_pdata
Again excuse my obvious ignorance, but would these solutions work well in the more general case?
Imagine a cpu dai that supports 16x2/16x2, 20x2/20x2, 24x2/24x2 (sample bits/frame bits -- wire format), sending audio to our tda998x hdmi xmitter. Depending on the type of samples that userspace chooses to play, one of the above formats gets selected by the ASoC core, resulting in a bclk_ratio of 16x2, 20x2 or 32x2. It's up to the card driver to call set_bclk_ratio(), right? So now this card driver needs intimate knowledge of bclk_ratios vs formats for the cpu dai. Also it needs knowledge of which bclk_ratios are supported by the hdmi xmitter, and a mechanism to filter our the 20x2 blk_ratio format.
That's why I said in a previous email that it would be good if there was some way that the capabilities of the codec and cpu DAIs were known to the ASoC core.
Which may not be trivial, and also prevents it from being generic, i.e. we can no longer use simple-card ?
It seems trivial today that we have a complex system called ALSA PCM, where lots of different parameters are negotiated between the kernel driver and userspace, which include:
- sample rate - number of channels - buffer size - period size - frame bits (unfortunately, not on-wire!) etc.
If you want to see the full list, see the SNDRV_PCM_HW_PARAM_* definitions in include/uapi/sound/asound.h.
The kernel driver can arbitarily apply rules to these, even inter- dependent rules - it's possible to specify in kernel space a rule such as "we can support 48kHz with up to six channels, or 96kHz with two channels" and ALSA will handle it. Other rules are possible.
More than that, if we have a PCM device that supports (eg) only 48kHz, 44.1kHz and 32kHz sample rates with two channels, and we attempt to play a 11.025kHz mono sample, userspace will negotiate one of the hardware supported formats, and then automatically assemble within libasound a set of plugins that convert the number of channels to what the hardware supports, and performs sample rate conversion.
Yet, we seem to be saying that we can't solve the problem of the sample-rate to bitclock ratio.
I suspect we could do using this infrastructure...
But it gets worse. Imagine a hypothetical cpu dai that supports 20x2/20x2 and 20x2/24x2. When the dai is sending to a codec that doesn't care about bclk_ratio, it should pick 20x2/20x2, because that's most efficient, right? Except on a tda998x it should select 20x2/24x2. So how would a card driver now even begin to deal with this, given that there appears to be no mechanism to even describe these differences? Because the params_physical_width() describes the memory format, and not the wire format, correct?
If we were able to describe the on-wire frame size to ALSA's constraint resolution core, I bet it can resolve this for us.
To take the above, for I2S, TDA998x would add a rules to ALSA saying:
1. "I support 2*N channels" where 1 < N < number of I2S inputs. 2. "I support sample widths from 16 to 24 bits" 3. "I support on-wire frame bits 32, 48, 64, 128" 4. "Depending on the sample width, I support <limited-from-above> on-wire frame bits".
We now have TDA998x's parameters described to the ALSA core.
The CPU on the other hand would say to the ALSA core (e.g.):
1. "I support 1-8 channels" 2. "I support sample widths from 8 to 32 bits" 3. "I support on-wire frame bits only of 64"
During ALSAs hardware parameter refining, this would result in ALSA settling on a frame bits of 64 for everything.
If, on the other hand, we had a CPU DAI specifying these rules:
1. "I support 1-8 channels" 2. "I support sample widths from 8 to 32 bits" 3. "Depending on the sample width, I support on-wire frame bits only of sample width * 2"
Then ALSA if you ask for 16 bit samples, it would end up settling on a frame bits of 32, for 24 bit, 48. For the others, it would notice that it can't do a sample width of 18 or 20 bits, and would probably decide upon 24 bit.
And... userspace would automatically pick up a plugin to convert to 24 bit sample width (provided its using one of libasound's plughw devices rather than the raw "no conversion" devices.)
Now, the problem is... there doesn't seem to be an existing hardware parameter for the on-wire frame bits - it isn't SNDRV_PCM_HW_PARAM_FRAME_BITS - sound/core/pcm_native.c already has rules for this parameter that lock it to sample_bits * channels, and it's used to determine the period bytes from the period size etc. However, if you look down the list of rules in snd_pcm_hw_constraints_init(), you'll see that these kinds of relationships can be described to ALSA's contraint resolution core.
It does look like struct snd_pcm_hw_params has some space to be able to extend the intervals, but I'm guessing that isn't going to be easy given that userspace won't know about the new interval, although I'm not sure whether that really matters for this case. However, I can see that there will be objections to exposing such a parameter to userspace.
Another problem is hdmi-codec trying to abstract ALSA and ASoC, and hide it from the actual HDMI bridge. It means bridges have no access to many of the ALSA facilities, and basically have to "do what they're told or fail" causing a hard-failure in ALSA.
Rather than the nice "lets refine the hardware parameters and then insitute whatever plugins are necessary" we get instead something like this (for example, using a slightly hacked driver to show what happens if we error out in hdmi_codec_hw_params()):
$ aplay -Dplughw:0,0 /usr/lib/libreoffice/share/gallery/sounds/theetone.wav Playing WAVE '/usr/lib/libreoffice/share/gallery/sounds/theetone.wav' : Signed 16 bit Little Endian, Rate 11025 Hz, Mono aplay: set_params:1297: Unable to install hw params: ACCESS: RW_INTERLEAVED FORMAT: S16_LE SUBFORMAT: STD SAMPLE_BITS: 16 FRAME_BITS: 16 CHANNELS: 1 RATE: 11025 PERIOD_TIME: (127709 127710) PERIOD_SIZE: 1408 PERIOD_BYTES: 2816 PERIODS: (3 4) BUFFER_TIME: (499229 499230) BUFFER_SIZE: 5504 BUFFER_BYTES: 11008 TICK_TIME: 0
which is showing what userspace does when hdmi_codec_hw_params() returns -EINVAL - ALSA just gives up. It doesn't even bother trying any of its format and sample rate conversion which _is_ available via the "plughw" device. If it knew ahead of time (iow, during the constraint refining) then it would've used plugins to convert from (e.g.) 11.025kHz mono to 48kHz stereo. By way of illustration, here's the difference on x86, HDA-Intel:
Playing WAVE '/usr/lib/libreoffice/share/gallery/sounds/theetone.wav' : Signed 16 bit Little Endian, Rate 11025 Hz, Mono Plug PCM: Rate conversion PCM (44100, sformat=S16_LE) Converter: linear-interpolation Protocol version: 10002 Its setup is: stream : PLAYBACK access : RW_INTERLEAVED format : S16_LE subformat : STD channels : 1 rate : 11025 exact rate : 11025 (11025/1) msbits : 16 buffer_size : 4096 period_size : 1024 period_time : 92879 tstamp_mode : NONE period_step : 1 avail_min : 1024 period_event : 0 start_threshold : 4096 stop_threshold : 4096 silence_threshold: 0 silence_size : 0 boundary : 268435456 Slave: Route conversion PCM (sformat=S16_LE) Transformation table: 0 <- 0 1 <- 0 Its setup is: stream : PLAYBACK access : MMAP_INTERLEAVED format : S16_LE subformat : STD channels : 1 rate : 44100 exact rate : 44100 (44100/1) msbits : 16 buffer_size : 16384 period_size : 4096 period_time : 92879 tstamp_mode : NONE period_step : 1 avail_min : 4096 period_event : 0 start_threshold : 16384 stop_threshold : 16384 silence_threshold: 0 silence_size : 0 boundary : 1073741824 Slave: Hardware PCM card 0 'HDA Intel' device 0 subdevice 0 Its setup is: stream : PLAYBACK access : MMAP_INTERLEAVED format : S16_LE subformat : STD channels : 2 rate : 44100 exact rate : 44100 (44100/1) msbits : 16 buffer_size : 16384 period_size : 4096 period_time : 92879 tstamp_mode : NONE period_step : 1 avail_min : 4096 period_event : 0 start_threshold : 16384 stop_threshold : 16384 silence_threshold: 0 silence_size : 0 boundary : 1073741824 appl_ptr : 0 hw_ptr : 0
And there you have the hardware running at 44.1kHz stereo, playing the very same 11.025kHz mono wav file with userspace plugins clearly automatically picked up.
So all this kind of suggests to me that the bclk_ratio could be part of the format description, or something?
static struct snd_soc_dai_driver acme_cpu_dai = { .playback = { .formats = SNDRV_PCM_FMTBIT_S20_3LE_20 | SNDRV_PCM_FMTBIT_S20_3LE_24, SNDRV_PCM_FMTBIT_S16_LE | // bclk_ratio 16 implied SNDRV_PCM_FMTBIT_S24_LE | // bclk_ratio 24 implied SNDRV_PCM_FMTBIT_S24_LE_32
I don't think this is going to work. Firstly, it'll break userspace, becuase userspace would need to be taught about all these new format identifiers. Secondly, it massively increases the number of formats to number_of_existing_formats * number_of_possible_bclk_ratios.
Russell, thank you for the amazing explanation, and for your continued patience.
On Wed, Feb 27, 2019 at 2:56 PM Russell King - ARM Linux admin linux@armlinux.org.uk wrote:
Now, the problem is... there doesn't seem to be an existing hardware parameter for the on-wire frame bits
Could a case be made to make the resolution core "bclk_ratio" aware ? Of course if this is just to support tda998x, it would be like a cannon to kill a fly...
So all this kind of suggests to me that the bclk_ratio could be part of the format description, or something?
static struct snd_soc_dai_driver acme_cpu_dai = { .playback = { .formats = SNDRV_PCM_FMTBIT_S20_3LE_20 | SNDRV_PCM_FMTBIT_S20_3LE_24, SNDRV_PCM_FMTBIT_S16_LE | // bclk_ratio 16 implied SNDRV_PCM_FMTBIT_S24_LE | // bclk_ratio 24 implied SNDRV_PCM_FMTBIT_S24_LE_32
I don't think this is going to work. Firstly, it'll break userspace, becuase userspace would need to be taught about all these new format identifiers. Secondly, it massively increases the number of formats to number_of_existing_formats * number_of_possible_bclk_ratios.
How about
static struct snd_soc_dai_driver acme_dai_template = { .playback = { .stream_name = "CPU-Playback", .channels_min = 1, .channels_max = 32, .rates = SNDRV_PCM_RATE_CONTINUOUS, .formats = FSLSSI_I2S_FORMATS,
.bclk_ratio_min = 8, .bclk_ratio_max = 64, or .bclk_ratios = RATIO_8 | RATIO_16 | RATIO_32 | RATIO_64, }, };
Then if codec/cpu cares about bclk_ratios, it would have to put rules in place such as: - SNDRV_PCM_FMTBIT_S16_LE -> bclk_ratio = 16x2 only - SNDRV_PCM_FMTBIT_S24_LE -> slave : bclk_ratio = [ 24x2 to 32x2 ] -> master: bclk_ratio = 32x2 only
On Wed, Feb 27, 2019 at 07:56:00PM +0000, Russell King - ARM Linux admin wrote:
On Wed, Feb 27, 2019 at 01:01:05PM -0500, Sven Van Asbroeck wrote:
On Wed, Feb 27, 2019 at 6:47 AM Russell King - ARM Linux admin linux@armlinux.org.uk wrote:
Given that, we do need some way to validate the bclk_ratio when it is set, and not during hw_params which would (a) lead to ALSA devices failing when userspace is negotiating the parameters and (b) gives no way in the kernel for set_bclk_ratio() to discover whether a particular ratio is supported by the codec.
So, I think there's three possible ways around this:
- adding a set_bclk_ratio() method to hdmi_codec_ops
- calling hw_params() when our set_bclk_ratio() method is called (what if the rest of the format isn't set or incompatible, which may cause the hw_params() method to fail?)
- adding a list of acceptable bclk_ratio values to hdmi_codec_pdata
Again excuse my obvious ignorance, but would these solutions work well in the more general case?
Imagine a cpu dai that supports 16x2/16x2, 20x2/20x2, 24x2/24x2 (sample bits/frame bits -- wire format), sending audio to our tda998x hdmi xmitter. Depending on the type of samples that userspace chooses to play, one of the above formats gets selected by the ASoC core, resulting in a bclk_ratio of 16x2, 20x2 or 32x2. It's up to the card driver to call set_bclk_ratio(), right? So now this card driver needs intimate knowledge of bclk_ratios vs formats for the cpu dai. Also it needs knowledge of which bclk_ratios are supported by the hdmi xmitter, and a mechanism to filter our the 20x2 blk_ratio format.
That's why I said in a previous email that it would be good if there was some way that the capabilities of the codec and cpu DAIs were known to the ASoC core.
Which may not be trivial, and also prevents it from being generic, i.e. we can no longer use simple-card ?
It seems trivial today that we have a complex system called ALSA PCM, where lots of different parameters are negotiated between the kernel driver and userspace, which include:
- sample rate
- number of channels
- buffer size
- period size
- frame bits (unfortunately, not on-wire!)
etc.
If you want to see the full list, see the SNDRV_PCM_HW_PARAM_* definitions in include/uapi/sound/asound.h.
The kernel driver can arbitarily apply rules to these, even inter- dependent rules - it's possible to specify in kernel space a rule such as "we can support 48kHz with up to six channels, or 96kHz with two channels" and ALSA will handle it. Other rules are possible.
More than that, if we have a PCM device that supports (eg) only 48kHz, 44.1kHz and 32kHz sample rates with two channels, and we attempt to play a 11.025kHz mono sample, userspace will negotiate one of the hardware supported formats, and then automatically assemble within libasound a set of plugins that convert the number of channels to what the hardware supports, and performs sample rate conversion.
Yet, we seem to be saying that we can't solve the problem of the sample-rate to bitclock ratio.
I suspect we could do using this infrastructure...
But it gets worse. Imagine a hypothetical cpu dai that supports 20x2/20x2 and 20x2/24x2. When the dai is sending to a codec that doesn't care about bclk_ratio, it should pick 20x2/20x2, because that's most efficient, right? Except on a tda998x it should select 20x2/24x2. So how would a card driver now even begin to deal with this, given that there appears to be no mechanism to even describe these differences? Because the params_physical_width() describes the memory format, and not the wire format, correct?
If we were able to describe the on-wire frame size to ALSA's constraint resolution core, I bet it can resolve this for us.
To take the above, for I2S, TDA998x would add a rules to ALSA saying:
- "I support 2*N channels" where 1 < N < number of I2S inputs.
- "I support sample widths from 16 to 24 bits"
- "I support on-wire frame bits 32, 48, 64, 128"
- "Depending on the sample width, I support <limited-from-above> on-wire frame bits".
We now have TDA998x's parameters described to the ALSA core.
The CPU on the other hand would say to the ALSA core (e.g.):
- "I support 1-8 channels"
- "I support sample widths from 8 to 32 bits"
- "I support on-wire frame bits only of 64"
During ALSAs hardware parameter refining, this would result in ALSA settling on a frame bits of 64 for everything.
If, on the other hand, we had a CPU DAI specifying these rules:
- "I support 1-8 channels"
- "I support sample widths from 8 to 32 bits"
- "Depending on the sample width, I support on-wire frame bits only of sample width * 2"
Then ALSA if you ask for 16 bit samples, it would end up settling on a frame bits of 32, for 24 bit, 48. For the others, it would notice that it can't do a sample width of 18 or 20 bits, and would probably decide upon 24 bit.
And... userspace would automatically pick up a plugin to convert to 24 bit sample width (provided its using one of libasound's plughw devices rather than the raw "no conversion" devices.)
Now, the problem is... there doesn't seem to be an existing hardware parameter for the on-wire frame bits - it isn't SNDRV_PCM_HW_PARAM_FRAME_BITS - sound/core/pcm_native.c already has rules for this parameter that lock it to sample_bits * channels, and it's used to determine the period bytes from the period size etc. However, if you look down the list of rules in snd_pcm_hw_constraints_init(), you'll see that these kinds of relationships can be described to ALSA's contraint resolution core.
It does look like struct snd_pcm_hw_params has some space to be able to extend the intervals, but I'm guessing that isn't going to be easy given that userspace won't know about the new interval, although I'm not sure whether that really matters for this case. However, I can see that there will be objections to exposing such a parameter to userspace.
Another problem is hdmi-codec trying to abstract ALSA and ASoC, and hide it from the actual HDMI bridge. It means bridges have no access to many of the ALSA facilities, and basically have to "do what they're told or fail" causing a hard-failure in ALSA.
Rather than the nice "lets refine the hardware parameters and then insitute whatever plugins are necessary" we get instead something like this (for example, using a slightly hacked driver to show what happens if we error out in hdmi_codec_hw_params()):
$ aplay -Dplughw:0,0 /usr/lib/libreoffice/share/gallery/sounds/theetone.wav Playing WAVE '/usr/lib/libreoffice/share/gallery/sounds/theetone.wav' : Signed 16 bit Little Endian, Rate 11025 Hz, Mono aplay: set_params:1297: Unable to install hw params: ACCESS: RW_INTERLEAVED FORMAT: S16_LE SUBFORMAT: STD SAMPLE_BITS: 16 FRAME_BITS: 16 CHANNELS: 1 RATE: 11025 PERIOD_TIME: (127709 127710) PERIOD_SIZE: 1408 PERIOD_BYTES: 2816 PERIODS: (3 4) BUFFER_TIME: (499229 499230) BUFFER_SIZE: 5504 BUFFER_BYTES: 11008 TICK_TIME: 0
which is showing what userspace does when hdmi_codec_hw_params() returns -EINVAL - ALSA just gives up. It doesn't even bother trying any of its format and sample rate conversion which _is_ available via the "plughw" device. If it knew ahead of time (iow, during the constraint refining) then it would've used plugins to convert from (e.g.) 11.025kHz mono to 48kHz stereo. By way of illustration, here's the difference on x86, HDA-Intel:
Playing WAVE '/usr/lib/libreoffice/share/gallery/sounds/theetone.wav' : Signed 16 bit Little Endian, Rate 11025 Hz, Mono Plug PCM: Rate conversion PCM (44100, sformat=S16_LE) Converter: linear-interpolation Protocol version: 10002 Its setup is: stream : PLAYBACK access : RW_INTERLEAVED format : S16_LE subformat : STD channels : 1 rate : 11025 exact rate : 11025 (11025/1) msbits : 16 buffer_size : 4096 period_size : 1024 period_time : 92879 tstamp_mode : NONE period_step : 1 avail_min : 1024 period_event : 0 start_threshold : 4096 stop_threshold : 4096 silence_threshold: 0 silence_size : 0 boundary : 268435456 Slave: Route conversion PCM (sformat=S16_LE) Transformation table: 0 <- 0 1 <- 0 Its setup is: stream : PLAYBACK access : MMAP_INTERLEAVED format : S16_LE subformat : STD channels : 1 rate : 44100 exact rate : 44100 (44100/1) msbits : 16 buffer_size : 16384 period_size : 4096 period_time : 92879 tstamp_mode : NONE period_step : 1 avail_min : 4096 period_event : 0 start_threshold : 16384 stop_threshold : 16384 silence_threshold: 0 silence_size : 0 boundary : 1073741824 Slave: Hardware PCM card 0 'HDA Intel' device 0 subdevice 0 Its setup is: stream : PLAYBACK access : MMAP_INTERLEAVED format : S16_LE subformat : STD channels : 2 rate : 44100 exact rate : 44100 (44100/1) msbits : 16 buffer_size : 16384 period_size : 4096 period_time : 92879 tstamp_mode : NONE period_step : 1 avail_min : 4096 period_event : 0 start_threshold : 16384 stop_threshold : 16384 silence_threshold: 0 silence_size : 0 boundary : 1073741824 appl_ptr : 0 hw_ptr : 0
And there you have the hardware running at 44.1kHz stereo, playing the very same 11.025kHz mono wav file with userspace plugins clearly automatically picked up.
So all this kind of suggests to me that the bclk_ratio could be part of the format description, or something?
static struct snd_soc_dai_driver acme_cpu_dai = { .playback = { .formats = SNDRV_PCM_FMTBIT_S20_3LE_20 | SNDRV_PCM_FMTBIT_S20_3LE_24, SNDRV_PCM_FMTBIT_S16_LE | // bclk_ratio 16 implied SNDRV_PCM_FMTBIT_S24_LE | // bclk_ratio 24 implied SNDRV_PCM_FMTBIT_S24_LE_32
I don't think this is going to work. Firstly, it'll break userspace, becuase userspace would need to be taught about all these new format identifiers. Secondly, it massively increases the number of formats to number_of_existing_formats * number_of_possible_bclk_ratios.
I'll add that yes, I do think that solving this bclk_ratio issue is not going to be an easy job, and I am hoping that those who know ALSA and ASoC better than I will make some suggestions on a way forward that looks sensible, otherwise I fear that this issue isn't going to be resolvable.
What I _do_ want to avoid are hard-failures (as illustrated above) that cause ALSA userspace to basically give up - which results in audio completely failing to work.
On Mon, Feb 25, 2019 at 03:45:44PM +0200, Jyri Sarha wrote:
On 22/02/2019 23:27, Russell King wrote:
- /*
* If the .set_bclk_ratio() has not been called, default it* using the sample width for compatibility for TDA998x.* Rather than changing this, drivers should arrange to make* an appropriate call to snd_soc_dai_set_bclk_ratio().*/- if (fmt.bclk_ratio == 0) {
switch (hp.sample_width) {case 16:fmt.bclk_ratio = 32;break;case 18:case 20:case 24:fmt.bclk_ratio = 48;break;
AFAIK, this is not the usual choice for 18- or 20-bit samples. Usually, the bclk_ratio is set to the exact frame length required by the sample width without any padding. That is at least the case with tlv320aic3x-driver and 20-bit sample width.
So, this is true. On the other hand like Russell says further down the thread it's preserving the existing behaviour so it would be surprising if it actually broke anything and it will help systems that explicitly set the ratio so I don't think we should let perfect be the enemy of good here.
As Russell outlined there's quite a bit of hopeful assumption in how ASoC handles the mapping of memory formats onto wire formats which works almost all the time but not always and definitely not through robust design, that should be a lot easier to address once the component conversion has been done as we'll actually have all the links in the system directly visible rather than bundled up together and implied as they are currently. Sadly that's a lot of work with not many people working on it so progress is super slow.
On 01/03/2019 14:36, Mark Brown wrote:
On Mon, Feb 25, 2019 at 03:45:44PM +0200, Jyri Sarha wrote:
On 22/02/2019 23:27, Russell King wrote:
- /*
* If the .set_bclk_ratio() has not been called, default it* using the sample width for compatibility for TDA998x.* Rather than changing this, drivers should arrange to make* an appropriate call to snd_soc_dai_set_bclk_ratio().*/- if (fmt.bclk_ratio == 0) {
switch (hp.sample_width) {case 16:fmt.bclk_ratio = 32;break;case 18:case 20:case 24:fmt.bclk_ratio = 48;break;AFAIK, this is not the usual choice for 18- or 20-bit samples. Usually, the bclk_ratio is set to the exact frame length required by the sample width without any padding. That is at least the case with tlv320aic3x-driver and 20-bit sample width.
So, this is true. On the other hand like Russell says further down the thread it's preserving the existing behaviour so it would be surprising if it actually broke anything and it will help systems that explicitly set the ratio so I don't think we should let perfect be the enemy of good here.
Sure. Still, the requirement for having bclk_ratio of either 32, 48, or 64 is coming from tda998x, so that requirement should be satisfied in tda998x driver, not in hdmi-codec that is supposed to be generic and imposes that behaviour to all other user too.
Of course we should not make things overly complicated just because of such principle. But in this case we are trying to preserve existing behaviour that has hardly ever worked, and the new behaviour will potentially cause more trouble. And if we really want to preserve the existing behaviour there is another way that affects only tda998x driver:
- hdmi-codec leaves the struct hdmi_codec_params bclk_ratio to 0 if snd_soc_dai_set_bclk_ratio() is not called
- tda998x_audio_hw_params() checks if bclk_ratio == 0 and makes the implicit bclk_ratio setting the same way as the code above does it
Of course the configurations that we are trying to fix would only work if the cpu-dai would by some luck (or intentional hack) use bclk_ratio of 48 for 18- and 20-bit sample formats.
This is the problem with implicit blck_ratio setting at frame clock slave end. Currently there is only a way to set bclk_ratio from the machine driver, but there is no way ask what a dai driver would prefer to use. That is why it is unlikely that 18- or 20-bit sample formats have worked with any platform with tda998x, or will work in future with just the implicit bclk_ratio setting.
But, after (hopefully) making my point, if both you and Russell want to use Russell's original approach, please go ahead. As you said, it is unlikely that it breaks anything.
As Russell outlined there's quite a bit of hopeful assumption in how ASoC handles the mapping of memory formats onto wire formats which works almost all the time but not always and definitely not through robust design, that should be a lot easier to address once the component conversion has been done as we'll actually have all the links in the system directly visible rather than bundled up together and implied as they are currently. Sadly that's a lot of work with not many people working on it so progress is super slow.
Yes, there is lot more that could be done there. Ideally there would be an option to let the dai drivers on the wire to negotiate the i2s format and related parameters by them selves based on sample width, channels, and sample rate.... but yes that is not a simple thing to implement.
Best regards, Jyri
On Fri, Mar 01, 2019 at 04:05:25PM +0200, Jyri Sarha wrote:
On 01/03/2019 14:36, Mark Brown wrote:
On Mon, Feb 25, 2019 at 03:45:44PM +0200, Jyri Sarha wrote:
On 22/02/2019 23:27, Russell King wrote:
- /*
* If the .set_bclk_ratio() has not been called, default it* using the sample width for compatibility for TDA998x.* Rather than changing this, drivers should arrange to make* an appropriate call to snd_soc_dai_set_bclk_ratio().*/- if (fmt.bclk_ratio == 0) {
switch (hp.sample_width) {case 16:fmt.bclk_ratio = 32;break;case 18:case 20:case 24:fmt.bclk_ratio = 48;break;AFAIK, this is not the usual choice for 18- or 20-bit samples. Usually, the bclk_ratio is set to the exact frame length required by the sample width without any padding. That is at least the case with tlv320aic3x-driver and 20-bit sample width.
So, this is true. On the other hand like Russell says further down the thread it's preserving the existing behaviour so it would be surprising if it actually broke anything and it will help systems that explicitly set the ratio so I don't think we should let perfect be the enemy of good here.
Sure. Still, the requirement for having bclk_ratio of either 32, 48, or 64 is coming from tda998x, so that requirement should be satisfied in tda998x driver, not in hdmi-codec that is supposed to be generic and imposes that behaviour to all other user too.
Of course we should not make things overly complicated just because of such principle. But in this case we are trying to preserve existing behaviour that has hardly ever worked, and the new behaviour will potentially cause more trouble.
Err, what?
With respect to TDA998x, my patches do not change the behaviour in any way of the TDA998x setup. As I have already established:
1. I'm introducing a new bclk_ratio member into the hdmi daifmt structure. There can be no other users of this except the ones that are introduced from this point forth.
2. No one calls the function that sets the bclk_ratio in the entire codebase that is sound/soc, so no one will set bclk_ratio to anything non-zero, except any new callers that get introduced.
So, at the point that patch 2 is merged, no one is using the values that the new code derives. No one is supplying bclk_ratio values to the code either.
As I have already stated, the way I see this code is it is a stop-gap to allow TDA998x to continue working as well as it does _today_ on BBB when we convert TDA998x to start using the supplied bclk_ratio. However, what we should strive to do is to eliminate that code as soon as possible - in other words, making BBB and all the other users set the bclk_ratio explicitly for I2S links.
However, you bring up another issue in what you have said above - you appear to be claiming that the "existing behaviour has hardly ever worked". If it hardly ever works, this seems to imply that audio support on BBB is currently broken.
If BBB audio is broken, then we don't need this at all, and we can just change TDA998x to error out on a zero bclk_ratio value.
And if we really want to preserve the existing behaviour there is another way that affects only tda998x driver:
hdmi-codec leaves the struct hdmi_codec_params bclk_ratio to 0 if snd_soc_dai_set_bclk_ratio() is not called
tda998x_audio_hw_params() checks if bclk_ratio == 0 and makes the implicit bclk_ratio setting the same way as the code above does it
I'm afraid I completely disagree. Pushing that into the tda998x driver means that:
1. there is no motivation to set a correct bclk_ratio value (no one sets it today.) 2. because no one sets the bclk_ratio, users of hdmi-codec will see bclk_ratio as zero, so if they care they will implement their drivers with their own defaults, which may be to always default to 64fs, or may be to default to 2x sample_width. 3. since the defaults match what the HDMI bridges are used with, no one will bother implementing a call to the ASoC function to set bclk_ratio.
So, we'll continue on as we have done, with no one calling the ASoC function, HDMI bridges making their own randomly different bclk_ratio assumptions, and the whole thing generally decending into an unmaintainable mess.
With the defaulting in the hdmi-codec code, it gives a bit more motivation for things to get fixed up: if the default in hdmi-codec is not suitable, it forces the I2S source to explicitly set the bclk_ratio accurately.
It becomes even better when we remove the defaulting, because we then require everyone to call it for I2S to be functional with hdmi-codec with a HDMI bridge that requires this information.
Even better than that would be to make hdmi-codec require that the bclk_ratio is set, which means we have a hope that we're not going to end up in the endless situation where we have to retrofit the bclk_ratio call this into future I2S source.
... where "I2S source" above I mean the non-codec parts of the subsystem.
On 01/03/2019 16:59, Russell King - ARM Linux admin wrote:
On Fri, Mar 01, 2019 at 04:05:25PM +0200, Jyri Sarha wrote:
On 01/03/2019 14:36, Mark Brown wrote:
On Mon, Feb 25, 2019 at 03:45:44PM +0200, Jyri Sarha wrote:
On 22/02/2019 23:27, Russell King wrote:
- /*
* If the .set_bclk_ratio() has not been called, default it* using the sample width for compatibility for TDA998x.* Rather than changing this, drivers should arrange to make* an appropriate call to snd_soc_dai_set_bclk_ratio().*/- if (fmt.bclk_ratio == 0) {
switch (hp.sample_width) {case 16:fmt.bclk_ratio = 32;break;case 18:case 20:case 24:fmt.bclk_ratio = 48;break;AFAIK, this is not the usual choice for 18- or 20-bit samples. Usually, the bclk_ratio is set to the exact frame length required by the sample width without any padding. That is at least the case with tlv320aic3x-driver and 20-bit sample width.
So, this is true. On the other hand like Russell says further down the thread it's preserving the existing behaviour so it would be surprising if it actually broke anything and it will help systems that explicitly set the ratio so I don't think we should let perfect be the enemy of good here.
Sure. Still, the requirement for having bclk_ratio of either 32, 48, or 64 is coming from tda998x, so that requirement should be satisfied in tda998x driver, not in hdmi-codec that is supposed to be generic and imposes that behaviour to all other user too.
Of course we should not make things overly complicated just because of such principle. But in this case we are trying to preserve existing behaviour that has hardly ever worked, and the new behaviour will potentially cause more trouble.
Err, what?
With respect to TDA998x, my patches do not change the behaviour in any way of the TDA998x setup. As I have already established:
I'm introducing a new bclk_ratio member into the hdmi daifmt structure. There can be no other users of this except the ones that are introduced from this point forth.
No one calls the function that sets the bclk_ratio in the entire codebase that is sound/soc, so no one will set bclk_ratio to anything non-zero, except any new callers that get introduced.
So, at the point that patch 2 is merged, no one is using the values that the new code derives. No one is supplying bclk_ratio values to the code either.
As I have already stated, the way I see this code is it is a stop-gap to allow TDA998x to continue working as well as it does _today_ on BBB when we convert TDA998x to start using the supplied bclk_ratio. However, what we should strive to do is to eliminate that code as soon as possible - in other words, making BBB and all the other users set the bclk_ratio explicitly for I2S links.
However, you bring up another issue in what you have said above - you appear to be claiming that the "existing behaviour has hardly ever worked". If it hardly ever works, this seems to imply that audio support on BBB is currently broken.
If BBB audio is broken, then we don't need this at all, and we can just change TDA998x to error out on a zero bclk_ratio value.
And if we really want to preserve the existing behaviour there is another way that affects only tda998x driver:
hdmi-codec leaves the struct hdmi_codec_params bclk_ratio to 0 if snd_soc_dai_set_bclk_ratio() is not called
tda998x_audio_hw_params() checks if bclk_ratio == 0 and makes the implicit bclk_ratio setting the same way as the code above does it
I'm afraid I completely disagree. Pushing that into the tda998x driver means that:
- there is no motivation to set a correct bclk_ratio value (no one sets it today.)
I fail to see how that is different when the implicit setting of bclk_ratio is done in hdmi-codec.c.
- because no one sets the bclk_ratio, users of hdmi-codec will see bclk_ratio as zero, so if they care they will implement their drivers with their own defaults, which may be to always default to 64fs, or may be to default to 2x sample_width.
- since the defaults match what the HDMI bridges are used with, no one will bother implementing a call to the ASoC function to set bclk_ratio.
Setting the bclk_ratio implicitly to what ever in tda998x or hdmi-codec does not really help the problem at all, since it is not visible outside it really has nothing to do with the bclk_ratio used by the cpu-dai.
So, we'll continue on as we have done, with no one calling the ASoC function, HDMI bridges making their own randomly different bclk_ratio assumptions, and the whole thing generally decending into an unmaintainable mess.
The i2s slaves, which HDMI encoders most often are, do not usually care too much about bclk_ratio in the first place. So normally there is no need for any defaults. tda998x-family, of course, is an exception to this rule, but for instance sii9022 does not use bclk for CTS values and does not care about bclk as long as it is big enough to fit all sample bits in to frame.
With the defaulting in the hdmi-codec code, it gives a bit more motivation for things to get fixed up: if the default in hdmi-codec is not suitable, it forces the I2S source to explicitly set the bclk_ratio accurately.
Ok, I see. The potential cause future of problems is the intention :).
The implementation of set_bclk_ratio() callback is only a small part of the problem. We need something more we want to have a generic solution to the situation where i2s slave needs to know, or even have a say to what the bclk-ratio should be.
Currently (AFAIK, I have not followed lately) in ASoC the i2s bclk-master generates the bclk to the best of its ability for the given sample-rate and sample-format (and number channels and possibly of tdm-slots etc.), usually it is also the frame-master and chooses the matching bclk-ratio for the situation. Forcing the sample clock from the machine driver (like simple-card) changes this behaviour a lot, and requires relatively big changes to the dai-drivers acting as bclk and frame masters, before this can work. This would at least be the case with McASP that I am familiar with.
It becomes even better when we remove the defaulting, because we then require everyone to call it for I2S to be functional with hdmi-codec with a HDMI bridge that requires this information.
Even better than that would be to make hdmi-codec require that the bclk_ratio is set, which means we have a hope that we're not going to end up in the endless situation where we have to retrofit the bclk_ratio call this into future I2S source.
This would effectively break hdmi-codec, for all platforms that do not have the above problem solved.
... where "I2S source" above I mean the non-codec parts of the subsystem.
For a (more) complete solution to this problem I would propose to add something to ASoC API for asking the chosen bclk-ratio from DAI drivers. Machine drivers could then ask that from the i2s frame masters and deliver the information to i2s slaves.
Best regards, Jyri
On Fri, Mar 1, 2019 at 7:36 AM Mark Brown broonie@kernel.org wrote:
As Russell outlined there's quite a bit of hopeful assumption in how ASoC handles the mapping of memory formats onto wire formats which works almost all the time but not always and definitely not through robust design
Yes, many of the painful tradeoffs in this discussion would go away, if the alsa negotiation infrastructure was 'bclk_ratio aware'.
They say that a 100-line patch is often better than 1000 words. To help kick- start a discussion, I offer a simple patch which adds bclk_ratio to hw_params, which lets alsa 'negotiate' it the same way as the format, channels, etc.
Obviously this is completely flawed, as it exposes bclk_ratio to userspace thru snd_pcm_hw_params. Userspace has no business even knowing about this on-wire property.
It may be flawed in a lot of other ways I can't see rn :)
As far as a flawed suggestion goes, it seems to behave quite well on my system. The bclk_ratio is nicely constrained within the cpu and codec's supported ranges and rules, and the lowest supported value is picked before hw_params() gets called. Which is at least channels * sample_width.
Would there be a way to hide the bclk_ratio from userspace?
Is it even worth taking this forward? How likely is it for alsa components to care about the bclk_ratio? Maybe this is a tda998x one-off?
Sven
On 04/03/2019 18:59, Sven Van Asbroeck wrote:
On Fri, Mar 1, 2019 at 7:36 AM Mark Brown broonie@kernel.org wrote:
As Russell outlined there's quite a bit of hopeful assumption in how ASoC handles the mapping of memory formats onto wire formats which works almost all the time but not always and definitely not through robust design
Yes, many of the painful tradeoffs in this discussion would go away, if the alsa negotiation infrastructure was 'bclk_ratio aware'.
They say that a 100-line patch is often better than 1000 words. To help kick- start a discussion, I offer a simple patch which adds bclk_ratio to hw_params, which lets alsa 'negotiate' it the same way as the format, channels, etc.
Obviously this is completely flawed, as it exposes bclk_ratio to userspace thru snd_pcm_hw_params. Userspace has no business even knowing about this on-wire property.
It may be flawed in a lot of other ways I can't see rn :)
As far as a flawed suggestion goes, it seems to behave quite well on my system. The bclk_ratio is nicely constrained within the cpu and codec's supported ranges and rules, and the lowest supported value is picked before hw_params() gets called. Which is at least channels * sample_width.
Would there be a way to hide the bclk_ratio from userspace?
Is it even worth taking this forward? How likely is it for alsa components to care about the bclk_ratio? Maybe this is a tda998x one-off?
I am not the right person say if this should be taken forward.
However, if we are going forward, I think we need to add bclk_rate to snd_pcm_hw_params too, since the available bclks affects the choice of bclk_ratios that are available. For instance if the bclk is generated from static clock with a simple divider then following equatios should always be satisfied:
bclk_rate = clk_source_rate / clk_divider, where clk_divider = 0, 1, ..n sample_rate = bclk_rate / bclk_ratio
The good news is that snd_pcm_hw_rule_add() should be able to cope with such cross dependencies.
BR, Jyri
participants (5)
-
Jyri Sarha -
Mark Brown -
Russell King -
Russell King - ARM Linux admin -
Sven Van Asbroeck