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

[Jani Nikula <jani.nikula@linux.intel.com>]

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