Introducing virtual FE by ranj063 · Pull Request #8 · thesofproject/linux

ranj063 · 2018-06-25T21:20:51Z

This set of patches introduces the concept of a virtual FE dai link.
Previously, when trying to enable the hostless pipeline, it was
discovered that the codec does not get enabled and the tone remained inaudible.
Therefore, in such cases, a virtual FE dai link will be used to establish
a connection to the BE dai and enable the codec when the pipeline is triggered.

The virtual FE will be created when a siggen widget is loaded and freed when
the siggen widget is unloaded. It will be used for enabling the BE dai in the
kcontrol IO handler for the kcontrol attached to the siggen by calling
the soc_spcm_runtime_update() method.

lgirdwood · 2018-06-26T11:51:11Z

+	 * This connection will be established at runtime by triggering the
+	 * hostless pipeline with a kcontrol attached to the component.
+	 */
+	unsigned int virtual:1;


Just wondering if we can set dynamic = 1 and no_pcm = 1 to achieve the same effect and hence no need for this ?

i had the same question, it feels somewhat redundant with existing mechanisms

Fixed in the revised version

lgirdwood · 2018-06-26T11:51:56Z

+	/* define virtual FE DAI link */
+	link->virtual = 1;
+	link->name = link_name;
+	link->id = 1;


Pass ID in ?

I removed the ID as it isnt really needed

lgirdwood · 2018-06-26T11:52:27Z

+	link->platform_name = platform_name;
+	link->codec_name = "snd-soc-dummy";
+	link->codec_dai_name = "snd-soc-dummy-dai";
+	link->num_codecs = 1;


Do we need codecs ? should it b 0 ?

lgirdwood · 2018-06-26T11:52:48Z

+	link->dynamic = 1;
+
+	/*TODO: check if we need to handle capture for virtual FE */
+	link->dpcm_playback = 1;


yes, this looks like a generic capability on capture where a component can capture audio and throw some sort of non-audio event DAPM knows nothing about.

Should the virtual FE have both dpcm_playback and dpcm_capture enabled by default ?
Or does it depend on the widget that calls vfe_new(). So in the case of tone, I can only have dpcm_playback enabled?

I've added capture support in virtual FE too. The widget creating the vfe must specify the stream direction.

lgirdwood · 2018-06-26T11:53:26Z

+EXPORT_SYMBOL_GPL(soc_dpcm_vfe_new);
+
+/* free virtual FE DAI link */
+int soc_dpcm_vfe_free(struct snd_soc_card *card)


we need to individually delete these link by link.

Is the idea then that the widget that created the virtual fe, will delete it?
In that case, can I associate the link with the comp by adding it to the snd_sof_control associated with the comp?

FIxed to delete vfe by name

lgirdwood · 2018-06-26T11:54:19Z

+			for_each_rtdcom_safe(rtd, rtdcom1, rtdcom2)
+				kfree(rtdcom1);
+
+			INIT_LIST_HEAD(&rtd->component_list);


Why is the list head init here in free() ?

lgirdwood · 2018-06-26T11:55:53Z


 	/* Drop starting point */
-	list_del(widgets.next);
+	if (!list_is_singular(&widgets))


What happens if you have more than one widget between widget and BE, it's valid and could be constructed.

This part is a bit unclear to me. The problem I was facing was that the snd_soc_dapm_dai_get_connected_widgets() returned a list with only the BE DAI in the case of the tone pipeline and dropping the starting point resulted in an empty list of paths to process.

the dai argument passed to this method was the cpu_dai->playback_widget() which is the BE dai in my case. And this was the only way I could get it to be enabled. I could use some help here to figure out the right way to do it.

Yes, what happens if you have tone followed by a volume control?

We do have a tone followed by volume in my topology. But the snd_soc_dapm_dai_get_connected_widgets() is called starting with the dai->playback_widget as the starting point. And this is the BE DAI.

I think we should also state in a comment what we are doing here and why.

lgirdwood · 2018-06-26T11:56:50Z

+		 * pcm device and substream for the requested direction
+		 */
+		if (rtd->dai_link->virtual) {
+			struct snd_pcm_str *pstr;


I would offload this logic as a static function.

and can any part of this be done by call core ALSA functions directly ?

The only function that does this is snd_new_pcm() but that also registers the pcm device. So I created this to avoid that part.

ranj063 · 2018-06-26T18:49:07Z

Thanks, Liam and Pierre. Let me work on revising the patchset today.

This patch proposes to enable kcontrols for siggen type dapm widgets. This will allow triggering the tone generating siggen component and also modifying the run-time attributes such as frequency, amplitude etc. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

This patch adds the methods for create a virtual FE dal link and add it to the sound card. It also adds the method to free the virtual FE connected to the card. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

When walking the graph to discover the path from the virtual FE to the BE, there is only one widget in the path. Do not remove this BE widget from the list, so it can be used to connect with the virtual FE dai link. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

…irtual dai link Virtual FE dai links should be manually set to running state by default with a pcm runtime. The active count of their cpu_dai and codec_dai's should also be updated. This is required to establish FE-BE connection and enable the BE DAI when the dpcm runtime is updated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 · 2018-06-27T20:45:27Z

@plbossart , @lgirdwood . I've pushed some changes. Could you please review the new version?

Virtual FE dai links do not need to register the pcm device. So just create the empty pcm device and substream in the requested direction. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

the soc_dpcm_runtime_update() method will be called to establish a connection to the BE and enable the codec. So make this method accessible to modules. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

plbossart

Looks better! I think it does make sense to reuse no_pcm and dynamic, this is easier to push upstream. but I am still not clear on a couple of comments related to init/free/activation.

plbossart · 2018-06-27T22:44:49Z

+			dpcm_be_disconnect(rtd, stream_dir);
+
+			/* free pcm runtime */
+			kfree(rtd->dpcm[stream_dir].runtime);


Question: all these frees don't reflect what is done for the _new method, could we rely on a helper to avoid tracking all these actions? It looks like either copy-paste or something extracted from somewhere else.

This is actually not a copy-paste at all. I traced all the steps from the point of adding the link to the time when the sound card is registered and freed all the data that is allocated. Here're the steps I traced:

Bind DAI link: The PCM runtime and the dpcm runtime is created during this step.
There are some other runtime child structures allocated during this step too ie codec dais and component list.

Probe DAI link :The pcm device and the substream is created here.

dpcm runtime update: the FE link is connected to the BE

So, in the vfe_free() method, I undid all of this.

But I wonder if I can avoid all of this manually in the free method. Let me dig into these steps a bit more.

plbossart · 2018-06-27T22:45:53Z


 	/* Drop starting point */
-	list_del(widgets.next);
+	if (!list_is_singular(&widgets))


Yes, what happens if you have tone followed by a volume control?

plbossart · 2018-06-27T22:49:04Z


 	soc_add_pcm_runtime(card, rtd);
+
+	/* if this is a virtual FE link, create runtime and set it as active */


I don't know what 'active' runtime means. I would think it becomes 'active' when the switch is On, but maybe it's a different concept you are referring to.

Oh, what I mean by active here is when the kcontrol is switched on and the dpcm_runtime_update() is called, it checks to see if playback or capture is active on the FE link. So I set the FE playback/capture active status by default.

I see, but I wonder if this has negative implications related to suspend/resume? If there is nothing going on it's a bit odd to force a runtime to be active.

Yes, thats a good point.

The problem is the runtime_update() method is oblivious to where it is being called from. It just looks for an active link to process the update.

Maybe I can avoid this by activating the FE in the kcontrol IO handler just before calling runtime_update.

lgirdwood

Sorry, have more questions on the core changes.

lgirdwood · 2018-07-02T14:40:19Z


 	/* Create any new FE <--> BE connections */
 	for (i = 0; i < list->num_widgets; i++) {
-


best to keep this line as is.

lgirdwood · 2018-07-02T14:43:54Z

+	substream->pstr = pstr;
+	substream->number = 0;
+	substream->stream = stream_dir;
+	sprintf(substream->name, "subdevice #%i", 0);


Is the name exposed by /sysff or /proc ? if so, do we have any naming collisions when we register 2 or more VFE ?

hmm I did not check if it is exposed. Let me try to make it unique

plbossart · 2018-07-03T16:38:05Z

 			kcname_in_long_name = true;
 		} else {
 			switch (w->id) {
+			case snd_soc_dapm_siggen:


as discussed last week, I wonder if we should also add a snd_soc_dapm_sig_sink (the dual of a generator). This would be very useful for testing e.g. by letting us set-up all sort of DMA channels with a self-test checking.

Sure, I will add it to my next update.

ranj063 · 2018-07-05T17:13:45Z

@plbossart , @lgirdwood . I'm closing this pull request and replacing it with a new pull request that has all the commits to enable the tone generator. This will be helpful for you review the entire flow for enabling the VFE link and the pipeline. I have addressed your comments in the new pull request.

lgirdwood requested changes Jun 26, 2018

View reviewed changes

ranj063 added 4 commits June 27, 2018 13:38

ASoC: pcm: add methods to create/remove virtual dai link

0452dd4

This patch adds the methods for create a virtual FE dal link and add it to the sound card. It also adds the method to free the virtual FE connected to the card. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the topic/tone-1 branch from 862fd6d to 7bfc86b Compare June 27, 2018 20:44

ranj063 force-pushed the topic/tone-1 branch from 7bfc86b to fc1bc98 Compare June 27, 2018 20:52

ranj063 added 2 commits June 27, 2018 13:52

ASoC: pcm: do not register pcm device for virtual FE dai links

bd1132f

Virtual FE dai links do not need to register the pcm device. So just create the empty pcm device and substream in the requested direction. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the topic/tone-1 branch from fc1bc98 to f00e120 Compare June 27, 2018 20:52

plbossart reviewed Jun 27, 2018

View reviewed changes

lgirdwood requested changes Jul 2, 2018

View reviewed changes

plbossart reviewed Jul 3, 2018

View reviewed changes

ranj063 closed this Jul 5, 2018

ranj063 mentioned this pull request Sep 14, 2018

kernel warning related to hdac_hdmi seen during boot #131

Closed

tokyovigilante mentioned this pull request Nov 7, 2019

Lenovo patches #1442

Merged

neuisenburger mentioned this pull request Feb 9, 2022

[FEATURE] Support for ES8336 on Intel Comet Lake #3248

Closed

yongzhi1 mentioned this pull request May 21, 2022

[MTL] hda_dsp_core_power_down timeout/failed when kmod removal #3662

Closed

aplitvinov mentioned this pull request Apr 21, 2023

No sound on Huawei Matebook 14 (2021) - even on Linux 6.2 #4305

Open

keithvassallomt mentioned this pull request Apr 13, 2026

[SoundWire] Lenovo Yoga 7 2-in-1 14IPH11 (83TC) — only 2 of 4 speakers working (PTL + RT721 + RT1320) #5731

Closed

davieslc mentioned this pull request Apr 15, 2026

No audio on ASUS Zenbook S14 UX5406AA (Panther Lake): sof_sdw fails to create DAI links for cs35l56 #5721

Open


		soc_add_pcm_runtime(card, rtd);

		/* if this is a virtual FE link, create runtime and set it as active */


		/* Create any new FE <--> BE connections */
		for (i = 0; i < list->num_widgets; i++) {

Conversation

ranj063 commented Jun 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Jun 26, 2018

Uh oh!

ranj063 commented Jun 27, 2018

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ranj063 Jun 27, 2018 •

edited

Loading