* [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