Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sdca support #71

Closed
wants to merge 2 commits into from
Closed

Conversation

libinyang
Copy link
Contributor

Add supports for SDW sdca codec devices

@libinyang
Copy link
Contributor Author

@perexg Ping for review. This PR has been tested on sdca codecs.

@libinyang
Copy link
Contributor Author

BTW: we are trying to remove PGA in rt715-sdca. But there is an issue in codec driver. After it is fixed, we will switch to 'rt714 FU02' for PGA

@libinyang
Copy link
Contributor Author

libinyang commented Jan 7, 2021

updated the patch to use 'rt714 FU02' as this bug is fixed by thesofproject/linux#2670

@perexg
Copy link
Member

perexg commented Jan 13, 2021

Basically, the patch looks ok. Can we have also output from alsa-info.sh --no-upload for the added sdca devices to improve the validator?

@libinyang
Copy link
Contributor Author

Basically, the patch looks ok. Can we have also output from alsa-info.sh --no-upload for the added sdca devices to improve the validator?

@perexg Do you mean submit the alsa-info.sh output to alsa-tests? I will do it.

I have another question about the UCM for the rt715 sdca codec. It perplexes me. We used PGA control as the volume/mute control before. But currently, we are trying to use rt715's kcontrol in non-sdca codec, such as 'rt715 Main Capture Switch' in #67. I also change to use "rt714 FU02" to replace the PGA control in this PR. Do you think we should use "rt714 FU02" or PGA kcontrols?
My concern is that it may impact on the MIC LED. Realtek's codec driver won't touch the MIC LED based on my test. So if we use "rt714 FU02", the MIC LED may not work on the old platforms.
Or we use "rt714 FU02" in the PR, and if we met some platforms which MIC LED doesn't work, then we can add patches for that platform. What do you think?

@GoPerry
Copy link

GoPerry commented Jan 15, 2021

@libinyang
Will the new SDCA Codec will not use the rt715 sdca ucm2 conf ?
The new Intel platforms and sdca codec will only use the new ucm2 conf, we can only use the new sdca conf replacing the PGA kcontrol,is it right ?

the non sdca codec, for example rt715 ucm conf will need to have the compatible conf for the other platforms which has the none sdca rt715 codec.

I have submmited the initial PR for the compatible case.
If @perexg agree to use this matching method, i will test that patch on my system.

@perexg
Copy link
Member

perexg commented Jan 15, 2021

My opinion is that the analog volume control is preferred than the PCM stream modifications. So PGA controls should not be used, if we have the analog controls.

But the situation for Dell device is a bit complicated, because we have multiple controls for the microphone array (2 stereo controls for four microphones) and we cannot describe this in UCM at the moment. I proposed to avoid the kernel driver change (which is incomplete anyway) and use the current scenario - use PGA for the volume control and set the FU registers to a sane maximal value per machine. We can do DMI checks at the UCM layer. My plan is to add the translation code for two stereo controls -> one four channel controls to alsa-lib in the near future so we can switch to FU then.

The LED problem is driver specific. I believe that all controls (FU, PGA in this case) on the path which effectively turns off capture in the kernel space (zero samples), should invoke LED off. See my proposal on the alsa-devel mailing list: https://lore.kernel.org/alsa-devel/1765b000-ac56-7373-b544-b5d9daa1565b@perex.cz/T/#t .

@libinyang
Copy link
Contributor Author

My opinion is that the analog volume control is preferred than the PCM stream modifications. So PGA controls should not be used, if we have the analog controls.

But the situation for Dell device is a bit complicated, because we have multiple controls for the microphone array (2 stereo controls for four microphones) and we cannot describe this in UCM at the moment. I proposed to avoid the kernel driver change (which is incomplete anyway) and use the current scenario - use PGA for the volume control and set the FU registers to a sane maximal value per machine. We can do DMI checks at the UCM layer. My plan is to add the translation code for two stereo controls -> one four channel controls to alsa-lib in the near future so we can switch to FU then.

The LED problem is driver specific. I believe that all controls (FU, PGA in this case) on the path which effectively turns off capture in the kernel space (zero samples), should invoke LED off. See my proposal on the alsa-devel mailing list: https://lore.kernel.org/alsa-devel/1765b000-ac56-7373-b544-b5d9daa1565b@perex.cz/T/#t .

@perexg Got it. I will use PGA in my updated patches. Thanks.

@libinyang
Copy link
Contributor Author

@libinyang
Will the new SDCA Codec will not use the rt715 sdca ucm2 conf ?

@GoPerry
SDCA rt715 codec will use rt715 sdca UCM.

The new Intel platforms and sdca codec will only use the new ucm2 conf, we can only use the new sdca conf replacing the PGA kcontrol,is it right ?

sdca codec will use xxx-sdca.conf.

the non sdca codec, for example rt715 ucm conf will need to have the compatible conf for the other platforms which has the none sdca rt715 codec.

Yes, this is why we needs separated sdca codec UCM.

I have submmited the initial PR for the compatible case.
If @perexg agree to use this matching method, i will test that patch on my system.

@libinyang
Copy link
Contributor Author

Patch is updated and alsa-info is submitted to alsa-project/alsa-tests#15

cset "name='rt714 ADC 22 Mux' 'DMIC3'"
cset "name='rt714 ADC 23 Mux' 'DMIC4'"
cset "name='rt714 FU02 Capture Switch' 1"
cset "name='rt714 FU02 Capture Volume' 124 124"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, the initialization for the second stereo pair is missing here? In the kernel patch (https://lore.kernel.org/alsa-devel/7d74544f-77f2-db01-db3d-d1d8a56d576d@linux.intel.com/T/#m9d8869261e4531f537336da6b81085c1567034a9) the four stereo controls are set:

+	unsigned int capture_reg_H[] = {RT715_SET_GAIN_MIC_ADC_H,
+		RT715_SET_GAIN_LINE_ADC_H, RT715_SET_GAIN_MIX_ADC_H,
+		RT715_SET_GAIN_MIX_ADC2_H};
+	unsigned int capture_reg_L[] = {RT715_SET_GAIN_MIC_ADC_L,
+		RT715_SET_GAIN_LINE_ADC_L, RT715_SET_GAIN_MIX_ADC_L,
+		RT715_SET_GAIN_MIX_ADC2_L};

The stereo controls are:

        SOC_DOUBLE_R_EXT_TLV("ADC 07 Capture Volume", RT715_SET_GAIN_MIC_ADC_H,
                        RT715_SET_GAIN_MIC_ADC_L, RT715_DIR_IN_SFT, 0x3f, 0,
                        rt715_set_amp_gain_get, rt715_set_amp_gain_put,
                        in_vol_tlv),
        SOC_DOUBLE_R_EXT_TLV("ADC 08 Capture Volume", RT715_SET_GAIN_LINE_ADC_H,
                        RT715_SET_GAIN_LINE_ADC_L, RT715_DIR_IN_SFT, 0x3f, 0,
                        rt715_set_amp_gain_get, rt715_set_amp_gain_put,
                        in_vol_tlv),
        SOC_DOUBLE_R_EXT_TLV("ADC 09 Capture Volume", RT715_SET_GAIN_MIX_ADC_H,
                        RT715_SET_GAIN_MIX_ADC_L, RT715_DIR_IN_SFT, 0x3f, 0,
                        rt715_set_amp_gain_get, rt715_set_amp_gain_put,
                        in_vol_tlv),
        SOC_DOUBLE_R_EXT_TLV("ADC 27 Capture Volume", RT715_SET_GAIN_MIX_ADC2_H,
                        RT715_SET_GAIN_MIX_ADC2_L, RT715_DIR_IN_SFT, 0x3f, 0,
                        rt715_set_amp_gain_get, rt715_set_amp_gain_put,
                        in_vol_tlv),

If you need only one control, we can use the hardware rt714 register controls now. My impression was that there are more microphones then 2 are connected... Perhaps, I need to study, how the FU controls are build for rt714/715 (they come from topology files?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, you have rt715-scda.c only in your development repo. It is not upstreamed and the FU02 (and maybe FU0E/FU0C boost) controls are problematic in my eyes - it contains the stereo volume/switch mapping/coupling to multiple hardware registers/inputs.

I created thesofproject/linux#2696 .

@perexg
Copy link
Member

perexg commented Jan 18, 2021

Until thesofproject/linux#2696 is resolved, we cannot continue. If FU SCDA controls will be multichannel, I would prefer to use them, otherwise see my #71 (comment) .

@libinyang
Copy link
Contributor Author

Until thesofproject/linux#2696 is resolved, we cannot continue. If FU SCDA controls will be multichannel, I would prefer to use them, otherwise see my #71 (comment) .

@perexg The patch for rt715-sdca is in https://github.com/thesofproject/linux/ already. And I suppose this will be soon upstreamed. And thanks to raise the issue thesofproject/linux#2696. In our current design, FU02 will control multiple channels. I would like to pending this PR and let's talk more details in thesofproject/linux#2696.

For now we don't have a specific handing of the number of speakers, we
should be handling the number of amplifiers instead. The number of
speakers was added in machine drivers as a placeholder, if and when we
you it it would be in addition to the number of amplifiers, not as a
substitute.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Libin Yang <libin.yang@intel.com>
@libinyang
Copy link
Contributor Author

Patch is updated based on @jack-cy-yu 's latest rt715 codec patch jack-cy-yu/linux@17706c5

@libinyang
Copy link
Contributor Author

@perexg This patch is based on @jack-cy-yu 's latest sdca codec patch. Could you please help review? Thanks.

Value {
PlaybackPriority 100
PlaybackPCM "hw:${CardId},2"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No hw volume control?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. There is no HW volume control.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like double check: The codec does not seem any volume control, but do we have any DSP (PGA) controls which can modify the stream volume to use them here?


Value {
PlaybackPriority 100
PlaybackPCM "hw:${CardId},2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No hw volume control?

CapturePCM "hw:${CardId},1"
JackControl "Headset Mic Jack"
CaptureSwitch "PGA2.0 2 Master Capture Switch"
CaptureVolume "PGA2.0 2 Master Capture Volume"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing MixerElem for PA and no hw volume control?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find a proper switch/volume kcontrol for it on my platform.

CapturePriority 100
CapturePCM "hw:${CardId},4"
CaptureSwitch "rt714 FU02 Capture Switch"
CaptureVolume "rt714 FU02 Capture Volume"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing MixerElem for PA.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the MixerElem, is CaptureMixerElem "rt714 FU02" good enough?
I checked how to set control for MixerElem in alsa-lib. It seems very little information about it. I don't know what is the best solution to set the MixerElem. Could you give a little hints?

Add support for rt711-sdca, rt1316 and rt715-sdca (aka rt714).

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Libin Yang <libin.yang@intel.com>
@libinyang
Copy link
Contributor Author

@perexg Patches are updated based on my understanding. Please help review whether my update is OK or not. I'm not so confident about the MixerElem setting is ok or not. For the rt711-sdca hw control, "rt711 FU0F" should be the right control. But the switch doesn't work on my platform. So I still use the PGA kcontrol in this version.

@libinyang
Copy link
Contributor Author

ping @perexg

@KaiChuan-Hsieh
Copy link
Contributor

KaiChuan-Hsieh commented Apr 8, 2021

The commit test passed on machine with sdca codec.
@perexg
May I know if there is uncertainty of sdca implementation? so that the change is still under discussion.

Thanks,

@perexg
Copy link
Member

perexg commented Apr 15, 2021

I applied this to ucm repo, but I still think that the hw controls should be improved for the best experience.

@perexg perexg closed this Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants