From 6b44ad3ec83b4f06bb6959f4e75998c7d7f7f070 Mon Sep 17 00:00:00 2001 From: Valerio Setti Date: Fri, 10 Jul 2026 23:09:49 +0200 Subject: [PATCH 1/3] ASoC: meson: aiu-encoder-i2s: fix bs quirk incompatibility check The bs-quirk incompatibility check has two flaws: - It only rejects one direction of the mismatch. A stream that does not require the quirk is rejected while a quirked stream is active, but the opposite is not true: a stream requiring the quirk passes the check while a non-quirked stream is active, silently reprogramming the shared mclk/bclk divider with the 50% increase and corrupting the output of the running stream. - 'bs_quirk' is only cleared in hw_free() when the last substream closes, but userspace may legally stop/reconfigure/start the stream without an intervening hw_free. Reconfiguring a single stream from the quirked configuration (8ch/16-bit) to one that does not need the quirk therefore fails with -EINVAL due to the stale flag. Drop the interface-wide flag and instead compare the quirk requirement of the incoming parameters against the committed configuration of the opposite stream at hw_params() time. The committed channels/width are cleared in hw_free() so that a released stream no longer constrains the other one. Signed-off-by: Valerio Setti Link: https://patch.msgid.link/20260710-aiu-improve-quirk-check-v1-1-2fdd1b6f8896@baylibre.com Signed-off-by: Mark Brown --- sound/soc/meson/aiu-encoder-i2s.c | 66 ++++++++++++++++++++----------- sound/soc/meson/gx-interface.h | 3 -- 2 files changed, 43 insertions(+), 26 deletions(-) diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c index 83b579e98f1c..c2a280bfdfe2 100644 --- a/sound/soc/meson/aiu-encoder-i2s.c +++ b/sound/soc/meson/aiu-encoder-i2s.c @@ -62,13 +62,36 @@ static int aiu_encoder_i2s_set_legacy_div(struct snd_soc_component *component, return 0; } +/* + * Return true if the given combination of channels and sample width requires + * the bs quirk. Return false otherwise. + */ +static bool aiu_encoder_is_bs_quirk(unsigned int channels, int width) +{ + return (channels == 8) && (width == 16); +} + +static int aiu_encoder_check_bs_quirk(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) +{ + struct gx_stream *other_stream = snd_soc_dai_dma_data_get(dai, !substream->stream); + + /* Nothing to do if the other stream doesn't exist or it's not configured yet. */ + if (!other_stream || !other_stream->channels) + return 0; + + if (aiu_encoder_is_bs_quirk(other_stream->channels, other_stream->width) != + aiu_encoder_is_bs_quirk(params_channels(params), params_width(params))) + return -EINVAL; + + return 0; +} + static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component, struct snd_pcm_hw_params *params, unsigned int bs) { - struct aiu *aiu = snd_soc_component_get_drvdata(component); - struct gx_iface *iface = &aiu->i2s.iface; - /* * NOTE: this HW is odd. * In most configuration, the i2s divider is 'mclk / blck'. @@ -76,25 +99,13 @@ static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component, * increased by 50% to get the correct output rate. * No idea why ! */ - if (params_width(params) == 16 && params_channels(params) == 8) { + if (aiu_encoder_is_bs_quirk(params_channels(params), params_width(params))) { if (bs % 2) { dev_err(component->dev, "Cannot increase i2s divider by 50%%\n"); return -EINVAL; } bs += bs / 2; - iface->bs_quirk = true; - } else { - /* - * If the bs quirk is currently applied for one stream and another - * ones tries to setup a configuration for which the quirk is - * not required, then fail. - */ - if (iface->bs_quirk) { - dev_err(component->dev, - "bclk requirements are incompatible with active stream\n"); - return -EINVAL; - } } /* Use CLK_MORE for mclk to bclk divider */ @@ -110,9 +121,11 @@ static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component, return 0; } -static int aiu_encoder_i2s_set_clocks(struct snd_soc_component *component, - struct snd_pcm_hw_params *params) +static int aiu_encoder_i2s_set_clocks(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) { + struct snd_soc_component *component = dai->component; struct aiu *aiu = snd_soc_component_get_drvdata(component); struct gx_iface *iface = &aiu->i2s.iface; unsigned int srate = params_rate(params); @@ -133,10 +146,15 @@ static int aiu_encoder_i2s_set_clocks(struct snd_soc_component *component, bs = fs / 64; - if (aiu->platform->has_clk_ctrl_more_i2s_div) + if (aiu->platform->has_clk_ctrl_more_i2s_div) { + if (aiu_encoder_check_bs_quirk(substream, params, dai)) { + dev_err(dai->dev, "bclk requirements incompatible with other stream\n"); + return -EINVAL; + } ret = aiu_encoder_i2s_set_more_div(component, params, bs); - else + } else { ret = aiu_encoder_i2s_set_legacy_div(component, params, bs); + } if (ret) return ret; @@ -155,7 +173,6 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream, { struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream); struct gx_iface *iface = ts->iface; - struct snd_soc_component *component = dai->component; int ret; /* @@ -170,7 +187,7 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream, } } - ret = aiu_encoder_i2s_set_clocks(component, params); + ret = aiu_encoder_i2s_set_clocks(substream, params, dai); if (ret) { dev_err(dai->dev, "setting i2s clocks failed: %d\n", ret); return ret; @@ -219,7 +236,6 @@ static int aiu_encoder_i2s_hw_free(struct snd_pcm_substream *substream, if (snd_soc_dai_active(dai) <= 1) { aiu_encoder_i2s_divider_enable(component, 0); iface->rate = 0; - iface->bs_quirk = false; } if (ts->clk_enabled) { @@ -227,6 +243,10 @@ static int aiu_encoder_i2s_hw_free(struct snd_pcm_substream *substream, ts->clk_enabled = false; } + ts->channels = 0; + ts->width = 0; + ts->physical_width = 0; + return 0; } diff --git a/sound/soc/meson/gx-interface.h b/sound/soc/meson/gx-interface.h index 65c46dcce32a..d9ab894589fa 100644 --- a/sound/soc/meson/gx-interface.h +++ b/sound/soc/meson/gx-interface.h @@ -22,9 +22,6 @@ struct gx_iface { /* For component wide symmetry */ int rate; - - /* Only for GX platform */ - int bs_quirk; }; struct gx_stream { From df3c987ab3d70146e2c657bc50efe8737aa4da28 Mon Sep 17 00:00:00 2001 From: Valerio Setti Date: Fri, 10 Jul 2026 23:09:50 +0200 Subject: [PATCH 2/3] ASoC: meson: aiu-encoder-i2s: reflect bs quirk in hw constraints Currently the only check for bs-quirk is implemented in hw_params(), but this is too late: nothing in the refined hw parameters hints at the restriction, so userspace has no way to know the configuration is invalid until the setup fails, as Jerome pointed out during review [1]. Add hw rules on CHANNELS and SAMPLE_BITS at startup() so that the restriction shows up during parameter refinement instead. The rules are refined against the committed configuration of the opposite stream: - if it uses the quirk, the current stream is narrowed to the same 8ch/16-bit configuration; - otherwise, selecting a 16-bit physical width limits the stream to 2 channels, and selecting 8 channels requires a physical width larger than 16 bits. The rules key on the physical width while the quirk is defined on the significant bits. This is safe because S16_LE is the only format supported by the encoder where both are 16 bits. The check in aiu_encoder_i2s_set_clocks() is kept as the last backstop: both streams may be refined concurrently before either commits its configuration. The rules are only registered on GX platforms where the bs-quirk exists and only when the DAI has a stream in the opposite direction. [1] https://lore.kernel.org/r/1jik7pebk7.fsf@starbuckisacylon.baylibre.com/ Suggested-by: Jerome Brunet Signed-off-by: Valerio Setti Link: https://lore.kernel.org/r/1jik7pebk7.fsf@starbuckisacylon.baylibre.com/ Link: https://patch.msgid.link/20260710-aiu-improve-quirk-check-v1-2-2fdd1b6f8896@baylibre.com Signed-off-by: Mark Brown --- sound/soc/meson/aiu-encoder-i2s.c | 67 +++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c index c2a280bfdfe2..4c62ea41d7e8 100644 --- a/sound/soc/meson/aiu-encoder-i2s.c +++ b/sound/soc/meson/aiu-encoder-i2s.c @@ -147,6 +147,13 @@ static int aiu_encoder_i2s_set_clocks(struct snd_pcm_substream *substream, bs = fs / 64; if (aiu->platform->has_clk_ctrl_more_i2s_div) { + /* + * The hw rules added in startup() make this unreachable in the + * sequential case, but both streams may be refined concurrently + * before either commits its config, since only ops->hw_params + * runs under the card's pcm_mutex. Re-check against the committed + * state of the other stream, which is stable under that mutex. + */ if (aiu_encoder_check_bs_quirk(substream, params, dai)) { dev_err(dai->dev, "bclk requirements incompatible with other stream\n"); return -EINVAL; @@ -333,10 +340,45 @@ static const struct snd_pcm_hw_constraint_list hw_channel_constraints = { .mask = 0, }; +static int aiu_encoder_i2s_pcm_hw_rule(struct snd_pcm_hw_params *params, + struct snd_pcm_hw_rule *rule) +{ + struct gx_stream *other = rule->private; + struct snd_interval *ch = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); + /* + * The quirk is technically based on the significant bits whereas here + * we're using the physical width for simplicity. This works because + * S16_LE is the only format supported by this encoder that has: + * significant bits = physical width = 16-bits + */ + struct snd_interval *phys_width = hw_param_interval(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS); + struct snd_interval new_i; + + if (other->channels == 0) + return 0; + + snd_interval_any(&new_i); + + if (rule->var == SNDRV_PCM_HW_PARAM_CHANNELS) { + if (aiu_encoder_is_bs_quirk(other->channels, other->width)) + new_i.min = new_i.max = 8; + else if (snd_interval_single(phys_width) && phys_width->min == 16) + new_i.max = 2; /* Force 2ch */ + } else { /* SNDRV_PCM_HW_PARAM_SAMPLE_BITS */ + if (aiu_encoder_is_bs_quirk(other->channels, other->width)) + new_i.min = new_i.max = 16; + else if (snd_interval_single(ch) && ch->min == 8) + new_i.min = 17; /* Request physical width > 16 bits */ + } + + return snd_interval_refine(hw_param_interval(params, rule->var), &new_i); +} + static int aiu_encoder_i2s_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct aiu *aiu = snd_soc_component_get_drvdata(dai->component); + struct gx_stream *other_stream = snd_soc_dai_dma_data_get(dai, !substream->stream); int ret; /* Make sure the encoder gets either 2 or 8 channels */ @@ -348,6 +390,31 @@ static int aiu_encoder_i2s_startup(struct snd_pcm_substream *substream, return ret; } + /* + * If DAI supports both playback and capture streams ensure the bs-quirk is + * handled correctly. + * This is only valid for GX platforms (has_clk_ctrl_more_i2s_div=true). + */ + if (aiu->platform->has_clk_ctrl_more_i2s_div && other_stream) { + ret = snd_pcm_hw_rule_add(substream->runtime, 0, + SNDRV_PCM_HW_PARAM_CHANNELS, + aiu_encoder_i2s_pcm_hw_rule, + other_stream, + SNDRV_PCM_HW_PARAM_CHANNELS, + SNDRV_PCM_HW_PARAM_SAMPLE_BITS, -1); + if (ret) + return ret; + + ret = snd_pcm_hw_rule_add(substream->runtime, 0, + SNDRV_PCM_HW_PARAM_SAMPLE_BITS, + aiu_encoder_i2s_pcm_hw_rule, + other_stream, + SNDRV_PCM_HW_PARAM_CHANNELS, + SNDRV_PCM_HW_PARAM_SAMPLE_BITS, -1); + if (ret) + return ret; + } + /* * Enable only clocks which are required for the interface internal * logic. MCLK is enabled/disabled from the formatter and the I2S From e82159384a5052212e7d2f2aa1de39827f1bed3c Mon Sep 17 00:00:00 2001 From: Valerio Setti Date: Fri, 10 Jul 2026 23:09:51 +0200 Subject: [PATCH 3/3] ASoC: meson: aiu-encoder-i2s: use the core symmetric_rate handling The driver manually implement the interface-wide rate symmetry enforcement in hw_params(), which suffers from the same problem addressed in the previous patch: the restriction is not visible in the hw parameter constraints, so a stream with a mismatching rate only finds out via -EINVAL late in the stream setup. The ASoC core already provides this feature through the DAI's 'symmetric_rate' flag: when another stream of the DAI is active, soc_pcm_apply_symmetry() constrains the rate at open time so the restriction shows up during parameter refinement, and soc_pcm_params_symmetry() still rejects a mismatch at hw_params() time as a backstop. Set 'symmetric_rate' on the I2S encoder DAI and drop the open-coded check along with the now unused 'rate' member of struct gx_iface. Signed-off-by: Valerio Setti Link: https://patch.msgid.link/20260710-aiu-improve-quirk-check-v1-3-2fdd1b6f8896@baylibre.com Signed-off-by: Mark Brown --- sound/soc/meson/aiu-encoder-i2s.c | 21 ++------------------- sound/soc/meson/aiu.c | 1 + sound/soc/meson/gx-interface.h | 3 --- 3 files changed, 3 insertions(+), 22 deletions(-) diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c index 4c62ea41d7e8..58dce9f08c9d 100644 --- a/sound/soc/meson/aiu-encoder-i2s.c +++ b/sound/soc/meson/aiu-encoder-i2s.c @@ -179,28 +179,14 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream); - struct gx_iface *iface = ts->iface; int ret; - /* - * Enforce interface wide rate symmetry only if there is more than - * 1 stream active. - */ - if (snd_soc_dai_active(dai) > 1) { - if (iface->rate && iface->rate != params_rate(params)) { - dev_err(dai->dev, "can't set iface rate (%d != %d)\n", - iface->rate, params_rate(params)); - return -EINVAL; - } - } - ret = aiu_encoder_i2s_set_clocks(substream, params, dai); if (ret) { dev_err(dai->dev, "setting i2s clocks failed: %d\n", ret); return ret; } - iface->rate = params_rate(params); ts->physical_width = params_physical_width(params); ts->width = params_width(params); ts->channels = params_channels(params); @@ -233,17 +219,14 @@ static int aiu_encoder_i2s_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream); - struct gx_iface *iface = ts->iface; struct snd_soc_component *component = dai->component; /* * If this is the last substream being closed then disable the i2s - * clock divider and clear 'iface->rate'. + * clock divider. */ - if (snd_soc_dai_active(dai) <= 1) { + if (snd_soc_dai_active(dai) <= 1) aiu_encoder_i2s_divider_enable(component, 0); - iface->rate = 0; - } if (ts->clk_enabled) { clk_disable_unprepare(ts->iface->mclk); diff --git a/sound/soc/meson/aiu.c b/sound/soc/meson/aiu.c index 64ace4d25d92..2668646e3597 100644 --- a/sound/soc/meson/aiu.c +++ b/sound/soc/meson/aiu.c @@ -154,6 +154,7 @@ static struct snd_soc_dai_driver aiu_cpu_dai_drv[] = { .formats = AIU_FORMATS, }, .ops = &aiu_encoder_i2s_dai_ops, + .symmetric_rate = 1, }, [CPU_SPDIF_ENCODER] = { .name = "SPDIF Encoder", diff --git a/sound/soc/meson/gx-interface.h b/sound/soc/meson/gx-interface.h index d9ab894589fa..2a6207e393e8 100644 --- a/sound/soc/meson/gx-interface.h +++ b/sound/soc/meson/gx-interface.h @@ -19,9 +19,6 @@ struct gx_iface { /* format is common to all the DAIs of the iface */ unsigned int fmt; - - /* For component wide symmetry */ - int rate; }; struct gx_stream {