drm/i915: Add link training test

Commit Message

Ander Conselvan de Oliveira Sept. 14, 2015, 11:51 a.m. UTC

---

On Fri, 2015-09-11 at 17:11 +0300, Ander Conselvan de Oliveira wrote:
> On Wed, 2015-09-09 at 11:33 +0100, Thomas Wood wrote:
> > On 8 September 2015 at 13:28, Ander Conselvan de Oliveira
> > <[e-mail]> wrote:
> > > 
> 
> > > diff --git a/link-training-test/Makefile b/link-training-test/Makefile
> > 
> > If this is meant to be part of the test suite, then it needs to be in
> > the tests directory and use the igt test infrastructure. Otherwise it
> > should be placed in tools or tools/link-training-test.
> 
> I made the test use the igt infrastructure, but I'm not sure if this is
> a good fit for it. The dependency on the kernel is on build time, but
> once compiled this can be run on any machine. This can also introduce
> build failures if the test is not kept in sync with the driver source.
> Ideally that a failure to build this would be reported as the test
> failing, but I have no idea of how to achieve that.

Alternatively, this could be in the kernel source tree directly. This
patch adds a test subdir to the i915 source dir, containing the link
training test. The test is compiled as part of the normal build using
the extra-y variable so that it doesn't get linked to the final kernel.

When make is run from the tests directory, a thin wrapper around the
tests is built and linked to the object file compiled as part of the
kernel build. Running make run_tests from the test dir runs the test
and reports success or failure.

Any thoughts?

Ander

---
 drivers/gpu/drm/i915/Makefile                      |   2 +
 drivers/gpu/drm/i915/tests/Makefile                |  39 ++
 drivers/gpu/drm/i915/tests/empty.c                 |   0
 drivers/gpu/drm/i915/tests/loader.c                | 108 +++++
 .../drm/i915/tests/test_intel_dp_link_training.c   | 464 +++++++++++++++++++++
 5 files changed, 613 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/tests/Makefile
 create mode 100644 drivers/gpu/drm/i915/tests/empty.c
 create mode 100644 drivers/gpu/drm/i915/tests/loader.c
 create mode 100644 drivers/gpu/drm/i915/tests/test_intel_dp_link_training.c

Comments

Daniel Vetter Sept. 14, 2015, 1:11 p.m. UTC | #1

On Mon, Sep 14, 2015 at 02:51:51PM +0300, Ander Conselvan de Oliveira wrote:
> ---
> 
> On Fri, 2015-09-11 at 17:11 +0300, Ander Conselvan de Oliveira wrote:
> > On Wed, 2015-09-09 at 11:33 +0100, Thomas Wood wrote:
> > > On 8 September 2015 at 13:28, Ander Conselvan de Oliveira
> > > <[e-mail]> wrote:
> > > > 
> > 
> > > > diff --git a/link-training-test/Makefile b/link-training-test/Makefile
> > > 
> > > If this is meant to be part of the test suite, then it needs to be in
> > > the tests directory and use the igt test infrastructure. Otherwise it
> > > should be placed in tools or tools/link-training-test.
> > 
> > I made the test use the igt infrastructure, but I'm not sure if this is
> > a good fit for it. The dependency on the kernel is on build time, but
> > once compiled this can be run on any machine. This can also introduce
> > build failures if the test is not kept in sync with the driver source.
> > Ideally that a failure to build this would be reported as the test
> > failing, but I have no idea of how to achieve that.
> 
> Alternatively, this could be in the kernel source tree directly. This
> patch adds a test subdir to the i915 source dir, containing the link
> training test. The test is compiled as part of the normal build using
> the extra-y variable so that it doesn't get linked to the final kernel.
> 
> When make is run from the tests directory, a thin wrapper around the
> tests is built and linked to the object file compiled as part of the
> kernel build. Running make run_tests from the test dir runs the test
> and reports success or failure.
> 
> Any thoughts?

I think there's some precedence in other subsystems to integrate unit
tests directly in the kernel, e.g. locking selftest or similar things.
Usual approach is to either have a special module (but that often means
piles of EXPORT_SYMBOL only for that selftest module). Or just a y/n
Kconfig option which enables that code and runs all the self/unit tests
when the module loads.

I'd go with that approach since it's simpler. And we'd only need to tell
QA to enable that Kconfig option for more testing.

Long-term we might go more fancy with some static debugfs interface (i.e.
in the top-level drm/ directory, no in the numbered device subdirs in
debugfs) to run specific testcases so that we can report out the results.

But as long as module reload is part of BAT we don't need that, at least
not for a start.

Anyway I think having the unit tests in the kernel makes more sense, the
igt ones will likely get out of sync fast. And igt is supposed to work
with any kernel version (hence all the igt_require checks).
-Daniel

> 
> Ander
> 
> ---
>  drivers/gpu/drm/i915/Makefile                      |   2 +
>  drivers/gpu/drm/i915/tests/Makefile                |  39 ++
>  drivers/gpu/drm/i915/tests/empty.c                 |   0
>  drivers/gpu/drm/i915/tests/loader.c                | 108 +++++
>  .../drm/i915/tests/test_intel_dp_link_training.c   | 464 +++++++++++++++++++++
>  5 files changed, 613 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/tests/Makefile
>  create mode 100644 drivers/gpu/drm/i915/tests/empty.c
>  create mode 100644 drivers/gpu/drm/i915/tests/loader.c
>  create mode 100644 drivers/gpu/drm/i915/tests/test_intel_dp_link_training.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 0851de07..7298ce5 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -97,6 +97,8 @@ i915-y += i915_vgpu.o
>  # legacy horrors
>  i915-y += i915_dma.o
>  
> +obj-y += tests/
> +
>  obj-$(CONFIG_DRM_I915)  += i915.o
>  
>  CFLAGS_i915_trace_points.o := -I$(src)
> diff --git a/drivers/gpu/drm/i915/tests/Makefile b/drivers/gpu/drm/i915/tests/Makefile
> new file mode 100644
> index 0000000..3062741
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/tests/Makefile
> @@ -0,0 +1,39 @@
> +# Please kbuild
> +# TODO: check for a better way to include this directory in the build
> +# without forcing the need for this empty file
> +obj-y += empty.o
> +
> +
> +# FIXME: The stack protector code causes tests to crash due to an improperly
> +# initialized %gs, so keep it disabled for now.
> +ccflags-y := -fstack-protector-explicit
> +
> +extra-y += test_intel_dp_link_training.o
> +
> +# If make is run from this directory
> +ifeq (0, $(MAKELEVEL))
> +
> +# For strlcpy
> +LDFLAGS = -lbsd
> +
> +TEST_PROGS = \
> +	test_intel_dp_link_training
> +
> +all: tests
> +
> +tests: $(TEST_PROGS)
> +
> +run_tests: tests
> +	@for TEST in $(TEST_PROGS); do \
> +		(./$$TEST && echo "$$TEST [PASS]") || echo "$$TEST [FAIL]"; \
> +	done
> +
> +test_%.o: test_%.c ../%.c
> +	make -C ../../../../../ drivers/gpu/drm/i915/tests/$@
> +
> +loader.o: loader.c
> +	$(CC) $(CFLAGS) -c -o $@ $<
> +
> +%: %.o loader.o
> +	$(CC) -o $@ $^ $(LDFLAGS)
> +endif
> diff --git a/drivers/gpu/drm/i915/tests/empty.c b/drivers/gpu/drm/i915/tests/empty.c
> new file mode 100644
> index 0000000..e69de29
> diff --git a/drivers/gpu/drm/i915/tests/loader.c b/drivers/gpu/drm/i915/tests/loader.c
> new file mode 100644
> index 0000000..9e38816
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/tests/loader.c
> @@ -0,0 +1,108 @@
> +/*
> + * Copyright © 2015 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.
> + *
> + * Authors:
> + *    Ander Conselvan de Oliveira <[e-mail]>
> + *
> + */
> +
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <stdarg.h>
> +
> +void run_test(void);
> +
> +void assert(bool condition)
> +{
> +	if (!condition) {
> +		printf("Assert failed\n");
> +		abort();
> +	}
> +}
> +
> +struct {
> +	uint8_t padding[32];
> +} param_ops_int;
> +
> +unsigned int drm_debug = 0;	/* 1 to enable debug output */
> +
> +void drm_err(const char *format, ...)
> +{
> +	va_list args;
> +
> +	va_start(args, format);
> +	vprintf(format, args);
> +	va_end(args);
> +
> +}
> +
> +void drm_ut_debug_printk(const char *function_name, const char *format, ...)
> +{
> +	va_list args;
> +
> +	va_start(args, format);
> +	printf("[%s] ", function_name);
> +	vprintf(format, args);
> +	va_end(args);
> +}
> +
> +void __const_udelay(unsigned long xloops)
> +{
> +}
> +
> +struct mutex;
> +void mutex_lock_nested(struct mutex *lock, unsigned int subclass)
> +{
> +}
> +
> +void mutex_unlock(struct mutex *lock)
> +{
> +}
> +
> +struct lock_class_key;
> +void
> +__mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key)
> +{
> +}
> +
> +void usleep_range(unsigned int min, unsigned int max)
> +{
> +}
> +
> +struct i2c_adapter;
> +int i2c_add_adapter(struct i2c_adapter *adapter)
> +{
> +	return 0;
> +}
> +
> +void i2c_del_adapter(struct i2c_adapter *adapater)
> +{
> +}
> +
> +int
> +main(int argc, char *argv[])
> +{
> +	run_test();
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/tests/test_intel_dp_link_training.c b/drivers/gpu/drm/i915/tests/test_intel_dp_link_training.c
> new file mode 100644
> index 0000000..11868f8
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/tests/test_intel_dp_link_training.c
> @@ -0,0 +1,464 @@
> +/*
> + * Copyright © 2015 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.
> + *
> + * Authors:
> + *    Ander Conselvan de Oliveira <[e-mail]>
> + *
> + */
> +
> +#include "../intel_dp_link_training.c"
> +
> +#define drm_dp_dpcd_write	real_drm_dp_dpcd_write
> +#include "../../drm_dp_helper.c"
> +#undef drm_dp_dpcd_write
> +
> +void assert(bool condition);
> +
> +
> +struct sink_device {
> +	ssize_t (*dpcd_write)(struct sink_device *sink, unsigned int offset,
> +			      void *buffer, size_t size);
> +	bool (*get_link_status)(struct sink_device *sink,
> +				uint8_t link_status[DP_LINK_STATUS_SIZE]);
> +
> +	struct {
> +		bool lane_count_and_bw_set;
> +		bool training_pattern_1_set;
> +		bool started_with_non_zero_levels;
> +		bool cr_done;
> +		bool channel_eq_done;
> +
> +		uint8_t dpcd[0x3000];
> +	} data;
> +};
> +
> +/* Fake sink device implementation */
> +
> +static uint8_t
> +sink_device_lane_count(struct sink_device *sink)
> +{
> +	return sink->data.dpcd[DP_LANE_COUNT_SET];
> +}
> +
> +static uint8_t
> +sink_device_get_training_pattern(struct sink_device *sink)
> +{
> +	return sink->data.dpcd[DP_TRAINING_PATTERN_SET] & DP_TRAINING_PATTERN_MASK;
> +}
> +
> +static uint8_t
> +sink_device_get_voltage_swing(struct sink_device *sink, int lane)
> +{
> +	return sink->data.dpcd[DP_TRAINING_LANE0_SET + lane] &
> +		DP_TRAIN_VOLTAGE_SWING_MASK;
> +}
> +
> +static uint8_t
> +sink_device_get_pre_emphasis_level(struct sink_device *sink, int lane)
> +{
> +	return (sink->data.dpcd[DP_TRAINING_LANE0_SET + lane] &
> +		 DP_TRAIN_PRE_EMPHASIS_MASK) >> DP_TRAIN_PRE_EMPHASIS_SHIFT;
> +}
> +
> +static void
> +sink_device_check_lane_count_and_bw(struct sink_device *sink)
> +{
> +	if (sink->data.lane_count_and_bw_set)
> +		return;
> +
> +	assert(sink->data.dpcd[DP_TRAINING_PATTERN_SET] == 0);
> +
> +	if (sink->data.dpcd[DP_LINK_BW_SET] != 0 &&
> +	    sink->data.dpcd[DP_LANE_COUNT_SET] != 0)
> +		sink->data.lane_count_and_bw_set = true;
> +}
> +
> +static void
> +sink_device_check_pattern_1_set(struct sink_device *sink)
> +{
> +	int lane;
> +
> +	if (!sink->data.lane_count_and_bw_set ||
> +	    sink->data.training_pattern_1_set)
> +		return;
> +
> +	assert(sink_device_get_training_pattern(sink) <= DP_TRAINING_PATTERN_1);
> +
> +	if (sink_device_get_training_pattern(sink) != DP_TRAINING_PATTERN_1)
> +		return;
> +
> +	assert(sink->data.dpcd[DP_LINK_BW_SET] == DP_LINK_BW_1_62 ||
> +	       sink->data.dpcd[DP_LINK_BW_SET] == DP_LINK_BW_2_7);
> +
> +	assert(sink->data.dpcd[DP_LANE_COUNT_SET] == 1 ||
> +		   sink->data.dpcd[DP_LANE_COUNT_SET] == 2 ||
> +		   sink->data.dpcd[DP_LANE_COUNT_SET] == 4);
> +
> +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> +		if (sink_device_get_voltage_swing(sink, lane) != DP_TRAIN_VOLTAGE_SWING_LEVEL_0 ||
> +		    sink_device_get_pre_emphasis_level(sink, lane) != DP_TRAIN_PRE_EMPH_LEVEL_0)
> +			sink->data.started_with_non_zero_levels = true;
> +	}
> +
> +	sink->data.training_pattern_1_set = true;
> +}
> +
> +static void
> +sink_device_check_pattern_2_set(struct sink_device *sink)
> +{
> +	if (!sink->data.cr_done)
> +		return;
> +
> +	assert(sink_device_get_training_pattern(sink) == DP_TRAINING_PATTERN_2);
> +}
> +
> +static void
> +sink_device_check_pattern_disable(struct sink_device *sink)
> +{
> +	if (!sink->data.cr_done || ! sink->data.channel_eq_done)
> +		return;
> +
> +	assert(sink_device_get_training_pattern(sink) == DP_TRAINING_PATTERN_DISABLE);
> +}
> +
> +static ssize_t
> +sink_device_dpcd_write(struct sink_device *sink, unsigned int offset,
> +		       void *buffer, size_t size)
> +{
> +	memcpy(sink->data.dpcd + offset, buffer, size);
> +
> +	sink_device_check_lane_count_and_bw(sink);
> +
> +	if (!sink->data.cr_done)
> +		sink_device_check_pattern_1_set(sink);
> +	else if (!sink->data.channel_eq_done)
> +		sink_device_check_pattern_2_set(sink);
> +	else
> +		sink_device_check_pattern_disable(sink);
> +
> +	return size;
> +}
> +
> +static bool
> +sink_device_max_voltage_reached(struct sink_device *sink, int lane)
> +{
> +	return (sink->data.dpcd[DP_TRAINING_LANE0_SET + lane] & DP_TRAIN_MAX_SWING_REACHED) ==
> +		DP_TRAIN_MAX_SWING_REACHED;
> +}
> +
> +static bool
> +sink_device_max_pre_emphasis_reached(struct sink_device *sink, int lane)
> +{
> +	return (sink->data.dpcd[DP_TRAINING_LANE0_SET + lane] & DP_TRAIN_MAX_PRE_EMPHASIS_REACHED) ==
> +		DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
> +}
> +
> +static void
> +sink_device_set_adjust_voltage(struct sink_device *sink,
> +			       int lane, uint8_t level)
> +{
> +	int shift = DP_ADJUST_VOLTAGE_SWING_LANE1_SHIFT * (lane & 1);
> +
> +	sink->data.dpcd[DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1)] &=
> +		~(DP_ADJUST_VOLTAGE_SWING_LANE0_MASK << shift);
> +	sink->data.dpcd[DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1)] |=
> +		level << shift;
> +}
> +
> +static void
> +sink_device_set_adjust_pre_emphasis(struct sink_device *sink,
> +				    int lane, uint8_t level)
> +{
> +	int shift = DP_ADJUST_VOLTAGE_SWING_LANE1_SHIFT * (lane & 1);
> +
> +	sink->data.dpcd[DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1)] &=
> +		~(DP_ADJUST_PRE_EMPHASIS_LANE0_MASK << shift);
> +	sink->data.dpcd[DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1)] |=
> +		level << (DP_ADJUST_PRE_EMPHASIS_LANE0_SHIFT + shift);
> +}
> +
> +static bool
> +sink_device_request_higher_voltage_swing(struct sink_device *sink)
> +{
> +	bool max_reached = false;
> +	int lane;
> +
> +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> +		if (sink_device_max_voltage_reached(sink, lane)) {
> +			max_reached = true;
> +			break;
> +		}
> +	}
> +
> +	if (max_reached)
> +		return false;
> +
> +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> +		uint8_t new_voltage =
> +			sink_device_get_voltage_swing(sink, lane) + 1;
> +
> +		sink_device_set_adjust_voltage(sink, lane, new_voltage);
> +	}
> +
> +	return true;
> +}
> +
> +static bool
> +sink_device_request_higher_pre_emphasis(struct sink_device *sink)
> +{
> +	bool max_reached = false;
> +	int lane;
> +
> +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> +		if (sink_device_max_pre_emphasis_reached(sink, lane)) {
> +			max_reached = true;
> +			break;
> +		}
> +	}
> +
> +	if (max_reached)
> +		return false;
> +
> +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> +		uint8_t new_pre_emphasis =
> +			sink_device_get_pre_emphasis_level(sink, lane) + 1;
> +
> +		sink_device_set_adjust_pre_emphasis(sink, lane, new_pre_emphasis);
> +	}
> +
> +	return true;
> +}
> +
> +static void
> +sink_device_mark_cr_done(struct sink_device *sink)
> +{
> +	int lane;
> +
> +	for (lane = 0; lane < sink_device_lane_count(sink); lane++)
> +		sink->data.dpcd[DP_LANE0_1_STATUS + (lane >> 1)] |=
> +			DP_LANE_CR_DONE << (4 * (lane & 1));
> +
> +	sink->data.cr_done = true;
> +}
> +
> +static void
> +sink_device_mark_channel_eq_done(struct sink_device *sink)
> +{
> +	int lane;
> +
> +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> +		uint8_t mask = (DP_LANE_CHANNEL_EQ_DONE | DP_LANE_SYMBOL_LOCKED);
> +		sink->data.dpcd[DP_LANE0_1_STATUS + (lane >> 1)] |=
> +			mask << (4 * (lane & 1));
> +	}
> +
> +	sink->data.dpcd[DP_LANE_ALIGN_STATUS_UPDATED] |= DP_INTERLANE_ALIGN_DONE;
> +
> +	sink->data.channel_eq_done = true;
> +}
> +
> +static bool
> +sink_device_get_link_status(struct sink_device *sink,
> +			    uint8_t link_status[DP_LINK_STATUS_SIZE])
> +{
> +	if (!sink->data.cr_done) {
> +		if (!sink_device_request_higher_voltage_swing(sink))
> +			sink_device_mark_cr_done(sink);
> +	} else if (!sink->data.channel_eq_done) {
> +		if (!sink_device_request_higher_pre_emphasis(sink))
> +			sink_device_mark_channel_eq_done(sink);
> +	}
> +
> +	memcpy(link_status, sink->data.dpcd + DP_LANE0_1_STATUS,
> +	       DP_LINK_STATUS_SIZE);
> +
> +	return true;
> +}
> +
> +static void
> +sink_device_reset(struct sink_device *sink, int lanes, uint8_t link_bw)
> +{
> +	memset(&sink->data, 0, sizeof sink->data);
> +	sink->data.dpcd[DP_MAX_LINK_RATE] = link_bw;
> +	sink->data.dpcd[DP_MAX_LANE_COUNT] = lanes;
> +}
> +
> +static struct sink_device simple_sink = {
> +	.get_link_status = sink_device_get_link_status,
> +	.dpcd_write = sink_device_dpcd_write,
> +};
> +
> +/* Glue code */
> +
> +struct test_intel_dp {
> +	struct intel_dp dp;
> +	struct sink_device *sink;
> +	uint8_t link_bw;
> +
> +	uint8_t max_voltage;
> +	uint8_t max_pre_emphasis;
> +};
> +
> +static struct test_intel_dp *
> +to_test_intel_dp(struct intel_dp *dp)
> +{
> +	return container_of(dp, struct test_intel_dp, dp);
> +}
> +
> +void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
> +{
> +}
> +
> +bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp)
> +{
> +	return false;
> +}
> +
> +bool
> +intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
> +{
> +	struct sink_device *sink = to_test_intel_dp(intel_dp)->sink;
> +	return sink->get_link_status(sink, link_status);
> +}
> +
> +void
> +intel_dp_update_signal_levels(struct intel_dp *intel_dp)
> +{
> +}
> +
> +void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
> +			   uint8_t *link_bw, uint8_t *rate_select)
> +{
> +	*link_bw = to_test_intel_dp(intel_dp)->link_bw;
> +	*rate_select = 0;
> +}
> +
> +void
> +intel_dp_program_link_training_pattern(struct intel_dp *intel_dp,
> +				       uint8_t dp_train_pat)
> +{
> +}
> +
> +uint8_t
> +intel_dp_voltage_max(struct intel_dp *intel_dp)
> +{
> +	return to_test_intel_dp(intel_dp)->max_voltage <<
> +		DP_TRAIN_VOLTAGE_SWING_SHIFT;
> +}
> +
> +uint8_t
> +intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing)
> +{
> +	return to_test_intel_dp(intel_dp)->max_pre_emphasis <<
> +		DP_TRAIN_PRE_EMPHASIS_SHIFT;
> +}
> +
> +ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
> +			  void *buffer, size_t size)
> +{
> +	struct intel_dp *intel_dp =
> +		container_of(aux, struct intel_dp, aux);
> +	struct sink_device *sink = to_test_intel_dp(intel_dp)->sink;
> +
> +	return sink->dpcd_write(sink, offset, buffer, size);
> +}
> +
> +/* --- */
> +
> +static struct test_intel_dp test_dp;
> +
> +static void
> +do_test(struct sink_device *sink, int lanes, uint8_t link_bw,
> +	     uint8_t max_voltage, uint8_t max_pre_emphasis)
> +{
> +	int lane;
> +
> +	memset(&test_dp, 0, sizeof test_dp);
> +	test_dp.dp.lane_count = lanes;
> +	test_dp.link_bw = link_bw;
> +	test_dp.sink = sink;
> +	test_dp.max_voltage =
> +		max_voltage >> DP_TRAIN_VOLTAGE_SWING_SHIFT;
> +	test_dp.max_pre_emphasis =
> +		max_pre_emphasis >> DP_TRAIN_PRE_EMPHASIS_SHIFT;
> +
> +	sink_device_reset(sink, lanes, link_bw);
> +
> +	intel_dp_start_link_train(&test_dp.dp);
> +	intel_dp_stop_link_train(&test_dp.dp);
> +
> +	assert(sink->data.cr_done);
> +	assert(sink->data.channel_eq_done);
> +
> +	for (lane = 0; lane < test_dp.dp.lane_count; lane++) {
> +		uint8_t cur_v = sink_device_get_voltage_swing(sink, lane);
> +		uint8_t cur_p = sink_device_get_pre_emphasis_level(sink, lane);
> +
> +		assert(cur_v == test_dp.max_voltage);
> +		assert(cur_p == test_dp.max_pre_emphasis);
> +	}
> +}
> +
> +int test_lanes[] = {
> +	1, 2, 4,
> +};
> +
> +uint8_t test_bw[] = {
> +	DP_LINK_BW_1_62,
> +	DP_LINK_BW_2_7,
> +};
> +
> +uint8_t test_max_voltage[] = {
> +	DP_TRAIN_VOLTAGE_SWING_LEVEL_0,
> +	DP_TRAIN_VOLTAGE_SWING_LEVEL_1,
> +	DP_TRAIN_VOLTAGE_SWING_LEVEL_2,
> +	DP_TRAIN_VOLTAGE_SWING_LEVEL_3,
> +};
> +
> +uint8_t test_max_pre_emphasis[] = {
> +	DP_TRAIN_PRE_EMPH_LEVEL_0,
> +	DP_TRAIN_PRE_EMPH_LEVEL_1,
> +	DP_TRAIN_PRE_EMPH_LEVEL_2,
> +	DP_TRAIN_PRE_EMPH_LEVEL_3,
> +};
> +
> +void
> +run_test(void)
> +{
> +	int lane, bw, voltage, emph;
> +
> +	for (lane = 0; lane < ARRAY_SIZE(test_lanes); lane++)
> +	for (bw = 0; bw < ARRAY_SIZE(test_bw); bw++)
> +	for (voltage = 0; voltage < ARRAY_SIZE(test_max_voltage); voltage++)
> +	for (emph = 0; emph < ARRAY_SIZE(test_max_pre_emphasis); emph++) {
> +		DRM_DEBUG_KMS("l%d-bw%d-v%d-pe%d",
> +			      test_lanes[lane],
> +			      test_bw[bw],
> +			      test_max_voltage[voltage],
> +			      test_max_pre_emphasis[emph]);
> +		do_test(&simple_sink,
> +			test_lanes[lane],
> +			test_bw[bw],
> +			test_max_voltage[voltage],
> +			test_max_pre_emphasis[emph]);
> +	}
> +}
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Ander Conselvan de Oliveira Sept. 14, 2015, 1:38 p.m. UTC | #2

On Mon, 2015-09-14 at 15:11 +0200, Daniel Vetter wrote:
> On Mon, Sep 14, 2015 at 02:51:51PM +0300, Ander Conselvan de Oliveira wrote:
> > ---
> > 
> > On Fri, 2015-09-11 at 17:11 +0300, Ander Conselvan de Oliveira wrote:
> > > On Wed, 2015-09-09 at 11:33 +0100, Thomas Wood wrote:
> > > > On 8 September 2015 at 13:28, Ander Conselvan de Oliveira
> > > > <[e-mail]> wrote:
> > > > > 
> > > 
> > > > > diff --git a/link-training-test/Makefile b/link-training-test/Makefile
> > > > 
> > > > If this is meant to be part of the test suite, then it needs to be in
> > > > the tests directory and use the igt test infrastructure. Otherwise it
> > > > should be placed in tools or tools/link-training-test.
> > > 
> > > I made the test use the igt infrastructure, but I'm not sure if this is
> > > a good fit for it. The dependency on the kernel is on build time, but
> > > once compiled this can be run on any machine. This can also introduce
> > > build failures if the test is not kept in sync with the driver source.
> > > Ideally that a failure to build this would be reported as the test
> > > failing, but I have no idea of how to achieve that.
> > 
> > Alternatively, this could be in the kernel source tree directly. This
> > patch adds a test subdir to the i915 source dir, containing the link
> > training test. The test is compiled as part of the normal build using
> > the extra-y variable so that it doesn't get linked to the final kernel.
> > 
> > When make is run from the tests directory, a thin wrapper around the
> > tests is built and linked to the object file compiled as part of the
> > kernel build. Running make run_tests from the test dir runs the test
> > and reports success or failure.
> > 
> > Any thoughts?
> 
> I think there's some precedence in other subsystems to integrate unit
> tests directly in the kernel, e.g. locking selftest or similar things.
> Usual approach is to either have a special module (but that often means
> piles of EXPORT_SYMBOL only for that selftest module). Or just a y/n
> Kconfig option which enables that code and runs all the self/unit tests
> when the module loads.
> 
> I'd go with that approach since it's simpler. And we'd only need to tell
> QA to enable that Kconfig option for more testing.

I'll have a look into that Kconfig approach, but there's a couple of things
I like about having the unit test as user space binaries:

  - there's no need to boot the newly compiled kernel, so doing a test run
    is super fast;
  - the binaries can be debugged with gdb just like other user space stuff.

The downside is the stubbing necessary. In this particular case it wasn't
that bad, but I guess it could get really ugly.

> Long-term we might go more fancy with some static debugfs interface (i.e.
> in the top-level drm/ directory, no in the numbered device subdirs in
> debugfs) to run specific testcases so that we can report out the results.
> 
> But as long as module reload is part of BAT we don't need that, at least
> not for a start.
> 
> Anyway I think having the unit tests in the kernel makes more sense, the
> igt ones will likely get out of sync fast. And igt is supposed to work
> with any kernel version (hence all the igt_require checks).

Yeah, agreed.

Ander

> -Daniel
> 
> > 
> > Ander
> > 
> > ---
> >  drivers/gpu/drm/i915/Makefile                      |   2 +
> >  drivers/gpu/drm/i915/tests/Makefile                |  39 ++
> >  drivers/gpu/drm/i915/tests/empty.c                 |   0
> >  drivers/gpu/drm/i915/tests/loader.c                | 108 +++++
> >  .../drm/i915/tests/test_intel_dp_link_training.c   | 464 +++++++++++++++++++++
> >  5 files changed, 613 insertions(+)
> >  create mode 100644 drivers/gpu/drm/i915/tests/Makefile
> >  create mode 100644 drivers/gpu/drm/i915/tests/empty.c
> >  create mode 100644 drivers/gpu/drm/i915/tests/loader.c
> >  create mode 100644 drivers/gpu/drm/i915/tests/test_intel_dp_link_training.c
> > 
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index 0851de07..7298ce5 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -97,6 +97,8 @@ i915-y += i915_vgpu.o
> >  # legacy horrors
> >  i915-y += i915_dma.o
> >  
> > +obj-y += tests/
> > +
> >  obj-$(CONFIG_DRM_I915)  += i915.o
> >  
> >  CFLAGS_i915_trace_points.o := -I$(src)
> > diff --git a/drivers/gpu/drm/i915/tests/Makefile b/drivers/gpu/drm/i915/tests/Makefile
> > new file mode 100644
> > index 0000000..3062741
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/tests/Makefile
> > @@ -0,0 +1,39 @@
> > +# Please kbuild
> > +# TODO: check for a better way to include this directory in the build
> > +# without forcing the need for this empty file
> > +obj-y += empty.o
> > +
> > +
> > +# FIXME: The stack protector code causes tests to crash due to an improperly
> > +# initialized %gs, so keep it disabled for now.
> > +ccflags-y := -fstack-protector-explicit
> > +
> > +extra-y += test_intel_dp_link_training.o
> > +
> > +# If make is run from this directory
> > +ifeq (0, $(MAKELEVEL))
> > +
> > +# For strlcpy
> > +LDFLAGS = -lbsd
> > +
> > +TEST_PROGS = \
> > +	test_intel_dp_link_training
> > +
> > +all: tests
> > +
> > +tests: $(TEST_PROGS)
> > +
> > +run_tests: tests
> > +	@for TEST in $(TEST_PROGS); do \
> > +		(./$$TEST && echo "$$TEST [PASS]") || echo "$$TEST [FAIL]"; \
> > +	done
> > +
> > +test_%.o: test_%.c ../%.c
> > +	make -C ../../../../../ drivers/gpu/drm/i915/tests/$@
> > +
> > +loader.o: loader.c
> > +	$(CC) $(CFLAGS) -c -o $@ $<
> > +
> > +%: %.o loader.o
> > +	$(CC) -o $@ $^ $(LDFLAGS)
> > +endif
> > diff --git a/drivers/gpu/drm/i915/tests/empty.c b/drivers/gpu/drm/i915/tests/empty.c
> > new file mode 100644
> > index 0000000..e69de29
> > diff --git a/drivers/gpu/drm/i915/tests/loader.c b/drivers/gpu/drm/i915/tests/loader.c
> > new file mode 100644
> > index 0000000..9e38816
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/tests/loader.c
> > @@ -0,0 +1,108 @@
> > +/*
> > + * Copyright © 2015 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.
> > + *
> > + * Authors:
> > + *    Ander Conselvan de Oliveira <[e-mail]>
> > + *
> > + */
> > +
> > +#include <stdbool.h>
> > +#include <stdint.h>
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <stdarg.h>
> > +
> > +void run_test(void);
> > +
> > +void assert(bool condition)
> > +{
> > +	if (!condition) {
> > +		printf("Assert failed\n");
> > +		abort();
> > +	}
> > +}
> > +
> > +struct {
> > +	uint8_t padding[32];
> > +} param_ops_int;
> > +
> > +unsigned int drm_debug = 0;	/* 1 to enable debug output */
> > +
> > +void drm_err(const char *format, ...)
> > +{
> > +	va_list args;
> > +
> > +	va_start(args, format);
> > +	vprintf(format, args);
> > +	va_end(args);
> > +
> > +}
> > +
> > +void drm_ut_debug_printk(const char *function_name, const char *format, ...)
> > +{
> > +	va_list args;
> > +
> > +	va_start(args, format);
> > +	printf("[%s] ", function_name);
> > +	vprintf(format, args);
> > +	va_end(args);
> > +}
> > +
> > +void __const_udelay(unsigned long xloops)
> > +{
> > +}
> > +
> > +struct mutex;
> > +void mutex_lock_nested(struct mutex *lock, unsigned int subclass)
> > +{
> > +}
> > +
> > +void mutex_unlock(struct mutex *lock)
> > +{
> > +}
> > +
> > +struct lock_class_key;
> > +void
> > +__mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key)
> > +{
> > +}
> > +
> > +void usleep_range(unsigned int min, unsigned int max)
> > +{
> > +}
> > +
> > +struct i2c_adapter;
> > +int i2c_add_adapter(struct i2c_adapter *adapter)
> > +{
> > +	return 0;
> > +}
> > +
> > +void i2c_del_adapter(struct i2c_adapter *adapater)
> > +{
> > +}
> > +
> > +int
> > +main(int argc, char *argv[])
> > +{
> > +	run_test();
> > +	return 0;
> > +}
> > diff --git a/drivers/gpu/drm/i915/tests/test_intel_dp_link_training.c 
> > b/drivers/gpu/drm/i915/tests/test_intel_dp_link_training.c
> > new file mode 100644
> > index 0000000..11868f8
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/tests/test_intel_dp_link_training.c
> > @@ -0,0 +1,464 @@
> > +/*
> > + * Copyright © 2015 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.
> > + *
> > + * Authors:
> > + *    Ander Conselvan de Oliveira <[e-mail]>
> > + *
> > + */
> > +
> > +#include "../intel_dp_link_training.c"
> > +
> > +#define drm_dp_dpcd_write	real_drm_dp_dpcd_write
> > +#include "../../drm_dp_helper.c"
> > +#undef drm_dp_dpcd_write
> > +
> > +void assert(bool condition);
> > +
> > +
> > +struct sink_device {
> > +	ssize_t (*dpcd_write)(struct sink_device *sink, unsigned int offset,
> > +			      void *buffer, size_t size);
> > +	bool (*get_link_status)(struct sink_device *sink,
> > +				uint8_t link_status[DP_LINK_STATUS_SIZE]);
> > +
> > +	struct {
> > +		bool lane_count_and_bw_set;
> > +		bool training_pattern_1_set;
> > +		bool started_with_non_zero_levels;
> > +		bool cr_done;
> > +		bool channel_eq_done;
> > +
> > +		uint8_t dpcd[0x3000];
> > +	} data;
> > +};
> > +
> > +/* Fake sink device implementation */
> > +
> > +static uint8_t
> > +sink_device_lane_count(struct sink_device *sink)
> > +{
> > +	return sink->data.dpcd[DP_LANE_COUNT_SET];
> > +}
> > +
> > +static uint8_t
> > +sink_device_get_training_pattern(struct sink_device *sink)
> > +{
> > +	return sink->data.dpcd[DP_TRAINING_PATTERN_SET] & DP_TRAINING_PATTERN_MASK;
> > +}
> > +
> > +static uint8_t
> > +sink_device_get_voltage_swing(struct sink_device *sink, int lane)
> > +{
> > +	return sink->data.dpcd[DP_TRAINING_LANE0_SET + lane] &
> > +		DP_TRAIN_VOLTAGE_SWING_MASK;
> > +}
> > +
> > +static uint8_t
> > +sink_device_get_pre_emphasis_level(struct sink_device *sink, int lane)
> > +{
> > +	return (sink->data.dpcd[DP_TRAINING_LANE0_SET + lane] &
> > +		 DP_TRAIN_PRE_EMPHASIS_MASK) >> DP_TRAIN_PRE_EMPHASIS_SHIFT;
> > +}
> > +
> > +static void
> > +sink_device_check_lane_count_and_bw(struct sink_device *sink)
> > +{
> > +	if (sink->data.lane_count_and_bw_set)
> > +		return;
> > +
> > +	assert(sink->data.dpcd[DP_TRAINING_PATTERN_SET] == 0);
> > +
> > +	if (sink->data.dpcd[DP_LINK_BW_SET] != 0 &&
> > +	    sink->data.dpcd[DP_LANE_COUNT_SET] != 0)
> > +		sink->data.lane_count_and_bw_set = true;
> > +}
> > +
> > +static void
> > +sink_device_check_pattern_1_set(struct sink_device *sink)
> > +{
> > +	int lane;
> > +
> > +	if (!sink->data.lane_count_and_bw_set ||
> > +	    sink->data.training_pattern_1_set)
> > +		return;
> > +
> > +	assert(sink_device_get_training_pattern(sink) <= DP_TRAINING_PATTERN_1);
> > +
> > +	if (sink_device_get_training_pattern(sink) != DP_TRAINING_PATTERN_1)
> > +		return;
> > +
> > +	assert(sink->data.dpcd[DP_LINK_BW_SET] == DP_LINK_BW_1_62 ||
> > +	       sink->data.dpcd[DP_LINK_BW_SET] == DP_LINK_BW_2_7);
> > +
> > +	assert(sink->data.dpcd[DP_LANE_COUNT_SET] == 1 ||
> > +		   sink->data.dpcd[DP_LANE_COUNT_SET] == 2 ||
> > +		   sink->data.dpcd[DP_LANE_COUNT_SET] == 4);
> > +
> > +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> > +		if (sink_device_get_voltage_swing(sink, lane) != DP_TRAIN_VOLTAGE_SWING_LEVEL_0 
> > ||
> > +		    sink_device_get_pre_emphasis_level(sink, lane) != 
> > DP_TRAIN_PRE_EMPH_LEVEL_0)
> > +			sink->data.started_with_non_zero_levels = true;
> > +	}
> > +
> > +	sink->data.training_pattern_1_set = true;
> > +}
> > +
> > +static void
> > +sink_device_check_pattern_2_set(struct sink_device *sink)
> > +{
> > +	if (!sink->data.cr_done)
> > +		return;
> > +
> > +	assert(sink_device_get_training_pattern(sink) == DP_TRAINING_PATTERN_2);
> > +}
> > +
> > +static void
> > +sink_device_check_pattern_disable(struct sink_device *sink)
> > +{
> > +	if (!sink->data.cr_done || ! sink->data.channel_eq_done)
> > +		return;
> > +
> > +	assert(sink_device_get_training_pattern(sink) == DP_TRAINING_PATTERN_DISABLE);
> > +}
> > +
> > +static ssize_t
> > +sink_device_dpcd_write(struct sink_device *sink, unsigned int offset,
> > +		       void *buffer, size_t size)
> > +{
> > +	memcpy(sink->data.dpcd + offset, buffer, size);
> > +
> > +	sink_device_check_lane_count_and_bw(sink);
> > +
> > +	if (!sink->data.cr_done)
> > +		sink_device_check_pattern_1_set(sink);
> > +	else if (!sink->data.channel_eq_done)
> > +		sink_device_check_pattern_2_set(sink);
> > +	else
> > +		sink_device_check_pattern_disable(sink);
> > +
> > +	return size;
> > +}
> > +
> > +static bool
> > +sink_device_max_voltage_reached(struct sink_device *sink, int lane)
> > +{
> > +	return (sink->data.dpcd[DP_TRAINING_LANE0_SET + lane] & DP_TRAIN_MAX_SWING_REACHED) ==
> > +		DP_TRAIN_MAX_SWING_REACHED;
> > +}
> > +
> > +static bool
> > +sink_device_max_pre_emphasis_reached(struct sink_device *sink, int lane)
> > +{
> > +	return (sink->data.dpcd[DP_TRAINING_LANE0_SET + lane] & 
> > DP_TRAIN_MAX_PRE_EMPHASIS_REACHED) ==
> > +		DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
> > +}
> > +
> > +static void
> > +sink_device_set_adjust_voltage(struct sink_device *sink,
> > +			       int lane, uint8_t level)
> > +{
> > +	int shift = DP_ADJUST_VOLTAGE_SWING_LANE1_SHIFT * (lane & 1);
> > +
> > +	sink->data.dpcd[DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1)] &=
> > +		~(DP_ADJUST_VOLTAGE_SWING_LANE0_MASK << shift);
> > +	sink->data.dpcd[DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1)] |=
> > +		level << shift;
> > +}
> > +
> > +static void
> > +sink_device_set_adjust_pre_emphasis(struct sink_device *sink,
> > +				    int lane, uint8_t level)
> > +{
> > +	int shift = DP_ADJUST_VOLTAGE_SWING_LANE1_SHIFT * (lane & 1);
> > +
> > +	sink->data.dpcd[DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1)] &=
> > +		~(DP_ADJUST_PRE_EMPHASIS_LANE0_MASK << shift);
> > +	sink->data.dpcd[DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1)] |=
> > +		level << (DP_ADJUST_PRE_EMPHASIS_LANE0_SHIFT + shift);
> > +}
> > +
> > +static bool
> > +sink_device_request_higher_voltage_swing(struct sink_device *sink)
> > +{
> > +	bool max_reached = false;
> > +	int lane;
> > +
> > +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> > +		if (sink_device_max_voltage_reached(sink, lane)) {
> > +			max_reached = true;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (max_reached)
> > +		return false;
> > +
> > +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> > +		uint8_t new_voltage =
> > +			sink_device_get_voltage_swing(sink, lane) + 1;
> > +
> > +		sink_device_set_adjust_voltage(sink, lane, new_voltage);
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static bool
> > +sink_device_request_higher_pre_emphasis(struct sink_device *sink)
> > +{
> > +	bool max_reached = false;
> > +	int lane;
> > +
> > +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> > +		if (sink_device_max_pre_emphasis_reached(sink, lane)) {
> > +			max_reached = true;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (max_reached)
> > +		return false;
> > +
> > +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> > +		uint8_t new_pre_emphasis =
> > +			sink_device_get_pre_emphasis_level(sink, lane) + 1;
> > +
> > +		sink_device_set_adjust_pre_emphasis(sink, lane, new_pre_emphasis);
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static void
> > +sink_device_mark_cr_done(struct sink_device *sink)
> > +{
> > +	int lane;
> > +
> > +	for (lane = 0; lane < sink_device_lane_count(sink); lane++)
> > +		sink->data.dpcd[DP_LANE0_1_STATUS + (lane >> 1)] |=
> > +			DP_LANE_CR_DONE << (4 * (lane & 1));
> > +
> > +	sink->data.cr_done = true;
> > +}
> > +
> > +static void
> > +sink_device_mark_channel_eq_done(struct sink_device *sink)
> > +{
> > +	int lane;
> > +
> > +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> > +		uint8_t mask = (DP_LANE_CHANNEL_EQ_DONE | DP_LANE_SYMBOL_LOCKED);
> > +		sink->data.dpcd[DP_LANE0_1_STATUS + (lane >> 1)] |=
> > +			mask << (4 * (lane & 1));
> > +	}
> > +
> > +	sink->data.dpcd[DP_LANE_ALIGN_STATUS_UPDATED] |= DP_INTERLANE_ALIGN_DONE;
> > +
> > +	sink->data.channel_eq_done = true;
> > +}
> > +
> > +static bool
> > +sink_device_get_link_status(struct sink_device *sink,
> > +			    uint8_t link_status[DP_LINK_STATUS_SIZE])
> > +{
> > +	if (!sink->data.cr_done) {
> > +		if (!sink_device_request_higher_voltage_swing(sink))
> > +			sink_device_mark_cr_done(sink);
> > +	} else if (!sink->data.channel_eq_done) {
> > +		if (!sink_device_request_higher_pre_emphasis(sink))
> > +			sink_device_mark_channel_eq_done(sink);
> > +	}
> > +
> > +	memcpy(link_status, sink->data.dpcd + DP_LANE0_1_STATUS,
> > +	       DP_LINK_STATUS_SIZE);
> > +
> > +	return true;
> > +}
> > +
> > +static void
> > +sink_device_reset(struct sink_device *sink, int lanes, uint8_t link_bw)
> > +{
> > +	memset(&sink->data, 0, sizeof sink->data);
> > +	sink->data.dpcd[DP_MAX_LINK_RATE] = link_bw;
> > +	sink->data.dpcd[DP_MAX_LANE_COUNT] = lanes;
> > +}
> > +
> > +static struct sink_device simple_sink = {
> > +	.get_link_status = sink_device_get_link_status,
> > +	.dpcd_write = sink_device_dpcd_write,
> > +};
> > +
> > +/* Glue code */
> > +
> > +struct test_intel_dp {
> > +	struct intel_dp dp;
> > +	struct sink_device *sink;
> > +	uint8_t link_bw;
> > +
> > +	uint8_t max_voltage;
> > +	uint8_t max_pre_emphasis;
> > +};
> > +
> > +static struct test_intel_dp *
> > +to_test_intel_dp(struct intel_dp *dp)
> > +{
> > +	return container_of(dp, struct test_intel_dp, dp);
> > +}
> > +
> > +void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
> > +{
> > +}
> > +
> > +bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp)
> > +{
> > +	return false;
> > +}
> > +
> > +bool
> > +intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
> > +{
> > +	struct sink_device *sink = to_test_intel_dp(intel_dp)->sink;
> > +	return sink->get_link_status(sink, link_status);
> > +}
> > +
> > +void
> > +intel_dp_update_signal_levels(struct intel_dp *intel_dp)
> > +{
> > +}
> > +
> > +void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
> > +			   uint8_t *link_bw, uint8_t *rate_select)
> > +{
> > +	*link_bw = to_test_intel_dp(intel_dp)->link_bw;
> > +	*rate_select = 0;
> > +}
> > +
> > +void
> > +intel_dp_program_link_training_pattern(struct intel_dp *intel_dp,
> > +				       uint8_t dp_train_pat)
> > +{
> > +}
> > +
> > +uint8_t
> > +intel_dp_voltage_max(struct intel_dp *intel_dp)
> > +{
> > +	return to_test_intel_dp(intel_dp)->max_voltage <<
> > +		DP_TRAIN_VOLTAGE_SWING_SHIFT;
> > +}
> > +
> > +uint8_t
> > +intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing)
> > +{
> > +	return to_test_intel_dp(intel_dp)->max_pre_emphasis <<
> > +		DP_TRAIN_PRE_EMPHASIS_SHIFT;
> > +}
> > +
> > +ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
> > +			  void *buffer, size_t size)
> > +{
> > +	struct intel_dp *intel_dp =
> > +		container_of(aux, struct intel_dp, aux);
> > +	struct sink_device *sink = to_test_intel_dp(intel_dp)->sink;
> > +
> > +	return sink->dpcd_write(sink, offset, buffer, size);
> > +}
> > +
> > +/* --- */
> > +
> > +static struct test_intel_dp test_dp;
> > +
> > +static void
> > +do_test(struct sink_device *sink, int lanes, uint8_t link_bw,
> > +	     uint8_t max_voltage, uint8_t max_pre_emphasis)
> > +{
> > +	int lane;
> > +
> > +	memset(&test_dp, 0, sizeof test_dp);
> > +	test_dp.dp.lane_count = lanes;
> > +	test_dp.link_bw = link_bw;
> > +	test_dp.sink = sink;
> > +	test_dp.max_voltage =
> > +		max_voltage >> DP_TRAIN_VOLTAGE_SWING_SHIFT;
> > +	test_dp.max_pre_emphasis =
> > +		max_pre_emphasis >> DP_TRAIN_PRE_EMPHASIS_SHIFT;
> > +
> > +	sink_device_reset(sink, lanes, link_bw);
> > +
> > +	intel_dp_start_link_train(&test_dp.dp);
> > +	intel_dp_stop_link_train(&test_dp.dp);
> > +
> > +	assert(sink->data.cr_done);
> > +	assert(sink->data.channel_eq_done);
> > +
> > +	for (lane = 0; lane < test_dp.dp.lane_count; lane++) {
> > +		uint8_t cur_v = sink_device_get_voltage_swing(sink, lane);
> > +		uint8_t cur_p = sink_device_get_pre_emphasis_level(sink, lane);
> > +
> > +		assert(cur_v == test_dp.max_voltage);
> > +		assert(cur_p == test_dp.max_pre_emphasis);
> > +	}
> > +}
> > +
> > +int test_lanes[] = {
> > +	1, 2, 4,
> > +};
> > +
> > +uint8_t test_bw[] = {
> > +	DP_LINK_BW_1_62,
> > +	DP_LINK_BW_2_7,
> > +};
> > +
> > +uint8_t test_max_voltage[] = {
> > +	DP_TRAIN_VOLTAGE_SWING_LEVEL_0,
> > +	DP_TRAIN_VOLTAGE_SWING_LEVEL_1,
> > +	DP_TRAIN_VOLTAGE_SWING_LEVEL_2,
> > +	DP_TRAIN_VOLTAGE_SWING_LEVEL_3,
> > +};
> > +
> > +uint8_t test_max_pre_emphasis[] = {
> > +	DP_TRAIN_PRE_EMPH_LEVEL_0,
> > +	DP_TRAIN_PRE_EMPH_LEVEL_1,
> > +	DP_TRAIN_PRE_EMPH_LEVEL_2,
> > +	DP_TRAIN_PRE_EMPH_LEVEL_3,
> > +};
> > +
> > +void
> > +run_test(void)
> > +{
> > +	int lane, bw, voltage, emph;
> > +
> > +	for (lane = 0; lane < ARRAY_SIZE(test_lanes); lane++)
> > +	for (bw = 0; bw < ARRAY_SIZE(test_bw); bw++)
> > +	for (voltage = 0; voltage < ARRAY_SIZE(test_max_voltage); voltage++)
> > +	for (emph = 0; emph < ARRAY_SIZE(test_max_pre_emphasis); emph++) {
> > +		DRM_DEBUG_KMS("l%d-bw%d-v%d-pe%d",
> > +			      test_lanes[lane],
> > +			      test_bw[bw],
> > +			      test_max_voltage[voltage],
> > +			      test_max_pre_emphasis[emph]);
> > +		do_test(&simple_sink,
> > +			test_lanes[lane],
> > +			test_bw[bw],
> > +			test_max_voltage[voltage],
> > +			test_max_pre_emphasis[emph]);
> > +	}
> > +}
> > -- 
> > 2.4.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Ander Conselvan de Oliveira Sept. 15, 2015, 1:08 p.m. UTC | #3

On Mon, 2015-09-14 at 16:38 +0300, Ander Conselvan De Oliveira wrote:
> On Mon, 2015-09-14 at 15:11 +0200, Daniel Vetter wrote:
> > On Mon, Sep 14, 2015 at 02:51:51PM +0300, Ander Conselvan de Oliveira wrote:
> > > ---
> > > 
> > > On Fri, 2015-09-11 at 17:11 +0300, Ander Conselvan de Oliveira wrote:
> > > > On Wed, 2015-09-09 at 11:33 +0100, Thomas Wood wrote:
> > > > > On 8 September 2015 at 13:28, Ander Conselvan de Oliveira
> > > > > <[e-mail]> wrote:
> > > > > > 
> > > > 
> > > > > > diff --git a/link-training-test/Makefile b/link-training-test/Makefile
> > > > > 
> > > > > If this is meant to be part of the test suite, then it needs to be in
> > > > > the tests directory and use the igt test infrastructure. Otherwise it
> > > > > should be placed in tools or tools/link-training-test.
> > > > 
> > > > I made the test use the igt infrastructure, but I'm not sure if this is
> > > > a good fit for it. The dependency on the kernel is on build time, but
> > > > once compiled this can be run on any machine. This can also introduce
> > > > build failures if the test is not kept in sync with the driver source.
> > > > Ideally that a failure to build this would be reported as the test
> > > > failing, but I have no idea of how to achieve that.
> > > 
> > > Alternatively, this could be in the kernel source tree directly. This
> > > patch adds a test subdir to the i915 source dir, containing the link
> > > training test. The test is compiled as part of the normal build using
> > > the extra-y variable so that it doesn't get linked to the final kernel.
> > > 
> > > When make is run from the tests directory, a thin wrapper around the
> > > tests is built and linked to the object file compiled as part of the
> > > kernel build. Running make run_tests from the test dir runs the test
> > > and reports success or failure.
> > > 
> > > Any thoughts?
> > 
> > I think there's some precedence in other subsystems to integrate unit
> > tests directly in the kernel, e.g. locking selftest or similar things.
> > Usual approach is to either have a special module (but that often means
> > piles of EXPORT_SYMBOL only for that selftest module). Or just a y/n
> > Kconfig option which enables that code and runs all the self/unit tests
> > when the module loads.
> > 
> > I'd go with that approach since it's simpler. And we'd only need to tell
> > QA to enable that Kconfig option for more testing.
> 
> I'll have a look into that Kconfig approach, but there's a couple of things
> I like about having the unit test as user space binaries:
> 
>   - there's no need to boot the newly compiled kernel, so doing a test run
>     is super fast;
>   - the binaries can be debugged with gdb just like other user space stuff.

I implemented the test using the Kconfig approach, and it seems to work well
without impacting the points above. I added the call to run the test as the
first thing in i915_init(), and with the driver compiled built-in, running
the kernel under qemu will run the tests. And qemu can also provide a gdb
remote target.

One thing might be a problem though. With the previous approach, the
functions overriden by the test where simply reimplemented in the new binary.
But now the test is linked to the entire driver, so that's not possible. To
work around that, I had to add function pointers to all the functions called
by the link training state machine to intel_dp. I don't think that method
scales well.

I'll send update patches for reference as replies to this mail.


Ander

> The downside is the stubbing necessary. In this particular case it wasn't
> that bad, but I guess it could get really ugly.
> 
> > Long-term we might go more fancy with some static debugfs interface (i.e.
> > in the top-level drm/ directory, no in the numbered device subdirs in
> > debugfs) to run specific testcases so that we can report out the results.
> > 
> > But as long as module reload is part of BAT we don't need that, at least
> > not for a start.
> > 
> > Anyway I think having the unit tests in the kernel makes more sense, the
> > igt ones will likely get out of sync fast. And igt is supposed to work
> > with any kernel version (hence all the igt_require checks).
> 
> Yeah, agreed.
> 
> Ander
> 
> > -Daniel
> > 
> > > 
> > > Ander
> > > 
> > > ---
> > >  drivers/gpu/drm/i915/Makefile                      |   2 +
> > >  drivers/gpu/drm/i915/tests/Makefile                |  39 ++
> > >  drivers/gpu/drm/i915/tests/empty.c                 |   0
> > >  drivers/gpu/drm/i915/tests/loader.c                | 108 +++++
> > >  .../drm/i915/tests/test_intel_dp_link_training.c   | 464 +++++++++++++++++++++
> > >  5 files changed, 613 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/i915/tests/Makefile
> > >  create mode 100644 drivers/gpu/drm/i915/tests/empty.c
> > >  create mode 100644 drivers/gpu/drm/i915/tests/loader.c
> > >  create mode 100644 drivers/gpu/drm/i915/tests/test_intel_dp_link_training.c
> > > 
> > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > > index 0851de07..7298ce5 100644
> > > --- a/drivers/gpu/drm/i915/Makefile
> > > +++ b/drivers/gpu/drm/i915/Makefile
> > > @@ -97,6 +97,8 @@ i915-y += i915_vgpu.o
> > >  # legacy horrors
> > >  i915-y += i915_dma.o
> > >  
> > > +obj-y += tests/
> > > +
> > >  obj-$(CONFIG_DRM_I915)  += i915.o
> > >  
> > >  CFLAGS_i915_trace_points.o := -I$(src)
> > > diff --git a/drivers/gpu/drm/i915/tests/Makefile b/drivers/gpu/drm/i915/tests/Makefile
> > > new file mode 100644
> > > index 0000000..3062741
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/tests/Makefile
> > > @@ -0,0 +1,39 @@
> > > +# Please kbuild
> > > +# TODO: check for a better way to include this directory in the build
> > > +# without forcing the need for this empty file
> > > +obj-y += empty.o
> > > +
> > > +
> > > +# FIXME: The stack protector code causes tests to crash due to an improperly
> > > +# initialized %gs, so keep it disabled for now.
> > > +ccflags-y := -fstack-protector-explicit
> > > +
> > > +extra-y += test_intel_dp_link_training.o
> > > +
> > > +# If make is run from this directory
> > > +ifeq (0, $(MAKELEVEL))
> > > +
> > > +# For strlcpy
> > > +LDFLAGS = -lbsd
> > > +
> > > +TEST_PROGS = \
> > > +	test_intel_dp_link_training
> > > +
> > > +all: tests
> > > +
> > > +tests: $(TEST_PROGS)
> > > +
> > > +run_tests: tests
> > > +	@for TEST in $(TEST_PROGS); do \
> > > +		(./$$TEST && echo "$$TEST [PASS]") || echo "$$TEST [FAIL]"; \
> > > +	done
> > > +
> > > +test_%.o: test_%.c ../%.c
> > > +	make -C ../../../../../ drivers/gpu/drm/i915/tests/$@
> > > +
> > > +loader.o: loader.c
> > > +	$(CC) $(CFLAGS) -c -o $@ $<
> > > +
> > > +%: %.o loader.o
> > > +	$(CC) -o $@ $^ $(LDFLAGS)
> > > +endif
> > > diff --git a/drivers/gpu/drm/i915/tests/empty.c b/drivers/gpu/drm/i915/tests/empty.c
> > > new file mode 100644
> > > index 0000000..e69de29
> > > diff --git a/drivers/gpu/drm/i915/tests/loader.c b/drivers/gpu/drm/i915/tests/loader.c
> > > new file mode 100644
> > > index 0000000..9e38816
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/tests/loader.c
> > > @@ -0,0 +1,108 @@
> > > +/*
> > > + * Copyright © 2015 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.
> > > + *
> > > + * Authors:
> > > + *    Ander Conselvan de Oliveira <[e-mail]>
> > > + *
> > > + */
> > > +
> > > +#include <stdbool.h>
> > > +#include <stdint.h>
> > > +#include <stdlib.h>
> > > +#include <stdio.h>
> > > +#include <stdarg.h>
> > > +
> > > +void run_test(void);
> > > +
> > > +void assert(bool condition)
> > > +{
> > > +	if (!condition) {
> > > +		printf("Assert failed\n");
> > > +		abort();
> > > +	}
> > > +}
> > > +
> > > +struct {
> > > +	uint8_t padding[32];
> > > +} param_ops_int;
> > > +
> > > +unsigned int drm_debug = 0;	/* 1 to enable debug output */
> > > +
> > > +void drm_err(const char *format, ...)
> > > +{
> > > +	va_list args;
> > > +
> > > +	va_start(args, format);
> > > +	vprintf(format, args);
> > > +	va_end(args);
> > > +
> > > +}
> > > +
> > > +void drm_ut_debug_printk(const char *function_name, const char *format, ...)
> > > +{
> > > +	va_list args;
> > > +
> > > +	va_start(args, format);
> > > +	printf("[%s] ", function_name);
> > > +	vprintf(format, args);
> > > +	va_end(args);
> > > +}
> > > +
> > > +void __const_udelay(unsigned long xloops)
> > > +{
> > > +}
> > > +
> > > +struct mutex;
> > > +void mutex_lock_nested(struct mutex *lock, unsigned int subclass)
> > > +{
> > > +}
> > > +
> > > +void mutex_unlock(struct mutex *lock)
> > > +{
> > > +}
> > > +
> > > +struct lock_class_key;
> > > +void
> > > +__mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key)
> > > +{
> > > +}
> > > +
> > > +void usleep_range(unsigned int min, unsigned int max)
> > > +{
> > > +}
> > > +
> > > +struct i2c_adapter;
> > > +int i2c_add_adapter(struct i2c_adapter *adapter)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +void i2c_del_adapter(struct i2c_adapter *adapater)
> > > +{
> > > +}
> > > +
> > > +int
> > > +main(int argc, char *argv[])
> > > +{
> > > +	run_test();
> > > +	return 0;
> > > +}
> > > diff --git a/drivers/gpu/drm/i915/tests/test_intel_dp_link_training.c 
> > > b/drivers/gpu/drm/i915/tests/test_intel_dp_link_training.c
> > > new file mode 100644
> > > index 0000000..11868f8
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/tests/test_intel_dp_link_training.c
> > > @@ -0,0 +1,464 @@
> > > +/*
> > > + * Copyright © 2015 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.
> > > + *
> > > + * Authors:
> > > + *    Ander Conselvan de Oliveira <[e-mail]>
> > > + *
> > > + */
> > > +
> > > +#include "../intel_dp_link_training.c"
> > > +
> > > +#define drm_dp_dpcd_write	real_drm_dp_dpcd_write
> > > +#include "../../drm_dp_helper.c"
> > > +#undef drm_dp_dpcd_write
> > > +
> > > +void assert(bool condition);
> > > +
> > > +
> > > +struct sink_device {
> > > +	ssize_t (*dpcd_write)(struct sink_device *sink, unsigned int offset,
> > > +			      void *buffer, size_t size);
> > > +	bool (*get_link_status)(struct sink_device *sink,
> > > +				uint8_t link_status[DP_LINK_STATUS_SIZE]);
> > > +
> > > +	struct {
> > > +		bool lane_count_and_bw_set;
> > > +		bool training_pattern_1_set;
> > > +		bool started_with_non_zero_levels;
> > > +		bool cr_done;
> > > +		bool channel_eq_done;
> > > +
> > > +		uint8_t dpcd[0x3000];
> > > +	} data;
> > > +};
> > > +
> > > +/* Fake sink device implementation */
> > > +
> > > +static uint8_t
> > > +sink_device_lane_count(struct sink_device *sink)
> > > +{
> > > +	return sink->data.dpcd[DP_LANE_COUNT_SET];
> > > +}
> > > +
> > > +static uint8_t
> > > +sink_device_get_training_pattern(struct sink_device *sink)
> > > +{
> > > +	return sink->data.dpcd[DP_TRAINING_PATTERN_SET] & DP_TRAINING_PATTERN_MASK;
> > > +}
> > > +
> > > +static uint8_t
> > > +sink_device_get_voltage_swing(struct sink_device *sink, int lane)
> > > +{
> > > +	return sink->data.dpcd[DP_TRAINING_LANE0_SET + lane] &
> > > +		DP_TRAIN_VOLTAGE_SWING_MASK;
> > > +}
> > > +
> > > +static uint8_t
> > > +sink_device_get_pre_emphasis_level(struct sink_device *sink, int lane)
> > > +{
> > > +	return (sink->data.dpcd[DP_TRAINING_LANE0_SET + lane] &
> > > +		 DP_TRAIN_PRE_EMPHASIS_MASK) >> DP_TRAIN_PRE_EMPHASIS_SHIFT;
> > > +}
> > > +
> > > +static void
> > > +sink_device_check_lane_count_and_bw(struct sink_device *sink)
> > > +{
> > > +	if (sink->data.lane_count_and_bw_set)
> > > +		return;
> > > +
> > > +	assert(sink->data.dpcd[DP_TRAINING_PATTERN_SET] == 0);
> > > +
> > > +	if (sink->data.dpcd[DP_LINK_BW_SET] != 0 &&
> > > +	    sink->data.dpcd[DP_LANE_COUNT_SET] != 0)
> > > +		sink->data.lane_count_and_bw_set = true;
> > > +}
> > > +
> > > +static void
> > > +sink_device_check_pattern_1_set(struct sink_device *sink)
> > > +{
> > > +	int lane;
> > > +
> > > +	if (!sink->data.lane_count_and_bw_set ||
> > > +	    sink->data.training_pattern_1_set)
> > > +		return;
> > > +
> > > +	assert(sink_device_get_training_pattern(sink) <= DP_TRAINING_PATTERN_1);
> > > +
> > > +	if (sink_device_get_training_pattern(sink) != DP_TRAINING_PATTERN_1)
> > > +		return;
> > > +
> > > +	assert(sink->data.dpcd[DP_LINK_BW_SET] == DP_LINK_BW_1_62 ||
> > > +	       sink->data.dpcd[DP_LINK_BW_SET] == DP_LINK_BW_2_7);
> > > +
> > > +	assert(sink->data.dpcd[DP_LANE_COUNT_SET] == 1 ||
> > > +		   sink->data.dpcd[DP_LANE_COUNT_SET] == 2 ||
> > > +		   sink->data.dpcd[DP_LANE_COUNT_SET] == 4);
> > > +
> > > +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> > > +		if (sink_device_get_voltage_swing(sink, lane) != 
> > > DP_TRAIN_VOLTAGE_SWING_LEVEL_0 
> > > > > 
> > > +		    sink_device_get_pre_emphasis_level(sink, lane) != 
> > > DP_TRAIN_PRE_EMPH_LEVEL_0)
> > > +			sink->data.started_with_non_zero_levels = true;
> > > +	}
> > > +
> > > +	sink->data.training_pattern_1_set = true;
> > > +}
> > > +
> > > +static void
> > > +sink_device_check_pattern_2_set(struct sink_device *sink)
> > > +{
> > > +	if (!sink->data.cr_done)
> > > +		return;
> > > +
> > > +	assert(sink_device_get_training_pattern(sink) == DP_TRAINING_PATTERN_2);
> > > +}
> > > +
> > > +static void
> > > +sink_device_check_pattern_disable(struct sink_device *sink)
> > > +{
> > > +	if (!sink->data.cr_done || ! sink->data.channel_eq_done)
> > > +		return;
> > > +
> > > +	assert(sink_device_get_training_pattern(sink) == DP_TRAINING_PATTERN_DISABLE);
> > > +}
> > > +
> > > +static ssize_t
> > > +sink_device_dpcd_write(struct sink_device *sink, unsigned int offset,
> > > +		       void *buffer, size_t size)
> > > +{
> > > +	memcpy(sink->data.dpcd + offset, buffer, size);
> > > +
> > > +	sink_device_check_lane_count_and_bw(sink);
> > > +
> > > +	if (!sink->data.cr_done)
> > > +		sink_device_check_pattern_1_set(sink);
> > > +	else if (!sink->data.channel_eq_done)
> > > +		sink_device_check_pattern_2_set(sink);
> > > +	else
> > > +		sink_device_check_pattern_disable(sink);
> > > +
> > > +	return size;
> > > +}
> > > +
> > > +static bool
> > > +sink_device_max_voltage_reached(struct sink_device *sink, int lane)
> > > +{
> > > +	return (sink->data.dpcd[DP_TRAINING_LANE0_SET + lane] & DP_TRAIN_MAX_SWING_REACHED) 
> > > ==
> > > +		DP_TRAIN_MAX_SWING_REACHED;
> > > +}
> > > +
> > > +static bool
> > > +sink_device_max_pre_emphasis_reached(struct sink_device *sink, int lane)
> > > +{
> > > +	return (sink->data.dpcd[DP_TRAINING_LANE0_SET + lane] & 
> > > DP_TRAIN_MAX_PRE_EMPHASIS_REACHED) ==
> > > +		DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
> > > +}
> > > +
> > > +static void
> > > +sink_device_set_adjust_voltage(struct sink_device *sink,
> > > +			       int lane, uint8_t level)
> > > +{
> > > +	int shift = DP_ADJUST_VOLTAGE_SWING_LANE1_SHIFT * (lane & 1);
> > > +
> > > +	sink->data.dpcd[DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1)] &=
> > > +		~(DP_ADJUST_VOLTAGE_SWING_LANE0_MASK << shift);
> > > +	sink->data.dpcd[DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1)] |=
> > > +		level << shift;
> > > +}
> > > +
> > > +static void
> > > +sink_device_set_adjust_pre_emphasis(struct sink_device *sink,
> > > +				    int lane, uint8_t level)
> > > +{
> > > +	int shift = DP_ADJUST_VOLTAGE_SWING_LANE1_SHIFT * (lane & 1);
> > > +
> > > +	sink->data.dpcd[DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1)] &=
> > > +		~(DP_ADJUST_PRE_EMPHASIS_LANE0_MASK << shift);
> > > +	sink->data.dpcd[DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1)] |=
> > > +		level << (DP_ADJUST_PRE_EMPHASIS_LANE0_SHIFT + shift);
> > > +}
> > > +
> > > +static bool
> > > +sink_device_request_higher_voltage_swing(struct sink_device *sink)
> > > +{
> > > +	bool max_reached = false;
> > > +	int lane;
> > > +
> > > +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> > > +		if (sink_device_max_voltage_reached(sink, lane)) {
> > > +			max_reached = true;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	if (max_reached)
> > > +		return false;
> > > +
> > > +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> > > +		uint8_t new_voltage =
> > > +			sink_device_get_voltage_swing(sink, lane) + 1;
> > > +
> > > +		sink_device_set_adjust_voltage(sink, lane, new_voltage);
> > > +	}
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +static bool
> > > +sink_device_request_higher_pre_emphasis(struct sink_device *sink)
> > > +{
> > > +	bool max_reached = false;
> > > +	int lane;
> > > +
> > > +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> > > +		if (sink_device_max_pre_emphasis_reached(sink, lane)) {
> > > +			max_reached = true;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	if (max_reached)
> > > +		return false;
> > > +
> > > +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> > > +		uint8_t new_pre_emphasis =
> > > +			sink_device_get_pre_emphasis_level(sink, lane) + 1;
> > > +
> > > +		sink_device_set_adjust_pre_emphasis(sink, lane, new_pre_emphasis);
> > > +	}
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +static void
> > > +sink_device_mark_cr_done(struct sink_device *sink)
> > > +{
> > > +	int lane;
> > > +
> > > +	for (lane = 0; lane < sink_device_lane_count(sink); lane++)
> > > +		sink->data.dpcd[DP_LANE0_1_STATUS + (lane >> 1)] |=
> > > +			DP_LANE_CR_DONE << (4 * (lane & 1));
> > > +
> > > +	sink->data.cr_done = true;
> > > +}
> > > +
> > > +static void
> > > +sink_device_mark_channel_eq_done(struct sink_device *sink)
> > > +{
> > > +	int lane;
> > > +
> > > +	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
> > > +		uint8_t mask = (DP_LANE_CHANNEL_EQ_DONE | DP_LANE_SYMBOL_LOCKED);
> > > +		sink->data.dpcd[DP_LANE0_1_STATUS + (lane >> 1)] |=
> > > +			mask << (4 * (lane & 1));
> > > +	}
> > > +
> > > +	sink->data.dpcd[DP_LANE_ALIGN_STATUS_UPDATED] |= DP_INTERLANE_ALIGN_DONE;
> > > +
> > > +	sink->data.channel_eq_done = true;
> > > +}
> > > +
> > > +static bool
> > > +sink_device_get_link_status(struct sink_device *sink,
> > > +			    uint8_t link_status[DP_LINK_STATUS_SIZE])
> > > +{
> > > +	if (!sink->data.cr_done) {
> > > +		if (!sink_device_request_higher_voltage_swing(sink))
> > > +			sink_device_mark_cr_done(sink);
> > > +	} else if (!sink->data.channel_eq_done) {
> > > +		if (!sink_device_request_higher_pre_emphasis(sink))
> > > +			sink_device_mark_channel_eq_done(sink);
> > > +	}
> > > +
> > > +	memcpy(link_status, sink->data.dpcd + DP_LANE0_1_STATUS,
> > > +	       DP_LINK_STATUS_SIZE);
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +static void
> > > +sink_device_reset(struct sink_device *sink, int lanes, uint8_t link_bw)
> > > +{
> > > +	memset(&sink->data, 0, sizeof sink->data);
> > > +	sink->data.dpcd[DP_MAX_LINK_RATE] = link_bw;
> > > +	sink->data.dpcd[DP_MAX_LANE_COUNT] = lanes;
> > > +}
> > > +
> > > +static struct sink_device simple_sink = {
> > > +	.get_link_status = sink_device_get_link_status,
> > > +	.dpcd_write = sink_device_dpcd_write,
> > > +};
> > > +
> > > +/* Glue code */
> > > +
> > > +struct test_intel_dp {
> > > +	struct intel_dp dp;
> > > +	struct sink_device *sink;
> > > +	uint8_t link_bw;
> > > +
> > > +	uint8_t max_voltage;
> > > +	uint8_t max_pre_emphasis;
> > > +};
> > > +
> > > +static struct test_intel_dp *
> > > +to_test_intel_dp(struct intel_dp *dp)
> > > +{
> > > +	return container_of(dp, struct test_intel_dp, dp);
> > > +}
> > > +
> > > +void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
> > > +{
> > > +}
> > > +
> > > +bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp)
> > > +{
> > > +	return false;
> > > +}
> > > +
> > > +bool
> > > +intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
> > > +{
> > > +	struct sink_device *sink = to_test_intel_dp(intel_dp)->sink;
> > > +	return sink->get_link_status(sink, link_status);
> > > +}
> > > +
> > > +void
> > > +intel_dp_update_signal_levels(struct intel_dp *intel_dp)
> > > +{
> > > +}
> > > +
> > > +void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
> > > +			   uint8_t *link_bw, uint8_t *rate_select)
> > > +{
> > > +	*link_bw = to_test_intel_dp(intel_dp)->link_bw;
> > > +	*rate_select = 0;
> > > +}
> > > +
> > > +void
> > > +intel_dp_program_link_training_pattern(struct intel_dp *intel_dp,
> > > +				       uint8_t dp_train_pat)
> > > +{
> > > +}
> > > +
> > > +uint8_t
> > > +intel_dp_voltage_max(struct intel_dp *intel_dp)
> > > +{
> > > +	return to_test_intel_dp(intel_dp)->max_voltage <<
> > > +		DP_TRAIN_VOLTAGE_SWING_SHIFT;
> > > +}
> > > +
> > > +uint8_t
> > > +intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing)
> > > +{
> > > +	return to_test_intel_dp(intel_dp)->max_pre_emphasis <<
> > > +		DP_TRAIN_PRE_EMPHASIS_SHIFT;
> > > +}
> > > +
> > > +ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
> > > +			  void *buffer, size_t size)
> > > +{
> > > +	struct intel_dp *intel_dp =
> > > +		container_of(aux, struct intel_dp, aux);
> > > +	struct sink_device *sink = to_test_intel_dp(intel_dp)->sink;
> > > +
> > > +	return sink->dpcd_write(sink, offset, buffer, size);
> > > +}
> > > +
> > > +/* --- */
> > > +
> > > +static struct test_intel_dp test_dp;
> > > +
> > > +static void
> > > +do_test(struct sink_device *sink, int lanes, uint8_t link_bw,
> > > +	     uint8_t max_voltage, uint8_t max_pre_emphasis)
> > > +{
> > > +	int lane;
> > > +
> > > +	memset(&test_dp, 0, sizeof test_dp);
> > > +	test_dp.dp.lane_count = lanes;
> > > +	test_dp.link_bw = link_bw;
> > > +	test_dp.sink = sink;
> > > +	test_dp.max_voltage =
> > > +		max_voltage >> DP_TRAIN_VOLTAGE_SWING_SHIFT;
> > > +	test_dp.max_pre_emphasis =
> > > +		max_pre_emphasis >> DP_TRAIN_PRE_EMPHASIS_SHIFT;
> > > +
> > > +	sink_device_reset(sink, lanes, link_bw);
> > > +
> > > +	intel_dp_start_link_train(&test_dp.dp);
> > > +	intel_dp_stop_link_train(&test_dp.dp);
> > > +
> > > +	assert(sink->data.cr_done);
> > > +	assert(sink->data.channel_eq_done);
> > > +
> > > +	for (lane = 0; lane < test_dp.dp.lane_count; lane++) {
> > > +		uint8_t cur_v = sink_device_get_voltage_swing(sink, lane);
> > > +		uint8_t cur_p = sink_device_get_pre_emphasis_level(sink, lane);
> > > +
> > > +		assert(cur_v == test_dp.max_voltage);
> > > +		assert(cur_p == test_dp.max_pre_emphasis);
> > > +	}
> > > +}
> > > +
> > > +int test_lanes[] = {
> > > +	1, 2, 4,
> > > +};
> > > +
> > > +uint8_t test_bw[] = {
> > > +	DP_LINK_BW_1_62,
> > > +	DP_LINK_BW_2_7,
> > > +};
> > > +
> > > +uint8_t test_max_voltage[] = {
> > > +	DP_TRAIN_VOLTAGE_SWING_LEVEL_0,
> > > +	DP_TRAIN_VOLTAGE_SWING_LEVEL_1,
> > > +	DP_TRAIN_VOLTAGE_SWING_LEVEL_2,
> > > +	DP_TRAIN_VOLTAGE_SWING_LEVEL_3,
> > > +};
> > > +
> > > +uint8_t test_max_pre_emphasis[] = {
> > > +	DP_TRAIN_PRE_EMPH_LEVEL_0,
> > > +	DP_TRAIN_PRE_EMPH_LEVEL_1,
> > > +	DP_TRAIN_PRE_EMPH_LEVEL_2,
> > > +	DP_TRAIN_PRE_EMPH_LEVEL_3,
> > > +};
> > > +
> > > +void
> > > +run_test(void)
> > > +{
> > > +	int lane, bw, voltage, emph;
> > > +
> > > +	for (lane = 0; lane < ARRAY_SIZE(test_lanes); lane++)
> > > +	for (bw = 0; bw < ARRAY_SIZE(test_bw); bw++)
> > > +	for (voltage = 0; voltage < ARRAY_SIZE(test_max_voltage); voltage++)
> > > +	for (emph = 0; emph < ARRAY_SIZE(test_max_pre_emphasis); emph++) {
> > > +		DRM_DEBUG_KMS("l%d-bw%d-v%d-pe%d",
> > > +			      test_lanes[lane],
> > > +			      test_bw[bw],
> > > +			      test_max_voltage[voltage],
> > > +			      test_max_pre_emphasis[emph]);
> > > +		do_test(&simple_sink,
> > > +			test_lanes[lane],
> > > +			test_bw[bw],
> > > +			test_max_voltage[voltage],
> > > +			test_max_pre_emphasis[emph]);
> > > +	}
> > > +}

Daniel Vetter Sept. 23, 2015, 8:24 a.m. UTC | #4

On Tue, Sep 15, 2015 at 04:08:53PM +0300, Ander Conselvan De Oliveira wrote:
> On Mon, 2015-09-14 at 16:38 +0300, Ander Conselvan De Oliveira wrote:
> > On Mon, 2015-09-14 at 15:11 +0200, Daniel Vetter wrote:
> > > On Mon, Sep 14, 2015 at 02:51:51PM +0300, Ander Conselvan de Oliveira wrote:
> > > > ---
> > > > 
> > > > On Fri, 2015-09-11 at 17:11 +0300, Ander Conselvan de Oliveira wrote:
> > > > > On Wed, 2015-09-09 at 11:33 +0100, Thomas Wood wrote:
> > > > > > On 8 September 2015 at 13:28, Ander Conselvan de Oliveira
> > > > > > <[e-mail]> wrote:
> > > > > > > 
> > > > > 
> > > > > > > diff --git a/link-training-test/Makefile b/link-training-test/Makefile
> > > > > > 
> > > > > > If this is meant to be part of the test suite, then it needs to be in
> > > > > > the tests directory and use the igt test infrastructure. Otherwise it
> > > > > > should be placed in tools or tools/link-training-test.
> > > > > 
> > > > > I made the test use the igt infrastructure, but I'm not sure if this is
> > > > > a good fit for it. The dependency on the kernel is on build time, but
> > > > > once compiled this can be run on any machine. This can also introduce
> > > > > build failures if the test is not kept in sync with the driver source.
> > > > > Ideally that a failure to build this would be reported as the test
> > > > > failing, but I have no idea of how to achieve that.
> > > > 
> > > > Alternatively, this could be in the kernel source tree directly. This
> > > > patch adds a test subdir to the i915 source dir, containing the link
> > > > training test. The test is compiled as part of the normal build using
> > > > the extra-y variable so that it doesn't get linked to the final kernel.
> > > > 
> > > > When make is run from the tests directory, a thin wrapper around the
> > > > tests is built and linked to the object file compiled as part of the
> > > > kernel build. Running make run_tests from the test dir runs the test
> > > > and reports success or failure.
> > > > 
> > > > Any thoughts?
> > > 
> > > I think there's some precedence in other subsystems to integrate unit
> > > tests directly in the kernel, e.g. locking selftest or similar things.
> > > Usual approach is to either have a special module (but that often means
> > > piles of EXPORT_SYMBOL only for that selftest module). Or just a y/n
> > > Kconfig option which enables that code and runs all the self/unit tests
> > > when the module loads.
> > > 
> > > I'd go with that approach since it's simpler. And we'd only need to tell
> > > QA to enable that Kconfig option for more testing.
> > 
> > I'll have a look into that Kconfig approach, but there's a couple of things
> > I like about having the unit test as user space binaries:
> > 
> >   - there's no need to boot the newly compiled kernel, so doing a test run
> >     is super fast;
> >   - the binaries can be debugged with gdb just like other user space stuff.
> 
> I implemented the test using the Kconfig approach, and it seems to work well
> without impacting the points above. I added the call to run the test as the
> first thing in i915_init(), and with the driver compiled built-in, running
> the kernel under qemu will run the tests. And qemu can also provide a gdb
> remote target.
> 
> One thing might be a problem though. With the previous approach, the
> functions overriden by the test where simply reimplemented in the new binary.
> But now the test is linked to the entire driver, so that's not possible. To
> work around that, I had to add function pointers to all the functions called
> by the link training state machine to intel_dp. I don't think that method
> scales well.
> 
> I'll send update patches for reference as replies to this mail.

I had a few discussions about this at XDC and I know think doing this is
userspace is better:
- Faster to run tests (since no module reloading required).
- Nouveau is developed in userspace and would like to reuse shared link
  training code too if possible from the dp helpers.

So I'm leaning towards scaffolding in userspace now. Might be good to
check out how the nouveau userspace runtime works just to steal a few
tricks.

Also I think longer-term we should move the link-training code into the
shared dp helpers, but that's another step later on.
-Daniel

Ander Conselvan de Oliveira Sept. 23, 2015, 9:18 a.m. UTC | #5

On Wed, 2015-09-23 at 10:24 +0200, Daniel Vetter wrote:
> On Tue, Sep 15, 2015 at 04:08:53PM +0300, Ander Conselvan De Oliveira wrote:
> > On Mon, 2015-09-14 at 16:38 +0300, Ander Conselvan De Oliveira wrote:
> > > On Mon, 2015-09-14 at 15:11 +0200, Daniel Vetter wrote:
> > > > On Mon, Sep 14, 2015 at 02:51:51PM +0300, Ander Conselvan de Oliveira wrote:
> > > > > ---
> > > > > 
> > > > > On Fri, 2015-09-11 at 17:11 +0300, Ander Conselvan de Oliveira wrote:
> > > > > > On Wed, 2015-09-09 at 11:33 +0100, Thomas Wood wrote:
> > > > > > > On 8 September 2015 at 13:28, Ander Conselvan de Oliveira
> > > > > > > <[e-mail]> wrote:
> > > > > > > > 
> > > > > > 
> > > > > > > > diff --git a/link-training-test/Makefile b/link-training-test/Makefile
> > > > > > > 
> > > > > > > If this is meant to be part of the test suite, then it needs to be in
> > > > > > > the tests directory and use the igt test infrastructure. Otherwise it
> > > > > > > should be placed in tools or tools/link-training-test.
> > > > > > 
> > > > > > I made the test use the igt infrastructure, but I'm not sure if this is
> > > > > > a good fit for it. The dependency on the kernel is on build time, but
> > > > > > once compiled this can be run on any machine. This can also introduce
> > > > > > build failures if the test is not kept in sync with the driver source.
> > > > > > Ideally that a failure to build this would be reported as the test
> > > > > > failing, but I have no idea of how to achieve that.
> > > > > 
> > > > > Alternatively, this could be in the kernel source tree directly. This
> > > > > patch adds a test subdir to the i915 source dir, containing the link
> > > > > training test. The test is compiled as part of the normal build using
> > > > > the extra-y variable so that it doesn't get linked to the final kernel.
> > > > > 
> > > > > When make is run from the tests directory, a thin wrapper around the
> > > > > tests is built and linked to the object file compiled as part of the
> > > > > kernel build. Running make run_tests from the test dir runs the test
> > > > > and reports success or failure.
> > > > > 
> > > > > Any thoughts?
> > > > 
> > > > I think there's some precedence in other subsystems to integrate unit
> > > > tests directly in the kernel, e.g. locking selftest or similar things.
> > > > Usual approach is to either have a special module (but that often means
> > > > piles of EXPORT_SYMBOL only for that selftest module). Or just a y/n
> > > > Kconfig option which enables that code and runs all the self/unit tests
> > > > when the module loads.
> > > > 
> > > > I'd go with that approach since it's simpler. And we'd only need to tell
> > > > QA to enable that Kconfig option for more testing.
> > > 
> > > I'll have a look into that Kconfig approach, but there's a couple of things
> > > I like about having the unit test as user space binaries:
> > > 
> > >   - there's no need to boot the newly compiled kernel, so doing a test run
> > >     is super fast;
> > >   - the binaries can be debugged with gdb just like other user space stuff.
> > 
> > I implemented the test using the Kconfig approach, and it seems to work well
> > without impacting the points above. I added the call to run the test as the
> > first thing in i915_init(), and with the driver compiled built-in, running
> > the kernel under qemu will run the tests. And qemu can also provide a gdb
> > remote target.
> > 
> > One thing might be a problem though. With the previous approach, the
> > functions overriden by the test where simply reimplemented in the new binary.
> > But now the test is linked to the entire driver, so that's not possible. To
> > work around that, I had to add function pointers to all the functions called
> > by the link training state machine to intel_dp. I don't think that method
> > scales well.
> > 
> > I'll send update patches for reference as replies to this mail.
> 
> I had a few discussions about this at XDC and I know think doing this is
> userspace is better:
> - Faster to run tests (since no module reloading required).
> - Nouveau is developed in userspace and would like to reuse shared link
>   training code too if possible from the dp helpers.
> 
> So I'm leaning towards scaffolding in userspace now. Might be good to
> check out how the nouveau userspace runtime works just to steal a few
> tricks.

I chatted with Martin Peres about this and had a look at the code. There are a few interesting
tricks in Nouveau indeed. They have user space implementation for some kernel internals such as
mutexes, ioremap on top of libpciaccess, etc. and an extensive set of stubs.

The one thing that wouldn't be easy to take advantage of is the build integration. Their upstream
repository is not actually a kernel tree. It contains their drm code, the same that ends up in the
kernel plus all the user space stuff. There's a script that converts the commits from that
repository for inclusion in the kernel tree.

Ander


> Also I think longer-term we should move the link-training code into the
> shared dp helpers, but that's another step later on.
> -Daniel

Daniel Vetter Sept. 23, 2015, 9:38 a.m. UTC | #6

On Wed, Sep 23, 2015 at 11:18 AM, Ander Conselvan De Oliveira <[e-mail]> wrote:
> On Wed, 2015-09-23 at 10:24 +0200, Daniel Vetter wrote:
>> On Tue, Sep 15, 2015 at 04:08:53PM +0300, Ander Conselvan De Oliveira wrote:
>> > On Mon, 2015-09-14 at 16:38 +0300, Ander Conselvan De Oliveira wrote:
>> > > On Mon, 2015-09-14 at 15:11 +0200, Daniel Vetter wrote:
>> > > > On Mon, Sep 14, 2015 at 02:51:51PM +0300, Ander Conselvan de Oliveira wrote:
>> > > > > ---
>> > > > >
>> > > > > On Fri, 2015-09-11 at 17:11 +0300, Ander Conselvan de Oliveira wrote:
>> > > > > > On Wed, 2015-09-09 at 11:33 +0100, Thomas Wood wrote:
>> > > > > > > On 8 September 2015 at 13:28, Ander Conselvan de Oliveira
>> > > > > > > <[e-mail]> wrote:
>> > > > > > > >
>> > > > > >
>> > > > > > > > diff --git a/link-training-test/Makefile b/link-training-test/Makefile
>> > > > > > >
>> > > > > > > If this is meant to be part of the test suite, then it needs to be in
>> > > > > > > the tests directory and use the igt test infrastructure. Otherwise it
>> > > > > > > should be placed in tools or tools/link-training-test.
>> > > > > >
>> > > > > > I made the test use the igt infrastructure, but I'm not sure if this is
>> > > > > > a good fit for it. The dependency on the kernel is on build time, but
>> > > > > > once compiled this can be run on any machine. This can also introduce
>> > > > > > build failures if the test is not kept in sync with the driver source.
>> > > > > > Ideally that a failure to build this would be reported as the test
>> > > > > > failing, but I have no idea of how to achieve that.
>> > > > >
>> > > > > Alternatively, this could be in the kernel source tree directly. This
>> > > > > patch adds a test subdir to the i915 source dir, containing the link
>> > > > > training test. The test is compiled as part of the normal build using
>> > > > > the extra-y variable so that it doesn't get linked to the final kernel.
>> > > > >
>> > > > > When make is run from the tests directory, a thin wrapper around the
>> > > > > tests is built and linked to the object file compiled as part of the
>> > > > > kernel build. Running make run_tests from the test dir runs the test
>> > > > > and reports success or failure.
>> > > > >
>> > > > > Any thoughts?
>> > > >
>> > > > I think there's some precedence in other subsystems to integrate unit
>> > > > tests directly in the kernel, e.g. locking selftest or similar things.
>> > > > Usual approach is to either have a special module (but that often means
>> > > > piles of EXPORT_SYMBOL only for that selftest module). Or just a y/n
>> > > > Kconfig option which enables that code and runs all the self/unit tests
>> > > > when the module loads.
>> > > >
>> > > > I'd go with that approach since it's simpler. And we'd only need to tell
>> > > > QA to enable that Kconfig option for more testing.
>> > >
>> > > I'll have a look into that Kconfig approach, but there's a couple of things
>> > > I like about having the unit test as user space binaries:
>> > >
>> > >   - there's no need to boot the newly compiled kernel, so doing a test run
>> > >     is super fast;
>> > >   - the binaries can be debugged with gdb just like other user space stuff.
>> >
>> > I implemented the test using the Kconfig approach, and it seems to work well
>> > without impacting the points above. I added the call to run the test as the
>> > first thing in i915_init(), and with the driver compiled built-in, running
>> > the kernel under qemu will run the tests. And qemu can also provide a gdb
>> > remote target.
>> >
>> > One thing might be a problem though. With the previous approach, the
>> > functions overriden by the test where simply reimplemented in the new binary.
>> > But now the test is linked to the entire driver, so that's not possible. To
>> > work around that, I had to add function pointers to all the functions called
>> > by the link training state machine to intel_dp. I don't think that method
>> > scales well.
>> >
>> > I'll send update patches for reference as replies to this mail.
>>
>> I had a few discussions about this at XDC and I know think doing this is
>> userspace is better:
>> - Faster to run tests (since no module reloading required).
>> - Nouveau is developed in userspace and would like to reuse shared link
>>   training code too if possible from the dp helpers.
>>
>> So I'm leaning towards scaffolding in userspace now. Might be good to
>> check out how the nouveau userspace runtime works just to steal a few
>> tricks.
>
> I chatted with Martin Peres about this and had a look at the code. There are a few interesting
> tricks in Nouveau indeed. They have user space implementation for some kernel internals such as
> mutexes, ioremap on top of libpciaccess, etc. and an extensive set of stubs.
>
> The one thing that wouldn't be easy to take advantage of is the build integration. Their upstream
> repository is not actually a kernel tree. It contains their drm code, the same that ends up in the
> kernel plus all the user space stuff. There's a script that converts the commits from that
> repository for inclusion in the kernel tree.

Could we perhaps move at least some of the scafffolding for kernel
functions to upstream? I guess nouveau has some means to pull changes from
upstream too, so maybe we could push that some place nice. And I'm pretty
sure we're not the only ones thinking about unit-testing specific
kernelcode in a userspace environment.

Adding noveau and kselftest folks.
-Daniel

Ander Conselvan de Oliveira Sept. 29, 2015, 8:47 a.m. UTC | #7

On Wed, 2015-09-23 at 11:38 +0200, Daniel Vetter wrote:
> On Wed, Sep 23, 2015 at 11:18 AM, Ander Conselvan De Oliveira <[e-mail]> wrote:
> > On Wed, 2015-09-23 at 10:24 +0200, Daniel Vetter wrote:
> > > On Tue, Sep 15, 2015 at 04:08:53PM +0300, Ander Conselvan De Oliveira wrote:
> > > > On Mon, 2015-09-14 at 16:38 +0300, Ander Conselvan De Oliveira wrote:
> > > > > On Mon, 2015-09-14 at 15:11 +0200, Daniel Vetter wrote:
> > > > > > On Mon, Sep 14, 2015 at 02:51:51PM +0300, Ander Conselvan de Oliveira wrote:
> > > > > > > ---
> > > > > > > 
> > > > > > > On Fri, 2015-09-11 at 17:11 +0300, Ander Conselvan de Oliveira wrote:
> > > > > > > > On Wed, 2015-09-09 at 11:33 +0100, Thomas Wood wrote:
> > > > > > > > > On 8 September 2015 at 13:28, Ander Conselvan de Oliveira
> > > > > > > > > <[e-mail]> wrote:
> > > > > > > > > > 
> > > > > > > > 
> > > > > > > > > > diff --git a/link-training-test/Makefile b/link-training-test/Makefile
> > > > > > > > > 
> > > > > > > > > If this is meant to be part of the test suite, then it needs to be in
> > > > > > > > > the tests directory and use the igt test infrastructure. Otherwise it
> > > > > > > > > should be placed in tools or tools/link-training-test.
> > > > > > > > 
> > > > > > > > I made the test use the igt infrastructure, but I'm not sure if this is
> > > > > > > > a good fit for it. The dependency on the kernel is on build time, but
> > > > > > > > once compiled this can be run on any machine. This can also introduce
> > > > > > > > build failures if the test is not kept in sync with the driver source.
> > > > > > > > Ideally that a failure to build this would be reported as the test
> > > > > > > > failing, but I have no idea of how to achieve that.
> > > > > > > 
> > > > > > > Alternatively, this could be in the kernel source tree directly. This
> > > > > > > patch adds a test subdir to the i915 source dir, containing the link
> > > > > > > training test. The test is compiled as part of the normal build using
> > > > > > > the extra-y variable so that it doesn't get linked to the final kernel.
> > > > > > > 
> > > > > > > When make is run from the tests directory, a thin wrapper around the
> > > > > > > tests is built and linked to the object file compiled as part of the
> > > > > > > kernel build. Running make run_tests from the test dir runs the test
> > > > > > > and reports success or failure.
> > > > > > > 
> > > > > > > Any thoughts?
> > > > > > 
> > > > > > I think there's some precedence in other subsystems to integrate unit
> > > > > > tests directly in the kernel, e.g. locking selftest or similar things.
> > > > > > Usual approach is to either have a special module (but that often means
> > > > > > piles of EXPORT_SYMBOL only for that selftest module). Or just a y/n
> > > > > > Kconfig option which enables that code and runs all the self/unit tests
> > > > > > when the module loads.
> > > > > > 
> > > > > > I'd go with that approach since it's simpler. And we'd only need to tell
> > > > > > QA to enable that Kconfig option for more testing.
> > > > > 
> > > > > I'll have a look into that Kconfig approach, but there's a couple of things
> > > > > I like about having the unit test as user space binaries:
> > > > > 
> > > > >   - there's no need to boot the newly compiled kernel, so doing a test run
> > > > >     is super fast;
> > > > >   - the binaries can be debugged with gdb just like other user space stuff.
> > > > 
> > > > I implemented the test using the Kconfig approach, and it seems to work well
> > > > without impacting the points above. I added the call to run the test as the
> > > > first thing in i915_init(), and with the driver compiled built-in, running
> > > > the kernel under qemu will run the tests. And qemu can also provide a gdb
> > > > remote target.
> > > > 
> > > > One thing might be a problem though. With the previous approach, the
> > > > functions overriden by the test where simply reimplemented in the new binary.
> > > > But now the test is linked to the entire driver, so that's not possible. To
> > > > work around that, I had to add function pointers to all the functions called
> > > > by the link training state machine to intel_dp. I don't think that method
> > > > scales well.
> > > > 
> > > > I'll send update patches for reference as replies to this mail.
> > > 
> > > I had a few discussions about this at XDC and I know think doing this is
> > > userspace is better:
> > > - Faster to run tests (since no module reloading required).
> > > - Nouveau is developed in userspace and would like to reuse shared link
> > >   training code too if possible from the dp helpers.
> > > 
> > > So I'm leaning towards scaffolding in userspace now. Might be good to
> > > check out how the nouveau userspace runtime works just to steal a few
> > > tricks.
> > 
> > I chatted with Martin Peres about this and had a look at the code. There are a few interesting
> > tricks in Nouveau indeed. They have user space implementation for some kernel internals such as
> > mutexes, ioremap on top of libpciaccess, etc. and an extensive set of stubs.
> > 
> > The one thing that wouldn't be easy to take advantage of is the build integration. Their 
> > upstream
> > repository is not actually a kernel tree. It contains their drm code, the same that ends up in 
> > the
> > kernel plus all the user space stuff. There's a script that converts the commits from that
> > repository for inclusion in the kernel tree.
> 
> Could we perhaps move at least some of the scafffolding for kernel
> functions to upstream? I guess nouveau has some means to pull changes from
> upstream too, so maybe we could push that some place nice. And I'm pretty
> sure we're not the only ones thinking about unit-testing specific
> kernelcode in a userspace environment.

Maybe this would add a burden on nouveau development without any added benefit? Since we need only a
small subset of what they already have, perhaps it would make more sense to first replicate that
small part for our consumption. I believe what need to be re-implemented and/or stubbed will vary
significantly with the code being tested, so it might make more sense to just let it grow
organically.

I think the more important question is how to include the right code when compiling the user space
binaries. The trick nouveau uses is to have only one header that includes code from the kernel. That
header is replaced with a version intended for user space if compiling user space binaries. We would
have to use the same trick for every subsystem under test, including some include path mangling. At
least drm and i915 would be affected for the tests I'm proposing.

Ander

Patch

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 0851de07..7298ce5 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -97,6 +97,8 @@  i915-y += i915_vgpu.o
 # legacy horrors
 i915-y += i915_dma.o
 
+obj-y += tests/
+
 obj-$(CONFIG_DRM_I915)  += i915.o
 
 CFLAGS_i915_trace_points.o := -I$(src)
diff --git a/drivers/gpu/drm/i915/tests/Makefile b/drivers/gpu/drm/i915/tests/Makefile
new file mode 100644
index 0000000..3062741
--- /dev/null
+++ b/drivers/gpu/drm/i915/tests/Makefile
@@ -0,0 +1,39 @@ 
+# Please kbuild
+# TODO: check for a better way to include this directory in the build
+# without forcing the need for this empty file
+obj-y += empty.o
+
+
+# FIXME: The stack protector code causes tests to crash due to an improperly
+# initialized %gs, so keep it disabled for now.
+ccflags-y := -fstack-protector-explicit
+
+extra-y += test_intel_dp_link_training.o
+
+# If make is run from this directory
+ifeq (0, $(MAKELEVEL))
+
+# For strlcpy
+LDFLAGS = -lbsd
+
+TEST_PROGS = \
+	test_intel_dp_link_training
+
+all: tests
+
+tests: $(TEST_PROGS)
+
+run_tests: tests
+	@for TEST in $(TEST_PROGS); do \
+		(./$$TEST && echo "$$TEST [PASS]") || echo "$$TEST [FAIL]"; \
+	done
+
+test_%.o: test_%.c ../%.c
+	make -C ../../../../../ drivers/gpu/drm/i915/tests/$@
+
+loader.o: loader.c
+	$(CC) $(CFLAGS) -c -o $@ $<
+
+%: %.o loader.o
+	$(CC) -o $@ $^ $(LDFLAGS)
+endif
diff --git a/drivers/gpu/drm/i915/tests/empty.c b/drivers/gpu/drm/i915/tests/empty.c
new file mode 100644
index 0000000..e69de29
diff --git a/drivers/gpu/drm/i915/tests/loader.c b/drivers/gpu/drm/i915/tests/loader.c
new file mode 100644
index 0000000..9e38816
--- /dev/null
+++ b/drivers/gpu/drm/i915/tests/loader.c
@@ -0,0 +1,108 @@ 
+/*
+ * Copyright © 2015 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.
+ *
+ * Authors:
+ *    Ander Conselvan de Oliveira <[e-mail]>
+ *
+ */
+
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <stdarg.h>
+
+void run_test(void);
+
+void assert(bool condition)
+{
+	if (!condition) {
+		printf("Assert failed\n");
+		abort();
+	}
+}
+
+struct {
+	uint8_t padding[32];
+} param_ops_int;
+
+unsigned int drm_debug = 0;	/* 1 to enable debug output */
+
+void drm_err(const char *format, ...)
+{
+	va_list args;
+
+	va_start(args, format);
+	vprintf(format, args);
+	va_end(args);
+
+}
+
+void drm_ut_debug_printk(const char *function_name, const char *format, ...)
+{
+	va_list args;
+
+	va_start(args, format);
+	printf("[%s] ", function_name);
+	vprintf(format, args);
+	va_end(args);
+}
+
+void __const_udelay(unsigned long xloops)
+{
+}
+
+struct mutex;
+void mutex_lock_nested(struct mutex *lock, unsigned int subclass)
+{
+}
+
+void mutex_unlock(struct mutex *lock)
+{
+}
+
+struct lock_class_key;
+void
+__mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key)
+{
+}
+
+void usleep_range(unsigned int min, unsigned int max)
+{
+}
+
+struct i2c_adapter;
+int i2c_add_adapter(struct i2c_adapter *adapter)
+{
+	return 0;
+}
+
+void i2c_del_adapter(struct i2c_adapter *adapater)
+{
+}
+
+int
+main(int argc, char *argv[])
+{
+	run_test();
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/tests/test_intel_dp_link_training.c b/drivers/gpu/drm/i915/tests/test_intel_dp_link_training.c
new file mode 100644
index 0000000..11868f8
--- /dev/null
+++ b/drivers/gpu/drm/i915/tests/test_intel_dp_link_training.c
@@ -0,0 +1,464 @@ 
+/*
+ * Copyright © 2015 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.
+ *
+ * Authors:
+ *    Ander Conselvan de Oliveira <[e-mail]>
+ *
+ */
+
+#include "../intel_dp_link_training.c"
+
+#define drm_dp_dpcd_write	real_drm_dp_dpcd_write
+#include "../../drm_dp_helper.c"
+#undef drm_dp_dpcd_write
+
+void assert(bool condition);
+
+
+struct sink_device {
+	ssize_t (*dpcd_write)(struct sink_device *sink, unsigned int offset,
+			      void *buffer, size_t size);
+	bool (*get_link_status)(struct sink_device *sink,
+				uint8_t link_status[DP_LINK_STATUS_SIZE]);
+
+	struct {
+		bool lane_count_and_bw_set;
+		bool training_pattern_1_set;
+		bool started_with_non_zero_levels;
+		bool cr_done;
+		bool channel_eq_done;
+
+		uint8_t dpcd[0x3000];
+	} data;
+};
+
+/* Fake sink device implementation */
+
+static uint8_t
+sink_device_lane_count(struct sink_device *sink)
+{
+	return sink->data.dpcd[DP_LANE_COUNT_SET];
+}
+
+static uint8_t
+sink_device_get_training_pattern(struct sink_device *sink)
+{
+	return sink->data.dpcd[DP_TRAINING_PATTERN_SET] & DP_TRAINING_PATTERN_MASK;
+}
+
+static uint8_t
+sink_device_get_voltage_swing(struct sink_device *sink, int lane)
+{
+	return sink->data.dpcd[DP_TRAINING_LANE0_SET + lane] &
+		DP_TRAIN_VOLTAGE_SWING_MASK;
+}
+
+static uint8_t
+sink_device_get_pre_emphasis_level(struct sink_device *sink, int lane)
+{
+	return (sink->data.dpcd[DP_TRAINING_LANE0_SET + lane] &
+		 DP_TRAIN_PRE_EMPHASIS_MASK) >> DP_TRAIN_PRE_EMPHASIS_SHIFT;
+}
+
+static void
+sink_device_check_lane_count_and_bw(struct sink_device *sink)
+{
+	if (sink->data.lane_count_and_bw_set)
+		return;
+
+	assert(sink->data.dpcd[DP_TRAINING_PATTERN_SET] == 0);
+
+	if (sink->data.dpcd[DP_LINK_BW_SET] != 0 &&
+	    sink->data.dpcd[DP_LANE_COUNT_SET] != 0)
+		sink->data.lane_count_and_bw_set = true;
+}
+
+static void
+sink_device_check_pattern_1_set(struct sink_device *sink)
+{
+	int lane;
+
+	if (!sink->data.lane_count_and_bw_set ||
+	    sink->data.training_pattern_1_set)
+		return;
+
+	assert(sink_device_get_training_pattern(sink) <= DP_TRAINING_PATTERN_1);
+
+	if (sink_device_get_training_pattern(sink) != DP_TRAINING_PATTERN_1)
+		return;
+
+	assert(sink->data.dpcd[DP_LINK_BW_SET] == DP_LINK_BW_1_62 ||
+	       sink->data.dpcd[DP_LINK_BW_SET] == DP_LINK_BW_2_7);
+
+	assert(sink->data.dpcd[DP_LANE_COUNT_SET] == 1 ||
+		   sink->data.dpcd[DP_LANE_COUNT_SET] == 2 ||
+		   sink->data.dpcd[DP_LANE_COUNT_SET] == 4);
+
+	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
+		if (sink_device_get_voltage_swing(sink, lane) != DP_TRAIN_VOLTAGE_SWING_LEVEL_0 ||
+		    sink_device_get_pre_emphasis_level(sink, lane) != DP_TRAIN_PRE_EMPH_LEVEL_0)
+			sink->data.started_with_non_zero_levels = true;
+	}
+
+	sink->data.training_pattern_1_set = true;
+}
+
+static void
+sink_device_check_pattern_2_set(struct sink_device *sink)
+{
+	if (!sink->data.cr_done)
+		return;
+
+	assert(sink_device_get_training_pattern(sink) == DP_TRAINING_PATTERN_2);
+}
+
+static void
+sink_device_check_pattern_disable(struct sink_device *sink)
+{
+	if (!sink->data.cr_done || ! sink->data.channel_eq_done)
+		return;
+
+	assert(sink_device_get_training_pattern(sink) == DP_TRAINING_PATTERN_DISABLE);
+}
+
+static ssize_t
+sink_device_dpcd_write(struct sink_device *sink, unsigned int offset,
+		       void *buffer, size_t size)
+{
+	memcpy(sink->data.dpcd + offset, buffer, size);
+
+	sink_device_check_lane_count_and_bw(sink);
+
+	if (!sink->data.cr_done)
+		sink_device_check_pattern_1_set(sink);
+	else if (!sink->data.channel_eq_done)
+		sink_device_check_pattern_2_set(sink);
+	else
+		sink_device_check_pattern_disable(sink);
+
+	return size;
+}
+
+static bool
+sink_device_max_voltage_reached(struct sink_device *sink, int lane)
+{
+	return (sink->data.dpcd[DP_TRAINING_LANE0_SET + lane] & DP_TRAIN_MAX_SWING_REACHED) ==
+		DP_TRAIN_MAX_SWING_REACHED;
+}
+
+static bool
+sink_device_max_pre_emphasis_reached(struct sink_device *sink, int lane)
+{
+	return (sink->data.dpcd[DP_TRAINING_LANE0_SET + lane] & DP_TRAIN_MAX_PRE_EMPHASIS_REACHED) ==
+		DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
+}
+
+static void
+sink_device_set_adjust_voltage(struct sink_device *sink,
+			       int lane, uint8_t level)
+{
+	int shift = DP_ADJUST_VOLTAGE_SWING_LANE1_SHIFT * (lane & 1);
+
+	sink->data.dpcd[DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1)] &=
+		~(DP_ADJUST_VOLTAGE_SWING_LANE0_MASK << shift);
+	sink->data.dpcd[DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1)] |=
+		level << shift;
+}
+
+static void
+sink_device_set_adjust_pre_emphasis(struct sink_device *sink,
+				    int lane, uint8_t level)
+{
+	int shift = DP_ADJUST_VOLTAGE_SWING_LANE1_SHIFT * (lane & 1);
+
+	sink->data.dpcd[DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1)] &=
+		~(DP_ADJUST_PRE_EMPHASIS_LANE0_MASK << shift);
+	sink->data.dpcd[DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1)] |=
+		level << (DP_ADJUST_PRE_EMPHASIS_LANE0_SHIFT + shift);
+}
+
+static bool
+sink_device_request_higher_voltage_swing(struct sink_device *sink)
+{
+	bool max_reached = false;
+	int lane;
+
+	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
+		if (sink_device_max_voltage_reached(sink, lane)) {
+			max_reached = true;
+			break;
+		}
+	}
+
+	if (max_reached)
+		return false;
+
+	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
+		uint8_t new_voltage =
+			sink_device_get_voltage_swing(sink, lane) + 1;
+
+		sink_device_set_adjust_voltage(sink, lane, new_voltage);
+	}
+
+	return true;
+}
+
+static bool
+sink_device_request_higher_pre_emphasis(struct sink_device *sink)
+{
+	bool max_reached = false;
+	int lane;
+
+	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
+		if (sink_device_max_pre_emphasis_reached(sink, lane)) {
+			max_reached = true;
+			break;
+		}
+	}
+
+	if (max_reached)
+		return false;
+
+	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
+		uint8_t new_pre_emphasis =
+			sink_device_get_pre_emphasis_level(sink, lane) + 1;
+
+		sink_device_set_adjust_pre_emphasis(sink, lane, new_pre_emphasis);
+	}
+
+	return true;
+}
+
+static void
+sink_device_mark_cr_done(struct sink_device *sink)
+{
+	int lane;
+
+	for (lane = 0; lane < sink_device_lane_count(sink); lane++)
+		sink->data.dpcd[DP_LANE0_1_STATUS + (lane >> 1)] |=
+			DP_LANE_CR_DONE << (4 * (lane & 1));
+
+	sink->data.cr_done = true;
+}
+
+static void
+sink_device_mark_channel_eq_done(struct sink_device *sink)
+{
+	int lane;
+
+	for (lane = 0; lane < sink_device_lane_count(sink); lane++) {
+		uint8_t mask = (DP_LANE_CHANNEL_EQ_DONE | DP_LANE_SYMBOL_LOCKED);
+		sink->data.dpcd[DP_LANE0_1_STATUS + (lane >> 1)] |=
+			mask << (4 * (lane & 1));
+	}
+
+	sink->data.dpcd[DP_LANE_ALIGN_STATUS_UPDATED] |= DP_INTERLANE_ALIGN_DONE;
+
+	sink->data.channel_eq_done = true;
+}
+
+static bool
+sink_device_get_link_status(struct sink_device *sink,
+			    uint8_t link_status[DP_LINK_STATUS_SIZE])
+{
+	if (!sink->data.cr_done) {
+		if (!sink_device_request_higher_voltage_swing(sink))
+			sink_device_mark_cr_done(sink);
+	} else if (!sink->data.channel_eq_done) {
+		if (!sink_device_request_higher_pre_emphasis(sink))
+			sink_device_mark_channel_eq_done(sink);
+	}
+
+	memcpy(link_status, sink->data.dpcd + DP_LANE0_1_STATUS,
+	       DP_LINK_STATUS_SIZE);
+
+	return true;
+}
+
+static void
+sink_device_reset(struct sink_device *sink, int lanes, uint8_t link_bw)
+{
+	memset(&sink->data, 0, sizeof sink->data);
+	sink->data.dpcd[DP_MAX_LINK_RATE] = link_bw;
+	sink->data.dpcd[DP_MAX_LANE_COUNT] = lanes;
+}
+
+static struct sink_device simple_sink = {
+	.get_link_status = sink_device_get_link_status,
+	.dpcd_write = sink_device_dpcd_write,
+};
+
+/* Glue code */
+
+struct test_intel_dp {
+	struct intel_dp dp;
+	struct sink_device *sink;
+	uint8_t link_bw;
+
+	uint8_t max_voltage;
+	uint8_t max_pre_emphasis;
+};
+
+static struct test_intel_dp *
+to_test_intel_dp(struct intel_dp *dp)
+{
+	return container_of(dp, struct test_intel_dp, dp);
+}
+
+void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
+{
+}
+
+bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp)
+{
+	return false;
+}
+
+bool
+intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
+{
+	struct sink_device *sink = to_test_intel_dp(intel_dp)->sink;
+	return sink->get_link_status(sink, link_status);
+}
+
+void
+intel_dp_update_signal_levels(struct intel_dp *intel_dp)
+{
+}
+
+void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
+			   uint8_t *link_bw, uint8_t *rate_select)
+{
+	*link_bw = to_test_intel_dp(intel_dp)->link_bw;
+	*rate_select = 0;
+}
+
+void
+intel_dp_program_link_training_pattern(struct intel_dp *intel_dp,
+				       uint8_t dp_train_pat)
+{
+}
+
+uint8_t
+intel_dp_voltage_max(struct intel_dp *intel_dp)
+{
+	return to_test_intel_dp(intel_dp)->max_voltage <<
+		DP_TRAIN_VOLTAGE_SWING_SHIFT;
+}
+
+uint8_t
+intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing)
+{
+	return to_test_intel_dp(intel_dp)->max_pre_emphasis <<
+		DP_TRAIN_PRE_EMPHASIS_SHIFT;
+}
+
+ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
+			  void *buffer, size_t size)
+{
+	struct intel_dp *intel_dp =
+		container_of(aux, struct intel_dp, aux);
+	struct sink_device *sink = to_test_intel_dp(intel_dp)->sink;
+
+	return sink->dpcd_write(sink, offset, buffer, size);
+}
+
+/* --- */
+
+static struct test_intel_dp test_dp;
+
+static void
+do_test(struct sink_device *sink, int lanes, uint8_t link_bw,
+	     uint8_t max_voltage, uint8_t max_pre_emphasis)
+{
+	int lane;
+
+	memset(&test_dp, 0, sizeof test_dp);
+	test_dp.dp.lane_count = lanes;
+	test_dp.link_bw = link_bw;
+	test_dp.sink = sink;
+	test_dp.max_voltage =
+		max_voltage >> DP_TRAIN_VOLTAGE_SWING_SHIFT;
+	test_dp.max_pre_emphasis =
+		max_pre_emphasis >> DP_TRAIN_PRE_EMPHASIS_SHIFT;
+
+	sink_device_reset(sink, lanes, link_bw);
+
+	intel_dp_start_link_train(&test_dp.dp);
+	intel_dp_stop_link_train(&test_dp.dp);
+
+	assert(sink->data.cr_done);
+	assert(sink->data.channel_eq_done);
+
+	for (lane = 0; lane < test_dp.dp.lane_count; lane++) {
+		uint8_t cur_v = sink_device_get_voltage_swing(sink, lane);
+		uint8_t cur_p = sink_device_get_pre_emphasis_level(sink, lane);
+
+		assert(cur_v == test_dp.max_voltage);
+		assert(cur_p == test_dp.max_pre_emphasis);
+	}
+}
+
+int test_lanes[] = {
+	1, 2, 4,
+};
+
+uint8_t test_bw[] = {
+	DP_LINK_BW_1_62,
+	DP_LINK_BW_2_7,
+};
+
+uint8_t test_max_voltage[] = {
+	DP_TRAIN_VOLTAGE_SWING_LEVEL_0,
+	DP_TRAIN_VOLTAGE_SWING_LEVEL_1,
+	DP_TRAIN_VOLTAGE_SWING_LEVEL_2,
+	DP_TRAIN_VOLTAGE_SWING_LEVEL_3,
+};
+
+uint8_t test_max_pre_emphasis[] = {
+	DP_TRAIN_PRE_EMPH_LEVEL_0,
+	DP_TRAIN_PRE_EMPH_LEVEL_1,
+	DP_TRAIN_PRE_EMPH_LEVEL_2,
+	DP_TRAIN_PRE_EMPH_LEVEL_3,
+};
+
+void
+run_test(void)
+{
+	int lane, bw, voltage, emph;
+
+	for (lane = 0; lane < ARRAY_SIZE(test_lanes); lane++)
+	for (bw = 0; bw < ARRAY_SIZE(test_bw); bw++)
+	for (voltage = 0; voltage < ARRAY_SIZE(test_max_voltage); voltage++)
+	for (emph = 0; emph < ARRAY_SIZE(test_max_pre_emphasis); emph++) {
+		DRM_DEBUG_KMS("l%d-bw%d-v%d-pe%d",
+			      test_lanes[lane],
+			      test_bw[bw],
+			      test_max_voltage[voltage],
+			      test_max_pre_emphasis[emph]);
+		do_test(&simple_sink,
+			test_lanes[lane],
+			test_bw[bw],
+			test_max_voltage[voltage],
+			test_max_pre_emphasis[emph]);
+	}
+}