unload physical dai link in remove by bardliao · Pull Request #511 · thesofproject/linux

bardliao · 2019-01-08T01:27:31Z

soc_tplg_link_config will find the physical dai link and load the dai link
but the dai link will not be unload in snd_soc_tplg_component_remove
since it is not dynamic created by topology. Add dai link unload to
balance it. Note that the link unload will only call topology ops link_inload,
the link will still be removed by soc core.

Fix #343

plbossart · 2019-01-08T01:38:17Z

@xiulipan I don't know why the build fails, it works fine for me when I check out this PR and compile locally? It's not even based on the 4.20 code and gives the same errors with the jump thingy

plbossart

@bardliao this is heavy-duty stuff which could benefit from additional explanations. It's not clear what the differences are between remove_link and unload_link, and you mention that it's done to "balance" the code but the initial init isn't mentioned.
No strong opposition from me here but we need to make it easier on reviewers since this needs to go upstream

plbossart · 2019-01-08T01:49:04Z

is this a hack or really a type we want to use?

@plbossart I think it is the best choice if we don't add a new item in snd_soc_dobj_type. The link which we want to unload is not a dynamic object. That's why I use SND_SOC_DOBJ_NONE. Currently, SND_SOC_DOBJ_NONE is only used for unloading link, but we have to consider if it can be used for other functions in the future.

@bardliao I think we are contradicting ourselves by calling it type SND_SOC_DOBJ_NONE and then adding it to the dobj_list.

I think the better option is to use SND_SOC_DOBJ_DAI_LINK type and re-use the remove_link to check for both passes, SOC_TPLG_PASS_PCM_DAI and SOC_TPLG_PASS_LINK.

Thanks @ranj063 It's a good idea. I will update my PR and add more comment to explain it.

@ranj063 There is a problem if we reuse SND_SOC_DOBJ_DAI_LINK. soc_remove_dai_links will check if the link is SND_SOC_DOBJ_DAI_LINK type. I think soc-core think all SND_SOC_DOBJ_DAI_LINK type is created by topology. So we will see "Topology forgot to remove link " warning if we reuse SND_SOC_DOBJ_DAI_LINK type.

@bardliao do BE links that are defined in the machine driver get added to the card's dai_link_list?

Then maybe we need to modify the check to make sure it is a BE link before we generate the warning.

bardliao · 2019-01-08T02:59:40Z

@plbossart There are two types of links. One is FE and the other is BE. The FE links are created by topology and we call remove_link() to remove it in current code. The BE links are predefined and will be removed by soc_remove_dai_links(). Topology will load the BE links in soc_tplg_link_config() without unloading it. And that is the patch trying to do. We add the link to &tplg->comp->dobj_list so we can unload it in snd_soc_tplg_component_remove()

plbossart · 2019-01-08T05:27:47Z

@plbossart There are two types of links. One is FE and the other is BE. The FE links are created by topology and we call remove_link() to remove it in current code. The BE links are predefined and will be removed by soc_remove_dai_links(). Topology will load the BE links in soc_tplg_link_config() without unloading it. And that is the patch trying to do. We add the link to &tplg->comp->dobj_list so we can unload it in snd_soc_tplg_component_remove()

Very useful explanation that's completely missing in the patch. It'd be useful to add it at least to the commit message.
Instead of doing this DOBJ_NONE hack can't we use the fact that the objects are dynamic and as Ranjani suggested free the dynamic objects in the FE parts and the rest for the BE parts?

xiulipan · 2019-01-08T05:56:30Z

@plbossart
Checked, it seems the make SUBDIRS=sound/soc/sof C=2 is not smart enough to update all changes when such a big PR comes.
A full make fix it.

bardliao · 2019-01-08T08:10:14Z

@plbossart @ranj063 I updated the PR with the following changes

Update change log
Add new dobj type named SND_SOC_DOBJ_BE_DAI_LINK instead of using DOBJ_NONE
Add more comments in the code

plbossart

Looks better but still some questions below, the biggest one being if there is a correlation with SND_SOC_TPLG_TYPE_BACKEND_LINK definitions in include/uapi/sound/asoc.h

plbossart · 2019-01-08T13:45:37Z

Hum. If I look at include/uapi/sound/asoc.h, I see the following definitions

#define SND_SOC_TPLG_TYPE_MIXER 1 #define SND_SOC_TPLG_TYPE_BYTES 2 #define SND_SOC_TPLG_TYPE_ENUM 3 #define SND_SOC_TPLG_TYPE_DAPM_GRAPH 4 #define SND_SOC_TPLG_TYPE_DAPM_WIDGET 5 #define SND_SOC_TPLG_TYPE_DAI_LINK 6 #define SND_SOC_TPLG_TYPE_PCM 7 #define SND_SOC_TPLG_TYPE_MANIFEST 8 #define SND_SOC_TPLG_TYPE_CODEC_LINK 9 #define SND_SOC_TPLG_TYPE_BACKEND_LINK 10 #define SND_SOC_TPLG_TYPE_PDATA 11 #define SND_SOC_TPLG_TYPE_DAI 12

It looks like there was a correlation between the two structures initially, so should we use BACKEND_LINK instead? @mengdonglin can you comment on this?

@plbossart @mengdonglin I have a question here. We create a dobj with SND_SOC_DOBJ_DAI_LINK type for tplg type SND_SOC_TPLG_TYPE_PCM.
case SND_SOC_TPLG_TYPE_PCM

soc_tplg_pcm_elems_load() -> soc_tplg_pcm_create() -> soc_tplg_fe_link_create()

link->dobj.type = SND_SOC_DOBJ_DAI_LINK

And config physical link on tplg type SND_SOC_TPLG_TYPE_DAI_LINK and SND_SOC_TPLG_TYPE_BACKEND_LINK.
case SND_SOC_TPLG_TYPE_DAI_LINK:
case SND_SOC_TPLG_TYPE_BACKEND_LINK:
It seems the tplg type and dobj type is mismatched already.
Should we fix it first?

sorry @bardliao i didn't understand your question, but it looks like there is a confusion that we need to solve. Care to elaborate maybe with a suggested diff?

@plbossart If we find a SND_SOC_TPLG_TYPE_PCM type tplg when we parse a topology file, we will create a SND_SOC_DOBJ_PCM type dobj AND a SND_SOC_DOBJ_DAI_LINK type dobj. Please see the chart below.

soc_tplg_load_header() case SND_SOC_TPLG_TYPE_PCM: -> soc_tplg_pcm_elems_load () -> soc_tplg_pcm_create() -> soc_tplg_dai_create() dai_drv->dobj.type = SND_SOC_DOBJ_PCM; list_add(&dai_drv->dobj.list, &tplg->comp->dobj_list); -> soc_tplg_fe_link_create() link->dobj.type = SND_SOC_DOBJ_DAI_LINK; list_add(&link->dobj.list, &tplg->comp->dobj_list);

So it is not one to one mapping.
Also, SND_SOC_TPLG_TYPE_DAI_LINK and SND_SOC_TPLG_TYPE_BACKEND_LINK are both using soc_tplg_link_elems_load() in soc_tplg_load_header(). So there is no different in dealing the two tplg type. My suggestion is as below. But it is a big change, we have to check and modify the topology file as well.

@@ -2427,10 +2427,10 @@ static int soc_tplg_load_header(struct soc_tplg *tplg, case SND_SOC_TPLG_TYPE_DAPM_WIDGET: return soc_tplg_dapm_widget_elems_load(tplg, hdr); case SND_SOC_TPLG_TYPE_PCM: + case SND_SOC_TPLG_TYPE_DAI_LINK: return soc_tplg_pcm_elems_load(tplg, hdr); case SND_SOC_TPLG_TYPE_DAI: return soc_tplg_dai_elems_load(tplg, hdr); - case SND_SOC_TPLG_TYPE_DAI_LINK: case SND_SOC_TPLG_TYPE_BACKEND_LINK: /* physical link configurations */ return soc_tplg_link_elems_load(tplg, hdr);

plbossart · 2019-01-08T13:46:00Z

unload_backend_link?

plbossart · 2019-01-08T13:47:57Z

unload it in snd_soc_tplg_component_remove, be explicit to avoid confusions with remove_link().

plbossart · 2019-01-08T13:52:19Z

You should describe what the unload_link does, since the link_load() only deals with initializations according to the comments. is this to free private data handled by the component driver and allocated by the link_load

@plbossart It is designed for deinitialization. I think it can also be used to free private data allocated by the link_load.

agree, I just want this to be mentioned in the comments. no functional change

ranj063 · 2019-01-08T17:07:32Z

@plbossart @bardliao I'm starting to question the need for this now. Looking into sof_link_unload() in topology.c, it looks like the only reason we need it is for HDA dai's and even there there's a FIXME stating the following:
/*
* FIXME: this call to hw_free is mainly to release the link DMA ID.
* This is abusing the API and handling SOC internals is not
* recommended. This part will be reworked.
*/

I think we should fix this before changing anything in soc-topology. We might end up not needing all this after all.

RanderWang · 2019-01-09T01:50:53Z

@plbossart @bardliao I'm starting to question the need for this now. Looking into sof_link_unload() in topology.c, it looks like the only reason we need it is for HDA dai's and even there there's a FIXME stating the following:
/*

FIXME: this call to hw_free is mainly to release the link DMA ID.

This is abusing the API and handling SOC internals is not

recommended. This part will be reworked.
*/

I think we should fix this before changing anything in soc-topology. We might end up not needing all this after all.

@plbossart I want to add a interface in our sof_dsp_ops to allock & free hda resource, what is your idea? I don't want to abuse the API in Asoc

ranj063 · 2019-01-09T02:27:16Z

@plbossart @bardliao I'm starting to question the need for this now. Looking into sof_link_unload() in topology.c, it looks like the only reason we need it is for HDA dai's and even there there's a FIXME stating the following:
/*

FIXME: this call to hw_free is mainly to release the link DMA ID.

This is abusing the API and handling SOC internals is not

recommended. This part will be reworked.
*/

I think we should fix this before changing anything in soc-topology. We might end up not needing all this after all.

@plbossart I want to add a interface in our sof_dsp_ops to allock & free hda resource, what is your idea? I don't want to abuse the API in Asoc

@plbossart I remember you didnt want to add another ops to alloc and free hda resource but I never really understood why. could you please shed some light on that?

bardliao · 2019-01-09T06:45:35Z

@ranj063 Agreed. However, it is different thing for me. My point is there are link_load and link_unload in snd_soc_tplg_ops {}. The link_load ops is called by BE and FE link, but link_unload is only called by FE link. People may want to use link_unload ops for whatever purpose, but it doesn't be supported by current code. For example, people may want to config a parameter in link_load and need to reset it to default in link_unload. He/She may expect the link_unload ops will be invoked by all links, but it won't. That's why I am trying to invoke the link_unload ops if the link_load ops is invoked. What do you think? @plbossart

plbossart · 2019-01-09T16:45:25Z

@plbossart @bardliao I'm starting to question the need for this now. Looking into sof_link_unload() in topology.c, it looks like the only reason we need it is for HDA dai's and even there there's a FIXME stating the following:
/*

FIXME: this call to hw_free is mainly to release the link DMA ID.

This is abusing the API and handling SOC internals is not

recommended. This part will be reworked.
*/

I think we should fix this before changing anything in soc-topology. We might end up not needing all this after all.

@plbossart I want to add a interface in our sof_dsp_ops to allock & free hda resource, what is your idea? I don't want to abuse the API in Asoc

how is this related to this thread?

plbossart · 2019-01-09T16:48:09Z

@ranj063 Agreed. However, it is different thing for me. My point is there are link_load and link_unload in snd_soc_tplg_ops {}. The link_load ops is called by BE and FE link, but link_unload is only called by FE link. People may want to use link_unload ops for whatever purpose, but it doesn't be supported by current code. For example, people may want to config a parameter in link_load and need to reset it to default in link_unload. He/She may expect the link_unload ops will be invoked by all links, but it won't. That's why I am trying to invoke the link_unload ops if the link_load ops is invoked. What do you think? @plbossart

I agree with @bardliao. If the current topology code cannot a reset a value for the next init, or release resources as needed, then it's a limitation. The proposal looks harmless to me, and it's only use if the callback is needed by an implementation.

ranj063 · 2019-01-09T16:57:09Z

@plbossart @bardliao I'm starting to question the need for this now. Looking into sof_link_unload() in topology.c, it looks like the only reason we need it is for HDA dai's and even there there's a FIXME stating the following:
/*

FIXME: this call to hw_free is mainly to release the link DMA ID.

This is abusing the API and handling SOC internals is not

recommended. This part will be reworked.
*/

I think we should fix this before changing anything in soc-topology. We might end up not needing all this after all.

@plbossart I want to add a interface in our sof_dsp_ops to allock & free hda resource, what is your idea? I don't want to abuse the API in Asoc

how is this related to this thread?

@plbossart It is not directly related to this thread but rather an extension to address the FIXME as above.

ranj063 · 2019-01-09T16:59:50Z

@ranj063 Agreed. However, it is different thing for me. My point is there are link_load and link_unload in snd_soc_tplg_ops {}. The link_load ops is called by BE and FE link, but link_unload is only called by FE link. People may want to use link_unload ops for whatever purpose, but it doesn't be supported by current code. For example, people may want to config a parameter in link_load and need to reset it to default in link_unload. He/She may expect the link_unload ops will be invoked by all links, but it won't. That's why I am trying to invoke the link_unload ops if the link_load ops is invoked. What do you think? @plbossart

I agree with @bardliao. If the current topology code cannot a reset a value for the next init, or release resources as needed, then it's a limitation. The proposal looks harmless to me, and it's only use if the callback is needed by an implementation.

@plbossart @bardliao agreed. The changes look good.

RanderWang

It is good for me

bardliao · 2019-01-10T05:35:54Z

@plbossart I add a patch which is not related to #343 to this PR. But the change is also in soc-topology.c. I guess put it in the same PR can avoid conflict. The patch is to fix a memory leak in soc_tplg_dapm_widget_create.

plbossart

Looks good, I only have minor cosmetic/wording suggestions to make it easier to merge upstream.
Thanks @bardliao , good work.

plbossart · 2019-01-10T16:28:55Z

let's be consistent with the original code and call this "remove_backend_link".

plbossart · 2019-01-10T16:29:19Z

add a comment here on why this is different from remove_link().

plbossart · 2019-01-10T16:37:54Z

Fix commit message
"soc_tplg_link_config() will find the physical dai link and call soc_tplg_dai_link_load() to load the BE dai link. Currently remove_link() is only used to remove the FE dai link which is created by the topology. The BE dai link cannot however be unloaded in snd_soc_tplg_component_remove(), which is problematic if anything needs to be released or reinitialized.

This patch aligns the definitions of dynamic types with the existing UAPI and adds a new remove_backend_link() routine to unload the the BE dai link when snd_soc_tplg_component_remove() is invoked.
"

We only handle BE links in sof_link_load. So we should only handle BE link in sof_link_unload, too. Signed-off-by: Bard liao <yung-chuan.liao@linux.intel.com>

template.sname and template.name are only freed when an error occur. They should be freed in the success return case, too. Signed-off-by: Bard liao <yung-chuan.liao@linux.intel.com>

soc_tplg_link_config() will find the physical dai link and call soc_tplg_dai_link_load() to load the BE dai link. Currently remove_link() is only used to remove the FE dai link which is created by the topology. The BE dai link cannot however be unloaded in snd_soc_tplg_component _remove(), which is problematic if anything needs to be released or reinitialized. This patch aligns the definitions of dynamic types with the existing UAPI and adds a new remove_backend_link() routine to unload the the BE dai link when snd_soc_tplg_component_remove() is invoked. Signed-off-by: Bard liao <yung-chuan.liao@linux.intel.com>

bardliao · 2019-01-11T07:27:56Z

@plbossart the PR is updated. Thanks.

plbossart

Looks good, thanks @bardliao

bardliao requested review from RanderWang, lgirdwood, mengdonglin and plbossart January 8, 2019 01:27

plbossart reviewed Jan 8, 2019

View reviewed changes

bardliao force-pushed the issue-343 branch from f85587f to be5f37f Compare January 8, 2019 08:04

plbossart reviewed Jan 8, 2019

View reviewed changes

bardliao force-pushed the issue-343 branch from be5f37f to 65c5938 Compare January 9, 2019 08:17

ranj063 approved these changes Jan 9, 2019

View reviewed changes

RanderWang approved these changes Jan 10, 2019

View reviewed changes

plbossart reviewed Jan 10, 2019

View reviewed changes

bardliao added 3 commits January 11, 2019 01:00

ASoC: SOF: topology: check no_pcm in sof_link_unload

c895dae

We only handle BE links in sof_link_load. So we should only handle BE link in sof_link_unload, too. Signed-off-by: Bard liao <yung-chuan.liao@linux.intel.com>

ASoC: topology: fix memory leak in soc_tplg_dapm_widget_create

fae9cc7

template.sname and template.name are only freed when an error occur. They should be freed in the success return case, too. Signed-off-by: Bard liao <yung-chuan.liao@linux.intel.com>

bardliao force-pushed the issue-343 branch 2 times, most recently from 9ce552c to d5d2cdd Compare January 11, 2019 06:53

plbossart approved these changes Jan 11, 2019

View reviewed changes

plbossart merged commit e9cc020 into thesofproject:topic/sof-dev Jan 11, 2019

Conversation

bardliao commented Jan 8, 2019

Uh oh!

plbossart commented Jan 8, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bardliao Jan 8, 2019 • 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!

Uh oh!

bardliao commented Jan 8, 2019

Uh oh!

plbossart commented Jan 8, 2019

Uh oh!

xiulipan commented Jan 8, 2019

Uh oh!

bardliao commented Jan 8, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Jan 8, 2019

Uh oh!

RanderWang commented Jan 9, 2019

Uh oh!

ranj063 commented Jan 9, 2019

Uh oh!

bardliao commented Jan 9, 2019

Uh oh!

plbossart commented Jan 9, 2019

Uh oh!

plbossart commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ranj063 commented Jan 9, 2019

Uh oh!

ranj063 commented Jan 9, 2019

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

bardliao commented Jan 10, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bardliao Jan 8, 2019 •

edited

Loading

plbossart commented Jan 9, 2019 •

edited

Loading