[PATCH RFC] ALSA: hda/tas2781: Reload DSP program if the amplifiers lost it

[PATCH RFC] ALSA: hda/tas2781: Reload DSP program if the amplifiers lost it

[Roman Maidanovych via B4 Relay]

From: Roman Maidanovych <devuarpd@gmail.com>

On Lenovo Legion Pro 7 16ARX8H (ALC287 + two TAS2781 on I2C, codec SSID
17aa:38a7) the speakers go silent once the HDA codec has been runtime
suspended (power_save=1, e.g. after pausing playback for a few seconds).
Headphones are not affected. power_save=0 avoids it; a system
suspend/resume cycle or rebinding the driver recovers it.

The amplifiers lose their DSP program while the codec is runtime
suspended, but they are not reset: book 0 page 0 keeps its values (e.g.
CLK_CONFIG reads 0xd9, not the reset value 0x19 that tas2781_hda_spi.c
checks for). The driver still holds the cached cur_prog/cur_conf, so on
the next playback tasdevice_tuning_switch() reports "Unneeded loading dsp
conf 0" and skips the download. Forcing it with the "Speaker Force
Firmware Load" control restores the sound until the next codec suspend.

Invalidating only cur_book/cur_conf on runtime suspend, as
tas2781_hda_spi.c does, is not enough here: the program itself has to be
downloaded again. Doing that unconditionally on every runtime resume
works, but adds about 1.1 s of I2C firmware download before each
playback.

Bit 7 of register 0x44 in book 0 page 0 is set on both amplifiers exactly
when the DSP program has been lost, and clear when it is intact. Check it
when a PCM is opened and, if it is set, invalidate the cache and download
the program again; tasdevice_tuning_switch() then reloads the
configuration and calibration data as usual.

Tested on 7.3.1 with power_save=1: the speakers keep working across
repeated pauses and headphone plug/unplug cycles, and the program is
only downloaded when the bit is set.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=221735
Signed-off-by: Roman Maidanovych <devuarpd@gmail.com>
---
RFC because the meaning of B0_P0_R0x44 bit 7 is not documented in the
kernel; it was found by comparing register dumps of working and silent
cycles. Questions for TI:

1. What does bit 7 of B0_P0_R0x44 indicate, and is it a reliable way to
   detect that the DSP program needs to be downloaded again? Is there a
   better (documented) status register for this?
2. Why do the amplifiers lose the DSP program without being reset while
   the HDA codec is runtime suspended? On 6.18.x this only started after
   headphones had been plugged in once since boot; on 7.3.1 it happens
   from the first codec suspend.
3. If a status check is not wanted, would you prefer to clear cur_prog
   as well on runtime suspend (full download on every resume)?

Register dumps (book 0 page 0 of both amplifiers for working and silent
cycles) and dynamic-debug logs can be provided on request.
---
 sound/hda/codecs/side-codecs/tas2781_hda_i2c.c | 36 ++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c b/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c
index 44855f593..177a2194a 100644
--- a/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c
+++ b/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c
@@ -46,6 +46,9 @@
 #define TAS2563_CAL_TLIM		TASDEVICE_REG(0, 0x10, 0x14)
 #define TAS2563_CAL_R0			TASDEVICE_REG(0, 0x0f, 0x34)
 
+#define TAS2781_REG_DSP_LOST		TASDEVICE_REG(0x0, 0x0, 0x44)
+#define TAS2781_DSP_LOST		BIT(7)
+
 enum device_chip_id {
 	HDA_TAS2563,
 	HDA_TAS2770,
@@ -162,6 +165,38 @@ err:
 	return ret;
 }
 
+/*
+ * The amplifiers can lose their DSP program while the HDA codec is runtime
+ * suspended, without the chip being reset. The cached program/configuration
+ * then makes tasdevice_tuning_switch() skip the download and the speakers
+ * stay silent. Bit 7 of B0_P0_R0x44 is set in that state: invalidate the
+ * cache and download the program again.
+ */
+static void tas2781_hda_reload_if_lost(struct tasdevice_priv *tas_priv)
+{
+	bool lost = false;
+	int i, ret, val;
+
+	if (tas_priv->chip_id != TAS2781)
+		return;
+
+	for (i = 0; i < tas_priv->ndev; i++) {
+		ret = tasdevice_dev_read(tas_priv, i, TAS2781_REG_DSP_LOST, &val);
+		if (ret || !(val & TAS2781_DSP_LOST))
+			continue;
+
+		tas_priv->tasdevice[i].cur_book = -1;
+		tas_priv->tasdevice[i].cur_prog = -1;
+		tas_priv->tasdevice[i].cur_conf = -1;
+		lost = true;
+	}
+
+	if (lost) {
+		dev_dbg(tas_priv->dev, "DSP program lost, reloading\n");
+		tasdevice_prmg_load(tas_priv, tas_priv->cur_prog);
+	}
+}
+
 static void tas2781_hda_playback_hook(struct device *dev, int action)
 {
 	struct tas2781_hda *tas_hda = dev_get_drvdata(dev);
@@ -171,6 +206,7 @@ static void tas2781_hda_playback_hook(struct device *dev, int action)
 	case HDA_GEN_PCM_ACT_OPEN:
 		pm_runtime_get_sync(dev);
 		scoped_guard(mutex, &tas_hda->priv->codec_lock) {
+			tas2781_hda_reload_if_lost(tas_hda->priv);
 			tasdevice_tuning_switch(tas_hda->priv, 0, false);
 			tas_hda->priv->playback_started = true;
 		}

---
base-commit: 231efd8f68c950ad3b774ff51c6d15fdd0443a5b
change-id: 20261101-tas2781-reload-if-lost-60ceb71dd3c0

Best regards,
--  
Roman Maidanovych <devuarpd@gmail.com>

Re: [PATCH RFC] ALSA: hda/tas2781: Reload DSP program if the amplifiers lost it

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] ALSA: hda/tas2781: partial reload in tas2781_hda_reload_if_lost() wipes config
- [Medium] ALSA: hda/tas2781: missing init_profile_id and double load in reload_if_lost()
- [Medium] ALSA: hda/tas2781: missing fw_state check in tas2781_hda_reload_if_lost()
--

commit 74dce121e5a4c4de7e8754632f2407d57f7750d6
Author: Roman Maidanovych <devuarpd@gmail.com>

ALSA: hda/tas2781: Reload DSP program if the amplifiers lost it

This patch checks bit 7 of B0_P0_R0x44 on TAS2781 amplifiers when a PCM stream
is opened and reloads the DSP program if the amplifier lost its program during
HDA codec runtime suspend.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=221735

> diff --git a/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c b/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c
> index 44855f593daf8..177a2194aeb70 100644
> --- a/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c
> +++ b/sound/hda/codecs/side-codecs/tas2781_hda_i2c.c

[ ... ]

> @@ -162,6 +165,38 @@ static int tas2781_read_acpi(struct tasdevice_priv *p, const char *hid)
>  	return ret;
>  }
>  
> +/*
> + * The amplifiers can lose their DSP program while the HDA codec is runtime
> + * suspended, without the chip being reset. The cached program/configuration
> + * then makes tasdevice_tuning_switch() skip the download and the speakers
> + * stay silent. Bit 7 of B0_P0_R0x44 is set in that state: invalidate the
> + * cache and download the program again.
> + */
> +static void tas2781_hda_reload_if_lost(struct tasdevice_priv *tas_priv)
> +{
> +	bool lost = false;
> +	int i, ret, val;
> +
> +	if (tas_priv->chip_id != TAS2781)
> +		return;

[Severity: Medium]
Should tas2781_hda_reload_if_lost() also check that tas_priv->fw_state is
TASDEVICE_DSP_FW_ALL_OK before checking TAS2781_REG_DSP_LOST and calling
tasdevice_prmg_load()?

In tas2781_hda_bind(), tascodec_init() schedules asynchronous firmware loading
via request_firmware_nowait() and registers comp->playback_hook while
tas_priv->fw_state is still TASDEVICE_DSP_FW_PENDING and tas_priv->fmw is NULL.
Similarly, if tasdevice_dsp_parser() fails in tasdevice_dspfw_init(),
tas_priv->fw_state is set to TASDEVICE_DSP_FW_FAIL without freeing a partially
populated tas_priv->fmw:

sound/hda/codecs/side-codecs/tas2781_hda_i2c.c:tasdevice_dspfw_init() {
	...
	ret = tasdevice_dsp_parser(tas_priv);
	if (ret) {
		dev_err(tas_priv->dev, "dspfw load %s error\n",
			tas_priv->coef_binaryname);
		tas_priv->fw_state = TASDEVICE_DSP_FW_FAIL;
		return;
	}
}

Previously, tas2781_hda_playback_hook() only called tasdevice_tuning_switch(),
which returns early unless fw_state is TASDEVICE_RCA_FW_OK or
TASDEVICE_DSP_FW_ALL_OK. Without checking fw_state == TASDEVICE_DSP_FW_ALL_OK
here, opening a PCM stream before firmware loading finishes, in RCA-only mode,
or after a DSP firmware parse error can read TAS2781_DSP_LOST as set on the
unprogrammed amplifier and call tasdevice_prmg_load() with NULL or partially
parsed firmware.

> +
> +	for (i = 0; i < tas_priv->ndev; i++) {
> +		ret = tasdevice_dev_read(tas_priv, i, TAS2781_REG_DSP_LOST, &val);
> +		if (ret || !(val & TAS2781_DSP_LOST))
> +			continue;
> +
> +		tas_priv->tasdevice[i].cur_book = -1;
> +		tas_priv->tasdevice[i].cur_prog = -1;
> +		tas_priv->tasdevice[i].cur_conf = -1;
> +		lost = true;
> +	}

[Severity: Medium]
What happens if only a subset of channels have TAS2781_DSP_LOST set (or if
tasdevice_dev_read() fails transiently on one channel while succeeding on
another)?

Earlier calls to tasdevice_select_cfg_blk() leave
tas_priv->tasdevice[chn].is_loading set to true on all channels:

sound/soc/codecs/tas2781-fmwlib.c:tasdevice_select_cfg_blk() {
	...
			for (; chn < chnend; chn++)
				tas_priv->tasdevice[chn].is_loading = true;
	...
}

When tas2781_hda_reload_if_lost() then calls tasdevice_prmg_load(), the loop
only updates channels where cur_prog != prm_no and does not clear is_loading
to false on the remaining channels:

sound/soc/codecs/tas2781-fmwlib.c:tasdevice_prmg_load() {
	...
	for (i = 0, prog_status = 0; i < tas_priv->ndev; i++) {
		if (prm_no >= 0 && tas_priv->tasdevice[i].cur_prog != prm_no) {
			tas_priv->tasdevice[i].cur_conf = -1;
			tas_priv->tasdevice[i].is_loading = true;
			prog_status++;
		}
	}
}

Because is_loading is still true on the non-lost channel,
tasdevice_load_data(tas_priv, &(program->dev_data)) downloads the DSP program
(including its software reset commands) to that channel as well, wiping its
hardware state. However, because cur_conf was never reset to -1 on that
channel, the subsequent tasdevice_tuning_switch() ->
tasdevice_select_tuningprm_cfg() call sees cur_conf == cfg_no and skips
reloading the configuration and calibration data on that channel, leaving it
unconfigured.

> +
> +	if (lost) {
> +		dev_dbg(tas_priv->dev, "DSP program lost, reloading\n");
> +		tasdevice_prmg_load(tas_priv, tas_priv->cur_prog);
> +	}
> +}

[Severity: Medium]
Does reloading the program here miss applying tas_priv->rcabin.init_profile_id,
and can it cause a double program download when force_fwload_status is enabled?

In both tasdevice_dspfw_init() and tas2781_system_resume(), calling
tasdevice_prmg_load() is immediately followed by applying init_profile_id
before tasdevice_tuning_switch():

sound/hda/codecs/side-codecs/tas2781_hda_i2c.c:tas2781_system_resume() {
	...
	tasdevice_prmg_load(tas_hda->priv, tas_hda->priv->cur_prog);

	/* Init common setting for different audio profiles */
	if (tas_hda->priv->rcabin.init_profile_id >= 0)
		tasdevice_select_cfg_blk(tas_hda->priv,
			tas_hda->priv->rcabin.init_profile_id,
			TASDEVICE_BIN_BLK_PRE_POWER_UP);
	...
}

Without that step in tas2781_hda_reload_if_lost(), common profile settings
from init_profile_id are not restored after tasdevice_prmg_load() reloads the
DSP program.

In addition, tasdevice_tuning_switch(tas_hda->priv, 0, false) runs immediately
after tas2781_hda_reload_if_lost() in tas2781_hda_playback_hook() and calls
tasdevice_select_tuningprm_cfg(), which reloads the DSP program whenever
cur_prog != prm_no || tas_priv->force_fwload_status:

sound/soc/codecs/tas2781-fmwlib.c:tasdevice_select_tuningprm_cfg() {
	...
			if (prm_no >= 0
				&& (tas_priv->tasdevice[i].cur_prog != prm_no
				|| tas_priv->force_fwload_status)) {
				tas_priv->tasdevice[i].cur_conf = -1;
				tas_priv->tasdevice[i].is_loading = true;
				prog_status++;
			}
	...
}

If the "Speaker Force Firmware Load" control (force_fwload_status) is enabled,
calling tasdevice_prmg_load() in tas2781_hda_reload_if_lost() and then calling
tasdevice_tuning_switch() will download the full DSP program twice in a row on
PCM open.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101-tas2781-reload-if-lost-v1-1-9d336120f1ba@gmail.com?part=1