[DNM] Direct use of cached / not-cached shared memory alias instead of coherent.h in case of buffers by tobonex · Pull Request #8106 · thesofproject/sof

tobonex · 2023-08-28T10:38:25Z

DO NOT MERGE - the PR will be spit to several PRs

All info and context: #8006

lgirdwood · 2023-08-28T12:03:17Z

@tobonex fyi - some conflicts (will block CI).

buffer_acquire and buffer_release are no longer in use remove stubs from the code Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

to verify that shared structures are used in a proper way a debug check is introduced If compiled with COHERENT_CHECK_NONSHARED_CORES flag each usage of shared structures will be verified against proper memory alias usage Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

kv2019i · 2023-09-06T09:18:22Z

Ok, let's do final review going through the details better. Please ack in your approve comment which parts of the patchset you covered (if not all). Target is to merge this PR today.

marcinszkudlinski · 2023-09-06T09:38:48Z

the most important is to check sparse: remove buffer_acquire / buffer_release

second huge commit -
sparse: remove all sparse_cache from struct sink/source api is created 100% by editor "replace all" so is much less likely be incorrect

other commits have already been discussed and approved

tobonex · 2023-09-06T10:03:44Z

While working on it I noted some significant changes, some may be outdated:

copier_ipcgtw_process() in copier_ipcgtw.c

mux_get_processing_function in mux_generic.c - buffer was not released here for some reason

dai_dma_cb in dai-zephyr.c err label is hanging after deleting buffer_release here, should we just break the list
loop? If yes what is the best way to do that?

audio_steram.h:1074 above audio_stream_get_source - do we want to delete comment above it now?
also do we want to delete buffer_acquire and buffer_release definitions now?

host_legacy.c: host_common_update() - had to leave the logic here
host_zephyr.c: host_common_update() - had to leave the logic here

module_adapter.c : check if I refactored those correctly: module_adapter_sink_src_prepare,
module_adapter_sink_source_copy

module_adapter_audio_stream_type_copy - leaving variables as they are, just removing sparse_cache. I want to avoid changing multiple functions for this one.

generic.h module_source_info_acquire() : is removing it from coherent ok?

some rebased files to check(all commits):
multiband_drc.c
volume.c

marcinszkudlinski · 2023-09-06T10:10:54Z

copier_ipcgtw_process() in copier_ipcgtw.c

What about it?

mux_get_processing_function in mux_generic.c - buffer was not released here for some reason

dai_dma_cb in dai-zephyr.c err label is hanging after deleting buffer_release here, should we just break the list
loop? If yes what is the best way to do that?

something worth to look twice.
BTW. please add such comments in the code as review - it is easier

audio_steram.h:1074 above audio_stream_get_source - do we want to delete comment above it now?
removed

5)also do we want to delete buffer_acquire and buffer_release definitions now?
removed

host_legacy.c: host_common_update() - had to leave the logic here
?????

7)host_zephyr.c: host_common_update() - had to leave the logic here
????

module_adapter.c : check if I refactored those correctly: module_adapter_sink_src_prepare,
module_adapter_sink_source_copy
module_adapter_audio_stream_type_copy

checked, is OK

generic.h module_source_info_acquire() : is removing it from coherent ok?
not at this step, left in the code

tobonex · 2023-09-06T10:18:45Z

mux_get_processing_function in mux_generic.c - buffer was not released here for some reason ?????

this is dmux_get_processing, i meant the one above, mux_get_processing, sink_c was not released for some reason

marcinszkudlinski · 2023-09-06T10:33:08Z

this is dmux_get_processing, i meant the one above, mux_get_processing, sink_c was not released for some reason

right, there was no release in mux_get_processing_function
looking as those 2 were called I bet is just a forgotten release

as long as the buffer was not shared such lack of release have had no negative effects

lgirdwood · 2023-09-06T10:47:12Z

ok, this is a problem. We need to play safer here. Can we

Create a new PR that will merge 1st and make coherent.h APIs a NOP i.e. turns then into empty static inline functions for all configurations.
Update this PR on top of PR 1 above and split patch per file. This could even mean breaking this PR into more PRs and incrementally removing coherent APIs. i.e. its no longer a big bang.

This approach means we have remove the performance penalty from coherent and break the work down into more manageable chunk that we can bisect and individually revert patches if needed with no impact to other modules.

kv2019i · 2023-09-06T10:48:44Z

Checked (potentially less coverage in upstream CI):

src/audio/module_adapter/module/cadence.c
src/audio/module_adapter/module/dts/dts.c
src/audio/module_adapter/module/waves/waves.c

tobonex · 2023-09-06T10:51:35Z

src/audio/copier/copier_ipcgtw.c

 		 */
-		buf_c = NULL;
 		comp_warn(dev, "copier_ipcgtw_process(): no buffer found");
 	}


about " copier_ipcgtw_process() in copier_ipcgtw.c" I just meant this if block,
I did not substitute buf for buf_c here, as we do not need to NULL it now

kv2019i · 2023-09-06T11:00:33Z

@tobonex Can you add the comments inline so it's easier to discuss specific points in the context of the code in question. The comment on dai-zephyr.c sounds a bit scary, but I guess you mean with "hanging" that the error label is left to code where it really is not needed anymore, right? Code is not actually hanging...

tobonex · 2023-09-06T11:06:24Z

@tobonex Can you add the comments inline so it's easier to discuss specific points in the context of the code in question. The comment on dai-zephyr.c sounds a bit scary, but I guess you mean with "hanging" that the error label is left to code where it really is not needed anymore, right? Code is not actually hanging...

Yes, I'm making the comments now. And yes I meant the label is pointing at the end of block, not execution stopping.

marcinszkudlinski · 2023-09-06T11:11:02Z

@tobonex Can you add the comments inline so it's easier to discuss specific points in the context of the code in question. The comment on dai-zephyr.c sounds a bit scary, but I guess you mean with "hanging" that the error label is left to code where it really is not needed anymore, right? Code is not actually hanging...

I'm double checking dai-zephyr right now

tobonex · 2023-09-06T11:26:39Z

src/audio/dai-zephyr.c

+									      bytes);
 			}
 		}
+err:


Looking at it again I think I made a mistake here, I moved the label out of the list loop. It seems the loop does not stop after an error, and just continues on to the next iteration, as error label was there to just release the buffer. Will it jump correctly if it points to end of a loop block?

jxstelter

Module_adapter, ipc

lyakh · 2023-09-12T08:29:18Z

src/ipc/ipc-helper.c

 		if (!comp->is_shared)
 			comp_make_shared(comp);
-	}
+		}


this looks bogus

lyakh · 2023-09-12T08:39:04Z

src/audio/copier/copier.c

 	/* convert format and copy to each active sink */
 	for (i = 0; i < num_output_buffers; i++) {
-		struct comp_buffer __sparse_cache *sink_c;
+		struct comp_buffer *sink_c;


should we drop "_c" from sink_c and src_c

marcinszkudlinski · 2023-09-18T05:54:37Z

closing, this PR has been split to several smaller parts

tobonex force-pushed the shared_memory_comp_buffer_localcopy branch 24 times, most recently from 62ec1df to 7cfd878 Compare September 4, 2023 09:13

tobonex changed the title ~~[CI TEST ONLY] Direct use of cached / not-cached shared memory alias instead of coherent.h in case of buffers~~ Direct use of cached / not-cached shared memory alias instead of coherent.h in case of buffers Sep 4, 2023

tobonex marked this pull request as ready for review September 4, 2023 09:22

tobonex requested review from bardliao, marcinszkudlinski and pblaszko as code owners September 4, 2023 09:22

marcinszkudlinski added 2 commits September 6, 2023 10:46

buf: remove buffer_acquire/buffer_release

9c5cd07

buffer_acquire and buffer_release are no longer in use remove stubs from the code Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

marcinszkudlinski force-pushed the shared_memory_comp_buffer_localcopy branch from 8f30aa9 to 756705c Compare September 6, 2023 08:47

tobonex commented Sep 6, 2023

View reviewed changes

marcinszkudlinski mentioned this pull request Sep 6, 2023

Remove sparse step1 #8166

Merged

tobonex commented Sep 6, 2023

View reviewed changes

jxstelter approved these changes Sep 6, 2023

View reviewed changes

kv2019i changed the title ~~Direct use of cached / not-cached shared memory alias instead of coherent.h in case of buffers~~ [DNM] Direct use of cached / not-cached shared memory alias instead of coherent.h in case of buffers Sep 6, 2023

kv2019i mentioned this pull request Sep 7, 2023

smart_amp: revamp to two-layer modular design structure #8036

Merged

lyakh reviewed Sep 12, 2023

View reviewed changes

btian1 mentioned this pull request Sep 18, 2023

cache: remove buffer_acquire/release form infrastructure #8206

Merged

marcinszkudlinski closed this Sep 18, 2023

tobonex mentioned this pull request Sep 28, 2023

cache: remove buffer_acquire/buffer_release stubs #8272

Merged

Conversation

tobonex commented Aug 28, 2023 • edited by kv2019i Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Aug 28, 2023

Uh oh!

kv2019i commented Sep 6, 2023

Uh oh!

marcinszkudlinski commented Sep 6, 2023

Uh oh!

tobonex commented Sep 6, 2023

Uh oh!

marcinszkudlinski commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobonex commented Sep 6, 2023

Uh oh!

marcinszkudlinski commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Sep 6, 2023

Uh oh!

kv2019i commented Sep 6, 2023

Uh oh!

tobonex Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

kv2019i commented Sep 6, 2023

Uh oh!

tobonex commented Sep 6, 2023

Uh oh!

marcinszkudlinski commented Sep 6, 2023

Uh oh!

tobonex Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

jxstelter left a comment

Choose a reason for hiding this comment

Uh oh!

lyakh Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

lyakh Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

marcinszkudlinski commented Sep 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tobonex commented Aug 28, 2023 •

edited by kv2019i

Loading

marcinszkudlinski commented Sep 6, 2023 •

edited

Loading

marcinszkudlinski commented Sep 6, 2023 •

edited

Loading