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

[Cezary Rojewski]

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