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

[Tarun Vyas <tarun.vyas@intel.com>]

On Tue, Sep 04, 2018 at 05:48:02PM -0700, Dhinakaran Pandiyan wrote:
> 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.
>
v3 keeps this at 0xf02ff per the DP spec. 
> > +		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?
v3 of this patch keeps the argument as required, but if this option is not passed at all then it defaults to 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?
> 
1 will be returned for all *non-option* arguments, which is what is intended for commands such as read, write, decode, dump etc.
> > +			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