Skip to content

Conversation

@kv2019i
Copy link
Collaborator

@kv2019i kv2019i commented Sep 11, 2025

Passing boolean parameters to key functions results in hard to read code. Add BUFFER_USAGE_SHARED and BUFFER_USAGE_NOT_SHARED definitions to make code allocating buffers easier to read and maintain.

No functional change.

Copilot AI review requested due to automatic review settings September 11, 2025 10:06
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces named constants BUFFER_USAGE_SHARED and BUFFER_USAGE_NOT_SHARED to replace boolean literals in buffer allocation functions, improving code readability and maintainability.

  • Adds buffer usage flag definitions to make code more self-documenting
  • Replaces false literals with BUFFER_USAGE_NOT_SHARED in buffer allocation calls
  • Updates multiple audio component files to use the new constants

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/include/sof/audio/buffer.h Defines BUFFER_USAGE_SHARED and BUFFER_USAGE_NOT_SHARED constants
src/audio/module_adapter/module_adapter.c Updates buffer allocation to use BUFFER_USAGE_NOT_SHARED
src/audio/host-zephyr.c Updates buffer allocation to use BUFFER_USAGE_NOT_SHARED
src/audio/host-legacy.c Updates buffer allocation to use BUFFER_USAGE_NOT_SHARED
src/audio/dai-zephyr.c Updates buffer allocation to use BUFFER_USAGE_NOT_SHARED
src/audio/dai-legacy.c Updates buffer allocation to use BUFFER_USAGE_NOT_SHARED
src/audio/chain_dma.c Updates buffer allocation to use BUFFER_USAGE_NOT_SHARED

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +121 to +122
#define BUFFER_USAGE_SHARED true /* buffer used by multiple DSP core and/or HW blocks */
#define BUFFER_USAGE_NOT_SHARED false /* buffer only used by one HW block */
Copy link

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider using enum values instead of boolean literals for buffer usage flags. This would provide better type safety and prevent accidental mixing of boolean values with these constants.

Suggested change
#define BUFFER_USAGE_SHARED true /* buffer used by multiple DSP core and/or HW blocks */
#define BUFFER_USAGE_NOT_SHARED false /* buffer only used by one HW block */
enum buffer_usage {
BUFFER_USAGE_NOT_SHARED = 0, /* buffer only used by one HW block */
BUFFER_USAGE_SHARED = 1 /* buffer used by multiple DSP core and/or HW blocks */
};

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are still places in code that have logic to calculate a boolean "is_shared" and I see no value to change these and make this an explicit enum. The define is fine to improve readability and matches existing coding style in SOF.

Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

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

could add buffer_new() here as well

Passing boolean parameters to key functions results in hard to read
code. Add BUFFER_USAGE_SHARED and BUFFER_USAGE_NOT_SHARED definitions
to make code allocating buffers easier to read and maintain.

No functional change.

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
@kv2019i
Copy link
Collaborator Author

kv2019i commented Sep 12, 2025

V2:

  • added flags to buffer_new() calls, feedback from @lyakh

@lgirdwood
Copy link
Member

@kv2019i can you check CI. Thanks !

@kv2019i
Copy link
Collaborator Author

kv2019i commented Sep 17, 2025

Sorry, I didn't realize this was failing. I was monitoring the previous quickbuild run for this PR and it passes (14737859 id -> all pass).

Now another quickbuild was run (14738805) and it filed to chain-dma xruns on MTL:

[    0.521330] <wrn> chain_dma: chain_task_run: dma_get_status() link xrun occurred, ret = -32
[    0.522226] <wrn> chain_dma: chain_task_run: dma_get_status() link xrun occurred, ret = -32
[    0.523166] <wrn> chain_dma: chain_task_run: dma_get_status() link xrun occurred, ret = -32
[    0.543078] <err> notification_pool: ipc_notification_pool_get: Pool depth exceeded

This seems like a real issue, but given this PR (at least should be) a no-op patch with no functional impact, I suspect this is not related to this PR.

@lrudyX have been seen similar failures in other PRs?

@lrudyX
Copy link

lrudyX commented Sep 17, 2025

Sorry, I didn't realize this was failing. I was monitoring the previous quickbuild run for this PR and it passes (14737859 id -> all pass).

Now another quickbuild was run (14738805) and it filed to chain-dma xruns on MTL:

[    0.521330] <wrn> chain_dma: chain_task_run: dma_get_status() link xrun occurred, ret = -32
[    0.522226] <wrn> chain_dma: chain_task_run: dma_get_status() link xrun occurred, ret = -32
[    0.523166] <wrn> chain_dma: chain_task_run: dma_get_status() link xrun occurred, ret = -32
[    0.543078] <err> notification_pool: ipc_notification_pool_get: Pool depth exceeded

This seems like a real issue, but given this PR (at least should be) a no-op patch with no functional impact, I suspect this is not related to this PR.

@lrudyX have been seen similar failures in other PRs?

Today we have some hardware problems with CI. We have quite often problems with this one test. I will rerun this PR as soon as possible.

@kv2019i kv2019i merged commit 4d898ff into thesofproject:main Sep 23, 2025
36 of 45 checks passed
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.

5 participants