Tools: Topology: Add ASRC capture to cavs-nocodec.conf topologies

[andrula-song]

This patch adds ASRC capture from Dmic such as sof-tgl-nocodec.tplg. Set ASRC_CAPTURE_ENABLE as true enables test of ASRC component with IPC4.

[andrula-song]

[lgirdwood]

@singalsu @jsarha @ranj063 pls review

[plbossart]

I still have concerns about
a) why we think ASRC is a good idea for a nocodec topology where all clocks are derived from the same master clock...
b) how ASRC is actually tested and what the value of this integration is.

NAK for now.

[singalsu]

I still have concerns about a) why we think ASRC is a good idea for a nocodec topology where all clocks are derived from the same master clock... b) how ASRC is actually tested and what the value of this integration is.

NAK for now.

Could the topology for nocodec be like this:

host copier <--- ASRC <--- DAI copier DMIC

host copier ---> ASRC ---> DAI copier SSP0
host copier <------------- DAI copier SSP0

host copier -------------> DAI copier SSP2
host copier <--- ASRC <--- DAI copier SSP2

host copier ---> ASRC ---> DAI copier SSP1
host copier <--- ASRC <--- DAI copier SSP1

It would allow to capture back playback ASRC output in SSP loopback mode (LBM). And feed data for capture ASRC.

The SSP1 duplex ASRC is to test shared DAI tracking. SSP1 is not working in UPX so some other DUT needs to be used or switch SSPs to test such after the two simpler cases are tested to work.

The default SSP configuration in nocodec is clock producer while we would need follower role. But this would be a start until we figure out how to get follower mode to work and how to hook a clock producer sound card to UPX audio HAT.

I tested ASRC originally with UP2 and Digiberry S/PDIF sound card and Pierre's SSP follower setup. I used S/PDIF connection to a USB sound card. It drifted in rate slightly vs. timer scheduled SOF.

It would be a fair effort to try to achieve similar setup with UPX. Do you think we should do it or just stick in tracking a synchronous DAI and believing that it would track a clock follower DAI?

I used for tracking filtering criteria of better than -90 dB THD+N for 997 Hz sine wave. The tracking issues (jitter, delayed measured values) are the same for synchronous and asynchronous DAI if we look it as black box.

[andrula-song]

the latest topology:

[singalsu]

Looks good to me

[andrula-song]

hello @ranj063 , can you help to review this PR? Thanks!

[lgirdwood]

@andrula-song conflicts.

[lgirdwood]

@andrula-song @singalsu @plbossart my preference is to maximise code under regular test, even if we have to fake the remote time domain here in this mode.

Thinking aloud we could have a test Kconfig for ASRC that fakes the remote time domain. i.e. we can read CCOUNT (via Zephyr API) and convert some of it's LSBs to a random drift value.

[andrula-song]

@lgirdwood Done, thanks!

[kv2019i]

Few minor notes on the tplg code

[plbossart]

@andrula-song @singalsu @plbossart my preference is to maximise code under regular test, even if we have to fake the remote time domain here in this mode.

Of course we want to test as many things as possible, but we are both resource-constrained and time-constrained.

The fundamental issue is IMHO a confusion between component testing and CI (Continuous Integration). We have exactly zero products that rely on ASRC, so while it's an important capability it has by construction a lower priority. I will assert that ASRC does not belong in the nocodec topology that is the basis for our quick smoke tests for hardware (DMA, DAIs, etc). Exhibit A for this assertion is what we do for each power-on: we fall-back to the traditional passthrough pipelines and gradually add complexity.

And even if this ASRC component is added as an opt-in, it's not clear to me if/when this component would be tested, by what/whom and at which frequency.

I would like the test methodology to be addressed before we merge anything. "Merge first, think later about test" will lead to bitrot or be dependent on developer bandwidth, both of which are not desirable directions.

[singalsu]

@plbossart The name of this PR is somewhat confusing, the usual nocodec topology we use is not impacted, this adds another nocodec type topology to use alternatively. It can be seen from the CMakeLists.txt part or PR development/cavs-nocodec-asrc\;sof-tgl-nocodec-asrc. The source .conf is different and the produced topology name is different.

Edit - realized after you responded to Liam's comment. Maybe we could take some these added nocodec topologies to regular test also?

[plbossart]

@plbossart The name of this PR is somewhat confusing, the usual nocodec topology we use is not impacted, this adds another nocodec type topology to use alternatively. It can be seen from the CMakeLists.txt part or PR development/cavs-nocodec-asrc\;sof-tgl-nocodec-asrc. The source .conf is different and the produced topology name is different.

Indeed, it would be hard to be more confusing :-).
I stand by my comments on testing, there's got to be a clear plan on if/when this topology will be tested and maintained.

[singalsu]

Even if this would not be used in regular test there should be a way to store a topology as evidence that the modifications to code (this time ipc4 & module adapter client) at least with that git hash worked. It's a lot of effort to create a new topology from scratch, and these topologies for test are very helpful when making the real production topologies. All the small details in getting things to work are quickly forgotten.

[plbossart]

@singalsu I can only concur with the notion that writing a new topology is hard, but so is maintaining a topology that has a gazillion options. It took us so much time to restore basic passthrough functionality which was broken by gradual additions. @ranj063 is still working on the format stuff, and we'll have even more changes coming.

I don't really follow the point of using a hash as a proof of the last working setup. We really need these files to be maintained, and that means agreeing on what gets tested and when.

[singalsu]

It took us so much time to restore basic passthrough functionality which was broken by gradual additions.

It could be solved by splitting the pass-through topology into a separate file and build, like this PR. I've done it myself to understand what is needed for a bare bones topology and made some new topologies from the minimum. The normal cavs-nocodec.conf has grown beyond my comprehension. Changing it a bit too easily breaks something, at least when I'm trying.

[kv2019i]

@plbossart I think this matches e.g. situation with "sof-nocodec-bt-*.tplg". I'm in practise the only one who runs tests on this and tests run purely on a per-need basis.

I'd still this is more useful than not having any topology at all. E.g. this is a reference of how to use a particular component in end-to-end context and with a topology than can be run on widely available hardware. Plus git will give you some indication on who to contact if you want run in to issue.

Would it help if we mandate a dedicated owner for each such topology? We could simply use CODEOWNERS to track owner.

[kv2019i]

Thanks @andrula-song , my concerns addressed.

[lgirdwood]

@plbossart good now ?

[plbossart]

@lgirdwood I am still opposed to the directions. This adds a new tools/topology/topology2/development/cavs-nocodec-asrc.conf file, and that brings up again the question on who is going to maintain yet another file.

I started talking with @ranj063 on a different approach where the host- and dai- copiers are included and only a simpler component added. This should be a pre-canned set of classes with a "PUT YOUR CODE HERE" when adding a new component, be it ASRC, DRC, and whatever is under development.

There is no reason to e.g. create yet another PCM device or replicate a complicated topology. We are again confusing component level testing and integration tests.

[lgirdwood]

@lgirdwood I am still opposed to the directions. This adds a new tools/topology/topology2/development/cavs-nocodec-asrc.conf file, and that brings up again the question on who is going to maintain yet another file.

I started talking with @ranj063 on a different approach where the host- and dai- copiers are included and only a simpler component added. This should be a pre-canned set of classes with a "PUT YOUR CODE HERE" when adding a new component, be it ASRC, DRC, and whatever is under development.

There is no reason to e.g. create yet another PCM device or replicate a complicated topology. We are again confusing component level testing and integration tests.

Ok, I'm fine as long as we can have upstream topologies that can easily enable developers

1. Usage of any module under testbench with IPC4.
2. Usage of direct A->B IPC4 pipelines under plugin.

[singalsu]

@andrula-song There's a conflict

[andrula-song]

@andrula-song There's a conflict

thanks, but since I have added the benchmark ASRC topologies, I would like to close this one.