[PATCH 0/4] Add Automation Support for DP Compliance (Rev 5)

[PATCH 0/4] Add Automation Support for DP Compliance (Rev 5)

[Manasi Navare]

This series addresses review comments from the previous series:
https://patchwork.freedesktop.org/series/18150/

Apart from addressing review comments, it also adds a fix for
CRC mismatch errors in Compliance tests 4.4.1.1 - 4.4.1.3

DP 1.2 compliance testing can be acheived using DPR-120's CTS suite.
This compliance unit sends a short pulse to initiate link training
and video pattern generation compliance tests and sends a long pulse
to initate EDID compliance tests. It also sets the AUTOMATED TEST REQUEST
bit in Device IRQ to 1. These patches add support in the kernel to respond
to these test requests sent by DPR-120. The test handler has been added for
Link training/EDID and Video pattern compliance tests that gets invoked on
short/long pulse sent by DPR-120. The test handler for each of the tests
reads the corresponding DPCD registers to read the test parameters.

These tests need to be run with the IGT DP compliance automation tool.
Link Training Tests (4.3.1.1 - 4.3.2.3):
It reads the DPCD registers to get the link rate and lane count requested by
test and sends a hotplug uevent for the userspace to redo a modeset in order
to train the link at the requested test parameters.

EDID Tests (4.2.2.3 - 4.2.2.6):
It reads the EDID set by the DPR-120, if EDID read succeeds, it sets the
video mode to PREFERRED and sets the test_active flag. This flag wakes up
the IGT compliance tool that then fills the framebuffers and triggers a
modeset.

Video Pattern Tests (4.3.3.1):
It reads the DPCD registers to set the requested video pattern parameters.
It then sets the test active flag that wakes up the IGT tool, userspace
reads the video pattern values from corresponding debugfs files and fills the
framebuffers and triggers a modeset.
If the requested video pattern is 18bpp, then we force the dither to be off
so that CRC for the RGB pixels of the rendered video pattern matches the expected
CRC.

Manasi Navare (4):
  drm/i915: Add support for DP link training compliance
  drm/i915: Fixes to support DP Compliance EDID tests
  drm: Add definitions for DP compliance Video pattern tests
  drm/i915: Add support for DP Video pattern compliance tests

 drivers/gpu/drm/i915/i915_debugfs.c  |  14 +++-
 drivers/gpu/drm/i915/intel_display.c |   8 ++-
 drivers/gpu/drm/i915/intel_dp.c      | 130 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_dp_mst.c  |   7 +-
 drivers/gpu/drm/i915/intel_drv.h     |  13 ++++
 include/drm/drm_dp_helper.h          |  64 +++++++++++++++++
 6 files changed, 225 insertions(+), 11 deletions(-)

-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/4] drm/i915: Add support for DP link training compliance

[Manasi Navare]

This patch adds support to handle automated DP compliance
link training test requests. This patch has been tested with
Unigraf DPR-120 DP Compliance device for testing Link
Training Compliance.
After we get a short pulse Compliance test request, test
request values are read and hotplug uevent is sent in order
to trigger another modeset during which the pipe is configured
and link is retrained and enabled for link parameters requested
by the test.

v3:
* Validate the test link rate and lane count as soon as
the erquest comes (Jani Nikula)
v2:
* Validate the test lane count before using it in
intel_dp_compute_config (Jani Nikula)
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 60 ++++++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e80d620..3e76b63 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1613,6 +1613,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 	/* Conveniently, the link BW constants become indices with a shift...*/
 	int min_clock = 0;
 	int max_clock;
+	int link_rate_index;
 	int bpp, mode_rate;
 	int link_avail, link_clock;
 	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
@@ -1654,6 +1655,15 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
 		return false;
 
+	/* Use values requested by Compliance Test Request */
+	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
+		link_rate_index = intel_dp_link_rate_index(intel_dp,
+							   common_rates,
+							   drm_dp_bw_code_to_link_rate(intel_dp->compliance.test_link_rate));
+		if (link_rate_index >= 0)
+			min_clock = max_clock = link_rate_index;
+		min_lane_count = max_lane_count = intel_dp->compliance.test_lane_count;
+	}
 	DRM_DEBUG_KMS("DP link computation with max lane count %i "
 		      "max bw %d pixel clock %iKHz\n",
 		      max_lane_count, common_rates[max_clock],
@@ -3920,7 +3930,42 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 
 static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
 {
-	uint8_t test_result = DP_TEST_ACK;
+	uint8_t test_result = DP_TEST_NAK;
+	int status = 0;
+	int min_lane_count = 1;
+	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
+	int link_rate_index;
+	/* (DP CTS 1.2)
+	 * 4.3.1.11
+	 */
+	/* Read the TEST_LANE_COUNT and TEST_LINK_RTAE fields (DP CTS 3.1.4) */
+	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LANE_COUNT,
+				  &intel_dp->compliance.test_lane_count);
+
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Lane count read failed\n");
+		return 0;
+	}
+	intel_dp->compliance.test_lane_count &= DP_MAX_LANE_COUNT_MASK;
+	/* Validate the requested lane count */
+	if (intel_dp->compliance.test_lane_count < min_lane_count ||
+	    intel_dp->compliance.test_lane_count > intel_dp->max_sink_lane_count)
+		return test_result;
+
+	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LINK_RATE,
+				   &intel_dp->compliance.test_link_rate);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Link Rate read failed\n");
+		return 0;
+	}
+	/* Validate the requested link rate */
+	link_rate_index = intel_dp_link_rate_index(intel_dp,
+						   common_rates,
+						   drm_dp_bw_code_to_link_rate(intel_dp->compliance.test_link_rate));
+	if (link_rate_index < 0)
+		return test_result;
+
+	test_result = DP_TEST_ACK;
 	return test_result;
 }
 
@@ -4135,9 +4180,8 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 	if (!intel_dp->lane_count)
 		return;
 
-	/* if link training is requested we should perform it always */
-	if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) ||
-	    (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
+	/* Retrain if Channel EQ or CR not ok */
+	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
 		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
 			      intel_encoder->base.name);
 
@@ -4162,6 +4206,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 intel_dp_short_pulse(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	u8 sink_irq_vector = 0;
 	u8 old_sink_count = intel_dp->sink_count;
 	bool ret;
@@ -4195,7 +4240,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 				   sink_irq_vector);
 
 		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
-			DRM_DEBUG_DRIVER("Test request in short pulse not handled\n");
+			intel_dp_handle_test_request(intel_dp);
 		if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
@@ -4203,6 +4248,11 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 	intel_dp_check_link_status(intel_dp);
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
+	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
+		DRM_DEBUG_KMS("Link Training Compliance Test requested\n");
+		/* Send a Hotplug Uevent to userspace to start modeset */
+		drm_kms_helper_hotplug_event(intel_encoder->base.dev);
+	}
 
 	return true;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0cec001..1586a02 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -894,6 +894,8 @@ struct intel_dp_compliance {
 	unsigned long test_type;
 	struct intel_dp_compliance_data test_data;
 	bool test_active;
+	u8 test_link_rate;
+	u8 test_lane_count;
 };
 
 struct intel_dp {
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915: Fixes to support DP Compliance EDID tests

[Manasi Navare]

This patch addresses a few issues from the original patch for
DP Compliance EDID test support submitted by
Todd Previte<todd.previte@gmail.com>

Video Mode requested in the EDID test handler for the EDID Read
test (CTS 4.2.2.3) should be set to PREFERRED as per the CTS spec.

v2:
* Added read debugfs data from test_data.edid if its EDID test (Jani NIkula)
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 5 ++++-
 drivers/gpu/drm/i915/intel_dp.c     | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index fa69d72..8ec7edf 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3772,7 +3772,10 @@ static int i915_displayport_test_data_show(struct seq_file *m, void *data)
 		if (connector->status == connector_status_connected &&
 		    connector->encoder != NULL) {
 			intel_dp = enc_to_intel_dp(connector->encoder);
-			seq_printf(m, "%lx", intel_dp->compliance.test_data.edid);
+			if (intel_dp->compliance.test_type ==
+			    DP_TEST_LINK_EDID_READ)
+				seq_printf(m, "%lx",
+					   intel_dp->compliance.test_data.edid);
 		} else
 			seq_puts(m, "0");
 	}
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3e76b63..7ca45e0 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3977,7 +3977,7 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 
 static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
 {
-	uint8_t test_result = DP_TEST_NAK;
+	uint8_t test_result = DP_TEST_ACK;
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	struct drm_connector *connector = &intel_connector->base;
 
@@ -4012,7 +4012,7 @@ static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
 			DRM_DEBUG_KMS("Failed to write EDID checksum\n");
 
 		test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
-		intel_dp->compliance.test_data.edid = INTEL_DP_RESOLUTION_STANDARD;
+		intel_dp->compliance.test_data.edid = INTEL_DP_RESOLUTION_PREFERRED;
 	}
 
 	/* Set test active flag here so userspace doesn't interrupt things */
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm: Add definitions for DP compliance Video pattern tests

[Manasi Navare]

v3:
* Fix the conventions in bit definitions (Jani Nikula)
v2:
* Add all the other DP Complianec TEST register defs (Jani Nikula)
Cc: dri-devel@lists.freedesktop.org
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 include/drm/drm_dp_helper.h | 64 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 0468135..7ab4153 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -417,6 +417,70 @@
 #define DP_TEST_LANE_COUNT		    0x220
 
 #define DP_TEST_PATTERN			    0x221
+# define DP_NO_TEST_PATTERN                 0x0
+# define DP_COLOR_RAMP                      0x1
+# define DP_BLACK_AND_WHITE_VERTICAL_LINES  0x2
+# define DP_COLOR_SQUARE                    0x3
+
+#define DP_TEST_H_TOTAL_HI                  0x222
+#define DP_TEST_H_TOTAl_LO                  0x223
+
+#define DP_TEST_V_TOTAL_HI                  0x224
+#define DP_TEST_V_TOTAl_LO                  0x225
+
+#define DP_TEST_H_START_HI                  0x226
+#define DP_TEST_H_START_LO                  0x227
+
+#define DP_TEST_V_START_HI                  0x228
+#define DP_TEST_V_START_LO                  0x229
+
+#define DP_TEST_HSYNC_HI                    0x22A
+# define DP_TEST_HSYNC_POLARITY             (1 << 7)
+# define DP_TEST_HSYNC_WIDTH_HI_MASK        (127 << 0)
+#define DP_TEST_HSYNC_WIDTH_LO              0x22B
+
+#define DP_TEST_VSYNC_HI                    0x22C
+# define DP_TEST_VSYNC_POLARITY             (1 << 7)
+# define DP_TEST_VSYNC_WIDTH_HI_MASK        (127 << 0)
+#define DP_TEST_VSYNC_WIDTH_LO              0x22D
+
+#define DP_TEST_H_WIDTH_HI                  0x22E
+#define DP_TEST_H_WIDTH_LO                  0x22F
+
+#define DP_TEST_V_HEIGHT_HI                 0x230
+#define DP_TEST_V_HEIGHT_LO                 0x231
+
+#define DP_TEST_MISC0                       0x232
+# define DP_TEST_SYNC_CLOCK                 (1 << 0)
+# define DP_CLOCK_ASYNC                     0x0
+# define DP_CLOCK_SYNC                      0x1
+# define DP_TEST_COLOR_FORMAT_MASK          (3 << 1)
+# define DP_TEST_COLOR_FORMAT_SHIFT         1
+# define DP_COLOR_FORMAT_RGB                (0 << 1)
+# define DP_COLOR_FORMAT_YCbCr422           (1 << 1)
+# define DP_COLOR_FORMAT_YCbCr444           (2 << 1)
+# define DP_TEST_DYNAMIC_RANGE              (1 << 3)
+# define DP_VESA_RANGE                      (0 << 3)
+# define DP_CEA_RANGE                       (1 << 3)
+# define DP_TEST_YCBCR_COEFFICIENTS         (1 << 4)
+# define DP_YCBCR_COEFFICIENTS_ITU601       (0 << 4)
+# define DP_YCBCR_COEFFICIENTS_ITU709       (1 << 4)
+# define DP_TEST_BIT_DEPTH_MASK             (7 << 5)
+# define DP_TEST_BIT_DEPTH_SHIFT            5
+# define DP_TEST_BIT_DEPTH_6                (0 << 5)
+# define DP_TEST_BIT_DEPTH_8                (1 << 5)
+# define DP_TEST_BIT_DEPTH_10               (2 << 5)
+# define DP_TEST_BIT_DEPTH_12               (3 << 5)
+# define DP_TEST_BIT_DEPTH_16               (4 << 5)
+#define DP_TEST_MISC1                       0x233
+# define DP_TEST_REFRESH_DENOMINATOR        (1 << 0)
+# define REFRESH_DENOMINATOR_1              0x0
+# define REFRESH_DENOMINATOR_1_001          0x1
+# define DP_TEST_INTERLACED                 (1 << 1)
+# define DP_NON_INTERLACED                  (0 << 1)
+# define DP_INTERLACED                      (1 << 1)
+
+#define DP_TEST_REFRESH_RATE_NUMERATOR      0x234
 
 #define DP_TEST_CRC_R_CR		    0x240
 #define DP_TEST_CRC_G_Y			    0x242
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915: Add support for DP Video pattern compliance tests

[Manasi Navare]

The intel_dp_autotest_video_pattern() function gets invoked through the
compliance test handler on a HPD short pulse if the test type is
set to DP_TEST_VIDEO_PATTERN. This performs the DPCD registers
reads to read the requested test pattern, video pattern resolution,
frame rate and bits per color value. The results of this analysis
are handed off to userspace so that the userspace app can set the
video pattern mode appropriately for the test result/response.
When the  test is requested with specific BPC value, we read the BPC
value from the DPCD register. If this BPC value in intel_dp structure
has a non-zero value and we're on a display port connector, then we use
the value to calculate the bpp for the pipe. Also in this case if its
a 18bpp video pattern request, then we force the dithering on pipe to be
disabled since it causes CRC mismatches.

The compliance_test_active flag is set at the end of the individual
test handling functions. This is so that the kernel-side operations
can be completed without the risk of interruption from the userspace
app that is polling on that flag.

v3:
* Use the updated properly shifted bit definitions (Jani Nikula)
* Force dithering to be disabled on 18bpp compliance
test request (Manasi Navare)
v2:
* Updated the DPCD Register reads based on proper defines in header (Jani Nikula)
* Squahsed the patch that forced the pipe bpp to compliance test bpp (Jani Nikula)
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  9 +++++
 drivers/gpu/drm/i915/intel_display.c |  8 +++--
 drivers/gpu/drm/i915/intel_dp.c      | 66 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp_mst.c  |  7 +++-
 drivers/gpu/drm/i915/intel_drv.h     | 11 ++++++
 5 files changed, 98 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 8ec7edf..2eb8e50 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3776,6 +3776,15 @@ static int i915_displayport_test_data_show(struct seq_file *m, void *data)
 			    DP_TEST_LINK_EDID_READ)
 				seq_printf(m, "%lx",
 					   intel_dp->compliance.test_data.edid);
+			else if (intel_dp->compliance.test_type ==
+				 DP_TEST_LINK_VIDEO_PATTERN) {
+				seq_printf(m, "hdisplay: %d\n",
+					   intel_dp->compliance.test_data.hdisplay);
+				seq_printf(m, "vdisplay: %d\n",
+					   intel_dp->compliance.test_data.vdisplay);
+				seq_printf(m, "bpc: %u\n",
+					   intel_dp->compliance.test_data.bpc);
+			}
 		} else
 			seq_puts(m, "0");
 	}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0f4272f..c593789 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13052,8 +13052,12 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
 	}
 
 	/* Dithering seems to not pass-through bits correctly when it should, so
-	 * only enable it on 6bpc panels. */
-	pipe_config->dither = pipe_config->pipe_bpp == 6*3;
+	 * only enable it on 6bpc panels and when its not a compliance
+	 * test requesting 6bpc video pattern.
+	 */
+	pipe_config->dither = (pipe_config->pipe_bpp == 6*3) &&
+		!pipe_config->dither_force_disable;
+
 	DRM_DEBUG_KMS("hw max bpp: %i, pipe bpp: %i, dithering: %i\n",
 		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7ca45e0..044e36a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -28,8 +28,10 @@
 #include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/export.h>
+#include <linux/types.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
+#include <asm/byteorder.h>
 #include <drm/drmP.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
@@ -1593,6 +1595,13 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 	if (bpc > 0)
 		bpp = min(bpp, 3*bpc);
 
+	/* For DP Compliance we override the computed bpp for the pipe */
+	if (intel_dp->compliance.test_data.bpc != 0) {
+		pipe_config->pipe_bpp =	3*intel_dp->compliance.test_data.bpc;
+		pipe_config->dither_force_disable = pipe_config->pipe_bpp == 6*3;
+		DRM_DEBUG_KMS("Setting pipe_bpp to %d\n",
+			      pipe_config->pipe_bpp);
+	}
 	return bpp;
 }
 
@@ -3972,6 +3981,63 @@ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
 static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 {
 	uint8_t test_result = DP_TEST_NAK;
+	uint8_t test_pattern;
+	uint16_t test_misc;
+	__be16 h_width, v_height;
+	int status = 0;
+
+	/* Read the TEST_PATTERN (DP CTS 3.1.5) */
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_PATTERN,
+				  &test_pattern, 1);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Test pattern read failed\n");
+		return 0;
+	}
+	if (test_pattern != DP_COLOR_RAMP)
+		return test_result;
+	intel_dp->compliance.test_data.video_pattern = test_pattern;
+
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_H_WIDTH_HI,
+				  &h_width, 2);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("H Width read failed\n");
+		return 0;
+	}
+	intel_dp->compliance.test_data.hdisplay = be16_to_cpu(h_width);
+
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_V_HEIGHT_HI,
+				  &v_height, 2);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("V Height read failed\n");
+		return 0;
+	}
+	intel_dp->compliance.test_data.vdisplay = be16_to_cpu(v_height);
+
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_MISC0,
+				  &test_misc, 1);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("TEST MISC read failed\n");
+		return 0;
+	}
+	if ((test_misc & DP_TEST_COLOR_FORMAT_MASK) != DP_COLOR_FORMAT_RGB)
+		return test_result;
+	if ((test_misc & DP_TEST_DYNAMIC_RANGE) != DP_VESA_RANGE)
+		return test_result;
+	switch (test_misc & DP_TEST_BIT_DEPTH_MASK) {
+	case DP_TEST_BIT_DEPTH_6:
+		intel_dp->compliance.test_data.bpc = 6;
+		break;
+	case DP_TEST_BIT_DEPTH_8:
+		intel_dp->compliance.test_data.bpc = 8;
+		break;
+	default:
+		return test_result;
+	}
+	/* Set test active flag here so userspace doesn't interrupt things */
+	intel_dp->compliance.test_active = 1;
+
+	test_result = DP_TEST_ACK;
+
 	return test_result;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 205fe47..29a9af1 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -47,6 +47,11 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 
 	pipe_config->has_pch_encoder = false;
 	bpp = 24;
+	if (intel_dp->compliance.test_data.bpc) {
+		bpp = intel_dp->compliance.test_data.bpc * 3;
+		DRM_DEBUG_KMS("Setting pipe bpp to %d\n",
+			      bpp);
+	}
 	/*
 	 * for MST we always configure max link bw - the spec doesn't
 	 * seem to suggest we should do otherwise.
@@ -55,7 +60,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 
 	pipe_config->lane_count = lane_count;
 
-	pipe_config->pipe_bpp = 24;
+	pipe_config->pipe_bpp = bpp;
 	pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
 
 	state = pipe_config->base.state;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1586a02..503e1b5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -578,6 +578,14 @@ struct intel_crtc_state {
 	 */
 	bool dither;
 
+	/*
+	 * Dither gets enabled for 18bpp which causes CRC mismatch errors for
+	 * compliance video pattern tests.
+	 * Disable dither only if it is a compliance test request for
+	 * 18bpp.
+	 */
+	bool dither_force_disable;
+
 	/* Controls for the clock computation, to override various stages. */
 	bool clock_set;
 
@@ -888,6 +896,9 @@ struct intel_dp_desc {
 
 struct intel_dp_compliance_data {
 	unsigned long edid;
+	uint8_t video_pattern;
+	uint16_t hdisplay, vdisplay;
+	uint8_t bpc;
 };
 
 struct intel_dp_compliance {
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✓ Fi.CI.BAT: success for Add Automation Support for DP Compliance (Rev 5)

[Patchwork]

== Series Details ==

Series: Add Automation Support for DP Compliance (Rev 5)
URL   : https://patchwork.freedesktop.org/series/18256/
State : success

== Summary ==

Series 18256v1 Add Automation Support for DP Compliance (Rev 5)
https://patchwork.freedesktop.org/api/1.0/series/18256/revisions/1/mbox/


fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:79   pass:66   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:224  dwarn:0   dfail:0   fail:1   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

6155f94d32e438a9656de680897060bd2245629a drm-tip: 2017y-01m-19d-18h-24m-49s UTC integration manifest
4c51428 drm/i915: Add support for DP Video pattern compliance tests
eed1f75 drm: Add definitions for DP compliance Video pattern tests
341cad7 drm/i915: Fixes to support DP Compliance EDID tests
75d54ed drm/i915: Add support for DP link training compliance

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3554/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Add support for DP link training compliance

[Jani Nikula]

On Fri, 20 Jan 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> This patch adds support to handle automated DP compliance
> link training test requests. This patch has been tested with
> Unigraf DPR-120 DP Compliance device for testing Link
> Training Compliance.
> After we get a short pulse Compliance test request, test
> request values are read and hotplug uevent is sent in order
> to trigger another modeset during which the pipe is configured
> and link is retrained and enabled for link parameters requested
> by the test.
>
> v3:
> * Validate the test link rate and lane count as soon as
> the erquest comes (Jani Nikula)
> v2:
> * Validate the test lane count before using it in
> intel_dp_compute_config (Jani Nikula)
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 60 ++++++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  2 files changed, 57 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e80d620..3e76b63 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1613,6 +1613,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>  	/* Conveniently, the link BW constants become indices with a shift...*/
>  	int min_clock = 0;
>  	int max_clock;
> +	int link_rate_index;
>  	int bpp, mode_rate;
>  	int link_avail, link_clock;
>  	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
> @@ -1654,6 +1655,15 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
>  		return false;
>  
> +	/* Use values requested by Compliance Test Request */
> +	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
> +		link_rate_index = intel_dp_link_rate_index(intel_dp,
> +							   common_rates,
> +							   drm_dp_bw_code_to_link_rate(intel_dp->compliance.test_link_rate));

You should probably store the link *rate* instead of the link rate
*code* to intel_dp->compliance.test_link_rate in
intel_dp_autotest_link_training().

> +		if (link_rate_index >= 0)
> +			min_clock = max_clock = link_rate_index;
> +		min_lane_count = max_lane_count = intel_dp->compliance.test_lane_count;
> +	}
>  	DRM_DEBUG_KMS("DP link computation with max lane count %i "
>  		      "max bw %d pixel clock %iKHz\n",
>  		      max_lane_count, common_rates[max_clock],
> @@ -3920,7 +3930,42 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  
>  static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>  {
> -	uint8_t test_result = DP_TEST_ACK;
> +	uint8_t test_result = DP_TEST_NAK;
> +	int status = 0;
> +	int min_lane_count = 1;
> +	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
> +	int link_rate_index;
> +	/* (DP CTS 1.2)
> +	 * 4.3.1.11
> +	 */
> +	/* Read the TEST_LANE_COUNT and TEST_LINK_RTAE fields (DP CTS 3.1.4) */
> +	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LANE_COUNT,
> +				  &intel_dp->compliance.test_lane_count);
> +
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Lane count read failed\n");
> +		return 0;

So if you fail to read the lane count, you return 0, write that to
TEST_RESPONSE, which is supposed to have no effect on TEST_REQ state per
the spec, i.e. writing 0 is useless. intel_dp->compliance.test_type is
set anyway, which will try to use the (stale) lane count and link rate
values.

> +	}
> +	intel_dp->compliance.test_lane_count &= DP_MAX_LANE_COUNT_MASK;
> +	/* Validate the requested lane count */
> +	if (intel_dp->compliance.test_lane_count < min_lane_count ||
> +	    intel_dp->compliance.test_lane_count > intel_dp->max_sink_lane_count)
> +		return test_result;

But if the lane count is out of bounds, you return NAK and write that to
TEST_RESPONSE, *but* set intel_dp->compliance.test_type and
intel_dp->compliance.test_lane_count anyway, and will try to use the
values later on anyway.

> +
> +	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LINK_RATE,
> +				   &intel_dp->compliance.test_link_rate);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Link Rate read failed\n");
> +		return 0;
> +	}
> +	/* Validate the requested link rate */
> +	link_rate_index = intel_dp_link_rate_index(intel_dp,
> +						   common_rates,
> +						   drm_dp_bw_code_to_link_rate(intel_dp->compliance.test_link_rate));
> +	if (link_rate_index < 0)
> +		return test_result;

Same as above for lane count, in both error scenarios.

You should probably read both lane count and link rate to local
variables first, bail out on read failures, check the values, bail out
on invalid values, then set the values in intel_dp->compliance.

intel_dp_handle_test_request() should probably be fixed to not set
intel_dp->compliance.test_type on errors. Presumably it should not write
zero values to TEST_RESPONSE because it should have no effect anyway;
but I'm not sure if we should return 0 from the autotest functions
anyway.

> +
> +	test_result = DP_TEST_ACK;
>  	return test_result;

Please tell me what purpose does the test_result variable have in this
function. I thought I said you could get rid of the variable.

BR,
Jani.

>  }
>  
> @@ -4135,9 +4180,8 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  	if (!intel_dp->lane_count)
>  		return;
>  
> -	/* if link training is requested we should perform it always */
> -	if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) ||
> -	    (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
> +	/* Retrain if Channel EQ or CR not ok */
> +	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
>  		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>  			      intel_encoder->base.name);
>  
> @@ -4162,6 +4206,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  intel_dp_short_pulse(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>  	u8 sink_irq_vector = 0;
>  	u8 old_sink_count = intel_dp->sink_count;
>  	bool ret;
> @@ -4195,7 +4240,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  				   sink_irq_vector);
>  
>  		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
> -			DRM_DEBUG_DRIVER("Test request in short pulse not handled\n");
> +			intel_dp_handle_test_request(intel_dp);
>  		if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
>  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>  	}
> @@ -4203,6 +4248,11 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  	intel_dp_check_link_status(intel_dp);
>  	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
> +		DRM_DEBUG_KMS("Link Training Compliance Test requested\n");
> +		/* Send a Hotplug Uevent to userspace to start modeset */
> +		drm_kms_helper_hotplug_event(intel_encoder->base.dev);
> +	}
>  
>  	return true;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0cec001..1586a02 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -894,6 +894,8 @@ struct intel_dp_compliance {
>  	unsigned long test_type;
>  	struct intel_dp_compliance_data test_data;
>  	bool test_active;
> +	u8 test_link_rate;
> +	u8 test_lane_count;
>  };
>  
>  struct intel_dp {

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm: Add definitions for DP compliance Video pattern tests

[Jani Nikula]

On Fri, 20 Jan 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> v3:
> * Fix the conventions in bit definitions (Jani Nikula)
> v2:
> * Add all the other DP Complianec TEST register defs (Jani Nikula)
> Cc: dri-devel@lists.freedesktop.org
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  include/drm/drm_dp_helper.h | 64 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 0468135..7ab4153 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -417,6 +417,70 @@
>  #define DP_TEST_LANE_COUNT		    0x220
>  
>  #define DP_TEST_PATTERN			    0x221
> +# define DP_NO_TEST_PATTERN                 0x0
> +# define DP_COLOR_RAMP                      0x1
> +# define DP_BLACK_AND_WHITE_VERTICAL_LINES  0x2
> +# define DP_COLOR_SQUARE                    0x3
> +
> +#define DP_TEST_H_TOTAL_HI                  0x222
> +#define DP_TEST_H_TOTAl_LO                  0x223
                         ^
Sorry, didn't notice it before, but the l in TOTAL is lower case.

> +
> +#define DP_TEST_V_TOTAL_HI                  0x224
> +#define DP_TEST_V_TOTAl_LO                  0x225
                         ^
Ditto.

> +
> +#define DP_TEST_H_START_HI                  0x226
> +#define DP_TEST_H_START_LO                  0x227
> +
> +#define DP_TEST_V_START_HI                  0x228
> +#define DP_TEST_V_START_LO                  0x229
> +
> +#define DP_TEST_HSYNC_HI                    0x22A
> +# define DP_TEST_HSYNC_POLARITY             (1 << 7)
> +# define DP_TEST_HSYNC_WIDTH_HI_MASK        (127 << 0)
> +#define DP_TEST_HSYNC_WIDTH_LO              0x22B
> +
> +#define DP_TEST_VSYNC_HI                    0x22C
> +# define DP_TEST_VSYNC_POLARITY             (1 << 7)
> +# define DP_TEST_VSYNC_WIDTH_HI_MASK        (127 << 0)
> +#define DP_TEST_VSYNC_WIDTH_LO              0x22D
> +
> +#define DP_TEST_H_WIDTH_HI                  0x22E
> +#define DP_TEST_H_WIDTH_LO                  0x22F
> +
> +#define DP_TEST_V_HEIGHT_HI                 0x230
> +#define DP_TEST_V_HEIGHT_LO                 0x231
> +
> +#define DP_TEST_MISC0                       0x232
> +# define DP_TEST_SYNC_CLOCK                 (1 << 0)
> +# define DP_CLOCK_ASYNC                     0x0
> +# define DP_CLOCK_SYNC                      0x1

The above two are not necessary.

> +# define DP_TEST_COLOR_FORMAT_MASK          (3 << 1)
> +# define DP_TEST_COLOR_FORMAT_SHIFT         1
> +# define DP_COLOR_FORMAT_RGB                (0 << 1)
> +# define DP_COLOR_FORMAT_YCbCr422           (1 << 1)
> +# define DP_COLOR_FORMAT_YCbCr444           (2 << 1)
> +# define DP_TEST_DYNAMIC_RANGE              (1 << 3)

I'd just name this DP_TEST_DYNAMIC_RANGE_CEA...

> +# define DP_VESA_RANGE                      (0 << 3)
> +# define DP_CEA_RANGE                       (1 << 3)

...and drop these two. But whatever.

> +# define DP_TEST_YCBCR_COEFFICIENTS         (1 << 4)
> +# define DP_YCBCR_COEFFICIENTS_ITU601       (0 << 4)
> +# define DP_YCBCR_COEFFICIENTS_ITU709       (1 << 4)
> +# define DP_TEST_BIT_DEPTH_MASK             (7 << 5)
> +# define DP_TEST_BIT_DEPTH_SHIFT            5
> +# define DP_TEST_BIT_DEPTH_6                (0 << 5)
> +# define DP_TEST_BIT_DEPTH_8                (1 << 5)
> +# define DP_TEST_BIT_DEPTH_10               (2 << 5)
> +# define DP_TEST_BIT_DEPTH_12               (3 << 5)
> +# define DP_TEST_BIT_DEPTH_16               (4 << 5)

Blank line here.

> +#define DP_TEST_MISC1                       0x233
> +# define DP_TEST_REFRESH_DENOMINATOR        (1 << 0)
> +# define REFRESH_DENOMINATOR_1              0x0
> +# define REFRESH_DENOMINATOR_1_001          0x1

DP_ prefix.

> +# define DP_TEST_INTERLACED                 (1 << 1)
> +# define DP_NON_INTERLACED                  (0 << 1)
> +# define DP_INTERLACED                      (1 << 1)

No need for these two. DP_TEST_INTERLACED is sufficient.

> +
> +#define DP_TEST_REFRESH_RATE_NUMERATOR      0x234
>  
>  #define DP_TEST_CRC_R_CR		    0x240
>  #define DP_TEST_CRC_G_Y			    0x242

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm: Add definitions for DP compliance Video pattern tests

[Manasi Navare]

On Fri, Jan 20, 2017 at 05:33:51PM +0200, Jani Nikula wrote:
> On Fri, 20 Jan 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > v3:
> > * Fix the conventions in bit definitions (Jani Nikula)
> > v2:
> > * Add all the other DP Complianec TEST register defs (Jani Nikula)
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  include/drm/drm_dp_helper.h | 64 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index 0468135..7ab4153 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -417,6 +417,70 @@
> >  #define DP_TEST_LANE_COUNT		    0x220
> >  
> >  #define DP_TEST_PATTERN			    0x221
> > +# define DP_NO_TEST_PATTERN                 0x0
> > +# define DP_COLOR_RAMP                      0x1
> > +# define DP_BLACK_AND_WHITE_VERTICAL_LINES  0x2
> > +# define DP_COLOR_SQUARE                    0x3
> > +
> > +#define DP_TEST_H_TOTAL_HI                  0x222
> > +#define DP_TEST_H_TOTAl_LO                  0x223
>                          ^
> Sorry, didn't notice it before, but the l in TOTAL is lower case.
>

Oh yes even my eyes didnt catch this.

 
> > +
> > +#define DP_TEST_V_TOTAL_HI                  0x224
> > +#define DP_TEST_V_TOTAl_LO                  0x225
>                          ^
> Ditto.
> 
> > +
> > +#define DP_TEST_H_START_HI                  0x226
> > +#define DP_TEST_H_START_LO                  0x227
> > +
> > +#define DP_TEST_V_START_HI                  0x228
> > +#define DP_TEST_V_START_LO                  0x229
> > +
> > +#define DP_TEST_HSYNC_HI                    0x22A
> > +# define DP_TEST_HSYNC_POLARITY             (1 << 7)
> > +# define DP_TEST_HSYNC_WIDTH_HI_MASK        (127 << 0)
> > +#define DP_TEST_HSYNC_WIDTH_LO              0x22B
> > +
> > +#define DP_TEST_VSYNC_HI                    0x22C
> > +# define DP_TEST_VSYNC_POLARITY             (1 << 7)
> > +# define DP_TEST_VSYNC_WIDTH_HI_MASK        (127 << 0)
> > +#define DP_TEST_VSYNC_WIDTH_LO              0x22D
> > +
> > +#define DP_TEST_H_WIDTH_HI                  0x22E
> > +#define DP_TEST_H_WIDTH_LO                  0x22F
> > +
> > +#define DP_TEST_V_HEIGHT_HI                 0x230
> > +#define DP_TEST_V_HEIGHT_LO                 0x231
> > +
> > +#define DP_TEST_MISC0                       0x232
> > +# define DP_TEST_SYNC_CLOCK                 (1 << 0)
> > +# define DP_CLOCK_ASYNC                     0x0
> > +# define DP_CLOCK_SYNC                      0x1
> 
> The above two are not necessary.
>

Wont these above defs be required for checking if it is DP_CLOCK_ASYNC or SYNC?
I can remove the DP_CLOCK_SINK.
 
> > +# define DP_TEST_COLOR_FORMAT_MASK          (3 << 1)
> > +# define DP_TEST_COLOR_FORMAT_SHIFT         1
> > +# define DP_COLOR_FORMAT_RGB                (0 << 1)
> > +# define DP_COLOR_FORMAT_YCbCr422           (1 << 1)
> > +# define DP_COLOR_FORMAT_YCbCr444           (2 << 1)
> > +# define DP_TEST_DYNAMIC_RANGE              (1 << 3)
> 
> I'd just name this DP_TEST_DYNAMIC_RANGE_CEA...
> 
> > +# define DP_VESA_RANGE                      (0 << 3)
> > +# define DP_CEA_RANGE                       (1 << 3)
> 
> ...and drop these two. But whatever.
>

But then how do I check for VESA?
 
> > +# define DP_TEST_YCBCR_COEFFICIENTS         (1 << 4)
> > +# define DP_YCBCR_COEFFICIENTS_ITU601       (0 << 4)
> > +# define DP_YCBCR_COEFFICIENTS_ITU709       (1 << 4)
> > +# define DP_TEST_BIT_DEPTH_MASK             (7 << 5)
> > +# define DP_TEST_BIT_DEPTH_SHIFT            5
> > +# define DP_TEST_BIT_DEPTH_6                (0 << 5)
> > +# define DP_TEST_BIT_DEPTH_8                (1 << 5)
> > +# define DP_TEST_BIT_DEPTH_10               (2 << 5)
> > +# define DP_TEST_BIT_DEPTH_12               (3 << 5)
> > +# define DP_TEST_BIT_DEPTH_16               (4 << 5)
> 
> Blank line here.
>

This blank line is on purpose since I am starting a diff register

Manasi

 
> > +#define DP_TEST_MISC1                       0x233
> > +# define DP_TEST_REFRESH_DENOMINATOR        (1 << 0)
> > +# define REFRESH_DENOMINATOR_1              0x0
> > +# define REFRESH_DENOMINATOR_1_001          0x1
> 
> DP_ prefix.
> 
> > +# define DP_TEST_INTERLACED                 (1 << 1)
> > +# define DP_NON_INTERLACED                  (0 << 1)
> > +# define DP_INTERLACED                      (1 << 1)
> 
> No need for these two. DP_TEST_INTERLACED is sufficient.
> 
> > +
> > +#define DP_TEST_REFRESH_RATE_NUMERATOR      0x234
> >  
> >  #define DP_TEST_CRC_R_CR		    0x240
> >  #define DP_TEST_CRC_G_Y			    0x242
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm: Add definitions for DP compliance Video pattern tests

[Jani Nikula]

On Fri, 20 Jan 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> On Fri, Jan 20, 2017 at 05:33:51PM +0200, Jani Nikula wrote:
>> On Fri, 20 Jan 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
>> > v3:
>> > * Fix the conventions in bit definitions (Jani Nikula)
>> > v2:
>> > * Add all the other DP Complianec TEST register defs (Jani Nikula)
>> > Cc: dri-devel@lists.freedesktop.org
>> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> > Cc: Daniel Vetter <daniel.vetter@intel.com>
>> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
>> > ---
>> >  include/drm/drm_dp_helper.h | 64 +++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 64 insertions(+)
>> >
>> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> > index 0468135..7ab4153 100644
>> > --- a/include/drm/drm_dp_helper.h
>> > +++ b/include/drm/drm_dp_helper.h
>> > @@ -417,6 +417,70 @@
>> >  #define DP_TEST_LANE_COUNT		    0x220
>> >  
>> >  #define DP_TEST_PATTERN			    0x221
>> > +# define DP_NO_TEST_PATTERN                 0x0
>> > +# define DP_COLOR_RAMP                      0x1
>> > +# define DP_BLACK_AND_WHITE_VERTICAL_LINES  0x2
>> > +# define DP_COLOR_SQUARE                    0x3
>> > +
>> > +#define DP_TEST_H_TOTAL_HI                  0x222
>> > +#define DP_TEST_H_TOTAl_LO                  0x223
>>                          ^
>> Sorry, didn't notice it before, but the l in TOTAL is lower case.
>>
>
> Oh yes even my eyes didnt catch this.
>
>  
>> > +
>> > +#define DP_TEST_V_TOTAL_HI                  0x224
>> > +#define DP_TEST_V_TOTAl_LO                  0x225
>>                          ^
>> Ditto.
>> 
>> > +
>> > +#define DP_TEST_H_START_HI                  0x226
>> > +#define DP_TEST_H_START_LO                  0x227
>> > +
>> > +#define DP_TEST_V_START_HI                  0x228
>> > +#define DP_TEST_V_START_LO                  0x229
>> > +
>> > +#define DP_TEST_HSYNC_HI                    0x22A
>> > +# define DP_TEST_HSYNC_POLARITY             (1 << 7)
>> > +# define DP_TEST_HSYNC_WIDTH_HI_MASK        (127 << 0)
>> > +#define DP_TEST_HSYNC_WIDTH_LO              0x22B
>> > +
>> > +#define DP_TEST_VSYNC_HI                    0x22C
>> > +# define DP_TEST_VSYNC_POLARITY             (1 << 7)
>> > +# define DP_TEST_VSYNC_WIDTH_HI_MASK        (127 << 0)
>> > +#define DP_TEST_VSYNC_WIDTH_LO              0x22D
>> > +
>> > +#define DP_TEST_H_WIDTH_HI                  0x22E
>> > +#define DP_TEST_H_WIDTH_LO                  0x22F
>> > +
>> > +#define DP_TEST_V_HEIGHT_HI                 0x230
>> > +#define DP_TEST_V_HEIGHT_LO                 0x231
>> > +
>> > +#define DP_TEST_MISC0                       0x232
>> > +# define DP_TEST_SYNC_CLOCK                 (1 << 0)
>> > +# define DP_CLOCK_ASYNC                     0x0
>> > +# define DP_CLOCK_SYNC                      0x1
>> 
>> The above two are not necessary.
>>
>
> Wont these above defs be required for checking if it is DP_CLOCK_ASYNC or SYNC?
> I can remove the DP_CLOCK_SINK.

If the bit is set, it's synchronous, and asynchronous otherwise? You
don't need three macros to check one bit.

>  
>> > +# define DP_TEST_COLOR_FORMAT_MASK          (3 << 1)
>> > +# define DP_TEST_COLOR_FORMAT_SHIFT         1
>> > +# define DP_COLOR_FORMAT_RGB                (0 << 1)
>> > +# define DP_COLOR_FORMAT_YCbCr422           (1 << 1)
>> > +# define DP_COLOR_FORMAT_YCbCr444           (2 << 1)
>> > +# define DP_TEST_DYNAMIC_RANGE              (1 << 3)
>> 
>> I'd just name this DP_TEST_DYNAMIC_RANGE_CEA...
>> 
>> > +# define DP_VESA_RANGE                      (0 << 3)
>> > +# define DP_CEA_RANGE                       (1 << 3)
>> 
>> ...and drop these two. But whatever.
>>
>
> But then how do I check for VESA?

...if it's not CEA, it's VESA?

>  
>> > +# define DP_TEST_YCBCR_COEFFICIENTS         (1 << 4)
>> > +# define DP_YCBCR_COEFFICIENTS_ITU601       (0 << 4)
>> > +# define DP_YCBCR_COEFFICIENTS_ITU709       (1 << 4)
>> > +# define DP_TEST_BIT_DEPTH_MASK             (7 << 5)
>> > +# define DP_TEST_BIT_DEPTH_SHIFT            5
>> > +# define DP_TEST_BIT_DEPTH_6                (0 << 5)
>> > +# define DP_TEST_BIT_DEPTH_8                (1 << 5)
>> > +# define DP_TEST_BIT_DEPTH_10               (2 << 5)
>> > +# define DP_TEST_BIT_DEPTH_12               (3 << 5)
>> > +# define DP_TEST_BIT_DEPTH_16               (4 << 5)
>> 
>> Blank line here.
>>
>
> This blank line is on purpose since I am starting a diff register

There was *no* blank line here, I meant you should add a blank line
here.

>
> Manasi
>
>  
>> > +#define DP_TEST_MISC1                       0x233
>> > +# define DP_TEST_REFRESH_DENOMINATOR        (1 << 0)
>> > +# define REFRESH_DENOMINATOR_1              0x0
>> > +# define REFRESH_DENOMINATOR_1_001          0x1
>> 
>> DP_ prefix.
>> 
>> > +# define DP_TEST_INTERLACED                 (1 << 1)
>> > +# define DP_NON_INTERLACED                  (0 << 1)
>> > +# define DP_INTERLACED                      (1 << 1)
>> 
>> No need for these two. DP_TEST_INTERLACED is sufficient.
>> 
>> > +
>> > +#define DP_TEST_REFRESH_RATE_NUMERATOR      0x234
>> >  
>> >  #define DP_TEST_CRC_R_CR		    0x240
>> >  #define DP_TEST_CRC_G_Y			    0x242
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/i915: Add support for DP Video pattern compliance tests

[Manasi Navare]

Jani,

This patch adds a fix for CRC mismatch errors as well.
The reason I have squashed this fix with this patch is because
these CRC failures are seen for a Video Pattern test.
It uses the approach that we discussed where a piep_config->dither_force_disable
is used to disable dithering for compliance 18bpp case in intel_modeset_pipe_config.

Regards
Manasi

On Thu, Jan 19, 2017 at 10:23:38PM -0800, Manasi Navare wrote:
> The intel_dp_autotest_video_pattern() function gets invoked through the
> compliance test handler on a HPD short pulse if the test type is
> set to DP_TEST_VIDEO_PATTERN. This performs the DPCD registers
> reads to read the requested test pattern, video pattern resolution,
> frame rate and bits per color value. The results of this analysis
> are handed off to userspace so that the userspace app can set the
> video pattern mode appropriately for the test result/response.
> When the  test is requested with specific BPC value, we read the BPC
> value from the DPCD register. If this BPC value in intel_dp structure
> has a non-zero value and we're on a display port connector, then we use
> the value to calculate the bpp for the pipe. Also in this case if its
> a 18bpp video pattern request, then we force the dithering on pipe to be
> disabled since it causes CRC mismatches.
> 
> The compliance_test_active flag is set at the end of the individual
> test handling functions. This is so that the kernel-side operations
> can be completed without the risk of interruption from the userspace
> app that is polling on that flag.
> 
> v3:
> * Use the updated properly shifted bit definitions (Jani Nikula)
> * Force dithering to be disabled on 18bpp compliance
> test request (Manasi Navare)
> v2:
> * Updated the DPCD Register reads based on proper defines in header (Jani Nikula)
> * Squahsed the patch that forced the pipe bpp to compliance test bpp (Jani Nikula)
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  9 +++++
>  drivers/gpu/drm/i915/intel_display.c |  8 +++--
>  drivers/gpu/drm/i915/intel_dp.c      | 66 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp_mst.c  |  7 +++-
>  drivers/gpu/drm/i915/intel_drv.h     | 11 ++++++
>  5 files changed, 98 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 8ec7edf..2eb8e50 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3776,6 +3776,15 @@ static int i915_displayport_test_data_show(struct seq_file *m, void *data)
>  			    DP_TEST_LINK_EDID_READ)
>  				seq_printf(m, "%lx",
>  					   intel_dp->compliance.test_data.edid);
> +			else if (intel_dp->compliance.test_type ==
> +				 DP_TEST_LINK_VIDEO_PATTERN) {
> +				seq_printf(m, "hdisplay: %d\n",
> +					   intel_dp->compliance.test_data.hdisplay);
> +				seq_printf(m, "vdisplay: %d\n",
> +					   intel_dp->compliance.test_data.vdisplay);
> +				seq_printf(m, "bpc: %u\n",
> +					   intel_dp->compliance.test_data.bpc);
> +			}
>  		} else
>  			seq_puts(m, "0");
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0f4272f..c593789 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13052,8 +13052,12 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
>  	}
>  
>  	/* Dithering seems to not pass-through bits correctly when it should, so
> -	 * only enable it on 6bpc panels. */
> -	pipe_config->dither = pipe_config->pipe_bpp == 6*3;
> +	 * only enable it on 6bpc panels and when its not a compliance
> +	 * test requesting 6bpc video pattern.
> +	 */
> +	pipe_config->dither = (pipe_config->pipe_bpp == 6*3) &&
> +		!pipe_config->dither_force_disable;
> +
>  	DRM_DEBUG_KMS("hw max bpp: %i, pipe bpp: %i, dithering: %i\n",
>  		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 7ca45e0..044e36a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -28,8 +28,10 @@
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
>  #include <linux/export.h>
> +#include <linux/types.h>
>  #include <linux/notifier.h>
>  #include <linux/reboot.h>
> +#include <asm/byteorder.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc.h>
> @@ -1593,6 +1595,13 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>  	if (bpc > 0)
>  		bpp = min(bpp, 3*bpc);
>  
> +	/* For DP Compliance we override the computed bpp for the pipe */
> +	if (intel_dp->compliance.test_data.bpc != 0) {
> +		pipe_config->pipe_bpp =	3*intel_dp->compliance.test_data.bpc;
> +		pipe_config->dither_force_disable = pipe_config->pipe_bpp == 6*3;
> +		DRM_DEBUG_KMS("Setting pipe_bpp to %d\n",
> +			      pipe_config->pipe_bpp);
> +	}
>  	return bpp;
>  }
>  
> @@ -3972,6 +3981,63 @@ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>  static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>  {
>  	uint8_t test_result = DP_TEST_NAK;
> +	uint8_t test_pattern;
> +	uint16_t test_misc;
> +	__be16 h_width, v_height;
> +	int status = 0;
> +
> +	/* Read the TEST_PATTERN (DP CTS 3.1.5) */
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_PATTERN,
> +				  &test_pattern, 1);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Test pattern read failed\n");
> +		return 0;
> +	}
> +	if (test_pattern != DP_COLOR_RAMP)
> +		return test_result;
> +	intel_dp->compliance.test_data.video_pattern = test_pattern;
> +
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_H_WIDTH_HI,
> +				  &h_width, 2);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("H Width read failed\n");
> +		return 0;
> +	}
> +	intel_dp->compliance.test_data.hdisplay = be16_to_cpu(h_width);
> +
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_V_HEIGHT_HI,
> +				  &v_height, 2);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("V Height read failed\n");
> +		return 0;
> +	}
> +	intel_dp->compliance.test_data.vdisplay = be16_to_cpu(v_height);
> +
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_MISC0,
> +				  &test_misc, 1);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("TEST MISC read failed\n");
> +		return 0;
> +	}
> +	if ((test_misc & DP_TEST_COLOR_FORMAT_MASK) != DP_COLOR_FORMAT_RGB)
> +		return test_result;
> +	if ((test_misc & DP_TEST_DYNAMIC_RANGE) != DP_VESA_RANGE)
> +		return test_result;
> +	switch (test_misc & DP_TEST_BIT_DEPTH_MASK) {
> +	case DP_TEST_BIT_DEPTH_6:
> +		intel_dp->compliance.test_data.bpc = 6;
> +		break;
> +	case DP_TEST_BIT_DEPTH_8:
> +		intel_dp->compliance.test_data.bpc = 8;
> +		break;
> +	default:
> +		return test_result;
> +	}
> +	/* Set test active flag here so userspace doesn't interrupt things */
> +	intel_dp->compliance.test_active = 1;
> +
> +	test_result = DP_TEST_ACK;
> +
>  	return test_result;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 205fe47..29a9af1 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -47,6 +47,11 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  
>  	pipe_config->has_pch_encoder = false;
>  	bpp = 24;
> +	if (intel_dp->compliance.test_data.bpc) {
> +		bpp = intel_dp->compliance.test_data.bpc * 3;
> +		DRM_DEBUG_KMS("Setting pipe bpp to %d\n",
> +			      bpp);
> +	}
>  	/*
>  	 * for MST we always configure max link bw - the spec doesn't
>  	 * seem to suggest we should do otherwise.
> @@ -55,7 +60,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  
>  	pipe_config->lane_count = lane_count;
>  
> -	pipe_config->pipe_bpp = 24;
> +	pipe_config->pipe_bpp = bpp;
>  	pipe_config->port_clock = intel_dp_max_link_rate(intel_dp);
>  
>  	state = pipe_config->base.state;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1586a02..503e1b5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -578,6 +578,14 @@ struct intel_crtc_state {
>  	 */
>  	bool dither;
>  
> +	/*
> +	 * Dither gets enabled for 18bpp which causes CRC mismatch errors for
> +	 * compliance video pattern tests.
> +	 * Disable dither only if it is a compliance test request for
> +	 * 18bpp.
> +	 */
> +	bool dither_force_disable;
> +
>  	/* Controls for the clock computation, to override various stages. */
>  	bool clock_set;
>  
> @@ -888,6 +896,9 @@ struct intel_dp_desc {
>  
>  struct intel_dp_compliance_data {
>  	unsigned long edid;
> +	uint8_t video_pattern;
> +	uint16_t hdisplay, vdisplay;
> +	uint8_t bpc;
>  };
>  
>  struct intel_dp_compliance {
> -- 
> 1.9.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Add support for DP link training compliance

[Manasi Navare]

Thanks Jani for reviewing this patch and for your feedback.
Its very useful feedback and below are some of my comments:

On Fri, Jan 20, 2017 at 05:05:03PM +0200, Jani Nikula wrote:
> On Fri, 20 Jan 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > This patch adds support to handle automated DP compliance
> > link training test requests. This patch has been tested with
> > Unigraf DPR-120 DP Compliance device for testing Link
> > Training Compliance.
> > After we get a short pulse Compliance test request, test
> > request values are read and hotplug uevent is sent in order
> > to trigger another modeset during which the pipe is configured
> > and link is retrained and enabled for link parameters requested
> > by the test.
> >
> > v3:
> > * Validate the test link rate and lane count as soon as
> > the erquest comes (Jani Nikula)
> > v2:
> > * Validate the test lane count before using it in
> > intel_dp_compute_config (Jani Nikula)
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  | 60 ++++++++++++++++++++++++++++++++++++----
> >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> >  2 files changed, 57 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index e80d620..3e76b63 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1613,6 +1613,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
> >  	/* Conveniently, the link BW constants become indices with a shift...*/
> >  	int min_clock = 0;
> >  	int max_clock;
> > +	int link_rate_index;
> >  	int bpp, mode_rate;
> >  	int link_avail, link_clock;
> >  	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
> > @@ -1654,6 +1655,15 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
> >  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> >  		return false;
> >  
> > +	/* Use values requested by Compliance Test Request */
> > +	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
> > +		link_rate_index = intel_dp_link_rate_index(intel_dp,
> > +							   common_rates,
> > +							   drm_dp_bw_code_to_link_rate(intel_dp->compliance.test_link_rate));
> 
> You should probably store the link *rate* instead of the link rate
> *code* to intel_dp->compliance.test_link_rate in
> intel_dp_autotest_link_training().
> 

I totally agree and this is how it is addressed in the new revision.
I have declared a local variable test_link_bw that stores the BW code
read from the DPCD test registers. This BW code is then validated to see
that it corresponds to the valid link rate values. If it is invalid then I return
a TEST_NAK and if it is valid then I convert this BW code to link_rate
and store the link_rate in intel_dp->compliance.test_link_rate.

> > +		if (link_rate_index >= 0)
> > +			min_clock = max_clock = link_rate_index;
> > +		min_lane_count = max_lane_count = intel_dp->compliance.test_lane_count;
> > +	}
> >  	DRM_DEBUG_KMS("DP link computation with max lane count %i "
> >  		      "max bw %d pixel clock %iKHz\n",
> >  		      max_lane_count, common_rates[max_clock],
> > @@ -3920,7 +3930,42 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> >  
> >  static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
> >  {
> > -	uint8_t test_result = DP_TEST_ACK;
> > +	uint8_t test_result = DP_TEST_NAK;
> > +	int status = 0;
> > +	int min_lane_count = 1;
> > +	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
> > +	int link_rate_index;
> > +	/* (DP CTS 1.2)
> > +	 * 4.3.1.11
> > +	 */
> > +	/* Read the TEST_LANE_COUNT and TEST_LINK_RTAE fields (DP CTS 3.1.4) */
> > +	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LANE_COUNT,
> > +				  &intel_dp->compliance.test_lane_count);
> > +
> > +	if (status <= 0) {
> > +		DRM_DEBUG_KMS("Lane count read failed\n");
> > +		return 0;
> 
> So if you fail to read the lane count, you return 0, write that to
> TEST_RESPONSE, which is supposed to have no effect on TEST_REQ state per
> the spec, i.e. writing 0 is useless. intel_dp->compliance.test_type is
> set anyway, which will try to use the (stale) lane count and link rate
> values.
> 

I agree that writing a 0 to test_result has no effect to the test response and
there is a possibility of using stale values of lane count and link rate.
So now in the new revision, I am returning a TEST_NAK in case of both read failures
as well as the invalid values.
As per the new patch you have submitted, once intel_dp_handle_test_request() recieves
a TEST_NAK it should not set the test_type and it will eliminate the possibility
of using stale values of link rate and lane count.


> > +	}
> > +	intel_dp->compliance.test_lane_count &= DP_MAX_LANE_COUNT_MASK;
> > +	/* Validate the requested lane count */
> > +	if (intel_dp->compliance.test_lane_count < min_lane_count ||
> > +	    intel_dp->compliance.test_lane_count > intel_dp->max_sink_lane_count)
> > +		return test_result;
> 
> But if the lane count is out of bounds, you return NAK and write that to
> TEST_RESPONSE, *but* set intel_dp->compliance.test_type and
> intel_dp->compliance.test_lane_count anyway, and will try to use the
> values later on anyway.
> 

This has been addressed in the new revision by storing the lane count read
from DPCD test registers first into a local test_lane_count variable. This
values is then validated and made sure it is not out of bounds. If it is invalid,
I return a TEST_NAK so test_type will not be set and this values will not be written to
intel_dp->compliance.test_lane_count. If this value is within bounds, only then
I go ahead and populate the intel_dp->compliance.test_lane_count field.


> > +
> > +	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LINK_RATE,
> > +				   &intel_dp->compliance.test_link_rate);
> > +	if (status <= 0) {
> > +		DRM_DEBUG_KMS("Link Rate read failed\n");
> > +		return 0;
> > +	}
> > +	/* Validate the requested link rate */
> > +	link_rate_index = intel_dp_link_rate_index(intel_dp,
> > +						   common_rates,
> > +						   drm_dp_bw_code_to_link_rate(intel_dp->compliance.test_link_rate));
> > +	if (link_rate_index < 0)
> > +		return test_result;
> 
> Same as above for lane count, in both error scenarios.
> 
> You should probably read both lane count and link rate to local
> variables first, bail out on read failures, check the values, bail out
> on invalid values, then set the values in intel_dp->compliance.
>

Yes this is exactly what the new revision does.
 
> intel_dp_handle_test_request() should probably be fixed to not set
> intel_dp->compliance.test_type on errors. Presumably it should not write
> zero values to TEST_RESPONSE because it should have no effect anyway;
> but I'm not sure if we should return 0 from the autotest functions
> anyway.
>

Yes, we should not return 0 from the autotest functions. So I am only returning
either a NAK on read failures or invalid values or a TEST_ACK in case of success.

 
> > +
> > +	test_result = DP_TEST_ACK;
> >  	return test_result;
> 
> Please tell me what purpose does the test_result variable have in this
> function. I thought I said you could get rid of the variable.
> 
> BR,
> Jani.
>

The whole purpose of adding a variable named test_result for readability.
I think that instead of returning NAK and ACK in differnt scenarios, its more
readable to return test_result instead. This variable can be set to NAK or ACK
as per the failures or success.
That is the reason why I have still kept this variable.

Regards
Manasi


> >  }
> >  
> > @@ -4135,9 +4180,8 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> >  	if (!intel_dp->lane_count)
> >  		return;
> >  
> > -	/* if link training is requested we should perform it always */
> > -	if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) ||
> > -	    (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
> > +	/* Retrain if Channel EQ or CR not ok */
> > +	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
> >  		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> >  			      intel_encoder->base.name);
> >  
> > @@ -4162,6 +4206,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> >  intel_dp_short_pulse(struct intel_dp *intel_dp)
> >  {
> >  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> >  	u8 sink_irq_vector = 0;
> >  	u8 old_sink_count = intel_dp->sink_count;
> >  	bool ret;
> > @@ -4195,7 +4240,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> >  				   sink_irq_vector);
> >  
> >  		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
> > -			DRM_DEBUG_DRIVER("Test request in short pulse not handled\n");
> > +			intel_dp_handle_test_request(intel_dp);
> >  		if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
> >  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
> >  	}
> > @@ -4203,6 +4248,11 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> >  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >  	intel_dp_check_link_status(intel_dp);
> >  	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > +	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
> > +		DRM_DEBUG_KMS("Link Training Compliance Test requested\n");
> > +		/* Send a Hotplug Uevent to userspace to start modeset */
> > +		drm_kms_helper_hotplug_event(intel_encoder->base.dev);
> > +	}
> >  
> >  	return true;
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 0cec001..1586a02 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -894,6 +894,8 @@ struct intel_dp_compliance {
> >  	unsigned long test_type;
> >  	struct intel_dp_compliance_data test_data;
> >  	bool test_active;
> > +	u8 test_link_rate;
> > +	u8 test_lane_count;
> >  };
> >  
> >  struct intel_dp {
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Add support for DP link training compliance

[Manasi Navare]

On Mon, Jan 23, 2017 at 12:50:07PM -0800, Manasi Navare wrote:
> Thanks Jani for reviewing this patch and for your feedback.
> Its very useful feedback and below are some of my comments:
> 
> On Fri, Jan 20, 2017 at 05:05:03PM +0200, Jani Nikula wrote:
> > On Fri, 20 Jan 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > > This patch adds support to handle automated DP compliance
> > > link training test requests. This patch has been tested with
> > > Unigraf DPR-120 DP Compliance device for testing Link
> > > Training Compliance.
> > > After we get a short pulse Compliance test request, test
> > > request values are read and hotplug uevent is sent in order
> > > to trigger another modeset during which the pipe is configured
> > > and link is retrained and enabled for link parameters requested
> > > by the test.
> > >
> > > v3:
> > > * Validate the test link rate and lane count as soon as
> > > the erquest comes (Jani Nikula)
> > > v2:
> > > * Validate the test lane count before using it in
> > > intel_dp_compute_config (Jani Nikula)
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c  | 60 ++++++++++++++++++++++++++++++++++++----
> > >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> > >  2 files changed, 57 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index e80d620..3e76b63 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1613,6 +1613,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
> > >  	/* Conveniently, the link BW constants become indices with a shift...*/
> > >  	int min_clock = 0;
> > >  	int max_clock;
> > > +	int link_rate_index;
> > >  	int bpp, mode_rate;
> > >  	int link_avail, link_clock;
> > >  	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
> > > @@ -1654,6 +1655,15 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
> > >  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> > >  		return false;
> > >  
> > > +	/* Use values requested by Compliance Test Request */
> > > +	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
> > > +		link_rate_index = intel_dp_link_rate_index(intel_dp,
> > > +							   common_rates,
> > > +							   drm_dp_bw_code_to_link_rate(intel_dp->compliance.test_link_rate));
> > 
> > You should probably store the link *rate* instead of the link rate
> > *code* to intel_dp->compliance.test_link_rate in
> > intel_dp_autotest_link_training().
> > 
> 
> I totally agree and this is how it is addressed in the new revision.
> I have declared a local variable test_link_bw that stores the BW code
> read from the DPCD test registers. This BW code is then validated to see
> that it corresponds to the valid link rate values. If it is invalid then I return
> a TEST_NAK and if it is valid then I convert this BW code to link_rate
> and store the link_rate in intel_dp->compliance.test_link_rate.
> 
> > > +		if (link_rate_index >= 0)
> > > +			min_clock = max_clock = link_rate_index;
> > > +		min_lane_count = max_lane_count = intel_dp->compliance.test_lane_count;
> > > +	}
> > >  	DRM_DEBUG_KMS("DP link computation with max lane count %i "
> > >  		      "max bw %d pixel clock %iKHz\n",
> > >  		      max_lane_count, common_rates[max_clock],
> > > @@ -3920,7 +3930,42 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> > >  
> > >  static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
> > >  {
> > > -	uint8_t test_result = DP_TEST_ACK;
> > > +	uint8_t test_result = DP_TEST_NAK;
> > > +	int status = 0;
> > > +	int min_lane_count = 1;
> > > +	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
> > > +	int link_rate_index;
> > > +	/* (DP CTS 1.2)
> > > +	 * 4.3.1.11
> > > +	 */
> > > +	/* Read the TEST_LANE_COUNT and TEST_LINK_RTAE fields (DP CTS 3.1.4) */
> > > +	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LANE_COUNT,
> > > +				  &intel_dp->compliance.test_lane_count);
> > > +
> > > +	if (status <= 0) {
> > > +		DRM_DEBUG_KMS("Lane count read failed\n");
> > > +		return 0;
> > 
> > So if you fail to read the lane count, you return 0, write that to
> > TEST_RESPONSE, which is supposed to have no effect on TEST_REQ state per
> > the spec, i.e. writing 0 is useless. intel_dp->compliance.test_type is
> > set anyway, which will try to use the (stale) lane count and link rate
> > values.
> > 
> 
> I agree that writing a 0 to test_result has no effect to the test response and
> there is a possibility of using stale values of lane count and link rate.
> So now in the new revision, I am returning a TEST_NAK in case of both read failures
> as well as the invalid values.
> As per the new patch you have submitted, once intel_dp_handle_test_request() recieves
> a TEST_NAK it should not set the test_type and it will eliminate the possibility
> of using stale values of link rate and lane count.
> 
> 
> > > +	}
> > > +	intel_dp->compliance.test_lane_count &= DP_MAX_LANE_COUNT_MASK;
> > > +	/* Validate the requested lane count */
> > > +	if (intel_dp->compliance.test_lane_count < min_lane_count ||
> > > +	    intel_dp->compliance.test_lane_count > intel_dp->max_sink_lane_count)
> > > +		return test_result;
> > 
> > But if the lane count is out of bounds, you return NAK and write that to
> > TEST_RESPONSE, *but* set intel_dp->compliance.test_type and
> > intel_dp->compliance.test_lane_count anyway, and will try to use the
> > values later on anyway.
> > 
> 
> This has been addressed in the new revision by storing the lane count read
> from DPCD test registers first into a local test_lane_count variable. This
> values is then validated and made sure it is not out of bounds. If it is invalid,
> I return a TEST_NAK so test_type will not be set and this values will not be written to
> intel_dp->compliance.test_lane_count. If this value is within bounds, only then
> I go ahead and populate the intel_dp->compliance.test_lane_count field.
> 
> 
> > > +
> > > +	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LINK_RATE,
> > > +				   &intel_dp->compliance.test_link_rate);
> > > +	if (status <= 0) {
> > > +		DRM_DEBUG_KMS("Link Rate read failed\n");
> > > +		return 0;
> > > +	}
> > > +	/* Validate the requested link rate */
> > > +	link_rate_index = intel_dp_link_rate_index(intel_dp,
> > > +						   common_rates,
> > > +						   drm_dp_bw_code_to_link_rate(intel_dp->compliance.test_link_rate));
> > > +	if (link_rate_index < 0)
> > > +		return test_result;
> > 
> > Same as above for lane count, in both error scenarios.
> > 
> > You should probably read both lane count and link rate to local
> > variables first, bail out on read failures, check the values, bail out
> > on invalid values, then set the values in intel_dp->compliance.
> >
> 
> Yes this is exactly what the new revision does.
>

Taking one more look at it, I think I should defer populating compliance test values
until all the validating is done. So I can move setting the compliance.test_lane_count
and compliance.test_link_rate to the end of the function just before returning a TEST_ACK.
The reason that this was not addressed earlier was because I assumed that we were invalidating
the compliance state on NAKs anyway by not setting test_type. But to be on the safe side
I will move all the compliance test data setting at the end of the function.

This means even for video pattern test handler I should read all the video pattern data
into local variables first and then set the compliance video pattern test data values only
at the end of the function just before returning a TEST ACK.

Regards
Manasi

  
> > intel_dp_handle_test_request() should probably be fixed to not set
> > intel_dp->compliance.test_type on errors. Presumably it should not write
> > zero values to TEST_RESPONSE because it should have no effect anyway;
> > but I'm not sure if we should return 0 from the autotest functions
> > anyway.
> >
> 
> Yes, we should not return 0 from the autotest functions. So I am only returning
> either a NAK on read failures or invalid values or a TEST_ACK in case of success.
> 
>  
> > > +
> > > +	test_result = DP_TEST_ACK;
> > >  	return test_result;
> > 
> > Please tell me what purpose does the test_result variable have in this
> > function. I thought I said you could get rid of the variable.
> > 
> > BR,
> > Jani.
> >
> 
> The whole purpose of adding a variable named test_result for readability.
> I think that instead of returning NAK and ACK in differnt scenarios, its more
> readable to return test_result instead. This variable can be set to NAK or ACK
> as per the failures or success.
> That is the reason why I have still kept this variable.
> 
> Regards
> Manasi
> 
> 
> > >  }
> > >  
> > > @@ -4135,9 +4180,8 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> > >  	if (!intel_dp->lane_count)
> > >  		return;
> > >  
> > > -	/* if link training is requested we should perform it always */
> > > -	if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) ||
> > > -	    (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
> > > +	/* Retrain if Channel EQ or CR not ok */
> > > +	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
> > >  		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> > >  			      intel_encoder->base.name);
> > >  
> > > @@ -4162,6 +4206,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> > >  intel_dp_short_pulse(struct intel_dp *intel_dp)
> > >  {
> > >  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > > +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> > >  	u8 sink_irq_vector = 0;
> > >  	u8 old_sink_count = intel_dp->sink_count;
> > >  	bool ret;
> > > @@ -4195,7 +4240,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> > >  				   sink_irq_vector);
> > >  
> > >  		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
> > > -			DRM_DEBUG_DRIVER("Test request in short pulse not handled\n");
> > > +			intel_dp_handle_test_request(intel_dp);
> > >  		if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
> > >  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
> > >  	}
> > > @@ -4203,6 +4248,11 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> > >  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > >  	intel_dp_check_link_status(intel_dp);
> > >  	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > > +	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
> > > +		DRM_DEBUG_KMS("Link Training Compliance Test requested\n");
> > > +		/* Send a Hotplug Uevent to userspace to start modeset */
> > > +		drm_kms_helper_hotplug_event(intel_encoder->base.dev);
> > > +	}
> > >  
> > >  	return true;
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 0cec001..1586a02 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -894,6 +894,8 @@ struct intel_dp_compliance {
> > >  	unsigned long test_type;
> > >  	struct intel_dp_compliance_data test_data;
> > >  	bool test_active;
> > > +	u8 test_link_rate;
> > > +	u8 test_lane_count;
> > >  };
> > >  
> > >  struct intel_dp {
> > 
> > -- 
> > Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/4] drm/i915: Add support for DP link training compliance

[Manasi Navare]

This patch adds support to handle automated DP compliance
link training test requests. This patch has been tested with
Unigraf DPR-120 DP Compliance device for testing Link
Training Compliance.
After we get a short pulse Compliance test request, test
request values are read and hotplug uevent is sent in order
to trigger another modeset during which the pipe is configured
and link is retrained and enabled for link parameters requested
by the test.

v4:
* Return TEST_NAK for read failures and invalid
values (Jani Nikula)
* Conver the test link BW to link rate before storing (Jani Nikula)
v3:
* Validate the test link rate and lane count as soon as
the request comes (Jani Nikula)
v2:
* Validate the test lane count before using it in
intel_dp_compute_config (Jani Nikula)
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 64 ++++++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 2 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e80d620..8d5008c7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1613,6 +1613,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 	/* Conveniently, the link BW constants become indices with a shift...*/
 	int min_clock = 0;
 	int max_clock;
+	int link_rate_index;
 	int bpp, mode_rate;
 	int link_avail, link_clock;
 	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
@@ -1654,6 +1655,15 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
 		return false;
 
+	/* Use values requested by Compliance Test Request */
+	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
+		link_rate_index = intel_dp_link_rate_index(intel_dp,
+							   common_rates,
+							   intel_dp->compliance.test_link_rate);
+		if (link_rate_index >= 0)
+			min_clock = max_clock = link_rate_index;
+		min_lane_count = max_lane_count = intel_dp->compliance.test_lane_count;
+	}
 	DRM_DEBUG_KMS("DP link computation with max lane count %i "
 		      "max bw %d pixel clock %iKHz\n",
 		      max_lane_count, common_rates[max_clock],
@@ -3920,7 +3930,46 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 
 static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
 {
-	uint8_t test_result = DP_TEST_ACK;
+	uint8_t test_result = DP_TEST_NAK;
+	int status = 0;
+	int min_lane_count = 1;
+	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
+	int link_rate_index, test_link_rate;
+	uint8_t test_lane_count, test_link_bw;
+	/* (DP CTS 1.2)
+	 * 4.3.1.11
+	 */
+	/* Read the TEST_LANE_COUNT and TEST_LINK_RTAE fields (DP CTS 3.1.4) */
+	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LANE_COUNT,
+				  &test_lane_count);
+
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Lane count read failed\n");
+		return test_result;
+	}
+	test_lane_count &= DP_MAX_LANE_COUNT_MASK;
+	/* Validate the requested lane count */
+	if (test_lane_count < min_lane_count ||
+	    test_lane_count > intel_dp->max_sink_lane_count)
+		return test_result;
+	intel_dp->compliance.test_lane_count = test_lane_count;
+
+	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LINK_RATE,
+				   &test_link_bw);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Link Rate read failed\n");
+		return test_result;
+	}
+	/* Validate the requested link rate */
+	test_link_rate = drm_dp_bw_code_to_link_rate(test_link_bw);
+	link_rate_index = intel_dp_link_rate_index(intel_dp,
+						   common_rates,
+						   test_link_rate);
+	if (link_rate_index < 0)
+		return test_result;
+	intel_dp->compliance.test_link_rate = test_link_rate;
+
+	test_result = DP_TEST_ACK;
 	return test_result;
 }
 
@@ -4135,9 +4184,8 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 	if (!intel_dp->lane_count)
 		return;
 
-	/* if link training is requested we should perform it always */
-	if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) ||
-	    (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
+	/* Retrain if Channel EQ or CR not ok */
+	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
 		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
 			      intel_encoder->base.name);
 
@@ -4162,6 +4210,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 intel_dp_short_pulse(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	u8 sink_irq_vector = 0;
 	u8 old_sink_count = intel_dp->sink_count;
 	bool ret;
@@ -4195,7 +4244,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 				   sink_irq_vector);
 
 		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
-			DRM_DEBUG_DRIVER("Test request in short pulse not handled\n");
+			intel_dp_handle_test_request(intel_dp);
 		if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
@@ -4203,6 +4252,11 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 	intel_dp_check_link_status(intel_dp);
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
+	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
+		DRM_DEBUG_KMS("Link Training Compliance Test requested\n");
+		/* Send a Hotplug Uevent to userspace to start modeset */
+		drm_kms_helper_hotplug_event(intel_encoder->base.dev);
+	}
 
 	return true;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0cec001..0b8d6e4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -894,6 +894,8 @@ struct intel_dp_compliance {
 	unsigned long test_type;
 	struct intel_dp_compliance_data test_data;
 	bool test_active;
+	int test_link_rate;
+	u8 test_lane_count;
 };
 
 struct intel_dp {
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/i915: Add support for DP link training compliance

[Jani Nikula]

On Wed, 18 Jan 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> This patch adds support to handle automated DP compliance
> link training test requests. This patch has been tested with
> Unigraf DPR-120 DP Compliance device for testing Link
> Training Compliance.
> After we get a short pulse Compliance test request, test
> request values are read and hotplug uevent is sent in order
> to trigger another modeset during which the pipe is configured
> and link is retrained and enabled for link parameters requested
> by the test.
>
> v2:
> * Validate the test lane count before using it in
> intel_dp_compute_config (Jani Nikula)
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 46 ++++++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  2 files changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e80d620..7a1d3c4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1613,6 +1613,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>  	/* Conveniently, the link BW constants become indices with a shift...*/
>  	int min_clock = 0;
>  	int max_clock;
> +	int link_rate_index;
>  	int bpp, mode_rate;
>  	int link_avail, link_clock;
>  	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
> @@ -1654,6 +1655,17 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
>  		return false;
>  
> +	/* Use values requested by Compliance Test Request */
> +	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
> +		link_rate_index = intel_dp_link_rate_index(intel_dp,
> +							   common_rates,
> +							   drm_dp_bw_code_to_link_rate(intel_dp->compliance.test_link_rate));
> +		if (link_rate_index >= 0)
> +			min_clock = max_clock = link_rate_index;
> +		if (min_lane_count <= intel_dp->compliance.test_lane_count
> +		    && intel_dp->compliance.test_lane_count >= max_lane_count)
> +			min_lane_count = max_lane_count = intel_dp->compliance.test_lane_count;
> +	}

Why don't we do these checks and conversions when we get the test
request, so we can NAK the request on errors?

>  	DRM_DEBUG_KMS("DP link computation with max lane count %i "
>  		      "max bw %d pixel clock %iKHz\n",
>  		      max_lane_count, common_rates[max_clock],
> @@ -3921,6 +3933,27 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>  {
>  	uint8_t test_result = DP_TEST_ACK;
> +	int status = 0;
> +	/* (DP CTS 1.2)
> +	 * 4.3.1.11
> +	 */
> +	/* Read the TEST_LANE_COUNT and TEST_LINK_RTAE fields (DP CTS 3.1.4) */
> +	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LANE_COUNT,
> +				  &intel_dp->compliance.test_lane_count);
> +
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Lane count read failed\n");
> +		return 0;

return DP_TEST_NAK;

> +	}
> +	intel_dp->compliance.test_lane_count &= DP_MAX_LANE_COUNT_MASK;
> +
> +	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LINK_RATE,
> +				   &intel_dp->compliance.test_link_rate);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Link Rate read failed\n");
> +		return 0;

return DP_TEST_NAK;

> +	}
> +
>  	return test_result;

Could replace this with a direct return DP_TEST_ACK without the temp
variable.

>  }
>  
> @@ -4135,9 +4168,8 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  	if (!intel_dp->lane_count)
>  		return;
>  
> -	/* if link training is requested we should perform it always */
> -	if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) ||
> -	    (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
> +	/* Retrain if Channel EQ or CR not ok */
> +	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
>  		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>  			      intel_encoder->base.name);
>  
> @@ -4162,6 +4194,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  intel_dp_short_pulse(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>  	u8 sink_irq_vector = 0;
>  	u8 old_sink_count = intel_dp->sink_count;
>  	bool ret;
> @@ -4195,7 +4228,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  				   sink_irq_vector);
>  
>  		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
> -			DRM_DEBUG_DRIVER("Test request in short pulse not handled\n");
> +			intel_dp_handle_test_request(intel_dp);
>  		if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
>  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>  	}
> @@ -4203,6 +4236,11 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  	intel_dp_check_link_status(intel_dp);
>  	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
> +		DRM_DEBUG_KMS("Link Training Compliance Test requested\n");
> +		/* Send a Hotplug Uevent to userspace to start modeset */
> +		drm_kms_helper_hotplug_event(intel_encoder->base.dev);
> +	}
>  
>  	return true;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 84258df..7a2eb76 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -893,6 +893,8 @@ struct intel_dp_compliance {
>  	unsigned long test_type;
>  	struct intel_dp_compliance_data test_data;
>  	bool test_active;
> +	u8 test_link_rate;
> +	u8 test_lane_count;
>  };
>  
>  struct intel_dp {

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Add support for DP link training compliance

[Manasi Navare]

Hi Jani,

Could you please review this patch?
I have added the lane count checking and other review comments you had.
Is there anything else that is blocking from getting this patch merged?

Regards
Manasi

On Tue, Jan 17, 2017 at 02:57:12PM -0800, Manasi Navare wrote:
> This patch adds support to handle automated DP compliance
> link training test requests. This patch has been tested with
> Unigraf DPR-120 DP Compliance device for testing Link
> Training Compliance.
> After we get a short pulse Compliance test request, test
> request values are read and hotplug uevent is sent in order
> to trigger another modeset during which the pipe is configured
> and link is retrained and enabled for link parameters requested
> by the test.
> 
> v2:
> * Validate the test lane count before using it in
> intel_dp_compute_config (Jani Nikula)
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 46 ++++++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  2 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e80d620..7a1d3c4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1613,6 +1613,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>  	/* Conveniently, the link BW constants become indices with a shift...*/
>  	int min_clock = 0;
>  	int max_clock;
> +	int link_rate_index;
>  	int bpp, mode_rate;
>  	int link_avail, link_clock;
>  	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
> @@ -1654,6 +1655,17 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
>  		return false;
>  
> +	/* Use values requested by Compliance Test Request */
> +	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
> +		link_rate_index = intel_dp_link_rate_index(intel_dp,
> +							   common_rates,
> +							   drm_dp_bw_code_to_link_rate(intel_dp->compliance.test_link_rate));
> +		if (link_rate_index >= 0)
> +			min_clock = max_clock = link_rate_index;
> +		if (min_lane_count <= intel_dp->compliance.test_lane_count
> +		    && intel_dp->compliance.test_lane_count >= max_lane_count)
> +			min_lane_count = max_lane_count = intel_dp->compliance.test_lane_count;
> +	}
>  	DRM_DEBUG_KMS("DP link computation with max lane count %i "
>  		      "max bw %d pixel clock %iKHz\n",
>  		      max_lane_count, common_rates[max_clock],
> @@ -3921,6 +3933,27 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>  {
>  	uint8_t test_result = DP_TEST_ACK;
> +	int status = 0;
> +	/* (DP CTS 1.2)
> +	 * 4.3.1.11
> +	 */
> +	/* Read the TEST_LANE_COUNT and TEST_LINK_RTAE fields (DP CTS 3.1.4) */
> +	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LANE_COUNT,
> +				  &intel_dp->compliance.test_lane_count);
> +
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Lane count read failed\n");
> +		return 0;
> +	}
> +	intel_dp->compliance.test_lane_count &= DP_MAX_LANE_COUNT_MASK;
> +
> +	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LINK_RATE,
> +				   &intel_dp->compliance.test_link_rate);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Link Rate read failed\n");
> +		return 0;
> +	}
> +
>  	return test_result;
>  }
>  
> @@ -4135,9 +4168,8 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  	if (!intel_dp->lane_count)
>  		return;
>  
> -	/* if link training is requested we should perform it always */
> -	if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) ||
> -	    (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
> +	/* Retrain if Channel EQ or CR not ok */
> +	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
>  		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>  			      intel_encoder->base.name);
>  
> @@ -4162,6 +4194,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  intel_dp_short_pulse(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>  	u8 sink_irq_vector = 0;
>  	u8 old_sink_count = intel_dp->sink_count;
>  	bool ret;
> @@ -4195,7 +4228,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  				   sink_irq_vector);
>  
>  		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
> -			DRM_DEBUG_DRIVER("Test request in short pulse not handled\n");
> +			intel_dp_handle_test_request(intel_dp);
>  		if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
>  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>  	}
> @@ -4203,6 +4236,11 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  	intel_dp_check_link_status(intel_dp);
>  	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
> +		DRM_DEBUG_KMS("Link Training Compliance Test requested\n");
> +		/* Send a Hotplug Uevent to userspace to start modeset */
> +		drm_kms_helper_hotplug_event(intel_encoder->base.dev);
> +	}
>  
>  	return true;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 84258df..7a2eb76 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -893,6 +893,8 @@ struct intel_dp_compliance {
>  	unsigned long test_type;
>  	struct intel_dp_compliance_data test_data;
>  	bool test_active;
> +	u8 test_link_rate;
> +	u8 test_lane_count;
>  };
>  
>  struct intel_dp {
> -- 
> 1.9.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/4] drm/i915: Add support for DP link training compliance

[Manasi Navare]

This patch adds support to handle automated DP compliance
link training test requests. This patch has been tested with
Unigraf DPR-120 DP Compliance device for testing Link
Training Compliance.
After we get a short pulse Compliance test request, test
request values are read and hotplug uevent is sent in order
to trigger another modeset during which the pipe is configured
and link is retrained and enabled for link parameters requested
by the test.

v2:
* Validate the test lane count before using it in
intel_dp_compute_config (Jani Nikula)
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 46 ++++++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 2 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e80d620..7a1d3c4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1613,6 +1613,7 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 	/* Conveniently, the link BW constants become indices with a shift...*/
 	int min_clock = 0;
 	int max_clock;
+	int link_rate_index;
 	int bpp, mode_rate;
 	int link_avail, link_clock;
 	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
@@ -1654,6 +1655,17 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
 		return false;
 
+	/* Use values requested by Compliance Test Request */
+	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
+		link_rate_index = intel_dp_link_rate_index(intel_dp,
+							   common_rates,
+							   drm_dp_bw_code_to_link_rate(intel_dp->compliance.test_link_rate));
+		if (link_rate_index >= 0)
+			min_clock = max_clock = link_rate_index;
+		if (min_lane_count <= intel_dp->compliance.test_lane_count
+		    && intel_dp->compliance.test_lane_count >= max_lane_count)
+			min_lane_count = max_lane_count = intel_dp->compliance.test_lane_count;
+	}
 	DRM_DEBUG_KMS("DP link computation with max lane count %i "
 		      "max bw %d pixel clock %iKHz\n",
 		      max_lane_count, common_rates[max_clock],
@@ -3921,6 +3933,27 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
 {
 	uint8_t test_result = DP_TEST_ACK;
+	int status = 0;
+	/* (DP CTS 1.2)
+	 * 4.3.1.11
+	 */
+	/* Read the TEST_LANE_COUNT and TEST_LINK_RTAE fields (DP CTS 3.1.4) */
+	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LANE_COUNT,
+				  &intel_dp->compliance.test_lane_count);
+
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Lane count read failed\n");
+		return 0;
+	}
+	intel_dp->compliance.test_lane_count &= DP_MAX_LANE_COUNT_MASK;
+
+	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LINK_RATE,
+				   &intel_dp->compliance.test_link_rate);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Link Rate read failed\n");
+		return 0;
+	}
+
 	return test_result;
 }
 
@@ -4135,9 +4168,8 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 	if (!intel_dp->lane_count)
 		return;
 
-	/* if link training is requested we should perform it always */
-	if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) ||
-	    (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
+	/* Retrain if Channel EQ or CR not ok */
+	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
 		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
 			      intel_encoder->base.name);
 
@@ -4162,6 +4194,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 intel_dp_short_pulse(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	u8 sink_irq_vector = 0;
 	u8 old_sink_count = intel_dp->sink_count;
 	bool ret;
@@ -4195,7 +4228,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 				   sink_irq_vector);
 
 		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
-			DRM_DEBUG_DRIVER("Test request in short pulse not handled\n");
+			intel_dp_handle_test_request(intel_dp);
 		if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
@@ -4203,6 +4236,11 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 	intel_dp_check_link_status(intel_dp);
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
+	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
+		DRM_DEBUG_KMS("Link Training Compliance Test requested\n");
+		/* Send a Hotplug Uevent to userspace to start modeset */
+		drm_kms_helper_hotplug_event(intel_encoder->base.dev);
+	}
 
 	return true;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 84258df..7a2eb76 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -893,6 +893,8 @@ struct intel_dp_compliance {
 	unsigned long test_type;
 	struct intel_dp_compliance_data test_data;
 	bool test_active;
+	u8 test_link_rate;
+	u8 test_lane_count;
 };
 
 struct intel_dp {
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx