On 2022-02-25 8:09 PM, Pierre-Louis Bossart wrote:
On 2/7/22 07:25, Cezary Rojewski wrote:
Path template is a pattern like its name suggests. It is tied to DAPM
the name really doesn't suggest anything to me, and I have no idea what
a 'pattern' represents for graph management.
You really ought to uplevel and expose the concepts earlier
Again, such 'concept' already exists in kernel since skylake-driver.
Topology never matched runtime 1:1, you are going to have some kind of
template or pattern that you later convert into actual runtime stream.
Please state what would you like to see here as nether the ASoC topology
nor the "template/pattern" is new here and I'm having trouble
understanding what's need to be added.
widget which represents a FE or a BE and is used during path
'a widget which represents a FE or a BE'. I do not see a 1:1
relationship between widget and FE/BE. these are different concepts/levels.
Huh? For skylake-driver you have a widget per module which together make
FE or BE. We simplified this here as duplicating widgets for no reason
is not good.
instantiation when substream is opened for streaming. It carries a range
of available variants - actual implementation of a runtime path on
AudioDSP side. Only one variant of given path template can be
instantiated at a time and selection is based off of audio format
provided from userspace and currently selected one on the codec.
well, the last part is the fundamental problem that I am trying to explain.
The codec rate is not necessarily fixed as you seem to assume. There are
many cases where the codec rate can be changed depending on the audio
format changes happening in the DSP.
It is an invalid assumption to assume the codec rate is selected
arbitrarily. It's often the case but that's more of a DPCM limitation
than a design guiding principle we should build on.
That case is perfectly fine and is supported by the topology implemented
here. I don't understand what's the issue here. Perhaps something is not
worded correctly in the description.
+static struct avs_tplg_path_template *
+avs_tplg_path_template_create(struct snd_soc_component *comp, struct avs_tplg *owner,
+ struct snd_soc_tplg_vendor_array *tuples, u32 block_size)
+{
+ struct avs_tplg_path_template *template;
+ int ret;
+
+ template = devm_kzalloc(comp->card->dev, sizeof(*template), GFP_KERNEL);
+ if (!template)
+ return ERR_PTR(-ENOMEM);
+
+ template->owner = owner; /* Used when building sysfs hierarchy. */
sysfs is a showstopper if the intent is to let userspace modify the
hierarchy.
Even for read-only information, it's not clear to me what application
would ever read this information. debugfs seems more appropriate?
please clarify what the intended use might be.
I'll drop the 'owner' part and move it into a separate subject. The
intent is not to modify the hierarchy, it's for having something to hook
to to read info during runtime from DPS.
+ INIT_LIST_HEAD(&template->path_list);
+ INIT_LIST_HEAD(&template->node);
+
+ ret = parse_path_template(comp, tuples, block_size, template, path_tmpl_parsers,
+ ARRAY_SIZE(path_tmpl_parsers), path_parsers,
+ ARRAY_SIZE(path_parsers));
+ if (ret)
+ return ERR_PTR(ret);
+
+ return template;
+}
struct avs_tplg_path {
u32 id;
+
+ /* Path format requirements. */
+ struct avs_audio_format *fe_fmt;
+ struct avs_audio_format *be_fmt;
this doesn't seem quite right or assumes a very narrow set of DPCM uses.
First I am not sure why there is only one format possible on both FE and
BE sides. If you have multiples paths to represent all possible
combinations of FE and BE formats, then it will become really confusing
really quickly.
This representation would also not scale if there are multiple FEs are
connected to the same BE, or conversely one FE is connected to multiple
BEs. It's also quite possible that the different BE and FE have
different formats, e.g. you could mix 24 and 32 bit data.
In addition, it is a really bad assumption to think that the BE is
independent of the FE. In cases where its format can be adjusted, we
would want constraints to be identified and select the 'best' possible
BE format.
struct avs_path_path_template can have a large list of struct
avs_tplg_path defined, so it's not limited to one format. Remember that
each format combination has its implication in form of need for
different modules being engaged, changes in number of pipelines running
and so on. And there's no running away from that. So, regardless of how
you layout the struct which represents a "path" each combination will
need a separate instance of it for its representation. Otherwise we
enter the world where kernel driver has additional logic implemented so
it modifies 'path' layouts on the fly. And that's a very dangerous path,
especially when considering long term maintenance and backward
compatibility subject.
Why would it not scale if multiple FEs are connected to the same BE? FE
paths attached to single BE can have SRC/UpDownMixers modules within
them to help with conversions. Remember that you have to take
copier/mixin/mixout/fw modules limitations into account. You cannot just
do random stuff and expect firmware to cope with that.
Sure, we want to select the best possible format. That's why you don't
have to have different FE/BE format. It's a flexible approach.
+
+ struct list_head ppl_list;
+
+ struct avs_tplg_path_template *owner;
+ /* Path template path-variants management. */
+ struct list_head node;
};
struct avs_tplg_pipeline {
