[TEST] Soundwire/multi link support - #1769
Conversation
|
update: I can also see a timeout on a bank switch with multi-link disabled, so something's broken in the sequence. That's good news to some extent, now we can compare with the existing solution in topic/sof-dev |
bardliao
left a comment
bardliao
left a comment
There was a problem hiding this comment.
@plbossart Thanks for starting this PR. It makes sense to me. Just two questions, please see my comments.
|
|
||
| /* | ||
| * Ignore BIOS err_threshold, it's a really bad idea when dealing | ||
| * with multiple hardware synchronized links |
There was a problem hiding this comment.
Sorry, I didn't get why we should not retry commands with multiple hardware synchronized links.
There was a problem hiding this comment.
when you have several devices on the same link, if one of them signals NAK during a bank switch it doesn't happen. It's an atomic decision, either all devices (master + slaves) do a bank switch, or none do.
When you have devices on different links, and one of them signals NAK, we don't have a mechanism to propagate the error from one link to the other. So you will have a bank switch on one link, but not on the other.
If you enable command retries, then the bank switch will be retries, but that will lead to a synchronization issue if the retry succeeds (the playback starts at different times on different links).
So my take is that if we see an error on multi-link case, it's better to stop everything, and retry all links. Not retry at the individual link level.
Even on a single link, the retry is problematic on a bank switch btw, the retry will have to happen for the next SSP, so the bus has no command/control until that happens (with could last 250ms).
Does this clarify my point?
There was a problem hiding this comment.
Yes, thanks for the explanation. :)
| intel_shim_init(sdw, clock_stop); | ||
|
|
||
| return sdw_cdns_init(&sdw->cdns, false); | ||
| return 0; |
There was a problem hiding this comment.
Is this for programming sequence? Sorry, I didn't get why we can't call sdw_cdns_init() here.
There was a problem hiding this comment.
yes, it's poorly explained. The point is that we need to call sdw_cdns_init() with the multi-master argument reflecting what the module parameter is. This is a left-over from the previous implementation which results in first initializating in multi-master==false, then doing a reinitialization later with multi-master== true (if enabled).
There was a problem hiding this comment.
Thanks for the explanation. In short, we don't want to call sdw_cdns_init() until we know the value of multi-master, right?
There was a problem hiding this comment.
it's more that we want to to the Cadence init after we call sync_arm. there's apparently a recommended programming sequence to avoid lockups when enabling the IOs during the inits
|
ok, so now we should be at the same level of functionality as before when multi-master is disabled Still need to figure out why we have a timeout when multi-master is enabled |
|
@plbossart I got bank switch timeout with MMaster enabled. It is ok if SSP sync is not enable for command. I checked SSP count setting. Now it is 0x18 in default, On CML, gsync value = 60000, Frame size = 125*2 = 250, SSP count should be 60000/250 = 240 ? But it made no effect. I checked the SSPSTAT reg(0xC), indicating the current ssp count in master. The value is not correct as I expected. Only LSB 4 bit value changed. |
|
@plbossart I also disabled PM and pulseaudio to make it simple. After booting up, just do playback. |
we always need to send bank switch commands with a ssp_sync tag set, even with no MMaster. minor correction: The value for gsync is 4kHz, the number of clocks is 6000. Your point about the LSB values is interesting, let me look into this. Edit; The SSP counter loops on the lower 8 LSBs, so will wrap around in 256 frames. that's 5.33ms, it'll be really hard to see unless you have a loop to look at the values. |
sorry , I made a mistake for 60000. I found an interesting behavior. If SSP count is set to 1, everything works! I don't know why. |
wow. That's a nice surprise, I'll give it a try. You're truly a hardware wizard @RanderWang ! |
ack, this works for me as well. No idea why either... |
7e98298 to
3084836
Compare
|
update: thanks to @RanderWang we have something to test at last in multi-link mode. The following option is still required options snd-sof-intel-hda-common sdw_clock_stop_quirks=0x2 Since I have not looked at clock stop exit flows. This PR has two hacks we need to sort out tests and comments welcome. |
|
Full test with options snd-sof-intel-hda-common sdw_clock_stop_quirks=0x2, found no other regression. But Still can be reproduced on latest topic/sof-dev+PR1769. Tested other sdw_clock_stop_quirks e.g 0x0/0x1, issue cannot be reproduced. Issue can be reproduced via below scenarios:
|
|
yes, I discussed with @YvonneYang2 and also found issues with sdw_clock_stop_quirks = 0x2, but 0x0 worked. |
|
@YvonneYang2 could you help to test this patch ? I found it work. Just git apply power.txt. Thanks! |
Tested it with patch above, since #1788 blocked the validation in sound setting, only tested scenario 2 in #1769 (comment) description, and issue cannot be reproduced. |
3084836 to
ad5c701
Compare
|
@RanderWang @YvonneYang2 I updated the code with an improved version of the code to keep the links powered-on. But I wonder if this is the right direction. It'd be useful to have a dmesg sequence with the last patch removed that describes the steps. It may very well be that we have two independent bank switches happening at the same time, and that our shim_lock mutex does not cover this sequence and prevent it from happening. Edit: I also added the correct way of programming the SSP interval, the values we were using were illegal :-( |
ad5c701 to
c1856da
Compare
|
Tested it with latest topic/sof-dev+PR1769, issue as #1769 (comment) mentioned is gone. And found no regression. |
Use a single loop to wait for hardware to set/clear fields. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Somehow we inverted the two, align with register definition to avoid further confusion. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
config_update() may time out or cannot be use in ClockStopMode, cover all cases and bail as needed. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
This helps isolate code and align with recommended programming flows Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In multi-master mode, the IP will only accept SSP intervals with integer relationships between the frame rate and the gsync frequency. E.g for a 48kHz frame rate and 4 kHz gsync signal, the SSP interval can only be 1, 2, 3, 4, 6, 12. To simplify we only allow one SSP per gsync interval. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Follow hardware programming flows and add placeholder comment for multi-master mode. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Follow recommended programming sequences, this needs to be enabled before the reset sequence. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The hardware default is 0x1F, and the existing code does an OR with 0xF. This is a no-op, remove. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Enable multi-link (aka multi-master configuration). In this configuration, updatres and commands with the 'ssp_sync' tag will be deferred and controlled by the gsync hardware signal. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
with multiple links synchronized in hardware, retrying commands in software is not recommended. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Refactor code and use same routines on set/clear Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We want to make sure SHIM register fields such as SYNCPRD are only programmed once. Since we don't have a controller-level driver, we need master-level drivers to collaborate: the registers will only be programmed when the first link is powered-up. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The current hard-coded value only works for CNL/WHL/CML platforms, it needs to be updated for IceLake and TigerLake (change from 24 MHz to 38.4 MHz oscillator). In addition, the values needs to be programmed before powering-up the links. Note that the SYNCPRD value is tied only to the XTAL value and not the current bus frequency or the frame rate. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Move code from pre_bank_switch to dedicated helper, will be used in follow-up patches as recommended by programming flows. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
After arming the synchronization, the SYNCGO field controls the hardware-based synchronization between links. Move the programming and wait for clear of SYNCGO to dedicated helper. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The multi-link support is enabled with a hardware gsync signal connecting all links. All commands and operations which typically are handled on an SSP boundary will be deferred further and enabled across all links with the 'syncGo' sequence. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Deal with the BUS_RESET case, which is the default. The only change is to add support for the exit sequence using the syncArm/syncGo mode for the exit reset sequence. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Hardware-based synchronization is typically required when the bus->multi_link flag is set. On Intel platforms, when the Cadence IP is configured in 'Multi Master Mode', the hardware synchronization is required even when a stream only uses a single segment. The existing code only deal with hardware synchronization when a stream uses more than one segment so to remain backwards compatible we add a configuration threshold. For Intel cases this threshold will be set to one, other platforms may be able to use the SSP-based sync in those cases. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
set the flags as required by hardware implementation Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Use platform-specific information to decide when to use hw_sync, not only a number of links > 1. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Detect cases where the clock is assumed to be stopped but the IP is not in the relevant state, and add a dynamic debug trace. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
c1856da to
5de3e57
Compare
|
@YvonneYang2 @RanderWang I pushed an update which works with clock stop usual modes. Please retest. However I could not reproduce the timeouts mentioned earlier, so I reverted the power hack. Please provide more information on how I can reproduce the issue. I tried switching from headset to playback and back, did all kinds of monkey testing, and did not see a problem. Note that the mainline is broken due to the machine driver and topology issues. @RanderWang please fix this asap, this is blocking validation and integration. thanks! |
I can't reproduce the issue with this PR now. I create a PR to fix this issue : #1796 [update] correct a typo |
|
With options snd-sof-intel-hda-common sdw_clock_stop_quirks=0x2/0x8, test with sof-dev(commit:b2d9823)+PR1769+PR1796, below issue still can be reproduced. steps: |
|
With options snd-sof-intel-hda-common sdw_clock_stop_quirks=0x8, tested it on topic/sof-dev+PR1769+PR1796, time out issue mentioned earlier can be reproduced during multipipline stress test. Found no other regression. Update: Log: Run command "./multiplex_pipeline-test_update.sh pcm0p pcm2p pcm5p pcm1c pcm4c" |
Sorry @YvonneYang2 I cannot follow the logic. So which of the two did you mean
Also please stop using clock quirks, we need to use the default. And while I am at it, there are serious issues with this script... |
…g platform device The SoundWire master device pm_runtime resume causes race conditions and errors before the .remove is called, so disable at the parent level. It's not clear why we cannot power up the links on pm_runtime_resume though, this should work. At any rate this will be removed before upstream since we are no longer going to use platform devices. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
While the hardware exposes independent bits to power-up each master, the recommended sequence is to power all links or none. Idle links can still use the clock stop mode while the master is powered. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Each link has separate power controls, but experimental results show we need to use an all-or-none approach to the link power management. This change has marginal power impacts, the DSP needs to be powered anyways before SoundWire links can be powered, and even when powered a link can be in clock-stopped mode. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
5de3e57 to
3144357
Compare
|
update branch-predicting that we have to keep the links powered. |
she reverted the 385d9f5 and didn't get timeout. @YvonneYang2 is this you did as we discussed ?
|
[Yvonne] I think maybe sof*.ldc file should be based on the same master branch commit as sof*.ri file.
Yes! Thanks Rander. What I want to say is that commit 385d9f5 caused timed out issue not fixed it. And issue is gone after removing commit 385d9f5 . Many sorry for the wrong typo. And I will test it with latest PR1769 with no clock quirks today. |
|
Tested it with latest topic/sof-dev+PR1796+PR1769, timed out issue is gone. And found no other regression on cml platform. But on TGL RVP, found a new issue #1806 |
|
ok, let's merge this |
5 participants
This is a WIP branch, for tests and reviews only, rebased on top of my previous work to revert to platform devices
I must still be missing something since the same issue with bank switches happen:
I could however use reviews from @bardliao and @RanderWang to see if I missed anything in the hardware sequences?
I suspect something fishy in the handling of the SYNCPRD values, we should probably only program that register once when the first link is powered-up.Edit: this doesn't seem to helpAlso it'd be useful to re-run validations with multi-master disabled, just to see if I didn't add any regression. the option for this is
options soundwire_intel sdw_md_flags=0x08080808
(#define SDW_INTEL_MASTER_DISABLE_MULTI_MASTER BIT(3))