Re: [Sound-open-firmware] [PATCH 1/2] Implement DMA trace level control.

On Thu, 2018-04-12 at 11:05 -0500, Pierre-Louis Bossart wrote:

On 4/12/18 2:46 AM, yan.wang@linux.intel.com wrote:

...

From: Yan Wang yan.wang@linux.intel.com

For DMA trace level control module by module, check its trace class when the trace adding API is called. And it also need one bit field collection to save trace level status for every module.

Signed-off-by: Yan Wang yan.wang@linux.intel.com

Test with: Mininow max rt5651 and APL UP^2 nocodec and CNL nocodec SOF master: 949c4339ab8cb10f2edd497a671af989d7ea80f3 SOF-Tool master: 8e8cd884c36e297f5996a7bb4f1e830a0f1ee84c https://github.com/plbossart/sound/tree/topic/sof-v4.14: 2cd03d26b66f8fee25b8b21ea3f60db11def5b13

---

src/include/sof/dma-trace.h |  57 +++++++++++++++++++++++   src/lib/dma-trace.c         | 109 ++++++++++++++++++++++++++++++++++++++++++++   2 files changed, 166 insertions(+)

diff --git a/src/include/sof/dma-trace.h b/src/include/sof/dma- trace.h index 81ce0a2..265214c 100644 --- a/src/include/sof/dma-trace.h +++ b/src/include/sof/dma-trace.h @@ -43,6 +43,60 @@   #include <platform/platform.h>   #include <platform/timer.h>    +/* DMA trace level */ +enum dma_trace_level_type {

• DMA_TRACE_LEVEL_IRQ = 0,
• DMA_TRACE_LEVEL_IPC,
• DMA_TRACE_LEVEL_PIPE,
• DMA_TRACE_LEVEL_HOST,
• DMA_TRACE_LEVEL_DAI,
• DMA_TRACE_LEVEL_DMA,
• DMA_TRACE_LEVEL_SSP,
• DMA_TRACE_LEVEL_COMP,
• DMA_TRACE_LEVEL_WAIT,
• DMA_TRACE_LEVEL_LOCK,
• DMA_TRACE_LEVEL_MEM,
• DMA_TRACE_LEVEL_MIXER,
• DMA_TRACE_LEVEL_BUFFER,
• DMA_TRACE_LEVEL_VOLUME,
• DMA_TRACE_LEVEL_SWITCH,
• DMA_TRACE_LEVEL_MUX,
• DMA_TRACE_LEVEL_SRC,
• DMA_TRACE_LEVEL_TONE,
• DMA_TRACE_LEVEL_EQ_FIR,
• DMA_TRACE_LEVEL_EQ_IIR,
• DMA_TRACE_LEVEL_SA,
• DMA_TRACE_LEVEL_VALUE,
• DMA_TRACE_LEVEL_ALL,

It makes no sense to me to put these definitions in a private header (on  both kernel and firmware sides). There should be a shared file that is  updated at the same time on both side.

I can move it to sof-ipc.h which is shared between kernel and firmware side.

But at the same time, I am concerned about adding new definitions for  every component just for tracing. this mixes infrastructure and  processing components, this looks unmanageable to me.

Based on previous Liam's comments, I will change it. For component type, trace level will based on component name. Thanks.

Yan Wang

...

+};

+#define BIT(x) (1 << (x))

+#define DMA_TRACE_LEVEL_MASK_IRQ BIT(DMA_TRACE_LEVEL_IRQ) +#define DMA_TRACE_LEVEL_MASK_IPC BIT(DMA_TRACE_LEVEL_IPC) +#define DMA_TRACE_LEVEL_MASK_PIPE BIT(DMA_TRACE_LEVEL_PIPE) +#define DMA_TRACE_LEVEL_MASK_HOST BIT(DMA_TRACE_LEVEL_HOST) +#define DMA_TRACE_LEVEL_MASK_DAI BIT(DMA_TRACE_LEVEL_DAI) +#define DMA_TRACE_LEVEL_MASK_DMA BIT(DMA_TRACE_LEVEL_DMA) +#define DMA_TRACE_LEVEL_MASK_SSP BIT(DMA_TRACE_LEVEL_SSP) +#define DMA_TRACE_LEVEL_MASK_COMP BIT(DMA_TRACE_LEVEL_COMP) +#define DMA_TRACE_LEVEL_MASK_WAIT BIT(DMA_TRACE_LEVEL_WAIT) +#define DMA_TRACE_LEVEL_MASK_LOCK BIT(DMA_TRACE_LEVEL_LOCK) +#define DMA_TRACE_LEVEL_MASK_MEM BIT(DMA_TRACE_LEVEL_MEM) +#define DMA_TRACE_LEVEL_MASK_MIXER BIT(DMA_TRACE_LEVEL_MIXE R) +#define DMA_TRACE_LEVEL_MASK_BUFFER BIT(DMA_TRACE_LEVEL_BUF FER) +#define DMA_TRACE_LEVEL_MASK_VOLUME BIT(DMA_TRACE_LEVEL_VOL UME) +#define DMA_TRACE_LEVEL_MASK_SWITCH BIT(DMA_TRACE_LEVEL_SWI TCH) +#define DMA_TRACE_LEVEL_MASK_MUX BIT(DMA_TRACE_LEVEL_MUX) +#define DMA_TRACE_LEVEL_MASK_SRC BIT(DMA_TRACE_LEVEL_SRC) +#define DMA_TRACE_LEVEL_MASK_TONE BIT(DMA_TRACE_LEVEL_TONE) +#define DMA_TRACE_LEVEL_MASK_EQ_FIR BIT(DMA_TRACE_LEVEL_EQ_ FIR) +#define DMA_TRACE_LEVEL_MASK_EQ_IIR BIT(DMA_TRACE_LEVEL_EQ_ IIR) +#define DMA_TRACE_LEVEL_MASK_SA BIT(DMA_TRACE_LEVEL _SA) +#define DMA_TRACE_LEVEL_MASK_VALUE BIT(DMA_TRACE_LEVEL_VALU E)

+#define DMA_TRACE_LEVEL_MASK_ALL 0xFFFFFFFF

struct dma_trace_buf {    void *w_ptr; /* buffer write pointer */    void *r_ptr; /* buffer read position */ @@ -65,6 +119,7 @@ struct dma_trace_data {    uint32_t enabled;    uint32_t copy_in_progress;    uint32_t stream_tag;

• uint32_t trace_level;

spinlock_t lock;   };    @@ -73,6 +128,8 @@ int dma_trace_init_complete(struct dma_trace_data *d);   int dma_trace_host_buffer(struct dma_trace_data *d, struct dma_sg_elem *elem,    uint32_t host_size);   int dma_trace_enable(struct dma_trace_data *d); +int dma_trace_set_level(struct dma_trace_data *d,

• 	enum dma_trace_level_type type, uint32_t
  

level);   void dma_trace_flush(void *t);      void dtrace_event(const char *e, uint32_t size); diff --git a/src/lib/dma-trace.c b/src/lib/dma-trace.c index 01a9925..e143aed 100644 --- a/src/lib/dma-trace.c +++ b/src/lib/dma-trace.c @@ -269,6 +269,23 @@ static int dma_trace_start(struct dma_trace_data *d)      #endif    +int dma_trace_set_level(struct dma_trace_data *d,

• 	enum dma_trace_level_type type, uint32_t
  

level) +{

• if (type == DMA_TRACE_LEVEL_ALL) {

• d->trace_level = (level == 1) ? 0xFFFFFFFF : 0;
  

• return 0;
  

• }
• /* set trace level based on module */
• if (level == 1)

• d->trace_level |= (1 << type);
  

• else

• d->trace_level &= ~(1 << type);
  

• return 0;

+}

int dma_trace_enable(struct dma_trace_data *d)   {   #if defined CONFIG_DMA_GW @@ -290,6 +307,7 @@ int dma_trace_enable(struct dma_trace_data *d)    }       d->enabled = 1;

• d->trace_level = 0xFFFFFFFF;

work_schedule_default(&d->dmat_work, DMA_TRACE_PERIOD);    return 0;   } @@ -354,6 +372,91 @@ static void dtrace_add_event(const char *e, uint32_t length)    trace_data->messages++;   }    +static int check_trace_level(const char *e, uint32_t length) +{

• uint64_t *dt = (uint64_t *)e;
• uint32_t class, mask;
• if (trace_data->trace_level == 0)

• return 0;
  

• else if (trace_data->trace_level == 0xffffffff)

• return 1;
  

• /* check whether this event should be sent */
• class = (uint32_t)(*(dt + 1)) & 0xff000000;
• switch (class) {
• case TRACE_CLASS_IRQ:

• mask = DMA_TRACE_LEVEL_MASK_IRQ;
  

• break;
  

• case TRACE_CLASS_IPC:

• mask = DMA_TRACE_LEVEL_MASK_IPC;
  

• break;
  

• case TRACE_CLASS_PIPE:

• mask = DMA_TRACE_LEVEL_MASK_PIPE;
  

• break;
  

• case TRACE_CLASS_HOST:

• mask = DMA_TRACE_LEVEL_MASK_HOST;
  

• break;
  

• case TRACE_CLASS_DAI:

• mask = DMA_TRACE_LEVEL_MASK_DAI;
  

• break;
  

• case TRACE_CLASS_DMA:

• mask = DMA_TRACE_LEVEL_MASK_DMA;
  

• break;
  

• case TRACE_CLASS_SSP:

• mask = DMA_TRACE_LEVEL_MASK_SSP;
  

• break;
  

• case TRACE_CLASS_COMP:

• mask = DMA_TRACE_LEVEL_MASK_COMP;
  

• break;
  

• case TRACE_CLASS_WAIT:

• mask = DMA_TRACE_LEVEL_MASK_WAIT;
  

• break;
  

• case TRACE_CLASS_LOCK:

• mask = DMA_TRACE_LEVEL_MASK_LOCK;
  

• break;
  

• case TRACE_CLASS_MEM:

• mask = DMA_TRACE_LEVEL_MASK_MEM;
  

• break;
  

• case TRACE_CLASS_MIXER:

• mask = DMA_TRACE_LEVEL_MASK_MIXER;
  

• break;
  

• case TRACE_CLASS_BUFFER:

• mask = DMA_TRACE_LEVEL_MASK_BUFFER;
  

• break;
  

• case TRACE_CLASS_VOLUME:

• mask = DMA_TRACE_LEVEL_MASK_VOLUME;
  

• break;
  

• case TRACE_CLASS_SWITCH:

• mask = DMA_TRACE_LEVEL_MASK_SWITCH;
  

• break;
  

• case TRACE_CLASS_MUX:

• mask = DMA_TRACE_LEVEL_MASK_MUX;
  

• break;
  

• case TRACE_CLASS_SRC:

• mask = DMA_TRACE_LEVEL_MASK_SRC;
  

• break;
  

• case TRACE_CLASS_TONE:

• mask = DMA_TRACE_LEVEL_MASK_TONE;
  

• break;
  

• case TRACE_CLASS_EQ_FIR:

• mask = DMA_TRACE_LEVEL_MASK_EQ_FIR;
  

• break;
  

• case TRACE_CLASS_EQ_IIR:

• mask = DMA_TRACE_LEVEL_MASK_EQ_IIR;
  

• break;
  

• case TRACE_CLASS_SA:

• mask = DMA_TRACE_LEVEL_MASK_SA;
  

• break;
  

• default:

• mask = DMA_TRACE_LEVEL_MASK_VALUE;
  

• break;
  

• }
• return (trace_data->trace_level & mask) ? 1 : 0;

+}

void dtrace_event(const char *e, uint32_t length)   {    struct dma_trace_buf *buffer = NULL; @@ -363,6 +466,9 @@ void dtrace_event(const char *e, uint32_t length)    length > DMA_TRACE_LOCAL_SIZE / 8 || length == 0)    return;

• if (!check_trace_level(e, length))

• return;
  

buffer = &trace_data->dmatb;       spin_lock_irq(&trace_data->lock, flags); @@ -395,5 +501,8 @@ void dtrace_event_atomic(const char *e, uint32_t length)    length > DMA_TRACE_LOCAL_SIZE / 8 || length == 0)    return;

• if (!check_trace_level(e, length))

• return;
  

dtrace_add_event(e, length);   }