[FEATURE] topology2 pipeline extension support

[plbossart]

Problem statement

The pipelines used in topology2 are difficult to extend for

• Intel-contributed algorithm and component validation (IIR EQ, DRC, Beamforming, ASRC, etc)

• 3rd party algorithm integration (Waves, DTS, Realtek, etc)

The main issues we are facing are

• the prevalent approach of “Copy/paste/edit” of existing pipelines, including host and DAI copiers, which causes immediate bit-rot in extended topologies when the base topologies used by CI are updated. Examples of bit-rot are for example the input/output format handling, introduction of new tokens, missing definitions, etc.

• the nested key-based conditional inclusion of capabilities in the same topology file. ExhibitA of this out-of-control complexity is the “cavs-nocodec.conf” topology which has become unmaintainable with 51 included files.

• Very few topologies are reviewed in depth and only a handful of developers can maintain topologies.

What can we improve?

Ironically the fact that all our topologies are accessible with an open-source license makes it possible for anyone to edit all parts of the topology. However, for maintainability and to speed-up integration of external components, we need to enforce “guiding principles”, such as

a) Intel or 3rd party algorithm developers SHALL NOT modify the host- or dai copiers. These components need to be taken as a given, and be validated with passthrough connections.

b) Intel or 3rd party algorithm developers SHALL rely on pre-existing templates, and only add new components without duplication of topology definitions.

c) Intel topology maintainers SHALL NOT assume that algorithms can only be added in specific positions. 3rd party algorithm developers MAY for example need to add new pipelines to a mixer-based topology, or remove/replace an Intel-provided component

d) The key-based conditional addition of topologies in the nocodec topology is no longer permitted. Rather than having a single file top-level topology file which conditionally includes others, we need to move to multiple top-level files with clear inclusion/inheritance rules. The main point is that instead of adding e.g. ASRC support to the nocodec topology file, developers can create their own minimal nocodec-asrc topology.

Suggested solution

The topology2 syntax has a notion of class/object which already provides inheritance capabilities. The key point is that we are not currently using this inheritance to build pipelines.

mixout-dai-copier-playback.conf defines the following widgets:

Object.Widget {

    mixout."1" {}
    dai-copier."1" { … }
    pipeline."1" { … }

}

mixout-gain-efx-dai-copier-playback.conf defines a longer list of widgets:

Object.Widget {

    mixout."1" {}
    dai-copier."1" {…}
    gain."1" { …}      <<<
    eqiir."1" { … }    <<<
    eqfir."1" { … }    <<<
    drc."1" { … }      <<<
    pipeline."1" {…}

}

Only the widgets marked with "<<<" change, the rest should be 100% identical. The current approach relies on an explicit enumeration of all components, which leads to copy/paste when extensions are needed.

In addition, the inclusion of mixout and dai-copier make it difficult to insert components in the middle.

What we need is a different split and integration methodology with

1. turn-key non-modifiable copier pipelines
2. turn-key non-modifiable minimal pipelines (e.g. mixin, mixout, module copier)
3. processing pipelines relying on inheritance of templates

Example split

Mixout.conf (NOT MODIFIABLE)

Object.Widget {
    mixout."1" {}
    pipeline."1" { … }
}

Dai-copier-playback.conf (NOT MODIFIABLE)

Object.Widget {

    dai-copier."1" { … }

    pipeline."1" { … }
}

Mixout-gain.conf (PROVIDED AS EXAMPLE TEMPLATE)

Object.widget {

    <INHERIT MIXOUT>

    gain."1" { …}

}
<ROUTE gain.1 to mixout.1>

Mixout-gain-eq.conf (PROVIDED AS EXAMPLE TEMPLATE)

Object.widget {

    <INHERIT MIXOUT-GAIN>

    eqiir."1" { … }
    eqfir."1" { … }
}       

<ROUTE eqiir1. to gain.1>
<ROUTE eqfir.1 to eqiir.1>

Mixout-gain-eq-drc.conf (PROVIDED AS EXAMPLE TEMPLATE)

Object.widget {

    <INHERIT MIXOUT-GAIN-EQ>

    drc."1" { … }
}       

<ROUTE drc.1 to eqfir.1>

mixout-gain-efx-dai-copier-playback.conf (integration level done by algorithm developer)

        INSTANTIATE Mixout-gain-eq

    INSTANTIATE dai-copier-playback

    ROUTE <drc.1 to dai-copier.1>

Another algorithm provider may use

Mixout-gain-algo.conf

Object.widget {

    <INHERIT MIXOUT-GAIN>

    algo."1" { … }
}       

<ROUTE algo1. to gain.1>

mixout-gain-algo-dai-copier-playback.conf (integration level done by ALGO developer)

        INSTANTIATE Mixout-gain-algo

    INSTANTIATE dai-copier-playback

    ROUTE <algo.1 to dai-copier.1>

Additional notes

The inheritance suggested above could be replaced by an 'extend' mechanism, which is another way to implementing the same thing.

The connections between pipelines would need to be specified at a high-level, and the topology compiler should connect the last widget of the one pipeline with the first widget of the connected pipelines.

The connections need to account for widget pins, this is really required since some pins have special content such as amplifier feedback.

[lgirdwood]

What we need is a different split and integration methodology with

1. turn-key non-modifiable copier pipelines
2. turn-key non-modifiable minimal pipelines (e.g. mixin, mixout, module copier)
3. processing pipelines relying on inheritance of templates

I think 1 & 2 are probably doable in the v2.10 time frame, but we do need a simple example of 1 & 2 maybe using volume as the "processing" element so we can align on the design and then scale the effort. @plbossart @jsarha @ranj063 does this sound like a good starting point ?

[plbossart]

Yeah, we discussed that the volume needs to be handled as all other components, mainly because a) it's completely optional except for DMICs b) the position of the volume may change depending on other elements, e.g. we may want to have an attenuation prior to some processing and a boost after.

So indeed this is the easiest component to handle for a prototype.

[lgirdwood]

Yeah, we discussed that the volume needs to be handled as all other components, mainly because a) it's completely optional except for DMICs b) the position of the volume may change depending on other elements, e.g. we may want to have an attenuation prior to some processing and a boost after.

So indeed this is the easiest component to handle for a prototype.

@jsarha @ranj063 pls go ahead with volume as example "processing" for tasks 1 & 2 above.

[ranj063]

@plbossart @jsarha @singalsu here's my first draft of the proposed template and the compiler changes needed.

Topology changes: https://github.com/ranj063/sof/tree/pipeline-extn Compiler changes: https://github.com/ranj063/alsa-utils/tree/pipeline-extn

I took the liberty to modify the proposal we discussed in our last meeting slightly to use only one new keyword "Pipelines" which is an array of pipeline fragments consisting of a set of modules. The fragments are assumed to be defined in the order source->sink. For example, for the SSP0 playback pipeline 1 in the MTL nocodec tplg, there would be 2 fragments like below.

Pipelines [
    # Pipeline 1 is made up of 2 fragments
    # Fragment 1 in pipeline 1 contains just the host-copier
    {
        pipeline_id 1
        direction   playback
        dynamic_pipeline    1

        Object.Widget {
            host-copier.1 {
                # TODO: Add the audio format objects and make them common for the pipeline
                num_input_audio_formats 1
                num_output_audio_formats 1
                type    "aif_in"
                node_type $HDA_HOST_OUTPUT_CLASS
                stream_name 'SSP0 Playback'
                pcm_id 0
            }
        }
    }
    # Fragment 2 in pipeline 1 is an extension of fragment 1 and contains gain and mixin
    {
        pipeline_id 1
        Object.Widget {
            gain.1 {
                num_input_audio_formats 1
                num_output_audio_formats 1
                curve_type "windows_fade"
                Object.Control.mixer.1 {
                    name 'Pre Mixer $SSP0_PCM_NAME Playback Volume'
                }
            }

            mixin.1 {}
        }
    }
]

The compiler also adds a new keyword for connecting top-level pipelines "ConnectPipelines" ex mixin.1.1 -> mixout.2.1 in the SSP0 playback pipeline case to give us the resulting pipeline as in the figure below:

[plbossart]

That's an interesting start @ranj063

I am not sure however how it would scale. If we need to include all possible configurations with 3rd party modules, the Pipelines[] array is quickly going to be unmanageable/un-maintainable (if that word exists). we probably need a means to extend a pipeline defined in a separate file to pick and choose.

I also don't quite see how elements are added, in your example you added gain and mixin together, how would for example represent a extended pipeline with just mixin and another with gain and mixin? The inclusion of elements has to allow for some elements to be skipped depending on the needs.

[ranj063]

That's an interesting start @ranj063

I am not sure however how it would scale. If we need to include all possible configurations with 3rd party modules, the Pipelines[] array is quickly going to be unmanageable/un-maintainable (if that word exists). we probably need a means to extend a pipeline defined in a separate file to pick and choose.

@plbossart but only pipelines that will potentially need to be extended or trimmed will be defined using this new fragmented array, right? You could have the the pipeline defined in a separate file and extend it at the top-level as long the original definition is also defined using the new "Pipelines" keyword. If it is defined with a pipeline class as we do today, it is assumed to be a fixed pipeline.

I also don't quite see how elements are added, in your example you added gain and mixin together, how would for example represent a extended pipeline with just mixin and another with gain and mixin? The inclusion of elements has to allow for some elements to be skipped depending on the needs.

The ability to skip elements or fragments can be achieved using the IncludeByKey feature that we have today right? So if we want the gain & mixin together always, you'd define them in the same fragment, otherwise we'd split them up into seprate fragments with the baility to insert fragemts in between or alternatively skip them if not need

[plbossart]

The IncludeByKey has proven relatively difficult to handle and leads to top-level warts added to the low-level common parts. That's the opposite of what we want to achieve.

Let's take an example to anchor the discussion. I would like to see how we deal with the following examples

Host-copier host-copier-gain host-copier-mixin host-copier-gain-mixin host-copier-IP1-gain-mixin host-copier-gain-IP2-mixin host-copier-IP1-IP2-IP3-gain-mixin

[plbossart]

Also question on the "mixin.1.1 -> mixout.2.1": can we specify the pin or leave it blank if it's irrelevant? The pin does matter for the speaker protection stuff, i.e. when the contents are different on each pin.

[ranj063]

Also question on the "mixin.1.1 -> mixout.2.1": can we specify the pin or leave it blank if it's irrelevant? The pin does matter for the speaker protection stuff, i.e. when the contents are different on each pin.

as far as the routes ni topology are concerned, there's no pin info. We use special tokens for these.

[plbossart]

as far as the routes ni topology are concerned, there's no pin info. We use special tokens for these.

So how would we specify the connections if the notion of pin doesn't exist?

[ranj063]

as far as the routes ni topology are concerned, there's no pin info. We use special tokens for these.

So how would we specify the connections if the notion of pin doesn't exist?

If you look at the topology graph info, all you'd see is that the smartamp is connected to the gain module in the playback path and the SSP.IN DAI in the feedback path. Then within the smartamp module, we add the input pin binding info which tells the kernel that pin 0 is connected to the gain and pin 1 is the feedback from the SSP.IN DAI

[plbossart]

@ranj063 doesn't this say then that it'll be really tricky to have a generic pipeline extension mechanism if we have those special tokens that need to know the connections? we would have to change both the routes and the special tokens if we insert something in a pipeline, no?

[ranj063]

The IncludeByKey has proven relatively difficult to handle and leads to top-level warts added to the low-level common parts. That's the opposite of what we want to achieve.

Let's take an example to anchor the discussion. I would like to see how we deal with the following examples

Host-copier host-copier-gain host-copier-mixin host-copier-gain-mixin host-copier-IP1-gain-mixin host-copier-gain-IP2-mixin host-copier-IP1-IP2-IP3-gain-mixin

@plbossart if this is what we want to do, I have a different proposal. Today we're having to create a new pipeline class if we want to add/remove widgets from an existing pipeline class because the routes are fixed in the class definition. Instead, if we withhold the route definition until the pipeline is created, then we can extend/trim or modify the order of widgets as we please.

So, in your question, if we have a pipeline class definition like this

Class.Pipeline.host-copier-gain-mixin {
       /* define the list of widgets, audio formats etc */
       Object.Widget {
              /* attributes & audio formats should be defined here */
               host.copier.1 {}
               gain.1 {}
               mixin.q {}
       }
}

Then when instantiating the pipeline, we can specify the order in which the widgets should be connected like this:

Object.Pipeline.host-copier-gain-mixin {
        index 5
        /* Widgets will be connected like this: hostcopier.5.1 -> gain.5.1 -> mixin.5.1
        WidgetConnectionOrder [
                 host-copier.1
                 gain.1
                 mixin.1
        ]
}

And if we wanted to insert a new widget in between the gain & mixin, we can do

Object.Pipeline.host-copier-gain.mixin {
          index 5
          # add the new widget in the pipeline object
          Object.Widget.newwidget.1 {
          }
          # specify the order of widget connections: host-copier.5.1->gain.5.1->newwidget.5.1 ->mixin.5.1
          WidgetConnectionOrder [
                     host-copier.1
                     gain.1
                     newwidget.1
                     mixin.1
          ]
}

And finally, if we wanted to just add a new widget and remove gain we'd do the same thing as above but the gain.1 widget would be missing from the WidgetConnectedOrder which tells the compiler that the pipeline looks like: host-copier.5.1 -> newwidget.5.1 -> mixin.5.1

With this proposal, we can reuse our existing pipeline class definitions, add default widgetconnectionorder that can be overridden when creating the pipeline object

[plbossart]

I don't follow how you can have two versions instantiating different pipelines but both named host-copier-gain.mixin

The key point was to have templates that can be extended, not internal wizardly to change pipelines defaults.

[ranj063]

I don't follow how you can have two versions instantiating different pipelines but both named host-copier-gain.mixin

the name host-copier-gain-mixin is just a name for the template. The name for a pipeline is not used for any purpose other than creating the widgets/connections defined in the pipeline class using the same name

The key point was to have templates that can be extended, not internal wizardly to change pipelines defaults.

Im not sure what you mean by extension. inserting a new widget between host-copier->gain->mixin is not quite extending but changing the pipeline definition isnt it?

[jsarha]

I was not sure what the previous proposal would mean under the alsatplg hood, but this one I can follow:

https://github.com/thesofproject/sof/issues/9082#issuecomment-2103130884

That would just need the route objects would be created based on WidgetConnectionOrder array, wouldn't it?

But if we go back to the previous proposal, there I'd need some check list what to do when I the pre-processor hits Pipelines object.

[ranj063]

@plbossart @jsarha this is the new proposal to use the extends/overrides keyword.

Please see the following commit for examples of how to use the extends/overrides keyword. I modified the widget order in the mixout-gain-efx-mbdrc-dai-copier pipeline using the overrides to insert the mbdrc between the gain and eqiir just for an example (just to show how overrides work)

https://github.com/ranj063/sof/blob/3900d214ada12eae3b345df6eb416448f94530f3/tools/topology/topology2/include/pipelines/cavs/mixout-gain-efx-mbdrc-dai-copier-playback.conf#L24 Note that we have to redefine all the routes to make sure that the mbdrc is inserted in the right place in the pipeline. https://github.com/ranj063/sof/blob/3900d214ada12eae3b345df6eb416448f94530f3/tools/topology/topology2/include/pipelines/cavs/mixout-gain-efx-mbdrc-dai-copier-playback.conf#L53

So you'd get the following pipeline with the change above.

As for the drc topology, it remains the same as before but it uses the extends keyword to extend the base class: https://github.com/ranj063/sof/blob/3900d214ada12eae3b345df6eb416448f94530f3/tools/topology/topology2/include/pipelines/cavs/mixout-gain-efx-dai-copier-playback.conf#L23 Note that we only define the new route rfom the eqiir->drc in this class now.

The compiler changes for this are quite minimal: https://github.com/ranj063/alsa-utils/commit/e33846456d356ae6dc15cf2a075cf4996aaac0ae

[kv2019i]

Stable branched for 2.10, moving this to 2.11.