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

[Jani Nikula]

On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala at 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 at 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 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.
>> >
>> >>  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