ASoC: SOF: dont wake dsp up in kcontrol IO #1006

bardliao · 2019-06-04T09:09:59Z

Always get kcontrol value from cache, set kcontrol value to DSP
when DSP is active. Kcontrol values will be restored when DSP boot up.
We will set the default value of kcontrol in sof_complete to make sure
the value is align with firmware.

The benefit of this PR is that we don't need to power up/down DSP repeatedly when we want to get/set a set of kcontrols.
Currently, we set the kcontorl default value by reading from FW. We can send another patch to prioritize it to read from topology first once we can get the default value from topology.

The PR is rebased from #169

plbossart

code can be further optimized and question on kcontrol init below.

plbossart · 2019-06-04T15:39:33Z

sound/soc/sof/topology.c

+			ret = snd_sof_ipc_set_get_comp_data(sdev->ipc, scontrol,
+							    ipc_cmd, ctrl_type,
+							    scontrol->cmd,
+							    false);


you can move this block outside of the switch case since it's duplicated in all cases.

plbossart · 2019-06-04T15:40:36Z

sound/soc/sof/topology.c

@@ -3005,6 +3053,7 @@ static void sof_complete(struct snd_soc_component *scomp)
 			break;
 		}
 	}
+	snd_sof_get_kcontrol_val(sdev);


I am still lost on how the controls are initialized

by topology

by reading DSP values over IPC?

by alsa-mixer?

@plbossart Currently, we don't have the ability to get the default value by topology. So it is by reading DSP value over IPC for now. And it may be overwrite by alsa-mixer after booting up.

ok, you should add a comment on this so that the sequence is documented.

@plbossart done

dbaluta · 2019-06-05T08:26:54Z

sound/soc/sof/topology.c

@@ -3005,6 +3048,7 @@ static void sof_complete(struct snd_soc_component *scomp)
 			break;
 		}
 	}
+	snd_sof_get_kcontrol_val(sdev);


Maybe add a comment here letting the readers know that we are caching kcontrol values now in order to use them later and not do calls to DSP everytime.

e.g /* cache initial values of SOF kcontrols */

@dbaluta done. I also rename the function.

plbossart

Still thrown-off by the last comment

plbossart · 2019-06-07T14:38:44Z

sound/soc/sof/topology.c

@@ -3005,6 +3048,12 @@ static void sof_complete(struct snd_soc_component *scomp)
 			break;
 		}
 	}
+	/*
+	 * cache initial values of SOF kcontrols by reading DSP value over
+	 * IPC. It may be overwrite by alsa-mixer after booting up


overwritten

plbossart · 2019-06-07T14:40:16Z

sound/soc/sof/topology.c

+	 * cache initial values of SOF kcontrols by reading DSP value over
+	 * IPC. It may be overwrite by alsa-mixer after booting up
+	 */
+	/* TODO: cache initial values of SOF kcontrols by topology */


well that confuses me.
@ranj063 is there a way to get those values from topology, and if yes, why do we read from the DSP?

@plbossart this was my thinking too. There are 3 open PR's all addressing kcontrol IO issues and they're all related. Let me spend some time over the next couple of days to connect them all

@plbossart @bardliao I looked into this and there are a few things I want to bring up:

There are no default values specified for volume/switch controls in topology

byte controls have the option of adding default data but this is optional

Even if there are default values specified in topology, the DSP can override these values based on whether these are valid or not. For example, if the defaul value for volume control exceeds the max supported, the DSP will only set the default value to 0DB atm

But overall I agree with the idea that kcontrol IO get could always return the cached value. But this does need the step below to cache the value by reading it once from the DSP.

Lastly, I do not understand what this comment means "TODO: cache initial values of SOF kcontrols by topology"?
For the case of process type components, it is already cached (please refer to jaska's PR).

@ranj063 The TODO is originally a reminder to me to get the cached data from topology is there is one. But on the second thought, I think we don't need to do here. We can set the topology's default value to the dsp in topology loading and get the value via ipc here. It can prevent the error if the dsp didn't accept the topology's default value.

@plbossart @bardliao I looked into this and there are a few things I want to bring up:

There are no default values specified for volume/switch controls in topology

byte controls have the option of adding default data but this is optional

Even if there are default values specified in topology, the DSP can override these values based on whether these are valid or not. For example, if the defaul value for volume control exceeds the max supported, the DSP will only set the default value to 0DB atm

But overall I agree with the idea that kcontrol IO get could always return the cached value. But this does need the step below to cache the value by reading it once from the DSP.

Lastly, I do not understand what this comment means "TODO: cache initial values of SOF kcontrols by topology"?
For the case of process type components, it is already cached (please refer to jaska's PR).

@ranj063 I take your answer as agreement but would like a formal approval before I merge. Thanks

Always get kcontrol value from cache, set kcontrol value to DSP when DSP is active. Kcontrol values will be restored when DSP boot up. We will set the default value of kcontrol in sof_complete to make sure the value is align with firmware. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

plbossart

looks good but question below:

plbossart · 2019-06-11T22:03:41Z

sound/soc/sof/topology.c

+	 * cache initial values of SOF kcontrols by reading DSP value over
+	 * IPC. It may be overwritten by alsa-mixer after booting up
+	 */
+	snd_sof_cache_kcontrol_val(sdev);


question: do we deal with this cache at the first boot or every time we resume (runtime_pm or not)?

@plbossart I think it is enough if we do this at first boot. When we resume, we actually do the opposite where we send the cached values to the DSP

bardliao requested review from plbossart, ranj063, libinyang and lgirdwood June 4, 2019 09:10

bardliao requested review from juimonen and singalsu as code owners June 4, 2019 09:10

plbossart requested changes Jun 4, 2019

View reviewed changes

bardliao force-pushed the kcontrol-io branch 2 times, most recently from 806a5b1 to 5c3c4b0 Compare June 5, 2019 01:37

dbaluta reviewed Jun 5, 2019

View reviewed changes

bardliao force-pushed the kcontrol-io branch 2 times, most recently from 301cd45 to fd4c78e Compare June 6, 2019 00:51

plbossart requested changes Jun 7, 2019

View reviewed changes

bardliao force-pushed the kcontrol-io branch from fd4c78e to db6dc79 Compare June 9, 2019 14:26

bardliao force-pushed the kcontrol-io branch from db6dc79 to b920093 Compare June 11, 2019 08:01

plbossart reviewed Jun 11, 2019

View reviewed changes

ranj063 approved these changes Jun 11, 2019

View reviewed changes

plbossart approved these changes Jun 11, 2019

View reviewed changes

plbossart merged commit 38d0e9f into thesofproject:topic/sof-dev Jun 11, 2019

ranj063 mentioned this pull request Jun 17, 2019

Fix pm_runtime_put_autosuspend() failed issues in sof driver #891

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASoC: SOF: dont wake dsp up in kcontrol IO #1006

ASoC: SOF: dont wake dsp up in kcontrol IO #1006

bardliao commented Jun 4, 2019

plbossart left a comment

plbossart Jun 4, 2019

bardliao Jun 5, 2019

plbossart Jun 4, 2019

bardliao Jun 5, 2019

plbossart Jun 5, 2019

bardliao Jun 6, 2019

dbaluta Jun 5, 2019 •

edited

Loading

bardliao Jun 5, 2019

plbossart left a comment

plbossart Jun 7, 2019

plbossart Jun 7, 2019

ranj063 Jun 7, 2019

ranj063 Jun 10, 2019 •

edited

Loading

bardliao Jun 11, 2019

plbossart Jun 11, 2019

plbossart left a comment

plbossart Jun 11, 2019

ranj063 Jun 11, 2019

ASoC: SOF: dont wake dsp up in kcontrol IO #1006

ASoC: SOF: dont wake dsp up in kcontrol IO #1006

Conversation

bardliao commented Jun 4, 2019

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaluta Jun 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranj063 Jun 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaluta Jun 5, 2019 •

edited

Loading

ranj063 Jun 10, 2019 •

edited

Loading