Jani Nikula <jani.nik...@intel.com> writes: > On Mon, 08 Jan 2018, Mika Kuoppala <mika.kuopp...@linux.intel.com> wrote: >> Add option to specify engine for register read/write operation. >> If engine is specified, use MI_LOAD_REGISTER_IMM and MI_STORE_REGISTER_IMM >> to write and read register using a batch targeted at that engine. > > Copy-pasting from the man page, we already have the notation: > > "Registers are defined as [(PORTNAME|PORTNUM|MMIO-OFFSET):](REGNAME|REGADDR)." > > Why don't we add this as ENGINE:REGNAME or something instead of an extra > --engine parameter? As a "port". Sure, it's more work, but I really like > the current possibility of reading all types of registers at once. Now > you prevent dumps that would contain both mmio and batch based reads. >
Are you ok with the latest version? As discussed in irc, there are problems with trying to add engines as ports, due to dynamic nature and also that the existing infra relies on PORTNAME being mmio for symbolic register dumping to work correctly. for the wart of if (reg->engine) in read/write paths we gain the benefits of mmio dumps with engines. -Mika > BR, > Jani. > > >> >> v2: no MI_NOOP after BBE (Chris) >> v3: use modern engine names (Chris), use global fd >> >> Cc: Jani Nikula <jani.nik...@intel.com> >> Cc: Chris Wilson <ch...@chris-wilson.co.uk> >> CC: Joonas Lahtinen <joonas.lahti...@linux.intel.com> >> Signed-off-by: Mika Kuoppala <mika.kuopp...@linux.intel.com> >> --- >> tools/intel_reg.c | 156 >> +++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 154 insertions(+), 2 deletions(-) >> >> diff --git a/tools/intel_reg.c b/tools/intel_reg.c >> index 00d2a4a1..7f3494ef 100644 >> --- a/tools/intel_reg.c >> +++ b/tools/intel_reg.c >> @@ -33,6 +33,7 @@ >> #include <unistd.h> >> >> #include "igt.h" >> +#include "igt_gt.h" >> #include "intel_io.h" >> #include "intel_chipset.h" >> >> @@ -73,6 +74,11 @@ struct config { >> >> /* register spec */ >> char *specfile; >> + >> + /* engine to use for lri (write) and srm (read) */ >> + char *engine; >> + int fd; >> + >> struct reg *regs; >> ssize_t regcount; >> >> @@ -236,13 +242,140 @@ static void dump_decode(struct config *config, struct >> reg *reg, uint32_t val) >> } >> } >> >> +static const struct intel_execution_engine2 *find_engine(const char *name, >> + bool *secure) >> +{ >> + const struct intel_execution_engine2 *e; >> + >> + if (strlen(name) < 2) >> + goto out; >> + >> + if (name[0] == '-') { >> + *secure = false; >> + name++; >> + } else { >> + *secure = true; >> + } >> + >> + for (e = intel_execution_engines2; e->name; e++) { >> + if (!strcmp(e->name, name)) >> + return e; >> + } >> + >> +out: >> + fprintf(stderr, "no such engine as '%s'\n", name); >> + >> + fprintf(stderr, "valid engines:"); >> + for (e = intel_execution_engines2; e->name; e++) >> + fprintf(stderr, " %s", e->name); >> + >> + fprintf(stderr, "\n"); >> + >> + exit(EXIT_FAILURE); >> +} >> + >> +static int register_srm(struct config *config, struct reg *reg, >> + uint32_t *val_in) >> +{ >> + const int gen = intel_gen(config->devid); >> + const bool r64b = gen >= 8; >> + const uint32_t ctx = 0; >> + struct drm_i915_gem_exec_object2 obj[2]; >> + struct drm_i915_gem_relocation_entry reloc[1]; >> + struct drm_i915_gem_execbuffer2 execbuf; >> + uint32_t *batch, *r; >> + const struct intel_execution_engine2 *engine; >> + bool secure; >> + int fd, i; >> + uint32_t val; >> + >> + if (config->fd == -1) { >> + config->fd = __drm_open_driver(DRIVER_INTEL); >> + if (config->fd == -1) { >> + fprintf(stderr, "Error opening driver: %s", >> + strerror(errno)); >> + exit(EXIT_FAILURE); >> + } >> + } >> + >> + fd = config->fd; >> + engine = find_engine(config->engine, &secure); >> + >> + memset(obj, 0, sizeof(obj)); >> + obj[0].handle = gem_create(fd, 4096); >> + obj[1].handle = gem_create(fd, 4096); >> + obj[1].relocs_ptr = to_user_pointer(reloc); >> + obj[1].relocation_count = 1; >> + >> + batch = gem_mmap__cpu(fd, obj[1].handle, 0, 4096, PROT_WRITE); >> + gem_set_domain(fd, obj[1].handle, >> + I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU); >> + >> + i = 0; >> + if (val_in) { >> + batch[i++] = MI_NOOP; >> + batch[i++] = MI_NOOP; >> + >> + batch[i++] = MI_LOAD_REGISTER_IMM; >> + batch[i++] = reg->addr; >> + batch[i++] = *val_in; >> + batch[i++] = MI_NOOP; >> + } >> + >> + batch[i++] = 0x24 << 23 | (1 + r64b); /* SRM */ >> + batch[i++] = reg->addr; >> + reloc[0].target_handle = obj[0].handle; >> + reloc[0].presumed_offset = obj[0].offset; >> + reloc[0].offset = i * sizeof(uint32_t); >> + reloc[0].delta = 0; >> + reloc[0].read_domains = I915_GEM_DOMAIN_RENDER; >> + reloc[0].write_domain = I915_GEM_DOMAIN_RENDER; >> + batch[i++] = reloc[0].delta; >> + if (r64b) >> + batch[i++] = 0; >> + >> + batch[i++] = MI_BATCH_BUFFER_END; >> + munmap(batch, 4096); >> + >> + memset(&execbuf, 0, sizeof(execbuf)); >> + execbuf.buffers_ptr = to_user_pointer(obj); >> + execbuf.buffer_count = 2; >> + execbuf.flags = gem_class_instance_to_eb_flags(fd, >> + engine->class, >> + engine->instance); >> + if (secure) >> + execbuf.flags |= I915_EXEC_SECURE; >> + >> + if (config->verbosity > 0) >> + printf("%s: using %sprivileged batch\n", >> + engine->name, >> + secure ? "" : "non-"); >> + >> + execbuf.rsvd1 = ctx; >> + gem_execbuf(fd, &execbuf); >> + gem_close(fd, obj[1].handle); >> + >> + r = gem_mmap__cpu(fd, obj[0].handle, 0, 4096, PROT_READ); >> + gem_set_domain(fd, obj[0].handle, I915_GEM_DOMAIN_CPU, 0); >> + >> + val = r[0]; >> + munmap(r, 4096); >> + >> + gem_close(fd, obj[0].handle); >> + >> + return val; >> +} >> + >> static int read_register(struct config *config, struct reg *reg, uint32_t >> *valp) >> { >> uint32_t val = 0; >> >> switch (reg->port_desc.port) { >> case PORT_MMIO: >> - val = INREG(reg->mmio_offset + reg->addr); >> + if (config->engine) >> + val = register_srm(config, reg, NULL); >> + else >> + val = INREG(reg->mmio_offset + reg->addr); >> break; >> case PORT_PORTIO_VGA: >> iopl(3); >> @@ -299,7 +432,11 @@ static int write_register(struct config *config, struct >> reg *reg, uint32_t val) >> >> switch (reg->port_desc.port) { >> case PORT_MMIO: >> - OUTREG(reg->mmio_offset + reg->addr, val); >> + if (config->engine) { >> + register_srm(config, reg, &val); >> + } else { >> + OUTREG(reg->mmio_offset + reg->addr, val); >> + } >> break; >> case PORT_PORTIO_VGA: >> if (val > 0xff) { >> @@ -641,6 +778,7 @@ static int intel_reg_help(struct config *config, int >> argc, char *argv[]) >> printf(" --spec=PATH Read register spec from directory or file\n"); >> printf(" --mmio=FILE Use an MMIO snapshot\n"); >> printf(" --devid=DEVID Specify PCI device ID for --mmio=FILE\n"); >> + printf(" --engine=[-]ENGINE Use a specific engine to read/write\n"); >> printf(" --all Decode registers for all known platforms\n"); >> printf(" --binary Binary dump registers\n"); >> printf(" --verbose Increase verbosity\n"); >> @@ -758,6 +896,7 @@ enum opt { >> OPT_ALL, >> OPT_BINARY, >> OPT_SPEC, >> + OPT_ENGINE, >> OPT_VERBOSE, >> OPT_QUIET, >> OPT_HELP, >> @@ -771,11 +910,13 @@ int main(int argc, char *argv[]) >> const struct command *command = NULL; >> struct config config = { >> .count = 1, >> + .fd = -1, >> }; >> bool help = false; >> >> static struct option options[] = { >> /* global options */ >> + { "engine", required_argument, NULL, OPT_ENGINE }, >> { "spec", required_argument, NULL, OPT_SPEC }, >> { "verbose", no_argument, NULL, OPT_VERBOSE }, >> { "quiet", no_argument, NULL, OPT_QUIET }, >> @@ -830,6 +971,14 @@ int main(int argc, char *argv[]) >> return EXIT_FAILURE; >> } >> break; >> + case OPT_ENGINE: >> + config.engine = strdup(optarg); >> + if (!config.engine) { >> + fprintf(stderr, "strdup: %s\n", >> + strerror(errno)); >> + return EXIT_FAILURE; >> + } >> + break; >> case OPT_ALL: >> config.all_platforms = true; >> break; >> @@ -898,5 +1047,8 @@ int main(int argc, char *argv[]) >> >> free(config.mmiofile); >> >> + if (config.fd >= 0) >> + close(config.fd); >> + >> return ret; >> } > > -- > Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx