| Message ID | 20220207132532.3782412-10-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: Intel: avs: Topology and path management | expand |
On 2/7/22 07:25, Cezary Rojewski wrote: > To implement ASoC PCM operations, DSP path handling is needed. With path > template concept present, information carried by topology file can be > converted into runtime path representation. Each may be composed of > several pipelines and each pipeline can contain a number of processing it's not quite clear how you can have different pipelines with a single dma_is per path? > modules inside. Number of templates and variants found within topology > may vastly outnumber the total amount of pipelines and modules supported > by AudioDSP firmware simultaneously (in runtime) so none of the IDs are > specified in the topology. These are assigned dynamically when needed > and account for limitations described by FIRMWARE_CONFIG and > HARDWARE_CONFIG basefw parameters. It's already quite hard to create a topology using static IDs that will work, this dynamic creation adds an element of risk and validation, doesn't it? Can you clarify how you validated this dynamic capability? > Paths are created on ->hw_params() and are freed on ->hw_free() ALSA PCM > operations. This choice is based on firmware expectations - need for So a path seems to be completely tied to an FE then? That would mean that a 'dai pipeline' with 'mixin-dai-copier' would not be managed by an avs-path, and would somehow need to be autonomously created? You really need to clarify how basic topologies with mixers/demux/loopbacks are handled. > complete set of information when attempting to instantiate pipelines and > modules on AudioDSP side. With DMA and audio format provided, search > mechanism tests all path variants available in given path template until > a matching variant is found. Once found, information already available > is combined with all avs_tplg_* pieces pointed by matching path variant. > This finally allows to begin a cascade of IPCs which goes is to reserve this sentence makes no sense. did you mean 'which goals'? > resources and prepare DSP for upcoming audio streaming. > +static struct avs_tplg_path * > +avs_path_find_variant(struct avs_dev *adev, > + struct avs_tplg_path_template *template, > + struct snd_pcm_hw_params *fe_params, > + struct snd_pcm_hw_params *be_params) > +{ > + struct avs_tplg_path *variant; > + > + list_for_each_entry(variant, &template->path_list, node) { > + dev_dbg(adev->dev, "check FE rate %d chn %d vbd %d bd %d\n", > + variant->fe_fmt->sampling_freq, variant->fe_fmt->num_channels, > + variant->fe_fmt->valid_bit_depth, variant->fe_fmt->bit_depth); > + dev_dbg(adev->dev, "check BE rate %d chn %d vbd %d bd %d\n", > + variant->be_fmt->sampling_freq, variant->be_fmt->num_channels, > + variant->be_fmt->valid_bit_depth, variant->be_fmt->bit_depth); > + > + if (variant->fe_fmt && avs_test_hw_params(fe_params, variant->fe_fmt) && > + variant->be_fmt && avs_test_hw_params(be_params, variant->be_fmt)) > + return variant; > + } This matching between FE and BE formats is the key problem with this patchset IMHO. We need the ability to reconfigure the BE based on constraint matching, not exact match with a fixed value! > + > + return NULL; > +} > + > +static void avs_path_module_free(struct avs_dev *adev, struct avs_path_module *mod) > +{ > + kfree(mod); > +} > + > +static struct avs_path_module * > +avs_path_module_create(struct avs_dev *adev, > + struct avs_path_pipeline *owner, > + struct avs_tplg_module *template) so you have templates for paths AND modules? Completely lost... I'll skip the rest of this patch.
On 2022-02-25 8:36 PM, Pierre-Louis Bossart wrote: > On 2/7/22 07:25, Cezary Rojewski wrote: >> To implement ASoC PCM operations, DSP path handling is needed. With path >> template concept present, information carried by topology file can be >> converted into runtime path representation. Each may be composed of >> several pipelines and each pipeline can contain a number of processing > > it's not quite clear how you can have different pipelines with a single > dma_is per path? Pipelines do not need to have a module that is connected to a gateway. Layout of a path is for architecture to decide i.e. one takes spreadsheet of all the scenarios to be supported and developers/archs discuss/decide what's the best and optimal way to shape the paths that implement all the possible scenarios. >> modules inside. Number of templates and variants found within topology >> may vastly outnumber the total amount of pipelines and modules supported >> by AudioDSP firmware simultaneously (in runtime) so none of the IDs are >> specified in the topology. These are assigned dynamically when needed >> and account for limitations described by FIRMWARE_CONFIG and >> HARDWARE_CONFIG basefw parameters. > > It's already quite hard to create a topology using static IDs that will > work, this dynamic creation adds an element of risk and validation, > doesn't it? > > Can you clarify how you validated this dynamic capability? Static ID assignment i.e. taking IDs found in the topology file directly when instantiating pipelines/modules in runtime is asking for trouble. Firmware has its limitations in terms of number of pipelines/modules supported simultaneously. Driver has to take these into account. Here, we send an IPC to obtain all the limitations before any stream could be opened for streaming. >> Paths are created on ->hw_params() and are freed on ->hw_free() ALSA PCM >> operations. This choice is based on firmware expectations - need for > > So a path seems to be completely tied to an FE then? > > That would mean that a 'dai pipeline' with 'mixin-dai-copier' would not > be managed by an avs-path, and would somehow need to be autonomously > created? > > You really need to clarify how basic topologies with > mixers/demux/loopbacks are handled. Path is either tied to a single FE or a BE. Don't understand what's difficult with handling mixin/copier pipeliens or loopback scenario here. We have all the tools necessary to do the job, no? >> complete set of information when attempting to instantiate pipelines and >> modules on AudioDSP side. With DMA and audio format provided, search >> mechanism tests all path variants available in given path template until >> a matching variant is found. Once found, information already available >> is combined with all avs_tplg_* pieces pointed by matching path variant. >> This finally allows to begin a cascade of IPCs which goes is to reserve > > this sentence makes no sense. > > did you mean 'which goals'? Ack, thanks! >> resources and prepare DSP for upcoming audio streaming. > >> +static struct avs_tplg_path * >> +avs_path_find_variant(struct avs_dev *adev, >> + struct avs_tplg_path_template *template, >> + struct snd_pcm_hw_params *fe_params, >> + struct snd_pcm_hw_params *be_params) >> +{ >> + struct avs_tplg_path *variant; >> + >> + list_for_each_entry(variant, &template->path_list, node) { >> + dev_dbg(adev->dev, "check FE rate %d chn %d vbd %d bd %d\n", >> + variant->fe_fmt->sampling_freq, variant->fe_fmt->num_channels, >> + variant->fe_fmt->valid_bit_depth, variant->fe_fmt->bit_depth); >> + dev_dbg(adev->dev, "check BE rate %d chn %d vbd %d bd %d\n", >> + variant->be_fmt->sampling_freq, variant->be_fmt->num_channels, >> + variant->be_fmt->valid_bit_depth, variant->be_fmt->bit_depth); >> + >> + if (variant->fe_fmt && avs_test_hw_params(fe_params, variant->fe_fmt) && >> + variant->be_fmt && avs_test_hw_params(be_params, variant->be_fmt)) >> + return variant; >> + } > > This matching between FE and BE formats is the key problem with this > patchset IMHO. > > We need the ability to reconfigure the BE based on constraint matching, > not exact match with a fixed value! We need to take into account what's set on the codec side too, can't just ignore it. Topology is written for concrete configuration, so we can crease a file that supports all possible configurations exposed by given codec. >> + >> + return NULL; >> +} >> + >> +static void avs_path_module_free(struct avs_dev *adev, struct avs_path_module *mod) >> +{ >> + kfree(mod); >> +} >> + >> +static struct avs_path_module * >> +avs_path_module_create(struct avs_dev *adev, >> + struct avs_path_pipeline *owner, >> + struct avs_tplg_module *template) > > so you have templates for paths AND modules? > > Completely lost... > > I'll skip the rest of this patch. All the objects here have topology and runtime representation both. Again, you cannot just take what's within a static topology file that knows nothing about firmware limitations and expect success. Don't understand what's surprising here. skylake-driver also has a separate representation for module within topology and a separate one for runtime. Nothing new here. Regards, Czarek
On 2022-03-21 6:19 PM, Cezary Rojewski wrote: > We need to take into account what's set on the codec side too, can't > just ignore it. Topology is written for concrete configuration, so we > can crease a file that supports all possible configurations exposed by > given codec. By "Topology is written for concrete configuration" I meant platform-level configuration e.g.: Skylake with single rt286 codec in I2S mode. Regards, Czarek
diff --git a/sound/soc/intel/avs/Makefile b/sound/soc/intel/avs/Makefile index 5e12a3a89e64..952f51977656 100644 --- a/sound/soc/intel/avs/Makefile +++ b/sound/soc/intel/avs/Makefile @@ -1,7 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only snd-soc-avs-objs := dsp.o ipc.o messages.o utils.o core.o loader.o \ - topology.o + topology.o path.o snd-soc-avs-objs += cldma.o obj-$(CONFIG_SND_SOC_INTEL_AVS) += snd-soc-avs.o diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h index b0c17c45d7c6..313001b0455f 100644 --- a/sound/soc/intel/avs/avs.h +++ b/sound/soc/intel/avs/avs.h @@ -91,6 +91,12 @@ struct avs_dev { char **lib_names; struct completion fw_ready; + + struct list_head comp_list; + struct mutex comp_list_mutex; + struct list_head path_list; + spinlock_t path_list_lock; + struct mutex path_mutex; }; /* from hda_bus to avs_dev */ diff --git a/sound/soc/intel/avs/path.c b/sound/soc/intel/avs/path.c new file mode 100644 index 000000000000..272d868bedc9 --- /dev/null +++ b/sound/soc/intel/avs/path.c @@ -0,0 +1,287 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// Copyright(c) 2021 Intel Corporation. All rights reserved. +// +// Authors: Cezary Rojewski <cezary.rojewski@intel.com> +// Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com> +// + +#include <sound/intel-nhlt.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include "avs.h" +#include "path.h" +#include "topology.h" + +static bool avs_test_hw_params(struct snd_pcm_hw_params *params, + struct avs_audio_format *fmt) +{ + return (params_rate(params) == fmt->sampling_freq && + params_channels(params) == fmt->num_channels && + params_physical_width(params) == fmt->bit_depth && + params_width(params) == fmt->valid_bit_depth); +} + +static struct avs_tplg_path * +avs_path_find_variant(struct avs_dev *adev, + struct avs_tplg_path_template *template, + struct snd_pcm_hw_params *fe_params, + struct snd_pcm_hw_params *be_params) +{ + struct avs_tplg_path *variant; + + list_for_each_entry(variant, &template->path_list, node) { + dev_dbg(adev->dev, "check FE rate %d chn %d vbd %d bd %d\n", + variant->fe_fmt->sampling_freq, variant->fe_fmt->num_channels, + variant->fe_fmt->valid_bit_depth, variant->fe_fmt->bit_depth); + dev_dbg(adev->dev, "check BE rate %d chn %d vbd %d bd %d\n", + variant->be_fmt->sampling_freq, variant->be_fmt->num_channels, + variant->be_fmt->valid_bit_depth, variant->be_fmt->bit_depth); + + if (variant->fe_fmt && avs_test_hw_params(fe_params, variant->fe_fmt) && + variant->be_fmt && avs_test_hw_params(be_params, variant->be_fmt)) + return variant; + } + + return NULL; +} + +static void avs_path_module_free(struct avs_dev *adev, struct avs_path_module *mod) +{ + kfree(mod); +} + +static struct avs_path_module * +avs_path_module_create(struct avs_dev *adev, + struct avs_path_pipeline *owner, + struct avs_tplg_module *template) +{ + struct avs_path_module *mod; + int module_id; + + module_id = avs_get_module_id(adev, &template->cfg_ext->type); + if (module_id < 0) + return ERR_PTR(module_id); + + mod = kzalloc(sizeof(*mod), GFP_KERNEL); + if (!mod) + return ERR_PTR(-ENOMEM); + + mod->template = template; + mod->module_id = module_id; + mod->owner = owner; + INIT_LIST_HEAD(&mod->node); + + return mod; +} + +static void avs_path_binding_free(struct avs_dev *adev, struct avs_path_binding *binding) +{ + kfree(binding); +} + +static struct avs_path_binding *avs_path_binding_create(struct avs_dev *adev, + struct avs_path_pipeline *owner, + struct avs_tplg_binding *t) +{ + struct avs_path_binding *binding; + + binding = kzalloc(sizeof(*binding), GFP_KERNEL); + if (!binding) + return ERR_PTR(-ENOMEM); + + binding->template = t; + binding->owner = owner; + INIT_LIST_HEAD(&binding->node); + + return binding; +} + +static void avs_path_pipeline_free(struct avs_dev *adev, + struct avs_path_pipeline *ppl) +{ + struct avs_path_binding *binding, *bsave; + struct avs_path_module *mod, *save; + + list_for_each_entry_safe(binding, bsave, &ppl->binding_list, node) { + list_del(&binding->node); + avs_path_binding_free(adev, binding); + } + + avs_dsp_delete_pipeline(adev, ppl->instance_id); + + /* Unload resources occupied by owned modules */ + list_for_each_entry_safe(mod, save, &ppl->mod_list, node) { + avs_dsp_delete_module(adev, mod->module_id, mod->instance_id, + mod->owner->instance_id, + mod->template->core_id); + avs_path_module_free(adev, mod); + } + + list_del(&ppl->node); + kfree(ppl); +} + +static struct avs_path_pipeline * +avs_path_pipeline_create(struct avs_dev *adev, struct avs_path *owner, + struct avs_tplg_pipeline *template) +{ + struct avs_path_pipeline *ppl; + struct avs_tplg_pplcfg *cfg = template->cfg; + struct avs_tplg_module *tmod; + int ret, i; + + ppl = kzalloc(sizeof(*ppl), GFP_KERNEL); + if (!ppl) + return ERR_PTR(-ENOMEM); + + ppl->template = template; + ppl->owner = owner; + INIT_LIST_HEAD(&ppl->binding_list); + INIT_LIST_HEAD(&ppl->mod_list); + INIT_LIST_HEAD(&ppl->node); + + ret = avs_dsp_create_pipeline(adev, cfg->req_size, cfg->priority, + cfg->lp, cfg->attributes, + &ppl->instance_id); + if (ret) { + dev_err(adev->dev, "error creating pipeline %d\n", ret); + kfree(ppl); + return ERR_PTR(ret); + } + + list_for_each_entry(tmod, &template->mod_list, node) { + struct avs_path_module *mod; + + mod = avs_path_module_create(adev, ppl, tmod); + if (IS_ERR(mod)) { + ret = PTR_ERR(mod); + dev_err(adev->dev, "error creating module %d\n", ret); + goto init_err; + } + + list_add_tail(&mod->node, &ppl->mod_list); + } + + for (i = 0; i < template->num_bindings; i++) { + struct avs_path_binding *binding; + + binding = avs_path_binding_create(adev, ppl, template->bindings[i]); + if (IS_ERR(binding)) { + ret = PTR_ERR(binding); + dev_err(adev->dev, "error creating binding %d\n", ret); + goto init_err; + } + + list_add_tail(&binding->node, &ppl->binding_list); + } + + return ppl; + +init_err: + avs_path_pipeline_free(adev, ppl); + return ERR_PTR(ret); +} + +static int avs_path_init(struct avs_dev *adev, struct avs_path *path, + struct avs_tplg_path *template, u32 dma_id) +{ + struct avs_tplg_pipeline *tppl; + + path->owner = adev; + path->template = template; + path->dma_id = dma_id; + INIT_LIST_HEAD(&path->ppl_list); + INIT_LIST_HEAD(&path->node); + + /* create all the pipelines */ + list_for_each_entry(tppl, &template->ppl_list, node) { + struct avs_path_pipeline *ppl; + + ppl = avs_path_pipeline_create(adev, path, tppl); + if (IS_ERR(ppl)) + return PTR_ERR(ppl); + + list_add_tail(&ppl->node, &path->ppl_list); + } + + spin_lock(&adev->path_list_lock); + list_add_tail(&path->node, &adev->path_list); + spin_unlock(&adev->path_list_lock); + + return 0; +} + +static void avs_path_free_unlocked(struct avs_path *path) +{ + struct avs_path_pipeline *ppl, *save; + + spin_lock(&path->owner->path_list_lock); + list_del(&path->node); + spin_unlock(&path->owner->path_list_lock); + + list_for_each_entry_safe(ppl, save, &path->ppl_list, node) + avs_path_pipeline_free(path->owner, ppl); + + kfree(path); +} + +static struct avs_path *avs_path_create_unlocked(struct avs_dev *adev, u32 dma_id, + struct avs_tplg_path *template) +{ + struct avs_soc_component *acomp; + struct avs_path *path; + int ret; + + acomp = to_avs_soc_component(template->owner->owner->comp); + + path = kzalloc(sizeof(*path), GFP_KERNEL); + if (!path) + return ERR_PTR(-ENOMEM); + + ret = avs_path_init(adev, path, template, dma_id); + if (ret < 0) + goto err; + + path->state = AVS_PPL_STATE_INVALID; + return path; +err: + avs_path_free_unlocked(path); + return ERR_PTR(ret); +} + +void avs_path_free(struct avs_path *path) +{ + struct avs_dev *adev = path->owner; + + mutex_lock(&adev->path_mutex); + avs_path_free_unlocked(path); + mutex_unlock(&adev->path_mutex); +} + +struct avs_path *avs_path_create(struct avs_dev *adev, u32 dma_id, + struct avs_tplg_path_template *template, + struct snd_pcm_hw_params *fe_params, + struct snd_pcm_hw_params *be_params) +{ + struct avs_tplg_path *variant; + struct avs_path *path; + + variant = avs_path_find_variant(adev, template, fe_params, be_params); + if (!variant) { + dev_err(adev->dev, "no matching variant found\n"); + return ERR_PTR(-ENOENT); + } + + /* Serialize path and its components creation. */ + mutex_lock(&adev->path_mutex); + /* Satisfy needs of avs_path_find_tplg(). */ + mutex_lock(&adev->comp_list_mutex); + + path = avs_path_create_unlocked(adev, dma_id, variant); + + mutex_unlock(&adev->comp_list_mutex); + mutex_unlock(&adev->path_mutex); + + return path; +} diff --git a/sound/soc/intel/avs/path.h b/sound/soc/intel/avs/path.h index ecfb687fdf36..3843e5a062a1 100644 --- a/sound/soc/intel/avs/path.h +++ b/sound/soc/intel/avs/path.h @@ -57,4 +57,10 @@ struct avs_path_binding { struct list_head node; }; +void avs_path_free(struct avs_path *path); +struct avs_path *avs_path_create(struct avs_dev *adev, u32 dma_id, + struct avs_tplg_path_template *template, + struct snd_pcm_hw_params *fe_params, + struct snd_pcm_hw_params *be_params); + #endif