public inbox for igt-dev@lists.freedesktop.org
 help / color / mirror / Atom feed
* [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers
@ 2018-09-14 23:57 Tarun Vyas
  2018-09-15  0:24 ` [igt-dev] ✓ Fi.CI.BAT: success for tools: Add a simple tool to read/write/decode dpcd registers (rev3) Patchwork
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Tarun Vyas @ 2018-09-14 23:57 UTC (permalink / raw)
  To: igt-dev; +Cc: dhinakaran.pandiyan, rodrigo.vivi

This tool serves as a wrapper around the constructs provided by the
drm_dpcd_aux_dev kernel module by working on the /dev/drm_dp_aux[n]
devices created by the kernel module.
It supports reading and writing dpcd registers on the connected aux
channels.
In the follow-up patch, support for decoding these registers will be
added to facilate debugging panel related issues.

v2: (Fixes by Rodrigo but no functional changes yet):
    - Indentations, Typo, Missed spaces
    - Removing mentioning to decode and spec that is not implemented yet.
    - Add Makefile.sources back
    - Missed s/printf/igt_warn

v3:
    - Addres DK's review comments from v2 above.
    - Squash Rodrigo's file handling unification patch.
    - Make count, offset and device id optional.

Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 tools/Makefile.sources |   1 +
 tools/dpcd_reg.c       | 213 +++++++++++++++++++++++++++++++++++++++++++++++++
 tools/meson.build      |   1 +
 3 files changed, 215 insertions(+)
 create mode 100644 tools/dpcd_reg.c

diff --git a/tools/Makefile.sources b/tools/Makefile.sources
index abd23a0f..50706f41 100644
--- a/tools/Makefile.sources
+++ b/tools/Makefile.sources
@@ -7,6 +7,7 @@ noinst_PROGRAMS =		\
 
 tools_prog_lists =		\
 	igt_stats		\
+	dpcd_reg		\
 	intel_audio_dump	\
 	intel_reg		\
 	intel_backlight		\
diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
new file mode 100644
index 00000000..cd9fed4f
--- /dev/null
+++ b/tools/dpcd_reg.c
@@ -0,0 +1,213 @@
+/*
+ * Copyright © 2018 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ * DPCD register read/write tool
+ * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read
+ * and write, so CONFIG_DRM_DP_AUX_DEV needs to be set.
+ */
+
+#include "igt_core.h"
+#include <errno.h>
+#include <fcntl.h>
+#include <limits.h>
+
+#define MAX_OFFSET	0xf02ff
+
+const char aux_dev[] = "/dev/drm_dp_aux";
+
+static void print_usage(char *tool, int exit_code)
+{
+	printf("DPCD register read and write tool\n\n");
+	printf("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
+		 "to be set in the kernel config.\n\n");
+	printf("Usage: %s [OPTION ...] COMMAND\n\n", tool);
+	printf("COMMAND is one of:\n");
+	printf("  read:		Read [count] bytes dpcd reg at an offset\n");
+	printf("  write:	Write a dpcd reg at an offset\n\n");
+	printf("Options for the above COMMANDS are\n");
+	printf(" --device=DEVID		Aux device id, as listed in /dev/drm_dp_aux_dev[n]."
+					"Defaults to 0\n");
+	printf(" --offset=REG_ADDR	DPCD register offset in hex. Defaults to 0x00\n");
+	printf(" --count=BYTES		For reads, specify number of bytes to be read from"
+					"the offset. Defaults to 1\n");
+	printf(" --val			For writes, specify a hex value to be written\n\n");
+
+	printf(" --help: print the usage\n");
+
+	exit(exit_code);
+}
+
+static int dpcd_read(int fd, const uint32_t offset, size_t count)
+{
+	int ret, i;
+	void *buf = calloc(count, sizeof(uint8_t));
+
+	if (!buf) {
+		fprintf(stderr, "Can't allocate read buffer\n");
+		return ENOMEM;
+	}
+
+	ret = pread(fd, buf, count, offset);
+	if (ret != count) {
+		fprintf(stderr, "Failed to read - %s\n", strerror(errno));
+		ret = errno;
+		goto out;
+	} else
+		ret = EXIT_SUCCESS;
+
+	printf("Read %zu byte(s) starting at offset %x\n\n", count, offset);
+	for (i = 0; i < count; i++)
+		printf(" %02x", *(((uint8_t *)(buf)) + i));
+	printf("\n");
+
+out:
+	free(buf);
+	return ret;
+}
+
+static int dpcd_write(int fd, const uint32_t offset, const uint8_t val)
+{
+	int ret;
+
+	ret = pwrite(fd, (const void *)&val, sizeof(uint8_t), offset);
+	if (ret < 0) {
+		fprintf(stderr, "Failed to write - %s\n", strerror(errno));
+		return errno;
+	} else
+		return EXIT_SUCCESS;
+}
+
+int main(int argc, char **argv)
+{
+	char dev_name[20];
+	int ret, devid, fd, vflag = 0;
+	uint32_t offset;
+	uint8_t val;
+	size_t count;
+	int file_op = O_RDONLY;
+
+	enum command {
+		INV = -1,
+		READ = 2,
+		WRITE,
+	} cmd = INV;
+
+	struct option longopts[] = {
+		{ "count",	required_argument,	NULL,		'c' },
+		{ "device",	required_argument,	NULL,		'd' },
+		{ "help",	no_argument,		NULL,		'h' },
+		{ "offset",	required_argument,	NULL,		'o' },
+		{ "value",	required_argument,	&vflag,		'v' },
+		{ 0 }
+	};
+
+	devid = 0, count = 1, offset = 0x0;
+
+	while ((ret = getopt_long(argc, argv, "-:c:d:h:o:", longopts, NULL)) != -1) {
+		switch (ret) {
+		case 'c':
+			count = strtoul(optarg, NULL, 10);
+			if (count == ULONG_MAX) {
+				fprintf(stderr, "Count argument too big\n");
+				exit(ERANGE);
+			}
+			break;
+		case 'd':
+			devid = strtoul(optarg, NULL, 10);
+			if (devid == ULONG_MAX) {
+				fprintf(stderr, "Devid argument too big\n");
+				exit(ERANGE);
+			}
+			break;
+		case 'h':
+			print_usage(argv[0], EXIT_SUCCESS);
+			break;
+		case 'o':
+			offset = strtoul(optarg, NULL, 16);
+			if (offset > MAX_OFFSET) {
+				fprintf(stderr, "Offset should be <= 0xf02ff\n");
+				exit(ERANGE);
+			}
+			break;
+		case 0:
+			if (vflag == 'v' && optarg)
+				val = (uint8_t) strtoul(optarg, NULL, 16);
+			break;
+		/* Command parsing */
+		case 1:
+			if (strcmp(optarg, "read") == 0) {
+				cmd = READ;
+			} else if (strcmp(optarg, "write") == 0) {
+				cmd = WRITE;
+				file_op = O_WRONLY;
+			}
+			break;
+		case ':':
+			fprintf(stderr, "The -%c option of %s requires an argument\n",
+				 optopt, argv[0]);
+			print_usage(argv[0], EXIT_FAILURE);
+		case '?':
+		default:
+			fprintf(stderr, "%s - option %c is invalid\n", argv[0],
+				 optopt);
+			print_usage(argv[0], EXIT_FAILURE);
+		}
+	}
+
+	if ((count + offset) > (MAX_OFFSET + 1)) {
+		fprintf(stderr, "Out of bounds. Count + Offset <= 0xf0300\n");
+		exit(ERANGE);
+	}
+
+	if ((cmd == WRITE) && (vflag != 'v')) {
+		fprintf(stderr, "Write value is missing\n");
+		print_usage(argv[0], EXIT_FAILURE);
+	}
+
+	memset(dev_name, '\0', 20);
+	snprintf(dev_name, strlen(aux_dev) + 3, "%s%d", aux_dev, devid);
+
+	fd = open(dev_name, file_op);
+	if (fd < 0) {
+		fprintf(stderr, "Failed to open %s aux device - error: %s\n", dev_name,
+			strerror(errno));
+		return errno;
+	}
+
+	switch (cmd) {
+	case READ:
+		ret = dpcd_read(fd, offset, count);
+		break;
+	case WRITE:
+		ret = dpcd_write(fd, offset, val);
+		break;
+	case INV:
+	default:
+		fprintf(stderr, "Please specify a command: read/write. See usage\n");
+		close(fd);
+		print_usage(argv[0], EXIT_FAILURE);
+	}
+
+	close(fd);
+
+	return ret;
+}
diff --git a/tools/meson.build b/tools/meson.build
index e4517d66..79f36aa9 100644
--- a/tools/meson.build
+++ b/tools/meson.build
@@ -36,6 +36,7 @@ tools_progs = [
 	'intel_watermark',
 	'intel_gem_info',
 	'intel_gvtg_test',
+	'dpcd_reg',
 ]
 tool_deps = igt_deps
 
-- 
2.14.1

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [igt-dev] ✓ Fi.CI.BAT: success for tools: Add a simple tool to read/write/decode dpcd registers (rev3)
  2018-09-14 23:57 [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas
@ 2018-09-15  0:24 ` Patchwork
  2018-09-15  3:25 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2018-09-15  0:24 UTC (permalink / raw)
  To: Tarun Vyas; +Cc: igt-dev

== Series Details ==

Series: tools: Add a simple tool to read/write/decode dpcd registers (rev3)
URL   : https://patchwork.freedesktop.org/series/44736/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4827 -> IGTPW_1847 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/44736/revisions/3/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_coherency:
      fi-gdg-551:         PASS -> DMESG-FAIL (fdo#107164)

    igt@gem_exec_suspend@basic-s4-devices:
      fi-blb-e6850:       NOTRUN -> INCOMPLETE (fdo#107718)

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     PASS -> FAIL (fdo#103167)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
      fi-ilk-650:         PASS -> DMESG-WARN (fdo#106387)

    igt@kms_psr@primary_page_flip:
      fi-kbl-7560u:       PASS -> FAIL (fdo#107336)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s3:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence:
      fi-byt-clapper:     FAIL (fdo#107362, fdo#103191) -> PASS

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#106387 https://bugs.freedesktop.org/show_bug.cgi?id=106387
  fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718


== Participating hosts (48 -> 43) ==

  Missing    (5): fi-bsw-cyan fi-ilk-m540 fi-byt-squawks fi-hsw-4200u fi-pnv-d510 


== Build changes ==

    * IGT: IGT_4642 -> IGTPW_1847

  CI_DRM_4827: 8b1968f143e8bf65acf0ea6f7ce9120259043521 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1847: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1847/
  IGT_4642: 7b3ea4efb9713cd67e17e33202fa9d0681a284d1 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [igt-dev] ✗ Fi.CI.IGT: failure for tools: Add a simple tool to read/write/decode dpcd registers (rev3)
  2018-09-14 23:57 [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas
  2018-09-15  0:24 ` [igt-dev] ✓ Fi.CI.BAT: success for tools: Add a simple tool to read/write/decode dpcd registers (rev3) Patchwork
@ 2018-09-15  3:25 ` Patchwork
  2018-09-21  6:12 ` [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Dhinakaran Pandiyan
  2018-09-22 23:34 ` Dhinakaran Pandiyan
  3 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2018-09-15  3:25 UTC (permalink / raw)
  To: Tarun Vyas; +Cc: igt-dev

== Series Details ==

Series: tools: Add a simple tool to read/write/decode dpcd registers (rev3)
URL   : https://patchwork.freedesktop.org/series/44736/
State : failure

== Summary ==

= CI Bug Log - changes from IGT_4642_full -> IGTPW_1847_full =

== Summary - FAILURE ==

  Serious unknown changes coming with IGTPW_1847_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1847_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/44736/revisions/3/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_busy@extended-modeset-hang-newfb-render-c:
      shard-glk:          PASS -> DMESG-WARN +11

    igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
      shard-kbl:          PASS -> DMESG-WARN +8
      shard-snb:          NOTRUN -> DMESG-WARN

    igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-c:
      shard-apl:          PASS -> DMESG-WARN +10

    igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
      shard-hsw:          PASS -> DMESG-WARN +8
      shard-snb:          PASS -> DMESG-WARN +3

    
    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@sysfs-reader:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665) +1

    igt@gem_ctx_isolation@bcs0-s3:
      shard-apl:          PASS -> INCOMPLETE (fdo#103927)

    igt@gem_exec_await@wide-contexts:
      shard-glk:          PASS -> FAIL (fdo#106680)
      shard-kbl:          PASS -> FAIL (fdo#106680)

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

    igt@kms_cursor_crc@cursor-64x21-offscreen:
      shard-glk:          PASS -> INCOMPLETE (k.org#198133, fdo#103359)

    igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
      shard-glk:          PASS -> FAIL (fdo#105454, fdo#106509)

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

    igt@kms_plane@pixel-format-pipe-a-planes:
      shard-snb:          PASS -> FAIL (fdo#103166, fdo#107749)

    igt@kms_plane_scaling@pipe-a-scaler-with-rotation:
      shard-glk:          PASS -> DMESG-WARN (fdo#105763, fdo#106538) +1

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

    
    ==== Possible fixes ====

    igt@gem_exec_schedule@preempt-hang-bsd1:
      shard-snb:          INCOMPLETE (fdo#105411) -> SKIP

    igt@kms_flip@2x-blocking-absolute-wf_vblank:
      shard-glk:          INCOMPLETE (k.org#198133, fdo#103359) -> PASS

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-mmap-cpu:
      shard-glk:          FAIL (fdo#103167) -> PASS +2

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

    igt@kms_vblank@pipe-b-wait-forked-busy-hang:
      shard-glk:          DMESG-WARN (fdo#105763, fdo#106538) -> PASS +2

    
  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#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#106680 https://bugs.freedesktop.org/show_bug.cgi?id=106680
  fdo#107749 https://bugs.freedesktop.org/show_bug.cgi?id=107749
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


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

  No changes in participating hosts


== Build changes ==

    * IGT: IGT_4642 -> IGTPW_1847
    * Linux: CI_DRM_4825 -> CI_DRM_4827

  CI_DRM_4825: b42528aaa961c0d469f381b4a5c3830fe46aedfa @ git://anongit.freedesktop.org/gfx-ci/linux
  CI_DRM_4827: 8b1968f143e8bf65acf0ea6f7ce9120259043521 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1847: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1847/
  IGT_4642: 7b3ea4efb9713cd67e17e33202fa9d0681a284d1 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers
  2018-09-14 23:57 [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas
  2018-09-15  0:24 ` [igt-dev] ✓ Fi.CI.BAT: success for tools: Add a simple tool to read/write/decode dpcd registers (rev3) Patchwork
  2018-09-15  3:25 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
@ 2018-09-21  6:12 ` Dhinakaran Pandiyan
  2018-09-21 20:36   ` Tarun Vyas
  2018-09-22 23:34 ` Dhinakaran Pandiyan
  3 siblings, 1 reply; 8+ messages in thread
From: Dhinakaran Pandiyan @ 2018-09-21  6:12 UTC (permalink / raw)
  To: igt-dev; +Cc: rodrigo.vivi

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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


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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

-DK



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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers
  2018-09-21  6:12 ` [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Dhinakaran Pandiyan
@ 2018-09-21 20:36   ` Tarun Vyas
  2018-09-21 21:09     ` Pandiyan, Dhinakaran
  0 siblings, 1 reply; 8+ messages in thread
From: Tarun Vyas @ 2018-09-21 20:36 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: igt-dev, rodrigo.vivi

On Thu, Sep 20, 2018 at 11:12:49PM -0700, Dhinakaran Pandiyan wrote:
> On Friday, September 14, 2018 4:57:02 PM PDT Tarun Vyas wrote:
> > This tool serves as a wrapper around the constructs provided by the
> > drm_dpcd_aux_dev kernel module by working on the /dev/drm_dp_aux[n]
> > devices created by the kernel module.
> > It supports reading and writing dpcd registers on the connected aux
> > channels.
> > In the follow-up patch, support for decoding these registers will be
> > added to facilate debugging panel related issues.
> > 
> > v2: (Fixes by Rodrigo but no functional changes yet):
> >     - Indentations, Typo, Missed spaces
> >     - Removing mentioning to decode and spec that is not implemented yet.
> >     - Add Makefile.sources back
> >     - Missed s/printf/igt_warn
> > 
> > v3:
> >     - Addres DK's review comments from v2 above.
> >     - Squash Rodrigo's file handling unification patch.
> >     - Make count, offset and device id optional.
> > 
> > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  tools/Makefile.sources |   1 +
> >  tools/dpcd_reg.c       | 213
> > +++++++++++++++++++++++++++++++++++++++++++++++++ tools/meson.build      | 
> >  1 +
> >  3 files changed, 215 insertions(+)
> >  create mode 100644 tools/dpcd_reg.c
> > 
> > diff --git a/tools/Makefile.sources b/tools/Makefile.sources
> > index abd23a0f..50706f41 100644
> > --- a/tools/Makefile.sources
> > +++ b/tools/Makefile.sources
> > @@ -7,6 +7,7 @@ noinst_PROGRAMS =		\
> > 
> >  tools_prog_lists =		\
> >  	igt_stats		\
> > +	dpcd_reg		\
> >  	intel_audio_dump	\
> >  	intel_reg		\
> >  	intel_backlight		\
> > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> > new file mode 100644
> > index 00000000..cd9fed4f
> > --- /dev/null
> > +++ b/tools/dpcd_reg.c
> > @@ -0,0 +1,213 @@
> > +/*
> > + * Copyright © 2018 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"), + * to deal in the Software without restriction, including
> > without limitation + * the rights to use, copy, modify, merge, publish,
> > distribute, sublicense, + * and/or sell copies of the Software, and to
> > permit persons to whom the + * Software is furnished to do so, subject to
> > the following conditions: + *
> > + * The above copyright notice and this permission notice (including the
> > next + * paragraph) shall be included in all copies or substantial portions
> > of the + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> > OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. 
> > IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
> > CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> > TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE
> > SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE.
> > + *
> > + * DPCD register read/write tool
> > + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register
> > read + * and write, so CONFIG_DRM_DP_AUX_DEV needs to be set.
> > + */
> > +
> > +#include "igt_core.h"
> What's the dependency on igt_core here?
>
Forgot to remove it. 
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <limits.h>
> > +
> > +#define MAX_OFFSET	0xf02ff
> I think we should be able to allow allow up to the max limit of 0xfffff (DP 
> 1.4 section 2.9.3)
> 
Will modify.
> > +
> > +const char aux_dev[] = "/dev/drm_dp_aux";
> > +
> > +static void print_usage(char *tool, int exit_code)
> > +{
> > +	printf("DPCD register read and write tool\n\n");
> > +	printf("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
> > +		 "to be set in the kernel config.\n\n");
> I think it would look better to print the above lines only when --help/-h was 
> passed.  Move this under  case 'h' ?
> 
Will do
> > +	printf("Usage: %s [OPTION ...] COMMAND\n\n", tool);
> > +	printf("COMMAND is one of:\n");
> > +	printf("  read:		Read [count] bytes dpcd reg at an offset\n");
> > +	printf("  write:	Write a dpcd reg at an offset\n\n");
> > +	printf("Options for the above COMMANDS are\n");
> > +	printf(" --device=DEVID		Aux device id, as listed in
> > /dev/drm_dp_aux_dev[n]." +					"Defaults to 0\n");
> > +	printf(" --offset=REG_ADDR	DPCD register offset in hex. Defaults to
> > 0x00\n"); +	printf(" --count=BYTES		For reads, specify number of bytes to
> > be read from" +					"the offset. Defaults to 1\n");
> > +	printf(" --val			For writes, specify a hex value to be written\n\n");
> > +
> > +	printf(" --help: print the usage\n");
> > +
> > +	exit(exit_code);
> Modify the callers so that you don't have to exit from a print function. 
>
Will do. 
> > +}
> > +
> > +static int dpcd_read(int fd, const uint32_t offset, size_t count)
> const is not needed as the arguments are passed by value.
> 
Will remove it.
> > +{
> > +	int ret, i;
> > +	void *buf = calloc(count, sizeof(uint8_t));
> uint8_t *buf ?
> 
Heh, yea.
> > +
> > +	if (!buf) {
> > +		fprintf(stderr, "Can't allocate read buffer\n");
> > +		return ENOMEM;
> > +	}
> > +
> > +	ret = pread(fd, buf, count, offset);
> > +	if (ret != count) {
> > +		fprintf(stderr, "Failed to read - %s\n", strerror(errno));
> Print ret too? If the number of bytes read were lower, errno won't be set.
> 
Ok
> > +		ret = errno;
> > +		goto out;
> > +	} else
> } else {
> 
> > +		ret = EXIT_SUCCESS;
> Intialize ret  and get rid of the else block?
> 
Aye
> > +
> > +	printf("Read %zu byte(s) starting at offset %x\n\n", count, offset);
> Print this debug message only if the expected number of bytes weren't read and 
> something like printf("Read %zu bytes, expected %zu\n");
> 
> printf("0x"); to clarify that the printed values are in hex.
Will modify
> > +	for (i = 0; i < count; i++)
> > +		printf(" %02x", *(((uint8_t *)(buf)) + i));
> You can avoid typecasting if you define the array as type uint8_t
> 
> > +	printf("\n");
> > +
> > +out:
> > +	free(buf);
> > +	return ret;
> > +}
> > +
> > +static int dpcd_write(int fd, const uint32_t offset, const uint8_t val)
> const isn't needed.
> 
> > +{
> > +	int ret;
> > +
> > +	ret = pwrite(fd, (const void *)&val, sizeof(uint8_t), offset);
> > +	if (ret < 0) {
> > +		fprintf(stderr, "Failed to write - %s\n", strerror(errno));
> Same as above, print number of bytes written in case of error.
> 
> > +		return errno;
> > +	} else
> > +		return EXIT_SUCCESS;
> You could avoid 'else' here.
> 
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +	char dev_name[20];
> > +	int ret, devid, fd, vflag = 0;
> > +	uint32_t offset;
> > +	uint8_t val;
> > +	size_t count;
> > +	int file_op = O_RDONLY;
> > +
> > +	enum command {
> > +		INV = -1,
> > +		READ = 2,
> Any reason to not use the value compiler generates?
> 
I wanted to init INVALID with an -ve value. Just following the trend from intel_reg.
> > +		WRITE,
> > +	} cmd = INV;
> > +
> > +	struct option longopts[] = {
> > +		{ "count",	required_argument,	NULL,		'c' },
> > +		{ "device",	required_argument,	NULL,		'd' },
> > +		{ "help",	no_argument,		NULL,		'h' },
> > +		{ "offset",	required_argument,	NULL,		'o' },
> > +		{ "value",	required_argument,	&vflag,		'v' },
> > +		{ 0 }
> > +	};
> > +
> > +	devid = 0, count = 1, offset = 0x0;
> > +
> > +	while ((ret = getopt_long(argc, argv, "-:c:d:h:o:", longopts, NULL)) !=
> Any reason to leave out the option -v ? And '-h' shouldn't need an argument.
> 
> 
I didnt wan't to rely on initializing the *write value* with an invalid , so based on the vflag, we'll know if the write value was ever set or not and error out accordingly.
getopt_long will only complain if the user supplies --value option but no argument with it. If the user chooses to completely skip the --value option then we won't know.
> > -1) { +		switch (ret) {
> > +		case 'c':
> > +			count = strtoul(optarg, NULL, 10);
> With strtol() you should be able reject all negative args and make use of the 
> second argument to reject invalid numbers.
> 
Hmm, makes sense.
> > +			if (count == ULONG_MAX) {
> > +				fprintf(stderr, "Count argument too big\n");
> > +				exit(ERANGE);
> > +			}
> > +			break;
> > +		case 'd':
> > +			devid = strtoul(optarg, NULL, 10);
> Same here, strtol() to reject negatives and use **endptr to fail on invalid 
> numbers.
> 
> > +			if (devid == ULONG_MAX) {
> > +				fprintf(stderr, "Devid argument too big\n");
> 
> > +				exit(ERANGE);
> > +			}
> > +			break;
> > +		case 'h':
> > +			print_usage(argv[0], EXIT_SUCCESS);
> > +			break;
> > +		case 'o':
> > +			offset = strtoul(optarg, NULL, 16);
> Same comment as for 'd'.  You might also want to check if errno is set.
> 
> > +			if (offset > MAX_OFFSET) {
> > +				fprintf(stderr, "Offset should be <= 0xf02ff\n");
> > +				exit(ERANGE);
> > +			}
> > +			break;
> > +		case 0:
> > +			if (vflag == 'v' && optarg)
> I didn't get why &vflag had to be used.
> 
My idea was to use the vflag to check if --value option was used at all or not.
> > +				val = (uint8_t) strtoul(optarg, NULL, 16);
> Reject values greater than 0xff?
> 
uint8_t cast should truncate everything, but yea, I'll explicitly check for anything > 0xff.
> > +			break;
> > +		/* Command parsing */
> > +		case 1:
> > +			if (strcmp(optarg, "read") == 0) {
> > +				cmd = READ;
> > +			} else if (strcmp(optarg, "write") == 0) {
> > +				cmd = WRITE;
> > +				file_op = O_WRONLY;
> > +			}
> > +			break;
>  } else {
>  and exit here?  or fall through to default block by re-arranging the blocks.
> 
Alright
> > +		case ':':
> > +			fprintf(stderr, "The -%c option of %s requires an argument\n",
> > +				 optopt, argv[0]);
> 
> Prints "The -c option of tools/dpcd_reg requires an argument", is that what 
> you intended? Skip printing the tool path?
> 
Oh no, the %c is the format specifier. So, if count, devid, offset etc. are passed w/o an argument,
then this will print something like "The -v option of tools/dpcd_reg requires an argument", 
for the value argument, in this case, or -d for the devid argument and so on.
> > +			print_usage(argv[0], EXIT_FAILURE);
> > +		case '?':
> > +		default:
> > +			fprintf(stderr, "%s - option %c is invalid\n", argv[0],
> > +				 optopt);
> > +			print_usage(argv[0], EXIT_FAILURE);
> > +		}
> > +	}
> > +
> > +	if ((count + offset) > (MAX_OFFSET + 1)) {
> > +		fprintf(stderr, "Out of bounds. Count + Offset <= 0xf0300\n");
> > +		exit(ERANGE);
> > +	}
> > +
> > +	if ((cmd == WRITE) && (vflag != 'v')) {
> > +		fprintf(stderr, "Write value is missing\n");
> > +		print_usage(argv[0], EXIT_FAILURE);
> > +	}
> > +
> > +	memset(dev_name, '\0', 20);
> Not needed as snprintf includes the null byte,
Ok.
> > +	snprintf(dev_name, strlen(aux_dev) + 3, "%s%d", aux_dev, devid);
> Allows a max of 99 for devid, which should be okay but define a macro and 
> reject opt args greater than that?
> 
Will modify.
> > +
> > +	fd = open(dev_name, file_op);
> > +	if (fd < 0) {
> > +		fprintf(stderr, "Failed to open %s aux device - error: %s\n", dev_name,
> > +			strerror(errno));
> > +		return errno;
> > +	}
> > +
> > +	switch (cmd) {
> > +	case READ:
> > +		ret = dpcd_read(fd, offset, count);
> > +		break;
> > +	case WRITE:
> > +		ret = dpcd_write(fd, offset, val);
> > +		break;
> > +	case INV:
> INVALID is easier to understand
> 
Right
> > +	default:
> > +		fprintf(stderr, "Please specify a command: read/write. See usage\n");
> Remove "See usage"
> 
> > +		close(fd);
> There's another close() below.
> 
print_usage was exiting so the other close() wouldnt be called. But with the removal of exit from print_usage, this will change.
> > +		print_usage(argv[0], EXIT_FAILURE);
> > +	}
> > +
> > +	close(fd);
> > +
> > +	return ret;
> > +}
> > diff --git a/tools/meson.build b/tools/meson.build
> > index e4517d66..79f36aa9 100644
> > --- a/tools/meson.build
> > +++ b/tools/meson.build
> > @@ -36,6 +36,7 @@ tools_progs = [
> >  	'intel_watermark',
> >  	'intel_gem_info',
> >  	'intel_gvtg_test',
> > +	'dpcd_reg',
> >  ]
> >  tool_deps = igt_deps
> 
> Thanks for working on this.  The tool looks good overall,. just needs some 
> minor polishing.
>
Thanks for the review :) 
> -DK
> 
> 
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers
  2018-09-21 20:36   ` Tarun Vyas
@ 2018-09-21 21:09     ` Pandiyan, Dhinakaran
  2018-09-21 22:10       ` Tarun Vyas
  0 siblings, 1 reply; 8+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-09-21 21:09 UTC (permalink / raw)
  To: Vyas, Tarun; +Cc: igt-dev@lists.freedesktop.org, Vivi, Rodrigo



> -----Original Message-----
> From: Vyas, Tarun
> Sent: Friday, September 21, 2018 1:36 PM
> To: Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com>
> Cc: igt-dev@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Subject: Re: [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to
> read/write/decode dpcd registers
> 
> On Thu, Sep 20, 2018 at 11:12:49PM -0700, Dhinakaran Pandiyan wrote:
> > On Friday, September 14, 2018 4:57:02 PM PDT Tarun Vyas wrote:
> > > This tool serves as a wrapper around the constructs provided by the
> > > drm_dpcd_aux_dev kernel module by working on the
> /dev/drm_dp_aux[n]
> > > devices created by the kernel module.
> > > It supports reading and writing dpcd registers on the connected aux
> > > channels.
> > > In the follow-up patch, support for decoding these registers will be
> > > added to facilate debugging panel related issues.
> > >
> > > v2: (Fixes by Rodrigo but no functional changes yet):
> > >     - Indentations, Typo, Missed spaces
> > >     - Removing mentioning to decode and spec that is not implemented
> yet.
> > >     - Add Makefile.sources back
> > >     - Missed s/printf/igt_warn
> > >
> > > v3:
> > >     - Addres DK's review comments from v2 above.
> > >     - Squash Rodrigo's file handling unification patch.
> > >     - Make count, offset and device id optional.
> > >
> > > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  tools/Makefile.sources |   1 +
> > >  tools/dpcd_reg.c       | 213
> > > +++++++++++++++++++++++++++++++++++++++++++++++++
> tools/meson.build      |
> > >  1 +
> > >  3 files changed, 215 insertions(+)
> > >  create mode 100644 tools/dpcd_reg.c
> > >
> > > diff --git a/tools/Makefile.sources b/tools/Makefile.sources index
> > > abd23a0f..50706f41 100644
> > > --- a/tools/Makefile.sources
> > > +++ b/tools/Makefile.sources
> > > @@ -7,6 +7,7 @@ noinst_PROGRAMS =		\
> > >
> > >  tools_prog_lists =		\
> > >  	igt_stats		\
> > > +	dpcd_reg		\
> > >  	intel_audio_dump	\
> > >  	intel_reg		\
> > >  	intel_backlight		\
> > > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c new file mode
> > > 100644 index 00000000..cd9fed4f
> > > --- /dev/null
> > > +++ b/tools/dpcd_reg.c
> > > @@ -0,0 +1,213 @@
> > > +/*
> > > + * Copyright (c) 2018 Intel Corporation
> > > + *
> > > + * Permission is hereby granted, free of charge, to any person
> > > +obtaining a
> > > + * copy of this software and associated documentation files (the
> > > "Software"), + * to deal in the Software without restriction,
> > > including without limitation + * the rights to use, copy, modify,
> > > merge, publish, distribute, sublicense, + * and/or sell copies of
> > > the Software, and to permit persons to whom the + * Software is
> > > furnished to do so, subject to the following conditions: + *
> > > + * The above copyright notice and this permission notice (including
> > > + the
> > > next + * paragraph) shall be included in all copies or substantial
> > > portions of the + * Software.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> KIND,
> > > + EXPRESS
> > > OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > > MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND
> NONINFRINGEMENT.
> > > IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE
> FOR
> > > ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION
> OF
> > > CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN
> > > CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> THE + * SOFTWARE.
> > > + *
> > > + * DPCD register read/write tool
> > > + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD
> > > + register
> > > read + * and write, so CONFIG_DRM_DP_AUX_DEV needs to be set.
> > > + */
> > > +
> > > +#include "igt_core.h"
> > What's the dependency on igt_core here?
> >
> Forgot to remove it.
> > > +#include <errno.h>
> > > +#include <fcntl.h>
> > > +#include <limits.h>
> > > +
> > > +#define MAX_OFFSET	0xf02ff
> > I think we should be able to allow allow up to the max limit of
> > 0xfffff (DP
> > 1.4 section 2.9.3)
> >
> Will modify.
> > > +
> > > +const char aux_dev[] = "/dev/drm_dp_aux";
> > > +
> > > +static void print_usage(char *tool, int exit_code) {
> > > +	printf("DPCD register read and write tool\n\n");
> > > +	printf("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
> > > +		 "to be set in the kernel config.\n\n");
> > I think it would look better to print the above lines only when
> > --help/-h was passed.  Move this under  case 'h' ?
> >
> Will do
> > > +	printf("Usage: %s [OPTION ...] COMMAND\n\n", tool);
> > > +	printf("COMMAND is one of:\n");
> > > +	printf("  read:		Read [count] bytes dpcd reg at an offset\n");
> > > +	printf("  write:	Write a dpcd reg at an offset\n\n");
> > > +	printf("Options for the above COMMANDS are\n");
> > > +	printf(" --device=DEVID		Aux device id, as listed in
> > > /dev/drm_dp_aux_dev[n]." +
> 	"Defaults to 0\n");
> > > +	printf(" --offset=REG_ADDR	DPCD register offset in hex. Defaults
> to
> > > 0x00\n"); +	printf(" --count=BYTES		For reads, specify number of
> bytes to
> > > be read from" +					"the offset. Defaults
> to 1\n");
> > > +	printf(" --val			For writes, specify a hex value to be
> written\n\n");
> > > +
> > > +	printf(" --help: print the usage\n");
> > > +
> > > +	exit(exit_code);
> > Modify the callers so that you don't have to exit from a print function.
> >
> Will do.
> > > +}
> > > +
> > > +static int dpcd_read(int fd, const uint32_t offset, size_t count)
> > const is not needed as the arguments are passed by value.
> >
> Will remove it.
> > > +{
> > > +	int ret, i;
> > > +	void *buf = calloc(count, sizeof(uint8_t));
> > uint8_t *buf ?
> >
> Heh, yea.
> > > +
> > > +	if (!buf) {
> > > +		fprintf(stderr, "Can't allocate read buffer\n");
> > > +		return ENOMEM;
> > > +	}
> > > +
> > > +	ret = pread(fd, buf, count, offset);
> > > +	if (ret != count) {
> > > +		fprintf(stderr, "Failed to read - %s\n", strerror(errno));
> > Print ret too? If the number of bytes read were lower, errno won't be set.
> >
> Ok
> > > +		ret = errno;
> > > +		goto out;
> > > +	} else
> > } else {
> >
> > > +		ret = EXIT_SUCCESS;
> > Intialize ret  and get rid of the else block?
> >
> Aye
> > > +
> > > +	printf("Read %zu byte(s) starting at offset %x\n\n", count,
> > > +offset);
> > Print this debug message only if the expected number of bytes weren't
> > read and something like printf("Read %zu bytes, expected %zu\n");
> >
> > printf("0x"); to clarify that the printed values are in hex.
> Will modify
> > > +	for (i = 0; i < count; i++)
> > > +		printf(" %02x", *(((uint8_t *)(buf)) + i));
> > You can avoid typecasting if you define the array as type uint8_t
> >
> > > +	printf("\n");
> > > +
> > > +out:
> > > +	free(buf);
> > > +	return ret;
> > > +}
> > > +
> > > +static int dpcd_write(int fd, const uint32_t offset, const uint8_t
> > > +val)
> > const isn't needed.
> >
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = pwrite(fd, (const void *)&val, sizeof(uint8_t), offset);
> > > +	if (ret < 0) {
> > > +		fprintf(stderr, "Failed to write - %s\n", strerror(errno));
> > Same as above, print number of bytes written in case of error.
> >
> > > +		return errno;
> > > +	} else
> > > +		return EXIT_SUCCESS;
> > You could avoid 'else' here.
> >
> > > +}
> > > +
> > > +int main(int argc, char **argv)
> > > +{
> > > +	char dev_name[20];
> > > +	int ret, devid, fd, vflag = 0;
> > > +	uint32_t offset;
> > > +	uint8_t val;
> > > +	size_t count;
> > > +	int file_op = O_RDONLY;
> > > +
> > > +	enum command {
> > > +		INV = -1,
> > > +		READ = 2,
> > Any reason to not use the value compiler generates?
> >
> I wanted to init INVALID with an -ve value. Just following the trend from
> intel_reg.
> > > +		WRITE,
> > > +	} cmd = INV;
> > > +
> > > +	struct option longopts[] = {
> > > +		{ "count",	required_argument,	NULL,		'c' },
> > > +		{ "device",	required_argument,	NULL,		'd' },
> > > +		{ "help",	no_argument,		NULL,		'h' },
> > > +		{ "offset",	required_argument,	NULL,		'o' },
> > > +		{ "value",	required_argument,	&vflag,		'v' },
> > > +		{ 0 }
> > > +	};
> > > +
> > > +	devid = 0, count = 1, offset = 0x0;
> > > +
> > > +	while ((ret = getopt_long(argc, argv, "-:c:d:h:o:", longopts,
> > > +NULL)) !=
> > Any reason to leave out the option -v ? And '-h' shouldn't need an
> argument.
> >
> >
> I didnt wan't to rely on initializing the *write value* with an invalid , so based
> on the vflag, we'll know if the write value was ever set or not and error out
> accordingly.
> getopt_long will only complain if the user supplies --value option but no
> argument with it. If the user chooses to completely skip the --value option
> then we won't know.

Shouldn't the tool accept -v <foo> too?

> > > -1) { +		switch (ret) {
> > > +		case 'c':
> > > +			count = strtoul(optarg, NULL, 10);
> > With strtol() you should be able reject all negative args and make use
> > of the second argument to reject invalid numbers.
> >
> Hmm, makes sense.
> > > +			if (count == ULONG_MAX) {
> > > +				fprintf(stderr, "Count argument too big\n");
> > > +				exit(ERANGE);
> > > +			}
> > > +			break;
> > > +		case 'd':
> > > +			devid = strtoul(optarg, NULL, 10);
> > Same here, strtol() to reject negatives and use **endptr to fail on
> > invalid numbers.
> >
> > > +			if (devid == ULONG_MAX) {
> > > +				fprintf(stderr, "Devid argument too big\n");
> >
> > > +				exit(ERANGE);
> > > +			}
> > > +			break;
> > > +		case 'h':
> > > +			print_usage(argv[0], EXIT_SUCCESS);
> > > +			break;
> > > +		case 'o':
> > > +			offset = strtoul(optarg, NULL, 16);
> > Same comment as for 'd'.  You might also want to check if errno is set.
> >
> > > +			if (offset > MAX_OFFSET) {
> > > +				fprintf(stderr, "Offset should be <=
> 0xf02ff\n");
> > > +				exit(ERANGE);
> > > +			}
> > > +			break;
> > > +		case 0:
> > > +			if (vflag == 'v' && optarg)
> > I didn't get why &vflag had to be used.
> >
> My idea was to use the vflag to check if --value option was used at all or not.

Makes sense. How about failing --value or -v was used along with read?

> > > +				val = (uint8_t) strtoul(optarg, NULL, 16);
> > Reject values greater than 0xff?
> >
> uint8_t cast should truncate everything, but yea, I'll explicitly check for
> anything > 0xff.
> > > +			break;
> > > +		/* Command parsing */
> > > +		case 1:
> > > +			if (strcmp(optarg, "read") == 0) {
> > > +				cmd = READ;
> > > +			} else if (strcmp(optarg, "write") == 0) {
> > > +				cmd = WRITE;
> > > +				file_op = O_WRONLY;
> > > +			}
> > > +			break;
> >  } else {
> >  and exit here?  or fall through to default block by re-arranging the blocks.
> >
> Alright
> > > +		case ':':
> > > +			fprintf(stderr, "The -%c option of %s requires an
> argument\n",
> > > +				 optopt, argv[0]);
> >
> > Prints "The -c option of tools/dpcd_reg requires an argument", is that
> > what you intended? Skip printing the tool path?
> >
> Oh no, the %c is the format specifier. So, if count, devid, offset etc. are
> passed w/o an argument, then this will print something like "The -v option of
> tools/dpcd_reg requires an argument", for the value argument, in this case,
> or -d for the devid argument and so on.

I meant printing "tools/dpcd_reg" was redundant and I used -c (count) without an arg as example.

-DK

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers
  2018-09-21 21:09     ` Pandiyan, Dhinakaran
@ 2018-09-21 22:10       ` Tarun Vyas
  0 siblings, 0 replies; 8+ messages in thread
From: Tarun Vyas @ 2018-09-21 22:10 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran; +Cc: igt-dev@lists.freedesktop.org, Vivi, Rodrigo

On Fri, Sep 21, 2018 at 02:09:36PM -0700, Pandiyan, Dhinakaran wrote:
> 
> 
> > -----Original Message-----
> > From: Vyas, Tarun
> > Sent: Friday, September 21, 2018 1:36 PM
> > To: Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com>
> > Cc: igt-dev@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Subject: Re: [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to
> > read/write/decode dpcd registers
> > 
> > On Thu, Sep 20, 2018 at 11:12:49PM -0700, Dhinakaran Pandiyan wrote:
> > > On Friday, September 14, 2018 4:57:02 PM PDT Tarun Vyas wrote:
> > > > This tool serves as a wrapper around the constructs provided by the
> > > > drm_dpcd_aux_dev kernel module by working on the
> > /dev/drm_dp_aux[n]
> > > > devices created by the kernel module.
> > > > It supports reading and writing dpcd registers on the connected aux
> > > > channels.
> > > > In the follow-up patch, support for decoding these registers will be
> > > > added to facilate debugging panel related issues.
> > > >
> > > > v2: (Fixes by Rodrigo but no functional changes yet):
> > > >     - Indentations, Typo, Missed spaces
> > > >     - Removing mentioning to decode and spec that is not implemented
> > yet.
> > > >     - Add Makefile.sources back
> > > >     - Missed s/printf/igt_warn
> > > >
> > > > v3:
> > > >     - Addres DK's review comments from v2 above.
> > > >     - Squash Rodrigo's file handling unification patch.
> > > >     - Make count, offset and device id optional.
> > > >
> > > > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >  tools/Makefile.sources |   1 +
> > > >  tools/dpcd_reg.c       | 213
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++
> > tools/meson.build      |
> > > >  1 +
> > > >  3 files changed, 215 insertions(+)
> > > >  create mode 100644 tools/dpcd_reg.c
> > > >
> > > > diff --git a/tools/Makefile.sources b/tools/Makefile.sources index
> > > > abd23a0f..50706f41 100644
> > > > --- a/tools/Makefile.sources
> > > > +++ b/tools/Makefile.sources
> > > > @@ -7,6 +7,7 @@ noinst_PROGRAMS =		\
> > > >
> > > >  tools_prog_lists =		\
> > > >  	igt_stats		\
> > > > +	dpcd_reg		\
> > > >  	intel_audio_dump	\
> > > >  	intel_reg		\
> > > >  	intel_backlight		\
> > > > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c new file mode
> > > > 100644 index 00000000..cd9fed4f
> > > > --- /dev/null
> > > > +++ b/tools/dpcd_reg.c
> > > > @@ -0,0 +1,213 @@
> > > > +/*
> > > > + * Copyright (c) 2018 Intel Corporation
> > > > + *
> > > > + * Permission is hereby granted, free of charge, to any person
> > > > +obtaining a
> > > > + * copy of this software and associated documentation files (the
> > > > "Software"), + * to deal in the Software without restriction,
> > > > including without limitation + * the rights to use, copy, modify,
> > > > merge, publish, distribute, sublicense, + * and/or sell copies of
> > > > the Software, and to permit persons to whom the + * Software is
> > > > furnished to do so, subject to the following conditions: + *
> > > > + * The above copyright notice and this permission notice (including
> > > > + the
> > > > next + * paragraph) shall be included in all copies or substantial
> > > > portions of the + * Software.
> > > > + *
> > > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> > KIND,
> > > > + EXPRESS
> > > > OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > > > MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND
> > NONINFRINGEMENT.
> > > > IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE
> > FOR
> > > > ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION
> > OF
> > > > CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN
> > > > CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> > THE + * SOFTWARE.
> > > > + *
> > > > + * DPCD register read/write tool
> > > > + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD
> > > > + register
> > > > read + * and write, so CONFIG_DRM_DP_AUX_DEV needs to be set.
> > > > + */
> > > > +
> > > > +#include "igt_core.h"
> > > What's the dependency on igt_core here?
> > >
> > Forgot to remove it.
> > > > +#include <errno.h>
> > > > +#include <fcntl.h>
> > > > +#include <limits.h>
> > > > +
> > > > +#define MAX_OFFSET	0xf02ff
> > > I think we should be able to allow allow up to the max limit of
> > > 0xfffff (DP
> > > 1.4 section 2.9.3)
> > >
> > Will modify.
> > > > +
> > > > +const char aux_dev[] = "/dev/drm_dp_aux";
> > > > +
> > > > +static void print_usage(char *tool, int exit_code) {
> > > > +	printf("DPCD register read and write tool\n\n");
> > > > +	printf("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
> > > > +		 "to be set in the kernel config.\n\n");
> > > I think it would look better to print the above lines only when
> > > --help/-h was passed.  Move this under  case 'h' ?
> > >
> > Will do
> > > > +	printf("Usage: %s [OPTION ...] COMMAND\n\n", tool);
> > > > +	printf("COMMAND is one of:\n");
> > > > +	printf("  read:		Read [count] bytes dpcd reg at an offset\n");
> > > > +	printf("  write:	Write a dpcd reg at an offset\n\n");
> > > > +	printf("Options for the above COMMANDS are\n");
> > > > +	printf(" --device=DEVID		Aux device id, as listed in
> > > > /dev/drm_dp_aux_dev[n]." +
> > 	"Defaults to 0\n");
> > > > +	printf(" --offset=REG_ADDR	DPCD register offset in hex. Defaults
> > to
> > > > 0x00\n"); +	printf(" --count=BYTES		For reads, specify number of
> > bytes to
> > > > be read from" +					"the offset. Defaults
> > to 1\n");
> > > > +	printf(" --val			For writes, specify a hex value to be
> > written\n\n");
> > > > +
> > > > +	printf(" --help: print the usage\n");
> > > > +
> > > > +	exit(exit_code);
> > > Modify the callers so that you don't have to exit from a print function.
> > >
> > Will do.
> > > > +}
> > > > +
> > > > +static int dpcd_read(int fd, const uint32_t offset, size_t count)
> > > const is not needed as the arguments are passed by value.
> > >
> > Will remove it.
> > > > +{
> > > > +	int ret, i;
> > > > +	void *buf = calloc(count, sizeof(uint8_t));
> > > uint8_t *buf ?
> > >
> > Heh, yea.
> > > > +
> > > > +	if (!buf) {
> > > > +		fprintf(stderr, "Can't allocate read buffer\n");
> > > > +		return ENOMEM;
> > > > +	}
> > > > +
> > > > +	ret = pread(fd, buf, count, offset);
> > > > +	if (ret != count) {
> > > > +		fprintf(stderr, "Failed to read - %s\n", strerror(errno));
> > > Print ret too? If the number of bytes read were lower, errno won't be set.
> > >
> > Ok
> > > > +		ret = errno;
> > > > +		goto out;
> > > > +	} else
> > > } else {
> > >
> > > > +		ret = EXIT_SUCCESS;
> > > Intialize ret  and get rid of the else block?
> > >
> > Aye
> > > > +
> > > > +	printf("Read %zu byte(s) starting at offset %x\n\n", count,
> > > > +offset);
> > > Print this debug message only if the expected number of bytes weren't
> > > read and something like printf("Read %zu bytes, expected %zu\n");
> > >
> > > printf("0x"); to clarify that the printed values are in hex.
> > Will modify
> > > > +	for (i = 0; i < count; i++)
> > > > +		printf(" %02x", *(((uint8_t *)(buf)) + i));
> > > You can avoid typecasting if you define the array as type uint8_t
> > >
> > > > +	printf("\n");
> > > > +
> > > > +out:
> > > > +	free(buf);
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int dpcd_write(int fd, const uint32_t offset, const uint8_t
> > > > +val)
> > > const isn't needed.
> > >
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = pwrite(fd, (const void *)&val, sizeof(uint8_t), offset);
> > > > +	if (ret < 0) {
> > > > +		fprintf(stderr, "Failed to write - %s\n", strerror(errno));
> > > Same as above, print number of bytes written in case of error.
> > >
> > > > +		return errno;
> > > > +	} else
> > > > +		return EXIT_SUCCESS;
> > > You could avoid 'else' here.
> > >
> > > > +}
> > > > +
> > > > +int main(int argc, char **argv)
> > > > +{
> > > > +	char dev_name[20];
> > > > +	int ret, devid, fd, vflag = 0;
> > > > +	uint32_t offset;
> > > > +	uint8_t val;
> > > > +	size_t count;
> > > > +	int file_op = O_RDONLY;
> > > > +
> > > > +	enum command {
> > > > +		INV = -1,
> > > > +		READ = 2,
> > > Any reason to not use the value compiler generates?
> > >
> > I wanted to init INVALID with an -ve value. Just following the trend from
> > intel_reg.
> > > > +		WRITE,
> > > > +	} cmd = INV;
> > > > +
> > > > +	struct option longopts[] = {
> > > > +		{ "count",	required_argument,	NULL,		'c' },
> > > > +		{ "device",	required_argument,	NULL,		'd' },
> > > > +		{ "help",	no_argument,		NULL,		'h' },
> > > > +		{ "offset",	required_argument,	NULL,		'o' },
> > > > +		{ "value",	required_argument,	&vflag,		'v' },
> > > > +		{ 0 }
> > > > +	};
> > > > +
> > > > +	devid = 0, count = 1, offset = 0x0;
> > > > +
> > > > +	while ((ret = getopt_long(argc, argv, "-:c:d:h:o:", longopts,
> > > > +NULL)) !=
> > > Any reason to leave out the option -v ? And '-h' shouldn't need an
> > argument.
> > >
> > >
> > I didnt wan't to rely on initializing the *write value* with an invalid , so based
> > on the vflag, we'll know if the write value was ever set or not and error out
> > accordingly.
> > getopt_long will only complain if the user supplies --value option but no
> > argument with it. If the user chooses to completely skip the --value option
> > then we won't know.
> 
> Shouldn't the tool accept -v <foo> too?
>
Yea getopt_long will accept both short and long options. Both of them will be caught by the same case in the switch. 
> > > > -1) { +		switch (ret) {
> > > > +		case 'c':
> > > > +			count = strtoul(optarg, NULL, 10);
> > > With strtol() you should be able reject all negative args and make use
> > > of the second argument to reject invalid numbers.
> > >
> > Hmm, makes sense.
> > > > +			if (count == ULONG_MAX) {
> > > > +				fprintf(stderr, "Count argument too big\n");
> > > > +				exit(ERANGE);
> > > > +			}
> > > > +			break;
> > > > +		case 'd':
> > > > +			devid = strtoul(optarg, NULL, 10);
> > > Same here, strtol() to reject negatives and use **endptr to fail on
> > > invalid numbers.
> > >
> > > > +			if (devid == ULONG_MAX) {
> > > > +				fprintf(stderr, "Devid argument too big\n");
> > >
> > > > +				exit(ERANGE);
> > > > +			}
> > > > +			break;
> > > > +		case 'h':
> > > > +			print_usage(argv[0], EXIT_SUCCESS);
> > > > +			break;
> > > > +		case 'o':
> > > > +			offset = strtoul(optarg, NULL, 16);
> > > Same comment as for 'd'.  You might also want to check if errno is set.
> > >
> > > > +			if (offset > MAX_OFFSET) {
> > > > +				fprintf(stderr, "Offset should be <=
> > 0xf02ff\n");
> > > > +				exit(ERANGE);
> > > > +			}
> > > > +			break;
> > > > +		case 0:
> > > > +			if (vflag == 'v' && optarg)
> > > I didn't get why &vflag had to be used.
> > >
> > My idea was to use the vflag to check if --value option was used at all or not.
> 
> Makes sense. How about failing --value or -v was used along with read?
> 
I thought of that, but left it out because we dont pass it the read function, so even if someone inadvertently does that, the tool will still dump the reg value. I can change that if you want.
> > > > +				val = (uint8_t) strtoul(optarg, NULL, 16);
> > > Reject values greater than 0xff?
> > >
> > uint8_t cast should truncate everything, but yea, I'll explicitly check for
> > anything > 0xff.
> > > > +			break;
> > > > +		/* Command parsing */
> > > > +		case 1:
> > > > +			if (strcmp(optarg, "read") == 0) {
> > > > +				cmd = READ;
> > > > +			} else if (strcmp(optarg, "write") == 0) {
> > > > +				cmd = WRITE;
> > > > +				file_op = O_WRONLY;
> > > > +			}
> > > > +			break;
> > >  } else {
> > >  and exit here?  or fall through to default block by re-arranging the blocks.
> > >
> > Alright
> > > > +		case ':':
> > > > +			fprintf(stderr, "The -%c option of %s requires an
> > argument\n",
> > > > +				 optopt, argv[0]);
> > >
> > > Prints "The -c option of tools/dpcd_reg requires an argument", is that
> > > what you intended? Skip printing the tool path?
> > >
> > Oh no, the %c is the format specifier. So, if count, devid, offset etc. are
> > passed w/o an argument, then this will print something like "The -v option of
> > tools/dpcd_reg requires an argument", for the value argument, in this case,
> > or -d for the devid argument and so on.
> 
> I meant printing "tools/dpcd_reg" was redundant and I used -c (count) without an arg as example.
> 
Ah ok, will remove it.
> -DK
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers
  2018-09-14 23:57 [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas
                   ` (2 preceding siblings ...)
  2018-09-21  6:12 ` [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Dhinakaran Pandiyan
@ 2018-09-22 23:34 ` Dhinakaran Pandiyan
  3 siblings, 0 replies; 8+ messages in thread
From: Dhinakaran Pandiyan @ 2018-09-22 23:34 UTC (permalink / raw)
  To: Tarun Vyas, igt-dev; +Cc: rodrigo.vivi

On Fri, 2018-09-14 at 16:57 -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.
> 
> v2: (Fixes by Rodrigo but no functional changes yet):
>     - Indentations, Typo, Missed spaces
>     - Removing mentioning to decode and spec that is not implemented
> yet.
>     - Add Makefile.sources back
>     - Missed s/printf/igt_warn
> 
> v3:
>     - Addres DK's review comments from v2 above.
>     - Squash Rodrigo's file handling unification patch.
>     - Make count, offset and device id optional.
> 
> Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  tools/Makefile.sources |   1 +
>  tools/dpcd_reg.c       | 213
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/meson.build      |   1 +
>  3 files changed, 215 insertions(+)
>  create mode 100644 tools/dpcd_reg.c
> 
> diff --git a/tools/Makefile.sources b/tools/Makefile.sources
> index abd23a0f..50706f41 100644
> --- a/tools/Makefile.sources
> +++ b/tools/Makefile.sources
> @@ -7,6 +7,7 @@ noinst_PROGRAMS =		\
>  
>  tools_prog_lists =		\
>  	igt_stats		\
> +	dpcd_reg		\
>  	intel_audio_dump	\
>  	intel_reg		\
>  	intel_backlight		\
> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> new file mode 100644
> index 00000000..cd9fed4f
> --- /dev/null
> +++ b/tools/dpcd_reg.c
> @@ -0,0 +1,213 @@
> +/*
> + * Copyright © 2018 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person
> obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> the
> + * Software is furnished to do so, subject to the following
> conditions:
> + *
> + * The above copyright notice and this permission notice (including
> the next
> + * paragraph) shall be included in all copies or substantial
> portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
> OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS IN THE
> + * SOFTWARE.
> + *
> + * DPCD register read/write tool
> + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD
> register read
> + * and write, so CONFIG_DRM_DP_AUX_DEV needs to be set.
> + */
> +
> +#include "igt_core.h"
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <limits.h>
> +
> +#define MAX_OFFSET	0xf02ff
> +
> +const char aux_dev[] = "/dev/drm_dp_aux";
> +
> +static void print_usage(char *tool, int exit_code)
> +{
> +	printf("DPCD register read and write tool\n\n");
> +	printf("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
> +		 "to be set in the kernel config.\n\n");
> +	printf("Usage: %s [OPTION ...] COMMAND\n\n", tool);
> +	printf("COMMAND is one of:\n");
> +	printf("  read:		Read [count] bytes dpcd reg
> at an offset\n");
> +	printf("  write:	Write a dpcd reg at an offset\n\n");
> +	printf("Options for the above COMMANDS are\n");
> +	printf(" --device=DEVID		Aux device id, as
> listed in /dev/drm_dp_aux_dev[n]."
> +					"Defaults to 0\n");
> +	printf(" --offset=REG_ADDR	DPCD register offset in
> hex. Defaults to 0x00\n");
> +	printf(" --count=BYTES		For reads, specify
> number of bytes to be read from"
> +					"the offset. Defaults to
> 1\n");
> +	printf(" --val			For writes, specify a
> hex value to be written\n\n");
> +
> +	printf(" --help: print the usage\n");
> +
> +	exit(exit_code);
> +}
> +
> +static int dpcd_read(int fd, const uint32_t offset, size_t count)
> +{
> +	int ret, i;
> +	void *buf = calloc(count, sizeof(uint8_t));
> +
> +	if (!buf) {
> +		fprintf(stderr, "Can't allocate read buffer\n");
> +		return ENOMEM;
> +	}
> +
> +	ret = pread(fd, buf, count, offset);
> +	if (ret != count) {
> +		fprintf(stderr, "Failed to read - %s\n",
> strerror(errno));
> +		ret = errno;
> +		goto out;
> +	} else
> +		ret = EXIT_SUCCESS;
> +
> +	printf("Read %zu byte(s) starting at offset %x\n\n", count,
> offset);
> +	for (i = 0; i < count; i++)
> +		printf(" %02x", *(((uint8_t *)(buf)) + i));
> +	printf("\n");
> +
> +out:
> +	free(buf);
> +	return ret;
> +}
> +
> +static int dpcd_write(int fd, const uint32_t offset, const uint8_t
> val)
> +{
> +	int ret;
> +
> +	ret = pwrite(fd, (const void *)&val, sizeof(uint8_t),
> offset);
> +	if (ret < 0) {
> +		fprintf(stderr, "Failed to write - %s\n",
> strerror(errno));
> +		return errno;
> +	} else
> +		return EXIT_SUCCESS;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	char dev_name[20];
> +	int ret, devid, fd, vflag = 0;
> +	uint32_t offset;
> +	uint8_t val;
> +	size_t count;
> +	int file_op = O_RDONLY;
> +
> +	enum command {
> +		INV = -1,
> +		READ = 2,
> +		WRITE,
> +	} cmd = INV;
> +
> +	struct option longopts[] = {
> +		{ "count",	required_argument,	NULL,	
> 	'c' },
> +		{ "device",	required_argument,	NULL,	
> 	'd' },
> +		{ "help",	no_argument,		NULL,	
> 	'h' },
> +		{ "offset",	required_argument,	NULL,	
> 	'o' },
> +		{ "value",	required_argument,	&vflag,	
> 	'v' },
> +		{ 0 }
> +	};
> +
> +	devid = 0, count = 1, offset = 0x0;
> +
> +	while ((ret = getopt_long(argc, argv, "-:c:d:h:o:",
> longopts, NULL)) != -1) {
> +		switch (ret) {
> +		case 'c':
> +			count = strtoul(optarg, NULL, 10);
> +			if (count == ULONG_MAX) {
> +				fprintf(stderr, "Count argument too
> big\n");
> +				exit(ERANGE);
> +			}
> +			break;
> +		case 'd':
> +			devid = strtoul(optarg, NULL, 10);
> +			if (devid == ULONG_MAX) {
> +				fprintf(stderr, "Devid argument too
> big\n");
> +				exit(ERANGE);
> +			}
> +			break;
> +		case 'h':
> +			print_usage(argv[0], EXIT_SUCCESS);
> +			break;
> +		case 'o':
> +			offset = strtoul(optarg, NULL, 16);
> +			if (offset > MAX_OFFSET) {
> +				fprintf(stderr, "Offset should be <=
> 0xf02ff\n");
> +				exit(ERANGE);
> +			}
> +			break;
> +		case 0:
> +			if (vflag == 'v' && optarg)
> +				val = (uint8_t) strtoul(optarg,
> NULL, 16);
> +			break;
> +		/* Command parsing */
> +		case 1:
> +			if (strcmp(optarg, "read") == 0) {
> +				cmd = READ;
> +			} else if (strcmp(optarg, "write") == 0) {
> +				cmd = WRITE;
> +				file_op = O_WRONLY;
> +			}
> +			break;
> +		case ':':
> +			fprintf(stderr, "The -%c option of %s
> requires an argument\n",
> +				 optopt, argv[0]);
> +			print_usage(argv[0], EXIT_FAILURE);
> +		case '?':
> +		default:
> +			fprintf(stderr, "%s - option %c is
> invalid\n", argv[0],
> +				 optopt);
> +			print_usage(argv[0], EXIT_FAILURE);
> +		}
> +	}
> +
> +	if ((count + offset) > (MAX_OFFSET + 1)) {
> +		fprintf(stderr, "Out of bounds. Count + Offset <=
> 0xf0300\n");
> +		exit(ERANGE);
> +	}
> +
> +	if ((cmd == WRITE) && (vflag != 'v')) {
> +		fprintf(stderr, "Write value is missing\n");
> +		print_usage(argv[0], EXIT_FAILURE);
> +	}
> +
Another change you might want to consider is wrap all the parsing code
above in a function and make it fill a

struct data {
	int devid;
	enum command command;
	int offset;
	int count;
	uint8_t wval;
};

and return error or 0 from there. You could use an union for count and
val as their use is mutually exclusive.

ret = parse_options(&data);
if (ret) {
	print_usage()
	exit(ret)
}

> +	memset(dev_name, '\0', 20);
> +	snprintf(dev_name, strlen(aux_dev) + 3, "%s%d", aux_dev,
> devid);
> +
> +	fd = open(dev_name, file_op);
> +	if (fd < 0) {
> +		fprintf(stderr, "Failed to open %s aux device -
> error: %s\n", dev_name,
> +			strerror(errno));
> +		return errno;
> +	}
> +
> +	switch (cmd) {
> +	case READ:
> +		ret = dpcd_read(fd, offset, count);
> +		break;
> +	case WRITE:
> +		ret = dpcd_write(fd, offset, val);
> +		break;
> +	case INV:
> +	default:
> +		fprintf(stderr, "Please specify a command:
> read/write. See usage\n");
> +		close(fd);
> +		print_usage(argv[0], EXIT_FAILURE);
> +	}
> +	close(fd);
> +
> +	return ret;
> +}
> diff --git a/tools/meson.build b/tools/meson.build
> index e4517d66..79f36aa9 100644
> --- a/tools/meson.build
> +++ b/tools/meson.build
> @@ -36,6 +36,7 @@ tools_progs = [
>  	'intel_watermark',
>  	'intel_gem_info',
>  	'intel_gvtg_test',
> +	'dpcd_reg',
>  ]
>  tool_deps = igt_deps
>  
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-09-22 23:34 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-14 23:57 [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas
2018-09-15  0:24 ` [igt-dev] ✓ Fi.CI.BAT: success for tools: Add a simple tool to read/write/decode dpcd registers (rev3) Patchwork
2018-09-15  3:25 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
2018-09-21  6:12 ` [igt-dev] [PATCH i-g-t v3] tools: Add a simple tool to read/write/decode dpcd registers Dhinakaran Pandiyan
2018-09-21 20:36   ` Tarun Vyas
2018-09-21 21:09     ` Pandiyan, Dhinakaran
2018-09-21 22:10       ` Tarun Vyas
2018-09-22 23:34 ` Dhinakaran Pandiyan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox