All of lore.kernel.org
 help / color / mirror / Atom feed
* [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers
@ 2018-06-14  7:11 Tarun Vyas
  2018-06-14  8:08 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Tarun Vyas @ 2018-06-14  7:11 UTC (permalink / raw)
  To: igt-dev; +Cc: dhinakaran.pandiyan

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

Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
---
 tools/Makefile.am      |   2 +-
 tools/Makefile.sources |   4 +-
 tools/dpcd_reg.c       | 219 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 223 insertions(+), 2 deletions(-)
 create mode 100644 tools/dpcd_reg.c

diff --git a/tools/Makefile.am b/tools/Makefile.am
index 09b6dbcc3ece..b78824dadfdb 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN)
 intel_error_decode_LDFLAGS = -lz
 endif
 
-bin_PROGRAMS += intel_dp_compliance
+bin_PROGRAMS += intel_dp_compliance dpcd_reg
 intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS)
 intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la
 
diff --git a/tools/Makefile.sources b/tools/Makefile.sources
index abd23a0f4628..db606b28560f 100644
--- a/tools/Makefile.sources
+++ b/tools/Makefile.sources
@@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \
         intel_dp_compliance.h \
         intel_dp_compliance_hotplug.c \
         $(NULL)
-
+dpcd_reg_SOURCES = \
+	dpcd_reg.c \
+	$(NULL)
diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
new file mode 100644
index 000000000000..2848277aa792
--- /dev/null
+++ b/tools/dpcd_reg.c
@@ -0,0 +1,219 @@
+/*
+ * Copyright © 2018 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ * 		DPCD register read/decode tool
+ * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read,
+ * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be set.
+ */
+
+#include "igt_core.h"
+#include <errno.h>
+#include <fcntl.h>
+
+#define INVALID	0xff
+#define RW_SIZE	1
+
+const char aux_dev[] = "/dev/drm_dp_aux";
+
+static void print_usage(char *tool, int help)
+{
+	igt_info("DPCD register rw and decode tool\n\n");
+	igt_info("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
+		 "to be set in the kernel config.\n");
+	igt_info("Usage %s command [options...]\n", tool);
+	igt_info("Supported commands are:\n"
+		 "\tread: Read a dpcd reg at an offset\n"
+		 "\twrite: Write a dpcd reg at an offset\n"
+		 "\tdecode: Decode the value of a dpcd reg\n");
+	igt_info("Options for the above commands are\n"
+		 "\t--device: Aux device id, IS_REQUIRED\n"
+		 "\t--help: print the usage\n"
+		 "\t--offset: DPCD register offset in hex, IS_REQUIRED\n"
+		 "\t--spec: Specify DP/eDP spec version for decoding, IS_OPTIONAL\n"
+		 "\t--val: Specify a value, for reg writes\n");
+	exit((help == 1)? EXIT_SUCCESS : EXIT_FAILURE);
+}
+
+static int dpcd_read(char *device, const uint32_t *offset)
+{
+	int fd, ret;
+	void *buf = malloc(sizeof(uint8_t));
+
+	if(NULL != buf)
+		memset(buf, 0, sizeof(uint8_t));
+	else {
+		igt_warn("Can't allocate read buffer\n");
+		ret = EXIT_FAILURE;
+		goto out;
+	}
+
+	fd = open(device, O_RDONLY);
+	if (fd > 0) {
+		ret = pread(fd, buf, RW_SIZE, *offset);
+		close(fd);
+		if (ret < 0) {
+			igt_warn("Failed to read from %s aux device - error %s\n", device, strerror(errno));
+			ret = EXIT_FAILURE;
+			goto free_up;
+		}
+		printf("Value @ offset %x is %x\n", *offset, *((int *)(buf)));
+	}
+	else {
+		igt_warn("Failed to open %s aux device - error: %s\n", device, strerror(errno));
+		ret = EXIT_FAILURE;
+	}
+free_up:
+	free(buf);
+out:
+	return ret;
+}
+
+static int dpcd_write(char *device, const uint32_t *offset, const uint8_t *val)
+{
+	int fd, ret;
+
+	fd = open(device, O_RDWR);
+	if (fd > 0) {
+		ret = pwrite(fd, (const void *)val, RW_SIZE, *offset);
+		close(fd);
+		if (ret < 0) {
+			igt_warn("Failed to write to %s aux device - error %s\n", device, strerror(errno));
+			ret = EXIT_FAILURE;
+			goto out;
+		}
+		ret = dpcd_read(device, offset);
+	}
+	else {
+		igt_warn("Failed to open %s aux device - error: %s\n", device, strerror(errno));
+		ret = EXIT_FAILURE;
+	}
+out:
+	return ret;
+}
+
+/*TO-DO: Decode dpcd reg value.
+ * By default, the latest DP/eDP spec
+ * versions will be used. To override,
+ * specify the spec version through the
+ * --spec option.
+static void dpcd_decode()
+{
+}
+*/
+
+int main(int argc, char **argv)
+{
+	char dev_name[20];
+	int ret, devid, help_flg;
+	uint32_t offset;
+	uint8_t val;
+
+	enum command {
+		INV = -1,
+		READ = 2,
+		WRITE,
+		DECODE,
+	} cmd = INV;
+
+	struct option longopts [] = {
+		{ "device",	required_argument,	NULL,      'd' },
+		{ "help", 	no_argument, 		&help_flg,  2  },
+		{ "offset",	required_argument,	NULL,	   'o' },
+		{ "spec",	optional_argument,	NULL,	   's' },
+		{ "val",	required_argument,	NULL,	   'v' },
+		{ 0 }
+	};
+
+	offset = val = INVALID;
+	devid = -1;
+
+	while ((ret = getopt_long(argc, argv, "-:d:o:s::v:", longopts, NULL)) != -1) {
+		switch (ret) {
+		case 'd':
+			devid = strtoul(optarg, NULL, 10);
+			break;
+		case 'o':
+			offset = strtoul(optarg, NULL, 16);
+			break;
+		case 's':
+			/* TO-DO: Parse --spec version */
+			break;
+		case 'v':
+			val = strtoul(optarg, NULL, 16);
+			break;
+		/* Fall through for --help */
+		case 0:
+			break;
+		/* Command parsing */
+		case 1:
+			if (strcmp(optarg, "read") == 0)
+				cmd = READ;
+			else if (strcmp(optarg, "write") == 0)
+				cmd = WRITE;
+			else if (strcmp(optarg, "decode") == 0)
+				cmd = DECODE;
+			break;
+		case ':':
+			fprintf(stderr, "The -%c option of %s requires an argument\n", optopt, argv[0]);
+			print_usage(argv[0], 0);
+		case '?':
+		default :
+			printf("%s - option %c is invalid\n", argv[0], optopt);
+			print_usage(argv[0], 0);
+		}
+	}
+
+	if (help_flg == 2)
+		print_usage(argv[0], 1);
+
+	if(devid == -1 || offset == INVALID) {
+		printf("Aux device id and/or offset missing\n");
+		print_usage(argv[0], 0);
+	}
+
+	memset(dev_name, '\0', 20);
+	snprintf(dev_name, sizeof(aux_dev) + 2, "%s%d", aux_dev, devid);
+
+	switch (cmd) {
+		case READ:
+			ret = dpcd_read(dev_name, &offset);
+			break;
+		case WRITE:
+			if (val == INVALID) {
+				igt_warn("Write value is missing\n");
+				print_usage(argv[0], 0);
+			}
+			ret = dpcd_write(dev_name, &offset, &val);
+			break;
+		case DECODE:
+			/* TO-DO: Decode dpcd reg vals
+			 * drm_dpcd_decode();
+			*/
+			break;
+		case INV:
+		default:
+			igt_warn("Please specify a comand: read/write/decode. See usage\n");
+			print_usage(argv[0], 0);
+	}
+
+	return ret;
+}
-- 
2.13.5

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

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

* [igt-dev] ✓ Fi.CI.BAT: success for tools: Add a simple tool to read/write/decode dpcd registers
  2018-06-14  7:11 [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas
@ 2018-06-14  8:08 ` Patchwork
  2018-06-14  9:27 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
  2018-06-14 10:43 ` [igt-dev] [PATCH i-g-t] " Ville Syrjälä
  2 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-06-14  8:08 UTC (permalink / raw)
  To: Tarun Vyas; +Cc: igt-dev

== Series Details ==

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

== Summary ==

= CI Bug Log - changes from CI_DRM_4309 -> IGTPW_1459 =

== Summary - WARNING ==

  Minor unknown changes coming with IGTPW_1459 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1459, 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/1/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_gttfill@basic:
      fi-byt-n2820:       PASS -> FAIL (fdo#106744)

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-skl-6700hq:      PASS -> DMESG-WARN (fdo#105998)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       DMESG-WARN (fdo#105128) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-cnl-psr:         DMESG-WARN (fdo#104951) -> PASS

    
  fdo#104951 https://bugs.freedesktop.org/show_bug.cgi?id=104951
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
  fdo#106744 https://bugs.freedesktop.org/show_bug.cgi?id=106744


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

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-glk-j4005 


== Build changes ==

    * IGT: IGT_4517 -> IGTPW_1459

  CI_DRM_4309: 2740c5b0d0f40092355b329a62ede8cced7f64b9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1459: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1459/
  IGT_4517: e94ce40798e35d2e3c4494f50b617908066bbf8b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

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

* [igt-dev] ✓ Fi.CI.IGT: success for tools: Add a simple tool to read/write/decode dpcd registers
  2018-06-14  7:11 [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas
  2018-06-14  8:08 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-06-14  9:27 ` Patchwork
  2018-06-14 10:43 ` [igt-dev] [PATCH i-g-t] " Ville Syrjälä
  2 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-06-14  9:27 UTC (permalink / raw)
  To: Tarun Vyas; +Cc: igt-dev

== Series Details ==

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

== Summary ==

= CI Bug Log - changes from IGT_4517_full -> IGTPW_1459_full =

== Summary - WARNING ==

  Minor unknown changes coming with IGTPW_1459_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1459_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/1/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@drv_suspend@shrink:
      shard-glk:          INCOMPLETE (fdo#103359, k.org#198133) -> FAIL

    igt@gem_exec_schedule@deep-bsd2:
      shard-kbl:          SKIP -> PASS

    igt@gem_exec_schedule@deep-vebox:
      shard-kbl:          PASS -> SKIP +1

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_hangman@error-state-capture-blt:
      shard-snb:          PASS -> INCOMPLETE (fdo#105411)

    igt@drv_selftest@live_hangcheck:
      shard-kbl:          PASS -> DMESG-FAIL (fdo#106560)

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

    igt@kms_atomic_transition@1x-modeset-transitions-nonblocking:
      shard-glk:          NOTRUN -> FAIL (fdo#105703)

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

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          PASS -> FAIL (fdo#105363)

    igt@kms_flip@flip-vs-expired-vblank:
      shard-hsw:          PASS -> FAIL (fdo#105189)

    igt@kms_flip_tiling@flip-to-y-tiled:
      shard-glk:          NOTRUN -> FAIL (fdo#104724, fdo#103822) +1

    igt@kms_flip_tiling@flip-y-tiled:
      shard-glk:          NOTRUN -> FAIL (fdo#104724)

    igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-pwrite:
      shard-glk:          PASS -> FAIL (fdo#104724, fdo#103167)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-render:
      shard-snb:          PASS -> FAIL (fdo#104724, fdo#103167)

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

    igt@prime_vgem@coherency-gtt:
      shard-glk:          NOTRUN -> FAIL (fdo#100587)

    igt@testdisplay:
      shard-glk:          NOTRUN -> INCOMPLETE (fdo#103359, k.org#198133)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_gtt:
      shard-kbl:          FAIL (fdo#105347) -> PASS

    igt@drv_suspend@shrink:
      shard-apl:          INCOMPLETE (fdo#103927) -> PASS

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

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

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

    igt@kms_flip@flip-vs-panning-vs-hang:
      shard-snb:          DMESG-WARN (fdo#103821) -> PASS

    igt@kms_flip@plain-flip-ts-check-interruptible:
      shard-hsw:          FAIL (fdo#100368) -> PASS

    igt@kms_rotation_crc@primary-rotation-180:
      shard-snb:          FAIL (fdo#104724, fdo#103925) -> PASS

    igt@perf_pmu@busy-accuracy-50-vcs1:
      shard-snb:          INCOMPLETE (fdo#105411) -> SKIP

    
    ==== Warnings ====

    igt@kms_sysfs_edid_timing:
      shard-hsw:          WARN (fdo#100047) -> FAIL (fdo#100047)

    
  fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#100587 https://bugs.freedesktop.org/show_bug.cgi?id=100587
  fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670
  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  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#103821 https://bugs.freedesktop.org/show_bug.cgi?id=103821
  fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#104873 https://bugs.freedesktop.org/show_bug.cgi?id=104873
  fdo#105189 https://bugs.freedesktop.org/show_bug.cgi?id=105189
  fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105703 https://bugs.freedesktop.org/show_bug.cgi?id=105703
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  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_4517 -> IGTPW_1459
    * Linux: CI_DRM_4307 -> CI_DRM_4309

  CI_DRM_4307: 0cf6f8f74cad691364d738d1607bc45945f3a5f9 @ git://anongit.freedesktop.org/gfx-ci/linux
  CI_DRM_4309: 2740c5b0d0f40092355b329a62ede8cced7f64b9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1459: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1459/
  IGT_4517: e94ce40798e35d2e3c4494f50b617908066bbf8b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

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

* Re: [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers
  2018-06-14  7:11 [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas
  2018-06-14  8:08 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
  2018-06-14  9:27 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
@ 2018-06-14 10:43 ` Ville Syrjälä
  2018-06-14 12:50   ` Jani Nikula
  2018-06-15  2:00   ` Tarun Vyas
  2 siblings, 2 replies; 10+ messages in thread
From: Ville Syrjälä @ 2018-06-14 10:43 UTC (permalink / raw)
  To: Tarun Vyas; +Cc: igt-dev, dhinakaran.pandiyan

On Thu, Jun 14, 2018 at 12:11:41AM -0700, Tarun Vyas wrote:
> This tool serves as a wrapper around the constructs provided by the
> drm_dpcd_aux_dev kernel module by working on the /dev/drm_dp_aux[n]
> devices created by the kernel module.
> It supports reading and writing dpcd registers on the connected aux
> channels.
> In the follow-up patch, support for decoding these registers will be
> added to facilate debugging panel related issues.
> 
> Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> ---
>  tools/Makefile.am      |   2 +-
>  tools/Makefile.sources |   4 +-
>  tools/dpcd_reg.c       | 219 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 223 insertions(+), 2 deletions(-)
>  create mode 100644 tools/dpcd_reg.c
> 
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index 09b6dbcc3ece..b78824dadfdb 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> @@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN)
>  intel_error_decode_LDFLAGS = -lz
>  endif
>  
> -bin_PROGRAMS += intel_dp_compliance
> +bin_PROGRAMS += intel_dp_compliance dpcd_reg

Not the best place for this I think.

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

>  intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS)
>  intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la
>  
> diff --git a/tools/Makefile.sources b/tools/Makefile.sources
> index abd23a0f4628..db606b28560f 100644
> --- a/tools/Makefile.sources
> +++ b/tools/Makefile.sources
> @@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \
>          intel_dp_compliance.h \
>          intel_dp_compliance_hotplug.c \
>          $(NULL)
> -
> +dpcd_reg_SOURCES = \
> +	dpcd_reg.c \
> +	$(NULL)
> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> new file mode 100644
> index 000000000000..2848277aa792
> --- /dev/null
> +++ b/tools/dpcd_reg.c
> @@ -0,0 +1,219 @@
> +/*
> + * Copyright © 2018 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + *
> + * 		DPCD register read/decode tool
> + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read,
> + * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be set.
> + */
> +
> +#include "igt_core.h"
> +#include <errno.h>
> +#include <fcntl.h>
> +
> +#define INVALID	0xff
> +#define RW_SIZE	1
> +
> +const char aux_dev[] = "/dev/drm_dp_aux";
> +
> +static void print_usage(char *tool, int help)
> +{
> +	igt_info("DPCD register rw and decode tool\n\n");
> +	igt_info("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
> +		 "to be set in the kernel config.\n");
> +	igt_info("Usage %s command [options...]\n", tool);
> +	igt_info("Supported commands are:\n"
> +		 "\tread: Read a dpcd reg at an offset\n"
> +		 "\twrite: Write a dpcd reg at an offset\n"
> +		 "\tdecode: Decode the value of a dpcd reg\n");

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

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

I think you should model the UI based on intel_reg.

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

Why is offset a pointer?

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

Pointless malloc.

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

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

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

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

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

* Re: [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers
  2018-06-14 10:43 ` [igt-dev] [PATCH i-g-t] " Ville Syrjälä
@ 2018-06-14 12:50   ` Jani Nikula
  2018-06-14 12:55     ` Ville Syrjälä
  2018-06-15  2:00   ` Tarun Vyas
  1 sibling, 1 reply; 10+ messages in thread
From: Jani Nikula @ 2018-06-14 12:50 UTC (permalink / raw)
  To: Ville Syrjälä, Tarun Vyas; +Cc: igt-dev, dhinakaran.pandiyan

On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Jun 14, 2018 at 12:11:41AM -0700, Tarun Vyas wrote:
>> This tool serves as a wrapper around the constructs provided by the
>> drm_dpcd_aux_dev kernel module by working on the /dev/drm_dp_aux[n]
>> devices created by the kernel module.
>> It supports reading and writing dpcd registers on the connected aux
>> channels.
>> In the follow-up patch, support for decoding these registers will be
>> added to facilate debugging panel related issues.
>> 
>> Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
>> ---
>>  tools/Makefile.am      |   2 +-
>>  tools/Makefile.sources |   4 +-
>>  tools/dpcd_reg.c       | 219 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 223 insertions(+), 2 deletions(-)
>>  create mode 100644 tools/dpcd_reg.c
>> 
>> diff --git a/tools/Makefile.am b/tools/Makefile.am
>> index 09b6dbcc3ece..b78824dadfdb 100644
>> --- a/tools/Makefile.am
>> +++ b/tools/Makefile.am
>> @@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN)
>>  intel_error_decode_LDFLAGS = -lz
>>  endif
>>  
>> -bin_PROGRAMS += intel_dp_compliance
>> +bin_PROGRAMS += intel_dp_compliance dpcd_reg
>
> Not the best place for this I think.
>
> Missing meson.build so no one is actually going to build this.
>
>>  intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS)
>>  intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la
>>  
>> diff --git a/tools/Makefile.sources b/tools/Makefile.sources
>> index abd23a0f4628..db606b28560f 100644
>> --- a/tools/Makefile.sources
>> +++ b/tools/Makefile.sources
>> @@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \
>>          intel_dp_compliance.h \
>>          intel_dp_compliance_hotplug.c \
>>          $(NULL)
>> -
>> +dpcd_reg_SOURCES = \
>> +	dpcd_reg.c \
>> +	$(NULL)
>> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
>> new file mode 100644
>> index 000000000000..2848277aa792
>> --- /dev/null
>> +++ b/tools/dpcd_reg.c
>> @@ -0,0 +1,219 @@
>> +/*
>> + * Copyright © 2018 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
>> + * SOFTWARE.
>> + *
>> + * 		DPCD register read/decode tool
>> + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read,
>> + * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be set.
>> + */
>> +
>> +#include "igt_core.h"
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +
>> +#define INVALID	0xff
>> +#define RW_SIZE	1
>> +
>> +const char aux_dev[] = "/dev/drm_dp_aux";
>> +
>> +static void print_usage(char *tool, int help)
>> +{
>> +	igt_info("DPCD register rw and decode tool\n\n");
>> +	igt_info("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
>> +		 "to be set in the kernel config.\n");
>> +	igt_info("Usage %s command [options...]\n", tool);
>> +	igt_info("Supported commands are:\n"
>> +		 "\tread: Read a dpcd reg at an offset\n"
>> +		 "\twrite: Write a dpcd reg at an offset\n"
>> +		 "\tdecode: Decode the value of a dpcd reg\n");
>
> You didn't implement decode so no point in mentioning it.
>
>> +	igt_info("Options for the above commands are\n"
>> +		 "\t--device: Aux device id, IS_REQUIRED\n"
>> +		 "\t--help: print the usage\n"
>> +		 "\t--offset: DPCD register offset in hex, IS_REQUIRED\n"
>> +		 "\t--spec: Specify DP/eDP spec version for decoding, IS_OPTIONAL\n"
>> +		 "\t--val: Specify a value, for reg writes\n");
>
> I think you should model the UI based on intel_reg.

I originally had plans to plug this *into* intel_reg, with e.g. dpcd:
prefix. There we have the framework ready for adding names to the
registers using separate definition files, and we have all the
frameworks ready for doing decoded dumps of the dpcd space. Just saying.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* Re: [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers
  2018-06-14 12:50   ` Jani Nikula
@ 2018-06-14 12:55     ` Ville Syrjälä
  2018-06-14 13:20       ` Jani Nikula
  0 siblings, 1 reply; 10+ messages in thread
From: Ville Syrjälä @ 2018-06-14 12:55 UTC (permalink / raw)
  To: Jani Nikula; +Cc: igt-dev, dhinakaran.pandiyan

On Thu, Jun 14, 2018 at 03:50:30PM +0300, Jani Nikula wrote:
> On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Jun 14, 2018 at 12:11:41AM -0700, Tarun Vyas wrote:
> >> This tool serves as a wrapper around the constructs provided by the
> >> drm_dpcd_aux_dev kernel module by working on the /dev/drm_dp_aux[n]
> >> devices created by the kernel module.
> >> It supports reading and writing dpcd registers on the connected aux
> >> channels.
> >> In the follow-up patch, support for decoding these registers will be
> >> added to facilate debugging panel related issues.
> >> 
> >> Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> >> ---
> >>  tools/Makefile.am      |   2 +-
> >>  tools/Makefile.sources |   4 +-
> >>  tools/dpcd_reg.c       | 219 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 223 insertions(+), 2 deletions(-)
> >>  create mode 100644 tools/dpcd_reg.c
> >> 
> >> diff --git a/tools/Makefile.am b/tools/Makefile.am
> >> index 09b6dbcc3ece..b78824dadfdb 100644
> >> --- a/tools/Makefile.am
> >> +++ b/tools/Makefile.am
> >> @@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN)
> >>  intel_error_decode_LDFLAGS = -lz
> >>  endif
> >>  
> >> -bin_PROGRAMS += intel_dp_compliance
> >> +bin_PROGRAMS += intel_dp_compliance dpcd_reg
> >
> > Not the best place for this I think.
> >
> > Missing meson.build so no one is actually going to build this.
> >
> >>  intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS)
> >>  intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la
> >>  
> >> diff --git a/tools/Makefile.sources b/tools/Makefile.sources
> >> index abd23a0f4628..db606b28560f 100644
> >> --- a/tools/Makefile.sources
> >> +++ b/tools/Makefile.sources
> >> @@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \
> >>          intel_dp_compliance.h \
> >>          intel_dp_compliance_hotplug.c \
> >>          $(NULL)
> >> -
> >> +dpcd_reg_SOURCES = \
> >> +	dpcd_reg.c \
> >> +	$(NULL)
> >> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> >> new file mode 100644
> >> index 000000000000..2848277aa792
> >> --- /dev/null
> >> +++ b/tools/dpcd_reg.c
> >> @@ -0,0 +1,219 @@
> >> +/*
> >> + * Copyright © 2018 Intel Corporation
> >> + *
> >> + * Permission is hereby granted, free of charge, to any person obtaining a
> >> + * copy of this software and associated documentation files (the "Software"),
> >> + * to deal in the Software without restriction, including without limitation
> >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> >> + * and/or sell copies of the Software, and to permit persons to whom the
> >> + * Software is furnished to do so, subject to the following conditions:
> >> + *
> >> + * The above copyright notice and this permission notice (including the next
> >> + * paragraph) shall be included in all copies or substantial portions of the
> >> + * Software.
> >> + *
> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> >> + * SOFTWARE.
> >> + *
> >> + * 		DPCD register read/decode tool
> >> + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read,
> >> + * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be set.
> >> + */
> >> +
> >> +#include "igt_core.h"
> >> +#include <errno.h>
> >> +#include <fcntl.h>
> >> +
> >> +#define INVALID	0xff
> >> +#define RW_SIZE	1
> >> +
> >> +const char aux_dev[] = "/dev/drm_dp_aux";
> >> +
> >> +static void print_usage(char *tool, int help)
> >> +{
> >> +	igt_info("DPCD register rw and decode tool\n\n");
> >> +	igt_info("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
> >> +		 "to be set in the kernel config.\n");
> >> +	igt_info("Usage %s command [options...]\n", tool);
> >> +	igt_info("Supported commands are:\n"
> >> +		 "\tread: Read a dpcd reg at an offset\n"
> >> +		 "\twrite: Write a dpcd reg at an offset\n"
> >> +		 "\tdecode: Decode the value of a dpcd reg\n");
> >
> > You didn't implement decode so no point in mentioning it.
> >
> >> +	igt_info("Options for the above commands are\n"
> >> +		 "\t--device: Aux device id, IS_REQUIRED\n"
> >> +		 "\t--help: print the usage\n"
> >> +		 "\t--offset: DPCD register offset in hex, IS_REQUIRED\n"
> >> +		 "\t--spec: Specify DP/eDP spec version for decoding, IS_OPTIONAL\n"
> >> +		 "\t--val: Specify a value, for reg writes\n");
> >
> > I think you should model the UI based on intel_reg.
> 
> I originally had plans to plug this *into* intel_reg, with e.g. dpcd:
> prefix. There we have the framework ready for adding names to the
> registers using separate definition files, and we have all the
> frameworks ready for doing decoded dumps of the dpcd space. Just saying.

That's one option. But maybe people working on non-Intel hw would want
to use this as well?

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

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

* Re: [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers
  2018-06-14 12:55     ` Ville Syrjälä
@ 2018-06-14 13:20       ` Jani Nikula
  2018-06-14 14:00         ` Ville Syrjälä
  0 siblings, 1 reply; 10+ messages in thread
From: Jani Nikula @ 2018-06-14 13:20 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: igt-dev, dhinakaran.pandiyan

On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Jun 14, 2018 at 03:50:30PM +0300, Jani Nikula wrote:
>> On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > On Thu, Jun 14, 2018 at 12:11:41AM -0700, Tarun Vyas wrote:
>> >> This tool serves as a wrapper around the constructs provided by the
>> >> drm_dpcd_aux_dev kernel module by working on the /dev/drm_dp_aux[n]
>> >> devices created by the kernel module.
>> >> It supports reading and writing dpcd registers on the connected aux
>> >> channels.
>> >> In the follow-up patch, support for decoding these registers will be
>> >> added to facilate debugging panel related issues.
>> >> 
>> >> Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> >> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
>> >> ---
>> >>  tools/Makefile.am      |   2 +-
>> >>  tools/Makefile.sources |   4 +-
>> >>  tools/dpcd_reg.c       | 219 +++++++++++++++++++++++++++++++++++++++++++++++++
>> >>  3 files changed, 223 insertions(+), 2 deletions(-)
>> >>  create mode 100644 tools/dpcd_reg.c
>> >> 
>> >> diff --git a/tools/Makefile.am b/tools/Makefile.am
>> >> index 09b6dbcc3ece..b78824dadfdb 100644
>> >> --- a/tools/Makefile.am
>> >> +++ b/tools/Makefile.am
>> >> @@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN)
>> >>  intel_error_decode_LDFLAGS = -lz
>> >>  endif
>> >>  
>> >> -bin_PROGRAMS += intel_dp_compliance
>> >> +bin_PROGRAMS += intel_dp_compliance dpcd_reg
>> >
>> > Not the best place for this I think.
>> >
>> > Missing meson.build so no one is actually going to build this.
>> >
>> >>  intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS)
>> >>  intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la
>> >>  
>> >> diff --git a/tools/Makefile.sources b/tools/Makefile.sources
>> >> index abd23a0f4628..db606b28560f 100644
>> >> --- a/tools/Makefile.sources
>> >> +++ b/tools/Makefile.sources
>> >> @@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \
>> >>          intel_dp_compliance.h \
>> >>          intel_dp_compliance_hotplug.c \
>> >>          $(NULL)
>> >> -
>> >> +dpcd_reg_SOURCES = \
>> >> +	dpcd_reg.c \
>> >> +	$(NULL)
>> >> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
>> >> new file mode 100644
>> >> index 000000000000..2848277aa792
>> >> --- /dev/null
>> >> +++ b/tools/dpcd_reg.c
>> >> @@ -0,0 +1,219 @@
>> >> +/*
>> >> + * Copyright © 2018 Intel Corporation
>> >> + *
>> >> + * Permission is hereby granted, free of charge, to any person obtaining a
>> >> + * copy of this software and associated documentation files (the "Software"),
>> >> + * to deal in the Software without restriction, including without limitation
>> >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> >> + * and/or sell copies of the Software, and to permit persons to whom the
>> >> + * Software is furnished to do so, subject to the following conditions:
>> >> + *
>> >> + * The above copyright notice and this permission notice (including the next
>> >> + * paragraph) shall be included in all copies or substantial portions of the
>> >> + * Software.
>> >> + *
>> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
>> >> + * SOFTWARE.
>> >> + *
>> >> + * 		DPCD register read/decode tool
>> >> + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read,
>> >> + * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be set.
>> >> + */
>> >> +
>> >> +#include "igt_core.h"
>> >> +#include <errno.h>
>> >> +#include <fcntl.h>
>> >> +
>> >> +#define INVALID	0xff
>> >> +#define RW_SIZE	1
>> >> +
>> >> +const char aux_dev[] = "/dev/drm_dp_aux";
>> >> +
>> >> +static void print_usage(char *tool, int help)
>> >> +{
>> >> +	igt_info("DPCD register rw and decode tool\n\n");
>> >> +	igt_info("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
>> >> +		 "to be set in the kernel config.\n");
>> >> +	igt_info("Usage %s command [options...]\n", tool);
>> >> +	igt_info("Supported commands are:\n"
>> >> +		 "\tread: Read a dpcd reg at an offset\n"
>> >> +		 "\twrite: Write a dpcd reg at an offset\n"
>> >> +		 "\tdecode: Decode the value of a dpcd reg\n");
>> >
>> > You didn't implement decode so no point in mentioning it.
>> >
>> >> +	igt_info("Options for the above commands are\n"
>> >> +		 "\t--device: Aux device id, IS_REQUIRED\n"
>> >> +		 "\t--help: print the usage\n"
>> >> +		 "\t--offset: DPCD register offset in hex, IS_REQUIRED\n"
>> >> +		 "\t--spec: Specify DP/eDP spec version for decoding, IS_OPTIONAL\n"
>> >> +		 "\t--val: Specify a value, for reg writes\n");
>> >
>> > I think you should model the UI based on intel_reg.
>> 
>> I originally had plans to plug this *into* intel_reg, with e.g. dpcd:
>> prefix. There we have the framework ready for adding names to the
>> registers using separate definition files, and we have all the
>> frameworks ready for doing decoded dumps of the dpcd space. Just saying.
>
> That's one option. But maybe people working on non-Intel hw would want
> to use this as well?

I know, that's a consideration.

It's just that it's totally obvious any DPCD tool will grow a large
bunch of code already written in intel_reg.c. Looking at the proposed
patch, there's only support for reading one offset at a time now. The
first thing anyone will want is ability to read N bytes. Or a bunch of
offsets here and there. With intel_reg, you get all of that for free.

Remember the times when we had separate and slightly different reg tools
for all of the different "register ports" in intel_reg? I don't
particularly miss that.

Note also that for i915 DP debugging, with DPCD support in intel_reg,
you could have the DPCD reads interspaced with register reads. How cool
is that?

So I'm not going to insist on using intel_reg. I understand the counter
argument. But when the two tools have grown a bunch of overlapping and
duplicated code with slightly conflicting user interfaces, I reserve the
right to be all, "I totally told you so."

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* Re: [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers
  2018-06-14 13:20       ` Jani Nikula
@ 2018-06-14 14:00         ` Ville Syrjälä
  2018-06-14 22:32           ` Dhinakaran Pandiyan
  0 siblings, 1 reply; 10+ messages in thread
From: Ville Syrjälä @ 2018-06-14 14:00 UTC (permalink / raw)
  To: Jani Nikula; +Cc: igt-dev, dhinakaran.pandiyan

On Thu, Jun 14, 2018 at 04:20:07PM +0300, Jani Nikula wrote:
> On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Jun 14, 2018 at 03:50:30PM +0300, Jani Nikula wrote:
> >> On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> > On Thu, Jun 14, 2018 at 12:11:41AM -0700, Tarun Vyas wrote:
> >> >> This tool serves as a wrapper around the constructs provided by the
> >> >> drm_dpcd_aux_dev kernel module by working on the /dev/drm_dp_aux[n]
> >> >> devices created by the kernel module.
> >> >> It supports reading and writing dpcd registers on the connected aux
> >> >> channels.
> >> >> In the follow-up patch, support for decoding these registers will be
> >> >> added to facilate debugging panel related issues.
> >> >> 
> >> >> Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >> >> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> >> >> ---
> >> >>  tools/Makefile.am      |   2 +-
> >> >>  tools/Makefile.sources |   4 +-
> >> >>  tools/dpcd_reg.c       | 219 +++++++++++++++++++++++++++++++++++++++++++++++++
> >> >>  3 files changed, 223 insertions(+), 2 deletions(-)
> >> >>  create mode 100644 tools/dpcd_reg.c
> >> >> 
> >> >> diff --git a/tools/Makefile.am b/tools/Makefile.am
> >> >> index 09b6dbcc3ece..b78824dadfdb 100644
> >> >> --- a/tools/Makefile.am
> >> >> +++ b/tools/Makefile.am
> >> >> @@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN)
> >> >>  intel_error_decode_LDFLAGS = -lz
> >> >>  endif
> >> >>  
> >> >> -bin_PROGRAMS += intel_dp_compliance
> >> >> +bin_PROGRAMS += intel_dp_compliance dpcd_reg
> >> >
> >> > Not the best place for this I think.
> >> >
> >> > Missing meson.build so no one is actually going to build this.
> >> >
> >> >>  intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS)
> >> >>  intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la
> >> >>  
> >> >> diff --git a/tools/Makefile.sources b/tools/Makefile.sources
> >> >> index abd23a0f4628..db606b28560f 100644
> >> >> --- a/tools/Makefile.sources
> >> >> +++ b/tools/Makefile.sources
> >> >> @@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \
> >> >>          intel_dp_compliance.h \
> >> >>          intel_dp_compliance_hotplug.c \
> >> >>          $(NULL)
> >> >> -
> >> >> +dpcd_reg_SOURCES = \
> >> >> +	dpcd_reg.c \
> >> >> +	$(NULL)
> >> >> diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> >> >> new file mode 100644
> >> >> index 000000000000..2848277aa792
> >> >> --- /dev/null
> >> >> +++ b/tools/dpcd_reg.c
> >> >> @@ -0,0 +1,219 @@
> >> >> +/*
> >> >> + * Copyright © 2018 Intel Corporation
> >> >> + *
> >> >> + * Permission is hereby granted, free of charge, to any person obtaining a
> >> >> + * copy of this software and associated documentation files (the "Software"),
> >> >> + * to deal in the Software without restriction, including without limitation
> >> >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> >> >> + * and/or sell copies of the Software, and to permit persons to whom the
> >> >> + * Software is furnished to do so, subject to the following conditions:
> >> >> + *
> >> >> + * The above copyright notice and this permission notice (including the next
> >> >> + * paragraph) shall be included in all copies or substantial portions of the
> >> >> + * Software.
> >> >> + *
> >> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> >> >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> >> >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> >> >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> >> >> + * SOFTWARE.
> >> >> + *
> >> >> + * 		DPCD register read/decode tool
> >> >> + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read,
> >> >> + * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be set.
> >> >> + */
> >> >> +
> >> >> +#include "igt_core.h"
> >> >> +#include <errno.h>
> >> >> +#include <fcntl.h>
> >> >> +
> >> >> +#define INVALID	0xff
> >> >> +#define RW_SIZE	1
> >> >> +
> >> >> +const char aux_dev[] = "/dev/drm_dp_aux";
> >> >> +
> >> >> +static void print_usage(char *tool, int help)
> >> >> +{
> >> >> +	igt_info("DPCD register rw and decode tool\n\n");
> >> >> +	igt_info("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
> >> >> +		 "to be set in the kernel config.\n");
> >> >> +	igt_info("Usage %s command [options...]\n", tool);
> >> >> +	igt_info("Supported commands are:\n"
> >> >> +		 "\tread: Read a dpcd reg at an offset\n"
> >> >> +		 "\twrite: Write a dpcd reg at an offset\n"
> >> >> +		 "\tdecode: Decode the value of a dpcd reg\n");
> >> >
> >> > You didn't implement decode so no point in mentioning it.
> >> >
> >> >> +	igt_info("Options for the above commands are\n"
> >> >> +		 "\t--device: Aux device id, IS_REQUIRED\n"
> >> >> +		 "\t--help: print the usage\n"
> >> >> +		 "\t--offset: DPCD register offset in hex, IS_REQUIRED\n"
> >> >> +		 "\t--spec: Specify DP/eDP spec version for decoding, IS_OPTIONAL\n"
> >> >> +		 "\t--val: Specify a value, for reg writes\n");
> >> >
> >> > I think you should model the UI based on intel_reg.
> >> 
> >> I originally had plans to plug this *into* intel_reg, with e.g. dpcd:
> >> prefix. There we have the framework ready for adding names to the
> >> registers using separate definition files, and we have all the
> >> frameworks ready for doing decoded dumps of the dpcd space. Just saying.
> >
> > That's one option. But maybe people working on non-Intel hw would want
> > to use this as well?
> 
> I know, that's a consideration.
> 
> It's just that it's totally obvious any DPCD tool will grow a large
> bunch of code already written in intel_reg.c. Looking at the proposed
> patch, there's only support for reading one offset at a time now. The
> first thing anyone will want is ability to read N bytes. Or a bunch of
> offsets here and there. With intel_reg, you get all of that for free.

Indeed. Actually that reminds me that it would be awesome to have a way
to specify offset:count pairs for intel_reg read. Currently one either
has to dump a bunch of irrelevant/non-existing registers, or execute
intel_reg multiple times.

> Remember the times when we had separate and slightly different reg tools
> for all of the different "register ports" in intel_reg? I don't
> particularly miss that.
> 
> Note also that for i915 DP debugging, with DPCD support in intel_reg,
> you could have the DPCD reads interspaced with register reads. How cool
> is that?

Yes, that could be interesting.

> 
> So I'm not going to insist on using intel_reg. I understand the counter
> argument. But when the two tools have grown a bunch of overlapping and
> duplicated code with slightly conflicting user interfaces, I reserve the
> right to be all, "I totally told you so."

Maybe we should rename intel_reg to igt_reg and make sure it can
actually be used without an Intel GPU? Other drivers could then
perhaps also plug in their own backends to the same tool? Not sure
how feasible that would be or whether anyone would even be interested
in doing so.

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

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

* Re: [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers
  2018-06-14 14:00         ` Ville Syrjälä
@ 2018-06-14 22:32           ` Dhinakaran Pandiyan
  0 siblings, 0 replies; 10+ messages in thread
From: Dhinakaran Pandiyan @ 2018-06-14 22:32 UTC (permalink / raw)
  To: Ville Syrjälä, Jani Nikula; +Cc: igt-dev

On Thu, 2018-06-14 at 17:00 +0300, Ville Syrjälä wrote:
> On Thu, Jun 14, 2018 at 04:20:07PM +0300, Jani Nikula wrote:
> > 
> > On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com>
> > wrote:
> > > 
> > > On Thu, Jun 14, 2018 at 03:50:30PM +0300, Jani Nikula wrote:
> > > > 
> > > > On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.c
> > > > om> wrote:
> > > > > 
> > > > > On Thu, Jun 14, 2018 at 12:11:41AM -0700, Tarun Vyas wrote:
> > > > > > 
> > > > > > This tool serves as a wrapper around the constructs
> > > > > > provided by the
> > > > > > drm_dpcd_aux_dev kernel module by working on the
> > > > > > /dev/drm_dp_aux[n]
> > > > > > devices created by the kernel module.
> > > > > > It supports reading and writing dpcd registers on the
> > > > > > connected aux
> > > > > > channels.
> > > > > > In the follow-up patch, support for decoding these
> > > > > > registers will be
> > > > > > added to facilate debugging panel related issues.
> > > > > > 
> > > > > > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@inte
> > > > > > l.com>
> > > > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > > > > > ---
> > > > > >  tools/Makefile.am      |   2 +-
> > > > > >  tools/Makefile.sources |   4 +-
> > > > > >  tools/dpcd_reg.c       | 219
> > > > > > +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  3 files changed, 223 insertions(+), 2 deletions(-)
> > > > > >  create mode 100644 tools/dpcd_reg.c
> > > > > > 
> > > > > > diff --git a/tools/Makefile.am b/tools/Makefile.am
> > > > > > index 09b6dbcc3ece..b78824dadfdb 100644
> > > > > > --- a/tools/Makefile.am
> > > > > > +++ b/tools/Makefile.am
> > > > > > @@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN)
> > > > > >  intel_error_decode_LDFLAGS = -lz
> > > > > >  endif
> > > > > >  
> > > > > > -bin_PROGRAMS += intel_dp_compliance
> > > > > > +bin_PROGRAMS += intel_dp_compliance dpcd_reg
> > > > > Not the best place for this I think.
> > > > > 
> > > > > Missing meson.build so no one is actually going to build
> > > > > this.
> > > > > 
> > > > > > 
> > > > > >  intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS)
> > > > > >  intel_dp_compliance_LDADD =
> > > > > > $(top_builddir)/lib/libintel_tools.la
> > > > > >  
> > > > > > diff --git a/tools/Makefile.sources
> > > > > > b/tools/Makefile.sources
> > > > > > index abd23a0f4628..db606b28560f 100644
> > > > > > --- a/tools/Makefile.sources
> > > > > > +++ b/tools/Makefile.sources
> > > > > > @@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \
> > > > > >          intel_dp_compliance.h \
> > > > > >          intel_dp_compliance_hotplug.c \
> > > > > >          $(NULL)
> > > > > > -
> > > > > > +dpcd_reg_SOURCES = \
> > > > > > +	dpcd_reg.c \
> > > > > > +	$(NULL)
> > > > > > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..2848277aa792
> > > > > > --- /dev/null
> > > > > > +++ b/tools/dpcd_reg.c
> > > > > > @@ -0,0 +1,219 @@
> > > > > > +/*
> > > > > > + * Copyright © 2018 Intel Corporation
> > > > > > + *
> > > > > > + * Permission is hereby granted, free of charge, to any
> > > > > > person obtaining a
> > > > > > + * copy of this software and associated documentation
> > > > > > files (the "Software"),
> > > > > > + * to deal in the Software without restriction, including
> > > > > > without limitation
> > > > > > + * the rights to use, copy, modify, merge, publish,
> > > > > > distribute, sublicense,
> > > > > > + * and/or sell copies of the Software, and to permit
> > > > > > persons to whom the
> > > > > > + * Software is furnished to do so, subject to the
> > > > > > following conditions:
> > > > > > + *
> > > > > > + * The above copyright notice and this permission notice
> > > > > > (including the next
> > > > > > + * paragraph) shall be included in all copies or
> > > > > > substantial portions of the
> > > > > > + * Software.
> > > > > > + *
> > > > > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF
> > > > > > ANY KIND, EXPRESS OR
> > > > > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > > > > > MERCHANTABILITY,
> > > > > > + * FITNESS FOR A PARTICULAR PURPOSE AND
> > > > > > NONINFRINGEMENT.  IN NO EVENT SHALL
> > > > > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
> > > > > > CLAIM, DAMAGES OR OTHER
> > > > > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> > > > > > OTHERWISE, ARISING FROM,
> > > > > > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > > > > > OTHER DEALINGS IN THE
> > > > > > + * SOFTWARE.
> > > > > > + *
> > > > > > + * 		DPCD register read/decode tool
> > > > > > + * This tool wraps around DRM_DP_AUX_DEV module to provide
> > > > > > DPCD register read,
> > > > > > + * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be
> > > > > > set.
> > > > > > + */
> > > > > > +
> > > > > > +#include "igt_core.h"
> > > > > > +#include <errno.h>
> > > > > > +#include <fcntl.h>
> > > > > > +
> > > > > > +#define INVALID	0xff
> > > > > > +#define RW_SIZE	1
> > > > > > +
> > > > > > +const char aux_dev[] = "/dev/drm_dp_aux";
> > > > > > +
> > > > > > +static void print_usage(char *tool, int help)
> > > > > > +{
> > > > > > +	igt_info("DPCD register rw and decode tool\n\n");
> > > > > > +	igt_info("This tool requires
> > > > > > CONFIG_DRM_DP_AUX_CHARDEV\n"
> > > > > > +		 "to be set in the kernel config.\n");
> > > > > > +	igt_info("Usage %s command [options...]\n", tool);
> > > > > > +	igt_info("Supported commands are:\n"
> > > > > > +		 "\tread: Read a dpcd reg at an offset\n"
> > > > > > +		 "\twrite: Write a dpcd reg at an
> > > > > > offset\n"
> > > > > > +		 "\tdecode: Decode the value of a dpcd
> > > > > > reg\n");
> > > > > You didn't implement decode so no point in mentioning it.
> > > > > 
> > > > > > 
> > > > > > +	igt_info("Options for the above commands are\n"
> > > > > > +		 "\t--device: Aux device id,
> > > > > > IS_REQUIRED\n"
> > > > > > +		 "\t--help: print the usage\n"
> > > > > > +		 "\t--offset: DPCD register offset in hex,
> > > > > > IS_REQUIRED\n"
> > > > > > +		 "\t--spec: Specify DP/eDP spec version
> > > > > > for decoding, IS_OPTIONAL\n"
> > > > > > +		 "\t--val: Specify a value, for reg
> > > > > > writes\n");
> > > > > I think you should model the UI based on intel_reg.
> > > > I originally had plans to plug this *into* intel_reg, with e.g.
> > > > dpcd:
> > > > prefix. There we have the framework ready for adding names to
> > > > the
> > > > registers using separate definition files, and we have all the
> > > > frameworks ready for doing decoded dumps of the dpcd space. 

I suppose we could go with this tool as it is now and then later reuse
parts of the decode logic from intel_reg.

> > > > Just saying.
> > > That's one option. But maybe people working on non-Intel hw would
> > > want
> > > to use this as well?
> > I know, that's a consideration.
> > 
> > It's just that it's totally obvious any DPCD tool will grow a large
> > bunch of code already written in intel_reg.c. Looking at the
> > proposed
> > patch, there's only support for reading one offset at a time now.
> > The
> > first thing anyone will want is ability to read N bytes. Or a bunch
> > of
> > offsets here and there. With intel_reg, you get all of that for
> > free.
> Indeed. Actually that reminds me that it would be awesome to have a
> way
> to specify offset:count pairs for intel_reg read. Currently one
> either
> has to dump a bunch of irrelevant/non-existing registers, or execute
> intel_reg multiple times.
> 
> > 
> > Remember the times when we had separate and slightly different reg
> > tools
> > for all of the different "register ports" in intel_reg? I don't
> > particularly miss that.
> > 
> > Note also that for i915 DP debugging, with DPCD support in
> > intel_reg,
> > you could have the DPCD reads interspaced with register reads. How
> > cool
> > is that?
> Yes, that could be interesting.
> 
> > 
> > So I'm not going to insist on using intel_reg. I understand the
> > counter
> > argument. But when the two tools have grown a bunch of overlapping
> > and
> > duplicated code with slightly conflicting user interfaces, I
> > reserve the
> > right to be all, "I totally told you so."
> Maybe we should rename intel_reg to igt_reg and make sure it can
> actually be used without an Intel GPU? Other drivers could then
> perhaps also plug in their own backends to the same tool? Not sure
> how feasible that would be or whether anyone would even be interested
> in doing so.
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* Re: [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers
  2018-06-14 10:43 ` [igt-dev] [PATCH i-g-t] " Ville Syrjälä
  2018-06-14 12:50   ` Jani Nikula
@ 2018-06-15  2:00   ` Tarun Vyas
  1 sibling, 0 replies; 10+ messages in thread
From: Tarun Vyas @ 2018-06-15  2:00 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: igt-dev, dhinakaran.pandiyan

On Thu, Jun 14, 2018 at 01:43:59PM +0300, Ville Syrjälä wrote:
> On Thu, Jun 14, 2018 at 12:11:41AM -0700, Tarun Vyas wrote:
> > This tool serves as a wrapper around the constructs provided by the
> > drm_dpcd_aux_dev kernel module by working on the /dev/drm_dp_aux[n]
> > devices created by the kernel module.
> > It supports reading and writing dpcd registers on the connected aux
> > channels.
> > In the follow-up patch, support for decoding these registers will be
> > added to facilate debugging panel related issues.
> > 
> > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> > ---
> >  tools/Makefile.am      |   2 +-
> >  tools/Makefile.sources |   4 +-
> >  tools/dpcd_reg.c       | 219 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 223 insertions(+), 2 deletions(-)
> >  create mode 100644 tools/dpcd_reg.c
> > 
> > diff --git a/tools/Makefile.am b/tools/Makefile.am
> > index 09b6dbcc3ece..b78824dadfdb 100644
> > --- a/tools/Makefile.am
> > +++ b/tools/Makefile.am
> > @@ -7,7 +7,7 @@ bin_PROGRAMS += $(LIBDRM_INTEL_BIN)
> >  intel_error_decode_LDFLAGS = -lz
> >  endif
> >  
> > -bin_PROGRAMS += intel_dp_compliance
> > +bin_PROGRAMS += intel_dp_compliance dpcd_reg
> 
> Not the best place for this I think.
> 
> Missing meson.build so no one is actually going to build this.
>
Thanks for the review Ville. Will fix in v2. 
> >  intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS)
> >  intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la
> >  
> > diff --git a/tools/Makefile.sources b/tools/Makefile.sources
> > index abd23a0f4628..db606b28560f 100644
> > --- a/tools/Makefile.sources
> > +++ b/tools/Makefile.sources
> > @@ -64,4 +64,6 @@ intel_dp_compliance_SOURCES = \
> >          intel_dp_compliance.h \
> >          intel_dp_compliance_hotplug.c \
> >          $(NULL)
> > -
> > +dpcd_reg_SOURCES = \
> > +	dpcd_reg.c \
> > +	$(NULL)
> > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> > new file mode 100644
> > index 000000000000..2848277aa792
> > --- /dev/null
> > +++ b/tools/dpcd_reg.c
> > @@ -0,0 +1,219 @@
> > +/*
> > + * Copyright © 2018 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> > + * SOFTWARE.
> > + *
> > + * 		DPCD register read/decode tool
> > + * This tool wraps around DRM_DP_AUX_DEV module to provide DPCD register read,
> > + * write and decode, so CONFIG_DRM_DP_AUX_DEV needs to be set.
> > + */
> > +
> > +#include "igt_core.h"
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +
> > +#define INVALID	0xff
> > +#define RW_SIZE	1
> > +
> > +const char aux_dev[] = "/dev/drm_dp_aux";
> > +
> > +static void print_usage(char *tool, int help)
> > +{
> > +	igt_info("DPCD register rw and decode tool\n\n");
> > +	igt_info("This tool requires CONFIG_DRM_DP_AUX_CHARDEV\n"
> > +		 "to be set in the kernel config.\n");
> > +	igt_info("Usage %s command [options...]\n", tool);
> > +	igt_info("Supported commands are:\n"
> > +		 "\tread: Read a dpcd reg at an offset\n"
> > +		 "\twrite: Write a dpcd reg at an offset\n"
> > +		 "\tdecode: Decode the value of a dpcd reg\n");
> 
> You didn't implement decode so no point in mentioning it.
> 
Will remove in v2.
> > +	igt_info("Options for the above commands are\n"
> > +		 "\t--device: Aux device id, IS_REQUIRED\n"
> > +		 "\t--help: print the usage\n"
> > +		 "\t--offset: DPCD register offset in hex, IS_REQUIRED\n"
> > +		 "\t--spec: Specify DP/eDP spec version for decoding, IS_OPTIONAL\n"
> > +		 "\t--val: Specify a value, for reg writes\n");
> 
> I think you should model the UI based on intel_reg.
> 
WIll change in v2.
> > +	exit((help == 1)? EXIT_SUCCESS : EXIT_FAILURE);
> > +}
> > +
> > +static int dpcd_read(char *device, const uint32_t *offset)
> 
> Why is offset a pointer?
> 
I wasn't thinking :(
> > +{
> > +	int fd, ret;
> > +	void *buf = malloc(sizeof(uint8_t));
> 
> Pointless malloc.
> 
Yea, now I've added the --count option to read multiple bytes at a time, so with that we can keep the malloc in v2.
> > +
> > +	if(NULL != buf)
> > +		memset(buf, 0, sizeof(uint8_t));
> > +	else {
> > +		igt_warn("Can't allocate read buffer\n");
> > +		ret = EXIT_FAILURE;
> > +		goto out;
> > +	}
> > +
> > +	fd = open(device, O_RDONLY);
> > +	if (fd > 0) {
> > +		ret = pread(fd, buf, RW_SIZE, *offset);
> > +		close(fd);
> > +		if (ret < 0) {
> > +			igt_warn("Failed to read from %s aux device - error %s\n", device, strerror(errno));
> > +			ret = EXIT_FAILURE;
> > +			goto free_up;
> > +		}
> > +		printf("Value @ offset %x is %x\n", *offset, *((int *)(buf)));
> > +	}
> > +	else {
> > +		igt_warn("Failed to open %s aux device - error: %s\n", device, strerror(errno));
> > +		ret = EXIT_FAILURE;
> > +	}
> > +free_up:
> > +	free(buf);
> > +out:
> > +	return ret;
> > +}
> > +
> > +static int dpcd_write(char *device, const uint32_t *offset, const uint8_t *val)
> > +{
> > +	int fd, ret;
> > +
> > +	fd = open(device, O_RDWR);
> > +	if (fd > 0) {
> > +		ret = pwrite(fd, (const void *)val, RW_SIZE, *offset);
> > +		close(fd);
> > +		if (ret < 0) {
> > +			igt_warn("Failed to write to %s aux device - error %s\n", device, strerror(errno));
> > +			ret = EXIT_FAILURE;
> > +			goto out;
> > +		}
> > +		ret = dpcd_read(device, offset);
> > +	}
> > +	else {
> > +		igt_warn("Failed to open %s aux device - error: %s\n", device, strerror(errno));
> > +		ret = EXIT_FAILURE;
> > +	}
> > +out:
> > +	return ret;
> > +}
> > +
> > +/*TO-DO: Decode dpcd reg value.
> > + * By default, the latest DP/eDP spec
> > + * versions will be used. To override,
> > + * specify the spec version through the
> > + * --spec option.
> > +static void dpcd_decode()
> > +{
> > +}
> > +*/
> > +
> > +int main(int argc, char **argv)
> > +{
> > +	char dev_name[20];
> > +	int ret, devid, help_flg;
> > +	uint32_t offset;
> > +	uint8_t val;
> > +
> > +	enum command {
> > +		INV = -1,
> > +		READ = 2,
> > +		WRITE,
> > +		DECODE,
> > +	} cmd = INV;
> > +
> > +	struct option longopts [] = {
> > +		{ "device",	required_argument,	NULL,      'd' },
> > +		{ "help", 	no_argument, 		&help_flg,  2  },
> > +		{ "offset",	required_argument,	NULL,	   'o' },
> > +		{ "spec",	optional_argument,	NULL,	   's' },
> > +		{ "val",	required_argument,	NULL,	   'v' },
> > +		{ 0 }
> > +	};
> > +
> > +	offset = val = INVALID;
> > +	devid = -1;
> > +
> > +	while ((ret = getopt_long(argc, argv, "-:d:o:s::v:", longopts, NULL)) != -1) {
> 
> What's that "-:" about? Also the sole ":"?
> 
I wanted to make getopt_long parse *all* the arguments, so with the '-' in the begining, it will
redirect all the non-option arguments to case 1, which in this case will be the command.
The sole ':' is to distinguish between an option with missing argument vs. an invalid option which
goes to '?'. Also, the first ':' after the '-' of the optstring "-:d:o:s::v:" will enable the distinction
between "case :" and "case ?".
> > +		switch (ret) {
> > +		case 'd':
> > +			devid = strtoul(optarg, NULL, 10);
> > +			break;
> > +		case 'o':
> > +			offset = strtoul(optarg, NULL, 16);
> > +			break;
> > +		case 's':
> > +			/* TO-DO: Parse --spec version */
> > +			break;
> > +		case 'v':
> > +			val = strtoul(optarg, NULL, 16);
> > +			break;
> > +		/* Fall through for --help */
> > +		case 0:
> > +			break;
> > +		/* Command parsing */
> > +		case 1:
> > +			if (strcmp(optarg, "read") == 0)
> > +				cmd = READ;
> > +			else if (strcmp(optarg, "write") == 0)
> > +				cmd = WRITE;
> > +			else if (strcmp(optarg, "decode") == 0)
> > +				cmd = DECODE;
> > +			break;
> > +		case ':':
> > +			fprintf(stderr, "The -%c option of %s requires an argument\n", optopt, argv[0]);
> > +			print_usage(argv[0], 0);
> > +		case '?':
> > +		default :
> > +			printf("%s - option %c is invalid\n", argv[0], optopt);
> > +			print_usage(argv[0], 0);
> > +		}
> > +	}
> > +
> > +	if (help_flg == 2)
> > +		print_usage(argv[0], 1);
> > +
> > +	if(devid == -1 || offset == INVALID) {
> > +		printf("Aux device id and/or offset missing\n");
> > +		print_usage(argv[0], 0);
> > +	}
> > +
> > +	memset(dev_name, '\0', 20);
> > +	snprintf(dev_name, sizeof(aux_dev) + 2, "%s%d", aux_dev, devid);
> > +
> > +	switch (cmd) {
> > +		case READ:
> > +			ret = dpcd_read(dev_name, &offset);
> > +			break;
> > +		case WRITE:
> > +			if (val == INVALID) {
> > +				igt_warn("Write value is missing\n");
> > +				print_usage(argv[0], 0);
> > +			}
> > +			ret = dpcd_write(dev_name, &offset, &val);
> > +			break;
> > +		case DECODE:
> > +			/* TO-DO: Decode dpcd reg vals
> > +			 * drm_dpcd_decode();
> > +			*/
> > +			break;
> > +		case INV:
> > +		default:
> > +			igt_warn("Please specify a comand: read/write/decode. See usage\n");
> > +			print_usage(argv[0], 0);
> > +	}
> > +
> > +	return ret;
> > +}
> > -- 
> > 2.13.5
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

end of thread, other threads:[~2018-06-15  2:00 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-14  7:11 [igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers Tarun Vyas
2018-06-14  8:08 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
2018-06-14  9:27 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
2018-06-14 10:43 ` [igt-dev] [PATCH i-g-t] " Ville Syrjälä
2018-06-14 12:50   ` Jani Nikula
2018-06-14 12:55     ` Ville Syrjälä
2018-06-14 13:20       ` Jani Nikula
2018-06-14 14:00         ` Ville Syrjälä
2018-06-14 22:32           ` Dhinakaran Pandiyan
2018-06-15  2:00   ` Tarun Vyas

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.