Repository navigation
[BUG] step up in output volume on Thinkpad X1 Carbon 7th #3072
Description
Describe the bug
When playing some streams on the Thinkpad X1 Carbon 7th, the volume level is noticeably (~50%?) lower for the first ~0.5s.
To Reproduce
I can hear it by playing the following sample via aplay:
aplay -Dhw:0,0 output.wav
The second bell ring sounds louder. Opening the file in ex. audacity shows that in fact the two bell rings are of the same amplitude.
Reproduction Rate
Reproduces all the time when playing directly through alsa.
Results are mixed when playing through pulseaudio. The volume step up is sometimes there, sometimes not; I don't know why.
It seems to me that this occurs more frequently with sof firmware v1.5 than v1.4.3 but when alternating between the two by changing the files under /lib/firmware and unloading/reloading kernel modules, I can sometimes reproduce the issue with v1.4.3 as well. v1.5.1 is the same as v1.5 in this regard (also bad).
This seems to occur regardless of mixer levels.
It occurs in headphones and speakers.
Expected behavior
Constant sound level.
Impact
annoyance, in daily use it doesn't seem to occur with pulseaudio output if I boot with v1.4.3 sof firmware and don't touch it afterwards.
Environment
-
Branch name and commit hash of the 2 repositories: sof (firmware/topology) and linux (kernel driver).
- Kernel: Debian unstable 5.6.0-2-amd64
- SOF: stable-v1.4.3/stable-v1.5.1 (from sof-bin repository)
-
Name of the topology file
- Topology: sof-tplg/sof-hda-generic-4ch.tplg sof/sof-cnl.ri
-
Name of the platform(s) on which the bug is observed.
- Platform: Realtek ALC285 vendor 0x10ec0285 codec SSID 0x17aa2293
Activity
lgirdwood commentedon Jun 22, 2020
@singalsu any comments ?
singalsu commentedon Jun 22, 2020
@lgirdwood @gobenji Thanks for reporting! The volume linear ramp is 250 ms long and the first "beep" in this sound clip about the same length so large amount of it, e.g. first 100 ms, get a significant attenuation. The ramp was made long to avoid zipper noise (higher frequency rattle while adjusting volume). There has been also need to conceal noise sounds in the beginning but it's more a capture issue.
But since we have now the zero crossings detector timed volume ramp it should be safe to shorten a lot the ramp. I'll test it how it sounds and if OK I'll create a PR for topology.
singalsu commentedon Jun 25, 2020
I created PR #3098 to address this. The topology change only works with v1.5.1 firmware but there can be some zipper noise rattle when the ramp is active. I can hear it with test signals easily, depending on music content less easily. But the beep beep notification becomes fully audible from beginning.
But the fixes for ramp rate adjustment and improvements to fractional time computation below ms granularity need new firmware build.
plbossart commentedon Jun 25, 2020
@singalsu we may want to be careful here in terms of user-experience.
When playing with PulseAudio, the sink remains open for 5s before being closed. So if a beep is send while the stream is active but sending zeroes, we would have no ramp in the firmware. We will however have a ramp when the sink is inactive and the beep makes it active.
Conversely, when playing with ALSA we will always have a ramp since we close immediately.
I am starting to wonder if the ramp duration and its activation should not be controlled by ALSA controls?
Adding @kv2019i and @juimonen for comments on PulseAudio usage.
singalsu commentedon Jun 25, 2020
@plbossart The ALSA audio open related fade-in ramp helps to avoid pop noise from codec in playback so this simple logic works for that. Pulse likely does also ramps so FW stream start ramp could be avoided for playback. But we don't have separate ramp duration parameters for start and normal volume adjust so some additional controls/parameters could be useful. Good to discuss to get this optimal for Pulse and CRAS usage. Bare ALSA is marginal use case.
gobenji commentedon Jun 25, 2020
Just to provide some context, in my case I noticed this problem because I use this bell sound for the terminal bell via pulseaudio:
Because of the ramp-up, the first terminal bell after the device is idle sounds noticeably weaker than a closely following bell. I find that really annoying from a UX PoV. I usually test it by simply openning an empty terminal with bash and pressing
<tab>a few times.As a workaround, I've edited
/etc/pulse/default.paand commented outload-module module-suspend-on-idle.singalsu commentedon Jun 30, 2020
The PR #3098 is merged now to git master. Successive FW releases will contain this fix.
@gobenji If you'd like to test you could build the topologies (sof-hda-generic-4ch.tplg) from git master and use it with v1.5.1 firmware. The difference to git master is that the ramp of 20 ms length is stepped in 20 ms that makes it sound a bit harsh. With the updated firmware the short ramp would use 125 us update rate. Whether the slightly harsh sounding beginning is an issue or not is quite subjective.
singalsu commentedon Jun 30, 2020
I created a zip file with testbench simulations of volume component that demonstrates the effect of tplg and fw updates in the pull request.
alert_tone_volume_processed.zip
Here's screenshots of the wav files with spectrograms shown:
The reported issue shows well in the waveform. The ramp harshness with just topology updated may not be fully exposed by this demo. When playing back music muting and unmuting can make some rattle sound at times and be annoying. With both updated tplg and fw algorithm the perceived use case quality is good.
gobenji commentedon Jul 5, 2020
@singalsu Thanks for tackling this problem so quickly. I tried to test the changes but the result was unsucessful.
Using the instructions from https://thesofproject.github.io/latest/getting_started/build-guide/build-with-docker.html I built topology files using the upstream docker image (thesofproject/sof). However, after installing sof-hda-generic-4ch.tplg into /lib/firmware and rebooting, I have no sound output at all.
The audio device finishes probing and appears in pulseaudio, however I see the following in dmesg:
I tried this from sof.git HEAD (882875f). I also tried it from v1.5.1 tag (which corresponded with the sof-cnl.ri file in /lib/firmware at that moment) + a cherry-pick of commit 673e335 ("Tools: Topology: Change playback volume ramp duration to 20 ms"). The result was the same as above.
I don't think this is a problem when building because if I build from the v1.5.1 tag, I get exactly the same tplg file as in sof-bin:
Please let me know if you have some advice.
BTW, in order to be able to use the upstream docker image, I have to apply the following small diff
otherwise, it uses
lgirdwood commentedon Jul 14, 2020
@gobenji thanks for docker fix, @xiulipan do we have a place for merging docker fixes ?
Btw @singalsu is on vacation.
14 remaining items
gobenji commentedon Oct 12, 2020
Regarding sof-logger, maybe I should also mention that when I run it and the output is empty, the tool exits right away. Here is an strace of sof-logger running as described previously, in case that's of any use: output.txt
This is on a debian 5.8.0-2-amd64 kernel (based on stable v5.8.10)
I've also double-checked that there is no
tracefile available:plbossart commentedon Oct 12, 2020
@gobenji it's always interesting to see error reports, even if they are classified as 'normal' later. We appreciate the time you spent and information provided.
If this happens with the legacy driver as well, it could very well be some ramp applied in the amplifier. I would test with a constant amplitude sine wave and see what the envelope looks like. If we see a ramp done at that level, then maybe we need to disable volume ramps in the SOF code, that'd be an interesting behavior to provide to users. @singalsu FYI.
singalsu commentedon Oct 13, 2020
The ramp can be disabled in topology per PGA instance by setting the token SOF_TKN_VOLUME_RAMP_STEP_MS to zero. Though that would disable the ramp from all volume transitions too. I wonder if a minor ABI version bump would allow to add token for stream start unmute ramp enable/disable or a separata length token for it (zero for disable).
plbossart commentedon Oct 13, 2020
@singalsu if the firmware does any sort of volume changes then I think the firmware needs to apply the relevant ramps. Disabling ramps during firmware-controlled volume changes isn't something the amps or codec can compensate for.
It's more when the volume is 0dB that we should consider disabling ramps. The firmware does nothing and hands-over the start/stop ramp management to the external device.
Am I making any sense or do I need another coffee :-)?
singalsu commentedon Oct 13, 2020
@plbossart Yes, makes sense. I think pulseaudio tries to use FW volume controls when available vs. using internal stream gain so the scenario of only 0 dB gain in use is not something to assume for playback. What do you think about adding token to separately control the stream start unmute ramp if know that the playback codec does the same thing (now tied to volume adjust ramp setting)?
plbossart commentedon Oct 13, 2020
I think that would be a good addition @singalsu. In general everything that an external chip might do should be exposed with a 'disable' control. Now we also want to be careful with the logic, in ALSA controls something is on or off, we don't do disable=1, that should be enable=0.
juimonen commentedon Oct 13, 2020
Hi, I get this also with my x1. You can get it in ubuntu just by hitting the backspace in terminal to get the "ping" notification. It happens only when the DSP goes to D3, you can see it in dmesg. If you hit the ping again before D3, there isn't any slow ramping. So this is related to dsp waking up, and I guess then the codec itself making some extra ramps...
gobenji commentedon Oct 15, 2020
I've tested with a 1s 440HZ sine wave sample:
With the Thinkpad X1 line out, ~62-76ms of the sample is clipped (at the beginning I guess..), then there is a ~12ms ramp up. This is similar when using the legacy driver or the sof driver. In that screenshot it looks like the beginning of the sof driver sample (#3) is not aligned correctly; in fact it's aligned according to the end of the sample.
For comparison, I repeated the same test with the line out of the Thinkpad Thunderbolt 3 dock which hosts a usb sound card (USB id 17ef:3083). With that device, ~17ms of the sample is clipped and there is only minimal ramp up.
With the X1 line out, when playing the same sample a second time in close succession, the duration was as expected (1s) - nothing was clipped - but there was still a ramp up (~19ms this time).
In the case of the usb adapter, the behavior was the same when playing a second sample in close succession; 17ms was still clipped.
audacity project with recordings
In summary, each device has different quirks... The main takeaway in this context is that using sof doesn't seem to make a difference for this issue.
gobenji commentedon Oct 15, 2020
How do you see the state of the dsp in dmesg?
I tried to disable runtime PM for the device but it did not avoid the step up.
I can see that the device remains "active" according to runtime_status, however the beginning of a sample is still noticeably clipped if I wait more than ~1s between playing two samples.
As mentioned in a previous comment, a workaround for this problem when using pulseaudio is to avoid using module-suspend-on-idle.
juimonen commentedon Oct 15, 2020
@gobenji
I have this in
cat /etc/modprobe.d/sof-dyndbg.conf
options snd_sof_intel_byt dyndbg=+p
options snd_sof_intel_bdw dyndbg=+p
options snd_sof_intel_ipc dyndbg=+p
options snd_sof_intel_hda_common dyndbg=+p
options snd_sof_intel_hda dyndbg=+p
options snd_sof dyndbg=+p
options snd_sof_pci dyndbg=+p
options snd_sof_acpi dyndbg=+p
options snd_sof_of dyndbg=+p
options snd_sof_nocodec dyndbg=+p
options soundwire_bus dyndbg=+p
options soundwire_generic_allocation dyndbg=+p
options soundwire_cadence dyndbg=+p
options soundwire_intel_init dyndbg=+p
options soundwire_intel dyndbg=+p
probably too much lines there, but anyway you should see sof logs in dmesg. At least with me it
says DSP goes to D3 and then I get these low volumes when waking up. I mean it is the driver reporting, so I can't be totally sure about if DSP is really in suspend, but let's assume so (so eventually this means that codec goes to suspend).
So I think it doesn't matter who makes the codec suspend, it can be pulseaudio (suspend-on-idle disabled -> alsa devices closed -> dsp&codec go to suspend after some delay), or I think there's some delay in the driver for PM to kick in also (I think usual amount is 2s). So depending on the driver PM delay, you could probably reproduce/not reproduce this with aplay also.
juimonen commentedon Oct 15, 2020
@gobenji I think this is annoying issue, we should really ask the codec vendor is this wake up ramp necessary and can we do something about it. Probably it is there because of some clicks/pops when powering the amps, but maybe it could be shorter...
plbossart commentedon Oct 15, 2020
this will only change the PCI device status, and we know the ramp is applied by the external codec. I would try to go one level deeper:
Indeed that will keep zeroes flowing into the amp. That's a bit overkill but that's classic. We use similar solutions for keep HDMI receivers awake and remove ramps on startup.
gobenji commentedon Oct 22, 2020
After enabling the dynamic debug messages, I also see messages like the following around the time I'm playing samples:
When doing
like before, I see that the device stays in D0I0, but still, if I wait long enough (~ >2s) in between samples, there's the ramp up like mentioned earlier.
Interestingly, when leaving the power control to auto, I can get the following sequence of events:
So the step up is not directly related to the power state shown by these messages I think.
@plbossart Correct! On my machine it's
and this indeed prevents the issue.
I had to fiddle quite a bit to come up with a functional udev rule to apply this setting automatically. The following rule seemed straightforward
however it was not effective. udev logs that the write completes successfully however it seems to be reset later on, not by udev.
This rule matches on /devices/pci0000:00/0000:00:1f.3/skl_hda_dsp_generic/sound/card0 which generates events later than the hdaudio device. It seems to do the job so far and I'm now using this workaround instead of changing the pulseaudio config.
plbossart commentedon Oct 22, 2020
Fascinating, I didn't think of using udev to alter the power management behavior. @kv2019i @ranj063 FYI.