ASOC: Intel: sof_sdw: restore playback functionality with max98373 amps #2424

RanderWang · 2020-09-09T08:20:13Z

The Max98373 amplifier provides I/V feedback information, which keeps
a DAPM path active even when there is no playback happening. This
prevents entry in low-power mode. Rather than adding new controls and
require UCM/user interaction, the method previously applied is to
enable/disable the Speaker pin during the dailink trigger operations.

Recent changes in the SoundWire stream management moved the stream
trigger to the dailink trigger. This change removed the Maxim-specific
pin handling and resulted in a regression. This patch restores
functionality by combining the SoundWire stream trigger with the pin
enable/disable.

Fixes: 7eec07f389a60 ('ASOC: Intel: sof_sdw: add dailink .trigger callback')
Fixes: 5595f95c32650 ('ASOC: Intel: sof_sdw: add dailink .prepare and .hw_free callback').

RanderWang · 2020-09-09T08:26:39Z

I checked the PR on TGL RVP but missed volteer. I found this issue in weekly validation later.

sound/soc/intel/boards/sof_sdw.c

plbossart

Sorry, I don't understand why we would use dailink functions for a codec specifically.
Something is not right here.

sound/soc/intel/boards/sof_sdw_max98373.c

plbossart · 2020-09-09T19:03:01Z

sound/soc/intel/boards/sof_sdw_max98373.c

-	.trigger = max98373_trigger,
+	.prepare = sdw_prepare,
+	.trigger = sdw_trigger,
+	.hw_free = sdw_hw_free,


why is this here? This makes no sense to me.

plbossart · 2020-09-09T19:44:15Z

Build fail as well

sound/soc/intel/boards/sof_sdw_max98373.c:58:5: error: no previous prototype for ‘max98373_sdw_trigger’ [-Werror=missing-prototypes]
   58 | int max98373_sdw_trigger(struct snd_pcm_substream *substream, int cmd)
      |     ^~~~~~~~~~~~~~~~~~~~

ranj063 · 2020-09-09T21:40:59Z

@RanderWang what playback issue does this fix? Can you please tag the issue here if one is filed?

plbossart · 2020-09-10T00:28:00Z

I would really recommend checking why we use sdw_startup and sdw_shutdown in the existing code.
I think we need to revert some of the previous patches with the follow diff

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index 210b66d1f9a2..9aeb34ef83c8 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -220,7 +220,7 @@ static struct snd_soc_dai_link_component platform_component[] = {
 };
 
 /* these wrappers are only needed to avoid typecast compilation errors */
-int sdw_startup(struct snd_pcm_substream *substream)
+static int sdw_startup(struct snd_pcm_substream *substream)
 {
        return sdw_startup_stream(substream);
 }
@@ -303,7 +303,7 @@ static int sdw_hw_free(struct snd_pcm_substream *substream)
        return sdw_deprepare_stream(sdw_stream);
 }
 
-void sdw_shutdown(struct snd_pcm_substream *substream)
+static void sdw_shutdown(struct snd_pcm_substream *substream)
 {
        sdw_shutdown_stream(substream);
 }
diff --git a/sound/soc/intel/boards/sof_sdw_common.h b/sound/soc/intel/boards/sof_sdw_common.h
index 6a5d46589baf..911bce2b8336 100644
--- a/sound/soc/intel/boards/sof_sdw_common.h
+++ b/sound/soc/intel/boards/sof_sdw_common.h
@@ -78,9 +78,6 @@ struct mc_private {
 
 extern unsigned long sof_sdw_quirk;
 
-int sdw_startup(struct snd_pcm_substream *substream);
-void sdw_shutdown(struct snd_pcm_substream *substream);
-
 /* generic HDMI support */
 int sof_sdw_hdmi_init(struct snd_soc_pcm_runtime *rtd);
 
diff --git a/sound/soc/intel/boards/sof_sdw_max98373.c b/sound/soc/intel/boards/sof_sdw_max98373.c
index 905582aaf58c..0afe99cf3f2f 100644
--- a/sound/soc/intel/boards/sof_sdw_max98373.c
+++ b/sound/soc/intel/boards/sof_sdw_max98373.c
@@ -56,9 +56,7 @@ static int spk_init(struct snd_soc_pcm_runtime *rtd)
 }
 
 static const struct snd_soc_ops max_98373_sdw_ops = {
-       .startup = sdw_startup,
        .trigger = max98373_trigger,
-       .shutdown = sdw_shutdown,
 };
 
 int sof_sdw_mx8373_init(const struct snd_soc_acpi_link_adr *link,

max98373.diff.txt

RanderWang · 2020-09-10T01:17:33Z

@RanderWang what playback issue does this fix? Can you please tag the issue here if one is filed?

I didn't create a issue. playback can't work on 98373 since the sdw master is never triggered

RanderWang · 2020-09-10T01:28:45Z

I would really recommend checking why we use sdw_startup and sdw_shutdown in the existing code.
I think we need to revert some of the previous patches with the follow diff

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index 210b66d1f9a2..9aeb34ef83c8 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -220,7 +220,7 @@ static struct snd_soc_dai_link_component platform_component[] = {
 };
 
 /* these wrappers are only needed to avoid typecast compilation errors */
-int sdw_startup(struct snd_pcm_substream *substream)
+static int sdw_startup(struct snd_pcm_substream *substream)
 {
        return sdw_startup_stream(substream);
 }
@@ -303,7 +303,7 @@ static int sdw_hw_free(struct snd_pcm_substream *substream)
        return sdw_deprepare_stream(sdw_stream);
 }
 
-void sdw_shutdown(struct snd_pcm_substream *substream)
+static void sdw_shutdown(struct snd_pcm_substream *substream)
 {
        sdw_shutdown_stream(substream);
 }
diff --git a/sound/soc/intel/boards/sof_sdw_common.h b/sound/soc/intel/boards/sof_sdw_common.h
index 6a5d46589baf..911bce2b8336 100644
--- a/sound/soc/intel/boards/sof_sdw_common.h
+++ b/sound/soc/intel/boards/sof_sdw_common.h
@@ -78,9 +78,6 @@ struct mc_private {
 
 }
 
 static const struct snd_soc_ops max_98373_sdw_ops = {
-       .startup = sdw_startup,
        .trigger = max98373_trigger,
-       .shutdown = sdw_shutdown,
 };
 
 int sof_sdw_mx8373_init(const struct snd_soc_acpi_link_adr *link,

max98373.diff.txt

@plbossart dai link ops sdw_ops is set at https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/intel/boards/sof_sdw.c#L810, but in following code https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/intel/boards/sof_sdw.c#L812 --> sof_sdw_mx8373_init will override it with its own ops at https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/intel/boards/sof_sdw_max98373.c#L75. Since sdw_ops is const and the dai->ops is also const, we can't change it dynamically such as changing trigger function only, so I create another dai ops for 98373

RanderWang · 2020-09-10T11:55:52Z

update my PR, thanks for review!

plbossart · 2020-09-10T13:29:43Z

@RanderWang I still don't get why the dailink ops is overridden. Why was this done in the first place? it seems that was a miss in the initial reviews and it's quite ugly architecturally. Blurring concepts like this is not quite right.

RanderWang · 2020-09-11T01:58:40Z

@RanderWang I still don't get why the dailink ops is overridden. Why was this done in the first place? it seems that was a miss in the initial reviews and it's quite ugly architecturally. Blurring concepts like this is not quite right.

First let`s check a6c4e1a. It fixed issue "[TGL] runtime PM always in active on TGL chromebook/I2S mode " (#2053) Then we also have a the same bug on SDW mode #2162. So I ported this commit to sdw platform with #2201

RanderWang · 2020-09-11T02:08:46Z

@RanderWang I still don't get why the dailink ops is overridden. Why was this done in the first place? it seems that was a miss in the initial reviews and it's quite ugly architecturally. Blurring concepts like this is not quite right.

Maybe I misuse "override" on max98373. Before we moved some sdw ops from sdw component driver to machine driver, we didn't set trigger function in our dai link ops. Max98373 used trigger function to enable | disable spk switch pin in dai link ops. This is a corner case. Now we move trigger function to dai link ops so we need to merge two trigger function into to one. I called it "override".

RanderWang · 2020-09-11T07:43:15Z

@plbossart @bardliao I also have another solution: move the trigger function of max98373 to codec driver to enable | disable spk switch. what is your idea ?

bardliao · 2020-09-11T08:00:34Z

@plbossart @bardliao I also have another solution: move the trigger function of max98373 to codec driver to enable | disable spk switch. what is your idea ?

I am not sure if it is a common issue on max98373 codec. I think it will be no issue if "VI Sense Switch" and "SpkFB Sense Switch" controls are off.

plbossart · 2020-09-11T14:57:57Z

@plbossart @bardliao I also have another solution: move the trigger function of max98373 to codec driver to enable | disable spk switch. what is your idea ?

I am not sure if it is a common issue on max98373 codec. I think it will be no issue if "VI Sense Switch" and "SpkFB Sense Switch" controls are off.

I don't think this belongs in the codec driver, this is needed because of the smart amp processing only, no?

plbossart · 2020-09-11T15:01:18Z

@RanderWang I still don't get why the dailink ops is overridden. Why was this done in the first place? it seems that was a miss in the initial reviews and it's quite ugly architecturally. Blurring concepts like this is not quite right.

Maybe I misuse "override" on max98373. Before we moved some sdw ops from sdw component driver to machine driver, we didn't set trigger function in our dai link ops. Max98373 used trigger function to enable | disable spk switch pin in dai link ops. This is a corner case. Now we move trigger function to dai link ops so we need to merge two trigger function into to one. I called it "override".

I don't see why we need to merge. The ASoC core will call the dailink .trigger and then the dai .trigger

static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
{
	int ret = -EINVAL;

	switch (cmd) {
	case SNDRV_PCM_TRIGGER_START:
	case SNDRV_PCM_TRIGGER_RESUME:
	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
		ret = snd_soc_link_trigger(substream, cmd);
		if (ret < 0)
			break;

		ret = snd_soc_pcm_component_trigger(substream, cmd);
		if (ret < 0)
			break;

		ret = snd_soc_pcm_dai_trigger(substream, cmd);
		break;
	case SNDRV_PCM_TRIGGER_STOP:
	case SNDRV_PCM_TRIGGER_SUSPEND:
	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
		ret = snd_soc_pcm_dai_trigger(substream, cmd);
		if (ret < 0)
			break;

		ret = snd_soc_pcm_component_trigger(substream, cmd);
		if (ret < 0)
			break;

		ret = snd_soc_link_trigger(substream, cmd);
		break;
	}

	return ret;
}

I don't see why we are doing in the machine driver what the ASoC core does already?

ranj063 · 2020-09-11T16:14:11Z

sound/soc/intel/boards/sof_sdw.c

 			      cpus + *cpu_id, cpu_dai_num,
 			      codecs, codec_num,
-			      NULL, &sdw_ops);
+			      NULL, codec_info_list[codec_index].ops);


@RanderWang this change is really hard to follow. Can youplease explain this in the commit message? I cant quite understand what this does

thanks, I will update it when we get a conclusion

bardliao · 2020-09-14T00:50:15Z

I don't see why we need to merge. The ASoC core will call the dailink .trigger and then the dai .trigger

@plbossart Currently, max98373_trigger is a dailink trigger, and we overwrite dailink ops in sof_sdw_mx8373_init().
dai_links->ops = &max_98373_sdw_ops;
As a result, the sdw_trigger() is not the dailink .trigger function and will never be triggered on mx8373 dailink.

plbossart · 2020-09-14T14:51:56Z

@plbossart Currently, max98373_trigger is a dailink trigger, and we overwrite dailink ops in sof_sdw_mx8373_init().
dai_links->ops = &max_98373_sdw_ops;

But WHY?

the dailink operations should only be about the stream handling. Why do we conflate the dailink and dai operations?
Like I said above this is blurring layers. There is no reason why we should treat max98373 in a different way from other amps/codecs.

plbossart · 2020-09-15T23:03:21Z

After reading through the comments, I think I get what @RanderWang was trying to do, but I don't think any of the suggested patches ever worked.

@RanderWang can you look at the attached patch and let me know if this is what you were trying to do. If yes, please push it. If not, please explain further what the issue is.

0001-ASOC-Intel-sof_sdw-restore-playback-functionality-wi.patch.txt

The Max98373 amplifier provides I/V feedback information, which keeps a DAPM path active even when there is no playback happening. This prevents entry in low-power mode. Rather than adding new controls and require UCM/user interaction, the method previously applied is to enable/disable the Speaker pin during the dailink trigger operations. Recent changes in the SoundWire stream management moved the stream trigger to the dailink trigger. This change removed the Maxim-specific pin handling and resulted in a regression. This patch restores functionality by combining the SoundWire stream trigger with the pin enable/disable. Fixes: 7eec07f ('ASOC: Intel: sof_sdw: add dailink .trigger callback') Fixes: 5595f95 ('ASOC: Intel: sof_sdw: add dailink .prepare and .hw_free callback'). Signed-off-by: Rander Wang <[email protected]> Signed-off-by: Pierre-Louis Bossart <[email protected]>

RanderWang · 2020-09-16T06:57:22Z

After reading through the comments, I think I get what @RanderWang was trying to do, but I don't think any of the suggested patches ever worked.

@RanderWang can you look at the attached patch and let me know if this is what you were trying to do. If yes, please push it. If not, please explain further what the issue is.

0001-ASOC-Intel-sof_sdw-restore-playback-functionality-wi.patch.txt

Thanks! It works on my volteer.

RanderWang · 2020-09-16T07:38:50Z

update my PR, thanks!

plbossart · 2020-09-16T18:59:44Z

@ranj063 @kv2019i please review and merge if you approve. I don't like grading my own paper.

ranj063

LGTM!

keyonjie

LGTM.

kv2019i

Thanks all for reviews!

RanderWang requested review from bardliao, dbaluta, kv2019i, lgirdwood, plbossart and ranj063 as code owners September 9, 2020 08:20

RanderWang force-pushed the 98373_playback_issue branch from 0c3da4d to 6a447bd Compare September 9, 2020 08:35

kv2019i requested changes Sep 9, 2020

View reviewed changes

sound/soc/intel/boards/sof_sdw.c Show resolved Hide resolved

plbossart requested changes Sep 9, 2020

View reviewed changes

plbossart added the Unclear No agreement on problem statement and resolution label Sep 10, 2020

RanderWang force-pushed the 98373_playback_issue branch from 6a447bd to 31e8d06 Compare September 10, 2020 09:10

RanderWang changed the title ~~ASOC: Intel: sof_sdw: fix playback issue on max98373~~ ASOC: Intel: sof_sdw: fix playback failure on max98373 Sep 10, 2020

RanderWang force-pushed the 98373_playback_issue branch from 31e8d06 to dcd2589 Compare September 10, 2020 11:05

RanderWang changed the title ~~ASOC: Intel: sof_sdw: fix playback failure on max98373~~ [RFC] ASOC: Intel: sof_sdw: fix playback failure on max98373 Sep 11, 2020

ranj063 reviewed Sep 11, 2020

View reviewed changes

RanderWang force-pushed the 98373_playback_issue branch from dcd2589 to ebedda4 Compare September 16, 2020 06:56

RanderWang changed the title ~~[RFC] ASOC: Intel: sof_sdw: fix playback failure on max98373~~ ASOC: Intel: sof_sdw: restore playback functionality with max98373 amps Sep 16, 2020

lyakh approved these changes Sep 16, 2020

View reviewed changes

plbossart approved these changes Sep 16, 2020

View reviewed changes

plbossart requested review from kv2019i and ranj063 September 16, 2020 13:29

plbossart removed the Unclear No agreement on problem statement and resolution label Sep 16, 2020

ranj063 approved these changes Sep 16, 2020

View reviewed changes

keyonjie approved these changes Sep 17, 2020

View reviewed changes

kv2019i reviewed Sep 17, 2020

View reviewed changes

kv2019i approved these changes Sep 17, 2020

View reviewed changes

kv2019i merged commit f237c1d into thesofproject:topic/sof-dev Sep 17, 2020

ASOC: Intel: sof_sdw: restore playback functionality with max98373 amps #2424

ASOC: Intel: sof_sdw: restore playback functionality with max98373 amps #2424

Uh oh!

Conversation

RanderWang commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Sep 9, 2020

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

plbossart Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

plbossart commented Sep 9, 2020

Uh oh!

ranj063 commented Sep 9, 2020

Uh oh!

plbossart commented Sep 10, 2020

Uh oh!

RanderWang commented Sep 10, 2020

Uh oh!

RanderWang commented Sep 10, 2020

Uh oh!

RanderWang commented Sep 10, 2020

Uh oh!

plbossart commented Sep 10, 2020

Uh oh!

RanderWang commented Sep 11, 2020

Uh oh!

RanderWang commented Sep 11, 2020

Uh oh!

RanderWang commented Sep 11, 2020

Uh oh!

bardliao commented Sep 11, 2020

Uh oh!

plbossart commented Sep 11, 2020

Uh oh!

plbossart commented Sep 11, 2020

Uh oh!

ranj063 Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

RanderWang Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

bardliao commented Sep 14, 2020

Uh oh!

plbossart commented Sep 14, 2020

Uh oh!

plbossart commented Sep 15, 2020

Uh oh!

RanderWang commented Sep 16, 2020

Uh oh!

RanderWang commented Sep 16, 2020

Uh oh!

plbossart commented Sep 16, 2020

Uh oh!

ranj063 left a comment

Choose a reason for hiding this comment

Uh oh!

keyonjie left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

RanderWang commented Sep 9, 2020 •

edited

Loading