Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove sparse step1 #8166

Merged
merged 3 commits into from
Sep 11, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 8 additions & 67 deletions src/audio/buffer.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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);

Expand Down Expand Up @@ -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
}

/*
Expand All @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

head is now unneeded, can be removed later

{
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));
}
4 changes: 2 additions & 2 deletions src/audio/chain_dma.c
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
4 changes: 2 additions & 2 deletions src/audio/copier/copier_generic.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 1 addition & 1 deletion src/audio/dai-legacy.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion src/audio/dai-zephyr.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/audio/host-legacy.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion src/audio/host-zephyr.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion src/audio/module_adapter/module_adapter.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 0 additions & 4 deletions src/audio/pipeline/pipeline-graph.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
27 changes: 11 additions & 16 deletions src/include/sof/audio/buffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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 */
marcinszkudlinski marked this conversation as resolved.
Show resolved Hide resolved

/* connected components */
struct comp_dev *source; /* source component */
Expand Down Expand Up @@ -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);
Expand All @@ -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;
}

/*
Expand Down
9 changes: 5 additions & 4 deletions src/ipc/ipc-helper.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this recoverable ? I assume this data is derived from topology so could a bad topology cause an assert here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not recoverable, must not happen.
If happens, a cached alias will be used for shared memory and system will crash later

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but here at least you have a perfect chance to error out, don't you?


if (!comp->is_shared)
comp_make_shared(comp);
Expand Down
14 changes: 13 additions & 1 deletion src/ipc/ipc3/helper.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}
Expand Down
10 changes: 6 additions & 4 deletions src/ipc/ipc4/helper.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -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)
Expand All @@ -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;
Expand All @@ -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) {
Expand All @@ -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;
Expand Down
Loading