platform-dependent INFO_ flags #1336

plbossart · 2019-10-17T18:53:11Z

This is an untested but very simple proof-of-concept to illustrate how we can account for platform differences reported to userspace. Currently we have a single setting for all platforms, which e.g does not allow us to notify userspace and other SOF parts of hardware capabilities (precision of the hw pointer position, pause/resume support, etc)

The first patch is just a move of the existing settings, follow-up patches change the settings for Intel platforms based on my understanding of the hardware.

This PR should fix the issue #1285

juimonen · 2019-10-17T19:01:48Z

At first glance looks good, did you try with pulseaudio what happens? I'll try myself tomorrow at office.

plbossart · 2019-10-17T19:06:25Z

On 10/17/19 2:01 PM, juimonen wrote: At first glance looks good, did you try with pulseaudio what happens? I'll try myself tomorrow at office.

No, I didn't try since this is a coffee-break PR, but I know the code will disable tsched when this BATCH flag is set. In theory this has the net effect of only relying on period events. I don't like disabling stuff but I don't see a path to solve this without firmware support.

ranj063 · 2019-10-17T19:57:57Z

@plbossart so whats the difference in behavior when the user pauses audio without INFO_PAUSE and with INFO_PAUSE?

juimonen · 2019-10-18T06:42:41Z

@ranj063 with "aplay -i" you just can't pause..."PAUSE command ignored (no hw support)", so not sure if this is what we want to have?

@plbossart otherwise pulseaudio now works with this one in mb-byt+rt5682

couple of remarks:

Pierre do you have L/R channels swapped in your UI speaker test, because I have?
I have very subtle L/R channel balance differences with different switches for aplay (like using -vvvv or -i), so there's some timing difference in the starting of the stream, which makes the samples corrupt, you have to use "all over the ears" headphone listening to detect this...

dbaluta · 2019-10-18T06:44:13Z

@paulstelian97 I think this will also help with our pause problems :)

juimonen · 2019-10-18T07:18:47Z

@plbossart and I think the channel balance issue is now fixed with thesofproject/sof#1922, I had couple of days old fw.

juimonen · 2019-10-18T07:30:36Z

https://github.com/orgs/thesofproject/teams/sof-developers/discussions/25

plbossart · 2019-10-18T11:43:51Z

@juimonen PulseAudio does not use PAUSE_PUSH/RELEASE, neither does CRAS. It's also questionable if pausing without a ramp (or not on zero-crossing) is good in terms of pop/clicks. Not to mention the horrible error messages that ALSA throws at us when doing a PAUSE_RELEASE after a suspend-resume operation.

We could make this optional with a Kconfig if people really need it, but I don't think anyone would complain. It think the one thing we can all agree on is that we should not make PAUSE the default for all platforms.

juimonen · 2019-10-18T12:24:18Z

@plbossart, yes pause thing is fine by me, just tried to answer somehow ranjani's question. I don't know what our CI will think of this? don't we have some pause release tests? or maybe they are done manually...

plbossart · 2019-10-18T13:58:10Z

update: I removed the INFO_PAUSE removal for now, we need to track this as a 'feature'

Was approved by mistake

ranj063 · 2019-10-20T03:23:11Z

@plbossart there're typos in the second commit "position" and reliable

RanderWang · 2019-10-22T05:43:13Z

@plbossart there're typos in the second commit "position" and reliable

good eye! I only found it by a tool.

juimonen

I will approve this now, because this will "fix" the stuttering playback in byt. @plbossart just fix the typos and let's just merge (?). Only thing I'm wondering do we need some more testing on other platforms?

plbossart · 2019-10-22T15:34:17Z

@plbossart there're typos in the second commit "position" and reliable

well spotted, somehow codespell doesn't seem to work any longer? Fixed now

plbossart · 2019-10-22T15:35:13Z

I will approve this now, because this will "fix" the stuttering playback in byt. @plbossart just fix the typos and let's just merge (?). Only thing I'm wondering do we need some more testing on other platforms?

I don't think we need to test further, this PR does not affect any HDaudio-based platforms.

ranj063

@plbossart this might be a good time to try my script. You'd definitely have permissions to push to your own branch.

plbossart · 2019-10-22T17:06:16Z

@ranj063 I know I am a goat at times.... I typed a control C by accident, and running the script twice adds the signature twice - this should be made idiot-proof. Also Jaska's information is invalid?

 Signed-off-by: Pierre-Louis Bossart <[email protected]>
    Reviewed-by: null <[email protected]>
    Reviewed-by: Ranjani Sridharan <[email protected]>
    Reviewed-by: null <[email protected]>
    Reviewed-by: Ranjani Sridharan <[email protected]>

ranj063 · 2019-10-22T17:10:04Z

@ranj063 I know I am a goat at times.... I typed a control C by accident, and running the script twice adds the signature twice - this should be made idiot-proof. Also Jaska's information is invalid?
 Signed-off-by: Pierre-Louis Bossart <[email protected]>
    Reviewed-by: null <[email protected]>
    Reviewed-by: Ranjani Sridharan <[email protected]>
    Reviewed-by: null <[email protected]>
    Reviewed-by: Ranjani Sridharan <[email protected]>

@plbossart thanks for trying. Yes, for sure it needs a bit more testing with the corner cases but looks like everyone needs to update their profiles too. I will fix the script to ignore the null name when there's no name in the profile.

Currently the INFO_ flags such as PAUSE/NO_PERIOD_WAKEUP are defined in the SOF PCM core, which doesn't scale. To account for platform variations, these flags need to be set in DSP ops. This patch only moves the definitions and does not change any functionality. Reviewed-by: Jaska Uimonen <[email protected]> Reviewed-by: Ranjani Sridharan <[email protected]> Signed-off-by: Pierre-Louis Bossart <[email protected]>

The current position update is not precise enough for PulseAudio to work reliably with the timer-based scheduling on Baytrail, Cherrytrail, Broadwell. Disable the NO_PERIOD_WAKEUP capability and use BATCH to signal that the position is only reliable and updated during period_elapsed events. This will be reverted when the firmware provides a more accurate position for those platforms. Reviewed-by: Jaska Uimonen <[email protected]> Reviewed-by: Ranjani Sridharan <[email protected]> Signed-off-by: Pierre-Louis Bossart <[email protected]>

plbossart · 2019-10-22T17:17:46Z

Also we seem to lose the approved status by force-pushing a new branch, so now we have the information in the commit message but not in GitHub any longer :-(

ranj063 · 2019-10-22T17:35:55Z

Also we seem to lose the approved status by force-pushing a new branch, so now we have the information in the commit message but not in GitHub any longer :-(

@plbossart this is because our repo settings dismiss reviews when the branch is updated. Can we please change this?

plbossart · 2019-10-22T17:38:24Z

Also we seem to lose the approved status by force-pushing a new branch, so now we have the information in the commit message but not in GitHub any longer :-(

@plbossart this is because our repo settings dismiss reviews when the branch is updated. Can we please change this?

not sure, it's a good feature to have.

ranj063 · 2019-10-22T17:42:26Z

not sure, it's a good feature to have.

@plbossart in general one person may agree with a solution and another may not. So if someone approves a PR and the other person "requests changes", it will automatically be flagged to not be merged. So I'm not sure if dismissing reviews automatically is a good feature or not.

BugLink: https://bugs.launchpad.net/bugs/1819515 [ Upstream commit 87c11f1 ] Similar to commit 44f49dd ("ipmr: fix possible race resulting from improper usage of IP_INC_STATS_BH() in preemptible context."), we cannot assume preemption is disabled when incrementing the counter and accessing a per-CPU variable. Preemption can be enabled when we add a route in process context that corresponds to packets stored in the unresolved queue, which are then forwarded using this route [1]. Fix this by using IP6_INC_STATS() which takes care of disabling preemption on architectures where it is needed. [1] [ 157.451447] BUG: using __this_cpu_add() in preemptible [00000000] code: smcrouted/2314 [ 157.460409] caller is ip6mr_forward2+0x73e/0x10e0 [ 157.460434] CPU: 3 PID: 2314 Comm: smcrouted Not tainted 5.0.0-rc7-custom-03635-g22f2712113f1 thesofproject#1336 [ 157.460449] Hardware name: Mellanox Technologies Ltd. MSN2100-CB2FO/SA001017, BIOS 5.6.5 06/07/2016 [ 157.460461] Call Trace: [ 157.460486] dump_stack+0xf9/0x1be [ 157.460553] check_preemption_disabled+0x1d6/0x200 [ 157.460576] ip6mr_forward2+0x73e/0x10e0 [ 157.460705] ip6_mr_forward+0x9a0/0x1510 [ 157.460771] ip6mr_mfc_add+0x16b3/0x1e00 [ 157.461155] ip6_mroute_setsockopt+0x3cb/0x13c0 [ 157.461384] do_ipv6_setsockopt.isra.8+0x348/0x4060 [ 157.462013] ipv6_setsockopt+0x90/0x110 [ 157.462036] rawv6_setsockopt+0x4a/0x120 [ 157.462058] __sys_setsockopt+0x16b/0x340 [ 157.462198] __x64_sys_setsockopt+0xbf/0x160 [ 157.462220] do_syscall_64+0x14d/0x610 [ 157.462349] entry_SYSCALL_64_after_hwframe+0x49/0xbe Fixes: 0912ea3 ("[IPV6] MROUTE: Add stats in multicast routing module method ip6_mr_forward().") Signed-off-by: Ido Schimmel <[email protected]> Reported-by: Amit Cohen <[email protected]> Signed-off-by: David S. Miller <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]> Signed-off-by: Thadeu Lima de Souza Cascardo <[email protected]>

plbossart requested review from dbaluta, juimonen, ranj063, bardliao, keyonjie and RanderWang October 17, 2019 18:53

plbossart requested review from lgirdwood and lyakh as code owners October 17, 2019 18:53

plbossart force-pushed the fix/hw_info branch from bf769e3 to 7516d95 Compare October 17, 2019 18:54

plbossart force-pushed the fix/hw_info branch from 7516d95 to c3f966b Compare October 18, 2019 13:57

plbossart changed the title ~~[RFC] platform-dependent INFO_ flags~~ platform-dependent INFO_ flags Oct 18, 2019

plbossart mentioned this pull request Oct 18, 2019

[INTEL] Remove support for INFO_PAUSE #1339

Closed

ranj063 previously approved these changes Oct 19, 2019

View reviewed changes

juimonen previously approved these changes Oct 22, 2019

View reviewed changes

plbossart dismissed juimonen’s stale review via 45bab9a October 22, 2019 15:33

plbossart force-pushed the fix/hw_info branch from c3f966b to 45bab9a Compare October 22, 2019 15:33

plbossart requested review from juimonen and ranj063 October 22, 2019 15:34

juimonen previously approved these changes Oct 22, 2019

View reviewed changes

ranj063 previously approved these changes Oct 22, 2019

View reviewed changes

plbossart dismissed stale reviews from ranj063 and juimonen via 2009a58 October 22, 2019 17:11

plbossart force-pushed the fix/hw_info branch from 45bab9a to 2009a58 Compare October 22, 2019 17:11

plbossart added 2 commits October 22, 2019 12:15

plbossart force-pushed the fix/hw_info branch from 2009a58 to e6cc243 Compare October 22, 2019 17:16

plbossart merged commit c105b6f into thesofproject:topic/sof-dev Oct 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

platform-dependent INFO_ flags #1336

platform-dependent INFO_ flags #1336

plbossart commented Oct 17, 2019

juimonen commented Oct 17, 2019

plbossart commented Oct 17, 2019 via email

ranj063 commented Oct 17, 2019

juimonen commented Oct 18, 2019

dbaluta commented Oct 18, 2019

juimonen commented Oct 18, 2019

juimonen commented Oct 18, 2019

plbossart commented Oct 18, 2019

juimonen commented Oct 18, 2019

plbossart commented Oct 18, 2019

ranj063 commented Oct 20, 2019 •

edited

Loading

RanderWang commented Oct 22, 2019

juimonen left a comment

plbossart commented Oct 22, 2019

plbossart commented Oct 22, 2019

ranj063 left a comment

plbossart commented Oct 22, 2019 •

edited

Loading

ranj063 commented Oct 22, 2019

plbossart commented Oct 22, 2019

ranj063 commented Oct 22, 2019

plbossart commented Oct 22, 2019

ranj063 commented Oct 22, 2019

platform-dependent INFO_ flags #1336

platform-dependent INFO_ flags #1336

Conversation

plbossart commented Oct 17, 2019

juimonen commented Oct 17, 2019

plbossart commented Oct 17, 2019 via email

ranj063 commented Oct 17, 2019

juimonen commented Oct 18, 2019

dbaluta commented Oct 18, 2019

juimonen commented Oct 18, 2019

juimonen commented Oct 18, 2019

plbossart commented Oct 18, 2019

juimonen commented Oct 18, 2019

plbossart commented Oct 18, 2019

ranj063 commented Oct 20, 2019 • edited Loading

RanderWang commented Oct 22, 2019

juimonen left a comment

Choose a reason for hiding this comment

plbossart commented Oct 22, 2019

plbossart commented Oct 22, 2019

ranj063 left a comment

Choose a reason for hiding this comment

plbossart commented Oct 22, 2019 • edited Loading

ranj063 commented Oct 22, 2019

plbossart commented Oct 22, 2019

ranj063 commented Oct 22, 2019

plbossart commented Oct 22, 2019

ranj063 commented Oct 22, 2019

ranj063 commented Oct 20, 2019 •

edited

Loading

plbossart commented Oct 22, 2019 •

edited

Loading