| Message ID | 390c016a47953561511f529467a1985b77cfd043.1471599648.git.mengdong.lin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 19, 2016 at 06:12:35PM +0800, mengdong.lin@linux.intel.com wrote: > Topology will check with ASoC if a BE DAI already exists by checking > its name. If the BE DAI doesn't exist, topology will create a new one > and the dai_load ops will be called for device specific init. This doesn't explain in what situation we'd want to dynamically create a back end, or why we're representing DPCM so directly in the topology - we really need more words about what the use case is and why this makes sense. The expectation would be that back ends refer to physical objects that exist in the system so it's not clear why we'd be loading them from topologies (and that even where this makes sense there wouldn't be any misuse by other users). This is a bit of a theme here, the series is adding a bunch of stuff but not explaining why.
On 08/24/2016 01:33 AM, Mark Brown wrote: > On Fri, Aug 19, 2016 at 06:12:35PM +0800, mengdong.lin@linux.intel.com wrote: > >> Topology will check with ASoC if a BE DAI already exists by checking >> its name. If the BE DAI doesn't exist, topology will create a new one >> and the dai_load ops will be called for device specific init. > > This doesn't explain in what situation we'd want to dynamically create a > back end, or why we're representing DPCM so directly in the topology - > we really need more words about what the use case is and why this makes > sense. The expectation would be that back ends refer to physical > objects that exist in the system so it's not clear why we'd be loading > them from topologies (and that even where this makes sense there > wouldn't be any misuse by other users). This is a bit of a theme here, > the series is adding a bunch of stuff but not explaining why. > Thanks for the review! Sorry for not giving enough background info. In previous design, we had thought that BE DAI and BE DAI links should be created based on ACPI info in BIOS. But unfortunately, the BIOS doesn't have enough physical information, so BE DAI & DAI links are hard coded in platform and machine driver. But when new platforms are coming out with different physical connections, this BIOS gap blocks us from sharing a generic driver across platforms. Now the gap in BIOS ACPI info still exists, the implementations also vary for different generations of platforms, BIOS for public shipped machines cannot change ... So finally we fall back to topology to overcome the BIOS gap and make driver sharing possible. We have tried creating BE DAI & DAI links to new platforms and plan to back port this to upstream drivers for existing platforms like SKL. I'll add this to the commit messages of next version. Thanks Mengdong
On Thu, Aug 25, 2016 at 02:40:34PM +0800, Mengdong Lin wrote: > In previous design, we had thought that BE DAI and BE DAI links should be > created based on ACPI info in BIOS. But unfortunately, the BIOS doesn't have > enough physical information, so BE DAI & DAI links are hard coded in > platform and machine driver. But when new platforms are coming out with > different physical connections, this BIOS gap blocks us from sharing a > generic driver across platforms. Now the gap in BIOS ACPI info still exists, > the implementations also vary for different generations of platforms, BIOS > for public shipped machines cannot change ... So finally we fall back to > topology to overcome the BIOS gap and make driver sharing possible. We have > tried creating BE DAI & DAI links to new platforms and plan to back port > this to upstream drivers for existing platforms like SKL. I don't understand why we're not able to just enumerate all the possible back ends in the driver and then select the required one at runtime - even if we do this there's going to be some fairly strict limits on the set of back ends that can be added. Do we have any concrete examples here?
Hi Mark, Here is the info from our audio platform enabling team that answers why we need this feature. Please review. Intel platform supports too many different types of DAIs based on the platform variant e.g. I2S, HDA, DMIC and in future there are few more like soundwire in the pipelines. Using this framework our attempt is to create DAI based on what is supported on the platform. If the platform does not support HDA then we would like not to create the HDA DAIs. e.g. SKL Chrome topology does not use HDA Codec and so there is only I2S and DMIC DAIs. Based on the SoC pin count limitation not all the interface pins are taken out of the SoC to connect various peripherals. So even though technically hardware supports many physical interfaces but the platform may have limited what can be connected on the platform. Surely we can create worst case number of DAIs but that would be too much. Here is the example of SKL/BXT: 6 SSP Ports Rx and Tx, 2 DMIC Ports, 7 Playback DMA channels, 6 Capture DMA channels.We are creating topology diagram in text to explain different configuration for different use case,like Android, Linux, Chrome and Automotive. Thanks Mengdong On 08/28/2016 10:12 PM, Mark Brown wrote: > On Thu, Aug 25, 2016 at 02:40:34PM +0800, Mengdong Lin wrote: > >> In previous design, we had thought that BE DAI and BE DAI links should be >> created based on ACPI info in BIOS. But unfortunately, the BIOS doesn't have >> enough physical information, so BE DAI & DAI links are hard coded in >> platform and machine driver. But when new platforms are coming out with >> different physical connections, this BIOS gap blocks us from sharing a >> generic driver across platforms. Now the gap in BIOS ACPI info still exists, >> the implementations also vary for different generations of platforms, BIOS >> for public shipped machines cannot change ... So finally we fall back to >> topology to overcome the BIOS gap and make driver sharing possible. We have >> tried creating BE DAI & DAI links to new platforms and plan to back port >> this to upstream drivers for existing platforms like SKL. > > I don't understand why we're not able to just enumerate all the possible > back ends in the driver and then select the required one at runtime > - even if we do this there's going to be some fairly strict limits on > the set of back ends that can be added. Do we have any concrete > examples here? >
On Tue, Aug 30, 2016 at 12:42:06PM +0800, Mengdong Lin wrote: > Surely we can create worst case number of DAIs but that would be too much. > Here is the example of SKL/BXT: > 6 SSP Ports Rx and Tx, 2 DMIC Ports, 7 Playback DMA channels, 6 Capture DMA > channels.We are creating topology diagram in text to explain different > configuration for different use case,like Android, Linux, Chrome and > Automotive. That's just 14 back ends by my count (the SSP and DMIC ports) which seems totally managable? Even if you include the DMA it's still not an absurdly large number especially if you compare it to something like the number of widgets we actively manage in a running CODEC.
diff --git a/include/sound/soc-topology.h b/include/sound/soc-topology.h index d318fe4..9ebb9f2 100644 --- a/include/sound/soc-topology.h +++ b/include/sound/soc-topology.h @@ -39,6 +39,7 @@ enum snd_soc_dobj_type { SND_SOC_DOBJ_ENUM, SND_SOC_DOBJ_BYTES, SND_SOC_DOBJ_PCM, + SND_SOC_DOBJ_BE_DAI, SND_SOC_DOBJ_DAI_LINK, SND_SOC_DOBJ_CODEC_LINK, SND_SOC_DOBJ_WIDGET, diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 84e3fa1..f6f5027 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2872,7 +2872,8 @@ int snd_soc_register_dai(struct snd_soc_component *component, struct snd_soc_dai *dai; int ret; - if (dai_drv->dobj.type != SND_SOC_DOBJ_PCM) { + if (!(dai_drv->dobj.type == SND_SOC_DOBJ_PCM || + dai_drv->dobj.type == SND_SOC_DOBJ_BE_DAI)) { dev_err(component->dev, "Invalid dai type %d\n", dai_drv->dobj.type); return -EINVAL; diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index a9e83a2..1e26dc6 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -1710,42 +1710,14 @@ static int soc_tplg_pcm_elems_load(struct soc_tplg *tplg, return 0; } -/* * - * soc_tplg_be_dai_config - Find and configure an existing BE DAI. - * @tplg: topology context - * @be: topology BE DAI configs. - * - * The BE dai should already be registered by the platform driver. The - * platform driver should specify the BE DAI name and ID for matching. - */ -static int soc_tplg_be_dai_config(struct soc_tplg *tplg, +static int config_be_dai(struct soc_tplg *tplg, + struct snd_soc_dai_driver *dai_drv, struct snd_soc_tplg_be_dai *be) { - struct snd_soc_dai_link_component dai_component = {0}; - struct snd_soc_dai *dai; - struct snd_soc_dai_driver *dai_drv; struct snd_soc_pcm_stream *stream; struct snd_soc_tplg_stream_caps *caps; int ret; - dai_component.dai_name = be->dai_name; - dai = snd_soc_find_dai(&dai_component); - if (!dai) { - dev_err(tplg->dev, "ASoC: BE DAI %s not registered\n", - be->dai_name); - return -EINVAL; - } - - if (be->dai_id != dai->id) { - dev_err(tplg->dev, "ASoC: BE DAI %s id mismatch\n", - be->dai_name); - return -EINVAL; - } - - dai_drv = dai->driver; - if (!dai_drv) - return -EINVAL; - if (be->playback) { stream = &dai_drv->playback; caps = &be->caps[SND_SOC_TPLG_STREAM_PLAYBACK]; @@ -1764,13 +1736,80 @@ static int soc_tplg_be_dai_config(struct soc_tplg *tplg, /* pass control to component driver for optional further init */ ret = soc_tplg_dai_load(tplg, dai_drv); if (ret < 0) { - dev_err(tplg->comp->dev, "ASoC: DAI loading failed\n"); + dev_err(tplg->comp->dev, "ASoC: BE DAI loading failed\n"); return ret; } return 0; } +static int soc_tplg_be_dai_create(struct soc_tplg *tplg, + struct snd_soc_tplg_be_dai *be) +{ + struct snd_soc_dai_driver *dai_drv; + int ret; + + dai_drv = kzalloc(sizeof(struct snd_soc_dai_driver), GFP_KERNEL); + if (dai_drv == NULL) + return -ENOMEM; + + dai_drv->name = be->dai_name; + dai_drv->id = be->dai_id; + + ret = config_be_dai(tplg, dai_drv, be); + if (ret < 0) { + kfree(dai_drv); + return ret; + } + + dai_drv->dobj.index = tplg->index; + dai_drv->dobj.ops = tplg->ops; + dai_drv->dobj.type = SND_SOC_DOBJ_BE_DAI; + list_add(&dai_drv->dobj.list, &tplg->comp->dobj_list); + + /* register the DAI to the component */ + return snd_soc_register_dai(tplg->comp, dai_drv); +} + +/* * + * soc_tplg_be_dai_config - Create a new BE DAI or configure an existing one. + * @tplg: topology context + * @be: topology BE DAI configs. + * + * The BE dai should already be registered by the platform driver. The + * platform driver should specify the BE DAI name and ID for matching. + */ +static int soc_tplg_be_dai_config(struct soc_tplg *tplg, + struct snd_soc_tplg_be_dai *be) +{ + struct snd_soc_dai_link_component dai_component = {0}; + struct snd_soc_dai *dai; + struct snd_soc_dai_driver *dai_drv; + + if (!strlen(be->dai_name)) { + dev_err(tplg->dev, "ASoC: Invalid BE DAI name\n"); + return -EINVAL; + } + + dai_component.dai_name = be->dai_name; + dai = snd_soc_find_dai(&dai_component); + if (!dai) /* BE DAI doesn't exist, create it */ + return soc_tplg_be_dai_create(tplg, be); + + /* configure an existing BE DAI */ + if (be->dai_id != dai->id) { + dev_err(tplg->dev, "ASoC: BE DAI %s id mismatch\n", + be->dai_name); + return -EINVAL; + } + + dai_drv = dai->driver; + if (!dai_drv) + return -EINVAL; + + return config_be_dai(tplg, dai_drv, be); +} + static int soc_tplg_be_dai_elems_load(struct soc_tplg *tplg, struct snd_soc_tplg_hdr *hdr) { @@ -2062,6 +2101,9 @@ int snd_soc_tplg_component_remove(struct snd_soc_component *comp, u32 index) case SND_SOC_DOBJ_PCM: remove_dai(comp, dobj, pass); break; + case SND_SOC_DOBJ_BE_DAI: + remove_dai(comp, dobj, pass); + break; case SND_SOC_DOBJ_DAI_LINK: remove_link(comp, dobj, pass); break;