| Message ID | 20220207132532.3782412-9-cezary.rojewski@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: Intel: avs: Topology and path management | expand |
On 2/7/22 07:25, Cezary Rojewski wrote: > Declare representatives for all crucial elements which stream on ADSP > side is made of. That covers pipelines and modules subject which are > presented by struct avs_path_pipeline and avs_path_module respetively. respectively > While struct avs_path_binding and struct avs_path do not represent any > object on firmware side directly, they are needed to help track the > interconnections and membership of every pipeline and module created. > +struct avs_path { > + u32 dma_id; that is very surprising... This would seem to limit the concept of an avs path to a single host DMA channel, which somewhat contradicts that there can be multiple pipelines in the same path, or that a path can contain mixers. And even if this is a single dma, what does this represent? the stream_tag? the BE DMA for SSP/DMIC? Please clarify the concepts first, it's frustrating to discover this at patch 8. > + struct list_head ppl_list; > + u32 state; > + > + struct avs_tplg_path *template; > + struct avs_dev *owner; > + /* device path management */ > + struct list_head node; > +}; > +struct avs_path_binding { > + struct avs_path_module *source; > + u8 source_pin; > + struct avs_path_module *sink; > + u8 sink_pin; > + > + struct avs_tplg_binding *template; I must admit not clearly seeing how the definitions of 'avs_path_binding' and 'avs_tplg_binding' are related. More specifically, having a template for something that directly connects a source to a sink is far from intuitive. > + struct avs_path_pipeline *owner; > + /* pipeline bindings management */ > + struct list_head node; > +}; > + > +#endif
On 2022-02-25 8:25 PM, Pierre-Louis Bossart wrote: > On 2/7/22 07:25, Cezary Rojewski wrote: >> Declare representatives for all crucial elements which stream on ADSP >> side is made of. That covers pipelines and modules subject which are >> presented by struct avs_path_pipeline and avs_path_module respetively. > > respectively Ack. >> While struct avs_path_binding and struct avs_path do not represent any >> object on firmware side directly, they are needed to help track the >> interconnections and membership of every pipeline and module created. > >> +struct avs_path { >> + u32 dma_id; > > that is very surprising... > > This would seem to limit the concept of an avs path to a single host DMA > channel, which somewhat contradicts that there can be multiple pipelines > in the same path, or that a path can contain mixers. > > And even if this is a single dma, what does this represent? the > stream_tag? the BE DMA for SSP/DMIC? > > Please clarify the concepts first, it's frustrating to discover this at > patch 8. A single path is tied to either FE or BE. So at most to a single, user-visible endpoint if it's FE. If there are more FEs, then we move to NxFE <-> 1xBE scenario. You can have many pipelines forming the path - most of the pipelines do not contain module connected to any gateway (HDA/SSP/DMIC etc.) anyway. Yes, dma_id represents any DMA i.e. HDA stream (DMA), SSP port, DMIC fifo etc. And it's not a concept, it's just a member of a structure. Similar field exists in skylake-driver topology too (it's called "port"). >> + struct list_head ppl_list; >> + u32 state; >> + >> + struct avs_tplg_path *template; >> + struct avs_dev *owner; >> + /* device path management */ >> + struct list_head node; >> +}; > >> +struct avs_path_binding { >> + struct avs_path_module *source; >> + u8 source_pin; >> + struct avs_path_module *sink; >> + u8 sink_pin; >> + >> + struct avs_tplg_binding *template; > > I must admit not clearly seeing how the definitions of > 'avs_path_binding' and 'avs_tplg_binding' are related. > > > More specifically, having a template for something that directly > connects a source to a sink is far from intuitive. The IDs found within the topology file do not reflect the IDs that are going to be assigned dynamically during runtime streaming. We do not specify e.g.: pipeline id=8 is made of copier id=3 and copier id=7. Firmware has limited number of modules that can be instantiated per type so static assignment is a big no no. >> + struct avs_path_pipeline *owner; >> + /* pipeline bindings management */ >> + struct list_head node; >> +}; >> + >> +#endif
>>> +struct avs_path { >>> + u32 dma_id; >> >> that is very surprising... >> >> This would seem to limit the concept of an avs path to a single host DMA >> channel, which somewhat contradicts that there can be multiple pipelines >> in the same path, or that a path can contain mixers. >> >> And even if this is a single dma, what does this represent? the >> stream_tag? the BE DMA for SSP/DMIC? >> >> Please clarify the concepts first, it's frustrating to discover this at >> patch 8. > > A single path is tied to either FE or BE. So at most to a single, > user-visible endpoint if it's FE. If there are more FEs, then we move to > NxFE <-> 1xBE scenario. You can have many pipelines forming the path - > most of the pipelines do not contain module connected to any gateway > (HDA/SSP/DMIC etc.) anyway. This should have been explained in the cover letter. Assuming that there's a single Back-End that can handle all possible routing cases is a very narrow interpretation of how DPCM is supposed to be used, and it adds quite a few opens on routing changes that can't be handled with regular triggers. What happens when not all interfaces are handled by the DSP 'gateway' is also interesting.
diff --git a/sound/soc/intel/avs/path.h b/sound/soc/intel/avs/path.h new file mode 100644 index 000000000000..ecfb687fdf36 --- /dev/null +++ b/sound/soc/intel/avs/path.h @@ -0,0 +1,60 @@ +/* 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> + */ + +#ifndef __SOUND_SOC_INTEL_AVS_PATH_H +#define __SOUND_SOC_INTEL_AVS_PATH_H + +#include <linux/list.h> +#include "avs.h" +#include "topology.h" + +struct avs_path { + u32 dma_id; + struct list_head ppl_list; + u32 state; + + struct avs_tplg_path *template; + struct avs_dev *owner; + /* device path management */ + struct list_head node; +}; + +struct avs_path_pipeline { + u8 instance_id; + struct list_head mod_list; + struct list_head binding_list; + + struct avs_tplg_pipeline *template; + struct avs_path *owner; + /* path pipelines management */ + struct list_head node; +}; + +struct avs_path_module { + u16 module_id; + u16 instance_id; + + struct avs_tplg_module *template; + struct avs_path_pipeline *owner; + /* pipeline modules management */ + struct list_head node; +}; + +struct avs_path_binding { + struct avs_path_module *source; + u8 source_pin; + struct avs_path_module *sink; + u8 sink_pin; + + struct avs_tplg_binding *template; + struct avs_path_pipeline *owner; + /* pipeline bindings management */ + struct list_head node; +}; + +#endif