On Mon, 22 Jan 2018, Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx> wrote:
> Jani Nikula <jani.nikula@xxxxxxxxx> writes:
>
>> On Mon, 08 Jan 2018, Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx> 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.
Can't say I would've reviewed all the engine bits and pieces, but
Acked-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>
> -Mika
>
>> BR,
>> Jani.
>>
>>
>>>
>>> v2: no MI_NOOP after BBE (Chris)
>>> v3: use modern engine names (Chris), use global fd
>>>
>>> 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 | 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
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
