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

west.yml: upgrade Zephyr revision - update usage of IPC4 ipc_platform_send_msg function signature #7857

Conversation

aborisovich
Copy link
Contributor

@aborisovich aborisovich commented Jun 23, 2023

During last changes in Zephyr that implemented power transition for the IPC Device, signature of the intel_adsp_ipc_send_message had changed and now returns negative int error codes (previously bool on success). Updated single function reference in SOF IPC4 ipc_platform_send_msg().
Implementation had not changed as the function may return only -EBUSY error code until the Zephyr Device Power Management option is disabled.

Signed-off-by: Andrey Borisovich andrey.borisovich@intel.com

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 23, 2023

Thanks, for submitting this separately from #7325, this will provide finer-grained daily test results. Continuous Integration and git bisect for the win!

During last changes in Zephyr that implemented power transition for the
IPC Device, signature of the intel_adsp_ipc_send_message had changed
and now returns negative int error codes (previously bool on success).
Updated single function reference in SOF ipc_platform_send_msg().
Implementation had not changed as the function may return only
-EBUSY error code until the Zephyr Device Power Management option
is disabled.

Signed-off-by: Andrey Borisovich <andrey.borisovich@intel.com>
@aborisovich aborisovich force-pushed the upgrade-zephyr-rev-update-ipc-send-func-signature branch from a89febe to c326b78 Compare June 25, 2023 20:58
@aborisovich
Copy link
Contributor Author

aborisovich commented Jun 25, 2023

Corrected commit SHA (zephyrproject-rtos/zephyr@23b3cae)
Previous was incorrect (for some reason SHA on PR is different than SHA after rebase merge).

@tmleman
Copy link
Contributor

tmleman commented Jun 26, 2023

We should fix this before merging:

In file included from <command-line>:
/zep_workspace/zephyr/boards/xtensa/nxp_adsp_imx8m/nxp_adsp_imx8m.dts:10:10: fatal error: nxp/nxp_imx/mimx8ml8dvnlz-pinctrl.dtsi: No such file or directory
   10 | #include <nxp/nxp_imx/mimx8ml8dvnlz-pinctrl.dtsi>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

@kv2019i
Copy link
Collaborator

kv2019i commented Jun 26, 2023

@tmleman wrote:

We should fix this before merging:

Although isn't this error in the mainline build? In the build with Zephyr from this PR (commit 23b3cae1b ), the error is not seen. Or am I missing something?

@aborisovich
Copy link
Contributor Author

aborisovich commented Jun 26, 2023

Although isn't this error in the mainline build? In the build with Zephyr from this PR (commit 23b3cae1b ), the error is not seen. Or am I missing something?

Yeah, it's actually zmain revision failing not the manifest. I think we can merge, nxp will resolve their refactors in other revision upgrade.

@tmleman
Copy link
Contributor

tmleman commented Jun 26, 2023

Although isn't this error in the mainline build? In the build with Zephyr from this PR (commit 23b3cae1b ), the error is not seen. Or am I missing something?

@aborisovich @kv2019i I haven't seen this error in any other PR so my assumption was it was caused by the zephyr update. If that's not the case we can proceed with merge.

@tmleman
Copy link
Contributor

tmleman commented Jun 26, 2023

Off-topic: I was convinced that those are build created using Zephyr SDK (with revisions from manifest). Maybe they should be labeled better?

@kv2019i
Copy link
Collaborator

kv2019i commented Jun 26, 2023

One unrelated PM fail in https://sof-ci.01.org/sofpr/PR7857/build10051/devicetest/index.html . One known pause-resume fail in https://sof-ci.01.org/sofpr/PR7857/build10052/devicetest/index.html . Proceeding with merge.

@kv2019i kv2019i merged commit 73cd960 into thesofproject:main Jun 26, 2023
@marc-hb
Copy link
Collaborator

marc-hb commented Jun 26, 2023

@tmleman naming is hard :-) https://www.martinfowler.com/bliki/TwoHardThings.html

It's especially hard here because of the very limited space on the fixed-size, left column.

Unlike many other CIs, there is absolutely nothing hidden/secret in Github Actions, it's ALL in the repo itself. So by all means please feel free to propose and even experiment with alternatives, see commit 4311da1 for a starting point.

If you do experiment, for pure .github/ changes please add a [SKIP SOF-TEST] prefix in the PR title to avoid wasting hardware cycles. Commit titles don't matter for this, only the PR title does.

There is no "self-hosted" runner used in Github Actions right now, it's all running on the default Ubuntu images provided by Github. There are 1 or 2 docker images used on top but they're all open-source too, no proprietary toolchain has ever been used in Github Actions.

Cadence toolchains are used outside Github Actions, see #7841 for some recent discussion on that topic.

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 28, 2023

Thanks, for submitting this separately from #7325, this will provide finer-grained daily test results. Continuous Integration and git bisect for the win!

In the "I hate to be right" category: this Zephyr PR is a prime suspect for a regression, see discussion in #7869 and others.

Except #7325 has already been merged now :-(

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 29, 2023

New bug filed by @fredoh9 (thx!) for this regression:

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.

4 participants