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.

Sign up for GitHub

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

dma-trace: Full support for re-configuration and make dmatb persistent from the time it is allocated #5106

Merged
merged 9 commits into from
Feb 18, 2022
Merged

dma-trace: Full support for re-configuration and make dmatb persistent from the time it is allocated #5106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3e1245e
dma-trace: Simply return with if dma_trace_buffer_init() fails
ujfalusi Oct 15, 2021
9aaa7dd
dma-trace: Use mtrace_printf() if dtrace buffer allocation fails
ujfalusi Oct 15, 2021
9fe90d8
dma-trace: Move the first trace printing within dma_trace_buffer_init()
ujfalusi Dec 17, 2021
b667426
dma-trace: Cancel the dtrace task before stopping the channel in reco…
ujfalusi Oct 15, 2021
c035084
ipc: ipc3: handler: Fix error handling in ipc_dma_trace_config()
ujfalusi Feb 17, 2022
7cef039
dma-trace: Fix memory leak with buffer allocation on dtrace re-config
ujfalusi Oct 15, 2021
11cbb52
dma-trace: Fix memory leak of host elem_array on reconfiguration
ujfalusi Feb 17, 2022
437100d
dma-trace: dma_sg_free() can not be used to free SOF_MEM_ZONE_SYS all…
ujfalusi Dec 17, 2021
8a051c2
dma-trace: Make dmatb and DMA_GW dma_sg persistent after first alloca…
ujfalusi Dec 17, 2021
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
3 changes: 3 additions & 0 deletions src/include/sof/trace/dma-trace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ struct dma_trace_buf {
struct dma_trace_data {
struct dma_sg_config config;
struct dma_trace_buf dmatb;
#if CONFIG_DMA_GW
struct dma_sg_config gw_config;
#endif
struct dma_copy dc;
struct sof_ipc_dma_trace_posn posn;
struct ipc_msg *msg;
Expand Down
8 changes: 7 additions & 1 deletion src/ipc/ipc3/handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -824,7 +824,7 @@ static int ipc_dma_trace_config(uint32_t header)
&elem_array,
&ring_size);
if (err < 0)
goto error;
goto processing_error;

err = dma_trace_host_buffer(dmat, &elem_array, ring_size);
if (err < 0) {
Expand All @@ -849,6 +849,12 @@ static int ipc_dma_trace_config(uint32_t header)
return 0;

error:
#if CONFIG_HOST_PTABLE
dma_sg_free(&elem_array);

processing_error:
#endif

return err;
}
#endif /* CONFIG_SUECREEK */
Expand Down
170 changes: 98 additions & 72 deletions src/trace/dma-trace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ static enum task_state trace_work(void *data)
int32_t size;
uint32_t overflow;

/* The host DMA channel is not available */
if (!d->dc.chan)
return SOF_TASK_STATE_RESCHEDULE;

/* make sure we don't write more than buffer */
if (avail > DMA_TRACE_LOCAL_SIZE) {
overflow = avail - DMA_TRACE_LOCAL_SIZE;
Expand Down Expand Up @@ -218,14 +222,43 @@ int dma_trace_host_buffer(struct dma_trace_data *d,
}
#endif

static void dma_trace_buffer_free(struct dma_trace_data *d)
{
struct dma_trace_buf *buffer = &d->dmatb;
k_spinlock_key_t key;

key = k_spin_lock(&d->lock);

rfree(buffer->addr);
memset(buffer, 0, sizeof(*buffer));
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this needed? I see, that it is just moved from another location, just wondering if we know why we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is to reset the variables within buffer.


k_spin_unlock(&d->lock, key);
}

static int dma_trace_buffer_init(struct dma_trace_data *d)
{
#if CONFIG_DMA_GW
struct dma_sg_config *config = &d->gw_config;
uint32_t elem_size, elem_addr, elem_num;
int ret;
#endif
struct dma_trace_buf *buffer = &d->dmatb;
void *buf;
k_spinlock_key_t key;
uint32_t addr_align;
int err;

/*
* Keep the existing dtrace buffer to avoid memory leak, unlikely to
* happen if host correctly using the dma_trace_disable().
*
* The buffer can not be freed up here as it is likely in use.
* The (re-)initialization will happen in dma_trace_start() when it is
* safe to do (the DMA is stopped)
*/
if (dma_trace_initialized(d))
return 0;

if (!d || !d->dc.dmac) {
mtrace_printf(LOG_LEVEL_ERROR,
"%s failed: no DMAC!", __func__);
Expand All @@ -241,7 +274,7 @@ static int dma_trace_buffer_init(struct dma_trace_data *d)
buf = rballoc_align(0, SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_DMA,
DMA_TRACE_LOCAL_SIZE, addr_align);
if (!buf) {
tr_err(&dt_tr, "dma_trace_buffer_init(): alloc failed");
mtrace_printf(LOG_LEVEL_ERROR, "dma_trace_buffer_init(): alloc failed");
return -ENOMEM;
}

Expand All @@ -260,28 +293,62 @@ static int dma_trace_buffer_init(struct dma_trace_data *d)

k_spin_unlock(&d->lock, key);

return 0;
}
#if CONFIG_DMA_GW
/* size of every trace record */
elem_size = sizeof(uint64_t) * 2;

static void dma_trace_buffer_free(struct dma_trace_data *d)
{
struct dma_trace_buf *buffer = &d->dmatb;
k_spinlock_key_t key;
/* Initialize address of local elem */
elem_addr = (uint32_t)buffer->addr;

key = k_spin_lock(&d->lock);
/* the number of elem list */
elem_num = DMA_TRACE_LOCAL_SIZE / elem_size;

rfree(buffer->addr);
memset(buffer, 0, sizeof(*buffer));
config->direction = DMA_DIR_LMEM_TO_HMEM;
config->src_width = sizeof(uint32_t);
config->dest_width = sizeof(uint32_t);
config->cyclic = 0;

k_spin_unlock(&d->lock, key);
ret = dma_sg_alloc(&config->elem_array, SOF_MEM_ZONE_SYS,
config->direction, elem_num, elem_size,
elem_addr, 0);
if (ret < 0) {
dma_trace_buffer_free(d);
return ret;
}
#endif

#ifdef __ZEPHYR__
#define ZEPHYR_VER_OPT " zephyr:" META_QUOTE(BUILD_VERSION)
#else
#define ZEPHYR_VER_OPT
#endif

/* META_QUOTE(SOF_SRC_HASH) is part of the format string so it
* goes to the .ldc file and does not go to the firmware
* binary. It will be different from SOF_SRC_HASH in case of
* mismatch.
*/
#define SOF_BANNER_COMMON \
"FW ABI 0x%x DBG ABI 0x%x tags SOF:" SOF_GIT_TAG ZEPHYR_VER_OPT \
" src hash 0x%08x (ldc hash " META_QUOTE(SOF_SRC_HASH) ")"

/* It should be the very first sent log for easy identification. */
mtrace_printf(LOG_LEVEL_INFO,
"SHM: " SOF_BANNER_COMMON,
SOF_ABI_VERSION, SOF_ABI_DBG_VERSION, SOF_SRC_HASH);

/* Use a different, DMA: prefix to ease identification of log files */
tr_info(&dt_tr,
"DMA: " SOF_BANNER_COMMON,
SOF_ABI_VERSION, SOF_ABI_DBG_VERSION, SOF_SRC_HASH);

return 0;
}

#if CONFIG_DMA_GW

static int dma_trace_start(struct dma_trace_data *d)
{
struct dma_sg_config config;
uint32_t elem_size, elem_addr, elem_num;
int err = 0;

/* DMA Controller initialization is platform-specific */
Expand All @@ -297,6 +364,7 @@ static int dma_trace_start(struct dma_trace_data *d)
"dma_trace_start(): DMA reconfiguration (active stream_tag: %u)",
d->active_stream_tag);

schedule_task_cancel(&d->dmat_work);
err = dma_stop(d->dc.chan);
if (err < 0) {
mtrace_printf(LOG_LEVEL_ERROR,
Expand All @@ -318,42 +386,23 @@ static int dma_trace_start(struct dma_trace_data *d)
if (err < 0)
return err;

d->active_stream_tag = d->stream_tag;

/* size of every trace record */
elem_size = sizeof(uint64_t) * 2;

/* Initialize address of local elem */
elem_addr = (uint32_t)d->dmatb.addr;

/* the number of elem list */
elem_num = DMA_TRACE_LOCAL_SIZE / elem_size;
/* Reset host buffer information as host is re-configuring dtrace */
d->old_host_offset = 0;
d->posn.host_offset = 0;

config.direction = DMA_DIR_LMEM_TO_HMEM;
config.src_width = sizeof(uint32_t);
config.dest_width = sizeof(uint32_t);
config.cyclic = 0;

err = dma_sg_alloc(&config.elem_array, SOF_MEM_ZONE_SYS,
config.direction,
elem_num, elem_size, elem_addr, 0);
if (err < 0)
goto err_alloc;
d->active_stream_tag = d->stream_tag;

err = dma_set_config(d->dc.chan, &config);
err = dma_set_config(d->dc.chan, &d->gw_config);
if (err < 0) {
mtrace_printf(LOG_LEVEL_ERROR, "dma_set_config() failed: %d", err);
goto err_config;
goto error;
}

err = dma_start(d->dc.chan);
if (err == 0)
return 0;

err_config:
dma_sg_free(&config.elem_array);

err_alloc:
error:
dma_channel_put(d->dc.chan);
d->dc.chan = NULL;

Expand Down Expand Up @@ -420,38 +469,10 @@ int dma_trace_enable(struct dma_trace_data *d)
{
int err;

/* initialize dma trace buffer */
/* Allocate and initialize the dma trace buffer if needed */
err = dma_trace_buffer_init(d);

if (err < 0) {
mtrace_printf(LOG_LEVEL_ERROR, "dma_trace_enable: buffer_init failed");
goto out;
}

#ifdef __ZEPHYR__
#define ZEPHYR_VER_OPT " zephyr:" META_QUOTE(BUILD_VERSION)
#else
#define ZEPHYR_VER_OPT
#endif

/* META_QUOTE(SOF_SRC_HASH) is part of the format string so it
* goes to the .ldc file and does not go to the firmware
* binary. It will be different from SOF_SRC_HASH in case of
* mismatch.
*/
#define SOF_BANNER_COMMON \
"FW ABI 0x%x DBG ABI 0x%x tags SOF:" SOF_GIT_TAG ZEPHYR_VER_OPT \
" src hash 0x%08x (ldc hash " META_QUOTE(SOF_SRC_HASH) ")"

/* It should be the very first sent log for easy identification. */
mtrace_printf(LOG_LEVEL_INFO,
"SHM: " SOF_BANNER_COMMON,
SOF_ABI_VERSION, SOF_ABI_DBG_VERSION, SOF_SRC_HASH);

/* Use a different, DMA: prefix to ease identification of log files */
tr_info(&dt_tr,
"DMA: " SOF_BANNER_COMMON,
SOF_ABI_VERSION, SOF_ABI_DBG_VERSION, SOF_SRC_HASH);
if (err < 0)
return err;

#if CONFIG_DMA_GW
/*
Expand Down Expand Up @@ -491,8 +512,13 @@ void dma_trace_disable(struct dma_trace_data *d)
d->dc.chan = NULL;
}

/* free trace buffer */
dma_trace_buffer_free(d);
#if (CONFIG_HOST_PTABLE)
/* Free up the host SG if it is set */
if (d->host_size) {
dma_sg_free(&d->config.elem_array);
d->host_size = 0;
}
#endif
}

/** Sends all pending DMA messages to mailbox (for emergencies) */
Expand Down