On 13 June 2017 at 02:55, Mike Leach mike.leach@linaro.org wrote:

Perf drivers now insert barrier packets into trace data blocks where

buffer

has wrapped to ensure that the decode can spot when perf is concatenating blocks. This activates the logic for spotting these barriers in the decoder.

Signed-off-by: Mike Leach mike.leach@linaro.org

Good day Mike,

---

tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c

b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c

index b4cbf4c..e72d207 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -481,6 +481,11 @@ struct cs_etm_decoder *cs_etm_decoder__new(uint32_t

num_cpu, struct cs_etm_decod

     flags |= (d_params->hsyncs ? OCSD_DFRMTR_HAS_HSYNCS : 0);
     flags |= (d_params->frame_aligned ? OCSD_DFRMTR_FRAME_MEM_ALIGN

: 0);

•   /* Drivers may add barrier frames when used with perf,
  

•    *   set up to handle this.
  

•    */
  

•   flags |= OCSD_DFRMTR_RESET_ON_4X_FSYNC;
  

Are barrier packets treated differently on 3x? Any reason to have a 4x specific flag? If we need two separate flags then the above snippet should go in the switch statement that checks the protocol. That way we can use the same function to create a new decoder on 3x.

Thanks, Mathieu

     /* Create decode tree for the data source */
     decoder->dcd_tree = ocsd_create_dcd_tree(format,flags);

-- 2.7.4

On 15 June 2017 at 11:07, Mike Leach mike.leach@linaro.org wrote:

Hi Mathieu,

The '4X" refers to the fact that the barrier packet is an FSYNC packet repeated 4 times. The barrier is related to the trace frame protocol not any ETM version protocol.

Ok, so the code has been inserted at the right place - the explanation you have above should be added to the comment.

Mathieu

Regards

Mike

On 15 June 2017 at 17:47, Mathieu Poirier mathieu.poirier@linaro.org wrote:

...

On 13 June 2017 at 02:55, Mike Leach mike.leach@linaro.org wrote:

...

Perf drivers now insert barrier packets into trace data blocks where buffer has wrapped to ensure that the decode can spot when perf is

concatenating

...

...

blocks. This activates the logic for spotting these barriers in the decoder.

Signed-off-by: Mike Leach mike.leach@linaro.org

Good day Mike,

...

---

tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c index b4cbf4c..e72d207 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -481,6 +481,11 @@ struct cs_etm_decoder *cs_etm_decoder__new(uint32_t num_cpu, struct cs_etm_decod flags |= (d_params->hsyncs ? OCSD_DFRMTR_HAS_HSYNCS : 0); flags |= (d_params->frame_aligned ?

OCSD_DFRMTR_FRAME_MEM_ALIGN :

...

...

0);

•   /* Drivers may add barrier frames when used with perf,
  

•    *   set up to handle this.
  

•    */
  

•   flags |= OCSD_DFRMTR_RESET_ON_4X_FSYNC;
  

Are barrier packets treated differently on 3x? Any reason to have a 4x specific flag? If we need two separate flags then the above snippet

should

...

go in the switch statement that checks the protocol. That way we can use the same function to create a new decoder on 3x.

Thanks, Mathieu

...

     /* Create decode tree for the data source */
     decoder->dcd_tree = ocsd_create_dcd_tree(format,flags);

-- 2.7.4

-- Mike Leach Principal Engineer, ARM Ltd. Blackburn Design Centre. UK

On 13 June 2017 at 02:55, Mike Leach mike.leach@linaro.org wrote:

Adds a list structure to the decoder data to allow each decoder CoreSight ID be associated with the callback context.

This allows the tracking of the source of the packets in the perf --dump command.

Signed-off-by: Mike Leach mike.leach@linaro.org

tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 60 ++++++++++++++++++++++--- 1 file changed, 55 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c index e72d207..bbe6923 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -18,6 +18,7 @@

#include <linux/err.h> #include <stdlib.h> +#include <linux/list.h>

#include "../cs-etm.h" #include "cs-etm-decoder.h" @@ -30,6 +31,13 @@

#define MAX_BUFFER 1024

+struct cs_etm_decoder;

+struct cs_etm_channel {

•   struct cs_etm_decoder   *decoder;
  

•   unsigned char           cs_id;
  

•   struct list_head        chan_list;
  

+};

struct cs_etm_decoder @@ -47,8 +55,10 @@ struct cs_etm_decoder uint32_t head; uint32_t tail; uint32_t end_tail;

•   struct cs_etm_channel   channel_list;
  
  

Here all "channel_list" is used for is to hold the list_head "chan_list". As such this should be a "struct list_head" rather than a "struct cs_etm_channel".

};

static uint32_t cs_etm_decoder__mem_access(const void *context, const ocsd_vaddr_t address, const ocsd_mem_space_acc_t mem_space, @@ -236,9 +246,10 @@ static ocsd_datapath_resp_t cs_etm_decoder__etmv4i_packet_printer( ocsd_datapath_resp_t ret = OCSD_RESP_CONT; char packet_str[PACKET_STR_LEN]; size_t offset;

•    struct cs_etm_decoder *decoder = (struct cs_etm_decoder *)
  

context;

•   struct cs_etm_channel *channel = (struct cs_etm_channel *) context;
  

•   struct cs_etm_decoder *decoder = channel->decoder;
  
  

•    sprintf(packet_str,"%ld: ", (long int) indx);
  

•   sprintf(packet_str, "%ld: id[%02X] ", (long int) indx,
  

channel->cs_id); offset = strlen(packet_str);

     switch(op) {

@@ -250,10 +261,17 @@ static ocsd_datapath_resp_t cs_etm_decoder__etmv4i_packet_printer( ret = OCSD_RESP_FATAL_INVALID_PARAM; break; case OCSD_OP_EOT:

•            sprintf(packet_str,"**** END OF TRACE ****\n");
  

•           sprintf(packet_str, "**** END OF TRACE id[%02X] ****",
  

•                   channel->cs_id);
             break;
     case OCSD_OP_FLUSH:
  

•           sprintf(packet_str, "**** FLUSH DECODER id[%02X] ****",
  

•                   channel->cs_id);
  

•           break;
     case OCSD_OP_RESET:
  

•           sprintf(packet_str, "**** RESET DECODER id[%02X] ****",
  

•                   channel->cs_id);
  

•           break;
     default:
             break;
     }
  

@@ -262,7 +280,22 @@ static ocsd_datapath_resp_t cs_etm_decoder__etmv4i_packet_printer(

     return ret;

}

+static struct cs_etm_channel *cs_etm_decoder__create_channel_item(

•   struct cs_etm_decoder *decoder, unsigned char cs_id)
  

+{

•   struct cs_etm_channel *chan;
  

•   chan = (struct cs_etm_channel *)zalloc(sizeof(struct
  

cs_etm_channel));

•   if (chan == NULL)
  

•           return NULL;
  

•   chan->decoder = decoder;
  

•   chan->cs_id = cs_id;
  

•   list_add(&(chan->chan_list), &(decoder->channel_list.chan_list));
  

•   return chan;
  

+}

static int cs_etm_decoder__create_etmv4i_packet_printer(struct cs_etm_decoder_params *d_params, struct cs_etm_trace_params *t_params,

                                              struct cs_etm_decoder

*decoder) @@ -270,6 +303,7 @@ static int cs_etm_decoder__create_etmv4i_packet_printer(struct cs_etm_decoder_pa ocsd_etmv4_cfg trace_config; int ret = 0; unsigned char CSID; /* CSID extracted from the config data */

•   struct  cs_etm_channel *channel;
  
     if (d_params->packet_printer == NULL)
             return -1;
  

@@ -290,11 +324,15 @@ static int cs_etm_decoder__create_etmv4i_packet_printer(struct cs_etm_decoder_pa if (ret != 0) return -1;

•   channel = cs_etm_decoder__create_channel_item(decoder, CSID);
  

•   if (channel == NULL)
  

•           return -1;
  

•   ret = ocsd_dt_attach_packet_callback(decoder->dcd_tree,
                                      CSID,
                                      OCSD_C_API_CB_PKT_SINK,
                                      cs_etm_decoder__etmv4i_packet_
  

printer,

•                                     decoder);
  

•                                     channel);
     return ret;
  

}

@@ -471,6 +509,9 @@ struct cs_etm_decoder *cs_etm_decoder__new(uint32_t num_cpu, struct cs_etm_decod return NULL; }

•   /* init the channel list */
  

•   INIT_LIST_HEAD(&(decoder->channel_list.chan_list));
  

•    decoder->state.data = d_params->data;
     decoder->prev_return = OCSD_RESP_CONT;
     cs_etm_decoder__clear_buffer(decoder);
  

@@ -527,10 +568,19 @@ struct cs_etm_decoder *cs_etm_decoder__new(uint32_t num_cpu, struct cs_etm_decod

void cs_etm_decoder__free(struct cs_etm_decoder *decoder) {

•   struct cs_etm_channel   *tmp;
  

•   struct list_head        *pos, *q;
  

•    if (decoder == NULL) return;
  
     ocsd_destroy_dcd_tree(decoder->dcd_tree);
     decoder->dcd_tree = NULL;
  
  

•   list_for_each_safe(pos, q, &(decoder->channel_list.chan_list)) {
  

•           tmp = list_entry(pos, struct cs_etm_channel, chan_list);
  

•           list_del(pos);
  

•           free(tmp);
  

•   }
  

•    free(decoder);
  

}

2.7.4

On 13 June 2017 at 02:55, Mike Leach mike.leach@linaro.org wrote:

Adds a compile-time option to print out the raw trace data bytes when dumping the trace packets using the perf report --dump.

Unpacked raw trace data is output by default, but packed trace frame output can be added.

Controlled by environment var "CSTRACE_RAW", checked in Makefile.config

Signed-off-by: Mike Leach mike.leach@linaro.org

tools/perf/Makefile.config | 6 ++++ tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 47 +++++++++++++++++++++++-- 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config index 432e25f..6a53e4b 100644 --- a/tools/perf/Makefile.config +++ b/tools/perf/Makefile.config @@ -586,6 +586,12 @@ ifdef CSTRACE_PATH $(call detected,CSTRACE) $(call detected_var,CSTRACE_PATH) EXTLIBS += -L$(CSTRACE_LIB_PATH) $(LIBCSTRACE) -lstdc++

• ifdef CSTRACE_RAW
• CFLAGS += -DCS_DEBUG_RAW
• ifeq (${CSTRACE_RAW}, packed)

•  CFLAGS += -DCS_RAW_PACKED
  

• endif
• endif

endif

ifdef NO_LIBPERL diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c index bbe6923..d5d39c0 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -31,6 +31,16 @@

#define MAX_BUFFER 1024

+/* use raw logging */ +#ifdef CS_DEBUG_RAW +#define CS_LOG_RAW_FRAMES +#ifdef CS_RAW_PACKED +#define CS_RAW_DEBUG_FLAGS (OCSD_DFRMTR_UNPACKED_RAW_OUT | OCSD_DFRMTR_PACKED_RAW_OUT) +#else +#define CS_RAW_DEBUG_FLAGS (OCSD_DFRMTR_UNPACKED_RAW_OUT) +#endif +#endif

The above two hunks won't be pleasant to upstream... Do we absolutely need packed and unpacked mode?

struct cs_etm_decoder;

struct cs_etm_channel { @@ -269,8 +279,7 @@ static ocsd_datapath_resp_t cs_etm_decoder__etmv4i_packet_printer( channel->cs_id); break; case OCSD_OP_RESET:

•           sprintf(packet_str, "**** RESET DECODER id[%02X] ****",
  

•                   channel->cs_id);
  

•           sprintf(packet_str+offset, "**** RESET DECODER ****");
  
  

Code added in an earlier patch and deleted in a subsequent one is tolerated only when it shows a progression and ease review. Here the changes are small enough that is it now required - please put this in the previous patch.

            break;
     default:
             break;

@@ -281,6 +290,16 @@ static ocsd_datapath_resp_t cs_etm_decoder__etmv4i_packet_printer( return ret; }

+#ifdef CS_LOG_RAW_FRAMES +static void cs_etm_decoder__print_str_cb(const void *p_context,

•                                    const char *psz_msg_str,
  

•                                    const int str_len)
  

+{

•   if (p_context && str_len)
  

•           ((struct cs_etm_decoder *)p_context)->packet_printer(
  

psz_msg_str); +} +#endif

static struct cs_etm_channel *cs_etm_decoder__create_channel_item( struct cs_etm_decoder *decoder, unsigned char cs_id) { @@ -530,6 +549,30 @@ struct cs_etm_decoder *cs_etm_decoder__new(uint32_t num_cpu, struct cs_etm_decod /* Create decode tree for the data source */ decoder->dcd_tree = ocsd_create_dcd_tree(format,flags);

+#ifdef CS_LOG_RAW_FRAMES

•   /* Only log these during a --dump operation */
  

•   if (d_params->operation == CS_ETM_OPERATION_PRINT) {
  

•           /* set up a library default logger to process the
  

•            *  raw frame printer we add later
  

•            */
  

•           ocsd_def_errlog_init(OCSD_ERR_SEV_ERROR, 1);
  

•           /* no stdout / err / file output */
  

•           ocsd_def_errlog_config_output(C_API_MSGLOGOUT_FLG_NONE,
  

NULL);

•           /* set the string CB for the default logger,
  

•            * passes strings to perf print logger.
  

•            */
  

•           ocsd_def_errlog_set_strprint_cb(decoder->dcd_tree,
  

•                                           (void *)decoder,
  

cs_etm_decoder__print_str_cb);

•           /* use the built in library printer for the raw frames */
  

•           ocsd_dt_set_raw_frame_printer(decoder->dcd_tree,
  

•                                         CS_RAW_DEBUG_FLAGS);
  

•   }
  

+#endif

The goal is to reduce the amount of #if/#endif in the code. Take this hunk, create a function with it and move it to the previous hunk. Then create an empty function with the same name and put it in and #else condition. That way said function can be called in cs_etm_decoder__new() without adding conditional defines.

Thanks for the code, Mathieu

     if (decoder->dcd_tree == 0) {
             goto err_free_decoder;
     }

-- 2.7.4