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

topology2: intel: hdmi-generic: Update rates and format support in pc… #9004

Merged

Conversation

ujfalusi
Copy link
Contributor

@ujfalusi ujfalusi commented Apr 4, 2024

…m.caps

With ChainDMA the HDMI PCM can support IEC958_SUBFRAME_LE and the rate is not limited to 48K only.

To be able to use bytestream passthrough (DD/DTS/etc).

channels_max 8
rate_min 32000
Copy link
Member

Choose a reason for hiding this comment

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

IIRC 64kHz is not supported by HDaudio and HDMI, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it is not, but can we specify rates or mask rate out with tplg?

Copy link
Member

@plbossart plbossart Apr 4, 2024

Choose a reason for hiding this comment

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

patch_hmdi.c uses a explicit

#define SUPPORTED_RATES \
	(SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |\
	SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_176400 |\
	 SNDRV_PCM_RATE_192000)

btw do we support 44.1kHz with the CHAIN_DMA? I would be surprised if it worked.

Copy link
Member

Choose a reason for hiding this comment

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

need to ask @ranj063 and @jsarha if we can have a list of rates in topology instead of min/max?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tested locally: 44.1, 48, 96 and 192 32bit stereo.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder what the buffer sizes are and how we avoid fractional number of samples in the 44.1kHz case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should work:

rate_min 0
rate_max 0
rates '32000,44100,48000,88200,96000,176400,192000'

Confirmed: it does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder what the buffer sizes are and how we avoid fractional number of samples in the 44.1kHz case?

I have other wip branch to sort out the buffer/period sizes.
The max is the max size allowed, if it is fraction then it is fraction, application does not need to use the whole buffer

@ujfalusi ujfalusi force-pushed the peter/pr/tplg2_hdmi_pcm_caps_01 branch from 81efb77 to 4b49fef Compare April 4, 2024 14:59
@ujfalusi
Copy link
Contributor Author

ujfalusi commented Apr 4, 2024

Changes since v1:

  • specify only the supported rates, not the range

channels_max 8
rate_min 0
rate_max 0
Copy link
Member

Choose a reason for hiding this comment

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

do we really need those set to zero? it's weird. @ranj063 any comments?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is the first and only use of a integer constraint with enumerated valid values, so there's not a lot of examples (in SOF or elsewhere in ALSA). The code to parse was added to alsa-utils by @ranj063 , so I'll let you confirm. I wonder if a comment would be nice to explain why this is needed. As this is the first example, this bit is likely to be copy'n'pasted to many places down the line....

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kv2019i what is the error without these set to 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ranj063, I have not tested w/o these set to 0. It just felt natural to 'disable' the rate_min/max when setting the rates individually, let me test it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ranj063, @plbossart, @kv2019i, the rate_min/max needs to be set to zero for the rates to be picked up. I only dropped it for HDMI1 (hw:0,3) and [rate_min/max is not set to 0]:

alsa_conformance_test -P hw:0,3 --dev_info_only                    
------DEVICE INFORMATION------
PCM handle name: hw:0,3
PCM type: HW
card: sofhdadsp [sof-hda-dsp]
device: HDMI1 (*) []
stream: PLAYBACK
available channels: 2 4 6 8
available formats: S16_LE S32_LE IEC958_SUBFRAME_LE
rate range: [48000, 48000]
available rates: 48000
period size range: [12, 4096]
buffer size range: [192, 16384]

vs the unmodified HDMI2 (hw:0,4) [rate_min/max is set to 0]:

alsa_conformance_test -P hw:0,4 --dev_info_only
------DEVICE INFORMATION------
PCM handle name: hw:0,4
PCM type: HW
card: sofhdadsp [sof-hda-dsp]
device: HDMI2 (*) []
stream: PLAYBACK
available channels: 2 4 6 8
available formats: S16_LE S32_LE IEC958_SUBFRAME_LE
rate range: [32000, 192000]
available rates: 32000 44100 48000 88200 96000 176400 192000
period size range: [8, 4096]
buffer size range: [128, 16384]
------------------------------

Copy link
Member

Choose a reason for hiding this comment

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

Model is that base object/class will set the default value which can be inherited and overridden if needed. If not set by base class/object then its always 0.

Copy link
Member

Choose a reason for hiding this comment

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

the problem is that the inheritance is irrelevant since we are moving from a max_rate/min_rate representation to a 'rates' representation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@plbossart the alternative we can use is to have only rates and drop rate_min/max. That way the default rates will be 48K and when needed can be overridden with other supported rates

Copy link
Member

Choose a reason for hiding this comment

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

yes, if the default rate_min == rate_max == 48000 then we might as well use rates = 48000 :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok here's the PR for it #9017

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.

Let's wait for @ranj063 to comment on the best method to express a integer enumeration (do we need the min/max definitons), but otherwise looks great!

channels_max 8
rate_min 0
rate_max 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is the first and only use of a integer constraint with enumerated valid values, so there's not a lot of examples (in SOF or elsewhere in ALSA). The code to parse was added to alsa-utils by @ranj063 , so I'll let you confirm. I wonder if a comment would be nice to explain why this is needed. As this is the first example, this bit is likely to be copy'n'pasted to many places down the line....

@ranj063
Copy link
Collaborator

ranj063 commented Apr 9, 2024

Let's wait for @ranj063 to comment on the best method to express a integer enumeration (do we need the min/max definitons), but otherwise looks great!

@kv2019i I think the simplest solution would be what @ujfalusi has in this PR. The right way to do it would be remove th edefaults for rate_min/max from the class definition though but that would mean we'd need to fix the pcm_caps objects in every other topology that has them

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.

If we need to set 0 today (to WA a CC issue) then lets at least comment it in the topology as rate min/max being 0 looks like all rates will fail (unless we expect other objects to override).

channels_max 8
rate_min 0
rate_max 0
Copy link
Member

Choose a reason for hiding this comment

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

Model is that base object/class will set the default value which can be inherited and overridden if needed. If not set by base class/object then its always 0.

@ujfalusi ujfalusi changed the title topology2: intel: hdmi-generic: Update rates and format support in pc… [DNM] topology2: intel: hdmi-generic: Update rates and format support in pc… Apr 10, 2024
@ujfalusi ujfalusi marked this pull request as draft April 10, 2024 07:33
…m.caps

With ChainDMA the HDMI PCM can support IEC958_SUBFRAME_LE and the rate
is not limited to 48K only.

To be able to use bytestream passthrough (DD/DTS/etc).

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
@ujfalusi ujfalusi force-pushed the peter/pr/tplg2_hdmi_pcm_caps_01 branch from 4b49fef to badf31c Compare April 17, 2024 08:44
@ujfalusi ujfalusi changed the title [DNM] topology2: intel: hdmi-generic: Update rates and format support in pc… topology2: intel: hdmi-generic: Update rates and format support in pc… Apr 17, 2024
@ujfalusi ujfalusi marked this pull request as ready for review April 17, 2024 08:45
@ujfalusi
Copy link
Contributor Author

Changes since v2:

  • drop setting rate_min/max
  • out from draft

@lgirdwood lgirdwood merged commit 3f7a589 into thesofproject:main Apr 17, 2024
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.

5 participants