Skip to content

dai: change to use new version of dai_config_get - #6939

Merged
lgirdwood merged 1 commit into
thesofproject:mainfrom
juimonen:get_config_fix
Feb 21, 2023
Merged

dai: change to use new version of dai_config_get#6939
lgirdwood merged 1 commit into
thesofproject:mainfrom
juimonen:get_config_fix

Conversation

@juimonen

@juimonenjuimonen commented Jan 11, 2023

Copy link
Copy Markdown

Start using new version of dai_config_get where config struct is given as pointer argument from outside.

Signed-off-by: Jaska Uimonen jaska.uimonen@intel.com

@juimonen

juimonen commented Jan 11, 2023

Copy link
Copy Markdown
Author

depends on zephyr side: zephyrproject-rtos/zephyr#53708

@juimonenjuimonen mentioned this pull request Jan 11, 2023
Merged
lgirdwood
lgirdwood approved these changes Jan 11, 2023
View reviewed changes

@lgirdwoodlgirdwood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@abonislawski pls review

ranj063
ranj063 reviewed Jan 11, 2023
View reviewed changes
Comment threadsrc/lib/dai.c Outdated

@ranj063ranj063Jan 11, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@juimonen is this correct? Should you continue here instead?

@juimonenjuimonenJan 12, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I mean dai_config_get should return 0 on success, otherwise negative. You kind of always get some config back, it is just testing the arguments you give are not bogus... so in that sense this can't fail, just added the check for completeness.

@ranj063ranj063Jan 12, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

even if this is not possible in practice, the right thing to do here is continue instead of return NULL right?

@juimonenjuimonenJan 12, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yes you're right.

@lgirdwood

lgirdwood commented Jan 18, 2023

Copy link
Copy Markdown
Member

@juimonen some conflicts.

@kv2019i

kv2019i commented Jan 19, 2023

Copy link
Copy Markdown
Collaborator

@juimonen Would be good to add some tag/note that this cannot be merged until the Zephyr dependency is merged (and a link to it).

@tmleman

tmleman commented Feb 1, 2023

Copy link
Copy Markdown
Contributor

Would be good to add some tag/note that this cannot be merged until the Zephyr dependency is merged

Nice way to do that is to add list with dependencies in pull request description.

I also think that good idea is to point the zephyr Pull Request in the west manifest. This way the code will compile and pass validation.

@lgirdwood

lgirdwood commented Feb 2, 2023

Copy link
Copy Markdown
Member

@juimonen any ETA for the Zephyr PR ?

@juimonen

juimonen commented Feb 20, 2023

Copy link
Copy Markdown
Author

zephyr side merged. Updated/rebased for sof main and added west update for zephyr main/head.

@juimonen

juimonen commented Feb 20, 2023

Copy link
Copy Markdown
Author

hmm, seems we are failing into a watchdog warning (treated as error), this commit is before set_config change, so can't really roll back. @softwarecki fyi.

kv2019i
kv2019i requested changes Feb 20, 2023
View reviewed changes

@kv2019ikv2019i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Need to be merged together with Zephyr baseline update.

@kv2019ikv2019i mentioned this pull request Feb 20, 2023
Closed
lyakh
lyakh reviewed Feb 20, 2023
View reviewed changes
Comment threadsrc/audio/dai-zephyr.c Outdated

@lyakhlyakhFeb 20, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is dai_config_get() now returning an error code? Could we propagate it to the caller instead of -EINVAL?

@juimonenjuimonenFeb 20, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ok updated.

@juimonen
juimonenforce-pushed the get_config_fix branch 2 times, most recently from 0873b09 to 7f59b61CompareFebruary 20, 2023 12:06
@kv2019i

kv2019i commented Feb 20, 2023

Copy link
Copy Markdown
Collaborator

So Intel System/merge/build passes (#7132 is NOT hit which is surprising), but the SOF CI fails due to the DMA API changes https://sof-ci.01.org/sofpr/PR6939/build4024/devicetest/index.html

@kv2019ikv2019i mentioned this pull request Feb 20, 2023
Closed
@kv2019ikv2019i mentioned this pull request Feb 20, 2023
Closed
Jaska Uimonen
dai: change to use new version of dai_config_get
6d75857
Start using new version of dai_config_get where config struct is given
as pointer argument.
Update west.yaml to point to correct zephyr version for this change.
Signed-off-by: Jaska Uimonen <jaska.uimonen@intel.com>
kv2019i
kv2019i approved these changes Feb 21, 2023
View reviewed changes

@kv2019ikv2019i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good to go now, let's wait until CI finishes.

@kv2019ikv2019i mentioned this pull request Feb 21, 2023
Merged
1 task
@kv2019i

kv2019i commented Feb 21, 2023

Copy link
Copy Markdown
Collaborator

One failure in CI https://sof-ci.01.org/sofpr/PR6939/build4057/devicetest/index.html , but no errors in the FW logs.

@kv2019i

kv2019i commented Feb 21, 2023

Copy link
Copy Markdown
Collaborator

SOFCI TEST

@lgirdwood
lgirdwood merged commit 5efc08d into thesofproject:mainFeb 21, 2023
@marc-hb

marc-hb commented Feb 22, 2023

Copy link
Copy Markdown
Collaborator

I also think that good idea is to point the zephyr Pull Request in the west manifest. This way the code will compile and pass validation.

Exactly that. This PR should have been combined with a manifest update. Instead it broke the daily tests ( https://github.com/thesofproject/sof/actions/runs/4228168969/jobs/7343348093) and caused very confusing PR results (e.g. https://github.com/thesofproject/sof/actions/runs/4233634492/jobs/7354897229)

@marc-hb

marc-hb commented Feb 22, 2023

Copy link
Copy Markdown
Collaborator

Daily compilation is green again https://github.com/thesofproject/sof/actions/workflows/daily-tests.yml

This was referenced Feb 22, 2023
Merged
Merged
@kv2019i

kv2019i commented Feb 22, 2023

Copy link
Copy Markdown
Collaborator

@marc-hb wrote:

Exactly that. This PR should have been combined with a manifest update. Instead it broke the daily tests (
https://github.com/thesofproject/sof/actions/runs/4228168969/jobs/7343348093) and caused very confusing PR results (e.g.
https://github.com/thesofproject/sof/actions/runs/4233634492/jobs/7354897229)

? This pr WAS combined with a manifest update, in a single git commit, just because of this.

@marc-hb

marc-hb commented Feb 22, 2023

Copy link
Copy Markdown
Collaborator

Then why did daily tests fail? (for just one day)

https://github.com/thesofproject/sof/actions/runs/4228168969/jobs/7343348093

@marc-hb

marc-hb commented Feb 22, 2023•
edited
Loading

Copy link
Copy Markdown
Collaborator

My bad, daily tests failed only when NOT using the manifest but the latest Zephyr. Sorry for the noise.

@aborisovich I cannot see the warning in the Windows build,
https://github.com/thesofproject/sof/actions/runs/4228168969/jobs/7343348530
This is suspicious because it's the same toolchain producing the same binaries, compared every time with Linux.
EDIT: because Windows builds only test the manifest. I need a vacation.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lyakhlyakhlyakh left review comments
@ranj063ranj063ranj063 approved these changes
@lgirdwoodlgirdwoodlgirdwood approved these changes
@abonislawskiabonislawskiabonislawski approved these changes
@kv2019ikv2019ikv2019i approved these changes
@iuliana-prodaniuliana-prodaniuliana-prodan approved these changes
@tmlemantmlemantmleman approved these changes
@libinyanglibinyangAwaiting requested review from libinyang
@plbossartplbossartAwaiting requested review from plbossartplbossart is a code owner
@mmaka1mmaka1Awaiting requested review from mmaka1mmaka1 is a code owner
@lbetlejlbetlejAwaiting requested review from lbetlejlbetlej is a code owner
@dbalutadbalutaAwaiting requested review from dbalutadbaluta is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@juimonen@lgirdwood@kv2019i@tmleman@marc-hb@lyakh@ranj063@abonislawski@iuliana-prodan