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

audio: copier: use v_index for DAI index in I2S link creation #8980

Merged
merged 1 commit into from
Apr 25, 2024
Merged

audio: copier: use v_index for DAI index in I2S link creation #8980

merged 1 commit into from
Apr 25, 2024

Conversation

dnikodem
Copy link
Contributor

@dnikodem dnikodem commented Mar 25, 2024 •
edited
Loading

This commit updates the copier DAI creation logic for I2S links to use the v_index field from the node_id structure as the DAI index. This change ensures that the correct DAI index is used when creating I2S links, aligning with the new SSP link management mechanism.

The following PR must be merged into the Zephyr repository before we can merge this PR:

@dnikodem dnikodem force-pushed the update_ssp_copier_init_for_intel_platform branch 2 times, most recently from 63e6366 to ab09ce4 Compare March 25, 2024 11:54
@dnikodem
Copy link
Contributor Author

SOFCI TEST

@dnikodem dnikodem force-pushed the update_ssp_copier_init_for_intel_platform branch from ab09ce4 to ebd532e Compare March 25, 2024 14:09
plbossart
plbossart reviewed Mar 25, 2024
View reviewed changes
src/audio/copier/copier_dai.c Outdated
@@ -259,7 +259,7 @@ int copier_dai_create(struct comp_dev *dev, struct copier_data *cd,
break;
case ipc4_i2s_link_output_class:
case ipc4_i2s_link_input_class:
dai_index[dai_count - 1] = (dai_index[dai_count - 1] >> 4) & 0xF;
dai_index[dai_count - 1] = node_id.f.v_index;
Copy link
Member

Choose a reason for hiding this comment

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

line 247 I can see this:

dai_index[dai_count - 1] = node_id.f.v_index;

So this new assignment is redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, this is redundant. I will remove it in the next update (This PR was created mainly to check the CI results - I missed this). Thanks

@dnikodem
Copy link
Contributor Author

This PR is related to zephyrproject-rtos/zephyr#70660 - Still in development

@dnikodem dnikodem force-pushed the update_ssp_copier_init_for_intel_platform branch 2 times, most recently from 1183253 to 562e0d7 Compare March 27, 2024 11:04
@dnikodem
Copy link
Contributor Author

SOFCI TEST

@plbossart
Copy link
Member

before merging we need to make sure that all 'nocodec' kernel tests pass, that's where we test the SSP mostly along with the topology definitions/blobs..

@dnikodem
Copy link
Contributor Author

before merging we need to make sure that all 'nocodec' kernel tests pass, that's where we test the SSP mostly along with the topology definitions/blobs..

Of course, I agree with that. I am currently trying to understand why some of the 'nocodec' type tests are not starting.

@dnikodem dnikodem force-pushed the update_ssp_copier_init_for_intel_platform branch from 562e0d7 to 1654f97 Compare March 28, 2024 15:16
@dnikodem
Copy link
Contributor Author

SOFCI TEST

@dnikodem dnikodem force-pushed the update_ssp_copier_init_for_intel_platform branch 4 times, most recently from 75b0b68 to 279d17b Compare April 15, 2024 16:43
@lgirdwood
Copy link
Member

@dnikodem @plbossart looks like all nocodec tests are working - does this mean we are ready now for non draft ?

@dnikodem
Copy link
Contributor Author

@dnikodem @plbossart looks like all nocodec tests are working - does this mean we are ready now for non draft ?

For my part - I am waiting for the "final" confirmation from the validation team (this is related to the recent changes in the zephyr part). The results should be available tomorrow.

@marc-hb marc-hb mentioned this pull request Apr 17, 2024
Merged
1 task
@dnikodem
Copy link
Contributor Author

Referring to Wojciech's comment from the Zephyr part:

wszypelt commented 2 days ago •
@dnikodem @kv2019i Based on the SOF PR 8980, I checked the changes for MTL and LNL, in MTL everything works properly, in LNL I have to wait a while until all the tests are done, however, I can tell you that so far everything looks good. I will write again as soon as all tests are completed

wszypelt commented yesterday
@dnikodem @kv2019i all LNL tests look good :)

@dnikodem dnikodem force-pushed the update_ssp_copier_init_for_intel_platform branch from 279d17b to 7ee1cda Compare April 22, 2024 08:06
@dnikodem
Copy link
Contributor Author

dnikodem commented Apr 22, 2024 •
edited
Loading

Rebased because the number of SSP devices was reduced with other PR: #9053

lgirdwood
lgirdwood reviewed Apr 22, 2024
View reviewed changes
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, @plbossart @ujfalusi pls confirm no driver impact. Thanks !

@plbossart
Copy link
Member

Can I ask where this 'v_index' is set?

topology?
NHLT blob inserted in topology?

in the latter case, @singalsu @jsarha can you confirm alsa-utils does set this field in the blob?

@lgirdwood
Copy link
Member

Can I ask where this 'v_index' is set?

topology? NHLT blob inserted in topology?

in the latter case, @singalsu @jsarha can you confirm alsa-utils does set this field in the blob?

@dnikodem can you confirm where v_index comes from, e.g. IPC, NHLT, topology ? Thanks !

@ujfalusi
Copy link
Contributor

Can I ask where this 'v_index' is set?
topology? NHLT blob inserted in topology?
in the latter case, @singalsu @jsarha can you confirm alsa-utils does set this field in the blob?

@dnikodem can you confirm where v_index comes from, e.g. IPC, NHLT, topology ? Thanks !

The v_index is coming from topology (it is essentially the SOF_TKN_DAI_INDEX token of a DAI copier.
For SSP we set it into the node_id |= dai_index << 4; and bit0-3 is unused for SSP.

In future platforms we might pass additional information for SSP selection via bit0-3 and this is what this patch is preparing for.
Note: Linux will always pass 0 as bit0-3 for now, which might not need to be changed at all.

@plbossart
Copy link
Member

@dnikodem rebase needed and draft status to be changed.

@dnikodem
Copy link
Contributor Author

@dnikodem rebase needed and draft status to be changed.

Sure, I'll do it, but before we merge it, we need to merge the zephyr part: zephyrproject-rtos/zephyr#70660

@dnikodem dnikodem force-pushed the update_ssp_copier_init_for_intel_platform branch from 7ee1cda to 550c778 Compare April 25, 2024 10:10
@dnikodem dnikodem marked this pull request as ready for review April 25, 2024 10:10
@dnikodem
audio: copier: use v_index for DAI index in I2S link creation
95c4e8c 
This commit updates the copier DAI creation logic for I2S links to use
the `v_index` field from the `node_id` structure as the DAI index. This
change ensures that the correct DAI index is used when creating I2S links,
aligning with the new SSP link management mechanism.

West update to necessary SSP changes:
driver: ssp: update Intel SSP DAI driver to support dynamic SSP management

Signed-off-by: Damian Nikodem <damian.nikodem@intel.com>
@dnikodem dnikodem force-pushed the update_ssp_copier_init_for_intel_platform branch from 550c778 to 95c4e8c Compare April 25, 2024 12:34
@dnikodem dnikodem changed the title [DNM] audio: copier: use v_index for DAI index in I2S link creation audio: copier: use v_index for DAI index in I2S link creation Apr 25, 2024
@dnikodem
Copy link
Contributor Author

It seems that CI failures are not related to these PR changes.

@lgirdwood
Copy link
Member

lgirdwood commented Apr 25, 2024 •
edited by dnikodem
Loading

It seems that CI failures are not related to these PR changes.

@wszypelt @lrudyX safe to merge ?

@kv2019i kv2019i added the Zephyr Issues only observed with Zephyr integrated label Apr 25, 2024
@lrudyX
Copy link

lrudyX commented Apr 25, 2024

It seems that CI failures are not related to these PR changes.

@wszypelt @lrudyX safe to merge ?

This PR was actually validated as green, CI failed after testing.

@lgirdwood
Copy link
Member

It seems that CI failures are not related to these PR changes.

@wszypelt @lrudyX safe to merge ?

This PR was actually validated as green, CI failed after testing.

Thanks @lrudyX, I will merge.

@lgirdwood lgirdwood merged commit e603d8f into thesofproject:main Apr 25, 2024
43 of 46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plbossart plbossart plbossart left review comments

@lgirdwood lgirdwood lgirdwood approved these changes

@kv2019i kv2019i kv2019i approved these changes

@iganakov iganakov iganakov approved these changes

@RanderWang RanderWang Awaiting requested review from RanderWang RanderWang is a code owner

@abonislawski abonislawski Awaiting requested review from abonislawski abonislawski is a code owner

@pblaszko pblaszko Awaiting requested review from pblaszko pblaszko is a code owner

@mmaka1 mmaka1 Awaiting requested review from mmaka1 mmaka1 is a code owner

@lbetlej lbetlej Awaiting requested review from lbetlej lbetlej is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

Assignees
No one assigned
Labels
Zephyr Issues only observed with Zephyr integrated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dnikodem @plbossart @lgirdwood @ujfalusi @lrudyX @kv2019i @iganakov