Re: [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Friday, September 14, 2018 4:57:02 PM PDT Tarun Vyas wrote:
> This tool serves as a wrapper around the constructs provided by the
> drm_dpcd_aux_dev kernel module by working on the /dev/drm_dp_aux[n]
> devices created by the kernel module.
> It supports reading and writing dpcd registers on the connected aux
> channels.
> In the follow-up patch, support for decoding these registers will be
> added to facilate debugging panel related issues.
> 
> v2: (Fixes by Rodrigo but no functional changes yet):
>     - Indentations, Typo, Missed spaces
>     - Removing mentioning to decode and spec that is not implemented yet.
>     - Add Makefile.sources back
>     - Missed s/printf/igt_warn
> 
> v3:
>     - Addres DK's review comments from v2 above.
>     - Squash Rodrigo's file handling unification patch.
>     - Make count, offset and device id optional.
> 
> Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  tools/Makefile.sources |   1 +
>  tools/dpcd_reg.c       | 213
> +++++++++++++++++++++++++++++++++++++++++++++++++ tools/meson.build      | 
>  1 +
>  3 files changed, 215 insertions(+)
>  create mode 100644 tools/dpcd_reg.c
> 
> diff --git a/tools/Makefile.sources b/tools/Makefile.sources
> index abd23a0f..50706f41 100644
> --- a/tools/Makefile.sources
> +++ b/tools/Makefile.sources
> @@ -7,6 +7,7 @@ noinst_PROGRAMS =		\
> 
>  tools_prog_lists =		\
>  	igt_stats		\
> +	dpcd_reg		\
>  	intel_audio_dump	\
>  	intel_reg		\
>  	intel_backlight		\
> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> new file mode 100644
> index 00000000..cd9fed4f
> --- /dev/null
> +++ b/tools/dpcd_reg.c
> @@ -0,0 +1,213 @@
> +/*
> + * Copyright © 2018 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> "Software"), + * to deal in the Software without restriction, including
> without limitation + * the rights to use, copy, modify, merge, publish,
> distribute, sublicense, + * and/or sell copies of the Software, and to
> permit persons to whom the + * Software is furnished to do so, subject to
> the following conditions: + *
> + * The above copyright notice and this permission notice (including the
> next + * paragraph) shall be included in all copies or substantial portions
> of the + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. 
> IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
> CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE
> SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE.
> + *
> + * DPCD register read/write tool
> + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register
> read + * and write, so CONFIG_DRM_DP_AUX_DEV needs to be set.
> + */
> +
> +#include "igt_core.h"
What's the dependency on igt_core here?

> +#include <errno.h>
> +#include <fcntl.h>
> +#include <limits.h>
> +
> +#define MAX_OFFSET	0xf02ff
I think we should be able to allow allow up to the max limit of 0xfffff (DP 
1.4 section 2.9.3)

> +
> +const char aux_dev[] = "/dev/drm_dp_aux";
> +
> +static void print_usage(char *tool, int exit_code)
> +{
> +	printf("DPCD register read and write tool\n\n");
> +	printf("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
> +		 "to be set in the kernel config.\n\n");
I think it would look better to print the above lines only when --help/-h was 
passed.  Move this under  case 'h' ?

> +	printf("Usage: %s [OPTION ...] COMMAND\n\n", tool);
> +	printf("COMMAND is one of:\n");
> +	printf("  read:		Read [count] bytes dpcd reg at an offset\n");
> +	printf("  write:	Write a dpcd reg at an offset\n\n");
> +	printf("Options for the above COMMANDS are\n");
> +	printf(" --device=DEVID		Aux device id, as listed in
> /dev/drm_dp_aux_dev[n]." +					"Defaults to 0\n");
> +	printf(" --offset=REG_ADDR	DPCD register offset in hex. Defaults to
> 0x00\n"); +	printf(" --count=BYTES		For reads, specify number of bytes to
> be read from" +					"the offset. Defaults to 1\n");
> +	printf(" --val			For writes, specify a hex value to be written\n\n");
> +
> +	printf(" --help: print the usage\n");
> +
> +	exit(exit_code);
Modify the callers so that you don't have to exit from a print function. 

> +}
> +
> +static int dpcd_read(int fd, const uint32_t offset, size_t count)
const is not needed as the arguments are passed by value.

> +{
> +	int ret, i;
> +	void *buf = calloc(count, sizeof(uint8_t));
uint8_t *buf ?

> +
> +	if (!buf) {
> +		fprintf(stderr, "Can't allocate read buffer\n");
> +		return ENOMEM;
> +	}
> +
> +	ret = pread(fd, buf, count, offset);
> +	if (ret != count) {
> +		fprintf(stderr, "Failed to read - %s\n", strerror(errno));
Print ret too? If the number of bytes read were lower, errno won't be set.

> +		ret = errno;
> +		goto out;
> +	} else
} else {

> +		ret = EXIT_SUCCESS;
Intialize ret  and get rid of the else block?

> +
> +	printf("Read %zu byte(s) starting at offset %x\n\n", count, offset);
Print this debug message only if the expected number of bytes weren't read and 
something like printf("Read %zu bytes, expected %zu\n");

printf("0x"); to clarify that the printed values are in hex.
> +	for (i = 0; i < count; i++)
> +		printf(" %02x", *(((uint8_t *)(buf)) + i));
You can avoid typecasting if you define the array as type uint8_t

> +	printf("\n");
> +
> +out:
> +	free(buf);
> +	return ret;
> +}
> +
> +static int dpcd_write(int fd, const uint32_t offset, const uint8_t val)
const isn't needed.

> +{
> +	int ret;
> +
> +	ret = pwrite(fd, (const void *)&val, sizeof(uint8_t), offset);
> +	if (ret < 0) {
> +		fprintf(stderr, "Failed to write - %s\n", strerror(errno));
Same as above, print number of bytes written in case of error.

> +		return errno;
> +	} else
> +		return EXIT_SUCCESS;
You could avoid 'else' here.

> +}
> +
> +int main(int argc, char **argv)
> +{
> +	char dev_name[20];
> +	int ret, devid, fd, vflag = 0;
> +	uint32_t offset;
> +	uint8_t val;
> +	size_t count;
> +	int file_op = O_RDONLY;
> +
> +	enum command {
> +		INV = -1,
> +		READ = 2,
Any reason to not use the value compiler generates?

> +		WRITE,
> +	} cmd = INV;
> +
> +	struct option longopts[] = {
> +		{ "count",	required_argument,	NULL,		'c' },
> +		{ "device",	required_argument,	NULL,		'd' },
> +		{ "help",	no_argument,		NULL,		'h' },
> +		{ "offset",	required_argument,	NULL,		'o' },
> +		{ "value",	required_argument,	&vflag,		'v' },
> +		{ 0 }
> +	};
> +
> +	devid = 0, count = 1, offset = 0x0;
> +
> +	while ((ret = getopt_long(argc, argv, "-:c:d:h:o:", longopts, NULL)) !=
Any reason to leave out the option -v ? And '-h' shouldn't need an argument.


> -1) { +		switch (ret) {
> +		case 'c':
> +			count = strtoul(optarg, NULL, 10);
With strtol() you should be able reject all negative args and make use of the 
second argument to reject invalid numbers.

> +			if (count == ULONG_MAX) {
> +				fprintf(stderr, "Count argument too big\n");
> +				exit(ERANGE);
> +			}
> +			break;
> +		case 'd':
> +			devid = strtoul(optarg, NULL, 10);
Same here, strtol() to reject negatives and use **endptr to fail on invalid 
numbers.

> +			if (devid == ULONG_MAX) {
> +				fprintf(stderr, "Devid argument too big\n");

> +				exit(ERANGE);
> +			}
> +			break;
> +		case 'h':
> +			print_usage(argv[0], EXIT_SUCCESS);
> +			break;
> +		case 'o':
> +			offset = strtoul(optarg, NULL, 16);
Same comment as for 'd'.  You might also want to check if errno is set.

> +			if (offset > MAX_OFFSET) {
> +				fprintf(stderr, "Offset should be <= 0xf02ff\n");
> +				exit(ERANGE);
> +			}
> +			break;
> +		case 0:
> +			if (vflag == 'v' && optarg)
I didn't get why &vflag had to be used.

> +				val = (uint8_t) strtoul(optarg, NULL, 16);
Reject values greater than 0xff?

> +			break;
> +		/* Command parsing */
> +		case 1:
> +			if (strcmp(optarg, "read") == 0) {
> +				cmd = READ;
> +			} else if (strcmp(optarg, "write") == 0) {
> +				cmd = WRITE;
> +				file_op = O_WRONLY;
> +			}
> +			break;
 } else {
 and exit here?  or fall through to default block by re-arranging the blocks.

> +		case ':':
> +			fprintf(stderr, "The -%c option of %s requires an argument\n",
> +				 optopt, argv[0]);

Prints "The -c option of tools/dpcd_reg requires an argument", is that what 
you intended? Skip printing the tool path?

> +			print_usage(argv[0], EXIT_FAILURE);
> +		case '?':
> +		default:
> +			fprintf(stderr, "%s - option %c is invalid\n", argv[0],
> +				 optopt);
> +			print_usage(argv[0], EXIT_FAILURE);
> +		}
> +	}
> +
> +	if ((count + offset) > (MAX_OFFSET + 1)) {
> +		fprintf(stderr, "Out of bounds. Count + Offset <= 0xf0300\n");
> +		exit(ERANGE);
> +	}
> +
> +	if ((cmd == WRITE) && (vflag != 'v')) {
> +		fprintf(stderr, "Write value is missing\n");
> +		print_usage(argv[0], EXIT_FAILURE);
> +	}
> +
> +	memset(dev_name, '\0', 20);
Not needed as snprintf includes the null byte,
> +	snprintf(dev_name, strlen(aux_dev) + 3, "%s%d", aux_dev, devid);
Allows a max of 99 for devid, which should be okay but define a macro and 
reject opt args greater than that?

> +
> +	fd = open(dev_name, file_op);
> +	if (fd < 0) {
> +		fprintf(stderr, "Failed to open %s aux device - error: %s\n", dev_name,
> +			strerror(errno));
> +		return errno;
> +	}
> +
> +	switch (cmd) {
> +	case READ:
> +		ret = dpcd_read(fd, offset, count);
> +		break;
> +	case WRITE:
> +		ret = dpcd_write(fd, offset, val);
> +		break;
> +	case INV:
INVALID is easier to understand

> +	default:
> +		fprintf(stderr, "Please specify a command: read/write. See usage\n");
Remove "See usage"

> +		close(fd);
There's another close() below.

> +		print_usage(argv[0], EXIT_FAILURE);
> +	}
> +
> +	close(fd);
> +
> +	return ret;
> +}
> diff --git a/tools/meson.build b/tools/meson.build
> index e4517d66..79f36aa9 100644
> --- a/tools/meson.build
> +++ b/tools/meson.build
> @@ -36,6 +36,7 @@ tools_progs = [
>  	'intel_watermark',
>  	'intel_gem_info',
>  	'intel_gvtg_test',
> +	'dpcd_reg',
>  ]
>  tool_deps = igt_deps

Thanks for working on this.  The tool looks good overall,. just needs some 
minor polishing.

-DK



_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev