| Message ID | 20220207132532.3782412-1-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ASoC: Intel: avs: Topology and path management | expand |
> A continuation of avs-driver initial series [1]. This chapter covers > path management and topology parsing part which was ealier path of the > main series. The two patches that represented these two subjects in the > initial series, have been split into many to allow for easier review and > discussion. > > AVS topology is split into two major parts: dictionaries - found within > ASoC topology manifest - and path templates - found within DAPM widget > private data. Dictionaries job is to reduce the total amount of memory > occupied by topology elements. Rather than having every pipeline and > module carry its own information, each refers to specific entry in > specific dictionary by provided (from topology file) indexes. In > consequence, most struct avs_tplg_xxx are made out of pointers. > > A 'path' represents a DSP side of audio stream in runtime - is a logical > container for pipelines which are themselves composed of modules - > processing units. Due to high range of possible audio format > combinations, there can be more variants of given path (and thus, its > pipelines and modules) than total number of pipelines and module > instances which firmware supports concurrently, all the instance IDs are > allocated dynamically with help of IDA interface. 'Path templates' come > from topology file and describe a pattern which is later used to > actually create runtime 'path'. This is one of the most 'dense' patchsets I've reviewed in a long time. While the code looks mostly good, I am afraid the directions and scope are rather unclear. Now that you've split the basic parts out, ironically it makes the intent of this patchset really difficult to grasp. The first order of business is really to clarify the concepts: What is a 'stream'? what is a 'path'? what is a 'path template'? What is a 'module template'? What topologies are supported? what is the link between a path and FE? how does this interoperate or duplicate DPAM? why does a path rely on a single DMA? What would happen if there is no host PCM, e.g. for a beep tone generated in firmware? How would this work for a firmware capture pipeline that only sends notification on acoustic events and no PCM data? I have reviewed this set of patches three times already and this set of concepts are still nebulous to me, please refer to my detailed comments. You really need to describe in layman's terms what the problem statement is, what your solution tries to fix, what other options you considered, what cases you didn't handle, etc. Put yourself if the shoes of someone that is not part of your team and has no prior exposure to the cAVS firmware design. I would recommend that you use the Windows documentation [1] to provide ascii-art examples with hierarchical mixing, multiple outputs and loopbacks on what a 'path' really is and how the concept of template helps. But at a more fundamental level, the main concern I have is with the BE use: this patchset assumes the BE configuration is fixed, and that's a very very strong limitation. It's clearly not right for e.g. Bluetooth offload where multiple profiles need to be supported. It's also not right when the codec or receiver can adjust to multiple hw_params, which could lead to simplifications such as removal of unnecessary SRC/downmixers, etc. We absolutely want the capability to reconfigure the BE by using constraint matching between FE hw_params requested by applications and what the hardware can support. If your solution solved that problem you would have my full support. But if we're adding a rather complicated framework on top of a known limitation, then that's really a missed opportunity to do things better collectively. [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/audio/audio-processing-object-architecture
On Fri, Feb 25, 2022 at 03:35:12PM -0600, Pierre-Louis Bossart wrote: > But at a more fundamental level, the main concern I have is with the BE > use: this patchset assumes the BE configuration is fixed, and that's a > very very strong limitation. It's clearly not right for e.g. Bluetooth > offload where multiple profiles need to be supported. It's also not > right when the codec or receiver can adjust to multiple hw_params, which > could lead to simplifications such as removal of unnecessary > SRC/downmixers, etc. > We absolutely want the capability to reconfigure the BE by using > constraint matching between FE hw_params requested by applications and > what the hardware can support. If your solution solved that problem you > would have my full support. But if we're adding a rather complicated > framework on top of a known limitation, then that's really a missed > opportunity to do things better collectively. On the one hand everything you say here is true. On the other hand I *did* suggest starting off with something with stripped down functionality and then building up from that to make things more digestable so some of this could be the result of that approach (I've not got enough recollection of previous serieses to confirm), obviously fixing the output is also quite a common thing for DSP based systems to do just in general.