ASoC: SOF: topology: set default values to volume control #258

juimonen · 2018-11-08T14:35:36Z

Currently volume control doesn't have any default values
when it is created. This will cause an issue when PM
kicks in and in resume the non-existing values are
set to firmware.

Signed-off-by: Jaska Uimonen jaska.uimonen@intel.com

sofci · 2018-11-08T14:37:26Z

Can one of the admins verify this patch?

plbossart

Some cosmetic changes and a more important question on whether we'd override user defaults (if any) stored in astate.conf?

plbossart · 2018-11-08T16:39:15Z

sound/soc/sof/topology.c

@@ -1258,6 +1261,13 @@ static int sof_widget_load_pga(struct snd_soc_component *scomp, int index,
 		goto err;
 	}

+	/* set default max volume values to control */


default 0dB?

Yep but in theory 0 dB could be out of min..max range. So use max if 0 dB is above range.

vol = min(vol_0db, vol_max);

ok. but so what is max and where do I get it :) ?

@singalsu I know left it to 0db. I can do later the min(0 or max), but I need to change a lot to get the max from topology.

plbossart · 2018-11-08T16:40:04Z

sound/soc/sof/topology.c

 	const unsigned int *p;
-	int ret, tlv[TLV_ITEMS];
+	int ret, i, tlv[TLV_ITEMS];


nitpick: kernel reviewers like xmas-tree declarations and variables on different lines.

ok will fix

plbossart · 2018-11-08T16:40:29Z

sound/soc/sof/topology.c

@@ -36,6 +36,8 @@
 #define VOLUME_FWL	16
 /* 0.5 dB step value in topology TLV */
 #define VOL_HALF_DB_STEP	50
+/* full volume for default values */
+#define VOL_ZERO_DB	(1 << 16)


BIT(VOLUME_FWL) ?

ok, I didn't even check/understand the previously defined values. Can change this.

Could all add comment to VOLUME_FWL that define to 16 sets the volume linear gain value to use Qx.16 format.

It's out of scope of this PR but those macros VOL_TWENTIETH_ROOT_OF_TEN and VOL_FORTIETH_ROOT_OF_TEN could be re-calculated for a higher number of fractional bits (31) and shifted right by (31-VOLUME_FWL) to match the volume multiplier Q format. Then the used Q format should be controllable in single macro.

@singalsu added your comment to the VOLUME_FWL define.

plbossart · 2018-11-08T16:41:35Z

sound/soc/sof/topology.c

+		cdata->chanv[i].channel = i;
+		cdata->chanv[i].value = VOL_ZERO_DB;
+	}
+


do you happen to know how this will work or interfere with alsactrl trying to restore previous values?

it should work, because this is topology loading, and I guess any alsactl command should not come in the middle (?). Not sure what happens if control is called in the middle of topology loading. PM is caching the stuff and in reload this part is never called. However, good point, I'll re-enable my alsactl and give it a try anyway.

@plbossart I tested with following scenario in my ubuntu:

alsactl removed, so "first boot" set both pga channels to 0db

I changed manually from alsamixer the pga volumes and re-enabled alsactl

after boot the pga volumes are still in positions I set in 2

So I can't still guarantee fully all use cases, but this basic one is working.

works for me as well, I took the code and all the settings were moved to 0dB on all PGAs. I changed one of the gains, rebooted and the values were kept, so it looks functional to me. Will merge, thanks Jaska!

Currently volume control doesn't have any default values when it is created. This will cause an issue when PM kicks in and in resume the non-existing values are set to firmware. Signed-off-by: Jaska Uimonen <jaska.uimonen@intel.com>

ranj063 · 2018-11-08T18:27:34Z

@juimonen I have 2 questions about this patch:

When do we see this issue with non-existent values being sent to the FW. On my Yorp, I can verify that the volume settings are retained after coming out of runtime suspend or system suspend.

After the system boots, there always a volume_put() call that sets these values to default. SO, I have not come across the problem of not the driver have no cached value for volume.

Assuming this change is required , should we also send an ipc to align the fw and driver ?

ranj063 · 2018-11-08T18:45:53Z

@juimonen I take back my first comment. I dont see a volume_put() call at boot anymore on the Yorp. So might be a good idea to add this.

But do we still need the ipc?

juimonen · 2018-11-08T18:46:30Z

@ranj063 I see this in up2 with ubuntu, when there's no asound.state file. I think this happens with first boot after flashing the image. I'm not sure if there's some other process that tries to set them at first boot, because I'm not sure I get it every time (I flash quite rarely). Anyway I've seen this issue many times happening. You get it by disabling alsactl. Yorp/Chrome might behave totally different. As said if there is some process in userspace setting the values, there's no issue.

I was thinking getting the values with ipc, but this is in the middle of creating the pga, so it doesn't exists yet at this point in dsp. So there are no default values yet, unless we do it after the pga creation. Pipeline is not ready so don't know what would happen with ipc. So this is kind of chicken/egg problem...

ranj063 · 2018-11-08T18:59:42Z

@juimonen for the ipc, we dont need the pipeline, do we?

After the ipc to create the volume component is done, we could send another ipc (SOF_IPC_COMP_SET_VALUE) to set the default value for it right?

juimonen · 2018-11-08T19:06:39Z

@ranj063 You mean calling "get" with ipc? So we should get the default that firmware is setting. Or do you mean that we decide that kernel side is deciding the default and just syncing with DSP? Sorry it is little bit hazy to me which one is deciding... :)

ranj063 · 2018-11-08T19:20:50Z

@juimonen im not sure... I think get() makes more sense. @singalsu what do you think?

juimonen · 2018-11-08T19:26:58Z

@ranj063 Ok. I'll try it out. let's see what happens and how it looks like...

ranj063 · 2018-11-08T19:49:26Z

@juimonen thinking about this...can we somehow use the sof_ipc_comp_reply when the new volume comp is setup to respond with the default value in the firmware and use that?

plbossart

verified on Up2 w/ HDMI

plbossart · 2018-11-08T23:57:25Z

@ranj063 the solution might not be the optimal or the final one but we've going to review all this anyways when we cache the values instead of always querying the DSP regardless of its state. merged for now to make the problem go away.

This check erroneously flags cases like the one in my recent printk enumeration patch[0], where the spaces are syntactic, and `section:' vs. `section :' is syntactically important: ERROR: space prohibited before that ':' (ctx:WxW) #258: FILE: include/asm-generic/vmlinux.lds.h:314: + .printk_fmts : AT(ADDR(.printk_fmts) - LOAD_OFFSET) { 0: https://lore.kernel.org/patchwork/patch/1375749/ Link: https://lkml.kernel.org/r/YBwhqsc2TIVeid3t@chrisdown.name Link: https://lkml.kernel.org/r/YB6UsjCOy1qrrlSD@chrisdown.name Signed-off-by: Chris Down <chris@chrisdown.name> Acked-by: Joe Perches <joe@perches.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Add various tests to check maximum number of supported programs being attached: # ./vmtest.sh -- ./test_progs -t tc_opts [...] ./test_progs -t tc_opts [ 1.185325] bpf_testmod: loading out-of-tree module taints kernel. [ 1.186826] bpf_testmod: module verification failed: signature and/or required key missing - tainting kernel [ 1.270123] tsc: Refined TSC clocksource calibration: 3407.988 MHz [ 1.272428] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x311fc932722, max_idle_ns: 440795381586 ns [ 1.276408] clocksource: Switched to clocksource tsc #252 tc_opts_after:OK #253 tc_opts_append:OK #254 tc_opts_basic:OK #255 tc_opts_before:OK #256 tc_opts_chain_classic:OK #257 tc_opts_chain_mixed:OK #258 tc_opts_delete_empty:OK #259 tc_opts_demixed:OK #260 tc_opts_detach:OK #261 tc_opts_detach_after:OK #262 tc_opts_detach_before:OK #263 tc_opts_dev_cleanup:OK #264 tc_opts_invalid:OK #265 tc_opts_max:OK <--- (new test) #266 tc_opts_mixed:OK #267 tc_opts_prepend:OK #268 tc_opts_replace:OK #269 tc_opts_revision:OK Summary: 18/0 PASSED, 0 SKIPPED, 0 FAILED Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> Link: https://lore.kernel.org/bpf/20230929204121.20305-2-daniel@iogearbox.net

Add a new test case which performs double query of the bpf_mprog through libbpf API, but also via raw bpf(2) syscall. This is testing to gather first the count and then in a subsequent probe the full information with the program array without clearing passed structs in between. # ./vmtest.sh -- ./test_progs -t tc_opts [...] ./test_progs -t tc_opts [ 1.398818] tsc: Refined TSC clocksource calibration: 3407.999 MHz [ 1.400263] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x311fd336761, max_idle_ns: 440795243819 ns [ 1.402734] clocksource: Switched to clocksource tsc [ 1.426639] bpf_testmod: loading out-of-tree module taints kernel. [ 1.428112] bpf_testmod: module verification failed: signature and/or required key missing - tainting kernel #252 tc_opts_after:OK #253 tc_opts_append:OK #254 tc_opts_basic:OK #255 tc_opts_before:OK #256 tc_opts_chain_classic:OK #257 tc_opts_chain_mixed:OK #258 tc_opts_delete_empty:OK #259 tc_opts_demixed:OK #260 tc_opts_detach:OK #261 tc_opts_detach_after:OK #262 tc_opts_detach_before:OK #263 tc_opts_dev_cleanup:OK #264 tc_opts_invalid:OK #265 tc_opts_max:OK #266 tc_opts_mixed:OK #267 tc_opts_prepend:OK #268 tc_opts_query:OK <--- (new test) #269 tc_opts_replace:OK #270 tc_opts_revision:OK Summary: 19/0 PASSED, 0 SKIPPED, 0 FAILED Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Link: https://lore.kernel.org/r/20231006220655.1653-4-daniel@iogearbox.net Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>

Add a new test case to query on an empty bpf_mprog and pass the revision directly into expected_revision for attachment to assert that this does succeed. ./test_progs -t tc_opts [ 1.406778] tsc: Refined TSC clocksource calibration: 3407.990 MHz [ 1.408863] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x311fcaf6eb0, max_idle_ns: 440795321766 ns [ 1.412419] clocksource: Switched to clocksource tsc [ 1.428671] bpf_testmod: loading out-of-tree module taints kernel. [ 1.430260] bpf_testmod: module verification failed: signature and/or required key missing - tainting kernel #252 tc_opts_after:OK #253 tc_opts_append:OK #254 tc_opts_basic:OK #255 tc_opts_before:OK #256 tc_opts_chain_classic:OK #257 tc_opts_chain_mixed:OK #258 tc_opts_delete_empty:OK #259 tc_opts_demixed:OK #260 tc_opts_detach:OK #261 tc_opts_detach_after:OK #262 tc_opts_detach_before:OK #263 tc_opts_dev_cleanup:OK #264 tc_opts_invalid:OK #265 tc_opts_max:OK #266 tc_opts_mixed:OK #267 tc_opts_prepend:OK #268 tc_opts_query:OK #269 tc_opts_query_attach:OK <--- (new test) #270 tc_opts_replace:OK #271 tc_opts_revision:OK Summary: 20/0 PASSED, 0 SKIPPED, 0 FAILED Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Link: https://lore.kernel.org/r/20231006220655.1653-6-daniel@iogearbox.net Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>

juimonen mentioned this pull request Nov 8, 2018

HDMI output only works on single channel on Up2 #226

Closed

juimonen requested review from plbossart, lgirdwood, singalsu and ranj063 November 8, 2018 14:36

plbossart requested changes Nov 8, 2018

View reviewed changes

juimonen force-pushed the vol_fix branch from 71c7c9c to 92712ef Compare November 8, 2018 17:41

plbossart approved these changes Nov 8, 2018

View reviewed changes

plbossart merged commit 4badf19 into thesofproject:topic/sof-dev Nov 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASoC: SOF: topology: set default values to volume control #258

ASoC: SOF: topology: set default values to volume control #258

juimonen commented Nov 8, 2018

sofci commented Nov 8, 2018

plbossart left a comment

plbossart Nov 8, 2018

singalsu Nov 8, 2018 •

edited

Loading

juimonen Nov 8, 2018

juimonen Nov 8, 2018

plbossart Nov 8, 2018

juimonen Nov 8, 2018

plbossart Nov 8, 2018

juimonen Nov 8, 2018

singalsu Nov 8, 2018 •

edited

Loading

juimonen Nov 8, 2018

plbossart Nov 8, 2018

juimonen Nov 8, 2018

juimonen Nov 8, 2018

plbossart Nov 8, 2018

ranj063 commented Nov 8, 2018

ranj063 commented Nov 8, 2018

juimonen commented Nov 8, 2018

ranj063 commented Nov 8, 2018

juimonen commented Nov 8, 2018

ranj063 commented Nov 8, 2018

juimonen commented Nov 8, 2018

ranj063 commented Nov 8, 2018

plbossart left a comment

plbossart commented Nov 8, 2018

ASoC: SOF: topology: set default values to volume control #258

ASoC: SOF: topology: set default values to volume control #258

Conversation

juimonen commented Nov 8, 2018

sofci commented Nov 8, 2018

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singalsu Nov 8, 2018 • edited Loading

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

singalsu Nov 8, 2018 • edited Loading

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

ranj063 commented Nov 8, 2018

ranj063 commented Nov 8, 2018

juimonen commented Nov 8, 2018

ranj063 commented Nov 8, 2018

juimonen commented Nov 8, 2018

ranj063 commented Nov 8, 2018

juimonen commented Nov 8, 2018

ranj063 commented Nov 8, 2018

plbossart left a comment

Choose a reason for hiding this comment

plbossart commented Nov 8, 2018

singalsu Nov 8, 2018 •

edited

Loading

singalsu Nov 8, 2018 •

edited

Loading