From bd220b212fde1e6771dcff5ee9de23d6eb1e8f08 Mon Sep 17 00:00:00 2001 From: Liam Girdwood Date: Tue, 19 Dec 2017 16:45:44 +0000 Subject: [PATCH 01/10] lock: Add deadlock detection. Add a debug option that can detect deadlock and panic(). The deadlock detection attempts to acquire a lock several times before giving up and causing a panic() taht dumps the deadlock details. Signed-off-by: Liam Girdwwod --- src/include/reef/debug.h | 1 + src/include/reef/lock.h | 71 +++++++++++++++++++++++++++++++--------- 2 files changed, 57 insertions(+), 15 deletions(-) diff --git a/src/include/reef/debug.h b/src/include/reef/debug.h index 70a78022f..7551fc5cb 100644 --- a/src/include/reef/debug.h +++ b/src/include/reef/debug.h @@ -45,6 +45,7 @@ #define PANIC_PLATFORM 4 #define PANIC_TASK 5 #define PANIC_EXCEPTION 6 +#define PANIC_DEADLOCK 7 #define DEBUG diff --git a/src/include/reef/lock.h b/src/include/reef/lock.h index deb0fb7c2..fd415aed5 100644 --- a/src/include/reef/lock.h +++ b/src/include/reef/lock.h @@ -35,6 +35,7 @@ #define __INCLUDE_LOCK__ #define DEBUG_LOCKS 0 +#define DEBUG_LOCKS_VERBOSE 0 #include #include @@ -62,19 +63,33 @@ * grep -rn lock --include *.c | grep 439 * src/lib/alloc.c:439: spin_lock_irq(&memmap.lock, flags); * - * Every lock entry and exit shows LcE and LcX in trace alonside the lock + * Every lock entry and exit shows LcE and LcX in trace alongside the lock * line numbers in hex. e.g. * * 0xfd60 [11032.730567] delta [0.000004] lock LcE * 0xfd70 [11032.730569] delta [0.000002] value 0x00000000000000ae * - * Deadlock would be a LcE without a subsequent LcX. + * Deadlock can be confirmed in rmbox :- * + * Debug log: + * debug: 0x0 (00) = 0xdead0007 (-559087609) |....| + * .... + * Error log: + * using 19.20MHz timestamp clock + * 0xc30 [26.247240] delta [26.245851] lock DED + * 0xc40 [26.247242] delta [0.000002] value 0x00000000000002b4 + * 0xc50 [26.247244] delta [0.000002] value 0x0000000000000109 + * + * DED means deadlock has been detected and the DSP is now halted. The first + * value after DEA is the line number where deadlock occurs and the second + * number is the line number where the lock is allocated. These can be grepped + * like above. */ #if DEBUG_LOCKS #define DBG_LOCK_USERS 8 +#define DBG_LOCK_TRIES 10000 #define trace_lock(__e) trace_error_atomic(TRACE_CLASS_LOCK, __e) #define tracev_lock(__e) tracev_event_atomic(TRACE_CLASS_LOCK, __e) @@ -84,22 +99,29 @@ extern uint32_t lock_dbg_atomic; extern uint32_t lock_dbg_user[DBG_LOCK_USERS]; -#define spin_lock_dbg() \ - trace_lock("LcE"); \ - trace_lock_value(__LINE__); - -#define spin_unlock_dbg() \ - trace_lock("LcX"); \ - trace_lock_value(__LINE__); \ - /* all SMP spinlocks need init, nothing todo on UP */ #define spinlock_init(lock) \ arch_spinlock_init(lock); \ (lock)->user = __LINE__; -/* does nothing on UP systems */ -#define spin_lock(lock) \ - spin_lock_dbg(); \ +/* panic on deadlock */ +#define spin_try_lock_dbg(lock) \ + do { \ + int __tries; \ + for (__tries = DBG_LOCK_TRIES; __tries > 0; __tries--) { \ + if (arch_try_lock(lock)) \ + break; /* lock acquired */ \ + } \ + if (__tries == 0) { \ + trace_lock_error("DED"); \ + trace_lock_value(__LINE__); \ + trace_lock_value((lock)->user); \ + panic(PANIC_DEADLOCK); /* lock not acquired */ \ + } \ + } while (0); + +#if DEBUG_LOCKS_VERBOSE +#define spin_lock_log(lock) \ if (lock_dbg_atomic) { \ int __i = 0; \ int __count = lock_dbg_atomic >= DBG_LOCK_USERS \ @@ -111,8 +133,27 @@ extern uint32_t lock_dbg_user[DBG_LOCK_USERS]; trace_lock_value((lock_dbg_atomic << 24) | \ lock_dbg_user[__i]); \ } \ - } \ - arch_spin_lock(lock); + } + +#define spin_lock_dbg() \ + trace_lock("LcE"); \ + trace_lock_value(__LINE__); + +#define spin_unlock_dbg() \ + trace_lock("LcX"); \ + trace_lock_value(__LINE__); + +#else +#define spin_lock_log(lock) +#define spin_lock_dbg() +#define spin_unlock_dbg() +#endif + +/* does nothing on UP systems */ +#define spin_lock(lock) \ + spin_lock_dbg(); \ + spin_lock_log(lock); \ + spin_try_lock_dbg(lock); #define spin_unlock(lock) \ arch_spin_unlock(lock); \ From b7cd6ac667d9cb37fe587a145c1ced6701f5fae2 Mon Sep 17 00:00:00 2001 From: Yan Wang Date: Wed, 20 Dec 2017 15:45:27 +0800 Subject: [PATCH 02/10] Change type of size in trace_work() callback from uint32_t to int32_t. This variable is set by the return value of dma_copy_to_host_nowait(). Unsigned type will mislead error checking. Signed-off-by: Yan Wang --- src/lib/dma-trace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/dma-trace.c b/src/lib/dma-trace.c index ea63ab481..1fbd050f6 100644 --- a/src/lib/dma-trace.c +++ b/src/lib/dma-trace.c @@ -48,7 +48,7 @@ static uint64_t trace_work(void *data, uint64_t delay) struct dma_sg_config *config = &d->config; unsigned long flags; uint32_t avail = buffer->avail; - uint32_t size; + int32_t size; uint32_t hsize; uint32_t lsize; From ece72b5e1aeda160ab456db0622e85341e840152 Mon Sep 17 00:00:00 2001 From: Seppo Ingalsuo Date: Wed, 20 Dec 2017 12:01:17 +0200 Subject: [PATCH 03/10] Volume: Code cleanup for minimum gain value limiting This patch changes the condition for limiting the smallest gain to eliminate a never executed code part when VOL_MIN is defined as zero. The variable v is unsigned integer so less than zero is not possible. The functionality is not modified by this patch. Signed-off-by: Seppo Ingalsuo --- src/audio/volume.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/audio/volume.c b/src/audio/volume.c index 6130563b0..3b3e2aaa2 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -427,7 +427,7 @@ static inline void volume_set_chan(struct comp_dev *dev, int chan, uint32_t vol) * multiplication overflow with the 32 bit value. Non-zero MIN option * can be useful to prevent totally muted small volume gain. */ - if (v < VOL_MIN) + if (v <= VOL_MIN) v = VOL_MIN; if (v > VOL_MAX) From b7bcd470852c49855c7f1c512fb43c921dc03f8e Mon Sep 17 00:00:00 2001 From: Liam Girdwood Date: Thu, 21 Dec 2017 16:14:58 +0000 Subject: [PATCH 04/10] baytrail: panic: fix panic on baytrail. Simplify and fix panic output on baytrail. Signed-off-by: Liam Girdwood --- src/platform/baytrail/include/platform/platform.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/platform/baytrail/include/platform/platform.h b/src/platform/baytrail/include/platform/platform.h index 72174f33c..dca0c47fd 100644 --- a/src/platform/baytrail/include/platform/platform.h +++ b/src/platform/baytrail/include/platform/platform.h @@ -93,13 +93,11 @@ struct reef; /* Platform defined panic code */ #define platform_panic(__x) \ - shim_write(SHIM_IPCXL, ((shim_read(SHIM_IPCXL) & 0xc0000000) |\ - ((0xdead000 | __x) & 0x3fffffff))) + shim_write(SHIM_IPCDH, (0xdead000 | (__x & 0xfff))) /* Platform defined trace code */ #define platform_trace_point(__x) \ - shim_write(SHIM_IPCDH, ((shim_read(SHIM_IPCDH) & 0xc0000000) |\ - ((__x) & 0x3fffffff))) + shim_write(SHIM_IPCDH, (__x & 0x3fffffff)) /* * APIs declared here are defined for every platform and IPC mechanism. */ From cb6c5fd669f61c1cf42928c96d71ea9d3ed8a7e9 Mon Sep 17 00:00:00 2001 From: Liam Girdwood Date: Thu, 21 Dec 2017 16:19:52 +0000 Subject: [PATCH 05/10] platform: Add a platform workQ clock source macro Add this macro at platform level so it can be used by others. Signed-off-by: Liam Girdwood --- src/platform/baytrail/include/platform/platform.h | 3 +++ src/platform/baytrail/platform.c | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/platform/baytrail/include/platform/platform.h b/src/platform/baytrail/include/platform/platform.h index dca0c47fd..bc905642c 100644 --- a/src/platform/baytrail/include/platform/platform.h +++ b/src/platform/baytrail/include/platform/platform.h @@ -76,6 +76,9 @@ struct reef; /* WorkQ window size in microseconds */ #define PLATFORM_WORKQ_WINDOW 2000 +/* platform WorkQ clock */ +#define PLATFORM_WORKQ_CLOCK CLK_SSP + /* local buffer size of DMA tracing */ #define DMA_TRACE_LOCAL_SIZE HOST_PAGE_SIZE diff --git a/src/platform/baytrail/platform.c b/src/platform/baytrail/platform.c index cd884a81c..5cda2dac4 100644 --- a/src/platform/baytrail/platform.c +++ b/src/platform/baytrail/platform.c @@ -78,7 +78,7 @@ static struct work_queue_timesource platform_generic_queue = { .id = TIMER3, /* external timer */ .irq = IRQ_NUM_EXT_TIMER, }, - .clk = CLK_SSP, + .clk = PLATFORM_WORKQ_CLOCK, .notifier = NOTIFIER_ID_SSP_FREQ, .timer_set = platform_timer_set, .timer_clear = platform_timer_clear, From bc73899f9a628f0c6c7a4ab5fe77a7e085ee8a29 Mon Sep 17 00:00:00 2001 From: Liam Girdwood Date: Thu, 21 Dec 2017 16:50:18 +0000 Subject: [PATCH 06/10] arch: add API call to get stack current pointer. Return the current stack pointer. Signed-off-by: Liam Girdwood --- src/arch/xtensa/include/arch/reef.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/arch/xtensa/include/arch/reef.h b/src/arch/xtensa/include/arch/reef.h index ef2f395f7..149de5d31 100644 --- a/src/arch/xtensa/include/arch/reef.h +++ b/src/arch/xtensa/include/arch/reef.h @@ -40,4 +40,16 @@ void *xthal_memcpy(void *dst, const void *src, size_t len); #define arch_memcpy(dest, src, size) \ xthal_memcpy(dest, src, size) +static inline size_t arch_get_stack_ptr(void) +{ + size_t ptr; + + /* stack pointer is in a1 */ + __asm__ __volatile__ ("mov %0, a1" + : "=a" (ptr) + : + : "memory"); + return ptr; +} + #endif From 0c9e55b6da372ce16dedd2b7764e90fa2b9f3f13 Mon Sep 17 00:00:00 2001 From: Liam Girdwood Date: Thu, 21 Dec 2017 16:52:43 +0000 Subject: [PATCH 07/10] panic: fix panic_dump_stack() Fix so that dump stack does not overwrite any debug data and does not read past the end of the stack. Signed-off-by: Liam Girdwood --- src/include/reef/debug.h | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/include/reef/debug.h b/src/include/reef/debug.h index 7551fc5cb..85f91d421 100644 --- a/src/include/reef/debug.h +++ b/src/include/reef/debug.h @@ -46,6 +46,7 @@ #define PANIC_TASK 5 #define PANIC_EXCEPTION 6 #define PANIC_DEADLOCK 7 +#define PANIC_STACK 8 #define DEBUG @@ -144,26 +145,26 @@ } while (0); /* dump stack as part of panic */ -/* TODO: rework to make this generic - the stack top can be moved to arch code */ #define panic_dump_stack(_p) \ do { \ extern uint32_t __stack; \ extern uint32_t _stack_sentry; \ uint32_t _stack_bottom = (uint32_t)&__stack; \ uint32_t _stack_limit = (uint32_t)&_stack_sentry; \ - uint32_t _stack_top; \ - \ - __asm__ __volatile__ ("mov %0, a1" : "=a" (_stack_top) : : "memory"); \ + uint32_t _stack_top = arch_get_stack_ptr(); \ + uint32_t _size = _stack_bottom - _stack_top; \ + uint32_t _panic = _p; \ dbg_val(0xdead0000 | _p) \ - platform_panic(_p); \ - dbg_val(_stack_top) \ - dbg_val(_stack_bottom) \ - \ + dbg_val_at(_stack_top, 1) \ + dbg_val_at(_stack_bottom, 2) \ + /* is stack smashed ? */\ if (_stack_bottom <= _stack_limit) { \ - dbg_val(0x51ac0000 | _p) \ + dbg_val_at(0x51ac0000 | _p, 3); \ _stack_bottom = _stack_limit; \ + _panic = PANIC_STACK; \ } \ - dump(_stack_top, _stack_bottom - _stack_top) \ + platform_panic(_panic); \ + dump_at(_stack_top, (_size - sizeof(uint32_t)) >> 2, 4) \ \ while(1) {}; \ } while (0); From c14a4e97f147903fc6bb8d3656db78b57fb3c6d3 Mon Sep 17 00:00:00 2001 From: Liam Girdwood Date: Thu, 21 Dec 2017 22:09:13 +0000 Subject: [PATCH 08/10] comp: pause: Make sure dai/host state are preserved during pause/resume The DAI and host components states must be preserved during pause so that when normal pipeline positions are used on resume. i.e. pause just looks like a very long pipeline schedule. Signed-off-by: Liam Girdwood --- src/audio/dai.c | 10 ++++------ src/audio/host.c | 1 - 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/audio/dai.c b/src/audio/dai.c index 9058f5cbb..b7ad1b453 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -95,7 +95,6 @@ static void dai_dma_cb(void *data, uint32_t type, struct dma_sg_elem *next) /* inform waiters */ wait_completed(&dd->complete); - return; } /* is our pipeline handling an XRUN ? */ @@ -172,9 +171,8 @@ static void dai_dma_cb(void *data, uint32_t type, struct dma_sg_elem *next) } /* notify pipeline that DAI needs its buffer processed */ - pipeline_schedule_copy(dev->pipeline, 0); - - return; + if (dev->state == COMP_STATE_ACTIVE) + pipeline_schedule_copy(dev->pipeline, 0); } static struct comp_dev *dai_new(struct sof_ipc_comp *comp) @@ -539,10 +537,10 @@ static int dai_cmd(struct comp_dev *dev, int cmd, void *data) return ret; switch (cmd) { - case COMP_CMD_RELEASE: case COMP_CMD_START: - dai_pointer_init(dev); + /* fall through */ + case COMP_CMD_RELEASE: /* only start the DAI if we are not XRUN handling */ if (dd->xrun == 0) { diff --git a/src/audio/host.c b/src/audio/host.c index 8f09952ea..7cb62ef36 100644 --- a/src/audio/host.c +++ b/src/audio/host.c @@ -529,7 +529,6 @@ static int host_cmd(struct comp_dev *dev, int cmd, void *data) return ret; switch (cmd) { - case COMP_CMD_PAUSE: case COMP_CMD_STOP: ret = host_stop(dev); break; From 3318a16455f4dea2fa8b34bc658b4b95de085b66 Mon Sep 17 00:00:00 2001 From: Keyon Jie Date: Fri, 22 Dec 2017 23:10:57 +0800 Subject: [PATCH 09/10] trace: do dma trace when CONFIG_DMA_TRACE is selected CONFIG_DMA_TRACE is selected by default. But we should not do dma trace when CONFIG_DMA_TRACE is not selected (--disable-dma-trace is added to configure command line), here add this judgement to fix compiling issue for that dma trace is not selected. Signed-off-by: Keyon Jie Acked-by: Yan Wang --- src/lib/trace.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/lib/trace.c b/src/lib/trace.c index bb81d477b..859bc0e1f 100644 --- a/src/lib/trace.c +++ b/src/lib/trace.c @@ -49,7 +49,9 @@ void _trace_error(uint32_t event) { unsigned long flags; volatile uint64_t *t; +#if defined(CONFIG_DMA_TRACE) uint64_t dt[2]; +#endif uint64_t time; if (!trace.enable) @@ -57,10 +59,12 @@ void _trace_error(uint32_t event) time = platform_timer_get(platform_timer); +#if defined(CONFIG_DMA_TRACE) /* save event to DMA tracing buffer */ dt[0] = time; dt[1] = event; dtrace_event((const char*)dt, sizeof(uint64_t) * 2); +#endif /* send event by mail box too. */ spin_lock_irq(&trace.lock, flags); @@ -84,7 +88,9 @@ void _trace_error(uint32_t event) void _trace_error_atomic(uint32_t event) { volatile uint64_t *t; +#if defined(CONFIG_DMA_TRACE) uint64_t dt[2]; +#endif uint64_t time; if (!trace.enable) @@ -92,10 +98,12 @@ void _trace_error_atomic(uint32_t event) time = platform_timer_get(platform_timer); +#if defined(CONFIG_DMA_TRACE) /* save event to DMA tracing buffer */ dt[0] = time; dt[1] = event; dtrace_event_atomic((const char*)dt, sizeof(uint64_t) * 2); +#endif /* write timestamp and event to trace buffer */ t = (volatile uint64_t*)(MAILBOX_TRACE_BASE + trace.pos); From 1a8a653af8ef4553192f2fa88407cc256aea5816 Mon Sep 17 00:00:00 2001 From: Keyon Jie Date: Fri, 22 Dec 2017 23:10:56 +0800 Subject: [PATCH 10/10] dma-trace: add build condition for dma trace Only compile dma-trace.c when BUILD_DMA_TRACE is true. Signed-off-by: Keyon Jie Acked-by: Yan Wang --- configure.ac | 2 ++ src/lib/Makefile.am | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 86d19cb33..44ede24b9 100644 --- a/configure.ac +++ b/configure.ac @@ -132,6 +132,8 @@ AS_IF([test "x$enable_dma_trace" != "xno"], [ AC_DEFINE([CONFIG_DMA_TRACE], [1], [Configure DMA trace]) ]) +AM_CONDITIONAL(BUILD_DMA_TRACE, test "x$enable_dma_trace" != "xno") + # Test after CFLAGS set othewise test of cross compiler fails. AM_PROG_AS AM_PROG_AR diff --git a/src/lib/Makefile.am b/src/lib/Makefile.am index b4521dfa0..6ea4caf10 100644 --- a/src/lib/Makefile.am +++ b/src/lib/Makefile.am @@ -6,9 +6,13 @@ libcore_a_SOURCES = \ work.c \ notifier.c \ trace.c \ - dma-trace.c \ schedule.c +if BUILD_DMA_TRACE +libcore_a_SOURCES += \ + dma-trace.c +endif + libcore_a_CFLAGS = \ $(ARCH_CFLAGS) \ $(ARCH_INCDIR) \