Re: [PATCH] intel_reg: Move decoding behind an option

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 16 Apr 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> Decoding the register can be only as good as the reg_spec being used.
> The current builtin register spec is not that. Move that decoding behind
> an option to make it better for the normal case when running from the
> repo checkout where the external reg spec is not available. Passing any
> of --decode, --all, --spec brings the old behavior back:
>
> 	$ sudo ./build/tools/intel_reg --decode read 0x2358
> 	Warning: stat '/usr/local/share/igt-gpu-tools/registers' failed: No such file or directory. Using builtin register spec.
> 					    (0x00002358): 0x00000000
>
> vs the new behavior:
>
> 	$ sudo ./build/tools/intel_reg --decode read 0x2358
> 					    (0x00002358): 0x00000000
>
> We could probably reduce the leading space since we won't have any name,
> but that can be improved later.
>
> v2: Instead of removing the warning, move the whole decoding logic
>     behind a command line option

There are commands dump and list that operate on the input from the spec
file. They'd be useless without --decode, and it's kind of silly to
require the user to specify that when it's required.

I think you need to add a .decode field to struct command, move reg spec
reading after command parsing, and set config.decode to command->decode
if the latter is true.

Otherwise LGTM.

BR,
Jani.


>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  tools/intel_reg.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/tools/intel_reg.c b/tools/intel_reg.c
> index 6c37e14d1..1b6a07aca 100644
> --- a/tools/intel_reg.c
> +++ b/tools/intel_reg.c
> @@ -68,6 +68,9 @@ struct config {
>  	/* write: do a posting read */
>  	bool post;
>  
> +	/* decode registers, otherwise use just raw values */
> +	bool decode;
> +
>  	/* decode register for all platforms */
>  	bool all_platforms;
>  
> @@ -195,7 +198,7 @@ static bool port_is_mmio(enum port_addr port)
>  	}
>  }
>  
> -static void dump_decode(struct config *config, struct reg *reg, uint32_t val)
> +static void dump_regval(struct config *config, struct reg *reg, uint32_t val)
>  {
>  	char decode[1300];
>  	char tmp[1024];
> @@ -206,8 +209,11 @@ static void dump_decode(struct config *config, struct reg *reg, uint32_t val)
>  	else
>  		*bin = '\0';
>  
> -	intel_reg_spec_decode(tmp, sizeof(tmp), reg, val,
> -			      config->all_platforms ? 0 : config->devid);
> +	if (config->decode)
> +		intel_reg_spec_decode(tmp, sizeof(tmp), reg, val,
> +				      config->all_platforms ? 0 : config->devid);
> +	else
> +		*tmp = '\0';
>  
>  	if (*tmp) {
>  		/* We have a decode result, and maybe binary decode. */
> @@ -573,7 +579,7 @@ static void dump_register(struct config *config, struct reg *reg)
>  	uint32_t val;
>  
>  	if (read_register(config, reg, &val) == 0)
> -		dump_decode(config, reg, val);
> +		dump_regval(config, reg, val);
>  }
>  
>  static int write_register(struct config *config, struct reg *reg, uint32_t val)
> @@ -941,7 +947,7 @@ static int intel_reg_decode(struct config *config, int argc, char *argv[])
>  			continue;
>  		}
>  
> -		dump_decode(config, &reg, val);
> +		dump_regval(config, &reg, val);
>  	}
>  
>  	return EXIT_SUCCESS;
> @@ -1037,10 +1043,11 @@ static int intel_reg_help(struct config *config, int argc, char *argv[])
>  	printf("\n\n");
>  
>  	printf("OPTIONS common to most COMMANDS:\n");
> -	printf(" --spec=PATH    Read register spec from directory or file\n");
> +	printf(" --spec=PATH    Read register spec from directory or file. Implies --decode\n");
>  	printf(" --mmio=FILE    Use an MMIO snapshot\n");
>  	printf(" --devid=DEVID  Specify PCI device ID for --mmio=FILE\n");
> -	printf(" --all          Decode registers for all known platforms\n");
> +	printf(" --decode       Decode registers\n");
> +	printf(" --all          Decode registers for all known platforms. Implies --decode\n");
>  	printf(" --binary       Binary dump registers\n");
>  	printf(" --verbose      Increase verbosity\n");
>  	printf(" --quiet        Reduce verbosity\n");
> @@ -1106,6 +1113,9 @@ static int read_reg_spec(struct config *config)
>  	struct stat st;
>  	int r;
>  
> +	if (!config->decode)
> +		return 0;
> +
>  	path = config->specfile;
>  	if (!path)
>  		path = getenv("INTEL_REG_SPEC");
> @@ -1154,6 +1164,7 @@ enum opt {
>  	OPT_DEVID,
>  	OPT_COUNT,
>  	OPT_POST,
> +	OPT_DECODE,
>  	OPT_ALL,
>  	OPT_BINARY,
>  	OPT_SPEC,
> @@ -1188,6 +1199,7 @@ int main(int argc, char *argv[])
>  		/* options specific to write */
>  		{ "post",	no_argument,		NULL,	OPT_POST },
>  		/* options specific to read, dump and decode */
> +		{ "decode",	no_argument,		NULL,	OPT_DECODE },
>  		{ "all",	no_argument,		NULL,	OPT_ALL },
>  		{ "binary",	no_argument,		NULL,	OPT_BINARY },
>  		{ 0 }
> @@ -1223,6 +1235,7 @@ int main(int argc, char *argv[])
>  			config.post = true;
>  			break;
>  		case OPT_SPEC:
> +			config.decode = true;
>  			config.specfile = strdup(optarg);
>  			if (!config.specfile) {
>  				fprintf(stderr, "strdup: %s\n",
> @@ -1232,6 +1245,10 @@ int main(int argc, char *argv[])
>  			break;
>  		case OPT_ALL:
>  			config.all_platforms = true;
> +			config.decode = true;
> +			break;
> +		case OPT_DECODE:
> +			config.decode = true;
>  			break;
>  		case OPT_BINARY:
>  			config.binary = true;

-- 
Jani Nikula, Intel