Linux-Amlogic Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] ASoC: meson: aiu: fix playback issue for 24-bit mode
@ 2026-07-02 10:56 Valerio Setti
  2026-07-02 10:56 ` [PATCH 1/2] ASoC: meson: gx-formatter: prepare on attach Valerio Setti
  2026-07-02 10:56 ` [PATCH 2/2] ASoC: meson: aiu-formatter: remove pipeline reset from prepare Valerio Setti
  0 siblings, 2 replies; 9+ messages in thread
From: Valerio Setti @ 2026-07-02 10:56 UTC (permalink / raw)
  To: Jerome Brunet, Liam Girdwood, Mark Brown, Jaroslav Kysela,
	Takashi Iwai, Neil Armstrong, Kevin Hilman, Martin Blumenstingl
  Cc: linux-sound, linux-arm-kernel, linux-amlogic, linux-kernel,
	Valerio Setti

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 happens when the BE already did it and
started the playback. This cause the 24-bit playback mode to be
corrupted.

This series:
- re-orders operations so that 'aiu_formatter_i2s_prepare' is called after
  DAIs' prepare, but before triggering them.
- 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.

Please apologize for the inconvenience.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
---
Valerio Setti (2):
      ASoC: meson: gx-formatter: prepare on attach
      ASoC: meson: aiu-formatter: remove pipeline reset from prepare

 sound/soc/meson/aiu-formatter-i2s.c |  9 ++++-----
 sound/soc/meson/gx-formatter.c      | 33 +++++++++++++++++++++++++--------
 2 files changed, 29 insertions(+), 13 deletions(-)
---
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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] ASoC: meson: gx-formatter: prepare on attach
  2026-07-02 10:56 [PATCH 0/2] ASoC: meson: aiu: fix playback issue for 24-bit mode Valerio Setti
@ 2026-07-02 10:56 ` Valerio Setti
  2026-07-02 11:10   ` sashiko-bot
  2026-07-02 10:56 ` [PATCH 2/2] ASoC: meson: aiu-formatter: remove pipeline reset from prepare Valerio Setti
  1 sibling, 1 reply; 9+ messages in thread
From: Valerio Setti @ 2026-07-02 10:56 UTC (permalink / raw)
  To: Jerome Brunet, Liam Girdwood, Mark Brown, Jaroslav Kysela,
	Takashi Iwai, Neil Armstrong, Kevin Hilman, Martin Blumenstingl
  Cc: linux-sound, linux-arm-kernel, linux-amlogic, linux-kernel,
	Valerio Setti

Instead of calling the formatter driver prepare and enable at the same
time when 'gx_stream_start' is called (which happens at trigger time),
split the operation in 2: prepare is called when the widget is powered
up, whereas enable is kept on 'gx_stream_start'.

This resolves a problem related to i2s playback in 24-bit mode. Commit
c7852d2dcf66 ("ASoC: meson: aiu: align I2S design to the AXG one") 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 happens when the BE already did it and
started the playback. This cause the 24-bit playback mode to be
corrupted.

This commit re-orders operations so that 'aiu_formatter_i2s_prepare' is
called after DAIs' prepare, but before triggering them.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
---
 sound/soc/meson/gx-formatter.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/sound/soc/meson/gx-formatter.c b/sound/soc/meson/gx-formatter.c
index 311e63affb23..c7333f624b15 100644
--- a/sound/soc/meson/gx-formatter.c
+++ b/sound/soc/meson/gx-formatter.c
@@ -15,27 +15,38 @@ struct gx_formatter {
 	struct gx_stream *stream;
 	const struct gx_formatter_driver *drv;
 	bool enabled;
+	bool prepared;
 	struct regmap *map;
 };
 
-static int gx_formatter_enable(struct gx_formatter *formatter)
+static int gx_formatter_prepare(struct gx_formatter *formatter)
 {
 	int ret;
 
-	/* Do nothing if the formatter is already enabled */
-	if (formatter->enabled)
+	if (formatter->prepared)
 		return 0;
 
 	/* Setup the stream parameter in the formatter */
 	if (formatter->drv->ops->prepare) {
 		ret = formatter->drv->ops->prepare(formatter->map,
-					   formatter->drv->quirks,
-					   formatter->stream);
+						   formatter->drv->quirks,
+						   formatter->stream);
 		if (ret)
 			return ret;
 	}
 
-	/* Finally, actually enable the formatter */
+	formatter->prepared = true;
+
+	return 0;
+}
+
+static int gx_formatter_enable(struct gx_formatter *formatter)
+{
+	/* Do nothing if the formatter is already enabled */
+	if (formatter->enabled)
+		return 0;
+
+	/* Enable the formatter */
 	if (formatter->drv->ops->enable)
 		formatter->drv->ops->enable(formatter->map);
 
@@ -63,6 +74,12 @@ static int gx_formatter_attach(struct gx_formatter *formatter)
 
 	mutex_lock(&ts->lock);
 
+	ret = gx_formatter_prepare(formatter);
+	if (ret) {
+		pr_err("failed to prepare the formatter\n");
+		goto out;
+	}
+
 	/* Catch up if the stream is already running when we attach */
 	if (ts->ready) {
 		ret = gx_formatter_enable(formatter);
@@ -87,9 +104,9 @@ static void gx_formatter_detach(struct gx_formatter *formatter)
 
 	mutex_lock(&ts->lock);
 	list_del(&formatter->list);
-	mutex_unlock(&ts->lock);
-
 	gx_formatter_disable(formatter);
+	formatter->prepared = false;
+	mutex_unlock(&ts->lock);
 }
 
 static int gx_formatter_power_up(struct gx_formatter *formatter,

-- 
2.47.3


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] ASoC: meson: aiu-formatter: remove pipeline reset from prepare
  2026-07-02 10:56 [PATCH 0/2] ASoC: meson: aiu: fix playback issue for 24-bit mode Valerio Setti
  2026-07-02 10:56 ` [PATCH 1/2] ASoC: meson: gx-formatter: prepare on attach Valerio Setti
@ 2026-07-02 10:56 ` Valerio Setti
  2026-07-02 11:14   ` sashiko-bot
  2026-07-03  7:58   ` Jerome Brunet
  1 sibling, 2 replies; 9+ messages in thread
From: Valerio Setti @ 2026-07-02 10:56 UTC (permalink / raw)
  To: Jerome Brunet, Liam Girdwood, Mark Brown, Jaroslav Kysela,
	Takashi Iwai, Neil Armstrong, Kevin Hilman, Martin Blumenstingl
  Cc: linux-sound, linux-arm-kernel, linux-amlogic, linux-kernel,
	Valerio Setti

'aiu-formatter-i2s' already performs the very same reset in
'aiu_fifo_i2s_trigger' for all relevant trigger scenarios. There is no
need to duplicate the operation in the formatter.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
---
 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 */

-- 
2.47.3


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] ASoC: meson: gx-formatter: prepare on attach
  2026-07-02 10:56 ` [PATCH 1/2] ASoC: meson: gx-formatter: prepare on attach Valerio Setti
@ 2026-07-02 11:10   ` sashiko-bot
  2026-07-02 20:34     ` Valerio Setti
  0 siblings, 1 reply; 9+ messages in thread
From: sashiko-bot @ 2026-07-02 11:10 UTC (permalink / raw)
  To: Valerio Setti; +Cc: neil.armstrong, linux-amlogic

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Moving formatter prepare logic to the DAPM PRE_PMU event breaks hardware reconfiguration for back-to-back playbacks or rapid restarts.
--

commit 0be8c647d2188e02c8519d4d117bc9cb1bdb746c
Author: Valerio Setti <vsetti@baylibre.com>

ASoC: meson: gx-formatter: prepare on attach

This commit splits the prepare and enable operations for the formatter
driver. It moves the prepare operation to happen when the widget is powered
up rather than at trigger time, aiming to resolve 24-bit I2S playback
corruption caused by pipeline reset ordering conflicts.

> diff --git a/sound/soc/meson/gx-formatter.c b/sound/soc/meson/gx-formatter.c
> index 311e63affb239..c7333f624b155 100644
> --- a/sound/soc/meson/gx-formatter.c
> +++ b/sound/soc/meson/gx-formatter.c

[ ... ]

> -static int gx_formatter_enable(struct gx_formatter *formatter)
> +static int gx_formatter_prepare(struct gx_formatter *formatter)
>  {
>  	int ret;
>  
> -	/* Do nothing if the formatter is already enabled */
> -	if (formatter->enabled)
> +	if (formatter->prepared)
>  		return 0;

[Severity: High]
Will this prevent hardware reconfiguration for back-to-back playbacks?

Because the ALSA core utilizes pmdown_time (delayed power down), the DAPM
widget remains in the powered-on state for a short time after playback
stops.

If a new stream with different parameters (e.g., changing from 16-bit to
24-bit) starts rapidly before the power-down occurs, dapm_power_widgets()
will not detect a power state change and will not emit a new
SND_SOC_DAPM_PRE_PMU event.

Since the widget wasn't powered down, gx_formatter_detach() hasn't cleared
formatter->prepared. This check will return early and skip updating the
hardware for the new sample format, causing the new stream to play through
incorrectly configured hardware.

>  
>  	/* Setup the stream parameter in the formatter */
>  	if (formatter->drv->ops->prepare) {
>  		ret = formatter->drv->ops->prepare(formatter->map,
> -					   formatter->drv->quirks,
> -					   formatter->stream);
> +						   formatter->drv->quirks,
> +						   formatter->stream);
>  		if (ret)
>  			return ret;
>  	}

[ ... ]

> @@ -63,6 +74,12 @@ static int gx_formatter_attach(struct gx_formatter *formatter)
>  
>  	mutex_lock(&ts->lock);
>  
> +	ret = gx_formatter_prepare(formatter);
> +	if (ret) {
> +		pr_err("failed to prepare the formatter\n");
> +		goto out;
> +	}
> +
>  	/* Catch up if the stream is already running when we attach */
>  	if (ts->ready) {
>  		ret = gx_formatter_enable(formatter);

[ ... ]

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

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] ASoC: meson: aiu-formatter: remove pipeline reset from prepare
  2026-07-02 10:56 ` [PATCH 2/2] ASoC: meson: aiu-formatter: remove pipeline reset from prepare Valerio Setti
@ 2026-07-02 11:14   ` sashiko-bot
  2026-07-02 20:44     ` Valerio Setti
  2026-07-03  7:58   ` Jerome Brunet
  1 sibling, 1 reply; 9+ messages in thread
From: sashiko-bot @ 2026-07-02 11:14 UTC (permalink / raw)
  To: Valerio Setti; +Cc: neil.armstrong, linux-amlogic

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing the pipeline reset from `aiu_formatter_i2s_prepare()` breaks dynamic DAPM routing (DPCM), potentially leaving the I2S formatter with an un-updated or corrupted state when attached to an already-running stream.
--

commit 7dba2169385903a506465200e005128765741cfe
Author: Valerio Setti <vsetti@baylibre.com>

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

This commit removes the pipeline reset from aiu_formatter_i2s_prepare(). The
commit message claims this is redundant because it is already handled by
aiu_fifo_i2s_trigger() during trigger time.

> 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: High]
Does removing this reset break dynamic DAPM routing (DPCM)?

When a backend is dynamically connected to an already-running frontend via
DPCM, the frontend's trigger() callback is not invoked again. 

For example, if the I2S FIFO frontend is started (e.g., routed to the SPDIF
backend) and then the I2S backend is dynamically connected while the stream is
running, the "I2S Formatter" DAPM widget powers up and calls
aiu_formatter_i2s_prepare().

Without the reset pulse here, will the hardware fail to latch new
configurations (like AIU_I2S_SOURCE_DESC), potentially resulting in corrupted
or no audio output from the I2S backend?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-fix-24-bit-i2s-playback-v1-0-bcd3a43fe32a@baylibre.com?part=2

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] ASoC: meson: gx-formatter: prepare on attach
  2026-07-02 11:10   ` sashiko-bot
@ 2026-07-02 20:34     ` Valerio Setti
  2026-07-03  7:34       ` Jerome Brunet
  0 siblings, 1 reply; 9+ messages in thread
From: Valerio Setti @ 2026-07-02 20:34 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: neil.armstrong, linux-amlogic



On 7/2/26 1:10 PM, sashiko-bot@kernel.org wrote:
>> diff --git a/sound/soc/meson/gx-formatter.c b/sound/soc/meson/gx-formatter.c
>> index 311e63affb239..c7333f624b155 100644
>> --- a/sound/soc/meson/gx-formatter.c
>> +++ b/sound/soc/meson/gx-formatter.c
> 
> [ ... ]
> 
>> -static int gx_formatter_enable(struct gx_formatter *formatter)
>> +static int gx_formatter_prepare(struct gx_formatter *formatter)
>>   {
>>   	int ret;
>>   
>> -	/* Do nothing if the formatter is already enabled */
>> -	if (formatter->enabled)
>> +	if (formatter->prepared)
>>   		return 0;
> 
> [Severity: High]
> Will this prevent hardware reconfiguration for back-to-back playbacks?
> 
> Because the ALSA core utilizes pmdown_time (delayed power down), the DAPM
> widget remains in the powered-on state for a short time after playback
> stops.
> 
> If a new stream with different parameters (e.g., changing from 16-bit to
> 24-bit) starts rapidly before the power-down occurs, dapm_power_widgets()
> will not detect a power state change and will not emit a new
> SND_SOC_DAPM_PRE_PMU event.
> 
> Since the widget wasn't powered down, gx_formatter_detach() hasn't cleared
> formatter->prepared. This check will return early and skip updating the
> hardware for the new sample format, causing the new stream to play through
> incorrectly configured hardware.
> 

Uh, this seems right, but with a caveat. When I tested this series I 
launched 'speaker-test' with 16-bits and 24-bits in sequence and it 
worked perfectly.
To generate the problem described here I had to create a custom binary 
that played the two sine tones, one at 16-bits and the one at 24-bits, 
without closing the PCM in the middle (as the 'speaker-test' case was 
doing instead). This way I was able to recreate the problem. I don't 
know how common it is for an application to switch sample width "on the 
fly", but it's worth fixing it.

For the records, I just tried removing this commit and both test 
scenarios work just fine so I'm inclined in sending a v2 with this 
commit dropped.

-- 
Valerio


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] ASoC: meson: aiu-formatter: remove pipeline reset from prepare
  2026-07-02 11:14   ` sashiko-bot
@ 2026-07-02 20:44     ` Valerio Setti
  0 siblings, 0 replies; 9+ messages in thread
From: Valerio Setti @ 2026-07-02 20:44 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: neil.armstrong, linux-amlogic



On 7/2/26 1:14 PM, sashiko-bot@kernel.org wrote:

>> 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: High]
> Does removing this reset break dynamic DAPM routing (DPCM)?
> 
> When a backend is dynamically connected to an already-running frontend via
> DPCM, the frontend's trigger() callback is not invoked again.
> 
> For example, if the I2S FIFO frontend is started (e.g., routed to the SPDIF
> backend) and then the I2S backend is dynamically connected while the stream is
> running, the "I2S Formatter" DAPM widget powers up and calls
> aiu_formatter_i2s_prepare().
> 
> Without the reset pulse here, will the hardware fail to latch new
> configurations (like AIU_I2S_SOURCE_DESC), potentially resulting in corrupted
> or no audio output from the I2S backend?
> 

I'm not sure about this. The reason I removed the pipeline reset was 
that having it happening after the one on the FIFO side caused the 
stream at 24-bits to be corrupted.
In the scenario described by Sashiko we would have had the pipeline 
reset again after the one from the FIFO (which is continuouly running). 
Moreover I think that this would have happened also before my series 
that was recently merged, i.e. c7852d2dcf66
("ASoC: meson: aiu: align I2S design to the AXG one"), so I have some 
doubt that this could have worked seamlessly also before recent changes.

-- 
Valerio


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] ASoC: meson: gx-formatter: prepare on attach
  2026-07-02 20:34     ` Valerio Setti
@ 2026-07-03  7:34       ` Jerome Brunet
  0 siblings, 0 replies; 9+ messages in thread
From: Jerome Brunet @ 2026-07-03  7:34 UTC (permalink / raw)
  To: Valerio Setti; +Cc: sashiko-reviews, neil.armstrong, linux-amlogic

On jeu. 02 juil. 2026 at 22:34, Valerio Setti <vsetti@baylibre.com> wrote:

> On 7/2/26 1:10 PM, sashiko-bot@kernel.org wrote:
>>> diff --git a/sound/soc/meson/gx-formatter.c b/sound/soc/meson/gx-formatter.c
>>> index 311e63affb239..c7333f624b155 100644
>>> --- a/sound/soc/meson/gx-formatter.c
>>> +++ b/sound/soc/meson/gx-formatter.c
>> [ ... ]
>> 
>>> -static int gx_formatter_enable(struct gx_formatter *formatter)
>>> +static int gx_formatter_prepare(struct gx_formatter *formatter)
>>>   {
>>>   	int ret;
>>>   -	/* Do nothing if the formatter is already enabled */
>>> -	if (formatter->enabled)
>>> +	if (formatter->prepared)
>>>   		return 0;
>> [Severity: High]
>> Will this prevent hardware reconfiguration for back-to-back playbacks?
>> Because the ALSA core utilizes pmdown_time (delayed power down), the DAPM
>> widget remains in the powered-on state for a short time after playback
>> stops.
>> If a new stream with different parameters (e.g., changing from 16-bit to
>> 24-bit) starts rapidly before the power-down occurs, dapm_power_widgets()
>> will not detect a power state change and will not emit a new
>> SND_SOC_DAPM_PRE_PMU event.
>> Since the widget wasn't powered down, gx_formatter_detach() hasn't
>> cleared
>> formatter->prepared. This check will return early and skip updating the
>> hardware for the new sample format, causing the new stream to play through
>> incorrectly configured hardware.
>> 
>
> Uh, this seems right, but with a caveat. When I tested this series I
> launched 'speaker-test' with 16-bits and 24-bits in sequence and it worked
> perfectly.
> To generate the problem described here I had to create a custom binary that
> played the two sine tones, one at 16-bits and the one at 24-bits, without
> closing the PCM in the middle (as the 'speaker-test' case was doing
> instead). This way I was able to recreate the problem. I don't know how
> common it is for an application to switch sample width "on the fly", but
> it's worth fixing it.

You'd have to go through through .hw_params at least. I would expect it
go through .shutdown()/.startup() too but I'm not sure about this.

>
> For the records, I just tried removing this commit and both test scenarios
> work just fine so I'm inclined in sending a v2 with this commit
> dropped.

+1

-- 
Jerome

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] ASoC: meson: aiu-formatter: remove pipeline reset from prepare
  2026-07-02 10:56 ` [PATCH 2/2] ASoC: meson: aiu-formatter: remove pipeline reset from prepare Valerio Setti
  2026-07-02 11:14   ` sashiko-bot
@ 2026-07-03  7:58   ` Jerome Brunet
  1 sibling, 0 replies; 9+ messages in thread
From: Jerome Brunet @ 2026-07-03  7:58 UTC (permalink / raw)
  To: Valerio Setti
  Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
	Neil Armstrong, Kevin Hilman, Martin Blumenstingl, linux-sound,
	linux-arm-kernel, linux-amlogic, linux-kernel

On jeu. 02 juil. 2026 at 12:56, Valerio Setti <vsetti@baylibre.com> wrote:

> 'aiu-formatter-i2s' already performs the very same reset in
> 'aiu_fifo_i2s_trigger' for all relevant trigger scenarios. There is no
> need to duplicate the operation in the formatter.
>
> Signed-off-by: Valerio Setti <vsetti@baylibre.com>

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>

> ---
>  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 */

-- 
Jerome

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-07-03  7:59 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-02 10:56 [PATCH 0/2] ASoC: meson: aiu: fix playback issue for 24-bit mode Valerio Setti
2026-07-02 10:56 ` [PATCH 1/2] ASoC: meson: gx-formatter: prepare on attach Valerio Setti
2026-07-02 11:10   ` sashiko-bot
2026-07-02 20:34     ` Valerio Setti
2026-07-03  7:34       ` Jerome Brunet
2026-07-02 10:56 ` [PATCH 2/2] ASoC: meson: aiu-formatter: remove pipeline reset from prepare Valerio Setti
2026-07-02 11:14   ` sashiko-bot
2026-07-02 20:44     ` Valerio Setti
2026-07-03  7:58   ` Jerome Brunet

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox