[PATCH 0/4] Sound fix for Valve Steam Deck OLED on resume from suspend

[PATCH 0/4] Sound fix for Valve Steam Deck OLED on resume from suspend

[Cristian Ciocaltea]

Stress testing resume from suspend on Valve Steam Deck OLED (Galileo)
revealed that audio may break and cannot be recovered without performing
a system reboot.

This patch series provides a new ACP quirk to address the issue, along
with a few additional improvements to AMD Vangogh/ACP SOF drivers.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
Cristian Ciocaltea (4):
      ASoC: SOF: amd: Add post_fw_run_delay ACP quirk
      ASoC: SOF: amd: Drop unused includes from Vangogh driver
      ASoC: SOF: amd: Handle IPC replies before FW_BOOT_COMPLETE
      ASoC: SOF: amd: Add branch prediction hint in ACP IRQ handler

 sound/soc/sof/amd/acp-ipc.c     | 25 +++++++++++++++++--------
 sound/soc/sof/amd/acp.c         |  1 +
 sound/soc/sof/amd/acp.h         |  1 +
 sound/soc/sof/amd/pci-vangogh.c |  2 --
 sound/soc/sof/amd/vangogh.c     | 22 ++++++++++++++++++----
 5 files changed, 37 insertions(+), 14 deletions(-)
---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20250207-sof-vangogh-fixes-528afd048737

[PATCH 1/4] ASoC: SOF: amd: Add post_fw_run_delay ACP quirk

[Cristian Ciocaltea]

Stress testing resume from suspend on Valve Steam Deck OLED (Galileo)
revealed that the DSP firmware could enter an unrecoverable faulty
state, where the kernel ring buffer is flooded with IPC related error
messages:

[  +0.017002] snd_sof_amd_vangogh 0000:04:00.5: acp_sof_ipc_send_msg: Failed to acquire HW lock
[  +0.000054] snd_sof_amd_vangogh 0000:04:00.5: ipc3_tx_msg_unlocked: ipc message send for 0x30100000 failed: -22
[  +0.000005] snd_sof_amd_vangogh 0000:04:00.5: Failed to setup widget PIPELINE.6.ACPHS1.IN
[  +0.000004] snd_sof_amd_vangogh 0000:04:00.5: Failed to restore pipeline after resume -22
[  +0.000003] snd_sof_amd_vangogh 0000:04:00.5: PM: dpm_run_callback(): pci_pm_resume returns -22
[  +0.000009] snd_sof_amd_vangogh 0000:04:00.5: PM: failed to resume async: error -22
[...]
[  +0.002582] PM: suspend exit
[  +0.065085] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for 0x30130000 (msg/reply size: 12/0): -22
[  +0.000499] snd_sof_amd_vangogh 0000:04:00.5: error: failed widget list set up for pcm 1 dir 0
[  +0.000011] snd_sof_amd_vangogh 0000:04:00.5: error: set pcm hw_params after resume
[  +0.000006] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_pcm_component_prepare on 0000:04:00.5: -22
[...]

A system reboot would be necessary to restore the speakers
functionality.

However, by delaying a bit any host to DSP transmission right after
the firmware boot completed, the issue could not be reproduced anymore
and sound continued to work flawlessly even after performing thousands
of suspend/resume cycles.

Introduce the post_fw_run_delay ACP quirk to allow providing the
aforementioned delay via the snd_sof_dsp_ops->post_fw_run() callback for
the affected devices.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/sof/amd/acp.c     |  1 +
 sound/soc/sof/amd/acp.h     |  1 +
 sound/soc/sof/amd/vangogh.c | 18 ++++++++++++++++++
 3 files changed, 20 insertions(+)

diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
index 33648ff8b83365e76d7d90e52c2cb8f884a2fe72..9e13c96528be3371e063072513c118cfc8b93fe8 100644
--- a/sound/soc/sof/amd/acp.c
+++ b/sound/soc/sof/amd/acp.c
@@ -27,6 +27,7 @@ MODULE_PARM_DESC(enable_fw_debug, "Enable Firmware debug");
 static struct acp_quirk_entry quirk_valve_galileo = {
 	.signed_fw_image = true,
 	.skip_iram_dram_size_mod = true,
+	.post_fw_run_delay = true,
 };
 
 const struct dmi_system_id acp_sof_quirk_table[] = {
diff --git a/sound/soc/sof/amd/acp.h b/sound/soc/sof/amd/acp.h
index 800594440f73914e7b8ccaf86369ac686e1da630..2a19d82d6200223cdfccd49fbcf1b52968ae1230 100644
--- a/sound/soc/sof/amd/acp.h
+++ b/sound/soc/sof/amd/acp.h
@@ -220,6 +220,7 @@ struct sof_amd_acp_desc {
 struct acp_quirk_entry {
 	bool signed_fw_image;
 	bool skip_iram_dram_size_mod;
+	bool post_fw_run_delay;
 };
 
 /* Common device data struct for ACP devices */
diff --git a/sound/soc/sof/amd/vangogh.c b/sound/soc/sof/amd/vangogh.c
index 8e2672106ac60a22824836a944503a05616f8661..d5f1dddd43e72dd62b3d031130193e6125edf9df 100644
--- a/sound/soc/sof/amd/vangogh.c
+++ b/sound/soc/sof/amd/vangogh.c
@@ -11,6 +11,7 @@
  * Hardware interface for Audio DSP on Vangogh platform
  */
 
+#include <linux/delay.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
 
@@ -136,6 +137,20 @@ static struct snd_soc_dai_driver vangogh_sof_dai[] = {
 	},
 };
 
+static int sof_vangogh_post_fw_run_delay(struct snd_sof_dev *sdev)
+{
+	/*
+	 * Resuming from suspend in some cases my cause the DSP firmware
+	 * to enter an unrecoverable faulty state.  Delaying a bit any host
+	 * to DSP transmission right after firmware boot completion seems
+	 * to resolve the issue.
+	 */
+	if (!sdev->first_boot)
+		usleep_range(100, 150);
+
+	return 0;
+}
+
 /* Vangogh ops */
 struct snd_sof_dsp_ops sof_vangogh_ops;
 EXPORT_SYMBOL_NS(sof_vangogh_ops, "SND_SOC_SOF_AMD_COMMON");
@@ -157,6 +172,9 @@ int sof_vangogh_ops_init(struct snd_sof_dev *sdev)
 
 		if (quirks->signed_fw_image)
 			sof_vangogh_ops.load_firmware = acp_sof_load_signed_firmware;
+
+		if (quirks->post_fw_run_delay)
+			sof_vangogh_ops.post_fw_run = sof_vangogh_post_fw_run_delay;
 	}
 
 	return 0;

-- 
2.48.1

[PATCH 2/4] ASoC: SOF: amd: Drop unused includes from Vangogh driver

[Cristian Ciocaltea]

Remove all the includes for headers which are not (directly) used from
the Vangogh SOF driver sources.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/sof/amd/pci-vangogh.c | 2 --
 sound/soc/sof/amd/vangogh.c     | 4 ----
 2 files changed, 6 deletions(-)

diff --git a/sound/soc/sof/amd/pci-vangogh.c b/sound/soc/sof/amd/pci-vangogh.c
index 53f64d6bc91bae9e2ec506384a83b209a296860f..28f2d4050a6769335b5908c16946eff85aa7161d 100644
--- a/sound/soc/sof/amd/pci-vangogh.c
+++ b/sound/soc/sof/amd/pci-vangogh.c
@@ -13,11 +13,9 @@
 
 #include <linux/module.h>
 #include <linux/pci.h>
-#include <linux/platform_device.h>
 #include <sound/sof.h>
 #include <sound/soc-acpi.h>
 
-#include "../ops.h"
 #include "../sof-pci-dev.h"
 #include "../../amd/mach-config.h"
 #include "acp.h"
diff --git a/sound/soc/sof/amd/vangogh.c b/sound/soc/sof/amd/vangogh.c
index d5f1dddd43e72dd62b3d031130193e6125edf9df..6ed5f9aaa414e696b0ae9b3bb83bd92a5dcd8d4b 100644
--- a/sound/soc/sof/amd/vangogh.c
+++ b/sound/soc/sof/amd/vangogh.c
@@ -12,13 +12,9 @@
  */
 
 #include <linux/delay.h>
-#include <linux/platform_device.h>
 #include <linux/module.h>
 
-#include "../ops.h"
-#include "../sof-audio.h"
 #include "acp.h"
-#include "acp-dsp-offset.h"
 
 #define I2S_HS_INSTANCE		0
 #define I2S_BT_INSTANCE		1

-- 
2.48.1

[PATCH 3/4] ASoC: SOF: amd: Handle IPC replies before FW_BOOT_COMPLETE

[Cristian Ciocaltea]

In some cases, e.g. during resuming from suspend, there is a possibility
that some IPC reply messages get received by the host while the DSP
firmware has not yet reached the complete boot state.

Detect when this happens and do not attempt to process the unexpected
replies from DSP.  Instead, provide proper debugging support.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/sof/amd/acp-ipc.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/sound/soc/sof/amd/acp-ipc.c b/sound/soc/sof/amd/acp-ipc.c
index 5f371d9263f3bad507236ace95b7ef323c369187..12caefd08788595be8de03a863b88b5bbc15847d 100644
--- a/sound/soc/sof/amd/acp-ipc.c
+++ b/sound/soc/sof/amd/acp-ipc.c
@@ -167,6 +167,7 @@ irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context)
 
 	if (sdev->first_boot && sdev->fw_state != SOF_FW_BOOT_COMPLETE) {
 		acp_mailbox_read(sdev, sdev->dsp_box.offset, &status, sizeof(status));
+
 		if ((status & SOF_IPC_PANIC_MAGIC_MASK) == SOF_IPC_PANIC_MAGIC) {
 			snd_sof_dsp_panic(sdev, sdev->dsp_box.offset + sizeof(status),
 					  true);
@@ -188,13 +189,21 @@ irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context)
 
 	dsp_ack = snd_sof_dsp_read(sdev, ACP_DSP_BAR, ACP_SCRATCH_REG_0 + dsp_ack_write);
 	if (dsp_ack) {
-		spin_lock_irq(&sdev->ipc_lock);
-		/* handle immediate reply from DSP core */
-		acp_dsp_ipc_get_reply(sdev);
-		snd_sof_ipc_reply(sdev, 0);
-		/* set the done bit */
-		acp_dsp_ipc_dsp_done(sdev);
-		spin_unlock_irq(&sdev->ipc_lock);
+		if (likely(sdev->fw_state == SOF_FW_BOOT_COMPLETE)) {
+			spin_lock_irq(&sdev->ipc_lock);
+
+			/* handle immediate reply from DSP core */
+			acp_dsp_ipc_get_reply(sdev);
+			snd_sof_ipc_reply(sdev, 0);
+			/* set the done bit */
+			acp_dsp_ipc_dsp_done(sdev);
+
+			spin_unlock_irq(&sdev->ipc_lock);
+		} else {
+			dev_dbg_ratelimited(sdev->dev, "IPC reply before FW_BOOT_COMPLETE: %#x\n",
+					    dsp_ack);
+		}
+
 		ipc_irq = true;
 	}
 

-- 
2.48.1

[PATCH 4/4] ASoC: SOF: amd: Add branch prediction hint in ACP IRQ handler

[Cristian Ciocaltea]

The conditional involving sdev->first_boot in acp_sof_ipc_irq_thread()
will succeed only once, i.e. during the very first run of the
DSP firmware.

Use the unlikely() annotation to help improve branch prediction
accuracy.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/sof/amd/acp-ipc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/sof/amd/acp-ipc.c b/sound/soc/sof/amd/acp-ipc.c
index 12caefd08788595be8de03a863b88b5bbc15847d..22d4b807e1bb75e6f4e6dbf161d79b1a43808004 100644
--- a/sound/soc/sof/amd/acp-ipc.c
+++ b/sound/soc/sof/amd/acp-ipc.c
@@ -165,7 +165,7 @@ irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context)
 	int dsp_msg, dsp_ack;
 	unsigned int status;
 
-	if (sdev->first_boot && sdev->fw_state != SOF_FW_BOOT_COMPLETE) {
+	if (unlikely(sdev->first_boot && sdev->fw_state != SOF_FW_BOOT_COMPLETE)) {
 		acp_mailbox_read(sdev, sdev->dsp_box.offset, &status, sizeof(status));
 
 		if ((status & SOF_IPC_PANIC_MAGIC_MASK) == SOF_IPC_PANIC_MAGIC) {

-- 
2.48.1

Re: [PATCH 3/4] ASoC: SOF: amd: Handle IPC replies before FW_BOOT_COMPLETE

[Mukunda,Vijendar]

On 07/02/25 17:16, Cristian Ciocaltea wrote:
> In some cases, e.g. during resuming from suspend, there is a possibility
> that some IPC reply messages get received by the host while the DSP
> firmware has not yet reached the complete boot state.
>
> Detect when this happens and do not attempt to process the unexpected
> replies from DSP.  Instead, provide proper debugging support.
As per our understanding, before FW boot completion there won't
be any IPC responses sent from Firmware.
In this case, do we really need such a condition check?
>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  sound/soc/sof/amd/acp-ipc.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/sound/soc/sof/amd/acp-ipc.c b/sound/soc/sof/amd/acp-ipc.c
> index 5f371d9263f3bad507236ace95b7ef323c369187..12caefd08788595be8de03a863b88b5bbc15847d 100644
> --- a/sound/soc/sof/amd/acp-ipc.c
> +++ b/sound/soc/sof/amd/acp-ipc.c
> @@ -167,6 +167,7 @@ irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context)
>  
>  	if (sdev->first_boot && sdev->fw_state != SOF_FW_BOOT_COMPLETE) {
>  		acp_mailbox_read(sdev, sdev->dsp_box.offset, &status, sizeof(status));
> +
>  		if ((status & SOF_IPC_PANIC_MAGIC_MASK) == SOF_IPC_PANIC_MAGIC) {
>  			snd_sof_dsp_panic(sdev, sdev->dsp_box.offset + sizeof(status),
>  					  true);
> @@ -188,13 +189,21 @@ irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context)
>  
>  	dsp_ack = snd_sof_dsp_read(sdev, ACP_DSP_BAR, ACP_SCRATCH_REG_0 + dsp_ack_write);
>  	if (dsp_ack) {
> -		spin_lock_irq(&sdev->ipc_lock);
> -		/* handle immediate reply from DSP core */
> -		acp_dsp_ipc_get_reply(sdev);
> -		snd_sof_ipc_reply(sdev, 0);
> -		/* set the done bit */
> -		acp_dsp_ipc_dsp_done(sdev);
> -		spin_unlock_irq(&sdev->ipc_lock);
> +		if (likely(sdev->fw_state == SOF_FW_BOOT_COMPLETE)) {
> +			spin_lock_irq(&sdev->ipc_lock);
> +
> +			/* handle immediate reply from DSP core */
> +			acp_dsp_ipc_get_reply(sdev);
> +			snd_sof_ipc_reply(sdev, 0);
> +			/* set the done bit */
> +			acp_dsp_ipc_dsp_done(sdev);
> +
> +			spin_unlock_irq(&sdev->ipc_lock);
> +		} else {
> +			dev_dbg_ratelimited(sdev->dev, "IPC reply before FW_BOOT_COMPLETE: %#x\n",
> +					    dsp_ack);
> +		}
> +
>  		ipc_irq = true;
>  	}
>  
>

Re: [PATCH 3/4] ASoC: SOF: amd: Handle IPC replies before FW_BOOT_COMPLETE

[Cristian Ciocaltea]

On 2/7/25 1:55 PM, Mukunda,Vijendar wrote:
> On 07/02/25 17:16, Cristian Ciocaltea wrote:
>> In some cases, e.g. during resuming from suspend, there is a possibility
>> that some IPC reply messages get received by the host while the DSP
>> firmware has not yet reached the complete boot state.
>>
>> Detect when this happens and do not attempt to process the unexpected
>> replies from DSP.  Instead, provide proper debugging support.
> As per our understanding, before FW boot completion there won't
> be any IPC responses sent from Firmware.
> In this case, do we really need such a condition check?

During the suspend/resume stress testing I was able to get this kind of
messages, and that's the actual reason for introducing the verification.

Also it doesn't seem to be uncommon, e.g. Intel HDA IPC also provides
similar checks.

>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  sound/soc/sof/amd/acp-ipc.c | 23 ++++++++++++++++-------
>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/sound/soc/sof/amd/acp-ipc.c b/sound/soc/sof/amd/acp-ipc.c
>> index 5f371d9263f3bad507236ace95b7ef323c369187..12caefd08788595be8de03a863b88b5bbc15847d 100644
>> --- a/sound/soc/sof/amd/acp-ipc.c
>> +++ b/sound/soc/sof/amd/acp-ipc.c
>> @@ -167,6 +167,7 @@ irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context)
>>  
>>  	if (sdev->first_boot && sdev->fw_state != SOF_FW_BOOT_COMPLETE) {
>>  		acp_mailbox_read(sdev, sdev->dsp_box.offset, &status, sizeof(status));
>> +
>>  		if ((status & SOF_IPC_PANIC_MAGIC_MASK) == SOF_IPC_PANIC_MAGIC) {
>>  			snd_sof_dsp_panic(sdev, sdev->dsp_box.offset + sizeof(status),
>>  					  true);
>> @@ -188,13 +189,21 @@ irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context)
>>  
>>  	dsp_ack = snd_sof_dsp_read(sdev, ACP_DSP_BAR, ACP_SCRATCH_REG_0 + dsp_ack_write);
>>  	if (dsp_ack) {
>> -		spin_lock_irq(&sdev->ipc_lock);
>> -		/* handle immediate reply from DSP core */
>> -		acp_dsp_ipc_get_reply(sdev);
>> -		snd_sof_ipc_reply(sdev, 0);
>> -		/* set the done bit */
>> -		acp_dsp_ipc_dsp_done(sdev);
>> -		spin_unlock_irq(&sdev->ipc_lock);
>> +		if (likely(sdev->fw_state == SOF_FW_BOOT_COMPLETE)) {
>> +			spin_lock_irq(&sdev->ipc_lock);
>> +
>> +			/* handle immediate reply from DSP core */
>> +			acp_dsp_ipc_get_reply(sdev);
>> +			snd_sof_ipc_reply(sdev, 0);
>> +			/* set the done bit */
>> +			acp_dsp_ipc_dsp_done(sdev);
>> +
>> +			spin_unlock_irq(&sdev->ipc_lock);
>> +		} else {
>> +			dev_dbg_ratelimited(sdev->dev, "IPC reply before FW_BOOT_COMPLETE: %#x\n",
>> +					    dsp_ack);
>> +		}
>> +
>>  		ipc_irq = true;
>>  	}
>>  
>>
> 

Re: [PATCH 3/4] ASoC: SOF: amd: Handle IPC replies before FW_BOOT_COMPLETE

[Mukunda,Vijendar]

On 07/02/25 17:46, Cristian Ciocaltea wrote:
> On 2/7/25 1:55 PM, Mukunda,Vijendar wrote:
>> On 07/02/25 17:16, Cristian Ciocaltea wrote:
>>> In some cases, e.g. during resuming from suspend, there is a possibility
>>> that some IPC reply messages get received by the host while the DSP
>>> firmware has not yet reached the complete boot state.
>>>
>>> Detect when this happens and do not attempt to process the unexpected
>>> replies from DSP.  Instead, provide proper debugging support.
>> As per our understanding, before FW boot completion there won't
>> be any IPC responses sent from Firmware.
>> In this case, do we really need such a condition check?
> During the suspend/resume stress testing I was able to get this kind of
> messages, and that's the actual reason for introducing the verification.
>
> Also it doesn't seem to be uncommon, e.g. Intel HDA IPC also provides
> similar checks.
>
Could you please share reference logs to know which IPC messages
are being received before FW_READY message/FW boot complete?
>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>> ---
>>>  sound/soc/sof/amd/acp-ipc.c | 23 ++++++++++++++++-------
>>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/sound/soc/sof/amd/acp-ipc.c b/sound/soc/sof/amd/acp-ipc.c
>>> index 5f371d9263f3bad507236ace95b7ef323c369187..12caefd08788595be8de03a863b88b5bbc15847d 100644
>>> --- a/sound/soc/sof/amd/acp-ipc.c
>>> +++ b/sound/soc/sof/amd/acp-ipc.c
>>> @@ -167,6 +167,7 @@ irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context)
>>>  
>>>  	if (sdev->first_boot && sdev->fw_state != SOF_FW_BOOT_COMPLETE) {
>>>  		acp_mailbox_read(sdev, sdev->dsp_box.offset, &status, sizeof(status));
>>> +
>>>  		if ((status & SOF_IPC_PANIC_MAGIC_MASK) == SOF_IPC_PANIC_MAGIC) {
>>>  			snd_sof_dsp_panic(sdev, sdev->dsp_box.offset + sizeof(status),
>>>  					  true);
>>> @@ -188,13 +189,21 @@ irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context)
>>>  
>>>  	dsp_ack = snd_sof_dsp_read(sdev, ACP_DSP_BAR, ACP_SCRATCH_REG_0 + dsp_ack_write);
>>>  	if (dsp_ack) {
>>> -		spin_lock_irq(&sdev->ipc_lock);
>>> -		/* handle immediate reply from DSP core */
>>> -		acp_dsp_ipc_get_reply(sdev);
>>> -		snd_sof_ipc_reply(sdev, 0);
>>> -		/* set the done bit */
>>> -		acp_dsp_ipc_dsp_done(sdev);
>>> -		spin_unlock_irq(&sdev->ipc_lock);
>>> +		if (likely(sdev->fw_state == SOF_FW_BOOT_COMPLETE)) {
>>> +			spin_lock_irq(&sdev->ipc_lock);
>>> +
>>> +			/* handle immediate reply from DSP core */
>>> +			acp_dsp_ipc_get_reply(sdev);
>>> +			snd_sof_ipc_reply(sdev, 0);
>>> +			/* set the done bit */
>>> +			acp_dsp_ipc_dsp_done(sdev);
>>> +
>>> +			spin_unlock_irq(&sdev->ipc_lock);
>>> +		} else {
>>> +			dev_dbg_ratelimited(sdev->dev, "IPC reply before FW_BOOT_COMPLETE: %#x\n",
>>> +					    dsp_ack);
>>> +		}
>>> +
>>>  		ipc_irq = true;
>>>  	}
>>>  
>>>

Re: [PATCH 1/4] ASoC: SOF: amd: Add post_fw_run_delay ACP quirk

[Mukunda,Vijendar]

On 07/02/25 17:16, Cristian Ciocaltea wrote:
> Stress testing resume from suspend on Valve Steam Deck OLED (Galileo)
> revealed that the DSP firmware could enter an unrecoverable faulty
> state, where the kernel ring buffer is flooded with IPC related error
> messages:
>
> [  +0.017002] snd_sof_amd_vangogh 0000:04:00.5: acp_sof_ipc_send_msg: Failed to acquire HW lock
> [  +0.000054] snd_sof_amd_vangogh 0000:04:00.5: ipc3_tx_msg_unlocked: ipc message send for 0x30100000 failed: -22
> [  +0.000005] snd_sof_amd_vangogh 0000:04:00.5: Failed to setup widget PIPELINE.6.ACPHS1.IN
> [  +0.000004] snd_sof_amd_vangogh 0000:04:00.5: Failed to restore pipeline after resume -22
> [  +0.000003] snd_sof_amd_vangogh 0000:04:00.5: PM: dpm_run_callback(): pci_pm_resume returns -22
> [  +0.000009] snd_sof_amd_vangogh 0000:04:00.5: PM: failed to resume async: error -22
> [...]
> [  +0.002582] PM: suspend exit
> [  +0.065085] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for 0x30130000 (msg/reply size: 12/0): -22
> [  +0.000499] snd_sof_amd_vangogh 0000:04:00.5: error: failed widget list set up for pcm 1 dir 0
> [  +0.000011] snd_sof_amd_vangogh 0000:04:00.5: error: set pcm hw_params after resume
> [  +0.000006] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_pcm_component_prepare on 0000:04:00.5: -22
> [...]
>
> A system reboot would be necessary to restore the speakers
> functionality.
>
> However, by delaying a bit any host to DSP transmission right after
> the firmware boot completed, the issue could not be reproduced anymore
> and sound continued to work flawlessly even after performing thousands
> of suspend/resume cycles.
>
> Introduce the post_fw_run_delay ACP quirk to allow providing the
> aforementioned delay via the snd_sof_dsp_ops->post_fw_run() callback for
> the affected devices.
As per our understanding, Till ACP firmware starts responding to the
driver's IPC messages, FW won't acquire the HW lock.
Adding delay in post_fw_run() callback seems to not a correct solution,
as there are no IPC messages will be sent from host till FW boot ready
is received.
Is fail to acquire the HW lock is for first host IPC message?

>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  sound/soc/sof/amd/acp.c     |  1 +
>  sound/soc/sof/amd/acp.h     |  1 +
>  sound/soc/sof/amd/vangogh.c | 18 ++++++++++++++++++
>  3 files changed, 20 insertions(+)
>
> diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
> index 33648ff8b83365e76d7d90e52c2cb8f884a2fe72..9e13c96528be3371e063072513c118cfc8b93fe8 100644
> --- a/sound/soc/sof/amd/acp.c
> +++ b/sound/soc/sof/amd/acp.c
> @@ -27,6 +27,7 @@ MODULE_PARM_DESC(enable_fw_debug, "Enable Firmware debug");
>  static struct acp_quirk_entry quirk_valve_galileo = {
>  	.signed_fw_image = true,
>  	.skip_iram_dram_size_mod = true,
> +	.post_fw_run_delay = true,
>  };
>  
>  const struct dmi_system_id acp_sof_quirk_table[] = {
> diff --git a/sound/soc/sof/amd/acp.h b/sound/soc/sof/amd/acp.h
> index 800594440f73914e7b8ccaf86369ac686e1da630..2a19d82d6200223cdfccd49fbcf1b52968ae1230 100644
> --- a/sound/soc/sof/amd/acp.h
> +++ b/sound/soc/sof/amd/acp.h
> @@ -220,6 +220,7 @@ struct sof_amd_acp_desc {
>  struct acp_quirk_entry {
>  	bool signed_fw_image;
>  	bool skip_iram_dram_size_mod;
> +	bool post_fw_run_delay;
>  };
>  
>  /* Common device data struct for ACP devices */
> diff --git a/sound/soc/sof/amd/vangogh.c b/sound/soc/sof/amd/vangogh.c
> index 8e2672106ac60a22824836a944503a05616f8661..d5f1dddd43e72dd62b3d031130193e6125edf9df 100644
> --- a/sound/soc/sof/amd/vangogh.c
> +++ b/sound/soc/sof/amd/vangogh.c
> @@ -11,6 +11,7 @@
>   * Hardware interface for Audio DSP on Vangogh platform
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  
> @@ -136,6 +137,20 @@ static struct snd_soc_dai_driver vangogh_sof_dai[] = {
>  	},
>  };
>  
> +static int sof_vangogh_post_fw_run_delay(struct snd_sof_dev *sdev)
> +{
> +	/*
> +	 * Resuming from suspend in some cases my cause the DSP firmware
> +	 * to enter an unrecoverable faulty state.  Delaying a bit any host
> +	 * to DSP transmission right after firmware boot completion seems
> +	 * to resolve the issue.
> +	 */
> +	if (!sdev->first_boot)
> +		usleep_range(100, 150);
> +
> +	return 0;
> +}
> +
>  /* Vangogh ops */
>  struct snd_sof_dsp_ops sof_vangogh_ops;
>  EXPORT_SYMBOL_NS(sof_vangogh_ops, "SND_SOC_SOF_AMD_COMMON");
> @@ -157,6 +172,9 @@ int sof_vangogh_ops_init(struct snd_sof_dev *sdev)
>  
>  		if (quirks->signed_fw_image)
>  			sof_vangogh_ops.load_firmware = acp_sof_load_signed_firmware;
> +
> +		if (quirks->post_fw_run_delay)
> +			sof_vangogh_ops.post_fw_run = sof_vangogh_post_fw_run_delay;
>  	}
>  
>  	return 0;
>

Re: [PATCH 1/4] ASoC: SOF: amd: Add post_fw_run_delay ACP quirk

[Cristian Ciocaltea]

On 2/7/25 2:35 PM, Mukunda,Vijendar wrote:
> On 07/02/25 17:16, Cristian Ciocaltea wrote:
>> Stress testing resume from suspend on Valve Steam Deck OLED (Galileo)
>> revealed that the DSP firmware could enter an unrecoverable faulty
>> state, where the kernel ring buffer is flooded with IPC related error
>> messages:
>>
>> [  +0.017002] snd_sof_amd_vangogh 0000:04:00.5: acp_sof_ipc_send_msg: Failed to acquire HW lock
>> [  +0.000054] snd_sof_amd_vangogh 0000:04:00.5: ipc3_tx_msg_unlocked: ipc message send for 0x30100000 failed: -22
>> [  +0.000005] snd_sof_amd_vangogh 0000:04:00.5: Failed to setup widget PIPELINE.6.ACPHS1.IN
>> [  +0.000004] snd_sof_amd_vangogh 0000:04:00.5: Failed to restore pipeline after resume -22
>> [  +0.000003] snd_sof_amd_vangogh 0000:04:00.5: PM: dpm_run_callback(): pci_pm_resume returns -22
>> [  +0.000009] snd_sof_amd_vangogh 0000:04:00.5: PM: failed to resume async: error -22
>> [...]
>> [  +0.002582] PM: suspend exit
>> [  +0.065085] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for 0x30130000 (msg/reply size: 12/0): -22
>> [  +0.000499] snd_sof_amd_vangogh 0000:04:00.5: error: failed widget list set up for pcm 1 dir 0
>> [  +0.000011] snd_sof_amd_vangogh 0000:04:00.5: error: set pcm hw_params after resume
>> [  +0.000006] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_pcm_component_prepare on 0000:04:00.5: -22
>> [...]
>>
>> A system reboot would be necessary to restore the speakers
>> functionality.
>>
>> However, by delaying a bit any host to DSP transmission right after
>> the firmware boot completed, the issue could not be reproduced anymore
>> and sound continued to work flawlessly even after performing thousands
>> of suspend/resume cycles.
>>
>> Introduce the post_fw_run_delay ACP quirk to allow providing the
>> aforementioned delay via the snd_sof_dsp_ops->post_fw_run() callback for
>> the affected devices.
> As per our understanding, Till ACP firmware starts responding to the
> driver's IPC messages, FW won't acquire the HW lock.
> Adding delay in post_fw_run() callback seems to not a correct solution,
> as there are no IPC messages will be sent from host till FW boot ready
> is received.

I suspect there is some sort of a race condition or hardware fault that
causes the DSP to become unresponsive if the host attempts to send
messages too fast after the firmware boot completed.  Delaying the
transmission by 100us proved to be a reliable workaround (going below
10us was not enough).

Please note this was not always easy to reproduce, e.g. in some cases I
had to run hundreds of suspend/resume cycles, hence I think it really
points to a timing issue.

Moreover, as soon as I tried to enable SOF debugging, the issue was
gone, no matter how hard I tried to reproduce (ran thousands of cycles).

> Is fail to acquire the HW lock is for first host IPC message?

That seemed to be the pattern, yes.

>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  sound/soc/sof/amd/acp.c     |  1 +
>>  sound/soc/sof/amd/acp.h     |  1 +
>>  sound/soc/sof/amd/vangogh.c | 18 ++++++++++++++++++
>>  3 files changed, 20 insertions(+)
>>
>> diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
>> index 33648ff8b83365e76d7d90e52c2cb8f884a2fe72..9e13c96528be3371e063072513c118cfc8b93fe8 100644
>> --- a/sound/soc/sof/amd/acp.c
>> +++ b/sound/soc/sof/amd/acp.c
>> @@ -27,6 +27,7 @@ MODULE_PARM_DESC(enable_fw_debug, "Enable Firmware debug");
>>  static struct acp_quirk_entry quirk_valve_galileo = {
>>  	.signed_fw_image = true,
>>  	.skip_iram_dram_size_mod = true,
>> +	.post_fw_run_delay = true,
>>  };
>>  
>>  const struct dmi_system_id acp_sof_quirk_table[] = {
>> diff --git a/sound/soc/sof/amd/acp.h b/sound/soc/sof/amd/acp.h
>> index 800594440f73914e7b8ccaf86369ac686e1da630..2a19d82d6200223cdfccd49fbcf1b52968ae1230 100644
>> --- a/sound/soc/sof/amd/acp.h
>> +++ b/sound/soc/sof/amd/acp.h
>> @@ -220,6 +220,7 @@ struct sof_amd_acp_desc {
>>  struct acp_quirk_entry {
>>  	bool signed_fw_image;
>>  	bool skip_iram_dram_size_mod;
>> +	bool post_fw_run_delay;
>>  };
>>  
>>  /* Common device data struct for ACP devices */
>> diff --git a/sound/soc/sof/amd/vangogh.c b/sound/soc/sof/amd/vangogh.c
>> index 8e2672106ac60a22824836a944503a05616f8661..d5f1dddd43e72dd62b3d031130193e6125edf9df 100644
>> --- a/sound/soc/sof/amd/vangogh.c
>> +++ b/sound/soc/sof/amd/vangogh.c
>> @@ -11,6 +11,7 @@
>>   * Hardware interface for Audio DSP on Vangogh platform
>>   */
>>  
>> +#include <linux/delay.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/module.h>
>>  
>> @@ -136,6 +137,20 @@ static struct snd_soc_dai_driver vangogh_sof_dai[] = {
>>  	},
>>  };
>>  
>> +static int sof_vangogh_post_fw_run_delay(struct snd_sof_dev *sdev)
>> +{
>> +	/*
>> +	 * Resuming from suspend in some cases my cause the DSP firmware
>> +	 * to enter an unrecoverable faulty state.  Delaying a bit any host
>> +	 * to DSP transmission right after firmware boot completion seems
>> +	 * to resolve the issue.
>> +	 */
>> +	if (!sdev->first_boot)
>> +		usleep_range(100, 150);
>> +
>> +	return 0;
>> +}
>> +
>>  /* Vangogh ops */
>>  struct snd_sof_dsp_ops sof_vangogh_ops;
>>  EXPORT_SYMBOL_NS(sof_vangogh_ops, "SND_SOC_SOF_AMD_COMMON");
>> @@ -157,6 +172,9 @@ int sof_vangogh_ops_init(struct snd_sof_dev *sdev)
>>  
>>  		if (quirks->signed_fw_image)
>>  			sof_vangogh_ops.load_firmware = acp_sof_load_signed_firmware;
>> +
>> +		if (quirks->post_fw_run_delay)
>> +			sof_vangogh_ops.post_fw_run = sof_vangogh_post_fw_run_delay;
>>  	}
>>  
>>  	return 0;
>>
> 

Re: [PATCH 3/4] ASoC: SOF: amd: Handle IPC replies before FW_BOOT_COMPLETE

[Cristian Ciocaltea]

On 2/7/25 2:24 PM, Mukunda,Vijendar wrote:
> On 07/02/25 17:46, Cristian Ciocaltea wrote:
>> On 2/7/25 1:55 PM, Mukunda,Vijendar wrote:
>>> On 07/02/25 17:16, Cristian Ciocaltea wrote:
>>>> In some cases, e.g. during resuming from suspend, there is a possibility
>>>> that some IPC reply messages get received by the host while the DSP
>>>> firmware has not yet reached the complete boot state.
>>>>
>>>> Detect when this happens and do not attempt to process the unexpected
>>>> replies from DSP.  Instead, provide proper debugging support.
>>> As per our understanding, before FW boot completion there won't
>>> be any IPC responses sent from Firmware.
>>> In this case, do we really need such a condition check?
>> During the suspend/resume stress testing I was able to get this kind of
>> messages, and that's the actual reason for introducing the verification.
>>
>> Also it doesn't seem to be uncommon, e.g. Intel HDA IPC also provides
>> similar checks.
>>
> Could you please share reference logs to know which IPC messages
> are being received before FW_READY message/FW boot complete?

As mentioned in a previous reply, I couldn't enable debugging during
stress testing because it had the annoying effect of hiding the audio
breakage issue.  I will try to experiment with some more targeted
instrumentation and see if it's possible to extract some useful data.

Regardless, I think it's worth keeping this check in place as it's
definitely helpful to spot potential FW issues (for current or future
products).

>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>> ---
>>>>  sound/soc/sof/amd/acp-ipc.c | 23 ++++++++++++++++-------
>>>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/sound/soc/sof/amd/acp-ipc.c b/sound/soc/sof/amd/acp-ipc.c
>>>> index 5f371d9263f3bad507236ace95b7ef323c369187..12caefd08788595be8de03a863b88b5bbc15847d 100644
>>>> --- a/sound/soc/sof/amd/acp-ipc.c
>>>> +++ b/sound/soc/sof/amd/acp-ipc.c
>>>> @@ -167,6 +167,7 @@ irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context)
>>>>  
>>>>  	if (sdev->first_boot && sdev->fw_state != SOF_FW_BOOT_COMPLETE) {
>>>>  		acp_mailbox_read(sdev, sdev->dsp_box.offset, &status, sizeof(status));
>>>> +
>>>>  		if ((status & SOF_IPC_PANIC_MAGIC_MASK) == SOF_IPC_PANIC_MAGIC) {
>>>>  			snd_sof_dsp_panic(sdev, sdev->dsp_box.offset + sizeof(status),
>>>>  					  true);
>>>> @@ -188,13 +189,21 @@ irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context)
>>>>  
>>>>  	dsp_ack = snd_sof_dsp_read(sdev, ACP_DSP_BAR, ACP_SCRATCH_REG_0 + dsp_ack_write);
>>>>  	if (dsp_ack) {
>>>> -		spin_lock_irq(&sdev->ipc_lock);
>>>> -		/* handle immediate reply from DSP core */
>>>> -		acp_dsp_ipc_get_reply(sdev);
>>>> -		snd_sof_ipc_reply(sdev, 0);
>>>> -		/* set the done bit */
>>>> -		acp_dsp_ipc_dsp_done(sdev);
>>>> -		spin_unlock_irq(&sdev->ipc_lock);
>>>> +		if (likely(sdev->fw_state == SOF_FW_BOOT_COMPLETE)) {
>>>> +			spin_lock_irq(&sdev->ipc_lock);
>>>> +
>>>> +			/* handle immediate reply from DSP core */
>>>> +			acp_dsp_ipc_get_reply(sdev);
>>>> +			snd_sof_ipc_reply(sdev, 0);
>>>> +			/* set the done bit */
>>>> +			acp_dsp_ipc_dsp_done(sdev);
>>>> +
>>>> +			spin_unlock_irq(&sdev->ipc_lock);
>>>> +		} else {
>>>> +			dev_dbg_ratelimited(sdev->dev, "IPC reply before FW_BOOT_COMPLETE: %#x\n",
>>>> +					    dsp_ack);
>>>> +		}
>>>> +
>>>>  		ipc_irq = true;
>>>>  	}
>>>>  
>>>>
> 

Re: [PATCH 2/4] ASoC: SOF: amd: Drop unused includes from Vangogh driver

[potturu venkata prasad]

On 2/7/25 17:16, Cristian Ciocaltea wrote:
> Remove all the includes for headers which are not (directly) used from
> the Vangogh SOF driver sources.

Okay.

Reviewed-by: Venkata Prasad Potturu <venkataprasad.potturu@amd.com>

>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>   sound/soc/sof/amd/pci-vangogh.c | 2 --
>   sound/soc/sof/amd/vangogh.c     | 4 ----
>   2 files changed, 6 deletions(-)
>
> diff --git a/sound/soc/sof/amd/pci-vangogh.c b/sound/soc/sof/amd/pci-vangogh.c
> index 53f64d6bc91bae9e2ec506384a83b209a296860f..28f2d4050a6769335b5908c16946eff85aa7161d 100644
> --- a/sound/soc/sof/amd/pci-vangogh.c
> +++ b/sound/soc/sof/amd/pci-vangogh.c
> @@ -13,11 +13,9 @@
>   
>   #include <linux/module.h>
>   #include <linux/pci.h>
> -#include <linux/platform_device.h>
>   #include <sound/sof.h>
>   #include <sound/soc-acpi.h>
>   
> -#include "../ops.h"
>   #include "../sof-pci-dev.h"
>   #include "../../amd/mach-config.h"
>   #include "acp.h"
> diff --git a/sound/soc/sof/amd/vangogh.c b/sound/soc/sof/amd/vangogh.c
> index d5f1dddd43e72dd62b3d031130193e6125edf9df..6ed5f9aaa414e696b0ae9b3bb83bd92a5dcd8d4b 100644
> --- a/sound/soc/sof/amd/vangogh.c
> +++ b/sound/soc/sof/amd/vangogh.c
> @@ -12,13 +12,9 @@
>    */
>   
>   #include <linux/delay.h>
> -#include <linux/platform_device.h>
>   #include <linux/module.h>
>   
> -#include "../ops.h"
> -#include "../sof-audio.h"
>   #include "acp.h"
> -#include "acp-dsp-offset.h"
>   
>   #define I2S_HS_INSTANCE		0
>   #define I2S_BT_INSTANCE		1
>

Re: [PATCH 4/4] ASoC: SOF: amd: Add branch prediction hint in ACP IRQ handler

[potturu venkata prasad]

On 2/7/25 17:16, Cristian Ciocaltea wrote:
> The conditional involving sdev->first_boot in acp_sof_ipc_irq_thread()
> will succeed only once, i.e. during the very first run of the
> DSP firmware.
>
> Use the unlikely() annotation to help improve branch prediction
> accuracy.
Okay.
Reviewed-by: Venkata Prasad Potturu <venkataprasad.potturu@amd.com>
>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>   sound/soc/sof/amd/acp-ipc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/sof/amd/acp-ipc.c b/sound/soc/sof/amd/acp-ipc.c
> index 12caefd08788595be8de03a863b88b5bbc15847d..22d4b807e1bb75e6f4e6dbf161d79b1a43808004 100644
> --- a/sound/soc/sof/amd/acp-ipc.c
> +++ b/sound/soc/sof/amd/acp-ipc.c
> @@ -165,7 +165,7 @@ irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context)
>   	int dsp_msg, dsp_ack;
>   	unsigned int status;
>   
> -	if (sdev->first_boot && sdev->fw_state != SOF_FW_BOOT_COMPLETE) {
> +	if (unlikely(sdev->first_boot && sdev->fw_state != SOF_FW_BOOT_COMPLETE)) {
>   		acp_mailbox_read(sdev, sdev->dsp_box.offset, &status, sizeof(status));
>   
>   		if ((status & SOF_IPC_PANIC_MAGIC_MASK) == SOF_IPC_PANIC_MAGIC) {
>

Re: [PATCH 0/4] Sound fix for Valve Steam Deck OLED on resume from suspend

[Mark Brown]

On Fri, 07 Feb 2025 13:46:01 +0200, Cristian Ciocaltea wrote:
> Stress testing resume from suspend on Valve Steam Deck OLED (Galileo)
> revealed that audio may break and cannot be recovered without performing
> a system reboot.
> 
> This patch series provides a new ACP quirk to address the issue, along
> with a few additional improvements to AMD Vangogh/ACP SOF drivers.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/4] ASoC: SOF: amd: Add post_fw_run_delay ACP quirk
      commit: 91b98d5a6e8067c5226207487681a48f0d651e46
[2/4] ASoC: SOF: amd: Drop unused includes from Vangogh driver
      commit: 2ecbc2e9f3b19e2199e8bc3ba603d299f1985f09
[3/4] ASoC: SOF: amd: Handle IPC replies before FW_BOOT_COMPLETE
      commit: ac84ca815adb4171a4276b1d44096b75f6a150b7
[4/4] ASoC: SOF: amd: Add branch prediction hint in ACP IRQ handler
      commit: ccc8480d90e8cb60f06bd90e227f34784927e19f

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark