Skip to content

Conversation

@lyakh
Copy link
Collaborator

@lyakh lyakh commented Nov 24, 2020

We're taking a new approach to this. Let's merge these 4 patches first, then we'll deal with the two more complex remaining patches from #3607 separately in PRs #3646 and #2647 and #3607 can be closed then.

@lyakh lyakh changed the title [TEST] Align test [TEST][NOT FOR REVIEW OR MERGING] Align test Nov 24, 2020
@lyakh
Copy link
Collaborator Author

lyakh commented Nov 24, 2020

The first test was only modifying allocating from single buffers. It "only" failed 3 cases from the "Internal Intel CI System/merge/build" test suite. Next going to try only modifying contiguous buffer allocations.

@lyakh lyakh force-pushed the align_test branch 3 times, most recently from e068b02 to de71fae Compare November 25, 2020 09:45
@lyakh
Copy link
Collaborator Author

lyakh commented Nov 26, 2020

all tests passed except one - travis-ci/pr, which hasn't completed since almost 24 hours now. Restarting.

@lyakh
Copy link
Collaborator Author

lyakh commented Nov 26, 2020

SOFCI TEST

@lyakh
Copy link
Collaborator Author

lyakh commented Nov 26, 2020

This time travis-ci/pr completed successfully. This time several device tests failed, but since these failures weren't there last time, I think they're unrelated. Once this is merged, I'll continue with the remaining more intrusive 2 patches

@lyakh lyakh marked this pull request as ready for review November 26, 2020 13:48
@lyakh lyakh changed the title [TEST][NOT FOR REVIEW OR MERGING] Align test Align basics Nov 27, 2020
@lyakh lyakh changed the title Align basics Align base Nov 27, 2020
@lyakh
Copy link
Collaborator Author

lyakh commented Nov 27, 2020

@zrombel could you please have a look at the quickbuild failures here? Yesterday the test was green, no idea why it decided to re-test and why it's failing now.
@lgirdwood current test failures seem to be inconsistent, so, looks like this series is safe?

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.

This all good except path 3, which should be a separate fix in another PR·

src/lib/alloc.c Outdated
Copy link
Member

Choose a reason for hiding this comment

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

ehm, they should be using this. Not all memory is equal. This fix should probably be a new PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, let's drop / postpone number 3 then, because they're interdependent

@lgirdwood
Copy link
Member

Not sure why we see module unload fail here. rerun.

@lgirdwood
Copy link
Member

SOFCI TEST

No need for two divisions for an alignment macro, one is enough.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
lyakh added 2 commits December 4, 2020 11:27
Simplify the align_ptr() function by re-using an existing ALIGN() macro.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
alloc_heap_buffer() is trying to allocate memory from a single heap,
no need to check its size on each iteration of an internal loop over
maps.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
@lyakh
Copy link
Collaborator Author

lyakh commented Dec 4, 2020

Not sure why we see module unload fail here. rerun.

@lgirdwood failures are so strange. I'd bet they're unrelated, but I don't see many similar failures in other PRs around now. The failure on CNL seems again to be the old problem of IPC contents flipping to 0xffffffff: thesofproject/linux#2498 , as for the other two failures on BSW and BDW I'd really need to debug them. I see a ton of "no bytes to consume" / "no bytes to produce" messages in logs and I don't see how this PR can introduce any functional changes. If anything it should only make alignment calculation slightly faster by avoiding one superfluous division - that's all! I'll split this into 3 PRs just to test...

@lgirdwood
Copy link
Member

@lyakh CI looks good now - there has been a back log.

@lgirdwood lgirdwood merged commit 8d277b5 into thesofproject:master Dec 15, 2020
@lyakh lyakh deleted the align_test branch December 21, 2020 08:45
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.

2 participants