Quoting Mika Kuoppala (2017-12-01 14:16:39)
> 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.
>
> v2: no MI_NOOP after BBE (Chris)
>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> CC: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> ---
> tools/intel_reg.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 137 insertions(+), 2 deletions(-)
>
> diff --git a/tools/intel_reg.c b/tools/intel_reg.c
> index 00d2a4a1..ec45c2c9 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,12 @@ struct config {
>
> /* register spec */
> char *specfile;
> +
> + /* engine to use for lri (write) and srm (read) */
> + char *engine;
> + /* use secure batch */
> + bool engine_secure_batch;
> +
> struct reg *regs;
> ssize_t regcount;
>
> @@ -236,13 +243,116 @@ static void dump_decode(struct config *config, struct reg *reg, uint32_t val)
> }
> }
>
> +static const struct intel_execution_engine *find_engine(const char *name)
> +{
Being modern, we should be using the "$class$instance" pattern (e.g. vcs1).
> + const struct intel_execution_engine *e;
> +
> + for (e = intel_execution_engines; e->name; e++) {
> + if (!strcmp(e->name, name))
> + return e;
> + }
> +
> + fprintf(stderr, "no such engine as '%s'\n", name);
> +
> + fprintf(stderr, "valid engines:");
> + for (e = intel_execution_engines; 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(intel_get_drm_devid(config->drm_fd));
> + 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_engine *engine;
> + int i915, i;
> + uint32_t val;
> + i915 = config->drm_fd;
> +
> + engine = find_engine(config->engine);
> +
> + memset(obj, 0, sizeof(obj));
> + obj[0].handle = gem_create(i915, 4096);
> + obj[1].handle = gem_create(i915, 4096);
> + obj[1].relocs_ptr = to_user_pointer(reloc);
> + obj[1].relocation_count = 1;
> +
> + batch = gem_mmap__cpu(i915, obj[1].handle, 0, 4096, PROT_WRITE);
> + gem_set_domain(i915, 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 = engine->exec_id;
> +
> + if (config->engine_secure_batch) {
> + execbuf.flags |= I915_EXEC_SECURE;
> +
> + if (config->verbosity > 0)
> + printf("%s: using priviledged (secure) batch\n",
privileged
Scrap saying (secure) to the users. It's blatantly not at this point if
we allow any old (root) user to write any old register.
> + engine->name);
> + }
> +
> + execbuf.rsvd1 = ctx;
> + gem_execbuf(i915, &execbuf);
> + gem_close(i915, obj[1].handle);
> +
> + r = gem_mmap__cpu(i915, obj[0].handle, 0, 4096, PROT_READ);
> + gem_set_domain(i915, obj[0].handle, I915_GEM_DOMAIN_CPU, 0);
> +
> + val = r[0];
> +
> + gem_close(i915, 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 +409,15 @@ 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) {
> + ret = register_srm(config, reg, &val);
> + if (config->verbosity > 0 && ret != val)
> + fprintf(stderr,
> + "value readback 0x%08x != 0x%08x\n",
> + ret, val);
Not reading back the same value is quite common. Don't we normally print
out the value after writing anyway? i.e. I don't see why the LRI/SRM path
is special in this regard.
> + } else {
> + OUTREG(reg->mmio_offset + reg->addr, val);
> + }
> break;
> case PORT_PORTIO_VGA:
> if (val > 0xff) {
> @@ -641,6 +759,8 @@ 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(" --secure Use secure batch to do engine 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 +878,8 @@ enum opt {
> OPT_ALL,
> OPT_BINARY,
> OPT_SPEC,
> + OPT_ENGINE,
> + OPT_SECURE,
> OPT_VERBOSE,
> OPT_QUIET,
> OPT_HELP,
> @@ -776,6 +898,8 @@ int main(int argc, char *argv[])
>
> static struct option options[] = {
> /* global options */
> + { "engine", required_argument, NULL, OPT_ENGINE },
> + { "secure", no_argument, NULL, OPT_SECURE},
Ah, I see. I would probably just encode it into the engine name, +rcs0
I'm failing to think of a switch name that makes sense to me. We're
being run as root with the express intent of writing a register, it
seems to me that by default we should be trying to write the register
(and hence use privileged instructions). So maybe it should be
--non-privileged, or -rcs0 to drop the privileges.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
