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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jun 14, 2018 at 12:11:41AM -0700, 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.
> 
> Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> ---
>  tools/Makefile.am      |   2 +-
>  tools/Makefile.sources |   4 +-
>  tools/dpcd_reg.c       | 219 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 223 insertions(+), 2 deletions(-)
>  create mode 100644 tools/dpcd_reg.c
> 
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index 09b6dbcc3ece..b78824dadfdb 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> @@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN)
>  intel_error_decode_LDFLAGS = -lz
>  endif
>  
> -bin_PROGRAMS += intel_dp_compliance
> +bin_PROGRAMS += intel_dp_compliance dpcd_reg

Not the best place for this I think.

Missing meson.build so no one is actually going to build this.

>  intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS)
>  intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la
>  
> diff --git a/tools/Makefile.sources b/tools/Makefile.sources
> index abd23a0f4628..db606b28560f 100644
> --- a/tools/Makefile.sources
> +++ b/tools/Makefile.sources
> @@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \
>          intel_dp_compliance.h \
>          intel_dp_compliance_hotplug.c \
>          $(NULL)
> -
> +dpcd_reg_SOURCES = \
> +	dpcd_reg.c \
> +	$(NULL)
> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> new file mode 100644
> index 000000000000..2848277aa792
> --- /dev/null
> +++ b/tools/dpcd_reg.c
> @@ -0,0 +1,219 @@
> +/*
> + * 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/decode tool
> + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read,
> + * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be set.
> + */
> +
> +#include "igt_core.h"
> +#include <errno.h>
> +#include <fcntl.h>
> +
> +#define INVALID	0xff
> +#define RW_SIZE	1
> +
> +const char aux_dev[] = "/dev/drm_dp_aux";
> +
> +static void print_usage(char *tool, int help)
> +{
> +	igt_info("DPCD register rw and decode tool\n\n");
> +	igt_info("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
> +		 "to be set in the kernel config.\n");
> +	igt_info("Usage %s command [options...]\n", tool);
> +	igt_info("Supported commands are:\n"
> +		 "\tread: Read a dpcd reg at an offset\n"
> +		 "\twrite: Write a dpcd reg at an offset\n"
> +		 "\tdecode: Decode the value of a dpcd reg\n");

You didn't implement decode so no point in mentioning it.

> +	igt_info("Options for the above commands are\n"
> +		 "\t--device: Aux device id, IS_REQUIRED\n"
> +		 "\t--help: print the usage\n"
> +		 "\t--offset: DPCD register offset in hex, IS_REQUIRED\n"
> +		 "\t--spec: Specify DP/eDP spec version for decoding, IS_OPTIONAL\n"
> +		 "\t--val: Specify a value, for reg writes\n");

I think you should model the UI based on intel_reg.

> +	exit((help == 1)? EXIT_SUCCESS : EXIT_FAILURE);
> +}
> +
> +static int dpcd_read(char *device, const uint32_t *offset)

Why is offset a pointer?

> +{
> +	int fd, ret;
> +	void *buf = malloc(sizeof(uint8_t));

Pointless malloc.

> +
> +	if(NULL != buf)
> +		memset(buf, 0, sizeof(uint8_t));
> +	else {
> +		igt_warn("Can't allocate read buffer\n");
> +		ret = EXIT_FAILURE;
> +		goto out;
> +	}
> +
> +	fd = open(device, O_RDONLY);
> +	if (fd > 0) {
> +		ret = pread(fd, buf, RW_SIZE, *offset);
> +		close(fd);
> +		if (ret < 0) {
> +			igt_warn("Failed to read from %s aux device - error %s\n", device, strerror(errno));
> +			ret = EXIT_FAILURE;
> +			goto free_up;
> +		}
> +		printf("Value @ offset %x is %x\n", *offset, *((int *)(buf)));
> +	}
> +	else {
> +		igt_warn("Failed to open %s aux device - error: %s\n", device, strerror(errno));
> +		ret = EXIT_FAILURE;
> +	}
> +free_up:
> +	free(buf);
> +out:
> +	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, RW_SIZE, *offset);
> +		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);
> +	}
> +	else {
> +		igt_warn("Failed to open %s aux device - error: %s\n", device, strerror(errno));
> +		ret = EXIT_FAILURE;
> +	}
> +out:
> +	return ret;
> +}
> +
> +/*TO-DO: Decode dpcd reg value.
> + * By default, the latest DP/eDP spec
> + * versions will be used. To override,
> + * specify the spec version through the
> + * --spec option.
> +static void dpcd_decode()
> +{
> +}
> +*/
> +
> +int main(int argc, char **argv)
> +{
> +	char dev_name[20];
> +	int ret, devid, help_flg;
> +	uint32_t offset;
> +	uint8_t val;
> +
> +	enum command {
> +		INV = -1,
> +		READ = 2,
> +		WRITE,
> +		DECODE,
> +	} cmd = INV;
> +
> +	struct option longopts [] = {
> +		{ "device",	required_argument,	NULL,      'd' },
> +		{ "help", 	no_argument, 		&help_flg,  2  },
> +		{ "offset",	required_argument,	NULL,	   'o' },
> +		{ "spec",	optional_argument,	NULL,	   's' },
> +		{ "val",	required_argument,	NULL,	   'v' },
> +		{ 0 }
> +	};
> +
> +	offset = val = INVALID;
> +	devid = -1;
> +
> +	while ((ret = getopt_long(argc, argv, "-:d:o:s::v:", longopts, NULL)) != -1) {

What's that "-:" about? Also the sole ":"?

> +		switch (ret) {
> +		case 'd':
> +			devid = strtoul(optarg, NULL, 10);
> +			break;
> +		case 'o':
> +			offset = strtoul(optarg, NULL, 16);
> +			break;
> +		case 's':
> +			/* TO-DO: Parse --spec version */
> +			break;
> +		case 'v':
> +			val = strtoul(optarg, NULL, 16);
> +			break;
> +		/* Fall through for --help */
> +		case 0:
> +			break;
> +		/* Command parsing */
> +		case 1:
> +			if (strcmp(optarg, "read") == 0)
> +				cmd = READ;
> +			else if (strcmp(optarg, "write") == 0)
> +				cmd = WRITE;
> +			else if (strcmp(optarg, "decode") == 0)
> +				cmd = DECODE;
> +			break;
> +		case ':':
> +			fprintf(stderr, "The -%c option of %s requires an argument\n", optopt, argv[0]);
> +			print_usage(argv[0], 0);
> +		case '?':
> +		default :
> +			printf("%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) {
> +		printf("Aux device id and/or offset missing\n");
> +		print_usage(argv[0], 0);
> +	}
> +
> +	memset(dev_name, '\0', 20);
> +	snprintf(dev_name, sizeof(aux_dev) + 2, "%s%d", aux_dev, devid);
> +
> +	switch (cmd) {
> +		case READ:
> +			ret = dpcd_read(dev_name, &offset);
> +			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 DECODE:
> +			/* TO-DO: Decode dpcd reg vals
> +			 * drm_dpcd_decode();
> +			*/
> +			break;
> +		case INV:
> +		default:
> +			igt_warn("Please specify a comand: read/write/decode. See usage\n");
> +			print_usage(argv[0], 0);
> +	}
> +
> +	return ret;
> +}
> -- 
> 2.13.5
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev