Dram data storage: fast_get()/_put() #9765

jsarha · 2025-01-07T13:11:44Z

@lyakh , @singalsu , @lrgirdwo This is the current situation with fast_get()/_put(), and its usage in SRC module.

singalsu

Looks good to me. Were you able to check MCPS impact before/after. I guess it should not impact?

jsarha · 2025-01-07T17:20:06Z

Looks good to me. Were you able to check MCPS impact before/after. I guess it should not impact?

No there was no difference:
No SRC
ll_thread2 stack 18.8% load 5.9%

44.1kHz to 48kHz SRC coefs in SRAM
ll_thread2 stack 18.8% load 16.1%

44.1kHz to 48kHz SRC coefs in DRAM
ll_thread2 stack 18.8% load 23.1%

44.1kHz to 48kHz SRC coefs copied from DRAM to SRAM
ll_thread2 stack 18.8% load 16.1%

The full measurements log is here:
src_tests_with_nocodec.txt

singalsu · 2025-01-07T17:38:10Z

Looks good to me. Were you able to check MCPS impact before/after. I guess it should not impact?

No there was no difference: No SRC ll_thread2 stack 18.8% load 5.9%

44.1kHz to 48kHz SRC coefs in SRAM ll_thread2 stack 18.8% load 16.1%

44.1kHz to 48kHz SRC coefs in DRAM ll_thread2 stack 18.8% load 23.1%

44.1kHz to 48kHz SRC coefs copied from DRAM to SRAM ll_thread2 stack 18.8% load 16.1%

The full measurements log is here: src_tests_with_nocodec.txt

Thanks, that's very good news! Actually I'm surprised that that load without copy to SRAM isn't higher.

jsarha · 2025-01-08T08:14:33Z

Thanks, that's very good news! Actually I'm surprised that that load without copy to SRAM isn't high

It is. There was three tests, coefs in SRAM all the time (16.1% load), coefs in DRAM no copying (23.1% load), and coefs in DRAM but copied to SRAM before use (16.1% load). An for comparison, same pipelines without need for SRC 5.9% load.

lyakh

Looks quite good! Just a couple of simple mostly cosmetic changes

lyakh · 2025-01-08T10:37:07Z

Kconfig.sof

+	  module data from DRAM to SRAM when the data is needed and freeing
+	  the SRAM when the data is not needed anymore. If multiple module
+	  instances need the same chunk the same copy is used with reference
+	  counting. Source is src/lib/gast-get.c. The option should be selected


typo "fast-get.c"

lyakh · 2025-01-08T10:38:35Z

src/include/sof/lib/fast-get.h

+
+#include <stddef.h>
+
+void *fast_get(const void * const dram_ptr, size_t size);


must return a const pointer too

lyakh · 2025-01-08T10:38:48Z

src/include/sof/lib/fast-get.h

+#include <stddef.h>
+
+void *fast_get(const void * const dram_ptr, size_t size);
+void fast_put(void *sram_ptr);


const void *

lyakh · 2025-01-08T10:42:23Z

src/lib/fast-get.c

+/* for cmocka environment */
+#ifndef EXPORT_SYMBOL
+#define EXPORT_SYMBOL(a)
+#endif


mmh, it is already defined in posix/include/rtos/symbol.h - isn't that enough? Need to

#include <rtos/symbol.h>

then it should work

lyakh · 2025-01-08T10:42:54Z

src/lib/fast-get.c

+	const void *dram_ptr;
+	void *sram_ptr;
+	size_t size;
+	uint32_t refcount;


why not just unsigned int?

lyakh · 2025-01-08T11:08:30Z

test/cmocka/src/lib/fast-get/fast-get-tests.c

+{
+	void *ret;
+
+	(void)state; /* unused */


does ARG_UNUSED() work in cmocka?

Its not defined in cmocka environment. Its coming from Zephyr side, and there its defined as
#define ARG_UNUSED(x) (void)(x)

I could in make some similar hack header for cmocka code, but open coded void cast is already used in several places cmocka tests.

lyakh · 2025-01-08T11:15:16Z

test/cmocka/src/lib/fast-get/fast-get-tests.c

+		copy[0][i] = fast_get(testdata[i], sizeof(testdata[0]));
+
+	for (i = 0; i < ARRAY_SIZE(copy[0]); i++)
+		copy[1][i] = fast_get(testdata[i], sizeof(testdata[0]));


you could also test unmatched allocation - with a wrong size. Also you could add a test for too large an allocation - don't think malloc() will succeed a 1GiB allocation even if it's available, IIRC there was a limit. But maybe I'm wrong. This can be an incremental PR, this test set is already quite good!

Sure, I can add one more test. But I do not think the allocation limit should be enforced by fast-get-code.

lyakh · 2025-01-08T11:16:55Z

src/audio/src/src_common.c

@@ -593,6 +594,60 @@ int src_param_set(struct comp_dev *dev, struct comp_data *cd)
 	return 0;
 }

+static void debug_print(struct comp_dev *dev, const char *name, const struct src_stage *s)


lyakh · 2025-01-08T11:17:43Z

src/audio/src/src_common.c

+	stage_dst1 = rmalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM,
+			     2 * sizeof(*stage_dst1));
+	if (!stage_dst1) {
+		comp_err(dev, "src_allocate_copy_stages(): failed allocate stages");


please don't use function name in logging, Zephyr does it already

lyakh · 2025-01-08T11:29:46Z

src/audio/src/src_common.c

+			     const struct src_stage *stage_src1,
+			     const struct src_stage *stage_src2)
+{
+	struct src_stage *stage_dst1;


maybe just use an array as struct src_stage *stage_dst and then stage_dst[0].coefs etc. but up to you too, just an idea

I don't bother with the change, not yet anyway. I have a hunch that there will be more rounds coming.

Can we put the ifdef FAST_GET at the function level i.e. if FAST_GET=n then src_allocate_copy_stages() does what it does today, if =y then it does the fast get logic

lgirdwood · 2025-01-08T16:24:45Z

@jsarha can you list any dependency PRs that need to be merged first here. Thanks !

With this data can be assigned to a separate read-only section, which then will be kept in DRAM without copying it to SRAM on module instantiation. Signed-off-by: Guennadi Liakhovetski <[email protected]>

src coefficients take a lot of space, keep them in DRAM to only copy used sets in SRAM. Signed-off-by: Guennadi Liakhovetski <[email protected]>

lyakh · 2025-01-09T09:55:15Z

@jsarha can you list any dependency PRs that need to be merged first here. Thanks !

@lgirdwood I think all the dependencies have been merged now. It just needs to switch to using a newer version of my commits from #9721

lgirdwood · 2025-01-13T17:08:02Z

@jsarha can you list any dependency PRs that need to be merged first here. Thanks !

@lgirdwood I think all the dependencies have been merged now. It just needs to switch to using a newer version of my commits from #9721

@lyakh just some conflicts now.

Implement fast_get() and fast_put() functions. The purpose of these functions is to maintain shared SRAM copies of data stored in DRAM. fast_get() First checks if there is already an SRAM copy of the same DRAM chunk. If there isn't reserve an SRAM chunk of the same size and copy the contents there, store the both pointers, size and reference count to in an internal data structure, and return the SRAM pointer. If there is, return the pointer to the existing SRAM copy and increase the reference count. fast_put() Look up the internal data record based on the SRAM address and decrement reference count. Free the SRAM chunk and the data record if reference count reaches zero, Signed-off-by: Jyri Sarha <[email protected]>

And simple module tests for fast_get and fast_put() implemented in src/lib/fast-get/fast-get.c. Signed-off-by: Jyri Sarha <[email protected]>

The SRC coefficients are loaded to DRAM and commit copies the coefficients to SRAM when they are needed. The copying is done using fast_get() and the copy is released with fast_put() when its not needed anymore. Signed-off-by: Jyri Sarha <[email protected]>

jsarha

Review comment applied and commits rebased on top of the latest #9721

jsarha · 2025-01-13T15:57:45Z

test/cmocka/src/lib/fast-get/fast-get-tests.c

+{
+	void *ret;
+
+	(void)state; /* unused */


Its not defined in cmocka environment. Its coming from Zephyr side, and there its defined as
#define ARG_UNUSED(x) (void)(x)

I could in make some similar hack header for cmocka code, but open coded void cast is already used in several places cmocka tests.

jsarha · 2025-01-13T16:51:57Z

test/cmocka/src/lib/fast-get/fast-get-tests.c

+		copy[0][i] = fast_get(testdata[i], sizeof(testdata[0]));
+
+	for (i = 0; i < ARRAY_SIZE(copy[0]); i++)
+		copy[1][i] = fast_get(testdata[i], sizeof(testdata[0]));


Sure, I can add one more test. But I do not think the allocation limit should be enforced by fast-get-code.

jsarha · 2025-01-15T15:20:05Z

src/audio/src/src_common.c

+			     const struct src_stage *stage_src1,
+			     const struct src_stage *stage_src2)
+{
+	struct src_stage *stage_dst1;


I don't bother with the change, not yet anyway. I have a hunch that there will be more rounds coming.

jsarha · 2025-01-15T15:28:39Z

After the rebase something has happened to the performance of the case where coefficients are left in DRAM and not copied to SRAM. The 44.1kHz playback load of that case has dropped from 23.1% to 18.8%. @lyakh , any idea where this is coming? This result is from MTL so it may not be important, but it is still interesting. Also, now the build fails if SRC is not built as module.

singalsu reviewed Jan 7, 2025

View reviewed changes

lyakh reviewed Jan 8, 2025

View reviewed changes

lyakh added 2 commits January 9, 2025 10:12

llext: add support for read-only data in DRAM

0f6caa5

With this data can be assigned to a separate read-only section, which then will be kept in DRAM without copying it to SRAM on module instantiation. Signed-off-by: Guennadi Liakhovetski <[email protected]>

audio: src: move all coefficients to DRAM

c234161

src coefficients take a lot of space, keep them in DRAM to only copy used sets in SRAM. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Jyri Sarha added 3 commits January 15, 2025 11:38

test: cmocka: Add module tests for fast_get() and fast_put()

1199bf3

And simple module tests for fast_get and fast_put() implemented in src/lib/fast-get/fast-get.c. Signed-off-by: Jyri Sarha <[email protected]>

jsarha commented Jan 15, 2025

View reviewed changes

jsarha force-pushed the dram_data_storage branch from 1c74172 to d7f2279 Compare January 15, 2025 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dram data storage: fast_get()/_put() #9765

Dram data storage: fast_get()/_put() #9765

jsarha commented Jan 7, 2025

singalsu left a comment

jsarha commented Jan 7, 2025 •

edited

Loading

singalsu commented Jan 7, 2025

jsarha commented Jan 8, 2025

lyakh left a comment

lyakh Jan 8, 2025 •

edited

Loading

lyakh Jan 8, 2025

lyakh Jan 8, 2025

lyakh Jan 8, 2025

lyakh Jan 8, 2025

lyakh Jan 8, 2025

jsarha Jan 13, 2025

lyakh Jan 8, 2025

jsarha Jan 13, 2025

lyakh Jan 8, 2025

lyakh Jan 8, 2025

lyakh Jan 8, 2025

jsarha Jan 15, 2025

lgirdwood Jan 16, 2025

lgirdwood commented Jan 8, 2025

lyakh commented Jan 9, 2025

lgirdwood commented Jan 13, 2025

jsarha left a comment

jsarha Jan 13, 2025

jsarha Jan 13, 2025

jsarha Jan 15, 2025

jsarha commented Jan 15, 2025 •

edited

Loading


		#include <stddef.h>

		void fast_get(const void const dram_ptr, size_t size);

Dram data storage: fast_get()/_put() #9765

Are you sure you want to change the base?

Dram data storage: fast_get()/_put() #9765

Conversation

jsarha commented Jan 7, 2025

singalsu left a comment

Choose a reason for hiding this comment

jsarha commented Jan 7, 2025 • edited Loading

singalsu commented Jan 7, 2025

jsarha commented Jan 8, 2025

lyakh left a comment

Choose a reason for hiding this comment

lyakh Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented Jan 8, 2025

lyakh commented Jan 9, 2025

lgirdwood commented Jan 13, 2025

jsarha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsarha commented Jan 15, 2025 • edited Loading

jsarha commented Jan 7, 2025 •

edited

Loading

lyakh Jan 8, 2025 •

edited

Loading

jsarha commented Jan 15, 2025 •

edited

Loading