[PATCH v2] ASoC: meson: aiu-formatter-i2s: remove pipeline reset from prepare

[PATCH v2] ASoC: meson: aiu-formatter-i2s: remove pipeline reset from prepare

[Valerio Setti]

'aiu-fifo-i2s' (DAI FE) already resets the I2S pipeline in
'aiu_fifo_i2s_trigger' for all relevant trigger scenarios, right
before starting the FIFO. Since the DAI triggering order is the
default one (FE before BE), the reset performed in
'aiu_formatter_i2s_prepare' happens after the FIFO has already been
reset and started, which corrupts playback in 24-bit mode. Remove
the duplicated reset from the formatter.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
---
A patch series has recently been merged in c7852d2dcf66
("ASoC: meson: aiu: align I2S design to the AXG one") which unfortunately
introduces a bug in 24 bit mode playback which this new series resolves.

Among other things the previous series moved the content of what was once
called 'aiu_encoder_i2s_setup_desc' from 'aiu-encoder-i2s' to
'aiu-formatter-i2s'.
'aiu_encoder_i2s_setup_desc' was basically accomplishing two tasks:
- reset the i2s pipeline.
- configure number of channels and physical samples width.
Before being moved 'aiu_encoder_i2s_setup_desc' was called in the encoder
DAI 'hw_params()', whereas after the move it is called at trigger time
('aiu_encoder_i2s_trigger'->'gx_stream_start' -> 'gx_formatter_enable' ->
'aiu_formatter_i2s_prepare').

In parallel 'aiu-fifo-i2s' (DAI FE) already performs the very same reset
of the pipeline at trigger time in 'aiu_fifo_i2s_trigger' and then it
triggers the playback.

Since the DAI triggering order is the default one (FE before BE) this
means that the pipeline reset in BE happens when the FE already did it and
started the playback. This causes the 24-bit playback mode to be
corrupted.

This series removes pipeline reset from 'aiu_formatter_i2s_prepare' because
the very same operation is done by 'aiu-fifo-i2s' on trigger.

This series depends on c7852d2dcf66 ("ASoC: meson: aiu: align I2S design
to the AXG one") which exists in 'broonie/sound.git#for-7.3', but which
has not been mainlined yet.

Apologies for the inconvenience.
---
Changes in v2:
- Dropped the first commit of the series because, as found by Sashiko, it
  could lead to other playback issues in case the sample width was changed
  without closing the PCM stream (i.e. play->stop->reconfigure->play).
- Link to v1: https://patch.msgid.link/20260702-fix-24-bit-i2s-playback-v1-0-bcd3a43fe32a@baylibre.com

To: Jerome Brunet <jbrunet@baylibre.com>
To: Liam Girdwood <lgirdwood@gmail.com>
To: Mark Brown <broonie@kernel.org>
To: Jaroslav Kysela <perex@perex.cz>
To: Takashi Iwai <tiwai@suse.com>
To: Neil Armstrong <neil.armstrong@linaro.org>
To: Kevin Hilman <khilman@baylibre.com>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: linux-sound@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-amlogic@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 sound/soc/meson/aiu-formatter-i2s.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/sound/soc/meson/aiu-formatter-i2s.c b/sound/soc/meson/aiu-formatter-i2s.c
index b4604734fe88..cb554c2e7ce4 100644
--- a/sound/soc/meson/aiu-formatter-i2s.c
+++ b/sound/soc/meson/aiu-formatter-i2s.c
@@ -13,7 +13,6 @@
 #define AIU_I2S_SOURCE_DESC_MODE_8CH	BIT(0)
 #define AIU_I2S_SOURCE_DESC_MODE_24BIT	BIT(5)
 #define AIU_I2S_SOURCE_DESC_MODE_32BIT	BIT(9)
-#define AIU_RST_SOFT_I2S_FAST		BIT(0)
 
 #define AIU_I2S_DAC_CFG_MSB_FIRST	BIT(2)
 
@@ -55,11 +54,11 @@ static int aiu_formatter_i2s_prepare(struct regmap *map,
 {
 	/* Always operate in split (classic interleaved) mode */
 	unsigned int desc = 0;
-	unsigned int tmp;
 
-	/* Reset required to update the pipeline */
-	regmap_write(map, AIU_RST_SOFT, AIU_RST_SOFT_I2S_FAST);
-	regmap_read(map, AIU_I2S_SYNC, &tmp);
+	/*
+	 * Pipeline reset is already implemented in aiu_fifo_i2s_trigger() at
+	 * trigger time.
+	 */
 
 	switch (ts->physical_width) {
 	case 16: /* Nothing to do */

---
base-commit: bff7fad1010eea6f183fb110b54171cf8700ef8e
change-id: 20260702-fix-24-bit-i2s-playback-6444174c7cd7

Best regards,
--  
Valerio Setti <vsetti@baylibre.com>


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v2] ASoC: meson: aiu-formatter-i2s: remove pipeline reset from prepare

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] The formatter is configured in the ALSA `.trigger` callback rather than `.prepare`, causing it to be updated after the FIFO has already been enabled and is actively streaming data.
--

commit 8c488e8a8134ef43be2df030518f521b015aa61c
Author: Valerio Setti <vsetti@baylibre.com>

ASoC: meson: aiu-formatter-i2s: remove pipeline reset from prepare

This commit removes a pipeline reset from the AIU I2S formatter prepare
callback. The reset is already handled by the DAI frontend during trigger,
and duplicating it here corrupts playback in 24-bit mode since the FIFO
has already been started.

> diff --git a/sound/soc/meson/aiu-formatter-i2s.c b/sound/soc/meson/aiu-formatter-i2s.c
> index b4604734fe88d..cb554c2e7ce4a 100644
> --- a/sound/soc/meson/aiu-formatter-i2s.c
> +++ b/sound/soc/meson/aiu-formatter-i2s.c

[ ... ]

> @@ -55,11 +54,11 @@ static int aiu_formatter_i2s_prepare(struct regmap *map,
>  {
>  	/* Always operate in split (classic interleaved) mode */
>  	unsigned int desc = 0;
> -	unsigned int tmp;
>  
> -	/* Reset required to update the pipeline */
> -	regmap_write(map, AIU_RST_SOFT, AIU_RST_SOFT_I2S_FAST);
> -	regmap_read(map, AIU_I2S_SYNC, &tmp);
> +	/*
> +	 * Pipeline reset is already implemented in aiu_fifo_i2s_trigger() at
> +	 * trigger time.
> +	 */

[Severity: Medium]
This is a pre-existing issue, but does the formatter configuration happen too
late in the sequence?

The commit notes that the frontend starts the FIFO before this formatter
gets configured. Looking at the call chain, aiu_formatter_i2s_prepare() in
sound/soc/meson/aiu-formatter-i2s.c is executed during the ALSA trigger
phase instead of the prepare phase:

sound/soc/meson/aiu-encoder-i2s.c:aiu_encoder_i2s_trigger()
    ret = gx_stream_start(ts);

sound/soc/meson/gx-formatter.c:gx_formatter_enable()
    ret = formatter->drv->ops->prepare(formatter->map,
                                       formatter->drv->quirks,
                                       formatter->stream);

Modifying hardware data path configuration like bit width and channel count
while the DMA FIFO is already feeding data to it can cause audio glitches,
dropped samples, or corruption at the start of playback.

Could the formatter configuration be moved to the actual ALSA prepare
callback rather than being deferred to the trigger callback?

>  
>  	switch (ts->physical_width) {
>  	case 16: /* Nothing to do */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-fix-24-bit-i2s-playback-v2-1-9c008ff0b211@baylibre.com?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic