buf: remove coherent.h from struct comp_buffer

marcinszkudlinski · kv2019i · commit 4a036999118c · 2023-09-11T11:10:14.000+03:00
This commit removes coherent.h from struct comp_buffer

As sharing status of a buffer is known at creation time, it is
enough to create a buffer in shared (not cached mem alias)
when it will be used by several cores

Signed-off-by: Marcin Szkudlinski &lt;marcin.szkudlinski@intel.com&gt;
diff --git a/src/audio/buffer.c b/src/audio/buffer.c
@@ -27,7 +27,8 @@ DECLARE_SOF_RT_UUID("buffer", buffer_uuid, 0x42544c92, 0x8e92, 0x4e41,
 		 0xb6, 0x79, 0x34, 0x51, 0x9f, 0x1c, 0x1d, 0x28);
 DECLARE_TR_CTX(buffer_tr, SOF_UUID(buffer_uuid), LOG_LEVEL_INFO);
 
-struct comp_buffer *buffer_alloc(uint32_t size, uint32_t caps, uint32_t flags, uint32_t align)
+struct comp_buffer *buffer_alloc(uint32_t size, uint32_t caps, uint32_t flags, uint32_t align,
+				 bool is_shared)
 {
 	struct comp_buffer *buffer;
 	struct comp_buffer __sparse_cache *buffer_c;
@@ -42,16 +43,17 @@ struct comp_buffer *buffer_alloc(uint32_t size, uint32_t caps, uint32_t flags, u
 		return NULL;
 	}
 
-	/*
-	 * allocate new buffer, align the allocation size to a cache line for
-	 * the coherent API
-	 */
-	buffer = coherent_init_thread(struct comp_buffer, c);
+	/* allocate new buffer	 */
+	enum mem_zone zone = is_shared ? SOF_MEM_ZONE_RUNTIME_SHARED : SOF_MEM_ZONE_RUNTIME;
+
+	buffer = rzalloc(zone, 0, SOF_MEM_CAPS_RAM, sizeof(*buffer));
+
 	if (!buffer) {
 		tr_err(&buffer_tr, "buffer_alloc(): could not alloc structure");
 		return NULL;
 	}
 
+	buffer->is_shared = is_shared;
 	stream_addr = rballoc_align(0, caps, size, align);
 	if (!stream_addr) {
 		rfree(buffer);
@@ -70,18 +72,6 @@ struct comp_buffer *buffer_alloc(uint32_t size, uint32_t caps, uint32_t flags, u
 
 	buffer_release(buffer_c);
 
-	/*
-	 * The buffer hasn't yet been marked as shared, hence buffer_release()
-	 * hasn't written back and invalidated the cache. Therefore we have to
-	 * do this manually now before adding to the lists. Buffer list
-	 * structures are always accessed uncached and they're never modified at
-	 * run-time, i.e. buffers are never relinked. So we have to make sure,
-	 * that what we have written into buffer's cache is in RAM before
-	 * modifying that RAM bypassing cache, and that after this cache is
-	 * re-loaded again.
-	 */
-	dcache_writeback_invalidate_region(uncache_to_cache(buffer), sizeof(*buffer));
-
 	list_init(&buffer->source_list);
 	list_init(&buffer->sink_list);
 
@@ -303,30 +293,7 @@ void comp_update_buffer_consume(struct comp_buffer __sparse_cache *buffer, uint3
 void buffer_attach(struct comp_buffer *buffer, struct list_item *head, int dir)
 {
 	struct list_item *list = buffer_comp_list(buffer, dir);
-	struct list_item __sparse_cache *needs_sync;
-	bool further_buffers_exist;
-
-	/*
-	 * There can already be buffers on the target list. If we just link this
-	 * buffer, we modify the first buffer's list header via uncached alias,
-	 * so its cached copy can later be written back, overwriting the
-	 * modified header. FIXME: this is still a problem with different cores.
-	 */
-	further_buffers_exist = !list_is_empty(head);
-	needs_sync = uncache_to_cache(head->next);
-	if (further_buffers_exist)
-		dcache_writeback_region(needs_sync, sizeof(struct list_item));
-	/* The cache line can be prefetched here, invalidate it after prepending */
 	list_item_prepend(list, head);
-	if (further_buffers_exist)
-		dcache_invalidate_region(needs_sync, sizeof(struct list_item));
-#if CONFIG_INTEL
-	/*
-	 * Until now the buffer object wasn't in cache, but uncached access to it could have
-	 * triggered a cache prefetch. Drop that cache line to avoid using stale data in it.
-	 */
-	dcache_invalidate_region(uncache_to_cache(list), sizeof(*list));
-#endif
 }
 
 /*
@@ -335,32 +302,6 @@ void buffer_attach(struct comp_buffer *buffer, struct list_item *head, int dir)
  */
 void buffer_detach(struct comp_buffer *buffer, struct list_item *head, int dir)
 {
-	struct list_item __sparse_cache *needs_sync_prev, *needs_sync_next;
-	bool buffers_after_exist, buffers_before_exist;
 	struct list_item *buf_list = buffer_comp_list(buffer, dir);
-
-	/*
-	 * There can be more buffers linked together with this one, that will
-	 * still be staying on their respective pipelines and might get used via
-	 * their cached aliases. If we just unlink this buffer, we modify their
-	 * list header via uncached alias, so their cached copy can later be
-	 * written back, overwriting the modified header. FIXME: this is still a
-	 * problem with different cores.
-	 */
-	buffers_after_exist = head != buf_list->next;
-	buffers_before_exist = head != buf_list->prev;
-	needs_sync_prev = uncache_to_cache(buf_list->prev);
-	needs_sync_next = uncache_to_cache(buf_list->next);
-	if (buffers_after_exist)
-		dcache_writeback_region(needs_sync_next, sizeof(struct list_item));
-	if (buffers_before_exist)
-		dcache_writeback_region(needs_sync_prev, sizeof(struct list_item));
-	dcache_writeback_region(uncache_to_cache(buf_list), sizeof(*buf_list));
-	/* buffers before or after can be prefetched here */
 	list_item_del(buf_list);
-	dcache_invalidate_region(uncache_to_cache(buf_list), sizeof(*buf_list));
-	if (buffers_after_exist)
-		dcache_invalidate_region(needs_sync_next, sizeof(struct list_item));
-	if (buffers_before_exist)
-		dcache_invalidate_region(needs_sync_prev, sizeof(struct list_item));
 }
diff --git a/src/audio/chain_dma.c b/src/audio/chain_dma.c
@@ -588,8 +588,8 @@ static int chain_task_init(struct comp_dev *dev, uint8_t host_dma_id, uint8_t li
 	}
 
 	fifo_size = ALIGN_UP_INTERNAL(fifo_size, addr_align);
-
-	cd->dma_buffer = buffer_alloc(fifo_size, SOF_MEM_CAPS_DMA, 0, addr_align);
+	/* allocate not shared buffer */
+	cd->dma_buffer = buffer_alloc(fifo_size, SOF_MEM_CAPS_DMA, 0, addr_align, false);
 
 	if (!cd->dma_buffer) {
 		comp_err(dev, "chain_task_init(): failed to alloc dma buffer");
diff --git a/src/audio/copier/copier_generic.c b/src/audio/copier/copier_generic.c
@@ -204,8 +204,8 @@ int create_endpoint_buffer(struct comp_dev *dev,
 	ipc_buf.size = buf_size;
 	ipc_buf.comp.pipeline_id = config->pipeline_id;
 	ipc_buf.comp.core = config->core;
-
-	buffer = buffer_new(&ipc_buf);
+	/* allocate not shared buffer */
+	buffer = buffer_new(&ipc_buf, false);
 	if (!buffer)
 		return -ENOMEM;
 
diff --git a/src/audio/dai-legacy.c b/src/audio/dai-legacy.c
@@ -600,7 +600,7 @@ int dai_common_params(struct dai_data *dd, struct comp_dev *dev,
 		}
 	} else {
 		dd->dma_buffer = buffer_alloc(buffer_size, SOF_MEM_CAPS_DMA, 0,
-					      addr_align);
+					      addr_align, false);
 		if (!dd->dma_buffer) {
 			comp_err(dev, "dai_params(): failed to alloc dma buffer");
 			return -ENOMEM;
diff --git a/src/audio/dai-zephyr.c b/src/audio/dai-zephyr.c
@@ -977,8 +977,9 @@ int dai_common_params(struct dai_data *dd, struct comp_dev *dev,
 			return err;
 		}
 	} else {
+		/* allocate not shared buffer */
 		dd->dma_buffer = buffer_alloc(buffer_size, SOF_MEM_CAPS_DMA, 0,
-					      addr_align);
+					      addr_align, false);
 		if (!dd->dma_buffer) {
 			comp_err(dev, "dai_common_params(): failed to alloc dma buffer");
 			return -ENOMEM;
diff --git a/src/audio/host-legacy.c b/src/audio/host-legacy.c
@@ -788,7 +788,7 @@ int host_common_params(struct host_data *hd, struct comp_dev *dev,
 		}
 	} else {
 		hd->dma_buffer = buffer_alloc(buffer_size, SOF_MEM_CAPS_DMA, 0,
-					      addr_align);
+					      addr_align, false);
 		if (!hd->dma_buffer) {
 			comp_err(dev, "host_params(): failed to alloc dma buffer");
 			err = -ENOMEM;
diff --git a/src/audio/host-zephyr.c b/src/audio/host-zephyr.c
@@ -857,8 +857,9 @@ int host_common_params(struct host_data *hd, struct comp_dev *dev,
 			goto out;
 		}
 	} else {
+		/* allocate not shared buffer */
 		hd->dma_buffer = buffer_alloc(buffer_size, SOF_MEM_CAPS_DMA, 0,
-					      addr_align);
+					      addr_align, false);
 		if (!hd->dma_buffer) {
 			comp_err(dev, "host_params(): failed to alloc dma buffer");
 			err = -ENOMEM;
diff --git a/src/audio/module_adapter/module_adapter.c b/src/audio/module_adapter/module_adapter.c
@@ -448,8 +448,9 @@ int module_adapter_prepare(struct comp_dev *dev)
 	/* allocate buffer for all sinks */
 	if (list_is_empty(&mod->sink_buffer_list)) {
 		for (i = 0; i < mod->num_output_buffers; i++) {
+			/* allocate not shared buffer */
 			struct comp_buffer *buffer = buffer_alloc(buff_size, SOF_MEM_CAPS_RAM,
-								  0, PLATFORM_DCACHE_ALIGN);
+								  0, PLATFORM_DCACHE_ALIGN, false);
 			uint32_t flags;
 
 			if (!buffer) {
diff --git a/src/audio/pipeline/pipeline-graph.c b/src/audio/pipeline/pipeline-graph.c
@@ -176,10 +176,6 @@ static void buffer_set_comp(struct comp_buffer *buffer, struct comp_dev *comp,
 		buffer_c->sink = comp;
 
 	buffer_release(buffer_c);
-
-	/* The buffer might be marked as shared later, write back the cache */
-	if (!buffer->c.shared)
-		dcache_writeback_invalidate_region(uncache_to_cache(buffer), sizeof(*buffer));
 }
 
 int pipeline_connect(struct comp_dev *comp, struct comp_buffer *buffer,
diff --git a/src/include/sof/audio/buffer.h b/src/include/sof/audio/buffer.h
@@ -133,8 +133,6 @@ extern struct tr_ctx buffer_tr;
  * 5) write back cached data and release lock using uncache pointer.
  */
 struct comp_buffer {
-	struct coherent c;
-
 	/* data buffer */
 	struct audio_stream stream;
 
@@ -144,6 +142,7 @@ struct comp_buffer {
 	uint32_t caps;
 	uint32_t core;
 	struct tr_ctx tctx;			/* trace settings */
+	bool is_shared;			/* buffer structure is shared between 2 cores */
 
 	/* connected components */
 	struct comp_dev *source;	/* source component */
@@ -188,8 +187,9 @@ struct buffer_cb_free {
 	} while (0)
 
 /* pipeline buffer creation and destruction */
-struct comp_buffer *buffer_alloc(uint32_t size, uint32_t caps, uint32_t flags, uint32_t align);
-struct comp_buffer *buffer_new(const struct sof_ipc_buffer *desc);
+struct comp_buffer *buffer_alloc(uint32_t size, uint32_t caps, uint32_t flags, uint32_t align,
+				 bool is_shared);
+struct comp_buffer *buffer_new(const struct sof_ipc_buffer *desc, bool is_shared);
 int buffer_set_size(struct comp_buffer __sparse_cache *buffer, uint32_t size, uint32_t alignment);
 void buffer_free(struct comp_buffer *buffer);
 void buffer_zero(struct comp_buffer __sparse_cache *buffer);
@@ -209,32 +209,27 @@ bool buffer_params_match(struct comp_buffer __sparse_cache *buffer,
 static inline void buffer_stream_invalidate(struct comp_buffer __sparse_cache *buffer,
 					    uint32_t bytes)
 {
-	if (!is_coherent_shared(buffer, c))
-		return;
-
-	audio_stream_invalidate(&buffer->stream, bytes);
+	if (buffer->is_shared)
+		audio_stream_invalidate(&buffer->stream, bytes);
 }
 
 static inline void buffer_stream_writeback(struct comp_buffer __sparse_cache *buffer,
 					   uint32_t bytes)
 {
-	if (!is_coherent_shared(buffer, c))
-		return;
-
-	audio_stream_writeback(&buffer->stream, bytes);
+	if (buffer->is_shared)
+		audio_stream_writeback(&buffer->stream, bytes);
 }
 
+/* stubs for acquire/release for compilation, to be removed at last step */
 __must_check static inline struct comp_buffer __sparse_cache *buffer_acquire(
 	struct comp_buffer *buffer)
 {
-	struct coherent __sparse_cache *c = coherent_acquire_thread(&buffer->c, sizeof(*buffer));
-
-	return attr_container_of(c, struct comp_buffer __sparse_cache, c, __sparse_cache);
+	return (struct comp_buffer __sparse_cache *)buffer;
 }
 
 static inline void buffer_release(struct comp_buffer __sparse_cache *buffer)
 {
-	coherent_release_thread(&buffer->c, sizeof(*buffer));
+	(void)buffer;
 }
 
 /*
diff --git a/src/ipc/ipc-helper.c b/src/ipc/ipc-helper.c
@@ -36,15 +36,16 @@
 LOG_MODULE_DECLARE(ipc, CONFIG_SOF_LOG_LEVEL);
 
 /* create a new component in the pipeline */
-struct comp_buffer *buffer_new(const struct sof_ipc_buffer *desc)
+struct comp_buffer *buffer_new(const struct sof_ipc_buffer *desc, bool is_shared)
 {
 	struct comp_buffer *buffer;
 
 	tr_info(&buffer_tr, "buffer new size 0x%x id %d.%d flags 0x%x",
 		desc->size, desc->comp.pipeline_id, desc->comp.id, desc->flags);
 
 	/* allocate buffer */
-	buffer = buffer_alloc(desc->size, desc->caps, desc->flags, PLATFORM_DCACHE_ALIGN);
+	buffer = buffer_alloc(desc->size, desc->caps, desc->flags, PLATFORM_DCACHE_ALIGN,
+			      is_shared);
 	if (buffer) {
 		buffer->id = desc->comp.id;
 		buffer->pipeline_id = desc->comp.pipeline_id;
@@ -174,8 +175,8 @@ int comp_buffer_connect(struct comp_dev *comp, uint32_t comp_core,
 {
 	/* check if it's a connection between cores */
 	if (buffer->core != comp_core) {
-		/* set the buffer as a coherent object */
-		coherent_shared_thread(buffer, c);
+		/* buffer must be shared */
+		assert(buffer->is_shared);
 
 		if (!comp->is_shared)
 			comp_make_shared(comp);
diff --git a/src/ipc/ipc3/helper.c b/src/ipc/ipc3/helper.c
@@ -454,7 +454,7 @@ int ipc_buffer_new(struct ipc *ipc, const struct sof_ipc_buffer *desc)
 	}
 
 	/* register buffer with pipeline */
-	buffer = buffer_new(desc);
+	buffer = buffer_new(desc, false);
 	if (!buffer) {
 		tr_err(&ipc_tr, "ipc_buffer_new(): buffer_new() failed");
 		return -ENOMEM;
@@ -572,6 +572,12 @@ static int ipc_comp_to_buffer_connect(struct ipc_comp_dev *comp,
 	tr_dbg(&ipc_tr, "ipc: comp sink %d, source %d -> connect", buffer->id,
 	       comp->id);
 
+#if CONFIG_INCOHERENT
+	if (comp->core != buffer->cb->core) {
+		tr_err(&ipc_tr, "ipc: shared buffers are not supported for IPC3 incoherent architectures");
+		return -ENOTSUP;
+	}
+#endif
 	return comp_buffer_connect(comp->cd, comp->core, buffer->cb,
 				   PPL_CONN_DIR_COMP_TO_BUFFER);
 }
@@ -582,6 +588,12 @@ static int ipc_buffer_to_comp_connect(struct ipc_comp_dev *buffer,
 	tr_dbg(&ipc_tr, "ipc: comp sink %d, source %d -> connect", comp->id,
 	       buffer->id);
 
+#if CONFIG_INCOHERENT
+	if (comp->core != buffer->cb->core) {
+		tr_err(&ipc_tr, "ipc: shared buffers are not supported for IPC3 incoherent architectures");
+		return -ENOTSUP;
+	}
+#endif
 	return comp_buffer_connect(comp->cd, comp->core, buffer->cb,
 				   PPL_CONN_DIR_BUFFER_TO_COMP);
 }
diff --git a/src/ipc/ipc4/helper.c b/src/ipc/ipc4/helper.c
@@ -315,7 +315,7 @@ int ipc_pipeline_free(struct ipc *ipc, uint32_t comp_id)
 	return IPC4_SUCCESS;
 }
 
-static struct comp_buffer *ipc4_create_buffer(struct comp_dev *src, struct comp_dev *sink,
+static struct comp_buffer *ipc4_create_buffer(struct comp_dev *src, bool is_shared,
 					      uint32_t src_obs, uint32_t src_queue,
 					      uint32_t dst_queue)
 {
@@ -330,7 +330,7 @@ static struct comp_buffer *ipc4_create_buffer(struct comp_dev *src, struct comp_
 	ipc_buf.comp.id = IPC4_COMP_ID(src_queue, dst_queue);
 	ipc_buf.comp.pipeline_id = src->ipc_config.pipeline_id;
 	ipc_buf.comp.core = src->ipc_config.core;
-	return buffer_new(&ipc_buf);
+	return buffer_new(&ipc_buf, is_shared);
 }
 
 int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect)
@@ -344,6 +344,7 @@ int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect)
 	struct ipc4_base_module_cfg sink_src_cfg;
 	uint32_t flags;
 	int src_id, sink_id;
+	bool is_shared;
 	int ret;
 
 	bu = (struct ipc4_module_bind_unbind *)_connect;
@@ -360,6 +361,7 @@ int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect)
 	/* Pass IPC to target core if both modules has the same target core */
 	if (!cpu_is_me(source->ipc_config.core) && source->ipc_config.core == sink->ipc_config.core)
 		return ipc4_process_on_core(source->ipc_config.core, false);
+	is_shared = (source->ipc_config.core != sink->ipc_config.core);
 
 	ret = comp_get_attribute(source, COMP_ATTR_BASE_CONFIG, &source_src_cfg);
 	if (ret < 0) {
@@ -373,8 +375,8 @@ int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect)
 		return IPC4_FAILURE;
 	}
 
-	buffer = ipc4_create_buffer(source, sink, source_src_cfg.obs, bu->extension.r.src_queue,
-				    bu->extension.r.dst_queue);
+	buffer = ipc4_create_buffer(source, is_shared, source_src_cfg.obs,
+				    bu->extension.r.src_queue, bu->extension.r.dst_queue);
 	if (!buffer) {
 		tr_err(&ipc_tr, "failed to allocate buffer to bind %d to %d", src_id, sink_id);
 		return IPC4_OUT_OF_MEMORY;
diff --git a/test/cmocka/src/audio/buffer/buffer_copy.c b/test/cmocka/src/audio/buffer/buffer_copy.c
diff --git a/test/cmocka/src/audio/buffer/buffer_new.c b/test/cmocka/src/audio/buffer/buffer_new.c
diff --git a/test/cmocka/src/audio/buffer/buffer_wrap.c b/test/cmocka/src/audio/buffer/buffer_wrap.c
diff --git a/test/cmocka/src/audio/buffer/buffer_write.c b/test/cmocka/src/audio/buffer/buffer_write.c
diff --git a/test/cmocka/src/math/fft/fft.c b/test/cmocka/src/math/fft/fft.c
diff --git a/test/cmocka/src/util.h b/test/cmocka/src/util.h

Original file line number	Diff line number	Diff line change
`@@ -588,8 +588,8 @@ static int chain_task_init(struct comp_dev *dev, uint8_t host_dma_id, uint8_t li`
`588`	`588`	`}`
`589`	`589`
`590`	`590`	`fifo_size = ALIGN_UP_INTERNAL(fifo_size, addr_align);`
`591`		`-`
`592`		`- cd->dma_buffer = buffer_alloc(fifo_size, SOF_MEM_CAPS_DMA, 0, addr_align);`
	`591`	`+ /* allocate not shared buffer */`
	`592`	`+ cd->dma_buffer = buffer_alloc(fifo_size, SOF_MEM_CAPS_DMA, 0, addr_align, false);`
`593`	`593`
`594`	`594`	`if (!cd->dma_buffer) {`
`595`	`595`	`comp_err(dev, "chain_task_init(): failed to alloc dma buffer");`
Original file line number	Diff line number	Diff line change
`@@ -600,7 +600,7 @@ int dai_common_params(struct dai_data dd, struct comp_dev dev,`
`600`	`600`	`}`
`601`	`601`	`} else {`
`602`	`602`	`dd->dma_buffer = buffer_alloc(buffer_size, SOF_MEM_CAPS_DMA, 0,`
`603`		`- addr_align);`
	`603`	`+ addr_align, false);`
`604`	`604`	`if (!dd->dma_buffer) {`
`605`	`605`	`comp_err(dev, "dai_params(): failed to alloc dma buffer");`
`606`	`606`	`return -ENOMEM;`
Original file line number	Diff line number	Diff line change
`@@ -788,7 +788,7 @@ int host_common_params(struct host_data hd, struct comp_dev dev,`
`788`	`788`	`}`
`789`	`789`	`} else {`
`790`	`790`	`hd->dma_buffer = buffer_alloc(buffer_size, SOF_MEM_CAPS_DMA, 0,`
`791`		`- addr_align);`
	`791`	`+ addr_align, false);`
`792`	`792`	`if (!hd->dma_buffer) {`
`793`	`793`	`comp_err(dev, "host_params(): failed to alloc dma buffer");`
`794`	`794`	`err = -ENOMEM;`
Original file line number	Diff line number	Diff line change
`@@ -176,10 +176,6 @@ static void buffer_set_comp(struct comp_buffer buffer, struct comp_dev comp,`
`176`	`176`	`buffer_c->sink = comp;`
`177`	`177`
`178`	`178`	`buffer_release(buffer_c);`
`179`		`-`
`180`		`- /* The buffer might be marked as shared later, write back the cache */`
`181`		`- if (!buffer->c.shared)`
`182`		`- dcache_writeback_invalidate_region(uncache_to_cache(buffer), sizeof(*buffer));`
`183`	`179`	`}`
`184`	`180`
`185`	`181`	`int pipeline_connect(struct comp_dev comp, struct comp_buffer buffer,`