| Message ID | 0364569ce2898a20d08243ba59a92973b2dc4735.1439217448.git.mengdong.lin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 10, 2015 at 10:47:47PM +0800, mengdong.lin@intel.com wrote: > Add dummy flag to a DAI link and PCM runtime. A dummy link will create > a dummy PCM runtime. > A machine driver can define dummy dai links to specify the expected > platform and codec. And its dummy runtime is used to probe the platform > and codec components, which can bring real DAI/DAI links based on topology. > After components probing, all dummy PCM runtimes will be removed. This isn't very clear to me either in terms of the description or the interface. As far as I can tell these links that are being created here are placeholders representing links that will actually be instantiated later but if that's the case they're not really dummies as I'd understand them, they are real links which physically exist and will be created later by topology data. If we are going to do things this way we need a better way of talking about these not yet instantiated links. I'm really confused as to why physical links that exist in the board are coming in via topology information loaded from userspace - it's not like the DSP firmware is going to be able to physically connect the DSP to things where no links existed before. I'd expect that these links are connections between physical devices which just exist in the system and that while the topology information might leave some of them dangling they wouldn't be created or removed. Presumably we're also going to need to handle the case where the firmware is unloaded and we destroy the links if we do this...
> -----Original Message----- > From: Mark Brown [mailto:broonie@kernel.org] > Sent: Saturday, August 15, 2015 4:22 AM > On Mon, Aug 10, 2015 at 10:47:47PM +0800, mengdong.lin@intel.com wrote: > > > Add dummy flag to a DAI link and PCM runtime. A dummy link will create > > a dummy PCM runtime. > > > A machine driver can define dummy dai links to specify the expected > > platform and codec. And its dummy runtime is used to probe the > > platform and codec components, which can bring real DAI/DAI links based > on topology. > > After components probing, all dummy PCM runtimes will be removed. > > This isn't very clear to me either in terms of the description or the interface. > As far as I can tell these links that are being created here are placeholders > representing links that will actually be instantiated later but if that's the case > they're not really dummies as I'd understand them, they are real links which > physically exist and will be created later by topology data. If we are going to > do things this way we need a better way of talking about these not yet > instantiated links. (Sorry. I'm still trying to enable my 2nd mail account & client. So this reply can still have format problems.) These *dummy* links are not place holders of real physical links. So they cannot come from ACPI but from topology. Here is an example used in my test on Broadwell: /* broadwell digital audio interface glue - connects codec <--> CPU */ static struct snd_soc_dai_link broadwell_rt286_dais[] = { /* Front End dummy DAI links */ { .name = "Dummy System PCM", .stream_name = "Dummy System Playback/Capture", .cpu_dai_name = "snd-soc-dummy-dai", .platform_name = "haswell-pcm-audio", .dynamic = 1, .codec_name = "snd-soc-dummy", .codec_dai_name = "snd-soc-dummy-dai", } ... All real FE DAI links will no longer be in this link array. We use this dummy DAI link to avoid snd_soc_instantiate_card() fail on binding DAI links. And since this dummy link specify the platform "haswell-pcm-audio", soc_bind_dai_link() will promise the platform component is there. And then platform probing will load topology and the topology core will create the real FE DAIs. And the machine driver will get notified when a new FE DAI is added to the ASoC core. It can create FE links in the callback context according to the new FE DAI info. This seems to give us small code change. Thanks Mengdong > I'm really confused as to why physical links that exist in the board are coming > in via topology information loaded from userspace - it's not like the DSP > firmware is going to be able to physically connect the DSP to things where no > links existed before. I'd expect that these links are connections between > physical devices which just exist in the system and that while the topology > information might leave some of them dangling they wouldn't be created or > removed. > > Presumably we're also going to need to handle the case where the firmware > is unloaded and we destroy the links if we do this... Yes. The DAI/DAI links created by topology should be destroyed when unloading the topology component. Now there is no product request to unload/change the firmware as well as its topology in the middle. We may consider this later. Thanks Mengdong
On Mon, Aug 17, 2015 at 10:01:16AM +0000, Lin, Mengdong wrote: > These *dummy* links are not place holders of real physical links. > So they cannot come from ACPI but from topology. OK, so this is something that should have been apparent from the changelog and if that's what's supposed to happen that shouldn be what the code does - it needs to be limited to these software only links. But honestly this just doesn't seem like the ideal design - what this means is that we're going to need to have a dummy link in place for any link that is defined by the topology which means that we've got a hard coded limit in the drivers for this dynamically defined thing from the firmware. It'd be much nicer if we just created the links from scratch when we detect them in firmware rather than requiring these stubs.
diff --git a/include/sound/soc.h b/include/sound/soc.h index 79891e2..7e04ec0 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -970,6 +970,11 @@ struct snd_soc_dai_link { enum snd_soc_dpcm_trigger trigger[2]; /* trigger type for DPCM */ + /* This DAI link is dummy. Machine drivers can define static dummy + * links to specify platform/codec with topology. + */ + bool dummy; + /* codec/machine specific init - e.g. add machine controls */ int (*init)(struct snd_soc_pcm_runtime *rtd); @@ -1087,6 +1092,7 @@ struct snd_soc_card { struct list_head rtd_list; int num_rtd; + int num_rtd_dev; /* monotonic increase and exclude dummy ones */ /* optional codec specific configuration */ struct snd_soc_codec_conf *codec_conf; @@ -1181,6 +1187,7 @@ struct snd_soc_pcm_runtime { struct dentry *debugfs_dpcm_state; #endif + bool dummy; unsigned int num; /* 0-based and monotonic increasing */ struct list_head list; /* rtd list of the soc card */ }; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 58eac57..f552fa0 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -561,6 +561,7 @@ static struct snd_soc_pcm_runtime *soc_new_pcm_runtime( rtd->card = card; rtd->dai_link = dai_link; + rtd->dummy = dai_link->dummy; rtd->codec_dais = devm_kzalloc(card->dev, sizeof(struct snd_soc_dai *) * dai_link->num_codecs, @@ -583,10 +584,28 @@ static void soc_add_pcm_runtime(struct snd_soc_card *card, struct snd_soc_pcm_runtime *rtd) { list_add_tail(&rtd->list, &card->rtd_list); - rtd->num = card->num_rtd; + + if (!rtd->dummy) { + rtd->num = card->num_rtd_dev; + card->num_rtd_dev++; + } + card->num_rtd++; } +static void soc_remove_dummy_pcm_runtimes(struct snd_soc_card *card) +{ + struct snd_soc_pcm_runtime *rtd, *_rtd; + + list_for_each_entry_safe(rtd, _rtd, &card->rtd_list, list) { + if (rtd->dummy) { + list_del(&rtd->list); + soc_free_pcm_runtime(card, rtd); + card->num_rtd--; + } + } +} + static void soc_remove_pcm_runtimes(struct snd_soc_card *card) { struct snd_soc_pcm_runtime *rtd, *_rtd; @@ -1007,6 +1026,15 @@ static int soc_bind_dai_link(struct snd_soc_card *card, } } + /* Dummy link can use dummy cpu DAI but set platform to get topology */ + if (dai_link->dummy && dai_link->platform_name) { + if (!soc_find_component(NULL, dai_link->platform_name)) { + dev_err(card->dev, "ASoC: Platform %s not registered\n", + dai_link->platform_name); + goto _err_defer; + } + } + /* Single codec links expect codec and codec_dai in runtime data */ rtd->codec_dai = codec_dais[0]; rtd->codec = rtd->codec_dai->codec; @@ -1250,6 +1278,10 @@ static int soc_init_dai_link(struct snd_soc_card *card, void snd_soc_add_dai_link(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link) { + /* Dummy links should be static, so overlook them */ + if (dai_link->dummy) + return; + lockdep_assert_held(&client_mutex); list_add_tail(&dai_link->list, &card->dai_link_list); card->num_dai_links++; @@ -1818,6 +1850,9 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) } } + /* no longer need dummy runtimes */ + soc_remove_dummy_pcm_runtimes(card); + /* probe all DAI links on this card */ for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST; order++) { @@ -2516,6 +2551,7 @@ int snd_soc_register_card(struct snd_soc_card *card) INIT_LIST_HEAD(&card->rtd_list); card->num_rtd = 0; + card->num_rtd_dev = 0; card->rtd_aux = devm_kzalloc(card->dev, sizeof(struct snd_soc_pcm_runtime) *