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

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

[Rodrigo Vivi]

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");
+
+	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);
+	else {
+		igt_warn("Can't allocate read buffer\n");
+		return EXIT_FAILURE;
+	}
+
+	fd = open(device, O_RDONLY);
+	if (fd > 0) {
+		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;
+		}
+
+		igt_info("Read %zu byte(s) starting at offset %x\n\n", count, offset);
+		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), 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, sizeof(uint8_t));
+	}
+	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,      'd' },
+		{ "help", 	no_argument, 		&help_flg,  2  },
+		{ "offset",	required_argument,	NULL,	   'o' },
+		{ "val",	required_argument,	NULL,	   'v' },
+		{ 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);
+			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;
+			break;
+		case ':':
+			igt_warn("The -%c option of %s requires an argument\n",
+				 optopt, argv[0]);
+			print_usage(argv[0], 0);
+		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");
+		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:
+		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
 
-- 
2.17.1

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

[igt-dev] [PATCH i-g-t 2/6] tools/dpcd_reg: Improve outputs

[Rodrigo Vivi]

Before:

$ dpcd_reg read --device=0 --offset=0200 --count 8
Read 8 byte(s) starting at offset 200

41
0
0
0
80
0
66
66

After:
$ dpcd_reg read --device=0 --offset=0200 --count 8
0200: 41 00 00 00 80 00 66 66

Besides the clean view this will allow us to integrate
the dump and multiple reads.

Cc: Tarun Vyas <tarun.vyas@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 tools/dpcd_reg.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
index 39dde2c9..d139007c 100644
--- a/tools/dpcd_reg.c
+++ b/tools/dpcd_reg.c
@@ -75,9 +75,10 @@ static int dpcd_read(char *device, const uint32_t offset, size_t count)
 			goto out;
 		}
 
-		igt_info("Read %zu byte(s) starting at offset %x\n\n", count, offset);
+		igt_info("%04x:", offset);
 		for (i = 0; i < count; i++)
-			igt_info("%x\n", *(((uint8_t *)(buf)) + i));
+			igt_info(" %02x", *(((uint8_t *)(buf)) + i));
+		igt_info("\n");
 	}
 	else {
 		igt_warn("Failed to open %s aux device - error: %s\n", device, strerror(errno));
-- 
2.17.1

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

[igt-dev] [PATCH i-g-t 3/6] tools/dpcd_reg: Unify file handling.

[Rodrigo Vivi]

Besides removing few code duplication this will
allow us to read multiple regs without having
to keep closing and opening the file for every
read.

Cc: Tarun Vyas <tarun.vyas@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 tools/dpcd_reg.c | 98 +++++++++++++++++++++++++++---------------------
 1 file changed, 55 insertions(+), 43 deletions(-)

diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
index d139007c..09da4109 100644
--- a/tools/dpcd_reg.c
+++ b/tools/dpcd_reg.c
@@ -53,61 +53,56 @@ static void print_usage(char *tool, int help)
 	exit((help == 1)? EXIT_SUCCESS : EXIT_FAILURE);
 }
 
-static int dpcd_read(char *device, const uint32_t offset, size_t count)
+static int dpcd_read(int fd, const uint32_t offset, size_t count)
 {
-	int fd, ret, i;
+	int ret, i;
 	void *buf = malloc(sizeof(uint8_t) * count);
 
+	ret = lseek(fd, offset, SEEK_SET);
+	if (ret < 0) {
+		igt_warn("lseek to %04x failed!\n", offset);
+		return ret;
+	}
+
 	if (NULL != buf)
 		memset(buf, 0, sizeof(uint8_t) * count);
 	else {
 		igt_warn("Can't allocate read buffer\n");
-		return EXIT_FAILURE;
+		return -1;
 	}
 
-	fd = open(device, O_RDONLY);
-	if (fd > 0) {
-		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;
-		}
-
-		igt_info("%04x:", offset);
-		for (i = 0; i < count; i++)
-			igt_info(" %02x", *(((uint8_t *)(buf)) + i));
-		igt_info("\n");
-	}
-	else {
-		igt_warn("Failed to open %s aux device - error: %s\n", device, strerror(errno));
-		ret = EXIT_FAILURE;
+	ret = read(fd, buf, count);
+	if (ret != count) {
+		igt_warn("Failed to read - error %s\n", strerror(errno));
+		goto out;
 	}
+
+	igt_info("%04x:", offset);
+	for (i = 0; i < count; i++)
+		igt_info(" %02x", *(((uint8_t *)(buf)) + i));
+	igt_info("\n");
+
 out:
 	free(buf);
 	return ret;
 }
 
-static int dpcd_write(char *device, const uint32_t offset, const uint8_t *val)
+static int dpcd_write(int fd, 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), 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, sizeof(uint8_t));
+	int ret;
+
+	ret = lseek(fd, offset, SEEK_SET);
+	if (ret < 0) {
+		igt_warn("lseek to %04x failed!\n", offset);
+		return ret;
 	}
-	else {
-		igt_warn("Failed to open %s aux device - error: %s\n", device, strerror(errno));
-		ret = EXIT_FAILURE;
+
+	ret = write(fd, (const void *)val, sizeof(uint8_t));
+	if (ret < 0) {
+		igt_warn("Failed to write - error %s\n", strerror(errno));
+		goto out;
 	}
+	ret = dpcd_read(fd, offset, sizeof(uint8_t));
 out:
 	return ret;
 }
@@ -115,10 +110,11 @@ out:
 int main(int argc, char **argv)
 {
 	char dev_name[20];
-	int ret, devid, help_flg;
+	int fd, ret, devid, help_flg;
 	uint32_t offset;
 	uint8_t val;
 	size_t count;
+	int file_op = O_RDONLY;
 
 	enum command {
 		INV = -1,
@@ -157,11 +153,12 @@ int main(int argc, char **argv)
 			break;
 		/* Command parsing */
 		case 1:
-			if (strcmp(optarg, "read") == 0)
+			if (strcmp(optarg, "read") == 0) {
 				cmd = READ;
-			else if (strcmp(optarg, "write") == 0)
+			} else if (strcmp(optarg, "write") == 0) {
 				cmd = WRITE;
-			break;
+				file_op = O_RDWR;
+			} break;
 		case ':':
 			igt_warn("The -%c option of %s requires an argument\n",
 				 optopt, argv[0]);
@@ -185,20 +182,33 @@ int main(int argc, char **argv)
 	memset(dev_name, '\0', 20);
 	snprintf(dev_name, sizeof(aux_dev) + 2, "%s%d", aux_dev, devid);
 
+	fd = open(dev_name, file_op);
+	if (fd < 0) {
+		igt_warn("Failed to open %s aux device - error: %s\n", dev_name,
+			 strerror(errno));
+		return EXIT_FAILURE;
+	}
+
 	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);
+		ret = dpcd_read(fd, offset, count);
+		if (ret != count)
+			igt_warn("Failed to read from %s aux device\n",
+				 dev_name);
 		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);
+		ret = dpcd_write(fd, offset, &val);
+		if (ret < 0)
+			igt_warn("Failed to write to %s aux device\n",
+				 dev_name);
 		break;
 	case INV:
 	default:
@@ -206,5 +216,7 @@ int main(int argc, char **argv)
 		print_usage(argv[0], 0);
 	}
 
+	close(fd);
+
 	return ret;
 }
-- 
2.17.1

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

[igt-dev] [PATCH i-g-t 4/6] tools/dpcd_reg: Introduce dump as the default operation.

[Rodrigo Vivi]

For now this only imports the registers that were used on
i915's debugfs dpcd dump. Later this could be extended.

Also for now this only do the dump on the specified
device. But it could become more flexible and dump
all DP devices if no char device number is specified.

Cc: Tarun Vyas <tarun.vyas@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 tools/dpcd_reg.c | 72 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 63 insertions(+), 9 deletions(-)

diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
index 09da4109..c533401c 100644
--- a/tools/dpcd_reg.c
+++ b/tools/dpcd_reg.c
@@ -31,6 +31,36 @@
 
 #define INVALID	0xff
 
+struct dpcd_block {
+	/* DPCD dump start address. */
+	unsigned int offset;
+	/* DPCD number of bytes to read. If unset, defaults to 1. */
+	size_t count;
+};
+
+static const struct dpcd_block dump_list[] = {
+	/* DP_DPCD_REV */
+	{ .offset = 0, .count = 15 },
+	/* DP_PSR_SUPPORT to DP_PSR_CAPS*/
+	{ .offset =  0x70, .count = 2 },
+	/* DP_DOWNSTREAM_PORT_0 */
+	{ .offset =  0x80, .count = 16 },
+	/* DP_LINK_BW_SET to DP_EDP_CONFIGURATION_SET */
+	{ .offset = 0x100, .count = 11 },
+	/* DP_SINK_COUNT to DP_ADJUST_REQUEST_LANE2_3 */
+	{ .offset = 0x200, .count = 8 },
+	/* DP_SET_POWER */
+	{ .offset = 0x600 },
+	/* DP_EDP_DPCD_REV */
+	{ .offset =  0x700 },
+	/* DP_EDP_GENERAL_CAP_1 to DP_EDP_GENERAL_CAP_3 */
+	{ .offset = 0x701, .count = 4 },
+	/* DP_EDP_DISPLAY_CONTROL_REGISTER to DP_EDP_BACKLIGHT_FREQ_CAP_MAX_LSB */
+	{ .offset = 0x720, .count = 16},
+	/* DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET to DP_EDP_DBC_MAXIMUM_BRIGHTNESS_SET */
+	{ .offset =  0x732, .count = 2 },
+};
+
 const char aux_dev[] = "/dev/drm_dp_aux";
 
 static void print_usage(char *tool, int help)
@@ -107,6 +137,23 @@ out:
 	return ret;
 }
 
+static int dpcd_dump(int fd)
+{
+	size_t count;
+	int ret, i;
+
+	for (i = 0; i < sizeof(dump_list) / sizeof(dump_list[0]); i++) {
+		count = dump_list[i].count ? dump_list[i].count: 1;
+		ret = dpcd_read(fd, dump_list[i].offset, count);
+		if (ret != count) {
+			igt_warn("Dump failed while reading %04x\n",
+				 dump_list[i].offset);
+			return ret;
+		}
+	}
+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	char dev_name[20];
@@ -117,10 +164,10 @@ int main(int argc, char **argv)
 	int file_op = O_RDONLY;
 
 	enum command {
-		INV = -1,
-		READ = 2,
+		DUMP,
+		READ,
 		WRITE,
-	} cmd = INV;
+	} cmd = DUMP;
 
 	struct option longopts [] = {
 		{ "count",      required_argument,      NULL,      'c' },
@@ -174,8 +221,13 @@ int main(int argc, char **argv)
 	if (help_flg == 2)
 		print_usage(argv[0], 1);
 
-	if (devid == -1 || offset == INVALID) {
-		igt_warn("Aux device id and/or offset missing\n");
+	if (devid == -1) {
+		igt_warn("Aux device missing\n");
+		print_usage(argv[0], 0);
+	}
+
+	if (cmd != DUMP && offset == INVALID) {
+		printf("Offset needed for this operation\n");
 		print_usage(argv[0], 0);
 	}
 
@@ -210,10 +262,12 @@ int main(int argc, char **argv)
 			igt_warn("Failed to write to %s aux device\n",
 				 dev_name);
 		break;
-	case INV:
-	default:
-		igt_warn("Please specify a command: read/write. See usage\n");
-		print_usage(argv[0], 0);
+	case DUMP:
+		ret = dpcd_dump(fd);
+		if (ret < 0)
+			igt_warn("Failed to dump %s aux device\n",
+				 dev_name);
+		break;
 	}
 
 	close(fd);
-- 
2.17.1

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

[igt-dev] [PATCH i-g-t 5/6] tools/dpcd_reg: Make --count optional.

[Rodrigo Vivi]

Defaults to 1 when not explicitly defined.

Cc: Tarun Vyas <tarun.vyas@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 tools/dpcd_reg.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
index c533401c..a141f1fc 100644
--- a/tools/dpcd_reg.c
+++ b/tools/dpcd_reg.c
@@ -170,7 +170,7 @@ int main(int argc, char **argv)
 	} cmd = DUMP;
 
 	struct option longopts [] = {
-		{ "count",      required_argument,      NULL,      'c' },
+		{ "count",      optional_argument,      NULL,      'c' },
 		{ "device",	required_argument,	NULL,      'd' },
 		{ "help", 	no_argument, 		&help_flg,  2  },
 		{ "offset",	required_argument,	NULL,	   'o' },
@@ -227,10 +227,15 @@ int main(int argc, char **argv)
 	}
 
 	if (cmd != DUMP && offset == INVALID) {
-		printf("Offset needed for this operation\n");
+		igt_warn("Offset needed for this operation\n");
 		print_usage(argv[0], 0);
 	}
 
+	if (cmd != DUMP && count == INVALID) {
+		igt_debug("count not defined, defaults to 1 byte\n");
+		count = 1;
+	}
+
 	memset(dev_name, '\0', 20);
 	snprintf(dev_name, sizeof(aux_dev) + 2, "%s%d", aux_dev, devid);
 
@@ -243,20 +248,12 @@ int main(int argc, char **argv)
 
 	switch (cmd) {
 	case READ:
-		if (count == INVALID) {
-			igt_warn("Please specify the count in bytes\n");
-			print_usage(argv[0], 0);
-		}
 		ret = dpcd_read(fd, offset, count);
 		if (ret != count)
 			igt_warn("Failed to read from %s aux device\n",
 				 dev_name);
 		break;
 	case WRITE:
-		if (val == INVALID) {
-			igt_warn("Write value is missing\n");
-			print_usage(argv[0], 0);
-		}
 		ret = dpcd_write(fd, offset, &val);
 		if (ret < 0)
 			igt_warn("Failed to write to %s aux device\n",
-- 
2.17.1

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

[igt-dev] [PATCH i-g-t 6/6] tools/dpcd_reg: Add dump all functionality

[Rodrigo Vivi]

And make it the default when no operation or device is given.

So, this will replace i915_dpcd for now but can be expanded
later.

Current debugfs:

$ sudo cat /sys/kernel/debug/dri/0/DP-1/i915_dpcd
0000: 12 14 c4 01 01 00 01 00 02 02 06 00 00 00 02
0070: 00 00
0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0100: 14 84 00 06 06 06 06 00 01 04 00
0200: 41 00 77 77 01 03 22 22
0600: 01
0700: 01
0701: 00 00 00 00
0720: 00 01 00 00 00 01 01 00 00 00 00 00 01 00 00 01
0732: 00 00

$ sudo cat /sys/kernel/debug/dri/0/eDP-1/i915_dpcd
0000: 12 14 84 40 00 00 01 01 02 00 00 00 00 0b 00
0070: 01 00
0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0100: 14 04 00 00 00 00 00 00 00 00 00
0200: 41 00 00 00 80 00 66 66
0600: 01
0700: 02
0701: 9f 40 00 00
0720: 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 00
0732: 00 00

new tool:

$ sudo tools/dpcd_reg

Dumping DPDDC-B
0000: 12 14 c4 01 01 00 01 00 02 02 06 00 00 00 02
0070: 00 00
0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0100: 14 84 00 06 06 06 06 00 01 04 00
0200: 41 00 77 77 01 03 22 22
0600: 01
0700: 01
0701: 00 00 00 00
0720: 00 01 00 00 00 01 01 00 00 00 00 00 01 00 00 01
0732: 00 00

Dumping DPDDC-A
0000: 12 14 84 40 00 00 01 01 02 00 00 00 00 0b 00
0070: 01 00
0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0100: 14 04 00 00 00 00 00 00 00 00 00
0200: 41 00 00 00 80 00 66 66
0600: 01
0700: 02
0701: 9f 40 00 00
0720: 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 00
0732: 00 00

Cc: Tarun Vyas <tarun.vyas@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 tools/dpcd_reg.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 69 insertions(+), 2 deletions(-)

diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
index a141f1fc..0f17383e 100644
--- a/tools/dpcd_reg.c
+++ b/tools/dpcd_reg.c
@@ -28,6 +28,7 @@
 #include "igt_core.h"
 #include <errno.h>
 #include <fcntl.h>
+#include <dirent.h>
 
 #define INVALID	0xff
 
@@ -154,6 +155,67 @@ static int dpcd_dump(int fd)
 	return 0;
 }
 
+static int dpcd_dump_all()
+{
+	struct dirent *dirent;
+	DIR *dir;
+	char dev_path[PATH_MAX];
+	char sysfs_path[PATH_MAX];
+	int dev_fd, sysfs_fd, ret = 0;
+	char dpcd_name[8];
+	char discard;
+
+	dir = opendir("/sys/class/drm_dp_aux_dev/");
+	if (!dir) {
+		igt_warn("fail to read /dev\n");
+		return EXIT_FAILURE;
+	}
+
+	while ((dirent = readdir(dir))) {
+		if (strncmp(dirent->d_name, "drm_dp_aux", 10) == 0) {
+			sprintf(dev_path, "/dev/%s", dirent->d_name);
+			sprintf(sysfs_path, "/sys/class/drm_dp_aux_dev/%s/name",
+				dirent->d_name);
+
+			sysfs_fd = open(sysfs_path, O_RDONLY);
+			if (sysfs_fd < 0) {
+				igt_warn("fail to open %s\n", sysfs_path);
+				continue;
+			}
+
+			dev_fd = open(dev_path, O_RDONLY);
+			if (dev_fd < 0) {
+				igt_warn("fail to open %s\n", dev_path);
+				continue;
+			}
+
+			ret = read(sysfs_fd, &dpcd_name, sizeof(dpcd_name));
+			if (ret < 0) {
+				igt_warn("fail to read dpcd name from %s\n\n", sysfs_path);
+				continue;
+			}
+			dpcd_name[ret] = '\0';
+
+			/* Dummy read to check if dpcd is available */
+			ret = read(dev_fd, &discard, 1);
+			if (ret != 1) {
+				igt_debug("DPCD %s seems not available. skipping...\n", dpcd_name);
+				continue;
+			}
+
+			igt_info("\nDumping %s", dpcd_name);
+			dpcd_dump(dev_fd);
+
+			close(sysfs_fd);
+			close(dev_fd);
+		}
+	}
+
+	closedir(dir);
+
+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	char dev_name[20];
@@ -222,8 +284,13 @@ int main(int argc, char **argv)
 		print_usage(argv[0], 1);
 
 	if (devid == -1) {
-		igt_warn("Aux device missing\n");
-		print_usage(argv[0], 0);
+		if (cmd != DUMP) {
+			igt_warn("Aux device missing\n");
+			print_usage(argv[0], 0);
+		}
+		else {
+			return dpcd_dump_all();
+		}
 	}
 
 	if (cmd != DUMP && offset == INVALID) {
-- 
2.17.1

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

[igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/6] tools: Add a simple tool to read/write/decode dpcd registers

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/6] tools: Add a simple tool to read/write/decode dpcd registers
URL   : https://patchwork.freedesktop.org/series/49037/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4749 -> IGTPW_1770 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/49037/revisions/1/mbox/

== Known issues ==

  Here are the changes found in IGTPW_1770 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       PASS -> INCOMPLETE (fdo#103713)

    igt@gem_exec_suspend@basic-s3:
      {fi-kbl-soraka}:    NOTRUN -> INCOMPLETE (fdo#107774, fdo#107556)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         PASS -> INCOMPLETE (fdo#103927)

    
    ==== Warnings ====

    {igt@pm_rpm@module-reload}:
      fi-bsw-n3050:       DMESG-WARN (fdo#107704) -> DMESG-FAIL (fdo#107704)

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
  fdo#107704 https://bugs.freedesktop.org/show_bug.cgi?id=107704
  fdo#107774 https://bugs.freedesktop.org/show_bug.cgi?id=107774


== Participating hosts (52 -> 46) ==

  Additional (1): fi-kbl-soraka 
  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-glk-j4005 fi-cfl-8109u 


== Build changes ==

    * IGT: IGT_4616 -> IGTPW_1770

  CI_DRM_4749: 4a46c18fad0de38a78b4b0c848892de494324a17 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1770: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1770/
  IGT_4616: 5800e46c6f851c370c944a7cb169e99657239f8d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1770/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.IGT: failure for series starting with [i-g-t,1/6] tools: Add a simple tool to read/write/decode dpcd registers

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/6] tools: Add a simple tool to read/write/decode dpcd registers
URL   : https://patchwork.freedesktop.org/series/49037/
State : failure

== Summary ==

= CI Bug Log - changes from IGT_4616_full -> IGTPW_1770_full =

== Summary - FAILURE ==

  Serious unknown changes coming with IGTPW_1770_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1770_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/49037/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in IGTPW_1770_full:

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_plane@pixel-format-pipe-a-planes:
      shard-snb:          PASS -> FAIL

    
== Known issues ==

  Here are the changes found in IGTPW_1770_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@fence-restore-untiled:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@gem_exec_big:
      shard-hsw:          PASS -> INCOMPLETE (fdo#103540)

    igt@gem_exec_schedule@pi-ringfull-blt:
      shard-glk:          NOTRUN -> FAIL (fdo#103158)

    igt@gem_exec_schedule@pi-ringfull-bsd1:
      shard-kbl:          NOTRUN -> FAIL (fdo#103158)

    igt@kms_cursor_legacy@cursor-vs-flip-toggle:
      shard-hsw:          PASS -> FAIL (fdo#103355)

    igt@kms_flip@modeset-vs-vblank-race:
      shard-glk:          PASS -> FAIL (fdo#103060)

    
    ==== Possible fixes ====

    igt@drv_suspend@shrink:
      shard-snb:          INCOMPLETE (fdo#106886, fdo#105411) -> PASS
      shard-kbl:          INCOMPLETE (fdo#106886, fdo#103665) -> PASS

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-kbl:          INCOMPLETE (fdo#106023, fdo#103665) -> PASS

    igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic:
      shard-glk:          FAIL (fdo#104873) -> PASS

    igt@kms_cursor_legacy@cursor-vs-flip-varying-size:
      shard-hsw:          FAIL (fdo#103355) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc:
      shard-glk:          FAIL (fdo#103167) -> PASS

    igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
      shard-snb:          FAIL (fdo#103166) -> PASS

    igt@kms_setmode@basic:
      shard-apl:          FAIL (fdo#99912) -> PASS
      shard-kbl:          FAIL (fdo#99912) -> PASS

    
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103355 https://bugs.freedesktop.org/show_bug.cgi?id=103355
  fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#104873 https://bugs.freedesktop.org/show_bug.cgi?id=104873
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * IGT: IGT_4616 -> IGTPW_1770
    * Linux: CI_DRM_4748 -> CI_DRM_4749

  CI_DRM_4748: 6caeb081ceb9282503439565e7093c1032758289 @ git://anongit.freedesktop.org/gfx-ci/linux
  CI_DRM_4749: 4a46c18fad0de38a78b4b0c848892de494324a17 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1770: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1770/
  IGT_4616: 5800e46c6f851c370c944a7cb169e99657239f8d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1770/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Dhinakaran Pandiyan]

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

Re: [igt-dev] [PATCH i-g-t 3/6] tools/dpcd_reg: Unify file handling.

[Dhinakaran Pandiyan]

On Fri, 2018-08-31 at 21:18 -0700, Rodrigo Vivi wrote:
> Besides removing few code duplication this will
> allow us to read multiple regs without having
> to keep closing and opening the file for every
> read.
> 
> Cc: Tarun Vyas <tarun.vyas@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  tools/dpcd_reg.c | 98 +++++++++++++++++++++++++++-------------------
> --
>  1 file changed, 55 insertions(+), 43 deletions(-)
> 
> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> index d139007c..09da4109 100644
> --- a/tools/dpcd_reg.c
> +++ b/tools/dpcd_reg.c
> @@ -53,61 +53,56 @@ static void print_usage(char *tool, int help)
>  	exit((help == 1)? EXIT_SUCCESS : EXIT_FAILURE);
>  }
>  
> -static int dpcd_read(char *device, const uint32_t offset, size_t
> count)
> +static int dpcd_read(int fd, const uint32_t offset, size_t count)
>  {
> -	int fd, ret, i;
> +	int ret, i;
>  	void *buf = malloc(sizeof(uint8_t) * count);
>  
> +	ret = lseek(fd, offset, SEEK_SET);
Does this offer any benefit over pread()/pwrite()?

> +	if (ret < 0) {
> +		igt_warn("lseek to %04x failed!\n", offset);
> +		return ret;
> +	}
> +
>  	if (NULL != buf)
>  		memset(buf, 0, sizeof(uint8_t) * count);
>  	else {
>  		igt_warn("Can't allocate read buffer\n");
> -		return EXIT_FAILURE;
> +		return -1;
>  	}
>  
> -	fd = open(device, O_RDONLY);
> -	if (fd > 0) {
> -		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;
> -		}
> -
> -		igt_info("%04x:", offset);
> -		for (i = 0; i < count; i++)
> -			igt_info(" %02x", *(((uint8_t *)(buf)) +
> i));
> -		igt_info("\n");
> -	}
> -	else {
> -		igt_warn("Failed to open %s aux device - error:
> %s\n", device, strerror(errno));
> -		ret = EXIT_FAILURE;
> +	ret = read(fd, buf, count);
> +	if (ret != count) {
> +		igt_warn("Failed to read - error %s\n",
> strerror(errno));
> +		goto out;
>  	}
> +
> +	igt_info("%04x:", offset);
> +	for (i = 0; i < count; i++)
> +		igt_info(" %02x", *(((uint8_t *)(buf)) + i));
> +	igt_info("\n");
> +
>  out:
>  	free(buf);
>  	return ret;
>  }
>  
> -static int dpcd_write(char *device, const uint32_t offset, const
> uint8_t *val)
> +static int dpcd_write(int fd, 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),
> 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, sizeof(uint8_t));
> +	int ret;
> +
> +	ret = lseek(fd, offset, SEEK_SET);
> +	if (ret < 0) {
> +		igt_warn("lseek to %04x failed!\n", offset);
> +		return ret;
>  	}
> -	else {
> -		igt_warn("Failed to open %s aux device - error:
> %s\n", device, strerror(errno));
> -		ret = EXIT_FAILURE;
> +
> +	ret = write(fd, (const void *)val, sizeof(uint8_t));
> +	if (ret < 0) {
> +		igt_warn("Failed to write - error %s\n",
> strerror(errno));
> +		goto out;
>  	}
> +	ret = dpcd_read(fd, offset, sizeof(uint8_t));
>  out:
>  	return ret;
>  }
> @@ -115,10 +110,11 @@ out:
>  int main(int argc, char **argv)
>  {
>  	char dev_name[20];
> -	int ret, devid, help_flg;
> +	int fd, ret, devid, help_flg;
>  	uint32_t offset;
>  	uint8_t val;
>  	size_t count;
> +	int file_op = O_RDONLY;
>  
>  	enum command {
>  		INV = -1,
> @@ -157,11 +153,12 @@ int main(int argc, char **argv)
>  			break;
>  		/* Command parsing */
>  		case 1:
> -			if (strcmp(optarg, "read") == 0)
> +			if (strcmp(optarg, "read") == 0) {
>  				cmd = READ;
> -			else if (strcmp(optarg, "write") == 0)
> +			} else if (strcmp(optarg, "write") == 0) {
>  				cmd = WRITE;
> -			break;
> +				file_op = O_RDWR;
> +			} break;
>  		case ':':
>  			igt_warn("The -%c option of %s requires an
> argument\n",
>  				 optopt, argv[0]);
> @@ -185,20 +182,33 @@ int main(int argc, char **argv)
>  	memset(dev_name, '\0', 20);
>  	snprintf(dev_name, sizeof(aux_dev) + 2, "%s%d", aux_dev,
> devid);
>  
> +	fd = open(dev_name, file_op);
> +	if (fd < 0) {
> +		igt_warn("Failed to open %s aux device - error:
> %s\n", dev_name,
> +			 strerror(errno));
> +		return EXIT_FAILURE;
> +	}
> +
>  	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);
> +		ret = dpcd_read(fd, offset, count);
> +		if (ret != count)
> +			igt_warn("Failed to read from %s aux
> device\n",
> +				 dev_name);
>  		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);
> +		ret = dpcd_write(fd, offset, &val);
> +		if (ret < 0)
> +			igt_warn("Failed to write to %s aux
> device\n",
> +				 dev_name);
>  		break;
>  	case INV:
>  	default:
> @@ -206,5 +216,7 @@ int main(int argc, char **argv)
>  		print_usage(argv[0], 0);
>  	}
>  
> +	close(fd);
> +
>  	return ret;
>  }
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 2/6] tools/dpcd_reg: Improve outputs

[Dhinakaran Pandiyan]

On Fri, 2018-08-31 at 21:18 -0700, Rodrigo Vivi wrote:
> Before:
> 
> $ dpcd_reg read --device=0 --offset=0200 --count 8
> Read 8 byte(s) starting at offset 200
> 
> 41
> 0
> 0
> 0
> 80
> 0
> 66
> 66
> 
> After:
> $ dpcd_reg read --device=0 --offset=0200 --count 8
> 0200: 41 00 00 00 80 00 66 66
> 
> Besides the clean view this will allow us to integrate
> the dump and multiple reads.
> 
> Cc: Tarun Vyas <tarun.vyas@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  tools/dpcd_reg.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> index 39dde2c9..d139007c 100644
> --- a/tools/dpcd_reg.c
> +++ b/tools/dpcd_reg.c
> @@ -75,9 +75,10 @@ static int dpcd_read(char *device, const uint32_t
> offset, size_t count)
>  			goto out;
>  		}
>  
> -		igt_info("Read %zu byte(s) starting at offset
> %x\n\n", count, offset);
> +		igt_info("%04x:", offset);
>  		for (i = 0; i < count; i++)
> -			igt_info("%x\n", *(((uint8_t *)(buf)) + i));
> +			igt_info(" %02x", *(((uint8_t *)(buf)) + 

I am expecting igt_info() in Patch 1/6 to go way, but this looks
better. Please make it obvious that the values are in hex by prefixing
"0x". Same with the offset too.

> i));
> +		igt_info("\n");
>  	}
>  	else {
>  		igt_warn("Failed to open %s aux device - error:
> %s\n", device, strerror(errno));
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 5/6] tools/dpcd_reg: Make --count optional.

[Dhinakaran Pandiyan]

On Fri, 2018-08-31 at 21:18 -0700, Rodrigo Vivi wrote:
> Defaults to 1 when not explicitly defined.
> 
> Cc: Tarun Vyas <tarun.vyas@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  tools/dpcd_reg.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> index c533401c..a141f1fc 100644
> --- a/tools/dpcd_reg.c
> +++ b/tools/dpcd_reg.c
> @@ -170,7 +170,7 @@ int main(int argc, char **argv)
>  	} cmd = DUMP;
>  
>  	struct option longopts [] = {
> -		{
> "count",      required_argument,      NULL,      'c' },
> +		{
> "count",      optional_argument,      NULL,      'c' },
Passing count can be optional, but when count is passed it should
require an argument.

From what I understand, this change makes passing an argument to count
optional.

>  		{ "device",	required_argument,	NULL,  
>     'd' },
>  		{ "help", 	no_argument, 		&help
> _flg,  2  },
>  		{ "offset",	required_argument,	NULL,	
>    'o' },
> @@ -227,10 +227,15 @@ int main(int argc, char **argv)
>  	}
>  
>  	if (cmd != DUMP && offset == INVALID) {
> -		printf("Offset needed for this operation\n");
> +		igt_warn("Offset needed for this operation\n");
>  		print_usage(argv[0], 0);
>  	}
>  
> +	if (cmd != DUMP && count == INVALID) {
> +		igt_debug("count not defined, defaults to 1
> byte\n");
> +		count = 1;
> +	}
> +
>  	memset(dev_name, '\0', 20);
>  	snprintf(dev_name, sizeof(aux_dev) + 2, "%s%d", aux_dev,
> devid);
>  
> @@ -243,20 +248,12 @@ int main(int argc, char **argv)
>  
>  	switch (cmd) {
>  	case READ:
> -		if (count == INVALID) {
> -			igt_warn("Please specify the count in
> bytes\n");
> -			print_usage(argv[0], 0);
> -		}
>  		ret = dpcd_read(fd, offset, count);
>  		if (ret != count)
>  			igt_warn("Failed to read from %s aux
> device\n",
>  				 dev_name);
>  		break;
>  	case WRITE:
> -		if (val == INVALID) {
> -			igt_warn("Write value is missing\n");
> -			print_usage(argv[0], 0);
> -		}
>  		ret = dpcd_write(fd, offset, &val);
>  		if (ret < 0)
>  			igt_warn("Failed to write to %s aux
> device\n",
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 3/6] tools/dpcd_reg: Unify file handling.

[Rodrigo Vivi]

On Tue, Sep 04, 2018 at 05:56:01PM -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-08-31 at 21:18 -0700, Rodrigo Vivi wrote:
> > Besides removing few code duplication this will
> > allow us to read multiple regs without having
> > to keep closing and opening the file for every
> > read.
> > 
> > Cc: Tarun Vyas <tarun.vyas@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  tools/dpcd_reg.c | 98 +++++++++++++++++++++++++++-------------------
> > --
> >  1 file changed, 55 insertions(+), 43 deletions(-)
> > 
> > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> > index d139007c..09da4109 100644
> > --- a/tools/dpcd_reg.c
> > +++ b/tools/dpcd_reg.c
> > @@ -53,61 +53,56 @@ static void print_usage(char *tool, int help)
> >  	exit((help == 1)? EXIT_SUCCESS : EXIT_FAILURE);
> >  }
> >  
> > -static int dpcd_read(char *device, const uint32_t offset, size_t
> > count)
> > +static int dpcd_read(int fd, const uint32_t offset, size_t count)
> >  {
> > -	int fd, ret, i;
> > +	int ret, i;
> >  	void *buf = malloc(sizeof(uint8_t) * count);
> >  
> > +	ret = lseek(fd, offset, SEEK_SET);
> Does this offer any benefit over pread()/pwrite()?

I'm honestly not sure. Can we trust pread/pwrite that offsets are always
relative to the begin of the file or relative to where the pointer
was left after last read/write operation but file not closed?

looking to the docs lseek seemed more crispy on the definition
of its navigation and since I cannot guarantee order on dpcd_list
I preferred lseek.

but if you are sure pread/pwrite is reliable and not relative position
and you have strong preference on that I can change back.

Thanks,
Rodrigo.

> 
> > +	if (ret < 0) {
> > +		igt_warn("lseek to %04x failed!\n", offset);
> > +		return ret;
> > +	}
> > +
> >  	if (NULL != buf)
> >  		memset(buf, 0, sizeof(uint8_t) * count);
> >  	else {
> >  		igt_warn("Can't allocate read buffer\n");
> > -		return EXIT_FAILURE;
> > +		return -1;
> >  	}
> >  
> > -	fd = open(device, O_RDONLY);
> > -	if (fd > 0) {
> > -		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;
> > -		}
> > -
> > -		igt_info("%04x:", offset);
> > -		for (i = 0; i < count; i++)
> > -			igt_info(" %02x", *(((uint8_t *)(buf)) +
> > i));
> > -		igt_info("\n");
> > -	}
> > -	else {
> > -		igt_warn("Failed to open %s aux device - error:
> > %s\n", device, strerror(errno));
> > -		ret = EXIT_FAILURE;
> > +	ret = read(fd, buf, count);
> > +	if (ret != count) {
> > +		igt_warn("Failed to read - error %s\n",
> > strerror(errno));
> > +		goto out;
> >  	}
> > +
> > +	igt_info("%04x:", offset);
> > +	for (i = 0; i < count; i++)
> > +		igt_info(" %02x", *(((uint8_t *)(buf)) + i));
> > +	igt_info("\n");
> > +
> >  out:
> >  	free(buf);
> >  	return ret;
> >  }
> >  
> > -static int dpcd_write(char *device, const uint32_t offset, const
> > uint8_t *val)
> > +static int dpcd_write(int fd, 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),
> > 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, sizeof(uint8_t));
> > +	int ret;
> > +
> > +	ret = lseek(fd, offset, SEEK_SET);
> > +	if (ret < 0) {
> > +		igt_warn("lseek to %04x failed!\n", offset);
> > +		return ret;
> >  	}
> > -	else {
> > -		igt_warn("Failed to open %s aux device - error:
> > %s\n", device, strerror(errno));
> > -		ret = EXIT_FAILURE;
> > +
> > +	ret = write(fd, (const void *)val, sizeof(uint8_t));
> > +	if (ret < 0) {
> > +		igt_warn("Failed to write - error %s\n",
> > strerror(errno));
> > +		goto out;
> >  	}
> > +	ret = dpcd_read(fd, offset, sizeof(uint8_t));
> >  out:
> >  	return ret;
> >  }
> > @@ -115,10 +110,11 @@ out:
> >  int main(int argc, char **argv)
> >  {
> >  	char dev_name[20];
> > -	int ret, devid, help_flg;
> > +	int fd, ret, devid, help_flg;
> >  	uint32_t offset;
> >  	uint8_t val;
> >  	size_t count;
> > +	int file_op = O_RDONLY;
> >  
> >  	enum command {
> >  		INV = -1,
> > @@ -157,11 +153,12 @@ int main(int argc, char **argv)
> >  			break;
> >  		/* Command parsing */
> >  		case 1:
> > -			if (strcmp(optarg, "read") == 0)
> > +			if (strcmp(optarg, "read") == 0) {
> >  				cmd = READ;
> > -			else if (strcmp(optarg, "write") == 0)
> > +			} else if (strcmp(optarg, "write") == 0) {
> >  				cmd = WRITE;
> > -			break;
> > +				file_op = O_RDWR;
> > +			} break;
> >  		case ':':
> >  			igt_warn("The -%c option of %s requires an
> > argument\n",
> >  				 optopt, argv[0]);
> > @@ -185,20 +182,33 @@ int main(int argc, char **argv)
> >  	memset(dev_name, '\0', 20);
> >  	snprintf(dev_name, sizeof(aux_dev) + 2, "%s%d", aux_dev,
> > devid);
> >  
> > +	fd = open(dev_name, file_op);
> > +	if (fd < 0) {
> > +		igt_warn("Failed to open %s aux device - error:
> > %s\n", dev_name,
> > +			 strerror(errno));
> > +		return EXIT_FAILURE;
> > +	}
> > +
> >  	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);
> > +		ret = dpcd_read(fd, offset, count);
> > +		if (ret != count)
> > +			igt_warn("Failed to read from %s aux
> > device\n",
> > +				 dev_name);
> >  		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);
> > +		ret = dpcd_write(fd, offset, &val);
> > +		if (ret < 0)
> > +			igt_warn("Failed to write to %s aux
> > device\n",
> > +				 dev_name);
> >  		break;
> >  	case INV:
> >  	default:
> > @@ -206,5 +216,7 @@ int main(int argc, char **argv)
> >  		print_usage(argv[0], 0);
> >  	}
> >  
> > +	close(fd);
> > +
> >  	return ret;
> >  }
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 5/6] tools/dpcd_reg: Make --count optional.

[Rodrigo Vivi]

On Tue, Sep 04, 2018 at 06:12:15PM -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-08-31 at 21:18 -0700, Rodrigo Vivi wrote:
> > Defaults to 1 when not explicitly defined.
> > 
> > Cc: Tarun Vyas <tarun.vyas@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  tools/dpcd_reg.c | 17 +++++++----------
> >  1 file changed, 7 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> > index c533401c..a141f1fc 100644
> > --- a/tools/dpcd_reg.c
> > +++ b/tools/dpcd_reg.c
> > @@ -170,7 +170,7 @@ int main(int argc, char **argv)
> >  	} cmd = DUMP;
> >  
> >  	struct option longopts [] = {
> > -		{
> > "count",      required_argument,      NULL,      'c' },
> > +		{
> > "count",      optional_argument,      NULL,      'c' },
> Passing count can be optional, but when count is passed it should
> require an argument.
> 
> From what I understand, this change makes passing an argument to count
> optional.

ops... indeed... my bad...

I will fix whenever touching this again. ;)

Thanks,
Rodrigo.

> 
> >  		{ "device",	required_argument,	NULL,  
> >     'd' },
> >  		{ "help", 	no_argument, 		&help
> > _flg,  2  },
> >  		{ "offset",	required_argument,	NULL,	
> >    'o' },
> > @@ -227,10 +227,15 @@ int main(int argc, char **argv)
> >  	}
> >  
> >  	if (cmd != DUMP && offset == INVALID) {
> > -		printf("Offset needed for this operation\n");
> > +		igt_warn("Offset needed for this operation\n");
> >  		print_usage(argv[0], 0);
> >  	}
> >  
> > +	if (cmd != DUMP && count == INVALID) {
> > +		igt_debug("count not defined, defaults to 1
> > byte\n");
> > +		count = 1;
> > +	}
> > +
> >  	memset(dev_name, '\0', 20);
> >  	snprintf(dev_name, sizeof(aux_dev) + 2, "%s%d", aux_dev,
> > devid);
> >  
> > @@ -243,20 +248,12 @@ int main(int argc, char **argv)
> >  
> >  	switch (cmd) {
> >  	case READ:
> > -		if (count == INVALID) {
> > -			igt_warn("Please specify the count in
> > bytes\n");
> > -			print_usage(argv[0], 0);
> > -		}
> >  		ret = dpcd_read(fd, offset, count);
> >  		if (ret != count)
> >  			igt_warn("Failed to read from %s aux
> > device\n",
> >  				 dev_name);
> >  		break;
> >  	case WRITE:
> > -		if (val == INVALID) {
> > -			igt_warn("Write value is missing\n");
> > -			print_usage(argv[0], 0);
> > -		}
> >  		ret = dpcd_write(fd, offset, &val);
> >  		if (ret < 0)
> >  			igt_warn("Failed to write to %s aux
> > device\n",
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 6/6] tools/dpcd_reg: Add dump all functionality

[Dhinakaran Pandiyan]

On Fri, 2018-08-31 at 21:18 -0700, Rodrigo Vivi wrote:
> And make it the default when no operation or device is given.
> 
> So, this will replace i915_dpcd for now but can be expanded
> later.

I think we can include PSR sink status registers too and kill the
debugfs file

> 
> Current debugfs:
> 
> $ sudo cat /sys/kernel/debug/dri/0/DP-1/i915_dpcd
> 0000: 12 14 c4 01 01 00 01 00 02 02 06 00 00 00 02
> 0070: 00 00
> 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0100: 14 84 00 06 06 06 06 00 01 04 00
> 0200: 41 00 77 77 01 03 22 22
> 0600: 01
> 0700: 01
> 0701: 00 00 00 00
> 0720: 00 01 00 00 00 01 01 00 00 00 00 00 01 00 00 01
> 0732: 00 00
> 
> $ sudo cat /sys/kernel/debug/dri/0/eDP-1/i915_dpcd
> 0000: 12 14 84 40 00 00 01 01 02 00 00 00 00 0b 00
> 0070: 01 00
> 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0100: 14 04 00 00 00 00 00 00 00 00 00
> 0200: 41 00 00 00 80 00 66 66
> 0600: 01
> 0700: 02
> 0701: 9f 40 00 00
> 0720: 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 00
> 0732: 00 00
> 
> new tool:
> 
> $ sudo tools/dpcd_reg
> 
> Dumping DPDDC-B
> 0000: 12 14 c4 01 01 00 01 00 02 02 06 00 00 00 02
> 0070: 00 00
> 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0100: 14 84 00 06 06 06 06 00 01 04 00
> 0200: 41 00 77 77 01 03 22 22
> 0600: 01
> 0700: 01
> 0701: 00 00 00 00
> 0720: 00 01 00 00 00 01 01 00 00 00 00 00 01 00 00 01
> 0732: 00 00
> 
> Dumping DPDDC-A
> 0000: 12 14 84 40 00 00 01 01 02 00 00 00 00 0b 00
> 0070: 01 00
> 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0100: 14 04 00 00 00 00 00 00 00 00 00
> 0200: 41 00 00 00 80 00 66 66
> 0600: 01
> 0700: 02
> 0701: 9f 40 00 00
> 0720: 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 00
> 0732: 00 00
> 
> Cc: Tarun Vyas <tarun.vyas@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  tools/dpcd_reg.c | 71
> ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 69 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> index a141f1fc..0f17383e 100644
> --- a/tools/dpcd_reg.c
> +++ b/tools/dpcd_reg.c
> @@ -28,6 +28,7 @@
>  #include "igt_core.h"
>  #include <errno.h>
>  #include <fcntl.h>
> +#include <dirent.h>
>  
>  #define INVALID	0xff
>  
> @@ -154,6 +155,67 @@ static int dpcd_dump(int fd)
>  	return 0;
>  }
>  
> +static int dpcd_dump_all()
> +{
> +	struct dirent *dirent;
> +	DIR *dir;
> +	char dev_path[PATH_MAX];
> +	char sysfs_path[PATH_MAX];
> +	int dev_fd, sysfs_fd, ret = 0;
> +	char dpcd_name[8];
> +	char discard;
> +
> +	dir = opendir("/sys/class/drm_dp_aux_dev/");
> +	if (!dir) {
> +		igt_warn("fail to read /dev\n");
> +		return EXIT_FAILURE;
> +	}
> +
> +	while ((dirent = readdir(dir))) {
> +		if (strncmp(dirent->d_name, "drm_dp_aux", 10) == 0)
> {
> +			sprintf(dev_path, "/dev/%s", dirent-
> >d_name);
> +			sprintf(sysfs_path,
> "/sys/class/drm_dp_aux_dev/%s/name",
> +				dirent->d_name);
> +
> +			sysfs_fd = open(sysfs_path, O_RDONLY);
> +			if (sysfs_fd < 0) {
> +				igt_warn("fail to open %s\n",
> sysfs_path);
> +				continue;
> +			}
> +
> +			dev_fd = open(dev_path, O_RDONLY);
> +			if (dev_fd < 0) {
> +				igt_warn("fail to open %s\n",
> dev_path);
> +				continue;
> +			}
> +
> +			ret = read(sysfs_fd, &dpcd_name,
> sizeof(dpcd_name));
> +			if (ret < 0) {
> +				igt_warn("fail to read dpcd name
> from %s\n\n", sysfs_path);
> +				continue;
> +			}
> +			dpcd_name[ret] = '\0';
> +
> +			/* Dummy read to check if dpcd is available
> */
> +			ret = read(dev_fd, &discard, 1);
> +			if (ret != 1) {
> +				igt_debug("DPCD %s seems not
> available. skipping...\n", dpcd_name);
> +				continue;
> +			}
> +
> +			igt_info("\nDumping %s", dpcd_name);
> +			dpcd_dump(dev_fd);
> +
> +			close(sysfs_fd);
> +			close(dev_fd);
> +		}
> +	}
> +
> +	closedir(dir);
> +
> +	return 0;
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	char dev_name[20];
> @@ -222,8 +284,13 @@ int main(int argc, char **argv)
>  		print_usage(argv[0], 1);
>  
>  	if (devid == -1) {
> -		igt_warn("Aux device missing\n");
> -		print_usage(argv[0], 0);
> +		if (cmd != DUMP) {
> +			igt_warn("Aux device missing\n");
> +			print_usage(argv[0], 0);
> +		}
> +		else {
> +			return dpcd_dump_all();
> +		}
>  	}
>  
>  	if (cmd != DUMP && offset == INVALID) {
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 6/6] tools/dpcd_reg: Add dump all functionality

[Rodrigo Vivi]

On Wed, Sep 05, 2018 at 05:26:26PM -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-08-31 at 21:18 -0700, Rodrigo Vivi wrote:
> > And make it the default when no operation or device is given.
> > 
> > So, this will replace i915_dpcd for now but can be expanded
> > later.
> 
> I think we can include PSR sink status registers too and kill the
> debugfs file

good idea! I will do on the next version.

> 
> > 
> > Current debugfs:
> > 
> > $ sudo cat /sys/kernel/debug/dri/0/DP-1/i915_dpcd
> > 0000: 12 14 c4 01 01 00 01 00 02 02 06 00 00 00 02
> > 0070: 00 00
> > 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 0100: 14 84 00 06 06 06 06 00 01 04 00
> > 0200: 41 00 77 77 01 03 22 22
> > 0600: 01
> > 0700: 01
> > 0701: 00 00 00 00
> > 0720: 00 01 00 00 00 01 01 00 00 00 00 00 01 00 00 01
> > 0732: 00 00
> > 
> > $ sudo cat /sys/kernel/debug/dri/0/eDP-1/i915_dpcd
> > 0000: 12 14 84 40 00 00 01 01 02 00 00 00 00 0b 00
> > 0070: 01 00
> > 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 0100: 14 04 00 00 00 00 00 00 00 00 00
> > 0200: 41 00 00 00 80 00 66 66
> > 0600: 01
> > 0700: 02
> > 0701: 9f 40 00 00
> > 0720: 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 00
> > 0732: 00 00
> > 
> > new tool:
> > 
> > $ sudo tools/dpcd_reg
> > 
> > Dumping DPDDC-B
> > 0000: 12 14 c4 01 01 00 01 00 02 02 06 00 00 00 02
> > 0070: 00 00
> > 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 0100: 14 84 00 06 06 06 06 00 01 04 00
> > 0200: 41 00 77 77 01 03 22 22
> > 0600: 01
> > 0700: 01
> > 0701: 00 00 00 00
> > 0720: 00 01 00 00 00 01 01 00 00 00 00 00 01 00 00 01
> > 0732: 00 00
> > 
> > Dumping DPDDC-A
> > 0000: 12 14 84 40 00 00 01 01 02 00 00 00 00 0b 00
> > 0070: 01 00
> > 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 0100: 14 04 00 00 00 00 00 00 00 00 00
> > 0200: 41 00 00 00 80 00 66 66
> > 0600: 01
> > 0700: 02
> > 0701: 9f 40 00 00
> > 0720: 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 00
> > 0732: 00 00
> > 
> > Cc: Tarun Vyas <tarun.vyas@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  tools/dpcd_reg.c | 71
> > ++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 69 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> > index a141f1fc..0f17383e 100644
> > --- a/tools/dpcd_reg.c
> > +++ b/tools/dpcd_reg.c
> > @@ -28,6 +28,7 @@
> >  #include "igt_core.h"
> >  #include <errno.h>
> >  #include <fcntl.h>
> > +#include <dirent.h>
> >  
> >  #define INVALID	0xff
> >  
> > @@ -154,6 +155,67 @@ static int dpcd_dump(int fd)
> >  	return 0;
> >  }
> >  
> > +static int dpcd_dump_all()
> > +{
> > +	struct dirent *dirent;
> > +	DIR *dir;
> > +	char dev_path[PATH_MAX];
> > +	char sysfs_path[PATH_MAX];
> > +	int dev_fd, sysfs_fd, ret = 0;
> > +	char dpcd_name[8];
> > +	char discard;
> > +
> > +	dir = opendir("/sys/class/drm_dp_aux_dev/");
> > +	if (!dir) {
> > +		igt_warn("fail to read /dev\n");
> > +		return EXIT_FAILURE;
> > +	}
> > +
> > +	while ((dirent = readdir(dir))) {
> > +		if (strncmp(dirent->d_name, "drm_dp_aux", 10) == 0)
> > {
> > +			sprintf(dev_path, "/dev/%s", dirent-
> > >d_name);
> > +			sprintf(sysfs_path,
> > "/sys/class/drm_dp_aux_dev/%s/name",
> > +				dirent->d_name);
> > +
> > +			sysfs_fd = open(sysfs_path, O_RDONLY);
> > +			if (sysfs_fd < 0) {
> > +				igt_warn("fail to open %s\n",
> > sysfs_path);
> > +				continue;
> > +			}
> > +
> > +			dev_fd = open(dev_path, O_RDONLY);
> > +			if (dev_fd < 0) {
> > +				igt_warn("fail to open %s\n",
> > dev_path);
> > +				continue;
> > +			}
> > +
> > +			ret = read(sysfs_fd, &dpcd_name,
> > sizeof(dpcd_name));
> > +			if (ret < 0) {
> > +				igt_warn("fail to read dpcd name
> > from %s\n\n", sysfs_path);
> > +				continue;
> > +			}
> > +			dpcd_name[ret] = '\0';
> > +
> > +			/* Dummy read to check if dpcd is available
> > */
> > +			ret = read(dev_fd, &discard, 1);
> > +			if (ret != 1) {
> > +				igt_debug("DPCD %s seems not
> > available. skipping...\n", dpcd_name);
> > +				continue;
> > +			}
> > +
> > +			igt_info("\nDumping %s", dpcd_name);
> > +			dpcd_dump(dev_fd);
> > +
> > +			close(sysfs_fd);
> > +			close(dev_fd);
> > +		}
> > +	}
> > +
> > +	closedir(dir);
> > +
> > +	return 0;
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >  	char dev_name[20];
> > @@ -222,8 +284,13 @@ int main(int argc, char **argv)
> >  		print_usage(argv[0], 1);
> >  
> >  	if (devid == -1) {
> > -		igt_warn("Aux device missing\n");
> > -		print_usage(argv[0], 0);
> > +		if (cmd != DUMP) {
> > +			igt_warn("Aux device missing\n");
> > +			print_usage(argv[0], 0);
> > +		}
> > +		else {
> > +			return dpcd_dump_all();
> > +		}
> >  	}
> >  
> >  	if (cmd != DUMP && offset == INVALID) {
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Tarun Vyas]

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

Re: [igt-dev] [PATCH i-g-t 3/6] tools/dpcd_reg: Unify file handling.

[Tarun Vyas]

On Wed, Sep 05, 2018 at 11:17:59AM -0700, Rodrigo Vivi wrote:
> On Tue, Sep 04, 2018 at 05:56:01PM -0700, Dhinakaran Pandiyan wrote:
> > On Fri, 2018-08-31 at 21:18 -0700, Rodrigo Vivi wrote:
> > > Besides removing few code duplication this will
> > > allow us to read multiple regs without having
> > > to keep closing and opening the file for every
> > > read.
> > > 
> > > Cc: Tarun Vyas <tarun.vyas@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  tools/dpcd_reg.c | 98 +++++++++++++++++++++++++++-------------------
> > > --
> > >  1 file changed, 55 insertions(+), 43 deletions(-)
> > > 
> > > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> > > index d139007c..09da4109 100644
> > > --- a/tools/dpcd_reg.c
> > > +++ b/tools/dpcd_reg.c
> > > @@ -53,61 +53,56 @@ static void print_usage(char *tool, int help)
> > >  	exit((help == 1)? EXIT_SUCCESS : EXIT_FAILURE);
> > >  }
> > >  
> > > -static int dpcd_read(char *device, const uint32_t offset, size_t
> > > count)
> > > +static int dpcd_read(int fd, const uint32_t offset, size_t count)
> > >  {
> > > -	int fd, ret, i;
> > > +	int ret, i;
> > >  	void *buf = malloc(sizeof(uint8_t) * count);
> > >  
> > > +	ret = lseek(fd, offset, SEEK_SET);
> > Does this offer any benefit over pread()/pwrite()?
> 
> I'm honestly not sure. Can we trust pread/pwrite that offsets are always
> relative to the begin of the file or relative to where the pointer
> was left after last read/write operation but file not closed?
> 
> looking to the docs lseek seemed more crispy on the definition
> of its navigation and since I cannot guarantee order on dpcd_list
> I preferred lseek.
> 
> but if you are sure pread/pwrite is reliable and not relative position
> and you have strong preference on that I can change back.
> 
> Thanks,
> Rodrigo.
> 
> >
I tried multiple reads with pread() and it works OK. The doc mentions that the file offset will not be changed by pread() so all the offsets passed to it will be relative to the begining of the file.
> > > +	if (ret < 0) {
> > > +		igt_warn("lseek to %04x failed!\n", offset);
> > > +		return ret;
> > > +	}
> > > +
> > >  	if (NULL != buf)
> > >  		memset(buf, 0, sizeof(uint8_t) * count);
> > >  	else {
> > >  		igt_warn("Can't allocate read buffer\n");
> > > -		return EXIT_FAILURE;
> > > +		return -1;
> > >  	}
> > >  
> > > -	fd = open(device, O_RDONLY);
> > > -	if (fd > 0) {
> > > -		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;
> > > -		}
> > > -
> > > -		igt_info("%04x:", offset);
> > > -		for (i = 0; i < count; i++)
> > > -			igt_info(" %02x", *(((uint8_t *)(buf)) +
> > > i));
> > > -		igt_info("\n");
> > > -	}
> > > -	else {
> > > -		igt_warn("Failed to open %s aux device - error:
> > > %s\n", device, strerror(errno));
> > > -		ret = EXIT_FAILURE;
> > > +	ret = read(fd, buf, count);
> > > +	if (ret != count) {
> > > +		igt_warn("Failed to read - error %s\n",
> > > strerror(errno));
> > > +		goto out;
> > >  	}
> > > +
> > > +	igt_info("%04x:", offset);
> > > +	for (i = 0; i < count; i++)
> > > +		igt_info(" %02x", *(((uint8_t *)(buf)) + i));
> > > +	igt_info("\n");
> > > +
> > >  out:
> > >  	free(buf);
> > >  	return ret;
> > >  }
> > >  
> > > -static int dpcd_write(char *device, const uint32_t offset, const
> > > uint8_t *val)
> > > +static int dpcd_write(int fd, 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),
> > > 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, sizeof(uint8_t));
> > > +	int ret;
> > > +
> > > +	ret = lseek(fd, offset, SEEK_SET);
> > > +	if (ret < 0) {
> > > +		igt_warn("lseek to %04x failed!\n", offset);
> > > +		return ret;
> > >  	}
> > > -	else {
> > > -		igt_warn("Failed to open %s aux device - error:
> > > %s\n", device, strerror(errno));
> > > -		ret = EXIT_FAILURE;
> > > +
> > > +	ret = write(fd, (const void *)val, sizeof(uint8_t));
> > > +	if (ret < 0) {
> > > +		igt_warn("Failed to write - error %s\n",
> > > strerror(errno));
> > > +		goto out;
> > >  	}
> > > +	ret = dpcd_read(fd, offset, sizeof(uint8_t));
> > >  out:
> > >  	return ret;
> > >  }
> > > @@ -115,10 +110,11 @@ out:
> > >  int main(int argc, char **argv)
> > >  {
> > >  	char dev_name[20];
> > > -	int ret, devid, help_flg;
> > > +	int fd, ret, devid, help_flg;
> > >  	uint32_t offset;
> > >  	uint8_t val;
> > >  	size_t count;
> > > +	int file_op = O_RDONLY;
> > >  
> > >  	enum command {
> > >  		INV = -1,
> > > @@ -157,11 +153,12 @@ int main(int argc, char **argv)
> > >  			break;
> > >  		/* Command parsing */
> > >  		case 1:
> > > -			if (strcmp(optarg, "read") == 0)
> > > +			if (strcmp(optarg, "read") == 0) {
> > >  				cmd = READ;
> > > -			else if (strcmp(optarg, "write") == 0)
> > > +			} else if (strcmp(optarg, "write") == 0) {
> > >  				cmd = WRITE;
> > > -			break;
> > > +				file_op = O_RDWR;
> > > +			} break;
> > >  		case ':':
> > >  			igt_warn("The -%c option of %s requires an
> > > argument\n",
> > >  				 optopt, argv[0]);
> > > @@ -185,20 +182,33 @@ int main(int argc, char **argv)
> > >  	memset(dev_name, '\0', 20);
> > >  	snprintf(dev_name, sizeof(aux_dev) + 2, "%s%d", aux_dev,
> > > devid);
> > >  
> > > +	fd = open(dev_name, file_op);
> > > +	if (fd < 0) {
> > > +		igt_warn("Failed to open %s aux device - error:
> > > %s\n", dev_name,
> > > +			 strerror(errno));
> > > +		return EXIT_FAILURE;
> > > +	}
> > > +
> > >  	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);
> > > +		ret = dpcd_read(fd, offset, count);
> > > +		if (ret != count)
> > > +			igt_warn("Failed to read from %s aux
> > > device\n",
> > > +				 dev_name);
> > >  		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);
> > > +		ret = dpcd_write(fd, offset, &val);
> > > +		if (ret < 0)
> > > +			igt_warn("Failed to write to %s aux
> > > device\n",
> > > +				 dev_name);
> > >  		break;
> > >  	case INV:
> > >  	default:
> > > @@ -206,5 +216,7 @@ int main(int argc, char **argv)
> > >  		print_usage(argv[0], 0);
> > >  	}
> > >  
> > > +	close(fd);
> > > +
> > >  	return ret;
> > >  }
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev