[Sound-open-firmware] [PATCH] [RFC]rmbox: include trace class definitions from trace header
Remove hard-coded trace class definitions from rmbox and use sof trace header instead. This makes it easier to keep track of any new trace class definitions.
This introduces a dependency on the sof headers being pre-installed before building rmbox.
Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Yan Wang yan.wang@linux.intel.com --- rmbox/Makefile.am | 20 +++++- rmbox/rmbox.c | 171 +++++++++++++++++++++++++++------------------- 2 files changed, 121 insertions(+), 70 deletions(-)
diff --git a/rmbox/Makefile.am b/rmbox/Makefile.am index bfeb184..9763361 100644 --- a/rmbox/Makefile.am +++ b/rmbox/Makefile.am @@ -1,4 +1,22 @@ +SOF_INC = $(prefix)/include/sof +SOF_INC_NEW := $(subst /,_,$(SOF_INC)) +START_OLD =_binary_$(SOF_INC_NEW)_sof_trace_h_start +END_OLD =_binary_$(SOF_INC_NEW)_sof_trace_h_end + bin_PROGRAMS = rmbox
rmbox_SOURCES = \ - rmbox.c \ No newline at end of file + rmbox.c + +rmbox_CFLAGS = \ + -I $(SOF_INC) + +rmbox_LDADD = \ + trace.o + +trace.o: + objcopy --input binary --output elf64-x86-64 \ + --binary-architecture i386:x86-64 \ + $(SOF_INC)/sof/trace.h trace.o + objcopy --redefine-sym $(START_OLD)=_trace_start trace.o + objcopy --redefine-sym $(END_OLD)=_trace_end trace.o diff --git a/rmbox/rmbox.c b/rmbox/rmbox.c index 4df2ee1..0281904 100644 --- a/rmbox/rmbox.c +++ b/rmbox/rmbox.c @@ -20,37 +20,109 @@ #include <errno.h> #include <string.h> #include <ctype.h> +#include <sof/trace.h>
#define KNRM "\x1B[0m" #define KRED "\x1B[31m"
-// TODO: include all this stuff - -#define TRACE_CLASS_IRQ (1 << 24) -#define TRACE_CLASS_IPC (2 << 24) -#define TRACE_CLASS_PIPE (3 << 24) -#define TRACE_CLASS_HOST (4 << 24) -#define TRACE_CLASS_DAI (5 << 24) -#define TRACE_CLASS_DMA (6 << 24) -#define TRACE_CLASS_SSP (7 << 24) -#define TRACE_CLASS_COMP (8 << 24) -#define TRACE_CLASS_WAIT (9 << 24) -#define TRACE_CLASS_LOCK (10 << 24) -#define TRACE_CLASS_MEM (11 << 24) -#define TRACE_CLASS_MIXER (12 << 24) -#define TRACE_CLASS_BUFFER (13 << 24) -#define TRACE_CLASS_VOLUME (14 << 24) -#define TRACE_CLASS_SWITCH (15 << 24) -#define TRACE_CLASS_MUX (16 << 24) -#define TRACE_CLASS_SRC (17 << 24) -#define TRACE_CLASS_TONE (18 << 24) -#define TRACE_CLASS_EQ_FIR (19 << 24) -#define TRACE_CLASS_EQ_IIR (20 << 24) - -#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0])) +#define TRACE_BLOCK_SIZE 8
+#define MAX_LINE_LEN 1024
-#define TRACE_BLOCK_SIZE 8 +struct trace_class_table { + int trace_class; + char *class_name; +}; + +struct trace_class_table *trace_table; +int num_trace_classes; + +/* trace defines pointers */ +extern char _trace_start; +extern char _trace_end; + +/* set up trace class identifier table based on trace.sh in sof */ +static void setup_trace_table() +{ + char *token; + char line[MAX_LINE_LEN]; + int ret, i = 0, j = 0; + char* p = &_trace_start; + + /* find out the number of trace class definitions from trace header */ + while (p != &_trace_end) { + line[i++] = *p; + + /* read until new line character */ + if (*p == '\n') { + line[i-1] = '\0'; + char identifier[MAX_LINE_LEN]; + int value = 0, shift = 0; + ret = sscanf(line, "#define %s (%d << %d)", identifier, + &value, &shift); + if (ret == 3) { + + /* if TRACE_CLASS definition */ + if (strstr(identifier, "TRACE_CLASS")) + j++; + } + i = 0; + } + p++; + } + + num_trace_classes = j; + + /* allocate memory for trace table */ + trace_table = (struct trace_class_table *)malloc(sizeof(struct trace_class_table) * num_trace_classes); + + i = 0; + j = 0; + + /* rewind pointer */ + p = &_trace_start; + + /* begin reading trace header */ + while (p != &_trace_end) { + line[i++] = *p; + if (*p == '\n') { + line[i-1] = '\0'; + char identifier[MAX_LINE_LEN]; + int value = 0, shift = 0; + ret = sscanf(line, "#define %s (%d << %d)", identifier, + &value, &shift); + if (ret == 3) { + + /* if TRACE_CLASS definition */ + if (strstr(identifier, "TRACE_CLASS")) { + token = strtok(identifier, "_"); + token = strtok(NULL, "_"); + token = strtok(NULL, "_"); + + /* add trace class entry */ + trace_table[j].trace_class = value; + trace_table[j++].class_name = strdup(token); + } + } + i = 0; + } + p++; + } +} + +/* look up subsystem class name from table */ +static char *get_trace_class(uint32_t trace_class) +{ + int i; + + /* look up trace class table and return subsystem name */ + for (i = 0; i < num_trace_classes; i++) { + if (trace_table[i].trace_class == trace_class) + return trace_table[i].class_name; + } + + return "value"; +}
static inline char get_char(uint32_t val, int idx) { @@ -122,50 +194,9 @@ static void show_trace(uint64_t val, uint64_t addr, uint64_t *timestamp, double }
class = val & 0xff000000; - if (class == TRACE_CLASS_IRQ) - trace = "irq"; - else if (class == TRACE_CLASS_IPC) - trace = "ipc"; - else if (class == TRACE_CLASS_PIPE) - trace = "pipe"; - else if (class == TRACE_CLASS_HOST) - trace = "host"; - else if (class == TRACE_CLASS_DAI) - trace = "dai"; - else if (class == TRACE_CLASS_DMA) - trace = "dma"; - else if (class == TRACE_CLASS_SSP) - trace = "ssp"; - else if (class == TRACE_CLASS_COMP) - trace = "comp"; - else if (class == TRACE_CLASS_WAIT) - trace = "wait"; - else if (class == TRACE_CLASS_LOCK) - trace = "lock"; - else if (class == TRACE_CLASS_MEM) - trace = "mem"; - else if (class == TRACE_CLASS_MIXER) - trace = "mixer"; - else if (class == TRACE_CLASS_BUFFER) - trace = "buffer"; - else if (class == TRACE_CLASS_VOLUME) - trace = "volume"; - else if (class == TRACE_CLASS_SWITCH) - trace = "switch"; - else if (class == TRACE_CLASS_MUX) - trace = "mux"; - else if (class == TRACE_CLASS_SRC) - trace = "src"; - else if (class == TRACE_CLASS_TONE) - trace = "tone"; - else if (class == TRACE_CLASS_EQ_FIR) - trace = "eq-fir"; - else if (class == TRACE_CLASS_EQ_IIR) - trace = "eq-iir"; - else { - printf("value 0x%8.8x\n", (uint32_t)val); - return; - } + + /* look up trace subsystem from table */ + trace = strdup(get_trace_class(class >> 24));
switch ((char)(val >> 16)) { case 'e': @@ -343,6 +374,8 @@ int main(int argc, char *argv[]) } }
+ setup_trace_table(); + /* trace requested ? */ if (trace) return trace_read("/sys/kernel/debug/sof/trace",
On Mon, 2018-05-14 at 09:17 -0700, Ranjani Sridharan wrote:
This introduces a dependency on the sof headers being pre-installed before building rmbox.
Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Yan Wang yan.wang@linux.intel.com
rmbox/Makefile.am | 20 +++++- rmbox/rmbox.c | 171 +++++++++++++++++++++++++++------------------- 2 files changed, 121 insertions(+), 70 deletions(-)
diff --git a/rmbox/Makefile.am b/rmbox/Makefile.am index bfeb184..9763361 100644 --- a/rmbox/Makefile.am +++ b/rmbox/Makefile.am @@ -1,4 +1,22 @@ +SOF_INC = $(prefix)/include/sof +SOF_INC_NEW := $(subst /,_,$(SOF_INC)) +START_OLD =_binary_$(SOF_INC_NEW)_sof_trace_h_start +END_OLD =_binary_$(SOF_INC_NEW)_sof_trace_h_end
What's this for ? You can use AC_ macro in configure.ac to detect the presence of an installed header.
bin_PROGRAMS = rmbox
rmbox_SOURCES = \
- rmbox.c
\ No newline at end of file
- rmbox.c
+rmbox_CFLAGS = \
- -I $(SOF_INC)
+rmbox_LDADD = \
- trace.o
+trace.o:
- objcopy --input binary --output elf64-x86-64 \
- --binary-architecture i386:x86-64 \
- $(SOF_INC)/sof/trace.h trace.o
- objcopy --redefine-sym $(START_OLD)=_trace_start trace.o
- objcopy --redefine-sym $(END_OLD)=_trace_end trace.o
diff --git a/rmbox/rmbox.c b/rmbox/rmbox.c index 4df2ee1..0281904 100644 --- a/rmbox/rmbox.c +++ b/rmbox/rmbox.c @@ -20,37 +20,109 @@ #include <errno.h> #include <string.h> #include <ctype.h> +#include <sof/trace.h>
#define KNRM "\x1B[0m" #define KRED "\x1B[31m"
-// TODO: include all this stuff
-#define TRACE_CLASS_IRQ (1 << 24) -#define TRACE_CLASS_IPC (2 << 24) -#define TRACE_CLASS_PIPE (3 << 24) -#define TRACE_CLASS_HOST (4 << 24) -#define TRACE_CLASS_DAI (5 << 24) -#define TRACE_CLASS_DMA (6 << 24) -#define TRACE_CLASS_SSP (7 << 24) -#define TRACE_CLASS_COMP (8 << 24) -#define TRACE_CLASS_WAIT (9 << 24) -#define TRACE_CLASS_LOCK (10 << 24) -#define TRACE_CLASS_MEM (11 << 24) -#define TRACE_CLASS_MIXER (12 << 24) -#define TRACE_CLASS_BUFFER (13 << 24) -#define TRACE_CLASS_VOLUME (14 << 24) -#define TRACE_CLASS_SWITCH (15 << 24) -#define TRACE_CLASS_MUX (16 << 24) -#define TRACE_CLASS_SRC (17 << 24) -#define TRACE_CLASS_TONE (18 << 24) -#define TRACE_CLASS_EQ_FIR (19 << 24) -#define TRACE_CLASS_EQ_IIR (20 << 24)
-#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0])) +#define TRACE_BLOCK_SIZE 8
+#define MAX_LINE_LEN 1024
-#define TRACE_BLOCK_SIZE 8 +struct trace_class_table {
- int trace_class;
- char *class_name;
+};
+struct trace_class_table *trace_table; +int num_trace_classes;
+/* trace defines pointers */ +extern char _trace_start; +extern char _trace_end;
+/* set up trace class identifier table based on trace.sh in sof */
trace.sh in another patch ?
Liam
+static void setup_trace_table() +{
- char *token;
- char line[MAX_LINE_LEN];
- int ret, i = 0, j = 0;
- char* p = &_trace_start;
- /* find out the number of trace class definitions from trace header
*/
- while (p != &_trace_end) {
line[i++] = *p;/* read until new line character */if (*p == '\n') {line[i-1] = '\0';char identifier[MAX_LINE_LEN];int value = 0, shift = 0;ret = sscanf(line, "#define %s (%d << %d)",identifier,
&value, &shift);if (ret == 3) {/* if TRACE_CLASS definition */if (strstr(identifier, "TRACE_CLASS"))j++;}i = 0;}p++;- }
- num_trace_classes = j;
- /* allocate memory for trace table */
- trace_table = (struct trace_class_table *)malloc(sizeof(struct
trace_class_table) * num_trace_classes);
- i = 0;
- j = 0;
- /* rewind pointer */
- p = &_trace_start;
- /* begin reading trace header */
- while (p != &_trace_end) {
line[i++] = *p;if (*p == '\n') {line[i-1] = '\0';char identifier[MAX_LINE_LEN];int value = 0, shift = 0;ret = sscanf(line, "#define %s (%d << %d)",identifier,
&value, &shift);if (ret == 3) {/* if TRACE_CLASS definition */if (strstr(identifier, "TRACE_CLASS")) {token = strtok(identifier, "_");token = strtok(NULL, "_");token = strtok(NULL, "_");/* add trace class entry */trace_table[j].trace_class = value;trace_table[j++].class_name =strdup(token);
}}i = 0;}p++;- }
+}
+/* look up subsystem class name from table */ +static char *get_trace_class(uint32_t trace_class) +{
- int i;
- /* look up trace class table and return subsystem name */
- for (i = 0; i < num_trace_classes; i++) {
if (trace_table[i].trace_class == trace_class)return trace_table[i].class_name;- }
- return "value";
+}
static inline char get_char(uint32_t val, int idx) { @@ -122,50 +194,9 @@ static void show_trace(uint64_t val, uint64_t addr, uint64_t *timestamp, double }
class = val & 0xff000000;
- if (class == TRACE_CLASS_IRQ)
trace = "irq";- else if (class == TRACE_CLASS_IPC)
trace = "ipc";- else if (class == TRACE_CLASS_PIPE)
trace = "pipe";- else if (class == TRACE_CLASS_HOST)
trace = "host";- else if (class == TRACE_CLASS_DAI)
trace = "dai";- else if (class == TRACE_CLASS_DMA)
trace = "dma";- else if (class == TRACE_CLASS_SSP)
trace = "ssp";- else if (class == TRACE_CLASS_COMP)
trace = "comp";- else if (class == TRACE_CLASS_WAIT)
trace = "wait";- else if (class == TRACE_CLASS_LOCK)
trace = "lock";- else if (class == TRACE_CLASS_MEM)
trace = "mem";- else if (class == TRACE_CLASS_MIXER)
trace = "mixer";- else if (class == TRACE_CLASS_BUFFER)
trace = "buffer";- else if (class == TRACE_CLASS_VOLUME)
trace = "volume";- else if (class == TRACE_CLASS_SWITCH)
trace = "switch";- else if (class == TRACE_CLASS_MUX)
trace = "mux";- else if (class == TRACE_CLASS_SRC)
trace = "src";- else if (class == TRACE_CLASS_TONE)
trace = "tone";- else if (class == TRACE_CLASS_EQ_FIR)
trace = "eq-fir";- else if (class == TRACE_CLASS_EQ_IIR)
trace = "eq-iir";- else {
printf("value 0x%8.8x\n", (uint32_t)val);return;- }
/* look up trace subsystem from table */
trace = strdup(get_trace_class(class >> 24));
switch ((char)(val >> 16)) { case 'e':
@@ -343,6 +374,8 @@ int main(int argc, char *argv[]) } }
- setup_trace_table();
- /* trace requested ? */ if (trace) return trace_read("/sys/kernel/debug/sof/trace",
On Thu, 2018-05-17 at 15:23 +0100, Liam Girdwood wrote:
On Mon, 2018-05-14 at 09:17 -0700, Ranjani Sridharan wrote:
This introduces a dependency on the sof headers being pre-installed before building rmbox.
Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com
Signed-off-by: Yan Wang yan.wang@linux.intel.com
rmbox/Makefile.am | 20 +++++- rmbox/rmbox.c | 171 +++++++++++++++++++++++++++---------------
2 files changed, 121 insertions(+), 70 deletions(-)
diff --git a/rmbox/Makefile.am b/rmbox/Makefile.am index bfeb184..9763361 100644 --- a/rmbox/Makefile.am +++ b/rmbox/Makefile.am @@ -1,4 +1,22 @@ +SOF_INC = $(prefix)/include/sof +SOF_INC_NEW := $(subst /,_,$(SOF_INC)) +START_OLD =_binary_$(SOF_INC_NEW)_sof_trace_h_start +END_OLD =_binary_$(SOF_INC_NEW)_sof_trace_h_end
What's this for ? You can use AC_ macro in configure.ac to detect the presence of an installed header.
because the SOF headers wont be installed on the device that we run rmbox on, I've bundled up the header in the executable. And the above definitions are to rename the symbols in the executable to something like trace_start and trace_end to access the trace class definitions from the header.
I couldnt think of a way better way that including the trace header object in the rmbox application. Is this acceptable?
bin_PROGRAMS = rmbox
rmbox_SOURCES = \
- rmbox.c
\ No newline at end of file
- rmbox.c
+rmbox_CFLAGS = \
- -I $(SOF_INC)
+rmbox_LDADD = \
- trace.o
+trace.o:
- objcopy --input binary --output elf64-x86-64 \
- --binary-architecture i386:x86-64 \
- $(SOF_INC)/sof/trace.h trace.o
- objcopy --redefine-sym $(START_OLD)=_trace_start trace.o
- objcopy --redefine-sym $(END_OLD)=_trace_end trace.o
diff --git a/rmbox/rmbox.c b/rmbox/rmbox.c index 4df2ee1..0281904 100644 --- a/rmbox/rmbox.c +++ b/rmbox/rmbox.c @@ -20,37 +20,109 @@ #include <errno.h> #include <string.h> #include <ctype.h> +#include <sof/trace.h>
#define KNRM "\x1B[0m" #define KRED "\x1B[31m"
-// TODO: include all this stuff
-#define TRACE_CLASS_IRQ (1 << 24) -#define TRACE_CLASS_IPC (2 << 24) -#define TRACE_CLASS_PIPE (3 << 24) -#define TRACE_CLASS_HOST (4 << 24) -#define TRACE_CLASS_DAI (5 << 24) -#define TRACE_CLASS_DMA (6 << 24) -#define TRACE_CLASS_SSP (7 << 24) -#define TRACE_CLASS_COMP (8 << 24) -#define TRACE_CLASS_WAIT (9 << 24) -#define TRACE_CLASS_LOCK (10 << 24) -#define TRACE_CLASS_MEM (11 << 24) -#define TRACE_CLASS_MIXER (12 << 24) -#define TRACE_CLASS_BUFFER (13 << 24) -#define TRACE_CLASS_VOLUME (14 << 24) -#define TRACE_CLASS_SWITCH (15 << 24) -#define TRACE_CLASS_MUX (16 << 24) -#define TRACE_CLASS_SRC (17 << 24) -#define TRACE_CLASS_TONE (18 << 24) -#define TRACE_CLASS_EQ_FIR (19 << 24) -#define TRACE_CLASS_EQ_IIR (20 << 24)
-#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0])) +#define TRACE_BLOCK_SIZE 8
+#define MAX_LINE_LEN 1024
-#define TRACE_BLOCK_SIZE 8 +struct trace_class_table {
- int trace_class;
- char *class_name;
+};
+struct trace_class_table *trace_table; +int num_trace_classes;
+/* trace defines pointers */ +extern char _trace_start; +extern char _trace_end;
+/* set up trace class identifier table based on trace.sh in sof */
trace.sh in another patch ?
Liam
+static void setup_trace_table() +{
- char *token;
- char line[MAX_LINE_LEN];
- int ret, i = 0, j = 0;
- char* p = &_trace_start;
- /* find out the number of trace class definitions from
trace header */
- while (p != &_trace_end) {
line[i++] = *p;/* read until new line character */if (*p == '\n') {line[i-1] = '\0';char identifier[MAX_LINE_LEN];int value = 0, shift = 0;ret = sscanf(line, "#define %s (%d <<%d)", identifier,
&value, &shift);if (ret == 3) {/* if TRACE_CLASS definition */if (strstr(identifier,"TRACE_CLASS"))
j++;}i = 0;}p++;- }
- num_trace_classes = j;
- /* allocate memory for trace table */
- trace_table = (struct trace_class_table
*)malloc(sizeof(struct trace_class_table) * num_trace_classes);
- i = 0;
- j = 0;
- /* rewind pointer */
- p = &_trace_start;
- /* begin reading trace header */
- while (p != &_trace_end) {
line[i++] = *p;if (*p == '\n') {line[i-1] = '\0';char identifier[MAX_LINE_LEN];int value = 0, shift = 0;ret = sscanf(line, "#define %s (%d <<%d)", identifier,
&value, &shift);if (ret == 3) {/* if TRACE_CLASS definition */if (strstr(identifier,"TRACE_CLASS")) {
token = strtok(identifier,"_");
token = strtok(NULL, "_");token = strtok(NULL, "_");/* add trace class entry*/
trace_table[j].trace_class= value;
trace_table[j++].class_name = strdup(token);
}}i = 0;}p++;- }
+}
+/* look up subsystem class name from table */ +static char *get_trace_class(uint32_t trace_class) +{
- int i;
- /* look up trace class table and return subsystem name */
- for (i = 0; i < num_trace_classes; i++) {
if (trace_table[i].trace_class == trace_class)return trace_table[i].class_name;- }
- return "value";
+}
static inline char get_char(uint32_t val, int idx) { @@ -122,50 +194,9 @@ static void show_trace(uint64_t val, uint64_t addr, uint64_t *timestamp, double }
class = val & 0xff000000;
- if (class == TRACE_CLASS_IRQ)
trace = "irq";- else if (class == TRACE_CLASS_IPC)
trace = "ipc";- else if (class == TRACE_CLASS_PIPE)
trace = "pipe";- else if (class == TRACE_CLASS_HOST)
trace = "host";- else if (class == TRACE_CLASS_DAI)
trace = "dai";- else if (class == TRACE_CLASS_DMA)
trace = "dma";- else if (class == TRACE_CLASS_SSP)
trace = "ssp";- else if (class == TRACE_CLASS_COMP)
trace = "comp";- else if (class == TRACE_CLASS_WAIT)
trace = "wait";- else if (class == TRACE_CLASS_LOCK)
trace = "lock";- else if (class == TRACE_CLASS_MEM)
trace = "mem";- else if (class == TRACE_CLASS_MIXER)
trace = "mixer";- else if (class == TRACE_CLASS_BUFFER)
trace = "buffer";- else if (class == TRACE_CLASS_VOLUME)
trace = "volume";- else if (class == TRACE_CLASS_SWITCH)
trace = "switch";- else if (class == TRACE_CLASS_MUX)
trace = "mux";- else if (class == TRACE_CLASS_SRC)
trace = "src";- else if (class == TRACE_CLASS_TONE)
trace = "tone";- else if (class == TRACE_CLASS_EQ_FIR)
trace = "eq-fir";- else if (class == TRACE_CLASS_EQ_IIR)
trace = "eq-iir";- else {
printf("value 0x%8.8x\n", (uint32_t)val);return;- }
/* look up trace subsystem from table */
trace = strdup(get_trace_class(class >> 24));
switch ((char)(val >> 16)) { case 'e':
@@ -343,6 +374,8 @@ int main(int argc, char *argv[]) } }
- setup_trace_table();
- /* trace requested ? */ if (trace) return trace_read("/sys/kernel/debug/sof/trace",
On Thu, 2018-05-17 at 10:48 -0700, Ranjani Sridharan wrote:
What's this for ? You can use AC_ macro in configure.ac to detect the presence of an installed header.
because the SOF headers wont be installed on the device that we run rmbox on, I've bundled up the header in the executable. And the above definitions are to rename the symbols in the executable to something like trace_start and trace_end to access the trace class definitions from the header.
I couldnt think of a way better way that including the trace header object in the rmbox application. Is this acceptable?
So the trace class strings are deprecated, Yan was going to export all the trace object names + IDs via debugfs (like DAPM widgets already do). rmbox should read these at startup so it could then lookup comp name from the ID number embedded in trace data.
Yan, do you have parts of this implemented ?
Liam
On 5/18/2018 2:38 AM, Liam Girdwood wrote:
On Thu, 2018-05-17 at 10:48 -0700, Ranjani Sridharan wrote:
What's this for ? You can use AC_ macro in configure.ac to detect the presence of an installed header.
because the SOF headers wont be installed on the device that we run rmbox on, I've bundled up the header in the executable. And the above definitions are to rename the symbols in the executable to something like trace_start and trace_end to access the trace class definitions from the header.
I couldnt think of a way better way that including the trace header object in the rmbox application. Is this acceptable?
So the trace class strings are deprecated, Yan was going to export all the trace object names + IDs via debugfs (like DAPM widgets already do). rmbox should read these at startup so it could then lookup comp name from the ID number embedded in trace data.
Yan, do you have parts of this implemented ?
In my previous RFC v2 patch set, I still keep trace class macro definition in trace record but support switch on/off based component id. I am working on displaying compoent/widget name in rmbox and will submit in the next version patch set. But for global trace like DMA/IPC/... or early component trace which locates in comp_new()/pipe_new()/buffer_new() may still need trace class because they haven't corresponding component/widget name?
Thanks. Yan Wang
Liam
On Fri, 2018-05-18 at 10:09 +0800, Yan Wang wrote:
So the trace class strings are deprecated, Yan was going to export all the trace object names + IDs via debugfs (like DAPM widgets already do). rmbox should read these at startup so it could then lookup comp name from the ID number embedded in trace data.
Yan, do you have parts of this implemented ?
In my previous RFC v2 patch set, I still keep trace class macro definition in trace record but support switch on/off based component id. I am working on displaying compoent/widget name in rmbox and will submit in the next version patch set. But for global trace like DMA/IPC/... or early component trace which locates in comp_new()/pipe_new()/buffer_new() may still need trace class because they haven't corresponding component/widget name?
Non component trace classes can be exported as "extended boot data" i.e.
1) create a structure array that can be appended onto the boot complete message (after SRAM windows) and sent to the host.
struct ipc_trace_class_elem { char name[SIZE]; uint32_t id; };
struct ipc_trace_class { int num_elems; struct ipc_trace_class_elem[0]; };
so you would have
struct ipc_trace_class_elem trace_class_elems[] = { {"pipeline", PIPELINE_TRACE_ID}, ..... };
2) The driver will create a debugFS file for each trace class elem. When read (by rmbox or cat) it will contain the ID number. This way rmbox can map IDs to names for classes too.
3) rmbox reads these debugFS files at strartup to create it's ID -> name mapings.
Steps 2 & 3 are very similar to the component ID name lookups.
Doing this means rmbox and the driver do not need to be modified when we add new trace objects in the FW.
Liam
On Fri, 2018-05-18 at 09:06 +0100, Liam Girdwood wrote:
On Fri, 2018-05-18 at 10:09 +0800, Yan Wang wrote:
So the trace class strings are deprecated, Yan was going to export all the trace object names + IDs via debugfs (like DAPM widgets already do). rmbox should read these at startup so it could then lookup comp name from the ID number embedded in trace data.
Yan, do you have parts of this implemented ?
In my previous RFC v2 patch set, I still keep trace class macro definition in trace record but support switch on/off based component id. I am working on displaying compoent/widget name in rmbox and will submit in the next version patch set. But for global trace like DMA/IPC/... or early component trace which locates in comp_new()/pipe_new()/buffer_new() may still need trace class because they haven't corresponding component/widget name?
Non component trace classes can be exported as "extended boot data" i.e.
- create a structure array that can be appended onto the boot
complete message (after SRAM windows) and sent to the host.
struct ipc_trace_class_elem { char name[SIZE]; uint32_t id; };
struct ipc_trace_class { int num_elems; struct ipc_trace_class_elem[0]; };
so you would have
struct ipc_trace_class_elem trace_class_elems[] = { {"pipeline", PIPELINE_TRACE_ID}, ..... };
Yes. I have finished this step now. And I also send name string from SOF side to kernel driver.
- The driver will create a debugFS file for each trace class elem.
When read (by rmbox or cat) it will contain the ID number. This way rmbox can map IDs to names for classes too.
I think we may not need so many debugfs interfaces. We can pass one parameter into one trace_level debugfs interface get all trace class map. And rmbox can understand trace class after getting this map.
- rmbox reads these debugFS files at strartup to create it's ID ->
name mapings.
Steps 2 & 3 are very similar to the component ID name lookups.
Doing this means rmbox and the driver do not need to be modified when we add new trace objects in the FW.
Agree. We can remove all trace class macro definition. both rmbox and kernel driver will be dependent on SOF. So it is scalable fully.
I attach my finished structure decalation for reviwing in the following. It still include trace class. I can remove it in the next version patch set.
922 /* DMA trace level type */ 923 enum sof_dma_trace_level_type { 924 SOF_DMA_TRACE_LEVEL_IRQ = 0, 925 SOF_DMA_TRACE_LEVEL_IPC, 926 SOF_DMA_TRACE_LEVEL_DMA, 927 SOF_DMA_TRACE_LEVEL_SSP, 928 SOF_DMA_TRACE_LEVEL_WAIT, 929 SOF_DMA_TRACE_LEVEL_LOCK, 930 SOF_DMA_TRACE_LEVEL_MEM, 931 SOF_DMA_TRACE_LEVEL_SA, 932 SOF_DMA_TRACE_LEVEL_DMIC, 933 SOF_DMA_TRACE_LEVEL_SCH, 934 SOF_DMA_TRACE_LEVEL_VALUE, 935 SOF_DMA_TRACE_LEVEL_END, 936 }; 937 938 /* DMA for Trace level elem - SOF_IPC_DEBUG_DMA_LEVELS */ 939 struct sof_ipc_dma_trace_level { 940 uint32_t trace_class; 941 enum sof_dma_trace_level_type type; 942 int32_t comp_id; 943 char name[8]; 944 uint32_t level; 945 } __attribute__((packed)); 946 947 /* DMA for Trace level info - SOF_IPC_DEBUG_DMA_LEVELS */ 948 struct sof_ipc_dma_trace_levels { 949 struct sof_ipc_ext_data_hdr ext_hdr; 950 uint32_t num_levels; 951 struct sof_ipc_dma_trace_level level[0]; 952 } __attribute__((packed));
We can also use this structure to set and query multiply module/class trace levels. My current focus issue is component/widget trace level list from SOF side. When FW_READY message sent, the topology should not be loadded. I may have to send component/widget trace level list later. Is it right?
Thanks. Yan Wang
Liam
On Fri, 2018-05-18 at 16:52 +0800, yanwang wrote:
On Fri, 2018-05-18 at 09:06 +0100, Liam Girdwood wrote:
On Fri, 2018-05-18 at 10:09 +0800, Yan Wang wrote:
So the trace class strings are deprecated, Yan was going to export all the trace object names + IDs via debugfs (like DAPM widgets already do). rmbox should read these at startup so it could then lookup comp name from the ID number embedded in trace data.
Yan, do you have parts of this implemented ?
In my previous RFC v2 patch set, I still keep trace class macro definition in trace record but support switch on/off based component id. I am working on displaying compoent/widget name in rmbox and will submit in the next version patch set. But for global trace like DMA/IPC/... or early component trace which locates in comp_new()/pipe_new()/buffer_new() may still need trace class because they haven't corresponding component/widget name?
Non component trace classes can be exported as "extended boot data" i.e.
- create a structure array that can be appended onto the boot
complete message (after SRAM windows) and sent to the host.
struct ipc_trace_class_elem { char name[SIZE]; uint32_t id; };
struct ipc_trace_class { int num_elems; struct ipc_trace_class_elem[0]; };
so you would have
struct ipc_trace_class_elem trace_class_elems[] = { {"pipeline", PIPELINE_TRACE_ID}, ..... };
Yes. I have finished this step now. And I also send name string from SOF side to kernel driver.
- The driver will create a debugFS file for each trace class elem.
When read (by rmbox or cat) it will contain the ID number. This way rmbox can map IDs to names for classes too.
I think we may not need so many debugfs interfaces. We can pass one parameter into one trace_level debugfs interface get all trace class map. And rmbox can understand trace class after getting this map.
Ok, you can do 1 file for classes, but components may come and go and may have different params for each so it makes sense to use a different file for each component.
- rmbox reads these debugFS files at strartup to create it's ID ->
name mapings.
Steps 2 & 3 are very similar to the component ID name lookups.
Doing this means rmbox and the driver do not need to be modified when we add new trace objects in the FW.
Agree. We can remove all trace class macro definition. both rmbox and kernel driver will be dependent on SOF. So it is scalable fully.
I attach my finished structure decalation for reviwing in the following. It still include trace class. I can remove it in the next version patch set.
922 /* DMA trace level type */ 923 enum sof_dma_trace_level_type { 924 SOF_DMA_TRACE_LEVEL_IRQ = 0, 925 SOF_DMA_TRACE_LEVEL_IPC, 926 SOF_DMA_TRACE_LEVEL_DMA, 927 SOF_DMA_TRACE_LEVEL_SSP, 928 SOF_DMA_TRACE_LEVEL_WAIT, 929 SOF_DMA_TRACE_LEVEL_LOCK, 930 SOF_DMA_TRACE_LEVEL_MEM, 931 SOF_DMA_TRACE_LEVEL_SA, 932 SOF_DMA_TRACE_LEVEL_DMIC, 933 SOF_DMA_TRACE_LEVEL_SCH, 934 SOF_DMA_TRACE_LEVEL_VALUE, 935 SOF_DMA_TRACE_LEVEL_END, 936 }; 937
So these are non component trace class IDs now, they dont need to be in ipc.h and should start at some high number so they wont conflict with component IDs (that start at 0).
938 /* DMA for Trace level elem - SOF_IPC_DEBUG_DMA_LEVELS */ 939 struct sof_ipc_dma_trace_level { 940 uint32_t trace_class; 941 enum sof_dma_trace_level_type type; 942 int32_t comp_id; 943 char name[8]; 944 uint32_t level; 945 } __attribute__((packed));
All you need here is name and ID. level, type will be set by userspace.
946 947 /* DMA for Trace level info - SOF_IPC_DEBUG_DMA_LEVELS */ 948 struct sof_ipc_dma_trace_levels { 949 struct sof_ipc_ext_data_hdr ext_hdr; 950 uint32_t num_levels;
num_classes
951 struct sof_ipc_dma_trace_level level[0];
trace_class
952 } __attribute__((packed));
We can also use this structure to set and query multiply module/class trace levels. My current focus issue is component/widget trace level list from SOF side. When FW_READY message sent, the topology should not be loadded.
Yes, topology is loaded after FW ready.
I may have to send component/widget trace level list later. Is it right?
No, driver will create component trace debugFS objects when it loads topology. Driver will already have this data so no need for FW to send it.
Liam
Thanks. Yan Wang
Liam
On 5/18/2018 6:27 PM, Liam Girdwood wrote:
On Fri, 2018-05-18 at 16:52 +0800, yanwang wrote:
On Fri, 2018-05-18 at 09:06 +0100, Liam Girdwood wrote:
On Fri, 2018-05-18 at 10:09 +0800, Yan Wang wrote:
So the trace class strings are deprecated, Yan was going to export all the trace object names + IDs via debugfs (like DAPM widgets already do). rmbox should read these at startup so it could then lookup comp name from the ID number embedded in trace data.
Yan, do you have parts of this implemented ?
In my previous RFC v2 patch set, I still keep trace class macro definition in trace record but support switch on/off based component id. I am working on displaying compoent/widget name in rmbox and will submit in the next version patch set. But for global trace like DMA/IPC/... or early component trace which locates in comp_new()/pipe_new()/buffer_new() may still need trace class because they haven't corresponding component/widget name?
Non component trace classes can be exported as "extended boot data" i.e.
- create a structure array that can be appended onto the boot
complete message (after SRAM windows) and sent to the host.
struct ipc_trace_class_elem { char name[SIZE]; uint32_t id; };
struct ipc_trace_class { int num_elems; struct ipc_trace_class_elem[0]; };
so you would have
struct ipc_trace_class_elem trace_class_elems[] = { {"pipeline", PIPELINE_TRACE_ID}, ..... };
Yes. I have finished this step now. And I also send name string from SOF side to kernel driver.
- The driver will create a debugFS file for each trace class elem.
When read (by rmbox or cat) it will contain the ID number. This way rmbox can map IDs to names for classes too.
I think we may not need so many debugfs interfaces. We can pass one parameter into one trace_level debugfs interface get all trace class map. And rmbox can understand trace class after getting this map.
Ok, you can do 1 file for classes, but components may come and go and may have different params for each so it makes sense to use a different file for each component.
Sure.
- rmbox reads these debugFS files at strartup to create it's ID ->
name mapings.
Steps 2 & 3 are very similar to the component ID name lookups.
Doing this means rmbox and the driver do not need to be modified when we add new trace objects in the FW.
Agree. We can remove all trace class macro definition. both rmbox and kernel driver will be dependent on SOF. So it is scalable fully.
I attach my finished structure decalation for reviwing in the following. It still include trace class. I can remove it in the next version patch set.
922 /* DMA trace level type */ 923 enum sof_dma_trace_level_type { 924 SOF_DMA_TRACE_LEVEL_IRQ = 0, 925 SOF_DMA_TRACE_LEVEL_IPC, 926 SOF_DMA_TRACE_LEVEL_DMA, 927 SOF_DMA_TRACE_LEVEL_SSP, 928 SOF_DMA_TRACE_LEVEL_WAIT, 929 SOF_DMA_TRACE_LEVEL_LOCK, 930 SOF_DMA_TRACE_LEVEL_MEM, 931 SOF_DMA_TRACE_LEVEL_SA, 932 SOF_DMA_TRACE_LEVEL_DMIC, 933 SOF_DMA_TRACE_LEVEL_SCH, 934 SOF_DMA_TRACE_LEVEL_VALUE, 935 SOF_DMA_TRACE_LEVEL_END, 936 }; 937
So these are non component trace class IDs now, they dont need to be in ipc.h and should start at some high number so they wont conflict with component IDs (that start at 0).
Yes. I will keep current class << 24 for global trace. For component id, it will be restored in another unused uint32 bit currently. So they will not cause conflicts.
938 /* DMA for Trace level elem - SOF_IPC_DEBUG_DMA_LEVELS */ 939 struct sof_ipc_dma_trace_level { 940 uint32_t trace_class; 941 enum sof_dma_trace_level_type type; 942 int32_t comp_id; 943 char name[8]; 944 uint32_t level; 945 } __attribute__((packed));
All you need here is name and ID. level, type will be set by userspace.
May we need one default level for this? I also want use comp_id to identify global and component trace level. If comp_id = -1, it is global trace level.
946 947 /* DMA for Trace level info - SOF_IPC_DEBUG_DMA_LEVELS */ 948 struct sof_ipc_dma_trace_levels { 949 struct sof_ipc_ext_data_hdr ext_hdr; 950 uint32_t num_levels;
num_classes
951 struct sof_ipc_dma_trace_level level[0];
trace_class
952 } __attribute__((packed));
We can also use this structure to set and query multiply module/class trace levels. My current focus issue is component/widget trace level list from SOF side. When FW_READY message sent, the topology should not be loadded.
Yes, topology is loaded after FW ready.
Yes.
I may have to send component/widget trace level list later. Is it right?
No, driver will create component trace debugFS objects when it loads topology. Driver will already have this data so no need for FW to send it.
Sure. I will keep previous solution. I will iterate widget list on kernel driver and get the list.
Thanks. Yan Wang
Liam
Thanks. Yan Wang
Liam
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On Fri, 2018-05-18 at 22:24 +0800, sound-open-firmware-bounces@alsa-project.org wrote:
So these are non component trace class IDs now, they dont need to be in ipc.h and should start at some high number so they wont conflict with component IDs (that start at 0).
Yes. I will keep current class << 24 for global trace.
No, ditch this. Just use a single ID with IDs > 0xffff0000 for non components.
For component id, it will be restored in another unused uint32 bit currently. So they will not cause conflicts.
938 /* DMA for Trace level elem - SOF_IPC_DEBUG_DMA_LEVELS */ 939 struct sof_ipc_dma_trace_level { 940 uint32_t trace_class; 941 enum sof_dma_trace_level_type type; 942 int32_t comp_id; 943 char name[8]; 944 uint32_t level; 945 } __attribute__((packed));
All you need here is name and ID. level, type will be set by userspace.
May we need one default level for this? I also want use comp_id to identify global and component trace level. If comp_id = -1, it is global trace level.
No, comp_id is the ID of the target (component or class). No need for global level.
Liam
On 5/19/2018 4:01 AM, Liam Girdwood wrote:
On Fri, 2018-05-18 at 22:24 +0800, sound-open-firmware-bounces@alsa-project.org wrote:
So these are non component trace class IDs now, they dont need to be in ipc.h and should start at some high number so they wont conflict with component IDs (that start at 0).
Yes. I will keep current class << 24 for global trace.
No, ditch this. Just use a single ID with IDs > 0xffff0000 for non components.
Sure. I can do it.
For component id, it will be restored in another unused uint32 bit currently. So they will not cause conflicts.
938 /* DMA for Trace level elem - SOF_IPC_DEBUG_DMA_LEVELS */ 939 struct sof_ipc_dma_trace_level { 940 uint32_t trace_class; 941 enum sof_dma_trace_level_type type; 942 int32_t comp_id; 943 char name[8]; 944 uint32_t level; 945 } __attribute__((packed));
All you need here is name and ID. level, type will be set by userspace.
May we need one default level for this? I also want use comp_id to identify global and component trace level. If comp_id = -1, it is global trace level.
No, comp_id is the ID of the target (component or class). No need for global level.
Sure. I can do it. Thanks.
Yan Wang
Liam _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
participants (4)
-
Liam Girdwood -
Ranjani Sridharan -
Yan Wang -
yanwang