[RFC] Implement APIs for coherent access of shared objects by ranj063 · Pull Request #4702 · thesofproject/sof

ranj063 · 2021-09-01T00:41:07Z

No description provided.

plbossart

I am pretty sure I missed the big picture here, this isn't straightforward to review...

plbossart · 2021-09-01T01:07:16Z

+ * audio component buffer - connects 2 audio components together in pipeline.
+ *
+ * The buffer is a hot structure that must be shared on certain cache
+ * incoherent architectures.


Shouldn't there be some property that actually checks if the buffer will be used between two cores? What happens in the typical case where the entire pipeline runs on one core? then all this cache coherency is not really needed except for DMA buffers, is it?

Put differently, is the multi-core support going to impact or possible cripple performance when chaining processing on a single core?

minimal impact for non shared objects - we still check the c->shared flag, so lock and cache ops are only performed on objects used by multiple cores.

__must_check static inline struct coherent *coherent_acquire(struct coherent *c, const size_t size) { /* assert if someone passes a cache/local address in here. */ ADDR_IS_COHERENT(c); /* this flavour should not be used in IRQ context */ CHECK_COHERENT_IRQ(c); /* access the shared coherent object */ if (c->shared) { CHECK_COHERENT_CORE(c); spin_lock(&c->lock); /* invalidate local copy */ dcache_invalidate_region(uncache_to_cache(c), size); } /* client can now use cached object safely */ return uncache_to_cache(c); }

concening @plbossart's remark on DMA use-cases: is this API actually supposed to be used for DMA memory too? Don't think it's well suited for that, since spinlocks don't protect against DMA. And I'm not sure why we'd need the Kconfig option then?

plbossart · 2021-09-01T01:17:37Z

 		if (!sinks[i])
 			continue;
 		buffer_writeback(sinks[i], sinks_bytes[i]);
+		sinks[i] = buffer_release(sinks[i]);


it's rather unclear to me when a writeback is explicitly required and when it's handled by the release?

plbossart · 2021-09-01T01:19:32Z

 	sink_bytes = frames * audio_stream_frame_bytes(&sink->stream);

+	sink = buffer_release(sink);
+


so that one has no invalidate for the sources?

plbossart · 2021-09-01T01:21:10Z

+
+		audio_stream_set_zero(&sink->stream, sink_bytes);
+		buffer_writeback(sink, sink_bytes);
+		comp_update_buffer_produce(sink, sink_bytes);


that one is super odd, you release before writing to tthe sink buffer?

lgirdwood

I've fixed build on all platforms - needs squashed and kconfig fix in lrg/topic/coherent_api

lgirdwood · 2021-09-02T09:25:44Z

I am pretty sure I missed the big picture here, this isn't straightforward to review...

Big picture is that we create a coherent object API so that

All coherent objects embed struct coherent (used for access, iteration and debug).
Objects must be acquired() before access (this means we get the lock and invalidate when lock is acquired).
We can now use cache ptr to object with no loss of performance.
Object is released() by user (this writes back and releases lock).

Further work would enhance this to support

separate coherent buffer R + W locking.
Opaque object types to enforce all object access through coherent APIs.

singalsu · 2021-09-02T14:31:26Z

"[DO NOT REVIEW YET]" ?

ranj063 · 2021-09-02T15:00:33Z

"[DO NOT REVIEW YET]" ?

Yes, please. still WIP and testing. I will remove the tag once it is ready for review

lyakh · 2021-09-08T08:01:17Z

looks like you have to make function parameters to struct comp_buffer **sink and struct comp_buffer **source to make sure the caller has valid pointers after function return, although in practice this has no effect

lyakh · 2021-09-08T08:02:41Z

...or maybe buffer_release() shouldn't be __must_check or maybe even return void. Wouldn't it be cleaner to use a separate handle variable for an "acquired buffer" and keep buffer as a generic object reference. But if they're both of type struct buffer * this creates danger of mistakenly accessing the object via the buffer pointer. Or you could make a separate struct buffer_handle type for it, but then you'd have to change more... Thinking out loud

lyakh · 2021-09-08T08:09:27Z

lyakh · 2021-09-08T08:12:21Z

also probably has to be **buffer and in all other similar cases

lyakh · 2021-09-08T08:16:14Z

I was thinking about making sure we don't have nested buffer_acquire() calls. So, maybe everywhere where a buffer is passed as a parameter it must be acquired by the caller?

nested will assert since the ptr alias being passed in will change..

lyakh · 2021-09-08T08:48:04Z

don't component buffer lists have to be protected? I still don't understand our design of component, buffer and other element sharing well enough, but it seems like it can be or become a problem? I see buffers are added to the lists in pipeline_connect() with just local interrupts disabled, but probably even with dynamic pipelines it cannot run simultaneously on different cores? Say, if you have a mixer, where one core is running a pipeline. Then an IPC is arriving to start a pipeline on another core with the same mixer, so that core tries to add a buffer to its list?

The coherent list iterator is needed and is a subsequent PR.

ranj063 · 2021-09-08T18:43:47Z

SOFCI TEST

lgirdwood · 2021-09-08T19:07:02Z

@ranj063 fwiw, the internal CI failures are all on TplgPipeComplete IPC after the first iteration with the multicore enabled in CI (so this may mean the multicore is not enabled in the Kconfig - and hence the failure)

lgirdwood · 2021-09-08T19:19:04Z

@ranj063 what I Mean is the KConfig say multicore and teh topology says multicore, but coherent APi probably says unicore.

lgirdwood · 2021-10-29T16:23:52Z

So the outstanding review comments are minor styling and could be fixed in a subsequent PR.

Certain data structures such as buffers, pipelines, comp_dev's are used across cores and cache boundaries. Introduce APIs to handle cached access to such objects by stipulating that the lock is held during access and releasing the lock after. For example: /* uncached work */ if (buffer->flag == somevalue) do this; /* cached work */ buffer = coherent_acquire(buffer); buffer->state = true; /* back to uncached */ buffer = coherent_release(buffer); Also, add a new kconfig INCOHERENT which will need to ibe set for platforms that need software-managed coherency. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Buffer are shared hot data structures that are used across core and cache boundaries. Manage cached buffer access by stipulating that cached access is granted by holding the buffer lock (which invalidates) and releasing the lock (invalidates and writeback). Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

instead of buffer_lock/unlock() which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

instead of buffer_lock/unlock which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

instead of buffer lock/unlock which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

instead of buffer_lock/unlock which will be deprecated Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

instead of buffer_lock/unlock() which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

instead of buffer_lock/unlock which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

instead of buffer_lock/unlock which will be deprecated Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

instead of buffer lock/unlock which will be deprecated Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

all users replaced with buffer_acquire/release Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

With the addition of struct coherent member in struct buffer, the size of the struct exceeds 128 bytes. Increase the 256 block heap size and reduce the 128 block size to fix out of memory issues when running multiple-pipeline-all test. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

lgirdwood · 2021-11-01T15:52:31Z

@kkarask @wszypelt looks like the CI is stuck ? The logs are not loading, can you refresh it ? Thanks !

kkarask · 2021-11-02T08:01:32Z

PR passed after rerun.

lgirdwood · 2021-11-02T15:54:31Z

SOFCI TEST

lgirdwood · 2021-11-02T22:16:25Z

CI showing a timeout on CML where the script was killed before it completed.
https://sof-ci.01.org/sofpr/PR4702/build10972/devicetest/?model=CML_RVP_SDW&testcase=check-suspend-resume-with-playback-5

lyakh · 2021-10-29T07:43:56Z

ehm, not sure __coherent is a good name for it. It does nothing for coherency. I understand that it's used for coherency, but to me it sounds as if it could provide coherency. Also why packed?

lyakh · 2021-10-29T08:41:29Z

sorry for late comments... Thinking about the name of coherent_acquire() - we are returning a cached alias, so it is incoherent actually. It's protected in software, yes, but that doesn't make it coherent...

lyakh · 2021-10-29T08:44:44Z

at some later time when we use lockdep, we might want to define __acquires() similar to the kernel...

lyakh · 2021-10-29T10:36:31Z

do we want to still use mixed cached / uncached access if CONFIG_INCOHERENT is disabled? Wouldn't it be possible and desirable to always use cached addresses in such cases?

lyakh · 2021-10-29T10:40:20Z

no need to assign

lyakh · 2021-10-29T10:41:12Z

lyakh · 2021-10-29T10:46:25Z

+ * audio component buffer - connects 2 audio components together in pipeline.
+ *
+ * The buffer is a hot structure that must be shared on certain cache
+ * incoherent architectures.


concening @plbossart's remark on DMA use-cases: is this API actually supposed to be used for DMA memory too? Don't think it's well suited for that, since spinlocks don't protect against DMA. And I'm not sure why we'd need the Kconfig option then?

lyakh · 2021-10-29T10:52:21Z

also here - it looks like all these methods aren't used for DMA handling

lyakh · 2021-10-29T11:00:30Z

wait, above you just asserted, that object is an uncached alias. And now you're using it for cache manipulation? Besides, if someone passes you an uncached object, doesn't it mean that there can be no stale cached data for it? Then why writing back its cache? Same in other similar locations

marc-hb · 2023-08-22T09:23:32Z

This topic is being revisited in:

[RFC] Direct use of cached / not-cached shared memory alias instead of coherent.h in case of buffers #8006

ranj063 requested review from bardliao, chiang831, cujomalainey, dbaluta, johnylin76, lbetlej, lgirdwood, mmaka1, plbossart and singalsu as code owners September 1, 2021 00:41

plbossart reviewed Sep 1, 2021

View reviewed changes

ranj063 changed the title ~~Implement APIs for coherent access of shared objects~~ [DO NOT REVIEW YET] Implement APIs for coherent access of shared objects Sep 1, 2021

lgirdwood reviewed Sep 1, 2021

View reviewed changes

ranj063 force-pushed the topic/coherent_api branch from c0b2c4d to d815821 Compare September 1, 2021 20:31

ranj063 force-pushed the topic/coherent_api branch from d815821 to 5419e8b Compare September 7, 2021 19:45

ranj063 requested review from akloniex, libinyang and mrajwa as code owners September 7, 2021 19:45

ranj063 changed the title ~~[DO NOT REVIEW YET] Implement APIs for coherent access of shared objects~~ [RFC] Implement APIs for coherent access of shared objects Sep 7, 2021

ranj063 force-pushed the topic/coherent_api branch 2 times, most recently from f865cbf to fdcc060 Compare September 7, 2021 20:28

lyakh reviewed Sep 8, 2021

View reviewed changes

ranj063 force-pushed the topic/coherent_api branch from fdcc060 to db1b9da Compare September 8, 2021 17:05

cujomalainey reviewed Sep 8, 2021

View reviewed changes

Comment thread src/include/sof/coherent.h Resolved

Comment thread src/include/sof/coherent.h Resolved

ranj063 added 21 commits November 1, 2021 07:59

host: use buffer acquire/release API

cdbb2fc

instead of buffer_lock/unlock() which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

dai: use buffer acquire/release

78cc2ae

instead of buffer_lock/unlock() which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

mixer: use buffer acquire/release

48cd865

instead of buffer_lock/unlock() which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

component: use buffer acquire/release

9491e57

instead of buffer_lock/unlock() which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ipc-helper: use buffer acquire/release

10f07f9

instead of buffer_lock/unlock() which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

src: use buffer acquire/release

ce7d768

instead of buffer_lock/unlock() which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

asrc: use buffer acquire/release

d204630

instead of buffer_lock/unlock() which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

pipeline: use buffer acquire/release

43c1213

instead of buffer_lock/unlock() which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

crossover: use buffer acquire/release

c6b379b

instead of buffer_lock/unlock() which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

google_hotword_detec: use buffer acquire/release

2e8696d

instead of buffer_lock/unlock which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

tone: use buffer acquire/release

6aac6ee

instead of buffer lock/unlock which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

kpb: use buffer_acquire/release

433dfb9

instead of buffer_lock/unlock which will be deprecated Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

mux: use buffer acquire/release

4f0717a

instead of buffer_lock/unlock which will be deprecated Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

selector: use buffer acquire/release

407744b

instead of buffer_lock/unlock() which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

smart_amp: use buffer_acquire/release

708ec8e

instead of buffer_lock/unlock which will be deprecated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

detect_test: use buffer acquire/release

515181a

instead of buffer_lock/unlock which will be deprecated Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

smart_amp_test: use buffer acquire/release

3dcae7c

instead of buffer lock/unlock which will be deprecated Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

buffer: deprecate buffer_lock/unlock

5b77437

all users replaced with buffer_acquire/release Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the topic/coherent_api branch from ca2a468 to 6b82d00 Compare November 1, 2021 15:00

lgirdwood merged commit b332dfa into thesofproject:main Nov 2, 2021

lyakh reviewed Nov 3, 2021

View reviewed changes

		sink_bytes = frames * audio_stream_frame_bytes(&sink->stream);

		sink = buffer_release(sink);

Conversation

ranj063 commented Sep 1, 2021

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Sep 2, 2021

Uh oh!

singalsu commented Sep 2, 2021

Uh oh!

ranj063 commented Sep 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Sep 8, 2021

Uh oh!

lgirdwood commented Sep 8, 2021

Uh oh!

lgirdwood commented Sep 8, 2021

Uh oh!

Uh oh!

Uh oh!

lgirdwood commented Oct 29, 2021

Uh oh!

lgirdwood commented Nov 1, 2021

Uh oh!

kkarask commented Nov 2, 2021

Uh oh!

lgirdwood commented Nov 2, 2021

Uh oh!

lgirdwood commented Nov 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!