[alsa-devel] [RFC PATCH 1/4] alsa: make hw_params negotiation infrastructure 'bclk_ratio aware'
Negotiation seems to work ok, but bclk_ratio is exposed to userspace via snd_pcm_hw_params, which is not acceptable.
Constrain bclk_ratio by: - cpu dai capabilities && rules - codec dai capabilities && rules - minimum bclk_ratio is sample_width * channels
In hw_params_choose(), pick the smallest supported bclk_ratio, which should correspond to the most efficient solution.
If cpu and codec dais do not specify or constrain supported bclk_ratios, alsa will pick sample_width * channels.
Signed-off-by: Sven Van Asbroeck TheSven73@gmail.com --- include/sound/pcm.h | 11 +++++++++++ include/sound/soc.h | 2 ++ include/uapi/sound/asound.h | 5 +++-- sound/core/pcm_native.c | 34 +++++++++++++++++++++++++++++++++- sound/soc/soc-pcm.c | 8 ++++++++ 5 files changed, 57 insertions(+), 3 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index d6bd3caf6878..71ac7e8de23d 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -56,6 +56,8 @@ struct snd_pcm_hardware { unsigned int periods_min; /* min # of periods */ unsigned int periods_max; /* max # of periods */ size_t fifo_size; /* fifo size in bytes */ + unsigned int bclk_ratio_min; /* min bclk ratio for wire format */ + unsigned int bclk_ratio_max; /* max bclk ratio for wire format */ };
struct snd_pcm_substream; @@ -980,6 +982,15 @@ static inline unsigned int params_buffer_bytes(const struct snd_pcm_hw_params *p return hw_param_interval_c(p, SNDRV_PCM_HW_PARAM_BUFFER_BYTES)->min; }
+/** + * params_bclk_ratio - Get the bclk_ratio from the hw params + * @p: hw params + */ +static inline unsigned int params_bclk_ratio(const struct snd_pcm_hw_params *p) +{ + return hw_param_interval_c(p, SNDRV_PCM_HW_PARAM_BCLK_RATIO)->min; +} + int snd_interval_refine(struct snd_interval *i, const struct snd_interval *v); int snd_interval_list(struct snd_interval *i, unsigned int count, const unsigned int *list, unsigned int mask); diff --git a/include/sound/soc.h b/include/sound/soc.h index e665f111b0d2..96d669423688 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -732,6 +732,8 @@ struct snd_soc_pcm_stream { unsigned int channels_min; /* min channels */ unsigned int channels_max; /* max channels */ unsigned int sig_bits; /* number of bits of content */ + unsigned int bclk_ratio_min; /* min bclk ratio for wire format */ + unsigned int bclk_ratio_max; /* max bclk ratio for wire format */ };
/* SoC audio ops */ diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 404d4b9ffe76..c3ea94eaaa77 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -371,8 +371,9 @@ typedef int snd_pcm_hw_param_t; #define SNDRV_PCM_HW_PARAM_BUFFER_SIZE 17 /* Size of buffer in frames */ #define SNDRV_PCM_HW_PARAM_BUFFER_BYTES 18 /* Size of buffer in bytes */ #define SNDRV_PCM_HW_PARAM_TICK_TIME 19 /* Approx tick duration in us */ +#define SNDRV_PCM_HW_PARAM_BCLK_RATIO 20 /* bclk_ratio for wire format */ #define SNDRV_PCM_HW_PARAM_FIRST_INTERVAL SNDRV_PCM_HW_PARAM_SAMPLE_BITS -#define SNDRV_PCM_HW_PARAM_LAST_INTERVAL SNDRV_PCM_HW_PARAM_TICK_TIME +#define SNDRV_PCM_HW_PARAM_LAST_INTERVAL SNDRV_PCM_HW_PARAM_BCLK_RATIO
#define SNDRV_PCM_HW_PARAMS_NORESAMPLE (1<<0) /* avoid rate resampling */ #define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER (1<<1) /* export buffer */ @@ -399,7 +400,7 @@ struct snd_pcm_hw_params { struct snd_mask mres[5]; /* reserved masks */ struct snd_interval intervals[SNDRV_PCM_HW_PARAM_LAST_INTERVAL - SNDRV_PCM_HW_PARAM_FIRST_INTERVAL + 1]; - struct snd_interval ires[9]; /* reserved intervals */ + struct snd_interval ires[8]; /* reserved intervals */ unsigned int rmask; /* W: requested masks */ unsigned int cmask; /* R: changed masks */ unsigned int info; /* R: Info flags for returned setup */ diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 818dff1de545..23dbe43a6691 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -516,6 +516,7 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { int err; + struct snd_interval *r = hw_param_interval(params, SNDRV_PCM_HW_PARAM_BCLK_RATIO);
params->info = 0; params->fifo_size = 0; @@ -525,6 +526,12 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, params->rate_num = 0; params->rate_den = 0; } + /* + * if left zero (not empty), assume userspace is oblivious, and + * completely flexible + */ + if (snd_interval_single(r) && snd_interval_min(r) == 0) + snd_interval_any(r);
err = constrain_mask_params(substream, params); if (err < 0) @@ -610,7 +617,8 @@ static inline void snd_pcm_timer_notify(struct snd_pcm_substream *substream, * Choose one configuration from configuration space defined by @params. * The configuration chosen is that obtained fixing in this order: * first access, first format, first subformat, min channels, - * min rate, min period time, max buffer size, min tick time + * min rate, min period time, max buffer size, min tick time, + * min bclk_ratio * * Return: Zero if successful, or a negative error code on failure. */ @@ -626,6 +634,7 @@ static int snd_pcm_hw_params_choose(struct snd_pcm_substream *pcm, SNDRV_PCM_HW_PARAM_PERIOD_TIME, SNDRV_PCM_HW_PARAM_BUFFER_SIZE, SNDRV_PCM_HW_PARAM_TICK_TIME, + SNDRV_PCM_HW_PARAM_BCLK_RATIO, -1 }; const int *v; @@ -2100,6 +2109,18 @@ static int snd_pcm_hw_rule_format(struct snd_pcm_hw_params *params, return snd_mask_refine(mask, &m); }
+static int snd_pcm_hw_rule_bclk_ratio(struct snd_pcm_hw_params *params, + struct snd_pcm_hw_rule *rule) +{ + struct snd_interval i; + struct snd_interval *ratios = hw_param_interval(params, SNDRV_PCM_HW_PARAM_BCLK_RATIO); + snd_interval_any(&i); + i.openmax = 1; + i.min = params_channels(params) * params_width(params); + i.integer = 1; + return snd_interval_refine(ratios, &i); +} + static int snd_pcm_hw_rule_sample_bits(struct snd_pcm_hw_params *params, struct snd_pcm_hw_rule *rule) { @@ -2180,12 +2201,18 @@ int snd_pcm_hw_constraints_init(struct snd_pcm_substream *substream) snd_interval_setinteger(constrs_interval(constrs, SNDRV_PCM_HW_PARAM_BUFFER_BYTES)); snd_interval_setinteger(constrs_interval(constrs, SNDRV_PCM_HW_PARAM_SAMPLE_BITS)); snd_interval_setinteger(constrs_interval(constrs, SNDRV_PCM_HW_PARAM_FRAME_BITS)); + snd_interval_setinteger(constrs_interval(constrs, SNDRV_PCM_HW_PARAM_BCLK_RATIO));
err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_FORMAT, snd_pcm_hw_rule_format, NULL, SNDRV_PCM_HW_PARAM_SAMPLE_BITS, -1); if (err < 0) return err; + err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_BCLK_RATIO, + snd_pcm_hw_rule_bclk_ratio, NULL, + SNDRV_PCM_HW_PARAM_SAMPLE_BITS, SNDRV_PCM_HW_PARAM_CHANNELS, -1); + if (err < 0) + return err; err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_SAMPLE_BITS, snd_pcm_hw_rule_sample_bits, NULL, SNDRV_PCM_HW_PARAM_FORMAT, @@ -2341,6 +2368,11 @@ int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream) if (err < 0) return err;
+ err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BCLK_RATIO, + hw->bclk_ratio_min, hw->bclk_ratio_max); + if (err < 0) + return err; + err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_BUFFER_BYTES, snd_pcm_hw_rule_buffer_bytes_max, substream, SNDRV_PCM_HW_PARAM_BUFFER_BYTES, -1); diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 03f36e534050..747026595634 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -381,6 +381,7 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream) struct snd_soc_pcm_stream *cpu_stream; unsigned int chan_min = 0, chan_max = UINT_MAX; unsigned int rate_min = 0, rate_max = UINT_MAX; + unsigned int bclk_ratio_min = 0, bclk_ratio_max = UINT_MAX; unsigned int rates = UINT_MAX; u64 formats = ULLONG_MAX; int i; @@ -413,6 +414,8 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream) codec_stream = &codec_dai_drv->capture; chan_min = max(chan_min, codec_stream->channels_min); chan_max = min(chan_max, codec_stream->channels_max); + bclk_ratio_min = max(bclk_ratio_min, codec_stream->bclk_ratio_min); + bclk_ratio_max = min_not_zero(bclk_ratio_max, codec_stream->bclk_ratio_max); rate_min = max(rate_min, codec_stream->rate_min); rate_max = min_not_zero(rate_max, codec_stream->rate_max); formats &= codec_stream->formats; @@ -443,6 +446,11 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream) hw->rate_min = max(hw->rate_min, rate_min); hw->rate_max = min_not_zero(hw->rate_max, cpu_stream->rate_max); hw->rate_max = min_not_zero(hw->rate_max, rate_max); + + hw->bclk_ratio_min = max(hw->bclk_ratio_min, cpu_stream->bclk_ratio_min); + hw->bclk_ratio_min = max(hw->bclk_ratio_min, bclk_ratio_min); + hw->bclk_ratio_max = min_not_zero(hw->bclk_ratio_max, cpu_stream->bclk_ratio_max); + hw->bclk_ratio_max = min_not_zero(hw->bclk_ratio_max, bclk_ratio_max); }
static int soc_pcm_components_close(struct snd_pcm_substream *substream,
Retrieve the bclk_ratio from the hw_params and provide it to the HDMI bridge.
Signed-off-by: Sven Van Asbroeck TheSven73@gmail.com --- include/sound/hdmi-codec.h | 1 + sound/soc/codecs/hdmi-codec.c | 1 + 2 files changed, 2 insertions(+)
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index 9483c55f871b..50062c773a12 100644 --- a/include/sound/hdmi-codec.h +++ b/include/sound/hdmi-codec.h @@ -53,6 +53,7 @@ struct hdmi_codec_params { int sample_rate; int sample_width; int channels; + int bclk_ratio; };
struct hdmi_codec_pdata; diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index e5b6769b9797..6a457278fe6d 100644 --- a/sound/soc/codecs/hdmi-codec.c +++ b/sound/soc/codecs/hdmi-codec.c @@ -519,6 +519,7 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, hp.sample_width = params_width(params); hp.sample_rate = params_rate(params); hp.channels = params_channels(params); + hp.bclk_ratio = params_bclk_ratio(params);
return hcp->hcd.ops->hw_params(dai->dev->parent, hcp->hcd.data, &hcp->daifmt[dai->id], &hp);
Signed-off-by: Sven Van Asbroeck TheSven73@gmail.com --- drivers/gpu/drm/i2c/tda998x_drv.c | 20 ++++++++++++++------ include/drm/i2c/tda998x.h | 1 + 2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index a7c39f39793f..caccfaf90dd2 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -893,19 +893,26 @@ tda998x_configure_audio(struct tda998x_priv *priv, reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S); clksel_aip = AIP_CLKSEL_AIP_I2S; clksel_fs = AIP_CLKSEL_FS_ACLK; - switch (params->sample_width) { + switch (params->bclk_ratio) { case 16: + cts_n = CTS_N_M(3) | CTS_N_K(0); + break; + case 32: cts_n = CTS_N_M(3) | CTS_N_K(1); break; - case 18: - case 20: - case 24: + case 48: cts_n = CTS_N_M(3) | CTS_N_K(2); break; - default: - case 32: + case 64: cts_n = CTS_N_M(3) | CTS_N_K(3); break; + case 128: + cts_n = CTS_N_M(0) | CTS_N_K(0); + break; + default: + dev_err(&priv->hdmi->dev, + "unsupported I2S bclk ratio\n"); + return -EINVAL; } break;
@@ -984,6 +991,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, struct tda998x_audio_params audio = { .sample_width = params->sample_width, .sample_rate = params->sample_rate, + .bclk_ratio = params->bclk_ratio, .cea = params->cea, };
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h index 3cb25ccbe5e6..62d85c662aa1 100644 --- a/include/drm/i2c/tda998x.h +++ b/include/drm/i2c/tda998x.h @@ -16,6 +16,7 @@ struct tda998x_audio_params { u8 format; unsigned sample_width; unsigned sample_rate; + unsigned int bclk_ratio; struct hdmi_audio_infoframe cea; u8 status[5]; };
The fsl_ssi always has a bclk_ratio of 64 in ssi master mode, no matter the configured word length.
Add a rule which constrains the bclk_ratio to 64 in ssi master mode.
Signed-off-by: Sven Van Asbroeck TheSven73@gmail.com --- sound/soc/fsl/fsl_ssi.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 0a648229e643..1c5b674fe253 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -627,6 +627,29 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi *ssi) regmap_write(regs, REG_SSI_SOR, SSI_SOR_WAIT(3)); }
+static int fsl_ssi_hw_rule_i2s_master(struct snd_pcm_hw_params *params, + struct snd_pcm_hw_rule *rule) +{ + struct snd_interval *it = hw_param_interval(params, + SNDRV_PCM_HW_PARAM_BCLK_RATIO); + struct fsl_ssi *ssi = rule->private; + struct snd_interval t; + + if (!fsl_ssi_is_i2s_master(ssi)) + return 0; + + /* + * In i2s master mode, the ssi always generates 32 physical + * bits/channel. This mode always has 2 channels. + * This results in a fixed bclk_ratio of 64. + */ + memset(&t, 0, sizeof(t)); + t.min = t.max = 64; + t.integer = 1; + + return snd_interval_refine(it, &t); +} + static int fsl_ssi_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { @@ -648,6 +671,12 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, snd_pcm_hw_constraint_step(substream->runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_SIZE, 2);
+ snd_pcm_hw_rule_add(substream->runtime, 0, + SNDRV_PCM_HW_PARAM_BCLK_RATIO, + fsl_ssi_hw_rule_i2s_master, ssi, + SNDRV_PCM_HW_PARAM_FORMAT, SNDRV_PCM_HW_PARAM_CHANNELS, + -1); + return 0; }
Hi,
On Mon, Mar 04, 2019 at 11:59:52AM -0500, Sven Van Asbroeck wrote:
Negotiation seems to work ok, but bclk_ratio is exposed to userspace via snd_pcm_hw_params, which is not acceptable.
Constrain bclk_ratio by:
- cpu dai capabilities && rules
- codec dai capabilities && rules
- minimum bclk_ratio is sample_width * channels
In hw_params_choose(), pick the smallest supported bclk_ratio, which should correspond to the most efficient solution.
If cpu and codec dais do not specify or constrain supported bclk_ratios, alsa will pick sample_width * channels.
Signed-off-by: Sven Van Asbroeck TheSven73@gmail.com
include/sound/pcm.h | 11 +++++++++++ include/sound/soc.h | 2 ++ include/uapi/sound/asound.h | 5 +++-- sound/core/pcm_native.c | 34 +++++++++++++++++++++++++++++++++- sound/soc/soc-pcm.c | 8 ++++++++ 5 files changed, 57 insertions(+), 3 deletions(-)
In UAPI of Linux sound subsystem, sample formats are represented in enumerators prefixed with 'SNDRV_PCM_FORMAT_'. In the enumerators, sample bits and padding bits are represented:
S8 S16 S20 S24 S32 S18_3 S20_3 S24_3 sample-bits: 8 16 20 24 32 18 20 24 (=width) padding-bits: 0 0 12 8 0 6 12 0 total bits: 8 16 32 32 32 24 24 24 (=physical_width)
When indicating a certain bit ratio of BCLK/WS from userspace, applications use correct combination of the above format (=physical_width) and the number of samples per audio data frame (=channels).
All of drivers can add constraints and rules for runtime of PCM substream in its .open callback. As a result, applications can see available combination of the format and the channels and choose correct combination.
In my opinion, your issue is a lack of the constraints and rules in relevant drivers; perhaps in tda998x driver. Or core functionality of ALSA SoC part has a lack of consideration about the above design of ALSA PCM core and PCM interface. If a certain combination of CPU-dai and CODEC-dai requires to ignore the above somehow, it should be done in interaction between drivers for CPU-dai/CODEC-dai. In short, your issue should not be exported to userspace.
I note that for your 'hypothetical' cpu dai[1], 20x2/20x2 mode (physical_width=20, channels=2) is not available from userspace application. We have no sample format suitable to it.
Anyway, when posting to change UAPI of Linux sound subsystem, it's better to describe the reason fot new stuffs; what they're designed for, and the reason to require them. Wnen posting in a result of series of discussion done previously, it's better to add any reference to it. The change effects all of userspace stuffs, regardless of whether they exist or doesn't. Furthermore the change has forced application developers to take care of codes newly added. Additionally, it's better to note actual example of applications with the added code. Unless, no one can judge that the added code is enough abstracted for an application to handle various type of hardware by the same code.
[1] https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/146062.h...
Regards
Takashi Sakamoto
On Tue, Mar 05, 2019 at 01:42:32PM +0900, Takashi Sakamoto wrote:
Hi,
On Mon, Mar 04, 2019 at 11:59:52AM -0500, Sven Van Asbroeck wrote:
Negotiation seems to work ok, but bclk_ratio is exposed to userspace via snd_pcm_hw_params, which is not acceptable.
Constrain bclk_ratio by:
- cpu dai capabilities && rules
- codec dai capabilities && rules
- minimum bclk_ratio is sample_width * channels
In hw_params_choose(), pick the smallest supported bclk_ratio, which should correspond to the most efficient solution.
If cpu and codec dais do not specify or constrain supported bclk_ratios, alsa will pick sample_width * channels.
Signed-off-by: Sven Van Asbroeck TheSven73@gmail.com
include/sound/pcm.h | 11 +++++++++++ include/sound/soc.h | 2 ++ include/uapi/sound/asound.h | 5 +++-- sound/core/pcm_native.c | 34 +++++++++++++++++++++++++++++++++- sound/soc/soc-pcm.c | 8 ++++++++ 5 files changed, 57 insertions(+), 3 deletions(-)
In UAPI of Linux sound subsystem, sample formats are represented in enumerators prefixed with 'SNDRV_PCM_FORMAT_'. In the enumerators, sample bits and padding bits are represented:
S8 S16 S20 S24 S32 S18_3 S20_3 S24_3sample-bits: 8 16 20 24 32 18 20 24 (=width) padding-bits: 0 0 12 8 0 6 12 0 total bits: 8 16 32 32 32 24 24 24 (=physical_width)
When indicating a certain bit ratio of BCLK/WS from userspace, applications use correct combination of the above format (=physical_width) and the number of samples per audio data frame (=channels).
You seem to be conflating the in-memory format with the on-wire format. These are two entirely different things. Your table above clearly shows the in-memory format, not the on-wire format.
Hi Takashi,
On Mon, Mar 4, 2019 at 11:42 PM Takashi Sakamoto o-takashi@sakamocchi.jp wrote:
Anyway, when posting to change UAPI of Linux sound subsystem, it's better to describe the reason fot new stuffs; what they're designed for, and the reason to require them. Wnen posting in a result of series of discussion done previously, it's better to add any reference to it.
My apologies for not linking in previous discussions, I'll certainly make sure to link in the future.
The original problem was that the fsl_ssi did not appear to work with the tda998x hdmi codec. This codec seems to be 'unique' in the sense that it needs to know the number of clocks per frame on the wire, i.e. the bclk_ratio, which no-one in alsa is providing or using at the moment.
I first made the error of conflating the physical width and the bclk_ratio, and posted this patch - Russell King quickly pointed out my error: https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/145916.h...
This then led to the following discussions: https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.h... https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.h...
Most of the discussion is about a mechanism to convey bclk_ratio from the frame master to the tda998x. We don't yet seem to have a consensus on how to do this best.
My rfc patch was only intended to provoke discussion, and to allow people to experiment with a flawed solution that would allow the alsa core to negotiate bclk_ratio between components. The uapi change is a serious flaw. bclk_ratio negotiation should be invisible to userspace. But I cannot see a way to accomplish this.
Sven
Dne 05. 03. 19 v 15:23 Sven Van Asbroeck napsal(a):
Hi Takashi,
On Mon, Mar 4, 2019 at 11:42 PM Takashi Sakamoto o-takashi@sakamocchi.jp wrote:
Anyway, when posting to change UAPI of Linux sound subsystem, it's better to describe the reason fot new stuffs; what they're designed for, and the reason to require them. Wnen posting in a result of series of discussion done previously, it's better to add any reference to it.
My apologies for not linking in previous discussions, I'll certainly make sure to link in the future.
The original problem was that the fsl_ssi did not appear to work with the tda998x hdmi codec. This codec seems to be 'unique' in the sense that it needs to know the number of clocks per frame on the wire, i.e. the bclk_ratio, which no-one in alsa is providing or using at the moment.
I first made the error of conflating the physical width and the bclk_ratio, and posted this patch - Russell King quickly pointed out my error: https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/145916.h...
This then led to the following discussions: https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.h... https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.h...
Most of the discussion is about a mechanism to convey bclk_ratio from the frame master to the tda998x. We don't yet seem to have a consensus on how to do this best.
My rfc patch was only intended to provoke discussion, and to allow people to experiment with a flawed solution that would allow the alsa core to negotiate bclk_ratio between components. The uapi change is a serious flaw. bclk_ratio negotiation should be invisible to userspace. But I cannot see a way to accomplish this.
I think that this might be resolved using double constraint rules in the affected driver:
snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_FORMAT, bclk_format_constraint, substream, SNDRV_PCM_HW_PARAM_CHANNELS, -1); snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, bclk_channels_constraint, substream, SNDRV_PCM_HW_PARAM_FORMAT, -1);
And refine the format bitmask / channel interval according your bclk constraint. The physical sample width can be obtained in both bclk_format_constraint / bclk_channels_constraint function so all input values are known. The final bclk value can be obtained like in your proposed code:
params_channels(params) * params_width(params)
Jaroslav
On Tue, Mar 05, 2019 at 09:23:46AM -0500, Sven Van Asbroeck wrote:
The original problem was that the fsl_ssi did not appear to work with the tda998x hdmi codec. This codec seems to be 'unique' in the sense that it needs to know the number of clocks per frame on the wire, i.e. the bclk_ratio, which no-one in alsa is providing or using at the moment.
I first made the error of conflating the physical width and the bclk_ratio, and posted this patch - Russell King quickly pointed out my error: https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/145916.h...
This then led to the following discussions: https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.h... https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.h...
Most of the discussion is about a mechanism to convey bclk_ratio from the frame master to the tda998x. We don't yet seem to have a consensus on how to do this best.
My rfc patch was only intended to provoke discussion, and to allow people to experiment with a flawed solution that would allow the alsa core to negotiate bclk_ratio between components. The uapi change is a serious flaw. bclk_ratio negotiation should be invisible to userspace. But I cannot see a way to accomplish this.
In your case:
+---+ +-----+ |CPU| <- wire -> |CODEC| |DAI| | DAI | +---+ +-----+
So that:
CPU-DAI = fsl_ssi CODEC-DAI = tda998x wire = I2S
In I2S: - SCK-line = serial clock - WS-line = word select - SD-line = serial data
In general I2S communication: - 2 samples are transferred in a phase of WS
In my opinion: - 'the number of clocks per frame on the wire' (=you need) = the number of phases of SCK per phase of WS
In expectation of ALSA PCM interface for hardware for usual device: - half number of phases of SCK per phase of WC = physical_width of sample = bytes per sample - the number of phases of SCK per phase of WC = physical_width * channels (=2) = bytes per frame - the ratio between phases of SCK and phases of WC per second = rate
Here, usual device is expected to give FIFO for DMA and I2S bus, synchronized to SCK without any conversion. CODED-DAI also has FIFO to process signals.
In my opinion, if both of fsl_ssi and tda988x have enough constraints and rules for runtime of PCM substream about the physical_width, channels and rate, things look to work well. Perhaps, tda998x driver should have a condition statement for its role of 'frame master' to add the constraints and rules, to describe its quirk.
It's helpful to describe the 'unique'ness of tda998x hdmi codec for the detail.
Regards
Takashi Sakamoto (not maintainer of this subsystem)
On Fri, Mar 08, 2019 at 01:10:57PM +0900, Takashi Sakamoto wrote:
On Tue, Mar 05, 2019 at 09:23:46AM -0500, Sven Van Asbroeck wrote:
The original problem was that the fsl_ssi did not appear to work with the tda998x hdmi codec. This codec seems to be 'unique' in the sense that it needs to know the number of clocks per frame on the wire, i.e. the bclk_ratio, which no-one in alsa is providing or using at the moment.
I first made the error of conflating the physical width and the bclk_ratio, and posted this patch - Russell King quickly pointed out my error: https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/145916.h...
This then led to the following discussions: https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.h... https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.h...
Most of the discussion is about a mechanism to convey bclk_ratio from the frame master to the tda998x. We don't yet seem to have a consensus on how to do this best.
My rfc patch was only intended to provoke discussion, and to allow people to experiment with a flawed solution that would allow the alsa core to negotiate bclk_ratio between components. The uapi change is a serious flaw. bclk_ratio negotiation should be invisible to userspace. But I cannot see a way to accomplish this.
In your case:
+---+ +-----+ |CPU| <- wire -> |CODEC| |DAI| | DAI | +---+ +-----+
So that:
CPU-DAI = fsl_ssi CODEC-DAI = tda998x wire = I2S
In I2S:
- SCK-line = serial clock
- WS-line = word select
- SD-line = serial data
In general I2S communication:
- 2 samples are transferred in a phase of WS
In my opinion:
- 'the number of clocks per frame on the wire' (=you need) = the number of phases of SCK per phase of WS
In expectation of ALSA PCM interface for hardware for usual device:
- half number of phases of SCK per phase of WC = physical_width of sample = bytes per sample
They are not the same thing.
Let's take SNDRV_PCM_FORMAT_S16_LE. The in-memory layout of this format is two 16-bit samples next to each other in a single 32-bit word. Their width is 16, their physical_width is 16, and bytes per sample is 2.
A CPU DAI can do one of two things:
1) it can generate exactly 16 SCK clock cycles per sample before WS changes state, leading to a total of 32 SCK clock cycles per frame.
2) it can generate more than 16 SCK clock cycles per sample.
Both are entirely legal and permissable under the I2S specification. Both look the same in memory.
The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify which of these two is used on the wire - it only specifies the in- memory format.
If it were to specify the on-wire format, then we'd have to multiply the number of in-memory formats by the number of on-wire formats. These are (at least): AC'97, SPDIF, I2S(Philips), I2S(Left justified), I2S(Right justified), two different DSP formats, and PDM. Then for at least the tree I2S modes, there are the number of SCK clocks per sample which can be anything from the number of bits in the sample up to an undefined limit.
What this means is that multiplying the number of in-memory formats by the number of unboundable bus-specific formats leads to an unboundable quantity of formats.
This is why ASoC has the ability to set the bclk (bit clock, SCK) to sample rate ratio - in other words, the number of clocks to completely transmit the samples for the number of channels on the link - bit clock rate = sample rate * bclk_ratio.
On Fri, Mar 08, 2019 at 12:59:16PM +0000, Russell King - ARM Linux admin wrote:
On Fri, Mar 08, 2019 at 01:10:57PM +0900, Takashi Sakamoto wrote:
On Tue, Mar 05, 2019 at 09:23:46AM -0500, Sven Van Asbroeck wrote:
The original problem was that the fsl_ssi did not appear to work with the tda998x hdmi codec. This codec seems to be 'unique' in the sense that it needs to know the number of clocks per frame on the wire, i.e. the bclk_ratio, which no-one in alsa is providing or using at the moment.
I first made the error of conflating the physical width and the bclk_ratio, and posted this patch - Russell King quickly pointed out my error: https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/145916.h...
This then led to the following discussions: https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.h... https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/thread.h...
Most of the discussion is about a mechanism to convey bclk_ratio from the frame master to the tda998x. We don't yet seem to have a consensus on how to do this best.
My rfc patch was only intended to provoke discussion, and to allow people to experiment with a flawed solution that would allow the alsa core to negotiate bclk_ratio between components. The uapi change is a serious flaw. bclk_ratio negotiation should be invisible to userspace. But I cannot see a way to accomplish this.
In your case:
+---+ +-----+ |CPU| <- wire -> |CODEC| |DAI| | DAI | +---+ +-----+
So that:
CPU-DAI = fsl_ssi CODEC-DAI = tda998x wire = I2S
In I2S:
- SCK-line = serial clock
- WS-line = word select
- SD-line = serial data
In general I2S communication:
- 2 samples are transferred in a phase of WS
In my opinion:
- 'the number of clocks per frame on the wire' (=you need) = the number of phases of SCK per phase of WS
In expectation of ALSA PCM interface for hardware for usual device:
- half number of phases of SCK per phase of WC = physical_width of sample = bytes per sample
They are not the same thing.
Let's take SNDRV_PCM_FORMAT_S16_LE. The in-memory layout of this format is two 16-bit samples next to each other in a single 32-bit word. Their width is 16, their physical_width is 16, and bytes per sample is 2.
A CPU DAI can do one of two things:
it can generate exactly 16 SCK clock cycles per sample before WS changes state, leading to a total of 32 SCK clock cycles per frame.
it can generate more than 16 SCK clock cycles per sample.
Both are entirely legal and permissable under the I2S specification. Both look the same in memory.
The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify which of these two is used on the wire - it only specifies the in- memory format.
If it were to specify the on-wire format, then we'd have to multiply the number of in-memory formats by the number of on-wire formats. These are (at least): AC'97, SPDIF, I2S(Philips), I2S(Left justified), I2S(Right justified), two different DSP formats, and PDM. Then for at least the tree I2S modes, there are the number of SCK clocks per sample which can be anything from the number of bits in the sample up to an undefined limit.
What this means is that multiplying the number of in-memory formats by the number of unboundable bus-specific formats leads to an unboundable quantity of formats.
I missed stating another point in that. Let's say that we were to have definitions such as:
SNDRV_PCM_FORMAT_S16_LE_I2S16 SNDRV_PCM_FORMAT_S16_LE_I2S32 SNDRV_PCM_FORMAT_S16_LE_SPDIF
We have a CPU interface that can simultaneously produce both SNDRV_PCM_FORMAT_S16_LE_I2S32 and SNDRV_PCM_FORMAT_S16_LE_SPDIF. However, the ALSA format negotiation is such that only _one_ format can be negotiated between user space and kernel space. So, one of those has to be selected. So, despite the hardware being perfectly capable of producing both streams, combining the on-wire formats with the in-memory formats means that we can't support simultaneous operation, despite the hardware being perfectly capable.
Another point to make there is that in the general case, if a codec supports reception of SNDRV_PCM_FORMAT_S16_LE_I2S16, then it supports reception of SNDRV_PCM_FORMAT_S16_LE_I2S32 - the I2S bus specification explicitly allows for more clocks than there are sample bits, and states what happens to the extra bits.
The exception to that is when the receiving device uses the bit clock (aka SCK) for some other purpose, such as TDA998x devices, where a driver for such a device needs to know the ratio between the bus bit clock and the sample rate. As previously illustrated, ASoC has partial support for this.
Hi,
On Fri, Mar 08, 2019 at 12:59:16PM +0000, Russell King - ARM Linux admin wrote:
In your case:
+---+ +-----+ |CPU| <- wire -> |CODEC| |DAI| | DAI | +---+ +-----+
So that:
CPU-DAI = fsl_ssi CODEC-DAI = tda998x wire = I2S
In I2S:
- SCK-line = serial clock
- WS-line = word select
- SD-line = serial data
In general I2S communication:
- 2 samples are transferred in a phase of WS
In my opinion:
- 'the number of clocks per frame on the wire' (=you need) = the number of phases of SCK per phase of WS
In expectation of ALSA PCM interface for hardware for usual device:
- half number of phases of SCK per phase of WC = physical_width of sample = bytes per sample
They are not the same thing.
Let's take SNDRV_PCM_FORMAT_S16_LE. The in-memory layout of this format is two 16-bit samples next to each other in a single 32-bit word. Their width is 16, their physical_width is 16, and bytes per sample is 2.
A CPU DAI can do one of two things:
it can generate exactly 16 SCK clock cycles per sample before WS changes state, leading to a total of 32 SCK clock cycles per frame.
it can generate more than 16 SCK clock cycles per sample.
Both are entirely legal and permissable under the I2S specification. Both look the same in memory.
The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify which of these two is used on the wire - it only specifies the in- memory format.
If it were to specify the on-wire format, then we'd have to multiply the number of in-memory formats by the number of on-wire formats. These are (at least): AC'97, SPDIF, I2S(Philips), I2S(Left justified), I2S(Right justified), two different DSP formats, and PDM. Then for at least the tree I2S modes, there are the number of SCK clocks per sample which can be anything from the number of bits in the sample up to an undefined limit.
What this means is that multiplying the number of in-memory formats by the number of unboundable bus-specific formats leads to an unboundable quantity of formats.
This is why ASoC has the ability to set the bclk (bit clock, SCK) to sample rate ratio - in other words, the number of clocks to completely transmit the samples for the number of channels on the link - bit clock rate = sample rate * bclk_ratio.
Hm. In ALSA PCM core, the combination of 'rate_num' and 'rate_den' is another representation of sampling rate[1], and drivers can have constraints and rules for these two parameters of runtime of PCM substream. Once core functionalities and drivers in ALSA SoC part have common specifications about actual value of these two parameters, the issue could be solved, in my opinion. (But I need time to consider it further in this weekend...)
However, I don't have clear view of your case 2) yet. In that case, how drivers calculate return value in 'struct snd_pcm_ops.pointer' callback? It should be frame count, but WS seems not to be available for 16 bit sample because the number of SCK clock cycles per sample is larger than 16. (If WS clock works as I expected, SCK clock cycles per sample include expression of padding bits added to 16 bit sample.)
[1] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/include...
Thanks
Takashi Sakamoto
On Fri, Mar 08, 2019 at 11:29:46PM +0900, Takashi Sakamoto wrote:
Hi,
On Fri, Mar 08, 2019 at 12:59:16PM +0000, Russell King - ARM Linux admin wrote:
In your case:
+---+ +-----+ |CPU| <- wire -> |CODEC| |DAI| | DAI | +---+ +-----+
So that:
CPU-DAI = fsl_ssi CODEC-DAI = tda998x wire = I2S
In I2S:
- SCK-line = serial clock
- WS-line = word select
- SD-line = serial data
In general I2S communication:
- 2 samples are transferred in a phase of WS
In my opinion:
- 'the number of clocks per frame on the wire' (=you need) = the number of phases of SCK per phase of WS
In expectation of ALSA PCM interface for hardware for usual device:
- half number of phases of SCK per phase of WC = physical_width of sample = bytes per sample
They are not the same thing.
Let's take SNDRV_PCM_FORMAT_S16_LE. The in-memory layout of this format is two 16-bit samples next to each other in a single 32-bit word. Their width is 16, their physical_width is 16, and bytes per sample is 2.
A CPU DAI can do one of two things:
it can generate exactly 16 SCK clock cycles per sample before WS changes state, leading to a total of 32 SCK clock cycles per frame.
it can generate more than 16 SCK clock cycles per sample.
Both are entirely legal and permissable under the I2S specification. Both look the same in memory.
The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify which of these two is used on the wire - it only specifies the in- memory format.
If it were to specify the on-wire format, then we'd have to multiply the number of in-memory formats by the number of on-wire formats. These are (at least): AC'97, SPDIF, I2S(Philips), I2S(Left justified), I2S(Right justified), two different DSP formats, and PDM. Then for at least the tree I2S modes, there are the number of SCK clocks per sample which can be anything from the number of bits in the sample up to an undefined limit.
What this means is that multiplying the number of in-memory formats by the number of unboundable bus-specific formats leads to an unboundable quantity of formats.
This is why ASoC has the ability to set the bclk (bit clock, SCK) to sample rate ratio - in other words, the number of clocks to completely transmit the samples for the number of channels on the link - bit clock rate = sample rate * bclk_ratio.
Hm. In ALSA PCM core, the combination of 'rate_num' and 'rate_den' is another representation of sampling rate[1], and drivers can have constraints and rules for these two parameters of runtime of PCM substream. Once core functionalities and drivers in ALSA SoC part have common specifications about actual value of these two parameters, the issue could be solved, in my opinion. (But I need time to consider it further in this weekend...)
rate_num and rate_den describe the sampling rate in fractional notation, as described by the ALSA tracepoint documentation:
``rate_num`` Read-only. This value represents numerator of sampling rate in fraction notation. Basically, when a parameter of SNDRV_PCM_HW_PARAM_RATE was decided as a single value, this value is also calculated according to it. Else, zero. But this behaviour depends on implementations in driver side. ``rate_den`` Read-only. This value represents denominator of sampling rate in fraction notation. Basically, when a parameter of SNDRV_PCM_HW_PARAM_RATE was decided as a single value, this value is also calculated according to it. Else, zero. But this behaviour depends on implementations in driver side.
and is also described in the header files as:
unsigned int rate_num; /* R: rate numerator */ unsigned int rate_den; /* R: rate denominator */
params_rate() returns the rate from the interval negotiated with userspace, and is described in the ALSA header files as:
#define SNDRV_PCM_HW_PARAM_RATE 11 /* Approx rate */
This only describes the sample rate, it does not describe anything to do with bit clocks or their ratio to the sample rate.
However, I don't have clear view of your case 2) yet. In that case, how drivers calculate return value in 'struct snd_pcm_ops.pointer' callback? It should be frame count, but WS seems not to be available for 16 bit sample because the number of SCK clock cycles per sample is larger than 16. (If WS clock works as I expected, SCK clock cycles per sample include expression of padding bits added to 16 bit sample.)
snd_pcm_ops.pointer has nothing at all to do with what is happening on the I2S bus. It has nothing to do with the WS nor SCK. Please see the documentation in Documentation/sound/kernel-api, which states:
pointer callback ~~~~~~~~~~~~~~~
::
static snd_pcm_uframes_t snd_xxx_pointer(struct snd_pcm_substream *substream)
This callback is called when the PCM middle layer inquires the current hardware position on the buffer. The position must be returned in frames, ranging from 0 to ``buffer_size - 1``.
This is called usually from the buffer-update routine in the pcm middle layer, which is invoked when :c:func:`snd_pcm_period_elapsed()` is called in the interrupt routine. Then the pcm middle layer updates the position and calculates the available space, and wakes up the sleeping poll threads, etc.
This is the position within the memory buffer in frames (hence the return type) where the hardware has read or written. It does *not* take account of any FIFO that may be between the DMA accessing the memory buffer and the samples appearing on the wire to the codec. ALSA models that effective delay using the "fifo_size" member of struct snd_pcm_hardware.
ALSA defines frames as (also from the documentation):
In the ALSA world, ``1 frame = channels * samples-size``.
On Fri, Mar 08, 2019 at 12:59:16PM +0000, Russell King - ARM Linux admin wrote:
On Fri, Mar 08, 2019 at 01:10:57PM +0900, Takashi Sakamoto wrote:
In expectation of ALSA PCM interface for hardware for usual device:
- half number of phases of SCK per phase of WC = physical_width of sample = bytes per sample
They are not the same thing.
Let's take SNDRV_PCM_FORMAT_S16_LE. The in-memory layout of this format is two 16-bit samples next to each other in a single 32-bit word. Their width is 16, their physical_width is 16, and bytes per sample is 2.
A CPU DAI can do one of two things:
- it can generate exactly 16 SCK clock cycles per sample before WS changes state, leading to a total of 32 SCK clock cycles per frame.
- it can generate more than 16 SCK clock cycles per sample.
Both are entirely legal and permissable under the I2S specification. Both look the same in memory.
The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify which of these two is used on the wire - it only specifies the in- memory format.
Everything Russell is saying here is correct. The actual ABI only affects the in memory format, userspace really shouldn't care what's going on on the wire. However we don't have separate infrastructure for what goes on the wire and 90% of the time you can just translate the memory layout into a wire layout which works so we're conflating the two in a lot of places internally which is confusing and fragile.
Dne 08. 03. 19 v 18:22 Mark Brown napsal(a):
On Fri, Mar 08, 2019 at 12:59:16PM +0000, Russell King - ARM Linux admin wrote:
On Fri, Mar 08, 2019 at 01:10:57PM +0900, Takashi Sakamoto wrote:
In expectation of ALSA PCM interface for hardware for usual device:
- half number of phases of SCK per phase of WC = physical_width of sample = bytes per sample
They are not the same thing.
Let's take SNDRV_PCM_FORMAT_S16_LE. The in-memory layout of this format is two 16-bit samples next to each other in a single 32-bit word. Their width is 16, their physical_width is 16, and bytes per sample is 2.
A CPU DAI can do one of two things:
- it can generate exactly 16 SCK clock cycles per sample before WS changes state, leading to a total of 32 SCK clock cycles per frame.
- it can generate more than 16 SCK clock cycles per sample.
Both are entirely legal and permissable under the I2S specification. Both look the same in memory.
The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify which of these two is used on the wire - it only specifies the in- memory format.
Everything Russell is saying here is correct. The actual ABI only affects the in memory format, userspace really shouldn't care what's going on on the wire. However we don't have separate infrastructure for what goes on the wire and 90% of the time you can just translate the memory layout into a wire layout which works so we're conflating the two in a lot of places internally which is confusing and fragile.
I agree. We just need a library which will:
1) gather the information from hardware drivers - a simple description of the bclk constrains 2) create right constraints (hw_params rules) for the ALSA PCM API 3) return the selected bclk when hw_params are installed
The description of the bclk constraints from the hardware driver might be min/max or a list of allowed wire format bit width * channels, eventually define the wire formats (bitmask) and use them in this library. I can imagine that all of those bclk contraints descriptions (or any future, if there are such requirement) can be implemented in this library.
The library should not extend hw_params (new interval), but it should work as a separate layer - use new structures / functions etc.
Jaroslav
On Fri, Mar 8, 2019 at 2:54 PM Jaroslav Kysela perex@perex.cz wrote:
I agree. We just need a library which will:
- gather the information from hardware drivers
- a simple description of the bclk constrains
- create right constraints (hw_params rules) for the ALSA PCM API
- return the selected bclk when hw_params are installed
Yes, that's what the RFC patch I posted attempts to do. But it extends hw_params, which is clearly wrong.
The library should not extend hw_params (new interval), but it should work as a separate layer - use new structures / functions etc.
This, I could not work out how to approach :)
I agree. We just need a library which will:
- gather the information from hardware drivers
- a simple description of the bclk constrains
- create right constraints (hw_params rules) for the ALSA PCM API
- return the selected bclk when hw_params are installed
Yes, that's what the RFC patch I posted attempts to do. But it extends hw_params, which is clearly wrong.
The library should not extend hw_params (new interval), but it should work as a separate layer - use new structures / functions etc.
This, I could not work out how to approach :)
I am not sure I fully understand the ask but wanted to point out that for ASoC topology-based solutions the bclk rate is typically passed as a parameter from userspace (w/ a request_firmware and topology parsing) and might be forwarded over IPC to a DSP. On some Intel platforms which can't support 32x fs that is typically how we represent a bclk ratio multiple of 25. the kernel has no idea of the relationship between the representation of the stream in memory and the final bit clock, only the DSP which programs the hardware registers knows about the latter.
It's really quite typical that the DAI is programmed for a fixed configuration and the DSP takes care of the conversions. The kernel only deals with stream triggers and power management without know all the internal details of the audio graph.
On Fri, Mar 08, 2019 at 02:49:48PM -0600, Pierre-Louis Bossart wrote:
I am not sure I fully understand the ask but wanted to point out that for ASoC topology-based solutions the bclk rate is typically passed as a parameter from userspace (w/ a request_firmware and topology parsing) and
You mean for x86 systems :) Well, big DSP really. It's not really topology related.
might be forwarded over IPC to a DSP. On some Intel platforms which can't support 32x fs that is typically how we represent a bclk ratio multiple of 25. the kernel has no idea of the relationship between the representation of the stream in memory and the final bit clock, only the DSP which programs the hardware registers knows about the latter.
It's really quite typical that the DAI is programmed for a fixed configuration and the DSP takes care of the conversions. The kernel only deals with stream triggers and power management without know all the internal details of the audio graph.
I think this is more the issue with not having transitioned fully to components which we've talked about before I think - it's related but not quite the same thing. In the big DSP case there's really two audio links that aren't really connected but we're currently trying to treat them as one since the code was designed for much smaller DSPs.
On 3/8/19 3:13 PM, Mark Brown wrote:
On Fri, Mar 08, 2019 at 02:49:48PM -0600, Pierre-Louis Bossart wrote:
I am not sure I fully understand the ask but wanted to point out that for ASoC topology-based solutions the bclk rate is typically passed as a parameter from userspace (w/ a request_firmware and topology parsing) and
You mean for x86 systems :) Well, big DSP really. It's not really topology related.
I was referring to asoc.h and the following structure. For once it's not an Intel-specific hack, though the topology does need a lot of love to be hardened and extended.
struct snd_soc_tplg_hw_config { __le32 size; /* in bytes of this structure */ __le32 id; /* unique ID - - used to match */ __le32 fmt; /* SND_SOC_DAI_FORMAT_ format value */ __u8 clock_gated; /* SND_SOC_TPLG_DAI_CLK_GATE_ value */ __u8 invert_bclk; /* 1 for inverted BCLK, 0 for normal */ __u8 invert_fsync; /* 1 for inverted frame clock, 0 for normal */ __u8 bclk_master; /* SND_SOC_TPLG_BCLK_ value */ __u8 fsync_master; /* SND_SOC_TPLG_FSYNC_ value */ __u8 mclk_direction; /* SND_SOC_TPLG_MCLK_ value */ __le16 reserved; /* for 32bit alignment */ __le32 mclk_rate; /* MCLK or SYSCLK freqency in Hz */ __le32 bclk_rate; /* BCLK frequency in Hz */ __le32 fsync_rate; /* frame clock in Hz */ __le32 tdm_slots; /* number of TDM slots in use */ __le32 tdm_slot_width; /* width in bits for each slot */ __le32 tx_slots; /* bit mask for active Tx slots */ __le32 rx_slots; /* bit mask for active Rx slots */ __le32 tx_channels; /* number of Tx channels */ __le32 tx_chanmap[SND_SOC_TPLG_MAX_CHAN]; /* array of slot number */ __le32 rx_channels; /* number of Rx channels */ __le32 rx_chanmap[SND_SOC_TPLG_MAX_CHAN]; /* array of slot number */ } __attribute__((packed));
might be forwarded over IPC to a DSP. On some Intel platforms which can't support 32x fs that is typically how we represent a bclk ratio multiple of 25. the kernel has no idea of the relationship between the representation of the stream in memory and the final bit clock, only the DSP which programs the hardware registers knows about the latter. It's really quite typical that the DAI is programmed for a fixed configuration and the DSP takes care of the conversions. The kernel only deals with stream triggers and power management without know all the internal details of the audio graph.
I think this is more the issue with not having transitioned fully to components which we've talked about before I think - it's related but not quite the same thing. In the big DSP case there's really two audio links that aren't really connected but we're currently trying to treat them as one since the code was designed for much smaller DSPs.
Yes, this notion of hw_params negotiation made me think about the constraints propagation that Lars talked about ~2 years ago, it's not as simple as a helper library I am afraid.
This discussion on bclk ratio makes a lot of sense. Some codecs have undocumented restrictions, e.g PCM512x or some Maxim amps, and it's not uncommon to come up with a configuration mismatch that take time to debug. If at least we could have an error thrown it'd be a good thing.
Hi all,
On Fri, Mar 08, 2019 at 08:54:03PM +0100, Jaroslav Kysela wrote:
Dne 08. 03. 19 v 18:22 Mark Brown napsal(a):
On Fri, Mar 08, 2019 at 12:59:16PM +0000, Russell King - ARM Linux admin wrote:
On Fri, Mar 08, 2019 at 01:10:57PM +0900, Takashi Sakamoto wrote:
In expectation of ALSA PCM interface for hardware for usual device:
- half number of phases of SCK per phase of WC = physical_width of sample = bytes per sample
They are not the same thing.
Let's take SNDRV_PCM_FORMAT_S16_LE. The in-memory layout of this format is two 16-bit samples next to each other in a single 32-bit word. Their width is 16, their physical_width is 16, and bytes per sample is 2.
A CPU DAI can do one of two things:
- it can generate exactly 16 SCK clock cycles per sample before WS changes state, leading to a total of 32 SCK clock cycles per frame.
- it can generate more than 16 SCK clock cycles per sample.
Both are entirely legal and permissable under the I2S specification. Both look the same in memory.
The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify which of these two is used on the wire - it only specifies the in- memory format.
Everything Russell is saying here is correct. The actual ABI only affects the in memory format, userspace really shouldn't care what's going on on the wire. However we don't have separate infrastructure for what goes on the wire and 90% of the time you can just translate the memory layout into a wire layout which works so we're conflating the two in a lot of places internally which is confusing and fragile.
I agree. We just need a library which will:
- gather the information from hardware drivers
- a simple description of the bclk constrains
- create right constraints (hw_params rules) for the ALSA PCM API
- return the selected bclk when hw_params are installed
The description of the bclk constraints from the hardware driver might be min/max or a list of allowed wire format bit width * channels, eventually define the wire formats (bitmask) and use them in this library. I can imagine that all of those bclk contraints descriptions (or any future, if there are such requirement) can be implemented in this library.
The library should not extend hw_params (new interval), but it should work as a separate layer - use new structures / functions etc.
As another option I can suggest; usage of 'subformat' field of 'struct snd_pcm_hw_params'.
At present, ALSA PCM interface has one entry to this field: - SNDRV_PCM_SUBFORMAT_STD
And no drivers use this entry.
Let's assume to add new entries to represent bclk/ws for the issued case: - SNDRV_PCM_SUBFORMAT_I2S_16BCLK_PER_WS - SNDRV_PCM_SUBFORMAT_I2S_48BCLK_PER_WS - SNDRV_PCM_SUBFORMAT_I2S_64BCLK_PER_WS - SNDRV_PCM_SUBFORMAT_I2S_128BCLK_PER_WS
Drivers can have constraints and rules for the parameter. At least, ALSA PCM core should have an rule (if I understand correctly): - BCLK count of these subformats > physical_width * channel
One week point is it's not 'interval' type of parameter but 'mask' parameter, thus we can't represent min/max/integer and so on. However, less changes for ALSA PCM interface.
This is just an idea so it's not enough for me to consider about relevant implementation. Any indication is welcome.
Regards
Takashi Sakamoto
Dne 11. 03. 19 v 9:15 Takashi Sakamoto napsal(a):
Hi all,
On Fri, Mar 08, 2019 at 08:54:03PM +0100, Jaroslav Kysela wrote:
Dne 08. 03. 19 v 18:22 Mark Brown napsal(a):
On Fri, Mar 08, 2019 at 12:59:16PM +0000, Russell King - ARM Linux admin wrote:
On Fri, Mar 08, 2019 at 01:10:57PM +0900, Takashi Sakamoto wrote:
In expectation of ALSA PCM interface for hardware for usual device:
- half number of phases of SCK per phase of WC = physical_width of sample = bytes per sample
They are not the same thing.
Let's take SNDRV_PCM_FORMAT_S16_LE. The in-memory layout of this format is two 16-bit samples next to each other in a single 32-bit word. Their width is 16, their physical_width is 16, and bytes per sample is 2.
A CPU DAI can do one of two things:
- it can generate exactly 16 SCK clock cycles per sample before WS changes state, leading to a total of 32 SCK clock cycles per frame.
- it can generate more than 16 SCK clock cycles per sample.
Both are entirely legal and permissable under the I2S specification. Both look the same in memory.
The ALSA format specification (SNDRV_PCM_FORMAT_*) does not specify which of these two is used on the wire - it only specifies the in- memory format.
Everything Russell is saying here is correct. The actual ABI only affects the in memory format, userspace really shouldn't care what's going on on the wire. However we don't have separate infrastructure for what goes on the wire and 90% of the time you can just translate the memory layout into a wire layout which works so we're conflating the two in a lot of places internally which is confusing and fragile.
I agree. We just need a library which will:
- gather the information from hardware drivers
- a simple description of the bclk constrains
- create right constraints (hw_params rules) for the ALSA PCM API
- return the selected bclk when hw_params are installed
The description of the bclk constraints from the hardware driver might be min/max or a list of allowed wire format bit width * channels, eventually define the wire formats (bitmask) and use them in this library. I can imagine that all of those bclk contraints descriptions (or any future, if there are such requirement) can be implemented in this library.
The library should not extend hw_params (new interval), but it should work as a separate layer - use new structures / functions etc.
As another option I can suggest; usage of 'subformat' field of 'struct snd_pcm_hw_params'.
At present, ALSA PCM interface has one entry to this field:
- SNDRV_PCM_SUBFORMAT_STD
And no drivers use this entry.
Let's assume to add new entries to represent bclk/ws for the issued case:
- SNDRV_PCM_SUBFORMAT_I2S_16BCLK_PER_WS
- SNDRV_PCM_SUBFORMAT_I2S_48BCLK_PER_WS
- SNDRV_PCM_SUBFORMAT_I2S_64BCLK_PER_WS
- SNDRV_PCM_SUBFORMAT_I2S_128BCLK_PER_WS
Drivers can have constraints and rules for the parameter. At least, ALSA PCM core should have an rule (if I understand correctly):
- BCLK count of these subformats > physical_width * channel
One week point is it's not 'interval' type of parameter but 'mask' parameter, thus we can't represent min/max/integer and so on. However, less changes for ALSA PCM interface.
This is just an idea so it's not enough for me to consider about relevant implementation. Any indication is welcome.
I would not use any of the "user space" ioctl API to represent the hardware bclk requirements. The applications should know just the DMA memory layout.
Also, think about the multiple simultaneous paths for the audio output in the sound controller (so one DMA from the user space to the controller, but the controller can do multiple simultaneous outputs using different clocks combining different wire buses or so). Yes, it's the corner case, but it's another reason to have the bclk code totally separated from the user space ALSA's PCM API.
Jaroslav
On Mon, Mar 11, 2019 at 04:43:39PM +0100, Jaroslav Kysela wrote:
I would not use any of the "user space" ioctl API to represent the hardware bclk requirements. The applications should know just the DMA memory layout.
Also, think about the multiple simultaneous paths for the audio output in the sound controller (so one DMA from the user space to the controller, but the controller can do multiple simultaneous outputs using different clocks combining different wire buses or so). Yes, it's the corner case, but it's another reason to have the bclk code totally separated from the user space ALSA's PCM API.
There's also a range of devices that either don't have visible buses at all due to integration or which are on buses that look nothing like the I2S/DSP mode style of bus, rendering the parameters meaningless.
Hi,
On Tue, Mar 12, 2019 at 03:03:00PM +0000, Mark Brown wrote:
On Mon, Mar 11, 2019 at 04:43:39PM +0100, Jaroslav Kysela wrote:
I would not use any of the "user space" ioctl API to represent the hardware bclk requirements. The applications should know just the DMA memory layout.
Also, think about the multiple simultaneous paths for the audio output in the sound controller (so one DMA from the user space to the controller, but the controller can do multiple simultaneous outputs using different clocks combining different wire buses or so). Yes, it's the corner case, but it's another reason to have the bclk code totally separated from the user space ALSA's PCM API.
There's also a range of devices that either don't have visible buses at all due to integration or which are on buses that look nothing like the I2S/DSP mode style of bus, rendering the parameters meaningless.
Indeed. That's resonable to add no changes to ALSA PCM interface. When suggesting usage of 'rate_num' and 'rate_den', I assumed to change ALSA SoC part internal to have constraints and rules for them, with no changes of ALSA PCM inteface itself. I agree that the dicision of on-wire format should not be exposed to userspace as well.
[1] https://mailman.alsa-project.org/pipermail/alsa-devel/2019-March/146261.html
Thanks
Takashi Sakamoto
participants (6)
-
Jaroslav Kysela -
Mark Brown -
Pierre-Louis Bossart -
Russell King - ARM Linux admin -
Sven Van Asbroeck -
Takashi Sakamoto