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

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

On Fri, 2018-08-31 at 21:18 -0700, Rodrigo Vivi wrote:
> From: Tarun Vyas <tarun.vyas@intel.com>
> 
> 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
> 
> 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       | 209
> +++++++++++++++++++++++++++++++++++++++++
>  tools/meson.build      |   1 +
>  3 files changed, 211 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..39dde2c9
> --- /dev/null
> +++ b/tools/dpcd_reg.c
> @@ -0,0 +1,209 @@
> +/*
> + * 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"
> +#include <errno.h>
> +#include <fcntl.h>
> +
> +#define INVALID	0xff
> +
> +const char aux_dev[] = "/dev/drm_dp_aux";
> +
> +static void print_usage(char *tool, int help)
> +{
> +	igt_info("DPCD register read and write tool\n\n");
> +	igt_info("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
> +		 "to be set in the kernel config.\n\n");
> +	igt_info("Usage: %s [OPTION ...] COMMAND\n\n", tool);
> +	igt_info("COMMAND is one of:\n");
> +	igt_info("  read:	Read [count] bytes dpcd reg at an
> offset\n");
> +	igt_info("  write:	Write a dpcd reg at an
> offset\n\n");
> +	igt_info("Options for the above COMMANDS are\n");
> +	igt_info(" --device=DEVID 	Aux device id, as listed
> in /dev/drm_dp_aux_dev[n]\n");
> +	igt_info(" --offset=REG_ADDR	DPCD register offset in
> hex\n");
> +	igt_info(" --count=BYTES	For reads, specify number of
> bytes to be read from the offset\n");
> +	igt_info(" --val=BYTE		For writes, specify a
> BYTE sized value to be writteni\n\n");
I didn't get this. Isn't the size always 1 byte?

> +
> +	igt_info(" --help: print the usage\n");
> +
> +	exit((help == 1)? EXIT_SUCCESS : EXIT_FAILURE);
> +}
> +
> +static int dpcd_read(char *device, const uint32_t offset, size_t
> count)
> +{
> +	int fd, ret, i;
> +	void *buf = malloc(sizeof(uint8_t) * count);
> +
> +	if (NULL != buf)
> +		memset(buf, 0, sizeof(uint8_t) * count);
calloc()

> +	else {
> +		igt_warn("Can't allocate read buffer\n");
> +		return EXIT_FAILURE;
> +	}
> +
> +	fd = open(device, O_RDONLY);
> +	if (fd > 0) {
0 is valid, isn't it?

> +		ret = pread(fd, buf, count, offset);
> +		close(fd);
> +		if (ret != count) {
> +			igt_warn("Failed to read from %s aux device
> - error %s\n", device, strerror(errno));
> +			ret = EXIT_FAILURE;
> +			goto out;
> +		}

Check errno and return to the user.

> +
> +		igt_info("Read %zu byte(s) starting at offset
> %x\n\n", count, offset);
igt_info() isn't needed, any reason not to print to stdout directly?
Applies to igt_foo() functions you are using.

> +		for (i = 0; i < count; i++)
> +			igt_info("%x\n", *(((uint8_t *)(buf)) + i));
> +	}
> +	else {
> +		igt_warn("Failed to open %s aux device - error:
> %s\n", device, strerror(errno));
> +		ret = EXIT_FAILURE;
> +	}
> +out:
> +	free(buf);
> +	return ret;
> +}
> +
> +static int dpcd_write(char *device, const uint32_t offset, const
> uint8_t *val)
> +{
> +	int fd, ret;
> +
> +	fd = open(device, O_RDWR);
> +	if (fd > 0) {
> +		ret = pwrite(fd, (const void *)val, sizeof(uint8_t),
 
Why pass val as a pointer if it's a just a 1 byte value?


> offset);

Offset needs checks for boundaries.

> +		close(fd);
> +		if (ret < 0) {
> +			igt_warn("Failed to write to %s aux device -
> error %s\n", device, strerror(errno));
> +			ret = EXIT_FAILURE;
> +			goto out;
> +		}
> +		ret = dpcd_read(device, offset, sizeof(uint8_t));
> +	}
Why read back? Let the user decide to read back.

> +	else {
> +		igt_warn("Failed to open %s aux device - error:
> %s\n", device, strerror(errno));
> +		ret = EXIT_FAILURE;
> +	}
> +out:
> +	return ret;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	char dev_name[20];
> +	int ret, devid, help_flg;
> +	uint32_t offset;
> +	uint8_t val;
> +	size_t count;
> +
> +	enum command {
> +		INV = -1,
> +		READ = 2,
> +		WRITE,
> +	} cmd = INV;
> +
> +	struct option longopts [] = {
> +		{
> "count",      required_argument,      NULL,      'c' },
> +		{ "device",	required_argument,	NULL, 

Make this optional and default to device ID 0?
>      'd' },
> +		{ "help", 	no_argument, 		&help
			 ^		    ^ white space
> _flg,  2  },
Why not 'h'?

> +		{ "offset",	required_argument,	NULL,
		  "offset in hex" ?
> 	   'o' },
> +		{ "val",	required_argument,	NULL,	
>    'v' },
		"value in hex"
> +		{ 0 }
> +	};
> +
> +	offset = val = count = INVALID;
> +	devid = -1;
> +
> +	while ((ret = getopt_long(argc, argv, "-:c:d:o:s::v:",
> longopts, NULL)) != -1) {
> +		switch (ret) {
> +		case 'c':
> +			count = strtoul(optarg, NULL, 10);
> +			break;
> +		case 'd':
> +			devid = strtoul(optarg, NULL, 10);
> +			break;
> +		case 'o':
> +			offset = strtoul(optarg, NULL, 16);
> +			break;
> +		case 'v':
> +			val = strtoul(optarg, NULL, 16);
Check for error returns.

> +			break;
> +		/* Fall through for --help */
There is no fall through here ^

> +		case 0:
> +			break;
> +		/* Command parsing */
> +		case 1:
When is 1 returned?

> +			if (strcmp(optarg, "read") == 0)
> +				cmd = READ;
> +			else if (strcmp(optarg, "write") == 0)
> +				cmd = WRITE;
> +			break;
> +		case ':':
> +			igt_warn("The -%c option of %s requires an 

Why use igt_warn() inside a tool? That is meant for tests,
print_usage() is sufficient here.

> argument\n",
> +				 optopt, argv[0]);
> +			print_usage(argv[0], 0);

The second argument looks odd, print_usage() barely does anything with
it. 


> +		case '?':
> +		default :
> +			igt_warn("%s - option %c is invalid\n",
> argv[0],
> +				 optopt);
> +			print_usage(argv[0], 0);
> +		}
> +	}
> +
> +	if (help_flg == 2)
> +		print_usage(argv[0], 1);
> +
> +	if (devid == -1 || offset == INVALID) {
> +		igt_warn("Aux device id and/or offset missing\n");
Using a default dev ID will avoid this.

> +		print_usage(argv[0], 0);
> +	}
> +
> +	memset(dev_name, '\0', 20);
> +	snprintf(dev_name, sizeof(aux_dev) + 2, "%s%d", aux_dev,
> devid);

Use strlen() and do this after validating all arguments.
> +
> +	switch (cmd) {
> +	case READ:
> +		if (count == INVALID) {
> +			igt_warn("Please specify the count in
> bytes\n");
> +			print_usage(argv[0], 0);
> +		}
> +		ret = dpcd_read(dev_name, offset, count);
> +		break;
> +	case WRITE:
> +		if (val == INVALID) {
> +			igt_warn("Write value is missing\n");
> +			print_usage(argv[0], 0);
> +		}
> +		ret = dpcd_write(dev_name, offset, &val);
> +		break;
> +	case INV:
> +	default:
> +		igt_warn("Please specify a command: read/write. See
> usage\n");
> +		print_usage(argv[0], 0);
> +	}
> +
> +	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
>  
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev