Skip to content

Conversation

@lyakh
Copy link
Collaborator

@lyakh lyakh commented Nov 26, 2020

ILL entries in the DW DMA driver are accessed both by the controller, using DMA and by the DSP. For this to work the DSP has to force cache synchronisation appropriately. Therefore the ILL area has to be allocated with cache-line size alignment. This patch switches allocation from rmalloc() to rballoc() to force such alignment.

Copy link
Collaborator

@paulstelian97 paulstelian97 Nov 26, 2020

Choose a reason for hiding this comment

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

Since you claim the problem is about alignment, why not rballoc_align with a specific alignment value that ought to solve it? Even if you pass it PLATFORM_DCACHE_ALIGN which thus makes it equivalent, I think it's better to be explicit in this particular case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@paulstelian97 It depends on how rballoc() is defined. If it is a part of its definition, that returned memory is always cache-line size aligned, then I think the code is fine as is. If however it's just a kind of a hidden side-effect, that might change in the future, then yes, it would be better to change this. I'm fine either way. Let's wait for a couple of hours, if there are no opinions one way or another, if the CI "failure" doesn't mean this is breaking functionality, I can change it, sure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh it's completely equivalent, rballoc(...) is precisely rballoc_aligned(..., PLATFORM_DCACHE_ALIGN). However I wanted to see the alignment being explicitly specified for coding style points.

@lyakh
Copy link
Collaborator Author

lyakh commented Nov 27, 2020

Do I understand it correctly, that the failure on BYT_MB_NOCODEC is because of the signal frequency being up to 10% off? Can it really be related to this PR?

@lgirdwood
Copy link
Member

Do I understand it correctly, that the failure on BYT_MB_NOCODEC is because of the signal frequency being up to 10% off? Can it really be related to this PR?

Looks like a dropped sample.
Screenshot from 2020-11-27 15-14-05

Will rerun CI again, this has happened before so may be unrelated.

@lgirdwood
Copy link
Member

SOFCI TEST

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

Can we make the alignment obvious in the code.

@lyakh
Copy link
Collaborator Author

lyakh commented Nov 27, 2020

Can we make the alignment obvious in the code.

@lgirdwood sure, let's wait for test completion first?

@lyakh
Copy link
Collaborator Author

lyakh commented Nov 27, 2020

Can we make the alignment obvious in the code.

@lgirdwood sure, let's wait for test completion first?

ok, one unrelated failure, will have to file a bug... Pushing update

@plbossart
Copy link
Member

Wondering if this could help with #3492. we could never figure out what causes the DMA to repeat samples.

@lgirdwood
Copy link
Member

@zrombel can you confirm if your APL DUT is good, it looks like all test of failed on this DUT whilst the others are fine.
@lyakh we may need to use existing method for APL if CI is right.

@zrombel
Copy link

zrombel commented Dec 3, 2020

@lgirdwood yes, it looks like platform failure. Running rebuild right away.

@lyakh
Copy link
Collaborator Author

lyakh commented Dec 3, 2020

@zrombel Thanks for re-running. @lgirdwood The latest failure is unrelated: "[drm] ERROR CPU pipe A FIFO underrun" and a fix has been merged 2 days ago thesofproject/sof-test#535 so, actually, this shouldn't appear any more if the current sof-test is used

ILL entries in the DW DMA driver are accessed both by the controller,
using DMA and by the DSP. For this to work the DSP has to force cache
synchronisation appropriately. Therefore the ILL area has to be
allocated with cache-line size alignment. This isn't the case with the
Zephyr allocator. This patch switches allocation from rmalloc() to
rballoc() to force such alignment. Eventually a generic fix will be
implemented in an SOF wrapper fpr the Zephyr allocator.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
@lgirdwood
Copy link
Member

SOFCI TEST

@lgirdwood
Copy link
Member

Rerun CI, seems jenkins stalled.

@lgirdwood
Copy link
Member

Jenkins known issue.

@lgirdwood lgirdwood merged commit a68c756 into thesofproject:master Dec 7, 2020
@lyakh lyakh deleted the dwdma branch December 18, 2020 17:32
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.

6 participants