Re: [PATCH 1/2] ASoC: meson: gx-formatter: prepare on attach

From: Jerome Brunet <jbrunet@baylibre.com>
To: Valerio Setti <vsetti@baylibre.com>
Cc: sashiko-reviews@lists.linux.dev,  neil.armstrong@linaro.org,
	linux-amlogic@lists.infradead.org
Subject: Re: [PATCH 1/2] ASoC: meson: gx-formatter: prepare on attach
Date: Fri, 03 Jul 2026 09:34:06 +0200	[thread overview]
Message-ID: <1jo6goil1t.fsf@starbuckisacylon.baylibre.com> (raw)
In-Reply-To: <b1a0a7de-c5b1-4325-8159-d5a0bad1668e@baylibre.com> (Valerio Setti's message of "Thu, 2 Jul 2026 22:34:52 +0200")

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