[PATCH umr] Allow * for ipname in --read/--write/--write-bit
Andres Rodriguez
andresx7 at gmail.com
Thu Feb 23 18:41:42 UTC 2017
On 2017-02-23 09:46 AM, Tom St Denis wrote:
> To simplify scripting you can now use * for ipnames as well.
Thanks for the patch, this change is very welcome.
>
> Signed-off-by: Tom St Denis <tom.stdenis at amd.com>
> ---
> doc/umr.1 | 6 ++-
> src/app/main.c | 5 ++-
> src/app/scan.c | 21 ++++++++++
> src/app/set_bit.c | 117 +++++++++++++++++++++++++++---------------------------
> src/app/set_reg.c | 91 +++++++++++++++++++++---------------------
> 5 files changed, 131 insertions(+), 109 deletions(-)
>
> diff --git a/doc/umr.1 b/doc/umr.1
> index 5ed2c3bb93ae..a12dfb0a00ff 100644
> --- a/doc/umr.1
> +++ b/doc/umr.1
> @@ -34,7 +34,7 @@ Look up an MMIO register by address and bitfield decode the value specified.
> Write a value specified in hex to a register specified with a complete
> register path in the form <
> .B asicname.ipname.regname
> ->. For example, fiji.uvd6.mmUVD_CGC_GATE. The value of asicname can be
> +>. For example, fiji.uvd6.mmUVD_CGC_GATE. The value of asicname and/or ipname can be
> .B *
> to simplify scripting. This command can be used multiple times to
> write to multiple registers in a single invocation.
> @@ -52,7 +52,9 @@ command but also allows
> for the regname field.
> .IP "--scan, -s <string>"
> Scan and print an IP block by name, for example,
> -.B uvd5.
> +.B uvd6
> +or
> +.B carrizo.uvd6.
> Can be used multiple times in a single invocation.
> .IP "--ring, -R <string>(from:to)"
> Read the contents of a ring named by the string without the
> diff --git a/src/app/main.c b/src/app/main.c
> index 8364d54c5227..99e014888571 100644
> --- a/src/app/main.c
> +++ b/src/app/main.c
> @@ -364,12 +364,13 @@ int main(int argc, char **argv)
> "\n\t--write, -w <address> <number>\n\t\tWrite a value in hex to a register specified as a register path in the"
> "\n\t\tform <asicname.ipname.regname>. For instance \"tonga.uvd5.mmUVD_SOFT_RESET\"."
> "\n\t\tCan be used multiple times to set multiple registers. You can"
> - "\n\t\tspecify * for asicname to simplify scripts.\n"
> + "\n\t\tspecify * for asicname and/or ipname to simplify scripts.\n"
> "\n\t--writebit, -wb <string> <number>\n\t\tWrite a value in hex to a register bitfield specified as in --write but"
> "\n\t\tthe addition of the bitfield name. For instance: \"*.gfx80.mmRLC_PG_CNTL.PG_OVERRIDE\"\n"
> "\n\t--read, -r <string>\n\t\tRead a value from a register and print it to stdout. This command"
> "\n\t\tuses the same path notation as --write. It also accepts * for regname.\n"
> -"\n\t--scan, -s <string>\n\t\tScan and print an ip block by name, e.g. \"uvd5\". Can be used multiple times.\n"
> +"\n\t--scan, -s <string>\n\t\tScan and print an ip block by name, e.g. \"uvd6\" or \"carrizo.uvd6\"."
> + "\n\t\tCan be used multiple times.\n"
> "\n\t--ring, -R <string>([from:to])\n\t\tRead the contents of a ring named by the string without the amdgpu_ring_ prefix. "
> "\n\t\tBy default it will read and display the entire ring. A starting and ending "
> "\n\t\taddress can be specified in decimal or a '.' can be used to indicate relative "
> diff --git a/src/app/scan.c b/src/app/scan.c
> index 19c97fe1499d..fbb8f5838069 100644
> --- a/src/app/scan.c
> +++ b/src/app/scan.c
> @@ -31,6 +31,27 @@ int umr_scan_asic(struct umr_asic *asic, char *asicname, char *ipname, char *reg
> uint64_t addr, scale;
> char buf[256];
>
> + // if ipname == '*' scan for a match and stop on first one
> + if (ipname[0] == '*' && regname[0] != '*') {
> + for (i = 0; i < asic->no_blocks; i++) {
> + for (j = 0; j < asic->blocks[i]->no_regs; j++) {
> + if (!strcmp(regname, asic->blocks[i]->regs[j].regname) ||
> + (options.many && strstr(asic->blocks[i]->regs[j].regname, regname))) {
> + ipname = asic->blocks[i]->ipname;
> + goto ip_found;
Since the star globs usually imply a wildcard match, I think it is a
little weird to truncate the output to only the first match.
If a star glob is requested by the user, I think it should be okay to
loop and print multiple outputs as usually happens when options.many is
enabled.
Alternative approach would be to print an 'ambiguous argument' error.
That way the user will at least have some feedback. Although I think the
behaviour of printing multiple results is preferable.
Cheers,
Andres
> + }
> + }
> + }
> + }
> +ip_found:
> + if (ipname[0] == '*') {
> + if (regname[0] == '*')
> + fprintf(stderr, "[ERROR]: You cannot specify * for both ipname and regname\n");
> + else
> + fprintf(stderr, "[ERROR]: Register name <%s> was not found with ANY ip name\n", regname);
> + return -1;
> + }
> +
> /* scan them all in order */
> if (!asicname[0] || !strcmp(asicname, "*") || !strcmp(asicname, asic->asicname)) {
> for (i = 0; i < asic->no_blocks; i++) {
> diff --git a/src/app/set_bit.c b/src/app/set_bit.c
> index d9ee7d8f3a55..bed7ee858b6a 100644
> --- a/src/app/set_bit.c
> +++ b/src/app/set_bit.c
> @@ -31,79 +31,78 @@
> */
> int umr_set_register_bit(struct umr_asic *asic, char *regpath, char *regvalue)
> {
> - char rpath[512];
> + char asicname[128], ipname[128], regname[128], bitname[128];
> int i, j, k, fd;
> uint32_t value, mask, copy;
> uint64_t addr, scale;
>
> - /* scan until we compare with regpath... */
> - for (i = 0; i < asic->no_blocks; i++) {
> - if (!memcmp(regpath, "*.", 2))
> - snprintf(rpath, sizeof(rpath)-1, "*.%s.", asic->blocks[i]->ipname);
> - else
> - snprintf(rpath, sizeof(rpath)-1, "%s.%s.", asic->asicname, asic->blocks[i]->ipname);
> - if (memcmp(regpath, rpath, strlen(rpath)) == 0) {
> - for (j = 0; j < asic->blocks[i]->no_regs; j++) {
> - if (asic->blocks[i]->regs[j].bits) {
> - for (k = 0; k < asic->blocks[i]->regs[j].no_bits; k++) {
> - if (!memcmp(regpath, "*.", 2))
> - snprintf(rpath, sizeof(rpath)-1, "*.%s.%s.%s", asic->blocks[i]->ipname, asic->blocks[i]->regs[j].regname, asic->blocks[i]->regs[j].bits[k].regname);
> - else
> - snprintf(rpath, sizeof(rpath)-1, "%s.%s.%s.%s", asic->asicname, asic->blocks[i]->ipname, asic->blocks[i]->regs[j].regname, asic->blocks[i]->regs[j].bits[k].regname);
> - if (!strcmp(regpath, rpath)) {
> - sscanf(regvalue, "%"SCNx32, &value);
> + if (sscanf(regpath, "%[^.].%[^.].%[^.].%[^.]", asicname, ipname, regname, bitname) != 4) {
> + fprintf(stderr, "[ERROR]: Invalid regpath for bit write\n");
> + return -1;
> + }
>
> - mask = (1UL<<((1+asic->blocks[i]->regs[j].bits[k].stop)-asic->blocks[i]->regs[j].bits[k].start))-1;
> - mask <<= asic->blocks[i]->regs[j].bits[k].start;
> - if (asic->pci.mem == NULL) {
> - // set this register
> - switch (asic->blocks[i]->regs[j].type){
> - case REG_MMIO: fd = asic->fd.mmio; scale = 4; break;
> - case REG_DIDT: fd = asic->fd.didt; scale = 1; break;
> - case REG_PCIE: fd = asic->fd.pcie; scale = 1; break;
> - case REG_SMC: fd = asic->fd.smc; scale = 1; break;
> - default: return -1;
> - }
> - if (asic->blocks[i]->grant) {
> - if (asic->blocks[i]->grant(asic)) {
> - fprintf(stderr, "[ERROR] Must specify at least one 'risky' before writing to this block.\n");
> - return -1;
> + if (asicname[0] == '*' || !strcmp(asicname, asic->asicname)) {
> + /* scan until we compare with regpath... */
> + for (i = 0; i < asic->no_blocks; i++) {
> + if (ipname[0] == '*' || !strcmp(ipname, asic->blocks[i]->ipname)) {
> + for (j = 0; j < asic->blocks[i]->no_regs; j++) {
> + if (!strcmp(regname, asic->blocks[i]->regs[j].regname) && asic->blocks[i]->regs[j].bits) {
> + for (k = 0; k < asic->blocks[i]->regs[j].no_bits; k++) {
> + if (!strcmp(bitname, asic->blocks[i]->regs[j].bits[k].regname)) {
> + sscanf(regvalue, "%"SCNx32, &value);
> +
> + mask = (1UL<<((1+asic->blocks[i]->regs[j].bits[k].stop)-asic->blocks[i]->regs[j].bits[k].start))-1;
> + mask <<= asic->blocks[i]->regs[j].bits[k].start;
> + if (asic->pci.mem == NULL) {
> + // set this register
> + switch (asic->blocks[i]->regs[j].type){
> + case REG_MMIO: fd = asic->fd.mmio; scale = 4; break;
> + case REG_DIDT: fd = asic->fd.didt; scale = 1; break;
> + case REG_PCIE: fd = asic->fd.pcie; scale = 1; break;
> + case REG_SMC: fd = asic->fd.smc; scale = 1; break;
> + default: return -1;
> + }
> + if (asic->blocks[i]->grant) {
> + if (asic->blocks[i]->grant(asic)) {
> + fprintf(stderr, "[ERROR] Must specify at least one 'risky' before writing to this block.\n");
> + return -1;
> + }
> }
> - }
>
> - if (options.use_bank && asic->blocks[i]->regs[j].type == REG_MMIO)
> - addr =
> - (1ULL << 62) |
> - (((uint64_t)options.se_bank) << 24) |
> - (((uint64_t)options.sh_bank) << 34) |
> - (((uint64_t)options.instance_bank) << 44);
> - else
> - addr = 0;
> + if (options.use_bank && asic->blocks[i]->regs[j].type == REG_MMIO)
> + addr =
> + (1ULL << 62) |
> + (((uint64_t)options.se_bank) << 24) |
> + (((uint64_t)options.sh_bank) << 34) |
> + (((uint64_t)options.instance_bank) << 44);
> + else
> + addr = 0;
>
> - lseek(fd, addr | (asic->blocks[i]->regs[j].addr*scale), SEEK_SET);
> - read(fd, ©, 4);
> + lseek(fd, addr | (asic->blocks[i]->regs[j].addr*scale), SEEK_SET);
> + read(fd, ©, 4);
>
> - // read-modify-write value back
> - copy &= ~mask;
> - value = (value << asic->blocks[i]->regs[j].bits[k].start) & mask;
> - copy |= value;
> + // read-modify-write value back
> + copy &= ~mask;
> + value = (value << asic->blocks[i]->regs[j].bits[k].start) & mask;
> + copy |= value;
>
> - lseek(fd, addr | (asic->blocks[i]->regs[j].addr<<2), SEEK_SET);
> - write(fd, ©, 4);
> - if (!options.quiet) printf("%s <= 0x%08lx\n", regpath, (unsigned long)copy);
> + lseek(fd, addr | (asic->blocks[i]->regs[j].addr<<2), SEEK_SET);
> + write(fd, ©, 4);
> + if (!options.quiet) printf("%s <= 0x%08lx\n", regpath, (unsigned long)copy);
>
> - if (asic->blocks[i]->release) {
> - if (asic->blocks[i]->release(asic)) {
> - return -1;
> + if (asic->blocks[i]->release) {
> + if (asic->blocks[i]->release(asic)) {
> + return -1;
> + }
> }
> + } else if (asic->blocks[i]->regs[j].type == REG_MMIO) {
> + copy = asic->pci.mem[asic->blocks[i]->regs[j].addr] & ~mask;
> + copy |= (value << asic->blocks[i]->regs[j].bits[k].start) & mask;
> + asic->pci.mem[asic->blocks[i]->regs[j].addr] = copy;
> + if (!options.quiet) printf("%s <= 0x%08lx\n", regpath, (unsigned long)copy);
> }
> - } else if (asic->blocks[i]->regs[j].type == REG_MMIO) {
> - copy = asic->pci.mem[asic->blocks[i]->regs[j].addr] & ~mask;
> - copy |= (value << asic->blocks[i]->regs[j].bits[k].start) & mask;
> - asic->pci.mem[asic->blocks[i]->regs[j].addr] = copy;
> - if (!options.quiet) printf("%s <= 0x%08lx\n", regpath, (unsigned long)copy);
> + return 0;
> }
> - return 0;
> }
> }
> }
> diff --git a/src/app/set_reg.c b/src/app/set_reg.c
> index 9861170d55c3..ed8f708e977f 100644
> --- a/src/app/set_reg.c
> +++ b/src/app/set_reg.c
> @@ -31,64 +31,63 @@
> */
> int umr_set_register(struct umr_asic *asic, char *regpath, char *regvalue)
> {
> - char rpath[512];
> + char asicname[128], ipname[128], regname[128];
> int i, j, fd;
> uint32_t value;
> uint64_t addr, scale;
>
> - /* scan until we compare with regpath... */
> - for (i = 0; i < asic->no_blocks; i++) {
> - if (!memcmp(regpath, "*.", 2))
> - snprintf(rpath, sizeof(rpath)-1, "*.%s.", asic->blocks[i]->ipname);
> - else
> - snprintf(rpath, sizeof(rpath)-1, "%s.%s.", asic->asicname, asic->blocks[i]->ipname);
> - if (memcmp(regpath, rpath, strlen(rpath)) == 0) {
> - for (j = 0; j < asic->blocks[i]->no_regs; j++) {
> - if (!memcmp(regpath, "*.", 2))
> - snprintf(rpath, sizeof(rpath)-1, "*.%s.%s", asic->blocks[i]->ipname, asic->blocks[i]->regs[j].regname);
> - else
> - snprintf(rpath, sizeof(rpath)-1, "%s.%s.%s", asic->asicname, asic->blocks[i]->ipname, asic->blocks[i]->regs[j].regname);
> - if (!strcmp(regpath, rpath)) {
> - sscanf(regvalue, "%"SCNx32, &value);
> + if (sscanf(regpath, "%[^.].%[^.].%[^.]", asicname, ipname, regname) != 3) {
> + fprintf(stderr, "[ERROR]: Invalid regpath for write\n");
> + return -1;
> + }
>
> - if (asic->pci.mem == NULL) {
> - // set this register
> - switch (asic->blocks[i]->regs[j].type){
> - case REG_MMIO: fd = asic->fd.mmio; scale = 4; break;
> - case REG_DIDT: fd = asic->fd.didt; scale = 1; break;
> - case REG_PCIE: fd = asic->fd.pcie; scale = 1; break;
> - case REG_SMC: fd = asic->fd.smc; scale = 1; break;
> - default: return -1;
> - }
> -
> - if (asic->blocks[i]->grant) {
> - if (asic->blocks[i]->grant(asic)) {
> - fprintf(stderr, "[ERROR] Must specify at least one 'risky' before writing to this block.\n");
> - return -1;
> + if (asicname[0] == '*' || !strcmp(asicname, asic->asicname)) {
> + // scan all ip blocks for matching entry
> + for (i = 0; i < asic->no_blocks; i++) {
> + if (ipname[0] == '*' || !strcmp(ipname, asic->blocks[i]->ipname)) {
> + for (j = 0; j < asic->blocks[i]->no_regs; j++) {
> + if (!strcmp(regname, asic->blocks[i]->regs[j].regname)) {
> + sscanf(regvalue, "%"SCNx32, &value);
> +
> + if (asic->pci.mem == NULL) {
> + // set this register
> + switch (asic->blocks[i]->regs[j].type){
> + case REG_MMIO: fd = asic->fd.mmio; scale = 4; break;
> + case REG_DIDT: fd = asic->fd.didt; scale = 1; break;
> + case REG_PCIE: fd = asic->fd.pcie; scale = 1; break;
> + case REG_SMC: fd = asic->fd.smc; scale = 1; break;
> + default: return -1;
> + }
> +
> + if (asic->blocks[i]->grant) {
> + if (asic->blocks[i]->grant(asic)) {
> + fprintf(stderr, "[ERROR] Must specify at least one 'risky' before writing to this block.\n");
> + return -1;
> + }
> }
> - }
>
> - if (options.use_bank && asic->blocks[i]->regs[j].type == REG_MMIO)
> - addr =
> - (1ULL << 62) |
> - (((uint64_t)options.se_bank) << 24) |
> - (((uint64_t)options.sh_bank) << 34) |
> - (((uint64_t)options.instance_bank) << 44);
> - else
> - addr = 0;
> + if (options.use_bank && asic->blocks[i]->regs[j].type == REG_MMIO)
> + addr =
> + (1ULL << 62) |
> + (((uint64_t)options.se_bank) << 24) |
> + (((uint64_t)options.sh_bank) << 34) |
> + (((uint64_t)options.instance_bank) << 44);
> + else
> + addr = 0;
>
> - lseek(fd, addr | (asic->blocks[i]->regs[j].addr*scale), SEEK_SET);
> - write(fd, &value, 4);
> + lseek(fd, addr | (asic->blocks[i]->regs[j].addr*scale), SEEK_SET);
> + write(fd, &value, 4);
>
> - if (asic->blocks[i]->release) {
> - if (asic->blocks[i]->release(asic)) {
> - return -1;
> + if (asic->blocks[i]->release) {
> + if (asic->blocks[i]->release(asic)) {
> + return -1;
> + }
> }
> + } else if (asic->blocks[i]->regs[j].type == REG_MMIO) {
> + asic->pci.mem[asic->blocks[i]->regs[j].addr] = value;
> }
> - } else if (asic->blocks[i]->regs[j].type == REG_MMIO) {
> - asic->pci.mem[asic->blocks[i]->regs[j].addr] = value;
> + return 0;
> }
> - return 0;
> }
> }
> }
>
More information about the amd-gfx
mailing list