Skip to content

Power management and other comments + constify ipc_buffer_new #4441

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
Merged
lgirdwood merged 4 commits into from
Jul 7, 2021
Merged

Power management and other comments + constify ipc_buffer_new #4441

lgirdwood merged 4 commits into from
Jul 7, 2021

Conversation

@marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Jul 1, 2021

See commit messages.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jul 1, 2021

The only checkpatch warning is WARNING: Consider removing the #if 1 and its #endif

@marc-hb marc-hb marked this pull request as ready for review July 1, 2021 21:21
This was referenced Jul 1, 2021
Closed
Merged
lgirdwood
lgirdwood approved these changes Jul 2, 2021
View reviewed changes
src/include/sof/audio/buffer.h Outdated
Copy link
Member

@lgirdwood lgirdwood Jul 2, 2021

Choose a reason for hiding this comment

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

We are probably missing other const usage wrt IPC too.

Copy link
Collaborator Author

@marc-hb marc-hb Jul 2, 2021

Choose a reason for hiding this comment

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

Probably, this one just came up when looking at your valgrind fixes.

@lgirdwood
Copy link
Member

lgirdwood commented Jul 2, 2021

The only checkpatch warning is WARNING: Consider removing the #if 1 and its #endif

So CI is building and running successfully here with the new assert. I assume you have checked the binary size difference with XCC (assume GCC will be the same).

marc-hb
marc-hb commented Jul 2, 2021
View reviewed changes
Copy link
Collaborator Author

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

So CI is building and running successfully here with the new assert.

No, it's commented out.

I assume you have checked the binary size difference with XCC (assume GCC will be the same).

I checked only with gcc10 and only with the default flags, that's why I left it commented out.

src/include/sof/audio/buffer.h Outdated
Copy link
Collaborator Author

@marc-hb marc-hb Jul 2, 2021

Choose a reason for hiding this comment

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

Probably, this one just came up when looking at your valgrind fixes.

@lgirdwood
Copy link
Member

lgirdwood commented Jul 2, 2021

So CI is building and running successfully here with the new assert.

No, it's commented out.

I assume you have checked the binary size difference with XCC (assume GCC will be the same).

I checked only with gcc10 and only with the default flags, that's why I left it commented out.

ok, lets check with XCC before we merge this as we can remove the conditional or have an #if XCC instead.

@lgirdwood
Copy link
Member

lgirdwood commented Jul 2, 2021

So CI is building and running successfully here with the new assert.

No, it's commented out.

I assume you have checked the binary size difference with XCC (assume GCC will be the same).

I checked only with gcc10 and only with the default flags, that's why I left it commented out.

ok, lets check with XCC before we merge this as we can remove the conditional or have an #if XCC instead.

oh - CI probably has the info for XCC builds ?

marc-hb added 4 commits July 3, 2021 05:58
@marc-hb
Add alternative, more reliable STATIC_ASSERT() disabled by default
503740a 
Does not silently succeeds when passed variables. Not enabled by default
in the unlikely case it wastes memory with some compiler and/or compiler
flags.

Issue found while testing tracing mismatch thesofproject#4388

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
ipc_buffer_new: make buffer argument const
7a906f2 
'const' documents APIs and catches bugs.

This came while discussing thesofproject#4408

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
zephyr/wrapper.c: add "temporary fix" comment to ipc_send_queued_msg()
2412748 
As requested in previous PR thesofproject#4428

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
pm_runtime.c: explain the opposition between "enable" and "active"
c48ddf6 
Pure comments, no code change. Follow-up to commit 8633d5f ("cavs:
pm: improve PM_RUNTIME_HOST_DMA_L1 documentation")

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb marc-hb force-pushed the comments-ipc-const branch from 8ff495c to c48ddf6 Compare July 3, 2021 06:01
@marc-hb
Copy link
Collaborator Author

marc-hb commented Jul 3, 2021

I tested gcc and xcc with various -O levels. I found only one case where the array-based assert increases the .bss: gcc with -O0. All other cases keep the sizes the same. The xcc versions I looked at optimize the unused array away even at the -O0 level.

Comment updated accordingly.

This was referenced Jul 3, 2021
Merged
Merged
lyakh
lyakh reviewed Jul 5, 2021
View reviewed changes
src/include/sof/common.h
#else
#define STATIC_ASSERT(COND, MESSAGE) \
__attribute__((unused)) \
static char META_CONCAT(arr_assertion_failed_, MESSAGE)[(COND) ? 1 : -1]
Copy link
Collaborator

@lyakh lyakh Jul 5, 2021

Choose a reason for hiding this comment

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

does it have to be static? And there's an additional space there

Copy link
Collaborator Author

@marc-hb marc-hb Jul 5, 2021

Choose a reason for hiding this comment

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

does it have to be static?

Depends where you prefer to waste a few bytes per assertion at the -O0 level. I suspect the .bss is likely to have less performance impact.

And there's an additional space there

That was for aligning with the other one but not very useful, I can remove.

Copy link
Collaborator Author

@marc-hb marc-hb Jul 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Depends where you prefer to waste a few bytes per assertion at the -O0 level. I suspect the .bss is likely to have less performance impact.

Actually: I didn't check whether gcc -O0 allocates unused local variables or not.

This is the reason this is disabled by default: it's very tedious and more importantly: unreliable to test every optimization level for every compiler.

@lgirdwood
Copy link
Member

lgirdwood commented Jul 6, 2021

I tested gcc and xcc with various -O levels. I found only one case where the array-based assert increases the .bss: gcc with -O0. All other cases keep the sizes the same. The xcc versions I looked at optimize the unused array away even at the -O0 level.

Comment updated accordingly.

Do we still need the #if 1 if not we can make this the default check or would you be happy to keep this around for more validation ?

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jul 6, 2021

Do we still need the #if 1 if not we can make this the default check or would you be happy to keep this around for more validation ?

I don't like #if 1 either but this is really tricky situation.

On one hand I discovered that the typedef-based solution can silently succeed with variables, which is really bad for something called STATIC_ASSERT!

On the other hand the array based solution is 100% reliable BUT it can waste some memory with some compilers at the -O0 level. I can't test future compilers. So that's why I'm submitting it commented out: if anyone ever has some STATIC_ASSERT doubts like I had, they can quickly and locally use the array-based one and find their mistake.

@lgirdwood
Copy link
Member

lgirdwood commented Jul 7, 2021

CI showung a non booting DUT and a know alsabat issue on APL nocodec, both unrelated.

@lgirdwood lgirdwood merged commit 9d84c15 into thesofproject:main Jul 7, 2021
@marc-hb marc-hb deleted the comments-ipc-const branch September 2, 2021 23:47
@marc-hb marc-hb mentioned this pull request Sep 28, 2021
Closed
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

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

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

@libinyang libinyang Awaiting requested review from libinyang

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

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

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

@mwasko mwasko Awaiting requested review from mwasko

@keyonjie keyonjie Awaiting requested review from keyonjie

@cujomalainey cujomalainey Awaiting requested review from cujomalainey

@ranj063 ranj063 Awaiting requested review from ranj063

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@marc-hb @lgirdwood @lyakh