Re: [RFC 09/13] ASoC: Intel: avs: Path creation and freeing

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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.