-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
drivers: dma: intel-adsp-hda: Report total_copied bytes on ACE2/3 #77726
drivers: dma: intel-adsp-hda: Report total_copied bytes on ACE2/3 #77726
Conversation
drivers/dma/dma_intel_adsp_hda.c
Outdated
|
||
llp_l = *DGLLLPL(cfg->base, cfg->regblock_size, channel); | ||
llp_u = *DGLLLPU(cfg->base, cfg->regblock_size, channel); | ||
stat->total_copied = ((uint64_t)llp_u << 32) | llp_l; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't there a potential problem with this code?
if llp_l is MAX_32, then llp_u will likely be increased by the time you read it, but the llp_l be close to zero. This sort of combination carries the risk of creating completely wrong counts. I think you need to re-read the llp_l and make sure its value did not become lower than before...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plbossart, it is copied directly from the existing intel_adsp_gpdma_llp_read()
from
https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/dma/dma_intel_adsp_gpdma.c#L133
I see, I will update, thanks for spotting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow, I earned by salary for the day. GPDMA is also wrong haha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably need something along those lines
diff --git a/drivers/dma/dma_intel_adsp_gpdma.c b/drivers/dma/dma_intel_adsp_gpdma.c
index 5e3850244d9b..a3e24e994ad5 100644
--- a/drivers/dma/dma_intel_adsp_gpdma.c
+++ b/drivers/dma/dma_intel_adsp_gpdma.c
@@ -129,9 +129,15 @@ static inline void intel_adsp_gpdma_llp_read(const struct device *dev,
{
#ifdef CONFIG_DMA_INTEL_ADSP_GPDMA_HAS_LLP
const struct intel_adsp_gpdma_cfg *const dev_cfg = dev->config;
+ uint32_t llp_l1;
+ uint32_t llp_l2;
- *llp_l = dw_read(dev_cfg->shim, GPDMA_CHLLPL(channel));
+ llp_l1 = dw_read(dev_cfg->shim, GPDMA_CHLLPL(channel));
*llp_u = dw_read(dev_cfg->shim, GPDMA_CHLLPU(channel));
+ llp_l2 = dw_read(dev_cfg->shim, GPDMA_CHLLPL(channel));
+ if (llp_l2 < llp_l1)
+ *llp_u = dw_read(dev_cfg->shim, GPDMA_CHLLPU(channel));
+ *llp_l = llp_l2;
#endif
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes... the ole double word counter read, good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ujfalusi do you want to send the same fix for GPDMA? same problem.
c62bcc6
to
2c596ec
Compare
Changes since v1:
|
drivers/dma/dma_intel_adsp_hda.c
Outdated
/* re-read the LLPU value, as LLPL just wrapped */ | ||
llp_u = *DGLLLPU(cfg->base, cfg->regblock_size, channel); | ||
} | ||
stat->total_copied = ((uint64_t)llp_u << 32) | llp_l; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what would stat->total_copied
be equal to on other platforms then? Not set at all? Should we set it to 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, but it is not subject of this PR, I can create a followup to reset the total_copied to 0 (set llp_l/u to 0) if it is not handled and at the same time do similar update to the gpdma.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a sec, I will update this PR, the gpdma handles this already.
With ACE2/3 the HDA DMA includes registers to read the Linear Link Position. Previous platforms (CAVS, ACE1) was able to report the LLP for GPDMA. Since ACE2 all links are handled with HD-DMA, hence the new register has been added for the firmware to report the LLP to the host. Set the total_copied to 0 for older ACE1/CAVS platforms and in case of host DMA on ACE2/3 since the informatiojn is not available. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
5da4b43
2c596ec
to
5da4b43
Compare
Changes since v2:
|
With ACE2/3 the HDA DMA includes registers to read the Linear Link
Position.
Previous platforms (CAVS, ACE1) was able to report the LLP for GPDMA. Since
ACE2 all links are handled with HD-DMA, hence the new register has been
added for the firmware to report the LLP to the host.
Set the total_copied to 0 for older ACE1/CAVS platforms and in case of
host DMA on ACE2/3 since the informatiojn is not available.