Skip to content

Conversation

@kv2019i
Copy link
Contributor

@kv2019i kv2019i commented Nov 8, 2024

Use the existing 'atomic' bitmask to speed up ISR processing for CONFIG_DMA_INTEL_ADSP_HDA_TIMING_L1_EXIT. This bitmask is used to track enabled DMA channels.

In the common case, only a few DMA channels are active and low channels are allocated first. Take advantage of this and not iterate over all DMA channels of all all host devices. Rather break out as soon as L1 exit handling is done for all enabled channels.

@rruuaanng rruuaanng added the area: DMA Direct Memory Access label Nov 9, 2024
Use the existing 'atomic' bitmask to speed up ISR processing for
CONFIG_DMA_INTEL_ADSP_HDA_TIMING_L1_EXIT. This bitmask is used
to track enabled DMA channels.

In the common case, only a few DMA channels are active and low
channels are allocated first. Take advantage of this and not
iterate over all DMA channels of all all host devices. Rather
break out as soon as L1 exit handling is done for all enabled
channels.

Signed-off-by: Kai Vehmanen <[email protected]>
Align to coding style and use braces for all if blocks.

Signed-off-by: Kai Vehmanen <[email protected]>
@kv2019i kv2019i force-pushed the 202411-dma-isr-optim branch from 158a984 to a2f1f78 Compare November 18, 2024 10:24
@kv2019i kv2019i marked this pull request as ready for review November 18, 2024 10:25
@kv2019i kv2019i added the platform: Intel ADSP Intel Audio platforms label Nov 18, 2024
@kv2019i
Copy link
Contributor Author

kv2019i commented Nov 18, 2024

V2:

  • fixed conformance issue
  • added one cosmetic patch to fix braces use in the ISR function
  • marked as ready for review

@kv2019i kv2019i requested a review from lyakh November 18, 2024 10:27

for (j = 0; j < dma_ctx->dma_channels; j++) {
if (!atomic_test_bit(dma_ctx->atomic, j))
enabled_chs = atomic_get(dma_ctx->atomic);
Copy link
Contributor

Choose a reason for hiding this comment

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

this suggests that the .atomic field cannot change while we're in the ISR? Also not by a different core?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lyakh It can change, but it's harmless for this loop as either we do one unnecessary check (for a channel that is already disabled by another core), or we have an additional ISR if it's enabled concurrently.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kv2019i ok, sounds good, thanks for explaining! If we ever end up changing this file again, maybe would be good to add this as a comment there

if (!(enabled_chs & BIT(j))) {
continue;
}
enabled_chs &= ~(BIT(j));
Copy link
Contributor

Choose a reason for hiding this comment

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

BIT() is properly defined, no need for additional parentheses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok this got merged already, I can fix if we have other changes to do in a follow-up.

@mmahadevan108
Copy link
Contributor

@kv2019i can you take a look at the comments from @lyakh

@aescolar aescolar merged commit 58df253 into zephyrproject-rtos:main Nov 21, 2024
26 checks passed
Copy link
Contributor Author

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Some post-merge comments. I can do a updated PR if needed, please check @lyakh


for (j = 0; j < dma_ctx->dma_channels; j++) {
if (!atomic_test_bit(dma_ctx->atomic, j))
enabled_chs = atomic_get(dma_ctx->atomic);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lyakh It can change, but it's harmless for this loop as either we do one unnecessary check (for a channel that is already disabled by another core), or we have an additional ISR if it's enabled concurrently.

if (!(enabled_chs & BIT(j))) {
continue;
}
enabled_chs &= ~(BIT(j));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok this got merged already, I can fix if we have other changes to do in a follow-up.

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

Labels

area: DMA Direct Memory Access platform: Intel ADSP Intel Audio platforms

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants