Re: [PATCH i-g-t v3] overlay: parse tracepoints from sysfs to figure out fields' location

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Dec 19, 2017 at 12:00:58PM +0000, Lionel Landwerlin wrote:
> With changes going to drm-tip, the tracepoints field locations are
> going to change. This change introduces a tracepoint parser (using a
> peg parser) which lets us figure out field positions on the fly.
> 
> v2: Fix automake build (Lionel)
> 
> v3: Make overlay build conditional on peg (Petri)
>     Make wait_end callback more readable (Chris)
>     Drop tracepoint_id(), instead parsing from format file (Lionel)
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
> ---
>  configure.ac                  |   7 ++
>  overlay/Makefile.am           |   5 ++
>  overlay/gpu-perf.c            | 163 ++++++++++++++++++++++++++++++++----------
>  overlay/meson.build           |  15 +++-
>  overlay/tracepoint_format.leg |  35 +++++++++
>  5 files changed, 185 insertions(+), 40 deletions(-)
>  create mode 100644 overlay/tracepoint_format.leg
> 
> diff --git a/configure.ac b/configure.ac
> index 8740f7a4..2d2ed8ce 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -146,6 +146,13 @@ if test x"$build_x86" = xyes; then
>  	AS_IF([test x"$LEX" != "x:" -a x"$YACC" != xyacc],
>  		[enable_assembler=yes],
>  		[enable_assembler=no])
> +
> +	AC_CHECK_TOOL([LEG], [leg])
> +	if test "x$LEG" != "xleg"; then
> +		AC_MSG_NOTICE([leg command missing, disabling overlay; try : apt-get install peg])
> +		enable_overlay_xvlib="no"
> +		enable_overlay_xlib="no"
> +	fi
>  else
>  	enable_overlay_xvlib="no"
>  	enable_overlay_xlib="no"


My earlier testing was insufficient. There's a bug lurking that we need to fix at the same time:


@@ -165,7 +165,7 @@ AM_CONDITIONAL(BUILD_ASSEMBLER, [test "x$enable_assembler" = xyes])
 
 AM_CONDITIONAL(BUILD_OVERLAY_XVLIB, [test "x$enable_overlay_xvlib" = xyes])
 AM_CONDITIONAL(BUILD_OVERLAY_XLIB, [test "x$enable_overlay_xlib" = xyes])
-AM_CONDITIONAL(BUILD_OVERLAY, [test "x$enable_overlay_xlib" = xyes -o "x$enable_overlay_xvlib"])
+AM_CONDITIONAL(BUILD_OVERLAY, [test "x$enable_overlay_xlib" = xyes -o "x$enable_overlay_xvlib" = xyes])
 if test x$enable_overlay_xvlib = xyes; then
 	AC_DEFINE(HAVE_OVERLAY_XVLIB, 1, [Enable XV backend])
 fi





> diff --git a/overlay/Makefile.am b/overlay/Makefile.am
> index fca04cae..0f553b7c 100644
> --- a/overlay/Makefile.am
> +++ b/overlay/Makefile.am
> @@ -1,7 +1,12 @@
>  if BUILD_OVERLAY
>  bin_PROGRAMS = intel-gpu-overlay
> +
> +BUILT_SOURCES = tracepoint_format.h
>  endif
>  
> +tracepoint_format.h: tracepoint_format.leg
> +	$(LEG) -o $@ $<
> +
>  AM_CPPFLAGS = -I. -I$(top_srcdir)/include/drm-uapi
>  AM_CFLAGS = $(DRM_CFLAGS) $(PCIACCESS_CFLAGS) $(CWARNFLAGS) \
>  	$(CAIRO_CFLAGS) $(OVERLAY_CFLAGS) $(WERROR_CFLAGS) -I$(srcdir)/../lib
> diff --git a/overlay/gpu-perf.c b/overlay/gpu-perf.c
> index 3d4a9be9..057e83eb 100644
> --- a/overlay/gpu-perf.c
> +++ b/overlay/gpu-perf.c
> @@ -57,39 +57,124 @@ struct sample_event {
>  	uint64_t time;
>  	uint64_t id;
>  	uint32_t raw_size;
> -	uint32_t raw_hdr0;
> -	uint32_t raw_hdr1;
> -	uint32_t raw[0];
> +	uint8_t tracepoint_data[0];
>  };
>  
>  enum {
> -	DEVICE = 0,
> -	CTX,
> -	ENGINE,
> -	CTX_SEQNO,
> -	GLOBAL_SEQNO
> +	TP_GEM_REQUEST_ADD,
> +	TP_GEM_REQUEST_WAIT_BEGIN,
> +	TP_GEM_REQUEST_WAIT_END,
> +	TP_FLIP_COMPLETE,
> +	TP_GEM_RING_SYNC_TO,
> +	TP_GEM_RING_SWITCH_CONTEXT,
> +
> +	TP_NB
>  };
>  
> -static uint64_t tracepoint_id(const char *sys, const char *name)
> +struct tracepoint {
> +	const char *name;
> +	int event_id;
> +
> +	struct {
> +		char name[128];
> +		int offset;
> +		int size;
> +		int is_signed;
> +	} fields[20];
> +	int n_fields;
> +
> +	int device_field;
> +	int ctx_field;
> +	int ring_field;
> +	int seqno_field;
> +	int global_seqno_field;
> +	int plane_field;
> +} tracepoints[TP_NB] = {
> +	[TP_GEM_REQUEST_ADD]         = { .name = "i915/i915_gem_request_add", },
> +	[TP_GEM_REQUEST_WAIT_BEGIN]  = { .name = "i915/i915_gem_request_wait_begin", },
> +	[TP_GEM_REQUEST_WAIT_END]    = { .name = "i915/i915_gem_request_wait_end", },
> +	[TP_FLIP_COMPLETE]           = { .name = "i915/flip_complete", },
> +	[TP_GEM_RING_SYNC_TO]        = { .name = "i915/gem_ring_sync_to", },
> +	[TP_GEM_RING_SWITCH_CONTEXT] = { .name = "i915/gem_ring_switch_context", },
> +};
> +
> +union parser_value {
> +    char *string;
> +    int integer;
> +};
> +
> +struct parser_ctx {
> +	struct tracepoint *tp;
> +	FILE *fp;
> +};
> +
> +#define YY_CTX_LOCAL
> +#define YY_CTX_MEMBERS struct parser_ctx ctx;
> +#define YYSTYPE union parser_value
> +#define YY_PARSE(T) static T
> +#define YY_INPUT(yy, buf, result, max)				\
> +	{							\
> +		int c = getc(yy->ctx.fp);			\
> +		result = (EOF == c) ? 0 : (*(buf)= c, 1);	\
> +		if (EOF != c) {					\
> +			yyprintf((stderr, "<%c>", c));		\
> +		}						\
> +	}
> +
> +#include "tracepoint_format.h"
> +
> +static int
> +tracepoint_id(int tp_id)
>  {
> +	struct tracepoint *tp = &tracepoints[tp_id];
> +	yycontext ctx;
>  	char buf[1024];
> -	int fd, n;
>  
> -	snprintf(buf, sizeof(buf), "%s/tracing/events/%s/%s/id", debugfs_path, sys, name);
> -	fd = open(buf, 0);
> -	if (fd < 0)
> -		return 0;
> -	n = read(fd, buf, sizeof(buf)-1);
> -	close(fd);
> -	if (n < 0)
> +	/* Already parsed? */
> +	if (tp->event_id != 0)
> +		return tp->event_id;
> +
> +	snprintf(buf, sizeof(buf), "%s/tracing/events/%s/format",
> +		 debugfs_path, tp->name);
> +
> +	memset(&ctx, 0, sizeof(ctx));
> +	ctx.ctx.tp = tp;
> +	ctx.ctx.fp = fopen(buf, "r");
> +
> +	if (ctx.ctx.fp == NULL)
>  		return 0;
>  
> -	buf[n] = '\0';
> -	return strtoull(buf, 0, 0);
> +	if (yyparse(&ctx)) {
> +		for (int f = 0; f < tp->n_fields; f++) {
> +			if (!strcmp(tp->fields[f].name, "device")) {
> +				tp->device_field = f;
> +			} else if (!strcmp(tp->fields[f].name, "ctx")) {
> +				tp->ctx_field = f;
> +			} else if (!strcmp(tp->fields[f].name, "ring")) {
> +				tp->ring_field = f;
> +			} else if (!strcmp(tp->fields[f].name, "seqno")) {
> +				tp->seqno_field = f;
> +			} else if (!strcmp(tp->fields[f].name, "global_seqno")) {
> +				tp->global_seqno_field = f;
> +			} else if (!strcmp(tp->fields[f].name, "plane")) {
> +				tp->plane_field = f;
> +			}
> +		}
> +	} else
> +		tp->event_id = tp->n_fields = 0;
> +
> +	yyrelease(&ctx);
> +	fclose(ctx.ctx.fp);
> +
> +	return tp->event_id;
>  }
>  
> -static int perf_tracepoint_open(struct gpu_perf *gp,
> -				const char *sys, const char *name,
> +#define READ_TP_FIELD_U32(sample, tp_id, field_name)			\
> +	(*(const uint32_t *)((sample)->tracepoint_data +		\
> +			     tracepoints[tp_id].fields[			\
> +				     tracepoints[tp_id].field_name##_field].offset))
> +
> +static int perf_tracepoint_open(struct gpu_perf *gp, int tp_id,
>  				int (*func)(struct gpu_perf *, const void *))
>  {
>  	struct perf_event_attr attr;
> @@ -99,7 +184,7 @@ static int perf_tracepoint_open(struct gpu_perf *gp,
>  	memset(&attr, 0, sizeof (attr));
>  
>  	attr.type = PERF_TYPE_TRACEPOINT;
> -	attr.config = tracepoint_id(sys, name);
> +	attr.config = tracepoint_id(tp_id);
>  	if (attr.config == 0)
>  		return ENOENT;
>  
> @@ -227,7 +312,7 @@ static int request_add(struct gpu_perf *gp, const void *event)
>  	if (comm == NULL)
>  		return 0;
>  
> -	comm->nr_requests[sample->raw[ENGINE]]++;
> +	comm->nr_requests[READ_TP_FIELD_U32(sample, TP_GEM_REQUEST_ADD, ring)]++;
>  	return 1;
>  }
>  
> @@ -235,7 +320,7 @@ static int flip_complete(struct gpu_perf *gp, const void *event)
>  {
>  	const struct sample_event *sample = event;
>  
> -	gp->flip_complete[sample->raw[0]]++;
> +	gp->flip_complete[READ_TP_FIELD_U32(sample, TP_FLIP_COMPLETE, plane)]++;
>  	return 1;
>  }
>  
> @@ -243,7 +328,7 @@ static int ctx_switch(struct gpu_perf *gp, const void *event)
>  {
>  	const struct sample_event *sample = event;
>  
> -	gp->ctx_switch[sample->raw[ENGINE]]++;
> +	gp->ctx_switch[READ_TP_FIELD_U32(sample, TP_GEM_RING_SWITCH_CONTEXT, ring)]++;
>  	return 1;
>  }
>  
> @@ -278,11 +363,11 @@ static int wait_begin(struct gpu_perf *gp, const void *event)
>  
>  	wait->comm = comm;
>  	wait->comm->active = true;
> -	wait->context = sample->raw[ENGINE];
> -	wait->seqno = sample->raw[CTX_SEQNO];
> +	wait->context = READ_TP_FIELD_U32(sample, TP_GEM_REQUEST_WAIT_BEGIN, ctx);
> +	wait->seqno = READ_TP_FIELD_U32(sample, TP_GEM_REQUEST_WAIT_BEGIN, seqno);
>  	wait->time = sample->time;
> -	wait->next = gp->wait[sample->raw[CTX]];
> -	gp->wait[sample->raw[CTX]] = wait;
> +	wait->next = gp->wait[READ_TP_FIELD_U32(sample, TP_GEM_REQUEST_WAIT_BEGIN, ring)];
> +	gp->wait[READ_TP_FIELD_U32(sample, TP_GEM_REQUEST_WAIT_BEGIN, ring)] = wait;
>  
>  	return 0;
>  }
> @@ -291,10 +376,12 @@ static int wait_end(struct gpu_perf *gp, const void *event)
>  {
>  	const struct sample_event *sample = event;
>  	struct gpu_perf_time *wait, **prev;
> +	uint32_t engine = READ_TP_FIELD_U32(sample, TP_GEM_REQUEST_WAIT_END, ring);
> +	uint32_t context = READ_TP_FIELD_U32(sample, TP_GEM_REQUEST_WAIT_END, ctx);
> +	uint32_t seqno = READ_TP_FIELD_U32(sample, TP_GEM_REQUEST_WAIT_END, seqno);
>  
> -	for (prev = &gp->wait[sample->raw[ENGINE]]; (wait = *prev) != NULL; prev = &wait->next) {
> -		if (wait->context != sample->raw[CTX] ||
> -		    wait->seqno != sample->raw[CTX_SEQNO])
> +	for (prev = &gp->wait[engine]; (wait = *prev) != NULL; prev = &wait->next) {
> +		if (wait->context != context || wait->seqno != seqno)
>  			continue;
>  
>  		wait->comm->wait_time += sample->time - wait->time;
> @@ -314,12 +401,12 @@ void gpu_perf_init(struct gpu_perf *gp, unsigned flags)
>  	gp->nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
>  	gp->page_size = getpagesize();
>  
> -	perf_tracepoint_open(gp, "i915", "i915_gem_request_add", request_add);
> -	if (perf_tracepoint_open(gp, "i915", "i915_gem_request_wait_begin", wait_begin) == 0)
> -		perf_tracepoint_open(gp, "i915", "i915_gem_request_wait_end", wait_end);
> -	perf_tracepoint_open(gp, "i915", "i915_flip_complete", flip_complete);
> -	perf_tracepoint_open(gp, "i915", "i915_gem_ring_sync_to", ring_sync);
> -	perf_tracepoint_open(gp, "i915", "i915_gem_ring_switch_context", ctx_switch);
> +	perf_tracepoint_open(gp, TP_GEM_REQUEST_ADD, request_add);
> +	if (perf_tracepoint_open(gp, TP_GEM_REQUEST_WAIT_BEGIN, wait_begin) == 0)
> +		perf_tracepoint_open(gp, TP_GEM_REQUEST_WAIT_END, wait_end);
> +	perf_tracepoint_open(gp, TP_FLIP_COMPLETE, flip_complete);
> +	perf_tracepoint_open(gp, TP_GEM_RING_SYNC_TO, ring_sync);
> +	perf_tracepoint_open(gp, TP_GEM_RING_SWITCH_CONTEXT, ctx_switch);
>  
>  	if (gp->nr_events == 0) {
>  		gp->error = "i915.ko tracepoints not available";
> diff --git a/overlay/meson.build b/overlay/meson.build
> index afacff5e..57d8fa61 100644
> --- a/overlay/meson.build
> +++ b/overlay/meson.build
> @@ -26,7 +26,7 @@ gpu_overlay_deps = [ realtime, math, cairo, pciaccess, libdrm,
>  
>  both_x11_src = ''
>  
> -gpu_overlay_cflags = []
> +gpu_overlay_cflags = [ '-g3', '-O0' ]

Why is this needed?



Can you silence this warning?

In file included from ../overlay/gpu-perf.c:124:0:
overlay/tracepoint_format.h:242:15: warning: ‘yyAccept’ defined but not used [-Wunused-function]
 YY_LOCAL(int) yyAccept(yycontext *yy, int tp0)
                ^~~~~~~~


For the build system changes,

Acked-by: Petri Latvala <petri.latvala@xxxxxxxxx>

-- 
Petri Latvala



>  if xv.found() and x11.found() and xext.found() and dri2proto.found()
>  	both_x11_src = 'x11/position.c'
>  	gpu_overlay_src += [
> @@ -51,7 +51,18 @@ gpu_overlay_src += both_x11_src
>  
>  gpu_overlay_src += 'kms/kms-overlay.c'
>  
> -if xrandr.found() and cairo.found()
> +leg = find_program('leg', required : false)
> +if leg.found()
> +	leg_file = custom_target('tracepoint_format',
> +		output: 'tracepoint_format.h',
> +		input: 'tracepoint_format.leg',
> +		command: [leg, '-P', '-o', '@OUTPUT@', '@INPUT@'])
> +	gpu_overlay_src += leg_file
> +else
> +	warning('leg command not found, disabling overlay; try : apt-get install peg')
> +endif
> +
> +if leg.found() and xrandr.found() and cairo.found()
>  	executable('intel-gpu-overlay', gpu_overlay_src,
>  			include_directories : inc,
>  			c_args : gpu_overlay_cflags,
> diff --git a/overlay/tracepoint_format.leg b/overlay/tracepoint_format.leg
> new file mode 100644
> index 00000000..7a09149d
> --- /dev/null
> +++ b/overlay/tracepoint_format.leg
> @@ -0,0 +1,35 @@
> +TracepointFmt =
> +    'name' ':' Space n:PropertyName EndLine
> +        { free(v.string); }
> +    'ID:' Space v:Number EndLine
> +        { yy->ctx.tp->event_id = v.integer; }
> +    'format:' EndLine
> +    Field+
> +    'print fmt:' [^.]* !.
> +
> +Field         = Space (Property ';' Space)+ EndLine
> +        { yy->ctx.tp->n_fields++; }
> +              | EndLine
> +
> +Property      = 'offset' ':' v:Number
> +        { yy->ctx.tp->fields[yy->ctx.tp->n_fields].offset = v.integer; }
> +              | 'size' ':' v:Number
> +        { yy->ctx.tp->fields[yy->ctx.tp->n_fields].size = v.integer; }
> +              | 'signed' ':' v:Number
> +        { yy->ctx.tp->fields[yy->ctx.tp->n_fields].is_signed = v.integer != 0; }
> +              | 'field' ':' v:PropertyValue
> +        { snprintf(yy->ctx.tp->fields[yy->ctx.tp->n_fields].name,
> +                   sizeof(yy->ctx.tp->fields[yy->ctx.tp->n_fields].name),
> +                   "%s", strrchr(v.string, ' ') + 1); free(v.string); }
> +              | n:PropertyName ':' v:PropertyValue
> +        { free(n.string); free(v.string); }
> +
> +PropertyName  = < [A-Za-z0-9_]+ >
> +        { $$.string = strdup(yytext); }
> +PropertyValue = < [^;]+ >
> +        { $$.string = strdup(yytext); }
> +Number        = < [0-9]+ >
> +        { $$.integer = atoi(yytext); }
> +
> +EndLine       = [\n]
> +Space         = [ \t]*
> -- 
> 2.15.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux