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

bardliao · 2018-09-28T11:03:47Z

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

Signed-off-by: Bard liao bard.liao@intel.com

bardliao · 2018-09-28T11:04:41Z

I verified volume kcontrol works fine. But I didn't test the binary type kcontro yet since l ave no idea how to test it. Sorry about that. The PR is for issue #166

lgirdwood · 2018-09-28T12:53:10Z

sound/soc/sof/topology.c

@@ -285,6 +285,7 @@ static int sof_control_load_volume(struct snd_soc_component *scomp,
 	scontrol->comp_id = sdev->next_comp_id;
 	scontrol->num_channels = le32_to_cpu(mc->num_channels);

+


@lgirdwood removed. Thanks.

lgirdwood · 2018-09-28T12:55:09Z

sound/soc/sof/control.c

@@ -330,15 +236,14 @@ int snd_sof_bytes_ext_put(struct snd_kcontrol *kcontrol,
 	cdata->data->comp_abi = SOF_ABI_VERSION;

 	/* notify DSP of mixer updates */
-	snd_sof_ipc_set_comp_data(sdev->ipc, scontrol, SOF_IPC_COMP_SET_DATA,
-				  SOF_CTRL_TYPE_DATA_SET, scontrol->cmd);
+	if (sdev->dev->power.runtime_status == RPM_ACTIVE)


ok, so I dont like this RPM_ACTIVE macro, we should use proper kernel power state macros and remove RPM_ACTIVE entirely. I also cant see where we cache the value if the DSP is in D3 ?

@lgirdwood replaced with pm_runtime_active(sdev->dev). The kcontrol value will be cached in scontrol->control_data. We do it in every _put functions before we send the value to firmware. Currently we get the kcontrol value from firmware before we put it into ucontrol->value struct in _put functions. To read from cache, I remove the code reading kcontrol value from firmware. So it will leave as it is when we put the value into ucontrol->value struct.

lgirdwood · 2018-09-28T12:59:08Z

sound/soc/sof/topology.c

@@ -2362,6 +2363,48 @@ int snd_sof_complete_pipeline(struct snd_sof_dev *sdev,
 	return 1;
 }

+static int snd_sof_get_kcontrol_val(struct snd_sof_dev *sdev)


Please provide a comment about what this function does. It looks like it gets all kcontrol values prior to DSP D3 ? If so we probably only want to read the volatile kcontrols (iirc, ALSA has a flag for volatile kcontrols).

@lgirdwood a comment is added. The function is called in sof_complete. I believe that it will be only called in boot up. What I want to do is to set the initial value in the cache where store the values of kcontrols. So I think we need to read all the kcontrols.

ok, the default kcontrol values should be set from topology loading or by userspace writing the value after soundcard int. i.e. alsactl

Agree. I noticed the kcontrol value will be restore when we restore pipelines. So the kcontrol value will keep align when DSP is active. And the default value should be set by alsactl though I don't like to rely on userspace. So, I think we don't need to care about the default value on this PR, right?

@lgirdwood So we don't need to deal with the default value on this PR, right?

@bardliao the default value for each kcontrol will be cached when get() is called on it the first time after initial loading from topology. All ALSA kcontrols are only modified via RMW sequence giving us a chance to cache them all when they are first accessed by userspace.

@lgirdwood Yes, and what the function do is very similar to the first read. Note that I removed the ipc functions from the ALSA kcontrol get() functions in this PR. So, it will get the cached data even when the get() is called first time. The snd_sof_get_kcontrol_val function will get data from DSP when firmware loading is finished and store it to the cache. And ALSA kcontrol get() function will get the right data from cache. But as you said, alsactl should set the default value for each kcontrol. So I think there will not be any issue if we remove this function.

@bardliao we will need to keep an invalidate and writeback flag for each kcontrol which will indicate whether we need to invalidate and read back values from FW or writeback new value to firmware.

@lgirdwood I update the PR according to your suggestion. Could you review it?

lgirdwood · 2018-09-29T19:44:32Z

sound/soc/sof/topology.c

@@ -2362,6 +2363,48 @@ int snd_sof_complete_pipeline(struct snd_sof_dev *sdev,
 	return 1;
 }

+static int snd_sof_get_kcontrol_val(struct snd_sof_dev *sdev)


ok, the default kcontrol values should be set from topology loading or by userspace writing the value after soundcard int. i.e. alsactl

lgirdwood

Just some minor changes needed. I also think we need to combine this with @ranj063 PM work and also only restore kcontrol values when their pipelines are loaded. i.e. when we play a stream on a PCM. @keqiaozhang can you check this with PM stress test.

lgirdwood · 2018-10-08T13:34:04Z

sound/soc/sof/control.c

-		dev_err(sdev->dev, "error: volume get failed to resume %d\n",
-			ret);
-		return ret;
+	if (!scontrol->is_valid) {


what does is_valid mean here ? do you mean is_dsp_resident instead. Please also comment this conditional.

lgirdwood · 2018-10-08T13:49:53Z

sound/soc/sof/sof-priv.h

@@ -250,6 +250,7 @@ struct snd_sof_control {
 	u32 size;	/* cdata size */
 	enum sof_ipc_ctrl_cmd cmd;
 	u32 *volume_table; /* volume table computed from tlv data*/
+	bool is_valid; /* is the control data valid */


I think this should be is_dsp_resident.

bardliao · 2018-10-09T05:11:46Z

@lgirdwood new version is pushed. I added some comments and renamed the flag.

ranj063 · 2018-10-09T05:17:27Z

@bardliao @lgirdwood I will test this today on the yorp and get back with comments

ranj063 · 2018-10-10T17:30:22Z

sound/soc/sof/control.c

-			ret);
-		return ret;
+	/* if dsp control data is not cached, wake it up and get the data */
+	if (!scontrol->is_dsp_resident) {


@bardliao @lgirdwood should we always use cached data or only when the device is runtime suspended?

@bardliao I also think this check for is_dsp_resident is redundant. the kcontrol data is always cached including at first boot when alsa restores all kcontrols to the previously stored values.

So in my opinion, we should check if the device is suspended and if it is, we set the cached values and if not we should get the value from the dsp. What do you think?

@ranj063 I think we can always use cached data. If it is not a volatile kcontrol, the cached data should be always aligned with dsp. On the other hand, we should always get the data from dsp if it is a volatile kcontrol. Regarding the is_dsp_resident flag, what will happen after factory reset? We may not set all kcontrols in the UCM (or similar) file. Also, I think Android will not restore "ALL" kcontrols. In my opinion, driver should guarantee the cached data is aligned with dsp. I think alternative solution is to set the default value to both dsp and cache when topology is loaded. I.e. The initial value will be decided by topology instead of dsp firmware.

@bardliao @ranj063 cache should always be used for readback unless kcontrol is volatile, same rules as regmap. writes updaes cache and FW iff component is active.

plbossart · 2018-10-22T20:35:55Z

is this PR still works-in-progress, waiting for comments or is it dropped? Unclear that the problem statement is and what you are trying to do.

bardliao · 2018-10-23T00:50:08Z

@plbossart I think it is waiting for comments. The PR is trying to cache the kcontrol value so that we can get kcontrol value from cache without resuming dsp. @ranj063 and I are talking about should we read the kcontrol value from dsp at boot up? alsactl will restore the kcontrol value when the system boots up in normal case. But I am thinking about is there any corner case that no one will restore the kcontrol value? For example, factory reset? Also, I think Android will set some kcontrols but not all. That's why I think it is necessary to have a flag to identify if the cache is aligned with the real value.

bardliao · 2018-10-26T06:17:11Z

@plbossart @lgirdwood Do you have any comments?

lgirdwood · 2018-10-26T13:18:26Z

@plbossart problem statement is caching all kcontrols meaning we dont need to enter D0 for every kcontrol R/W. This is also to use the compund IPC so that kcontrol cache can be flushed when we enter D0 as a single IPC.

@bardliao the initial default values of all kcontrol could be read after topology loading has completed (after first boot) - using single (or a small number of) compound IPC.

bardliao · 2018-10-28T01:53:01Z

@lgirdwood It seems be the same as my 1st version except the compound ipc. Should we wait for the PR of compound ipc or we can use regular ipc first and modify it by the compound ipc's patch?

plbossart · 2018-10-29T14:45:02Z

@lgirdwood Do we really care if the DSP comes out of D3 when a control is changed? the entire machine is in D0 anyways. Are we chasing micro optimizations that aren't even KPIs?

Also I wonder why we need SOF-specific cache handling, shouldn't the cache handling be handled at the ASoC level?

Last I am confused on what the compound ipc is, if we don't have the code just yet then should we postpone this change to later?

lgirdwood · 2018-10-29T16:59:39Z

@bardliao yes use regular IPC first for local testing but then update to use compound IPC in PR.

@plbossart The intention here is not to bring DSP into D0 just to change a kcontrol that will just be cached anyway. i.e. just cache the value. I did consider regmap handling, but there are limitations on size for coefficients. Compound IPC is taking N similar IPCs and merging them into a single IPC with one doorbell. i.e. taking 15 topology creation or 15 kcontrol updates and merging into 1 IPC.

plbossart · 2018-10-29T23:25:11Z

@lgirdwood not sure I get your point... This still looks like a third-order optimization that's not a priority. Also is the compound IPC implemented or not, if not then i suggest we work on more important stuff for the time being.

lgirdwood · 2018-10-30T19:15:28Z

@plbossart we need this for PM KPIs as we will only create pipelines on PCM open and dismantle on PCM close (as we will power off SRAM banks). The compound IPC just makes this quicker so that we dont delay playback. @mmaka1 is working on the power aware allocator updates.

plbossart · 2018-10-31T23:37:21Z

@lgirdwood don't get your point, what's the link between this patch (action on kcontrol change) and pipeline creation with PCM open/close, and delaying playback? You lost me in the dust and I wonder if you replied to the right PR...

lgirdwood · 2018-11-01T19:59:30Z

@plbossart from a high level we add kcontrol caching and compound IPC for PM and latency reasons. The end goal is to be able to only load topology components for active streams (inactive streams wont have topology loaded in FW - driver only), as agreed for December update.

we save on SRAM leakage by unloading any components not in the active (playback/capture) pipeline (and disabling the SRAM bank), hence any kcontrols IO to these controls is cached. The allocator is being modified to support this for Yorp by @mmaka1 .
We dont do any superfluous D3 -> D0 -> D3 state transitions if DSP is inactive (since kcontrols are cached already during D3 entry - we write to cache directly) - runtime simplification with latency improvements.

All the IPC related to kcontrols and topology can be easily aggregated (compared to stream IPC) so it additionally makes sense to do this here to in order to prevent any latency regressions related to additional IPCs (for pipeline/kcontrol setup on each active PCM).

plbossart · 2018-11-01T23:26:36Z

@lgirdwood I understand now the savings you are trying to enable. I would feel more comfortable however with the dual approach of enabling everything first, then release what is not used. That would help catch configuration issues or worst-cases upfront (fail big and fail early). If you only enable the IPC when needed, you are going to have a hard time dealing with bad parameters or topologies issues, and unwinding those errors way after boot is complicated at best. It's safer in my opinion to keep the initial handshakes with the DSP and go to idle on specific components later. see already all the problems we have with runtime_pm, if you compound this with deferred IPC then be prepared for long and painful debug sessions....

plbossart · 2018-11-05T20:01:49Z

BTW the code for controls is not quite right, we still query the DSP on _get access, e.g.

int snd_sof_enum_get(struct snd_kcontrol *kcontrol,
		     struct snd_ctl_elem_value *ucontrol)
{
	struct soc_enum *se =
		(struct soc_enum *)kcontrol->private_value;
	struct snd_sof_control *scontrol = se->dobj.private;
	struct snd_sof_dev *sdev = scontrol->sdev;
	struct sof_ipc_ctrl_data *cdata = scontrol->control_data;
	unsigned int i, channels = scontrol->num_channels;
	int err, ret;

	ret = pm_runtime_get_sync(sdev->dev);
	if (ret < 0) {
		dev_err(sdev->dev, "error: enum get failed to resume %d\n",
			ret);
		return ret;
	}

	/* get all the mixer data from DSP */
	snd_sof_ipc_get_comp_data(sdev->ipc, scontrol, SOF_IPC_COMP_GET_VALUE,
				  SOF_CTRL_TYPE_VALUE_CHAN_GET,
				  SOF_CTRL_CMD_ENUM);

	/* read back each channel */
	for (i = 0; i < channels; i++)
		ucontrol->value.integer.value[i] = cdata->chanv[i].value;

	pm_runtime_mark_last_busy(sdev->dev);
	err = pm_runtime_put_autosuspend(sdev->dev);
	if (err < 0)
		dev_err(sdev->dev, "error: enum get failed to idle %d\n",
			ret);
	return 0;
}

so before we optimize the access on a _put maybe we should cleanup control.c and use the cached values rather than bring the DSP out of D3 when reading controls?

bardliao · 2018-11-06T07:52:12Z

@plbossart agree. That is included in my patch. It seems that I closed the PR accidentally. I will reopen it once the compound IPC patch is ready. Probably you can review my patch from bardliao@6056880

plbossart · 2018-11-06T18:02:09Z

@bardliao Hah, I missed this change for the get calls. Do you mind re-submitting a patch just for this, so that we can clean-up the code for upstream. We can deal with the _put case later when the compound IPC is there, but there's no reason to delay the _get case, is there?

bardliao · 2018-11-07T10:51:32Z

@plbossart sure I am willing to do it. But did you mean a new path just for the _get calls or the full patch (including the _put part) but using regular ipc? My concern is if we don't get the data through ipc at least one time and write it to the cache, we may get a mismatched data from cache. My suggestion is we can merge the full patch first and replace the regular ipc with compound ipc latter or deal with _put case first. I think the purpose of compound ipc here is only for getting data.

plbossart · 2018-11-08T15:14:29Z

@bardliao i've had several discussions with Ranjani on this, and to be honest I just don't understand how initial mixer values are set. It looks like we rely on the firmware to get them, see e.g. #226, but that makes little sense to me. I thought the mixers would all come with a default value and then be overwritten by userspace (asound.conf). Why exactly we get the values from the DSP is not quite clear to me.

juimonen · 2018-11-08T15:53:06Z

@plbossart I think currently we are just not setting them. We have min and max value in pga private struct but it is neither used in topology nor parsed in kernel. When pga is created this private struct is sent to dsp and because the values are not there or parsed, they are 0 -> dsp pga component sets max value to hard coded define (1 << 16). Now this private struct is also created as part of the related kcontrol. If get_pga_values_from_dsp is never called also the private values in control are empty (I mean it is newly created zeroed struct). This is an issue when we restore from PM, because then the "empty" values are set and the pga is really "confused".

I think this is a more general question, how we can set initial firmware private values to related control in pga creation, when they are actually defined in DSP. So for e.g can the range or format change and so on. So we could set the control values to max, but we should take care that the private part, which the firmware/dsp understands. So it needs to know the format and max value so they can be set the first time. Or they come all the way from topology and they just have to match to the pga implementation.

juimonen · 2018-11-08T16:01:28Z

and of course all the above is masked away, if something/someone from userspace starts to set the mixer values. That works well.

bardliao · 2018-11-10T16:00:07Z

Thanks @juimonen for the explanation. @plbossart To sync between the cache and the real value, I think we can either get the initial value from FW or set an initial value to FW. The reason I use option 1 is that I though FW will have their own default value. ie. FW will have a default value without setting from outside. I also agree with "the mixers would all come with a default value and then be overwritten by userspace". The default value of a codec's kcontrol is from HW if the kcontrol is associated with a codec register. And I think the same logic can also apply to FW's kcontrol.

plbossart · 2018-11-12T14:06:00Z

@bardliao I don't think we necessarily need to unconditionally rely on firmware defaults. My view is that we should use a priority order (less-important to more important)
a. firmware defaults
b. topology defaults (which currently don't seem to exist - probably a miss)
c. user changes

bardliao · 2018-11-13T06:55:13Z

@plbossart Yes, firmware defaults is the lowest priority and we don't have topology defaults so far. So I think it makes sense to get the initial value from FW since we will not get initial value from topology. User changes will certainly sync cached value with the real value. However, we can't guarantee that user will set every kcontrol before reading it.

plbossart · 2018-11-13T15:41:05Z

I added an issue thesofproject/soft#128 to track this separately.

When arm_ffa firmware driver module is unloaded or removed we call __ffa_devices_unregister on all the devices on the ffa bus. It must unregister all the devices instead it is currently just releasing the devices without unregistering. That is pure wrong as when we try to load the module back again, it will result in the kernel crash something like below. -->8 CPU: 2 PID: 232 Comm: modprobe Not tainted 5.15.0-rc2+ thesofproject#169 Hardware name: FVP Base RevC (DT) Call trace: dump_backtrace+0x0/0x1cc show_stack+0x18/0x64 dump_stack_lvl+0x64/0x7c dump_stack+0x18/0x38 sysfs_create_dir_ns+0xe4/0x140 kobject_add_internal+0x170/0x358 kobject_add+0x94/0x100 device_add+0x178/0x5f0 device_register+0x20/0x30 ffa_device_register+0x80/0xcc [ffa_module] ffa_setup_partitions+0x7c/0x108 [ffa_module] init_module+0x290/0x2dc [ffa_module] do_one_initcall+0xbc/0x230 do_init_module+0x58/0x304 load_module+0x15e0/0x1f68 __arm64_sys_finit_module+0xb8/0xf4 invoke_syscall+0x44/0x140 el0_svc_common+0xb4/0xf0 do_el0_svc+0x24/0x80 el0_svc+0x20/0x50 el0t_64_sync_handler+0x84/0xe4 el0t_64_sync+0x1a0/0x1a4 kobject_add_internal failed for arm-ffa-8001 with -EEXIST, don't try to register things with the same name in the same directory. ---- Fix the issue by calling device_unregister in __ffa_devices_unregister which will also take care of calling device_release(which is mapped to ffa_release_device) Link: https://lore.kernel.org/r/20210924092859.3057562-2-sudeep.holla@arm.com Fixes: e781858 ("firmware: arm_ffa: Add initial FFA bus support for device enumeration") Tested-by: Jens Wiklander <jens.wiklander@linaro.org> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

bardliao requested review from ranj063 and lgirdwood September 28, 2018 11:03

bardliao force-pushed the up2_pm branch from 769cedc to 4cd1ab4 Compare September 28, 2018 11:06

lgirdwood requested changes Sep 28, 2018

View reviewed changes

bardliao force-pushed the up2_pm branch 4 times, most recently from 90e6f61 to f576e1a Compare September 29, 2018 05:19

lgirdwood requested changes Sep 29, 2018

View reviewed changes

bardliao force-pushed the up2_pm branch from f576e1a to 3874704 Compare October 8, 2018 02:54

lgirdwood requested changes Oct 8, 2018

View reviewed changes

bardliao force-pushed the up2_pm branch from 3874704 to 74afc05 Compare October 9, 2018 00:43

ranj063 reviewed Oct 10, 2018

View reviewed changes

plbossart force-pushed the topic/sof-dev branch from 2af9599 to 7e41d53 Compare November 1, 2018 16:18

bardliao force-pushed the up2_pm branch from 74afc05 to 9fb80f4 Compare November 6, 2018 01:31

bardliao closed this Nov 6, 2018

bardliao force-pushed the up2_pm branch from 9fb80f4 to a5ddc3d Compare November 6, 2018 01:32

bardliao mentioned this pull request Jun 4, 2019

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

Merged

		@@ -285,6 +285,7 @@ static int sof_control_load_volume(struct snd_soc_component *scomp,
		scontrol->comp_id = sdev->next_comp_id;
		scontrol->num_channels = le32_to_cpu(mc->num_channels);

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

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

Conversation

bardliao commented Sep 28, 2018 • edited Loading

bardliao commented Sep 28, 2018

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

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

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bardliao commented Oct 9, 2018

ranj063 commented Oct 9, 2018

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

plbossart commented Oct 22, 2018

bardliao commented Oct 23, 2018

bardliao commented Oct 26, 2018

lgirdwood commented Oct 26, 2018

bardliao commented Oct 28, 2018

plbossart commented Oct 29, 2018

lgirdwood commented Oct 29, 2018

plbossart commented Oct 29, 2018

lgirdwood commented Oct 30, 2018

plbossart commented Oct 31, 2018

lgirdwood commented Nov 1, 2018

plbossart commented Nov 1, 2018

plbossart commented Nov 5, 2018

bardliao commented Nov 6, 2018

plbossart commented Nov 6, 2018

bardliao commented Nov 7, 2018

plbossart commented Nov 8, 2018

juimonen commented Nov 8, 2018

juimonen commented Nov 8, 2018

bardliao commented Nov 10, 2018

plbossart commented Nov 12, 2018

bardliao commented Nov 13, 2018

plbossart commented Nov 13, 2018

bardliao commented Sep 28, 2018 •

edited

Loading