Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tools: Topology2: Add other rates support for DMIC and HDA #8728

Merged
merged 7 commits into from
Apr 16, 2024

Conversation

singalsu
Copy link
Collaborator

No description provided.

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, @RanderWang @bardliao @ujfalusi fyi - starting point for 96kHz topologies.

@singalsu singalsu force-pushed the hda_setup_to_96k branch 2 times, most recently from 849a1d6 to ae6cbae Compare January 16, 2024 11:08
@singalsu singalsu changed the title Tools: Topology2: Add other rates support for HDA generic Tools: Topology2: Add other rates support for DMIC and HDA Jan 16, 2024
@singalsu
Copy link
Collaborator Author

There was a mistake in previous PR version with sof-mtl-max98360a-rt5682 PCM IDs. DMIC0 should be 99 (it was 100) and I decided to set DMIC1 PCM ID to 100. The PCM name in all cases is "DMIC Raw2nd".

With sof-mtl-sdw-cs42l42-l0-max98363-l2 and HDA generic 2ch or 4ch the added DMIC1 capture PCM ID is default 11. It's the same as with wake-on-voice, but ultrasound and WoV should be mutually exclusive for DMIC1.

@jsarha @plbossart @RanderWang Do these names and IDs loook OK?

@singalsu
Copy link
Collaborator Author

Note: This PR needs alsa-project/alsa-utils#250

@RanderWang
Copy link
Collaborator

There was a mistake in previous PR version with sof-mtl-max98360a-rt5682 PCM IDs. DMIC0 should be 99 (it was 100) and I decided to set DMIC1 PCM ID to 100. The PCM name in all cases is "DMIC Raw2nd".

With sof-mtl-sdw-cs42l42-l0-max98363-l2 and HDA generic 2ch or 4ch the added DMIC1 capture PCM ID is default 11. It's the same as with wake-on-voice, but ultrasound and WoV should be mutually exclusive for DMIC1.

@jsarha @plbossart @RanderWang Do these names and IDs loook OK?

Name is good for me. It is for pass-through pipeline, we can rename it for real case.

@plbossart
Copy link
Member

for SoundWire, best to follow the list I came up with a couple of years ago...

PCMDeviceList.txt

Device 11 is "just" for 16khz, for KPB/WOV it's a different device really...

@singalsu
Copy link
Collaborator Author

for SoundWire, best to follow the list I came up with a couple of years ago...

PCMDeviceList.txt

Device 11 is "just" for 16khz, for KPB/WOV it's a different device really...

Thanks, I'll change it to PCM22 for 96 kHz capture. That's very useful plan for other applications too.

@singalsu singalsu marked this pull request as ready for review February 23, 2024 09:43
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ujfalusi @singalsu is the ace/cavs NHLT blob naming now resolved ?

@fredoh9
Copy link
Contributor

fredoh9 commented Feb 24, 2024

SOFCI TEST

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NUM_DMICS=4,PDM1_MIC_A_ENABLE=1,PDM1_MIC_B_ENABLE=1,DMIC1_ENABLE=passthrough,DMIC1_RATE=16000"

# SDW + DMIC + HDMI, with 16 kHz DMIC1
"cavs-sdw\;sof-mtl-sdw-cs42l42-l0-max98363-l2-48k-16k\;PLATFORM=mtl,\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the naming isn't great, there's nothing that tells us that the 48k-16k is related to dmics. same comment for all SoundWire platforms.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could add "dmic" but on the other hand we have already 2ch/4ch without indication that it's for microphones count. I'm not sure what to do with this issue and next.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could use e.g. 4ch-48k-16k, i.e. channel, DMIC0 rate, DMIC1 rate in that order. That's different enough from the convention used for SoundWire links.

NUM_DMICS=4,PDM1_MIC_A_ENABLE=1,PDM1_MIC_B_ENABLE=1,DMIC1_ENABLE=passthrough,DMIC1_RATE=16000"

# SDW + DMIC + HDMI, with 16 kHz DMIC1
"cavs-sdw\;sof-mtl-sdw-cs42l42-l0-max98363-l2-48k-16k\;PLATFORM=mtl,\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could use e.g. 4ch-48k-16k, i.e. channel, DMIC0 rate, DMIC1 rate in that order. That's different enough from the convention used for SoundWire links.

tools/topology/topology2/development/tplg-targets.cmake Outdated Show resolved Hide resolved
@singalsu singalsu requested a review from plbossart March 14, 2024 11:37
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concerns addressed. CI tests still need to pass, but e.g. quickbuild fails should have nothing to do with tplg changes, so that is likely a false fail.

@plbossart
Copy link
Member

@singalsu sorry if I am perceived as the painful reviewer, but I have to ask a background technical question:

In a recent thread (#8834), @perexg mentioned that "The limitation (on some hardware) is that the speaker is usually connected to one DAC with a fixed rate (48000Hz) and the headphones to other (more rates up to 192kHz are available). "

So my question is "do we know for sure if the 96kHz rate is supported on the 'analog' endpoint on all HDAudio hardware platforms"? If not, then we would need some sort of hardware discovery at the kernel level to filter/supersede topology definitions.

I don't mind if we add 96kHz for 'development' topologies, I just wonder if this will remain an experimental capability for the foreseeable future or if we can also add it to mainstream HDaudio topologies at some point.

@lgirdwood
Copy link
Member

@wszypelt @lrudyX good to merge ? I dont think these files are tested by internal CI today ?

@singalsu
Copy link
Collaborator Author

singalsu commented Mar 20, 2024

@singalsu sorry if I am perceived as the painful reviewer, but I have to ask a background technical question:

In a recent thread (#8834), @perexg mentioned that "The limitation (on some hardware) is that the speaker is usually connected to one DAC with a fixed rate (48000Hz) and the headphones to other (more rates up to 192kHz are available). "

So my question is "do we know for sure if the 96kHz rate is supported on the 'analog' endpoint on all HDAudio hardware platforms"? If not, then we would need some sort of hardware discovery at the kernel level to filter/supersede topology definitions.

I don't mind if we add 96kHz for 'development' topologies, I just wonder if this will remain an experimental capability for the foreseeable future or if we can also add it to mainstream HDaudio topologies at some point.

I don't know. I can't find from this specification that 96 kHz would be a mandatory feature of all HD audio codecs. It only describes how 96 kHz data is transmitted. Does someome know? https://www.intel.com/content/dam/www/public/us/en/documents/product-specifications/high-definition-audio-specification.pdf

But I'd assume that the the driver would error if 96 kHz would be attempted on a non-96 kHz capable system. Also this patch enables non-48 kHz only for test topologies in development folder. They are not part of FW release.

@wszypelt
Copy link

@lgirdwood @singalsu Unfortunately, Intel Internal CI shows an error in the tools step: make tools cmake

@plbossart
Copy link
Member

I don't know. I can't find from this specification that 96 kHz would be a mandatory feature of all HD audio codecs. It only describes how 96 kHz data is transmitted. Does someome know? https://www.intel.com/content/dam/www/public/us/en/documents/product-specifications/high-definition-audio-specification.pdf

48kHz seems to be a mandatory feature, since it's the baseline for the transmission based on 20.83us.

Other frequencies are derived with MULT/DIV bitfields in SDnDMT registers

There's wording in section 5.4.1 "Table 56 itemizes all the samples rates defined in (but not necessarily required by) in the HDaudio specification", so clearly not all devices will support all MULT/DIV values.

Section 7.3.4.7 tells us the 'Supported Rates' parameters returns a bitfield with the information.
R7 (48kHz) must be supported by all codecs. All other bits can be left unset.

So the answer is: this is platform/codec dependent.

Thinking a bit more on this, maybe it's ok to have a topology which allows for additional rates beyond 48kHz. We could filter this at the kernel level by leaving the topology with all desired rates, and then at the codec driver level prevent non-supported streams from being configured at the hw_params stage.

The point is that we MUST always support 48kHz on all PCM devices.

@kv2019i @ujfalusi should double-check that what I am saying is correct.

But I'd assume that the the driver would error if 96 kHz would be attempted on a non-96 kHz capable system. Also this patch enables non-48 kHz only for test topologies in development folder. They are not part of FW release.

Just to clarify, is the topology going to support both 48+96kHz, or just 96kHz? The latter case is problematic, I have no real issues with the former.

@singalsu
Copy link
Collaborator Author

@lgirdwood @singalsu Unfortunately, Intel Internal CI shows an error in the tools step: make tools cmake

Is the alsa-utils up to date there? It needs to have recent changes, commit 906a56f9ff136211e53db201383a47d9b0ee445d.

@singalsu
Copy link
Collaborator Author

Just to clarify, is the topology going to support both 48+96kHz, or just 96kHz? The latter case is problematic, I have no real issues with the former.

The changes common to all (HDA) topology definitions cause no issues to build topologies for 48 kHz like we today build. The rate is just set explicitly in formats in addition to word lengths to enable changing the rate for PCM0 and in DMIC case the PCMs for it (DMIC0 and DMIC1). For simplicity there is no SRC placeholder and macros for multiple rates. So when build for 96 kHz is defined, I'm afraid this is " just 96kHz". I was asked to make this example for HDA and DMIC. I didn't want to make anything complicated, just something that allowed colleagues at Intel to further define this for customer.

Can you draw a picture of how these ultrasound development enabler topologies should look like ("both 48+96kHz").

@plbossart
Copy link
Member

plbossart commented Mar 21, 2024

Just to clarify, is the topology going to support both 48+96kHz, or just 96kHz? The latter case is problematic, I have no real issues with the former.

The changes common to all (HDA) topology definitions cause no issues to build topologies for 48 kHz like we today build. The rate is just set explicitly in formats in addition to word lengths to enable changing the rate for PCM0 and in DMIC case the PCMs for it (DMIC0 and DMIC1). For simplicity there is no SRC placeholder and macros for multiple rates. So when build for 96 kHz is defined, I'm afraid this is " just 96kHz". I was asked to make this example for HDA and DMIC. I didn't want to make anything complicated, just something that allowed colleagues at Intel to further define this for customer.

Can you draw a picture of how these ultrasound development enabler topologies should look like ("both 48+96kHz").

Now that I think of it, we can only support multiple rates if we remove the mixer. When we have a mixer in place we have to pick ONE sample rate, otherwise it's a mess if different streams with different rates get mixed.

But assuming we remove the deep buffer for playback (which no one uses), then we can make the entire pipeline support multiples of 48kHz, as done for SSP/BT. If the hardware doesn't support a rate, that would be rejected by the codec DAI so there's a clear error sent by to apps.

@singalsu
Copy link
Collaborator Author

Just to clarify, is the topology going to support both 48+96kHz, or just 96kHz? The latter case is problematic, I have no real issues with the former.

The changes common to all (HDA) topology definitions cause no issues to build topologies for 48 kHz like we today build. The rate is just set explicitly in formats in addition to word lengths to enable changing the rate for PCM0 and in DMIC case the PCMs for it (DMIC0 and DMIC1). For simplicity there is no SRC placeholder and macros for multiple rates. So when build for 96 kHz is defined, I'm afraid this is " just 96kHz". I was asked to make this example for HDA and DMIC. I didn't want to make anything complicated, just something that allowed colleagues at Intel to further define this for customer.
Can you draw a picture of how these ultrasound development enabler topologies should look like ("both 48+96kHz").

Now that I think of it, we can only support multiple rates if we remove the mixer. When we have a mixer in place we have to pick ONE sample rate, otherwise it's a mess if different streams with different rates get mixed.

But assuming we remove the deep buffer for playback (which no one uses), then we can make the entire pipeline support multiples of 48kHz, as done for SSP/BT. If the hardware doesn't support a rate, that would be rejected by the codec DAI so there's a clear error sent by to apps.

Removing the deep buffer from these 96 kHz topologies builds should be possible, I'll check it and update.

@singalsu
Copy link
Collaborator Author

Removing the deep buffer from these 96 kHz topologies builds should be possible, I'll check it and update.

Actually the deep buffer is already disabled with DEEPBUFFER_FW_DMA_MS=false. I just forgot about it, sorry.

@plbossart
Copy link
Member

DEEPBUFFER_FW_DMA_MS=false

Does this actually disable the deep buffer path or just default to the regular buffer size for the deep-buffer path?

that's pretty bad...

@singalsu
Copy link
Collaborator Author

DEEPBUFFER_FW_DMA_MS=false

Does this actually disable the deep buffer path or just default to the regular buffer size for the deep-buffer path?

that's pretty bad...

The first one, everything related to deep buffer is removed. See here:

IncludeByKey.DEEPBUFFER_FW_DMA_MS{

@plbossart
Copy link
Member

DEEPBUFFER_FW_DMA_MS=false

Does this actually disable the deep buffer path or just default to the regular buffer size for the deep-buffer path?
that's pretty bad...

The first one, everything related to deep buffer is removed. See here:

IncludeByKey.DEEPBUFFER_FW_DMA_MS{

wow. I thought IncludeByKey was using a true/false variable. I didn't know it could be used for integers. Oh well, you live and learn.

@kv2019i
Copy link
Collaborator

kv2019i commented Apr 9, 2024

Ping @singalsu -- this pending ack from @plbossart and CI check is failing.

The needed defaults for DMIC1 for PCM ID and sample rate are added
similarly as for DMIC0 for later use. DMIC1 related pipelines are
not used by default so DMIC1_ENABLE is set to false.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
The input audio format and output audio format definitions are
missing for the copiers and IIR. The formats are added in
preparation to be able to set non-default sample rate for all
widgets.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
The added DMIC0_RATE allows to use the DAI, pipelines, and PCM with
any supported rate for DMIC in range 8 - 96 kHz. E.g. configure
DMIC0 to 96 kHz.

There is no change to existing functionality with default 48 kHz rate.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
This patch optionally includes dmic1-passthrough.conf if
DMIC1_ENABLE is set in topologies build to passthrough.

The sample rate of DMIC1 DAI can be set to 8 - 96 kHz with
macro DMIC1_RATE.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
Topologies with DMIC1 passthrough capture are added to development
directory with 16 and 96 kHz rates. Also a topology with DMIC0
changed to 96 kHz is added as example to test DMIC0 with other
than default rate.

The new topologies are:
	- sof-hda-generic-cavs25-4ch-48k-16k
	- sof-hda-generic-ace1-4ch-48k-16k
	- sof-mtl-sdw-cs42l42-l0-max98363-l2-4ch-48k-16k
	- sof-mtl-sdw-cs42l42-l0-max98363-l2-4ch-48k-96k
	- sof-mtl-sdw-cs42l42-l0-max98363-l2-4ch-96k
	- sof-mtl-max98360a-rt5682-4ch-48k-16k
	- sof-mtl-max98360a-rt5682-4ch-48k-96k

The DMIC1 PCM ID with 96 kHz is set to 22 (capture ultrasonic).
PCM IDs definition is in:
https://github.com/thesofproject/sof/files/14021187/PCMDeviceList.txt

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
This patch modifies cavs-mixin-mixout-hda.conf with added input
and output format attributes for in_rate and out_rate. The bit_depth
attributes need to be added to as well to avoid corrupt audio.

The HDA_ANALOG_CAPTURE_RATE and HDA_ANALOG_PLAYBACK_RATE are by default
set to 48000. The topologies' characteristics are not changed with
this patch.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
The development topologies are with -/2ch/4ch DMIC for cavs25
and ace1 platforms for testing HDA with 96 kHz rate.

The build topologies names are:
	- sof-hda-generic-cavs25-4ch-48k-96k
	- sof-hda-generic-cavs25-2ch-48k-96k
	- sof-hda-generic-cavs25-4ch-96k
	- sof-hda-generic-cavs25-2ch-96k
	- sof-hda-generic-ace1-4ch-48k-96k
	- sof-hda-generic-ace1-2ch-48k-96k
	- sof-hda-generic-ace1-4ch-96k
	- sof-hda-generic-ace1-2ch-96k
	- sof-hda-generic-96k

The DMIC1 PCM ID with 96 kHz is set to 22 (capture ultrasonic).
PCM IDs definition is in:
https://github.com/thesofproject/sof/files/14021187/PCMDeviceList.txt

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@singalsu
Copy link
Collaborator Author

Ping @singalsu -- this pending ack from @plbossart and CI check is failing.

@kv2019i Thanks for following this. The fail is because is CI at IPG has not updated their alsa-utils. I've reported it and they are working on it ( @wszypelt ).

I rebased and pushed this again to see if it works now. There's no changes to "code".

@lgirdwood
Copy link
Member

@wszypelt I assume this is good for internal CI as AFAIK none of the changes here are tested by Internal CI ?

@wszypelt
Copy link

@lgirdwood @singalsu we managed to fix alsa-utils in CI, everything works :)

@lgirdwood lgirdwood merged commit a485f27 into thesofproject:main Apr 16, 2024
42 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants