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

audio: dai-zephyr: update channel verification logic #8760

Conversation

dnikodem
Copy link
Contributor

This commit updates the channel verification logic in dai-zephyr.c. The previous condition checked for an exact match between hw_params.channels and params->channels.

The new condition allows for hw_params.channels to be greater than or equal to params->channels, providing more flexibility in channel configuration.

@dnikodem dnikodem force-pushed the compare_hw_params_and_ipc_params_channel_value branch from 716cce2 to 76aa374 Compare January 19, 2024 08:37
This commit updates the channel verification logic in dai-zephyr.c.
The previous condition checked for an exact match between
hw_params.channels and params->channels.

The new condition allows for hw_params.channels to be greater than
or equal to params->channels, providing more flexibility in channel
configuration.

Signed-off-by: Damian Nikodem <[email protected]>
@dnikodem dnikodem force-pushed the compare_hw_params_and_ipc_params_channel_value branch from 76aa374 to cd00456 Compare January 19, 2024 08:49
@dnikodem dnikodem marked this pull request as ready for review January 19, 2024 12:00
@dbaluta
Copy link
Collaborator

dbaluta commented Jan 22, 2024

There are some build failures. Can you please check?

https://github.com/thesofproject/sof/actions/runs/7581488658/job/20649189700?pr=8760

@dnikodem
Copy link
Contributor Author

dnikodem commented Jan 22, 2024

There are some build failures. Can you please check?

https://github.com/thesofproject/sof/actions/runs/7581488658/job/20649189700?pr=8760

I've noticed that the Zephyr / build-linux for (zmain, lnl/tgl/mtl) is failing on various PRs as well – the reason is unclear.

@kv2019i
Copy link
Collaborator

kv2019i commented Jan 22, 2024

@dbaluta @dnikodem The mainlin Zephyr fails are known and related to this interface change (zephyrproject-rtos#32 , #7593 and zephyrproject-rtos/zephyr#64755 ).

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.

Some concern how this impacts cases where the DAI really only supports a specific fixed channel count.

Could the existing mechanism to use 0 to signal that the DAI driver can support a variable property, be used here instead of this patch?

@@ -553,7 +553,7 @@ static int dai_verify_params(struct dai_data *dd, struct comp_dev *dev,
return -EINVAL;
}

if (hw_params.channels && hw_params.channels != params->channels) {
if (hw_params.channels && hw_params.channels < params->channels) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, with some DAI implementations (like Intel PCH dmic), the channel count really is fixed, so the old check is correct. But it's true if this constrained cannot be assume to hold for all DAIs, we can enforce the condition here. DAI specific checks needs to return failure later.

Copy link
Member

Choose a reason for hiding this comment

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

@dnikodem might be worth an inline comment here to explain. Also it might be worth a

switch (dai_type) {
case dynamic_dai:
   // compare using new flow
    break;
default:
  // compare using old flow for fixed dais.
   break;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Colleagues, after reading your comments, I'm not sure if my change is correct – I'm working on confirming my doubts. Thanks for the feedback

@dnikodem
Copy link
Contributor Author

After further analysis of the code and feedback from Liam and Kai, I have concluded that this condition may remain unchanged. Therefore, I am closing this pull request. Thank you.

@dnikodem dnikodem closed this Jan 26, 2024
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.

6 participants