alsa-project / alsa-ucm-conf

ALSA Use Case Manager configuration
BSD 3-Clause "New" or "Revised" License
72 stars 209 forks source link

rt715-sdca soundwire micphone record no sound #255

Closed KaiChuan-Hsieh closed 1 year ago

KaiChuan-Hsieh commented 1 year ago

On linux kernel 6.0, with rt715-sdca codec on Dell platform.

The internal mic can't record sound after boot. Try $ amixer cset name='PGA5.0 5 Master Capture' 1

Then the internal mic can record sound successfully.

Upload alsa-info for reference, alsa-info.txt.G6EnxHQlnM.zip

plbossart commented 1 year ago

I don't understand how it's possible. This control is not modified by UCM, you might have something in your system that modified the default values.

bardliao commented 1 year ago

I don't understand how it's possible. This control is not modified by UCM, you might have something in your system that modified the default values.

@plbossart Maybe the value is off by default?

plbossart commented 1 year ago

No, our PGAs are always on @bardliao. There must have been an intermediate step or distro configuration that saves the alsa-state to an incorrect value.

KaiChuan-Hsieh commented 1 year ago

@plbossart

Hello,

I did the fresh install, at the time asound.state hasn't created. May I know if the default value applied after tplg loaded, and the default value is inside the tplg file? because I can't find it's setting in kernel driver.

plbossart commented 1 year ago

@KaiChuan-Hsieh check with amixer -Dhw:0 controls if you have this PGA control and the numid, then use amixer -Dhw:0 cget numid=value to see what the value is.

The default in the topology is 0dB.

KaiChuan-Hsieh commented 1 year ago

@plbossart

which contents value you'd like me to get? All PGA related contents? By the way, it is RPL platform with rt715-sdca driver, I install the same image on Dell XPS 9320, which has the same codec, and the value of 'PGA5.0 5 Master Capture Switch' is on after installation. Is there any difference between sof-{adl,rpl}-rt1316-l12-rt714-l0.tplg? My sof-adl-rt1316-l12-rt714-l0.tplg is v2.1, sof-rpl-rt1316-l12-rt714-l0.tplg is v2.2.3.

plbossart commented 1 year ago

there is no difference in the topologies.

amixer cget name='PGA5.0 5 Master Capture' will tell you what the default is

perexg commented 1 year ago

Note that there was a problem in the kernel code which changed the default values from topologies. Not sure if your 6.0.0-9009-oem kernel is affected. Try the latest upstream kernel at first.

andychi117 commented 1 year ago

Compared the file https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/intel/common/soc-acpi-intel-rpl-match.c and https://git.launchpad.net/~canonical-kernel/ubuntu/+source/linux-oem/+git/jammy/tree/sound/soc/intel/common/soc-acpi-intel-rpl-match.c?h=oem-6.0-next. There are no difference. Could you also let me know which commit you're referring to fix the problem which changed the default value? Upload the asound.state with internal microphone working machine and internal microphone non-working machine. asound-no.state.txt asound-no.state.txt

bardliao commented 1 year ago

@KaiChuan-Hsieh

I will see below log

[    6.561437] snd_sof_switch_put name PGA2.0 2 Master Capture Switch value 0
[    6.561452] snd_sof_switch_put name PGA5.0 5 Master Capture Switch value 0
[    6.582346] snd_sof_switch_put name PGA2.0 2 Master Capture Switch value 0
[    6.582361] snd_sof_switch_put name PGA5.0 5 Master Capture Switch value 0

if I apply change below

diff --git a/sound/soc/sof/control.c b/sound/soc/sof/control.c
index 75e13f4fd1eb..2fb925dd4721 100644
--- a/sound/soc/sof/control.c
+++ b/sound/soc/sof/control.c
@@ -91,6 +91,7 @@ int snd_sof_switch_put(struct snd_kcontrol *kcontrol,
        struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp);
        const struct sof_ipc_tplg_ops *tplg_ops = sof_ipc_get_ops(sdev, tplg);

+       pr_info("%s name %s value %ld\n", __func__, kcontrol->id.name, ucontrol->value.integer.value[0]);
        if (tplg_ops && tplg_ops->control && tplg_ops->control->switch_put)
                return tplg_ops->control->switch_put(scontrol, ucontrol);

It means something from user space set all Master Capture Switch to 0.

BTW, PGA Master Capture Switches are from SOF. It is not related to rt715-sdca at all.

KaiChuan-Hsieh commented 1 year ago

@bardliao

May I know which image and kernel you are using for the results? Can you run the alsaucm init to rerun the bootsequence or alsa restore state to identify which operation do it?

Thanks,

bardliao commented 1 year ago

@KaiChuan-Hsieh We installed Ubuntu 22.04 and update kernel to the kernel built from https://github.com/thesofproject/linux/

I don't think UCM set it. If I remove

ctl.hw {
        @args [ CARD ]
        @args.CARD {
                type string
                default {
                        @func getenv
                        vars [
                                ALSA_CTL_CARD
                                ALSA_CARD
                        ]
                        default {
                                @func refer
                                name defaults.ctl.card
                        }
                }
        }
        type hw
        card $CARD
        hint.description "Direct control device"
}

from /usr/share/alsa/alsa.conf, user space will not set the switches.

@plbossart We set default volume value but didn't set cdata->chanv[i].value = 1 for the PGA Switches in sof_ipc3_control_load_volume(). And `cdata->chanv[i].value' will be 0 if we don't set it. Therefore, 'PGA5.0 5 Master Capture Switch' and 'PGA2.0 2 Master Capture Switch' will be Off even user space doesn't set them.

@KaiChuan-Hsieh Can you try if below change help?

diff --git a/sound/soc/sof/ipc3-topology.c b/sound/soc/sof/ipc3-topology.c
index 27b034d7ff6e..101be94596bd 100644
--- a/sound/soc/sof/ipc3-topology.c
+++ b/sound/soc/sof/ipc3-topology.c
@@ -1728,6 +1728,8 @@ static int sof_ipc3_control_load_volume(struct snd_sof_dev *sdev, struct snd_sof
        /* set cmd for mixer control */
        if (scontrol->max == 1) {
                cdata->cmd = SOF_CTRL_CMD_SWITCH;
+               for (i = 0; i < scontrol->num_channels; i++)
+                       cdata->chanv[i].value = 1;
                return 0;
        }

@gongjun-song FYI

andychi117 commented 1 year ago

Apply the change mentioned

@KaiChuan-Hsieh We installed Ubuntu 22.04 and update kernel to the kernel built from https://github.com/thesofproject/linux/

I don't think UCM set it. If I remove

ctl.hw {
        @args [ CARD ]
        @args.CARD {
                type string
                default {
                        @func getenv
                        vars [
                                ALSA_CTL_CARD
                                ALSA_CARD
                        ]
                        default {
                                @func refer
                                name defaults.ctl.card
                        }
                }
        }
        type hw
        card $CARD
        hint.description "Direct control device"
}

from /usr/share/alsa/alsa.conf, user space will not set the switches.

@plbossart We set default volume value but didn't set cdata->chanv[i].value = 1 for the PGA Switches in sof_ipc3_control_load_volume(). And `cdata->chanv[i].value' will be 0 if we don't set it. Therefore, 'PGA5.0 5 Master Capture Switch' and 'PGA2.0 2 Master Capture Switch' will be Off even user space doesn't set them.

@KaiChuan-Hsieh Can you try if below change help?

diff --git a/sound/soc/sof/ipc3-topology.c b/sound/soc/sof/ipc3-topology.c
index 27b034d7ff6e..101be94596bd 100644
--- a/sound/soc/sof/ipc3-topology.c
+++ b/sound/soc/sof/ipc3-topology.c
@@ -1728,6 +1728,8 @@ static int sof_ipc3_control_load_volume(struct snd_sof_dev *sdev, struct snd_sof
        /* set cmd for mixer control */
        if (scontrol->max == 1) {
                cdata->cmd = SOF_CTRL_CMD_SWITCH;
+               for (i = 0; i < scontrol->num_channels; i++)
+                       cdata->chanv[i].value = 1;
                return 0;
        }

@gongjun-song FYI

@bardliao Hi, I applied the change above and use amixer to get the status.

$ amixer cget name='PGA5.0 5 Master Capture Switch'
numid=52,iface=MIXER,name='PGA5.0 5 Master Capture Switch'
  ; type=BOOLEAN,access=rw------,values=2
  : values=off,off

arecord -D hw:0,4 -f S16_LE -c 2 -r 48000 -d 5 record.wav -vvv remains 0%.

bardliao commented 1 year ago

@bardliao Hi, I applied the change above and use amixer to get the status.

$ amixer cget name='PGA5.0 5 Master Capture Switch'
numid=52,iface=MIXER,name='PGA5.0 5 Master Capture Switch'
  ; type=BOOLEAN,access=rw------,values=2
  : values=off,off

arecord -D hw:0,4 -f S16_LE -c 2 -r 48000 -d 5 record.wav -vvv remains 0%.

@andychi117 The patch will set 'PGA5.0 5 Master Capture Switch' default on, but it can't prevent user space from setting it on. So, you need to make sure user space will not set the control. Maybe you can install a brand new system and see if it helps. I am wondering if ALSA or Ubuntu will store all control's value somewhere and restore the value every boot.

perexg commented 1 year ago
systemctl stop alsa-state
systemctl stop alsa-restore
rm /var/lib/alsa/asound.state

And reboot.. It's not necessary to reinstall the system.

andychi117 commented 1 year ago

@bardliao Hi, I applied the change above and use amixer to get the status.

$ amixer cget name='PGA5.0 5 Master Capture Switch'
numid=52,iface=MIXER,name='PGA5.0 5 Master Capture Switch'
  ; type=BOOLEAN,access=rw------,values=2
  : values=off,off

arecord -D hw:0,4 -f S16_LE -c 2 -r 48000 -d 5 record.wav -vvv remains 0%.

@andychi117 The patch will set 'PGA5.0 5 Master Capture Switch' default on, but it can't prevent user space from setting it on. So, you need to make sure user space will not set the control. Maybe you can install a brand new system and see if it helps. I am wondering if ALSA or Ubuntu will store all control's value somewhere and restore the value every boot.

Hi @bardliao , The internal microphone works, below is my test steps.

amixer cset name='PGA5.0 5 Master Capture Switch' 0
sudo rm /var/lib/alsa/asound.conf
echo  b | sudo tee /proc/sysrq-trigger

After boot into OS,

amixer cget name='PGA5.0 5 Master Capture Switch
numid=52,iface=MIXER,name='PGA5.0 5 Master Capture Switch'
  ; type=BOOLEAN,access=rw------,values=2
  : values=on,on

Would you help to upstream the kernel patch? Thanks.

bardliao commented 1 year ago

Hi @bardliao , The internal microphone works, below is my test steps.

@andychi117 Thanks for the testing.


amixer cset name='PGA5.0 5 Master Capture Switch' 0

Why do you need to set 'PGA5.0 5 Master Capture Switch' off?

sudo rm /var/lib/alsa/asound.conf echo b | sudo tee /proc/sysrq-trigger


After boot into OS,

amixer cget name='PGA5.0 5 Master Capture Switch numid=52,iface=MIXER,name='PGA5.0 5 Master Capture Switch' ; type=BOOLEAN,access=rw------,values=2 : values=on,on



Would you help to upstream the kernel patch? Thanks.

Could you run below commands and test without my patch?

systemctl stop alsa-state
systemctl stop alsa-restore
rm /var/lib/alsa/asound.state

According to Zorro, it works without my patch.

If my patch is required, I will submit a PR on https://github.com/thesofproject/linux/. BTW, many Intel Audio folks are off this week. Please expect slow review process.

KaiChuan-Hsieh commented 1 year ago

Hello,

@bardliao

I did it on my rt715-sdca platform. it didn't help, the control keeps off without your patch. I think we require your patch to make default to be on.

systemctl stop alsa-state
systemctl stop alsa-restore
rm /var/lib/alsa/asound.state

Thanks,

bardliao commented 1 year ago

@andychi117 @KaiChuan-Hsieh PR https://github.com/thesofproject/linux/pull/4116 is created.

perexg commented 1 year ago

I would NAK this kernel change. If it's a known behavior - not a bug (and we prefer to turn all off by default in the drivers), we can set only used paths in the BootSequence in UCM.

Proposed change:

diff --git a/ucm2/sof-soundwire/sof-soundwire.conf b/ucm2/sof-soundwire/sof-soundwire.conf
index 03df16c..a349b07 100644
--- a/ucm2/sof-soundwire/sof-soundwire.conf
+++ b/ucm2/sof-soundwire/sof-soundwire.conf
@@ -61,3 +61,14 @@ If.mic_init {
        }
        True.Include.mic_init.File "/codecs/${var:MicCodec1}/init.conf"
 }
+
+If.mic_init_rt715 {
+       Condition {
+               Type String
+               Needle "rt715"
+               Haystack "${var:MicCodec1}"
+       }
+       True.BootSequence [
+               cset "name='PGA5.0 5 Master Capture' 1"
+       ]
+}
bardliao commented 1 year ago

I would NAK this kernel change. If it's a known behavior - not a bug (and we prefer to turn all off by default in the drivers), we can set only used paths in the BootSequence in UCM.

Proposed change:

diff --git a/ucm2/sof-soundwire/sof-soundwire.conf b/ucm2/sof-soundwire/sof-soundwire.conf
index 03df16c..a349b07 100644
--- a/ucm2/sof-soundwire/sof-soundwire.conf
+++ b/ucm2/sof-soundwire/sof-soundwire.conf
@@ -61,3 +61,14 @@ If.mic_init {
        }
        True.Include.mic_init.File "/codecs/${var:MicCodec1}/init.conf"
 }
+
+If.mic_init_rt715 {
+       Condition {
+               Type String
+               Needle "rt715"
+               Haystack "${var:MicCodec1}"
+       }
+       True.BootSequence [
+               cset "name='PGA5.0 5 Master Capture' 1"
+       ]
+}

Thanks @perexg for the input. I also prefer to only set used paths in UCM instead of changing the default value in driver. That's why we asked @KaiChuan-Hsieh report issue here. I will close my PR.

KaiChuan-Hsieh commented 1 year ago

@perexg

Hello,

I tried the ucm config propsed by you. The only thing I modify is that the name of the control should be 'PGA5.0 5 Master Capture Switch' but not 'PGA5.0 5 Master Capture'. And your change can make the control be enabled by default too. Would you propose PR for it?

Thanks,

perexg commented 1 year ago

Thanks for the quick feedback. Fixed in e395d7b743584cba60876b6356fb3bc7834992aa.