Repository navigation
[BUG] v1.5 - Digital Microphone issues #2937
Description
Issues:
- the controls were (again!) renamed from "Dmic0 Capture Volume" to "Dmic0 10 Capture Volume" ?
- the dmic volume is weaker than with 1.4.2
Control settings:
numid=49,iface=MIXER,name='Dmic0 10 Capture Switch'
; type=BOOLEAN,access=rw------,values=2
: values=on,on
numid=48,iface=MIXER,name='Dmic0 10 Capture Volume'
; type=INTEGER,access=rw---R--,values=2,min=0,max=70,step=0
: values=70,70
| dBscale-min=-50.00dB,step=1.00dB,mute=1
numid=51,iface=MIXER,name='Dmic1 Capture Volume'
; type=INTEGER,access=rw---R--,values=2,min=0,max=70,step=0
: values=70,70
| dBscale-min=-50.00dB,step=1.00dB,mute=1
Testing sof-bin - v1.5 branch...
The peak is only ~1% for my voice with this setting using the arecord test:
arecord -vvv -f dat -c 4 -D hw:0,6 /tmp/a.wav
Activity
juimonen commentedon May 14, 2020
@perexg, crap, this is clear bug from us... We obviously have still issues testing the full stack from kernel to user space, dmic being the hard one. It is caused by firmware commit d65495e. This needs to be fixed.
Before we might have had also amplifying eq parameters so you could have better volume then, @singalsu FYI.
singalsu commentedon May 14, 2020
@perexg What device model is this and what topology name does the kernel load? We seem to have a confusion with gains, sorry for that. Also does the above indicate that you have already maximized the available user volume control for capture (70. 70)?
kv2019i commentedon May 14, 2020
FYI @lgirdwood @mengdonglin @fredoh9
This seems to be a regression with this patch:
#2660
The patch changed ALSA mixer controls which our UCMs rely on:
https://github.com/alsa-project/alsa-ucm-conf/blob/master/ucm2/sof-hda-dsp/HiFi.conf
... I think this calls for a 1.5 hotfix. Can you @mengdonglin check how this passed validation?
perexg commentedon May 14, 2020
HW: It's Lenovo Carbon X1 7th gen (sof-cnl.ri + sof-hda-generic-idisp-4ch.tplg).
Volumes: Yes, I tried to increase the appropriate volume controls to max - if you have another ideas, I can try them.
juimonen commentedon May 14, 2020
@fredoh9 can you take a look at this? If I remove the pipeline numbering "index" values cause issues with alsatplg. Old topologies before your commit had dmic indexes close to each other, but now when the dmic0 and dmic1 index are separated far away, alsatplg can't compile same name mixer elements...
@ranj063 you have any idea how this should work? And what's the meaning of index values in topology? I understood it was grouping the widgets together, but now I'm puzzled...
juimonen commentedon May 14, 2020
hmm, I think the change was made because we had 2 "Capture Volume" control definitions (name identical) for Dmic0 and Dmic1 in conf file. I think ASoC layer is combining these for alsamixer as, so we get nice "Dmic0 Capture Volume".
But as I now see the alsa-lib conf parsing code, it is actually merging the "duplicate" definitions, so we have in essence only 1 "Capture Volume" definition. It was working before, because the Dmic0 and Dmic1 "index" definition we're close to each other (something like 7 & 8) and alsa conf parsing survives this for some reason. So it finds the single "Capture Volume" for both Dmic0 and Dmic1. Now with change in 1.5 fw the indexes are far apart (like 3 vs 8) and alsa conf parser bails out. And that's probably @fredoh9 was adding the number to the control name.
I'm little bit confused how this index thing would work before and not anymore...
Anyway it seems that if we want to have 2 separate pga's with 2 separate controls, they all need to have different name. This however makes the naming little bit funny as we would get:
"Dmic0 Capture Volume 0" and "Dmic1 Capture Volume 1". You can replace the last number with foo/bar, but this kind of explains why we have pga monster names in the topologies.
If I add a ?-mark like following to the 2 section names:
SectionControlMixer.?"Capture Volume"
I get the alsatplg to create the tplg binary with 2 capture volumes and the indexing doesn't matter, but I'm not sure about the semantics of the ?-sign here...
It says "? = do not override" in alsaconf docs, but not sure it affects as we want here.
@perexg you have any insights of the above behavior, or any preference about the naming?
perexg commentedon May 14, 2020
Sorry, I don't get what you're doing with the alsa-lib configs. Could you show me the pieces you refer (from the .conf files not .m4 files)?
juimonen commentedon May 14, 2020
@perexg sorry for being confusing... I'm just trying to understand how the conf parser works and I need to jump between alsa-utils and alsa-lib.
Attached a conf where I removed the volume control numbering (working as the old way) and it doesn't compile with alsatplg. I renamed the conf to txt as you can't attach otherwise
it compiles.
testi.txt
perexg commentedon May 14, 2020
You can imagine the configuration like a tree. You have this in your config:
The result after load is:
If I normalize your unreadable configs using
alsatplg -n t.conf -o n.confthe problem is visible:There is no 'index 3' section for 'Capture Volume', because configs get merged (identical identifier "Capture Volume" in the configuration tree) - last parsed entry wins in this case.
The 'Dmic0' in SectionWidget has 'index 3', thus you need to define the SectionControlMixer controls with this 'index 3'.
The '?' or '-' operations (?='replace' or minus='remove before parse') won't help you, because you cannot link blocks from the SectionWidget to SectionControlMixer with different indexes (functional groups).
Just choose a different name for "Capture Volume" for Dmic1 like "Second Capture Volume". It's ugly, but you're in trouble because nobody in ASoC use the right ALSA control indexes (do not mix with the functional topology group). ALSA allows multiple controls with the same name with the only integer index difference like "Capture Volume,0" and "Capture Volume,1". Another way is to put Dmic0 and Dmic1 mixer controls to the one functional group (index).
I would really suggest to add the proper support for the ALSA mixer control indexes to ASoC and topology.
perexg commentedon May 14, 2020
Thinking more: Perhaps, it may be sufficient to add a suffix to the control mixer name specified in SectionControlMixer and strip it in the topology library when the contents is written to the output binary file. Like "Capture Volume|Dmic0" where "|Dmic0" will be stripped. If I understand correctly, the identical mixer control names may exist if they're in different functional groups, right?
juimonen commentedon May 15, 2020
@perexg, Yes the idea was exactly that we would have same named controls, but in different indexes. So the prefixing pga name would be the differentiator. But as you said, with current methods, to get this right, we need to have different control names also.
Anyway this current method of attaching pipeline id to control name is bad, because it changes all the time (or can change). So it needs to be changed now to some static identifier, but that requires changes in UCM configs also.
juimonen commentedon May 15, 2020
I made now this:
#2942
So Dmic0 name remains as it was and Dmic1 is indexed. I think we don't currently reference Dmic1 in upstream UCM's so UCM's should work with this....
perexg commentedon May 15, 2020
It's just a workaround. I would really prefer a fix. Some thoughts:
the SectionControlMixer controls are static, so the name should be unique and adding a next extra prefix in the driver seems like a big mistake (why not define directly 'Dmic0 Volume Control') ?
allow private instantions of the control mixer elements for DAPM widget (enhance the config syntax and improve the alsa-lib's topology code)
8 remaining items
juimonen commentedon May 20, 2020
@Liviali155 please use together the 2 PR's from @singalsu and me:
#2942 and #2949. Together these should fix naming and volume.
Liviali155 commentedon May 20, 2020
After applied with #2949,volume still has issue when arecord via dmic(0,7)
juimonen commentedon May 20, 2020
@Liviali155 ok this is as it supposed to be. We left the 16khz pipeline volume intentionally without eq boost as the 16khz pipe is not referred in upstream UCM's currently. @singalsuo FYI.
singalsu commentedon May 20, 2020
@juimonen @Liviali155 @lgirdwood I realized afterwards that it would be possible to increase the loudness of 16 kHz PCM. There's a separate macro for it so it's possible without conflicts. The macros system that was introduced is not yet complete but this seems possible. Maybe it should be done to avoid confusion. I'll do that change for next non-RFC version.
Thanks for testing this!
singalsu commentedon May 20, 2020
@Liviali155 There's now PR #2961. Can you please check that it gives similar capture level for 16 kHz DMIC as the just merged #2949 change for 48 kHz did.
Liviali155 commentedon May 21, 2020
Tried with PR #2961 on Matins and TGL HDA RVP, it actually gives similar capture level for 16kHz DMIC as the just merged #2949 change for 48 kHz did.
perexg commentedon May 26, 2020
Could you release 1.5.1 in sof-bin with this fix?
lgirdwood commentedon May 27, 2020
@lbetlej @fkaczmar @mengdonglin can we do a v1.5.1 next week ?
hsanjuan commentedon May 31, 2020
I was going to open an issue about the lack of signed binaries for v1.5.0, but seeing this I guess I'll wait. Would it be possible to get signed binaries with this v1.5.1? Sound does not work with v1.4.2 and the latest kernels.
ClarexZhou commentedon Jun 11, 2020
fw v1.5.1 is released, find binaries from https://github.com/thesofproject/sof-bin/tree/stable-v1.5.1
lgirdwood commentedon Jun 11, 2020
@perexg fyi
slawblauciak commentedon Jun 16, 2020
Fixes merged and released. Closing.