thesofproject / sof

Sound Open Firmware
Other
548 stars 316 forks source link

[FEATURE] Dynamic pcm capabilities discovery #6032

Open cujomalainey opened 2 years ago

cujomalainey commented 2 years ago

Is your feature request related to a problem? Please describe. Currently our topology system defines the pcm capabilities for a pipeline, this causes issues when they are overreaching/incorrect. This is further compounded when a component in the pipe does not check for valid params correctly and tries to run something it shouldnt (look back to glk bugs where DAI was running 4ch into speakers). This also makes passing tests consistently across a large array of configurations much more challenging when there are adjustments needed to the PCM capabilities caused by SW modules in the pipe not related to the HW (e.g. AEC).

Describe the solution you'd like PCM capabilities should be a FW construct, not a topology construct. It is both a pain and near impossible to express capabilities at a pipeline level when they are a collection of the component capabilities.

A possible solution is to create a new callback where a pcm capabilities range is passed down the pipe (in reverse order) and components refine the range of supported rates/formats/channels/etc. Components like SRC can reset the range as the pipe is walked in reverse without lose of features.

Another possible solution, if we need to keep it in the topology is to express the capabilities in the component and parse it at topology build time for all components in the pipe. This will be a challenge for M4, but might work for topology2. Another issue for this is that it detaches the FW capabilities from the PCM capabilities. So if a module supported a new format, it would require a update to the topology as well versus the above method just requiring a change the firmware. The advantage being that this method requires no changes to the IPC/FW to achieve its goal.

Describe alternatives you've considered pcm capability ifdef macro hell

Additional context Possible good first issue depending on solution chosen

Note there are edge cases here, e.g. feedback paths for DSM that will make this a bit more complex to resolve. E.g. System walks Feedback path first, resolves FMT range is 16-32bit, but then walks speaker path and discovers only S16 is supported due to processing module, but feedback and playback must match. This will be an interesting challenge.

plbossart commented 2 years ago

@cujomalainey the topology defines how many PCMs will be present and it's a natural progression to have the PCM capabilities part of the topology as well. The firmware cannot possibly know what the graph looks like, so there's no way it could store any sort of information.

in practice PCM capabilities are often quite broad, with converters inside the graph restricting the formats as needed (e.g. some mixers might only work with S32_LE).

It seems that we have multiple issues here, but the fundamental problem is that if the topology is garbage we can't expect miracles with dynamic discovery and settings.

cujomalainey commented 2 years ago

The firmware cannot possibly know what the graph looks like, so there's no way it could store any sort of information.

@plbossart that is why I proposed solution one, which relies on dynamic discovery of capabilities after the graph has been created

Also solution 2 works as the topology still defines the capabilities, but requires the topology to be update to date on a per module basis.

plbossart commented 2 years ago

There are very very few cases where we actually need any sort of discovery/negotiation/configuration IMHO @cujomalainey

In all topologies, we have 'host' and 'dai' pipelines. The latter usually operate at a fixed rate which is pretty much aligned with hardware capabilities of external hardware components. E.g. speaker protection works at a fixed rate, with a fixed number of channels. On capture, the DMIC operates at preset frequencies and number of channels. All 'acoustic' processing is tuned once at a fixed frequency, it's not like we have a variety of settings and operating modes.

There is more flexibility on the host side of things, where 3rd party processing might be added for EQ, spatialization, what have you. This is more for the 'user experience' that 'acoustic' processing. At this level, it's true that we could have more rates, maybe samplerate conversion, etc, but the best way to define the pipeline capabilities is to make sure there are adapters to convert as needed.

cujomalainey commented 2 years ago

The problem is we are dropping in algorithms that cannot operate within the full range of the DAI/Host endpoints. Especially when there is more than one block in a pipe, having the capabilities as a overall sum in each pipe independently is prime for stale and incorrect values. I am just trying to move us to a way in which we are at least able to easily update a component with new features (e.g. NC algorithms only support S16 right now, what if one updates to S32). The first solution ties the capabilities back to the component so the topology cannot be out of sync with the FW. The second solution leaves the data in the topology, but at least consolidates it away from the pipe and back to the component so there is a single place to update should anything change, and therefore if there is a change to a component, all pipes get the benefit in a single update.

plbossart commented 2 years ago

If you chain components that rely on incompatible formats, you have a problem and need adapters. If most of your components can only support S16_LE, then you can limit the pipeline at the topology definition time.

It seems you're asking for a run-time tool to fix broken topologies. That doesn't seem wise to me. Why not fix the topologies? Arguably dealing with topologies requires skills, but magic fixes are hard to define and really hard to debug if they don't do the right thing.

cujomalainey commented 2 years ago

If most of your components can only support S16_LE, then you can limit the pipeline at the topology definition time.

Yes but further updates to the module may change that, and it requires updating across all the pipe definitions which may contain modules the updater is not familiar with. Hence it does not scale.

It seems you're asking for a run-time tool to fix broken topologies.

Hence why i recommended solution 2 which is a modification in the definition of the topologies as a possible solution

but magic fixes are hard to define and really hard to debug if they don't do the right thing.

The system can always be built with overrides (as any good design should)

cujomalainey commented 2 years ago

@ranj063 how hard would this be in topology 2?

plbossart commented 2 years ago

@cujomalainey another point I forgot to mention is that we have a more urgent need to fix components such as mixers. They really need to be specified with their own sample rate and number of channels instead of using whatever the first enabled source configures.

When we have multi-source and multi-sink components, we cannot just propagate the PCM rates all the way to the DAI, otherwise the user experience and audio quality will depend on the order in which sources are enabled. That's really not so good.

That's not science fiction btw, this is how all userspace servers behave :-)

ranj063 commented 2 years ago

@cujomalainey @plbossart with IPC4 and topology2, we also have the concept of supported audio formats per component. So even if advertised PCM capabilities are illegal, the hw_params ioctl in the driver would fail today if the component does not support it. Is that good enough?

cujomalainey commented 2 years ago

@plbossart that makes sense to me, you would want to pick the best and upmix everything else

@ranj063 it would be best if the PCM advertised the correct capabilities, alsa_conformance does not pass when you test a pcm accepted format and get an error in return