On Wed, Oct 05, 2016 at 11:48:27PM +0100, Lionel Landwerlin wrote:
> This comes handy if you want to look at your application output without
> having to save it into a file. For example, use this with aubinator from
> Mesa :
>
> $ intel_aubdump -c '/path/to/aubinator --gen=hsw' my_gl_app
>
> v2: Fix handling empty command line option
>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
> Cc: Sirisha Gandikota <Sirisha.Gandikota@xxxxxxxxx>
> ---
> tools/aubdump.c | 107 ++++++++++++++++++++++++++++++++++++++++---------
> tools/intel_aubdump.in | 31 +++++++++++++-
> 2 files changed, 117 insertions(+), 21 deletions(-)
>
> diff --git a/tools/aubdump.c b/tools/aubdump.c
> index 30dc742..3b85bc7 100644
> --- a/tools/aubdump.c
> +++ b/tools/aubdump.c
> @@ -50,6 +50,7 @@ static int (*libc_close)(int fd) = close_init_helper;
> static int (*libc_ioctl)(int fd, unsigned long request, ...) = ioctl_init_helper;
>
> static int drm_fd = -1;
> +static char *command;
> static char *filename;
> static FILE *file;
> static int gen = 0;
> @@ -113,6 +114,82 @@ fail_if(int cond, const char *format, ...)
> raise(SIGTRAP);
> }
>
> +static FILE *
> +launch_command(void)
> +{
> + int i = 0, fds[2];
> + char **args = calloc(strlen(command), sizeof(char *));
> + char *iter = command;
> +
> + args[i++] = iter = command;
> +
> + while ((iter = strstr(iter, ",")) != NULL) {
> + *iter = '\0';
> + iter += 1;
> + args[i++] = iter;
> + }
> +
> + if (pipe(fds) == -1)
> + return NULL;
> +
> + switch (fork()) {
> + case 0:
> + dup2(fds[0], 0);
> + fail_if(execv(args[0], args) == -1,
> + "intel_aubdump: fail to launch child command\n");
> + return NULL;
> +
Why not use execvp to avoid having to use an absolute path?
"failed" instead of "fail"
> + default:
> + free(args);
> + return fdopen(fds[1], "w");
> +
> + case -1:
> + return NULL;
> + }
> +}
> +
> +static void
> +maybe_init_output(void)
> +{
> + const char *args;
> + static bool initialized = false;
> + int nb_args;
> +
> + if (initialized)
> + return;
> +
> + args = getenv("INTEL_AUBDUMP_ARGS");
> +
> + nb_args = sscanf(args, "verbose=%d;file=%m[^;];device=%i;command=%m[^;];",
> + &verbose, &filename, &device, &command);
> + if (nb_args != 4) {
> + if (filename)
> + free(filename);
> + nb_args = sscanf(args, "verbose=%d;file=%m[^;];device=%i;",
> + &verbose, &filename, &device);
> + command = strdup("");
> + }
> + fail_if(filename == NULL || command == NULL,
> + "intel_aubdump: out of memory\n");
> + if (device)
> + device_override = true;
> +
Indentation on that fail_if.
> + bos = malloc(MAX_BO_COUNT * sizeof(bos[0]));
> + fail_if(bos == NULL, "intel_aubdump: out of memory\n");
> +
> + if (strlen(command) != 0) {
> + file = launch_command();
> + fail_if(file == NULL,
> + "intel_aubdump: failed to launch command '%s'\n", command);
> + } else {
> + file = fopen(filename, "w+");
> + fail_if(file == NULL,
> + "intel_aubdump: failed to open file '%s'\n", filename);
> + }
> +
> + initialized = true;
> +}
> +
Indentation again.
> static struct bo *
> get_bo(uint32_t handle)
> {
> @@ -140,13 +217,18 @@ align_u64(uint64_t v, uint64_t a)
> static void
> dword_out(uint32_t data)
> {
> - fwrite(&data, 1, 4, file);
> + fail_if(fwrite(&data, 1, 4, file) == 0,
> + "Writing to output failed\n");
> }
>
> static void
> data_out(const void *data, size_t size)
> {
> - fwrite(data, 1, size, file);
> + if (size == 0)
> + return;
> +
> + fail_if(fwrite(data, 1, size, file) == 0,
> + "Writing to output failed\n");
> }
>
> static void
> @@ -447,6 +529,8 @@ ioctl(int fd, unsigned long request, ...)
> }
>
> if (fd == drm_fd) {
> + maybe_init_output();
> +
> switch (request) {
> case DRM_IOCTL_I915_GETPARAM: {
> struct drm_i915_getparam *getparam = argp;
> @@ -550,26 +634,8 @@ ioctl(int fd, unsigned long request, ...)
> static void
> init(void)
> {
> - const char *args = getenv("INTEL_AUBDUMP_ARGS");
> -
> libc_close = dlsym(RTLD_NEXT, "close");
> libc_ioctl = dlsym(RTLD_NEXT, "ioctl");
> - fail_if(libc_close == NULL || libc_ioctl == NULL,
> - "intel_aubdump: failed to get libc ioctl or close\n");
> -
> - if (sscanf(args, "verbose=%d;file=%m[^;];device=%i",
> - &verbose, &filename, &device) != 3)
> - filename = strdup("intel.aub");
> - fail_if(filename == NULL, "intel_aubdump: out of memory\n");
> -
> - if (device)
> - device_override = true;
> -
> - bos = malloc(MAX_BO_COUNT * sizeof(bos[0]));
> - fail_if(bos == NULL, "intel_aubdump: out of memory\n");
> -
> - file = fopen(filename, "w+");
> - fail_if(file == NULL, "intel_aubdump: failed to open file '%s'\n", filename);
> }
>
> static int
> @@ -596,6 +662,7 @@ ioctl_init_helper(int fd, unsigned long request, ...)
> static void __attribute__ ((destructor))
> fini(void)
> {
> + free(command);
> free(filename);
> if (file)
> fclose(file);
> diff --git a/tools/intel_aubdump.in b/tools/intel_aubdump.in
> index 3666b6e..2a1704c 100644
> --- a/tools/intel_aubdump.in
> +++ b/tools/intel_aubdump.in
> @@ -10,6 +10,9 @@ contents and execution of the GEM application.
>
> -o, --output=FILE Name of AUB file. Defaults to COMMAND.aub
>
> + -c, --command=CMD Execute CMD and write the AUB file's content to its
> + standard input
> +
When -c is used, -o is then ignored. Document that here.
> --device=ID Override PCI ID of the reported device
>
> -v Enable verbose output
> @@ -23,6 +26,19 @@ EOF
>
> verbose=0
> device=0
> +file="intel.aub"
> +command=""
> +
> +build_command () {
> + command=""
> + for i in $1; do
> + if [ -z $command ]; then
> + command=$i
> + else
> + command="$command,$i"
> + fi;
> + done
> +}
Passing the command this way feels a bit hairy. What if the command
string has to contain a ',', or a ';'? That should maybe be checked
and error out if there's a nonrepresentable character present.
>
> while true; do
> case "$1" in
> @@ -38,6 +54,14 @@ while true; do
> file=${1##--output=}
> shift
> ;;
> + -c)
> + build_command "$2"
> + shift 2
> + ;;
> + --command=*)
> + build_command "${1##--command=}"
> + shift
> + ;;
Indentation on those build_command lines.
> --device=*)
> device=${1##--device=}
> shift
> @@ -68,6 +92,11 @@ prefix=@prefix@
> exec_prefix=@exec_prefix@
> libdir=@libdir@
>
> +ARGS="verbose=$verbose;file=$file;device=$device"
> +if [ ! -z $command ]; then
> + ARGS="$INTEL_AUBDUMP_ARGS;command=$command;"
> +fi
> +
This needs to be ARGS="$ARGS;command=$command;"
> LD_PRELOAD=${libdir}/intel_aubdump.so${LD_PPRELOAD:+:${LD_PRELOAD}} \
> - INTEL_AUBDUMP_ARGS="verbose=$verbose;file=$file;device=$device" \
> + INTEL_AUBDUMP_ARGS="$ARGS" \
> exec -- "$@"
Indentation looks off.
There's also an old typo on LD_PRELOAD on the line above, feel free to
fix that at the same time.
With those nitpicks addressed,
Reviewed-by: Petri Latvala <petri.latvala@xxxxxxxxx>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
