[igt-dev] [PATCH i-g-t] tools: Add a simple tool to read/write/decode dpcd registers
Tarun Vyas
tarun.vyas at intel.com
Fri Jun 15 02:00:26 UTC 2018
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 at intel.com>
> > Signed-off-by: Tarun Vyas <tarun.vyas at 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
>
> --
> Ville Syrjälä
> Intel
More information about the igt-dev
mailing list