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

k_usleep() and k_msleep() wrappers for xtos #5770

Merged
merged 2 commits into from
May 9, 2022
Merged

k_usleep() and k_msleep() wrappers for xtos #5770

merged 2 commits into from
May 9, 2022

Conversation

jsarha
Copy link
Contributor

@jsarha jsarha commented May 4, 2022

I left out the k_sleep() for cycles alone, to avoid k_timeout_t definition for XTOS. It did not look like that was needed.

@lgirdwood
Copy link
Member

@wszypelt can you check the CI, it looks like the logs are timing out on the test failure. Thanks !

@wszypelt
Copy link

wszypelt commented May 4, 2022

@lgirdwood
Sure i will check this, I think after #5593 we also need to rebase all pull requests that aren't checked yet. for proper codec building to work on them.

@lgirdwood
Copy link
Member

@jsarha can you rebase this PR on HEAD. Thanks

@jsarha
xtos-wrapper: Add Zephyr kernel.h replica with k_[mu]sleep() for XTOS
f1949fb 
The commit adds kernel.h header at the top level of xtos-wrapper
include directory. The idea is to be able to write OS agnostic code
using Zephyr kernel API calls and simply including kernel.h without
worrying if we are building for XTOS or Zephyr.

This first commit contains Zephyr API style k_msleep() and k_usleep()
implementations. The functions in fact call the old
wait_delay-functions.

More implementations and declarations following Zephyr kernel.h
content can be added as needed.

Signed-off-by: Jyri Sarha <jyri.sarha@intel.com>
@jsarha jsarha force-pushed the k_sleep_wrapper_for_xtos branch from 9a52847 to 4b25ba4 Compare May 4, 2022 16:32
lyakh
lyakh reviewed May 5, 2022
View reviewed changes
@jsarha jsarha force-pushed the k_sleep_wrapper_for_xtos branch from 4b25ba4 to b31443b Compare May 5, 2022 08:33
@lgirdwood
Copy link
Member

@wszypelt CI not showing logs for failure. Can you check ? Thanks !

@wszypelt
Copy link

wszypelt commented May 6, 2022 •
edited
Loading

@lgirdwood
Unfortunately, collecting logs from IPC4 is not working properly at the moment. From what I can see on our service.
It throws the following error:

15:31:24,346 INFO  - cavs_scripts-py\utilities\test_logger.py:709: in wrapper
15:31:24,346 INFO  -     returned = method(self, *args, **kwargs)
15:31:24,347 INFO  - cavs_scripts-py\utilities\ipc_driver.py:820: in send
15:31:24,347 INFO  -     read_response_payload=message.READ_RESPONSE_PAYLOAD)
15:31:24,347 INFO  - cavs_scripts-py\utilities\test_logger.py:709: in wrapper
15:31:24,347 INFO  -     returned = method(self, *args, **kwargs)
15:31:24,347 INFO  - cavs_scripts-py\utilities\ipc_driver.py:652: in block_on_response
15:31:24,347 INFO  -     expect_error=expect_error, expected_reply_size=expected_reply_size)
15:31:24,347 INFO  - cavs_scripts-py\utilities\ipc_driver.py:770: in _block_on_response_read_avs
15:31:24,347 INFO  -     rec_ipc = self.read(exit_right_away=True, expect_error=expect_error, expected_reply_size=expected_reply_size)
15:31:24,347 INFO  - cavs_scripts-py\utilities\ipc_driver.py:462: in read
15:31:24,347 INFO  -     self.raise_or_save(error)
15:31:24,347 INFO  - cavs_scripts-py\utilities\ipc_driver.py:632: in raise_or_save
15:31:24,347 INFO  -     raise error
15:31:24,348 INFO  - E   utilities.ipc_driver_exceptions.AvsIpcErrorResponseException: FW responded with error
15:31:24,348 INFO  - E   FW status:                    0x00000005
15:31:24,348 INFO  - E   IPC error:                             6 (ADSP_IPC_FAILURE)

@lgirdwood
Copy link
Member

@jsarha can you drop the IPC4 patch from this PR (i.e no change in IPC4 code) and we can try the CI again. If it's good we can merge the API update and come back to the IPC4 issue in a new PR.

@jsarha
pipeline: Use Zephyr API k_msleep() instead of obsolete wait_delay_ms()
a4d2d85 
Use Zephyr API k_msleep() instead of obsolete wait_delay_ms(). In XTOS
build the k_msleep() is implemented in xtos-wrapper/include/kernel.h
and it actually calls the wait_delay_ms().

Signed-off-by: Jyri Sarha <jyri.sarha@intel.com>
@jsarha jsarha force-pushed the k_sleep_wrapper_for_xtos branch from b31443b to a4d2d85 Compare May 6, 2022 11:47
@lgirdwood
Copy link
Member

Lots of sof logger already dead on CML_RVP_SDW_ZEPHYR rerun (and seen this on other PRs).

@lgirdwood
Copy link
Member

SOFCI TEST

@lgirdwood
Copy link
Member

@jsarha ok, this part looks good - the ipc4 part needs a look with @RanderWang .

@jsarha
Copy link
Contributor Author

jsarha commented May 6, 2022

Hmm, now that I look the new Zephyr build setup, the kernel.h is not anymore directly under zephyr/include, but in zephyr/include/zephyr/. I wonder if I should still move the xtos-wrapper/include/kernel.h to xtos-wrapper/include/zephyr/kernel.h, to match the new location?

@jsarha
Copy link
Contributor Author

jsarha commented May 6, 2022

However, the code still compiles fine as it is, but I wonder if there is extraneous "-I zephyr" hack somewhere... Well, this is of course easy to fix later too.

@lgirdwood
Copy link
Member

@jsarha I can merge this one now, then another PR for the header move/rename (we could have a xtos/kernel.h) and the a PR for the IPC4 fix.

lyakh
lyakh reviewed May 9, 2022
View reviewed changes
src/audio/pipeline/pipeline-stream.c
@@ -430,7 +433,7 @@ int pipeline_trigger_run(struct pipeline *p, struct comp_dev *host, int cmd)
list_init(&walk_ctx.pipelines);

if (data.delay_ms)
wait_delay_ms(data.delay_ms);
k_msleep(data.delay_ms);
Copy link
Collaborator

Choose a reason for hiding this comment

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

note, that this might incur a context switch now, IIUC. Before it was a busy loop, now under Zephyr this can actually cause a context switch. Although this path is only taken for DMA-driven pipelines and only when that delay is non-zero... And there might not be any other threads to switch to. So in practice this should be harmless and have no effect in 99.99% of the cases

Copy link
Member

Choose a reason for hiding this comment

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

yep, this is intended - and we do need to rework the DMA driver scheduling at some point.

@lgirdwood lgirdwood merged commit 301d3d8 into thesofproject:main May 9, 2022
@lgirdwood
Copy link
Member

@jsarha lets fix the IPC4 and header rename/move in other PRs

@kv2019i kv2019i mentioned this pull request May 10, 2022
Open
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lyakh lyakh lyakh left review comments

@lgirdwood lgirdwood lgirdwood approved these changes

@paulstelian97 paulstelian97 paulstelian97 approved these changes

@RanderWang RanderWang RanderWang approved these changes

@plbossart plbossart Awaiting requested review from plbossart plbossart 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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jsarha @lgirdwood @wszypelt @lyakh @paulstelian97 @RanderWang