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.

Sign up for GitHub

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

Jump to bottom

topology2: Modify the HDMI number to support the Rex variant #8131

Merged
merged 1 commit into from
Sep 6, 2023
Merged

topology2: Modify the HDMI number to support the Rex variant #8131

merged 1 commit into from
Sep 6, 2023

Conversation

udaymb
Copy link
Contributor

@udaymb udaymb commented Aug 30, 2023 •
edited
Loading

Modify NUM_HDMIS to 3 to support the Rex variant. Based on this change, SPK_ID and BT_ID is modified.

@udaymb udaymb requested review from ranj063 and jsarha as code owners August 30, 2023 17:57
@udaymb udaymb mentioned this pull request Aug 30, 2023
Merged
plbossart
plbossart requested changes Aug 30, 2023
View reviewed changes
tools/topology/topology2/sof-ace-tplg/tplg-targets.cmake Outdated
HEADSET_SSP_DAI_INDEX=2,SPEAKER_SSP_DAI_INDEX=0,HEADSET_CODEC_NAME=SSP2-Codec,SPEAKER_CODEC_NAME=SSP0-Codec,\
BT_NAME=SSP1-BT,BT_INDEX=1,BT_ID=8,BT_PCM_NAME=Bluetooth,INCLUDE_ECHO_REF=true,USE_CHAIN_DMA=true,\
DEEPBUFFER_FW_DMA_MS=10,DEEPBUFFER_D0I3_COMPATIBLE=true"
NUM_HDMI=3,HEADSET_SSP_DAI_INDEX=2,SPEAKER_SSP_DAI_INDEX=0,SPK_ID=6,HEADSET_CODEC_NAME=SSP2-Codec,\
Copy link
Member

Choose a reason for hiding this comment

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

it's NUM_HDMIS=3 and this should be set for all devices.

@ranj063 @bardliao can we make this platform-dependent so that we don't have this sort of issues?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's NUM_HDMIS=3 and this should be set for all devices.

@ranj063 @bardliao can we make this platform-dependent so that we don't have this sort of issues?

Yes, we can define NUM_HDMI in platform/intel/mtl.conf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bardliao @plbossart - If this is included in the mtl.conf, other topologies would affect as below information still exists for one configuration
image

https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/intel/boards/sof_rt5682.c#L188

Should we need to update with NUM_DMICS=4 for these topologies?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@udaymb if there's 1 topology with NUM_HDMIS=4, then thats the one that needs to override it at build time. Please make 3 the default and set it to 4 where needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the commit

@udaymb udaymb changed the title topology2: Modify the HDMI number to support the eex variant topology2: Modify the HDMI number to support the Rex variant Aug 31, 2023
@udaymb udaymb force-pushed the rex-pdm1-hdmi3 branch 2 times, most recently from 826d51a to cd68253 Compare August 31, 2023 08:10
@lgirdwood
Copy link
Member

@ranj063 @jsarha any comments ?

@lgirdwood
Copy link
Member

@plbossart good for you ? and do you want a followup PR for NUM_DMICS=4 ?

@plbossart
Copy link
Member

@plbossart good for you ? and do you want a followup PR for NUM_DMICS=4 ?

yes I think we need to do the right thing and only use 3 HDMI across the board.

jsarha
jsarha approved these changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@jsarha jsarha left a comment

Choose a reason for hiding this comment

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

So due to one less HDMI PCMs the BT ans speaker IDs are shifted down? SPK_ID = 7 default coming from some file and overridden here?

I guess this is Ok. However I'd urge to put all possible explanations into the commit message. These topo2 makefile changes are hardly self explanatory.

@bardliao
Copy link
Collaborator

bardliao commented Sep 1, 2023

The code looks good. However, we need to revisit the machine driver. We have SOF_RT5682_NUM_HDMIDEV(4) for some Rex variants.
For example,
https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/intel/boards/sof_rt5682.c#L188

We need to make sure machine driver and topology are aligned.

@lgirdwood
Copy link
Member

The code looks good. However, we need to revisit the machine driver. We have SOF_RT5682_NUM_HDMIDEV(4) for some Rex variants. For example, https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/intel/boards/sof_rt5682.c#L188

We need to make sure machine driver and topology are aligned.

@bardliao is this something you do in the machine driver ?

@udaymb
Copy link
Contributor Author

udaymb commented Sep 1, 2023

So due to one less HDMI PCMs the BT ans speaker IDs are shifted down? SPK_ID = 7 default coming from some file and overridden here?

I guess this is Ok. However I'd urge to put all possible explanations into the commit message. These topo2 makefile changes are hardly self explanatory.

@jsarha - Updated the cmake file and commit message accordingly

@bardliao
Copy link
Collaborator

bardliao commented Sep 1, 2023

The code looks good. However, we need to revisit the machine driver. We have SOF_RT5682_NUM_HDMIDEV(4) for some Rex variants. For example, https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/intel/boards/sof_rt5682.c#L188
We need to make sure machine driver and topology are aligned.

@bardliao is this something you do in the machine driver ?

No, that was not me.

@udaymb
topology2: NUM_HDMIS is set to 3 for MTL devices and updated in platf…
652e1fe 
…orm file

NUM_HDMIS is set to 3 as default value for MTL platform. SPK_ID and BT_ID
are modified based on this change in the target cmake file.

Signed-off-by: Yong Zhi <[email protected]>
Signed-off-by: Uday M Bhat <[email protected]>
@udaymb
Copy link
Contributor Author

udaymb commented Sep 5, 2023

@plbossart @ranj063 @jsarha
Modified HDMI to 3 for MTL devices - updated the same in kernel thesofproject/linux#4551

Please share your comments and support to merge this PR

@bardliao bardliao merged commit e12a099 into thesofproject:main Sep 6, 2023
40 of 41 checks passed
@udaymb udaymb mentioned this pull request Sep 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kv2019i kv2019i kv2019i approved these changes

@bardliao bardliao bardliao approved these changes

@ranj063 ranj063 ranj063 approved these changes

@plbossart plbossart plbossart approved these changes

@jsarha jsarha jsarha approved these changes

@lgirdwood lgirdwood Awaiting requested review from lgirdwood

@mmaka1 mmaka1 Awaiting requested review from mmaka1

@lbetlej lbetlej Awaiting requested review from lbetlej

@dbaluta dbaluta Awaiting requested review from dbaluta

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@udaymb @lgirdwood @plbossart @bardliao @jsarha @ranj063 @kv2019i