google_aec: Sparse fixups #9265
Conversation
Commit 0eb34db ("google_aec: Don't allocate giant blobs on the heap") dropped some sparse annotations and exposed warnings with dealing with the cached temporary buffers handed to the AEC code. Unfortunately the underlying API isn't sparse-aware, so rather than go through the trouble to keep the tags consistent through the new conversion API (which is a little non-trivial syntactically) just to throw it away on entry to the library, force it off at conversion time for simplicity. Essentially now it's checking that all computation in this module is "uncached", which is fine as long as we don't try to mix conventions. Signed-off-by: Andy Ross <andyross@google.com>
| @@ -526,7 +535,7 @@ static int google_rtc_audio_processing_init(struct processing_module *mod) | |||
| cd->num_frames = NUM_FRAMES; | |||
|
|
|||
| /* Giant blob of scratch memory. */ | |||
| GoogleRtcAudioProcessingAttachMemoryBuffer(sys_cache_cached_ptr_get(&aec_mem_blob[0]), | |||
| GoogleRtcAudioProcessingAttachMemoryBuffer(cached_ptr(&aec_mem_blob[0]), | |||
There was a problem hiding this comment.
could we not just put a linter disable here for sparse?
There was a problem hiding this comment.
could we not just put a linter disable here for sparse?
I thought a "linter disable" was what this PR is already doing :-D
More seriously, what do you mean by "here"? This file, this directory, other?
How about something like this below? Now you could really rid all this code of all sparse annotations.
At this point it would better to create a new app/overlays/sparse.conf, similar to #9264 which I just submitted.
--- a/.github/workflows/sparse-zephyr.yml
+++ b/.github/workflows/sparse-zephyr.yml
@@ -70,6 +70,7 @@ jobs:
./sof/zephyr/docker-build.sh ${{ matrix.platform }} \
--cmake-args=-DZEPHYR_SCA_VARIANT=sparse --cmake-args=-DCONFIG_LOG_USE_VLA=n \
--cmake-args=-DCONFIG_MINIMAL_LIBC=y \
+ --cmake-args=-DCONFIG_GOOGLE_RTC_STUFF=n \
--pristine 2>&1 | tee _.log
printf '\n\n\t\t\t ---- Messages below are treated as sparse errors --- \n\n\n'There was a problem hiding this comment.
No no, we want the sparse checking in general in this file. It was this one situation where we're dealing with buffers (in cached addresses) that need to be passed to a third party library without sparse annotation that something needs to be fudged. I think there's room for argument about where the fudging should be (I lean hard to the "brevity" side over "pedantic correctness" in this patch). But all the other SOF API use wants to know that it will be cache-correct going forward.
There was a problem hiding this comment.
Ah got it, wasn't sure if we could do something like a style linter and drop a comment to silence this one line
|
And as for title, yeah.... that's just off. What this patch is literally doing is adjusting the sparse annotations that got dropped. And we can't put " |
We can. checkpatch is not always right AND it is easy to ignore because it is not "persistent" like many other checkers. I'm afraid this lack of "persistency" is very often missed. |
marc-hb
added
bug
|
All 3 MTL models in https://sof-ci.01.org/sofpr/PR9265/build6047/devicetest/index.html have a DSP panic, wow... EDIT: it's unrelated: LNL https://sof-ci.01.org/sofpr/PR9265/build6046/devicetest/index.html looks OK. |
|
The failure in https://sof-ci.01.org/sof-pr-viewer/#/build/PR9265/build14070008 is just a |
marc-hb
requested review from
dbaluta,
lyakh,
LaurentiuM1234,
dcpleung,
abonislawski,
marcinszkudlinski and
jxstelter
|
OMG, while looking for reviewers I added @lyakh (who had already approved without being in the list) and this had the side-effect of dropping his approval... Yay Github - sorry about that. |
Commit 0eb34db ("google_aec: Don't allocate giant blobs on the heap") dropped some sparse annotations and exposed warnings with dealing with the cached temporary buffers handed to the AEC code.
Unfortunately the underlying API isn't sparse-aware, so rather than go through the trouble to keep the tags consistent through the new conversion API (which is a little non-trivial syntactically) just to throw it away on entry to the library, force it off at conversion time for simplicity. Essentially now it's checking that all computation in this module is "uncached", which is fine as long as we don't try to mix conventions.