[intel-gfx] Displayport Compliance Testing V2

[intel-gfx] Displayport Compliance Testing V2

[Todd Previte]

This is version 2 of the patch set for Displayport compliance testing support 
in the i915 driver. This implementation of compliance testing conforms to the 
VESA specification Displayport Link Compliance Testing Specification (Link CTS)
1.2 Core Rev1.1. The code has been partitioned into two segments - the kernel 
side comprised of this patch set and a userspace application that handles the 
heavy lifting for necessary mode sets. It can also handle link configuration
changes if necessary.

Overview Changes for V2:
- Removed the kernel<->userpsace signal mechanism for communication during testing. The
  original code used signals for communication between the kernel and userspace which met
  with significant resistance during review. That mechanism has been removed in favor of
  a file polling mechanism that contains the necessary data for the user app to properly
  communicate with the kernel during compliance testing.
- Review feedback from as far back as July has been rolled up and integrated into this
  patch set where necessary/applicable. For those cases, the feedback has been directly
  noted in the commit message
- Some patches have been either moved or deleted from the set as they have gone through
  the review cycle. Where this has occurred, it has been noted in the commit message.


drm/i915: Add debugfs write and test param parsing function for DP test control
drm/i915: Add new debugfs file for Displaypor compliance test control
drm/i915: Update debugfs functions for Displayport compliance testing
drm/i915: Add debugfs function to check connector status for compliance testing
drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation
drm/i915: Update intel_dp_compute_config() to handle compliance test requests
drm/i915: Update the EDID automated compliance test function
drm/i915: Implement the 'open' and 'write' debugfs functions for Displayport compliance
drm/i915: Add config parsing utilities in debugfs for Displayport compliance
drm/i915: Add Displayport link configuration structure
drm/i915: Add and implement the debugfs 'show' functions for Displayport compliance
drm/i915: Add support functions in debugfs for handling Displayport compliance configuration
drm/i915: Add file ops for Displayport configuration in debugfs
drm/i915: Add debugfs information for Displayport compliance testing
drm/i915: Add a delay in Displayport AUX transactions for compliance testing
drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing
drm/i915: Add automated testing support for Displayport compliance testing

 drivers/gpu/drm/i915/i915_debugfs.c | 569 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c     | 219 ++++++++++--
 drivers/gpu/drm/i915/intel_drv.h    |  19 ++
 3 files changed, 779 insertions(+), 28 deletions(-)

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

[PATCH 01/17] drm/i915: Add automated testing support for Displayport compliance testing

[Todd Previte]

Add the skeleton framework for supporting automation for Displayport compliance
testing. This patch adds the necessary framework for the source device to
appropriately respond to test automation requests from a sink device.

V2:
- Addressed previous mailing list feedback
- Fixed compilation issue (struct members declared in a later patch)
- Updated debug messages to be more accurate
- Added status checks for the DPCD read/write calls
- Removed excess comments and debug messages
- Fixed debug message compilation warnings
- Fixed compilation issue with missing variables
- Updated link training autotest to ACK

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 72 +++++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h |  4 +++
 2 files changed, 72 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3fc3296..3dc92a3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3744,11 +3744,75 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 	return true;
 }
 
-static void
-intel_dp_handle_test_request(struct intel_dp *intel_dp)
+static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_ACK;
+	return test_result;
+}
+
+static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_NAK;
+	return test_result;
+}
+
+static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_NAK;
+	return test_result;
+}
+
+static uint8_t intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_NAK;
+	return test_result;
+}
+
+static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 {
-	/* NAK by default */
-	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK);
+	uint8_t response = DP_TEST_NAK;
+	uint8_t rxdata = 0;
+	int status = 0;
+
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
+	if (status != 0) {
+		response = DP_TEST_NAK;
+		DRM_DEBUG_KMS("Could not read test request from sink\n");
+		return;
+	}
+
+	switch (rxdata) {
+	case DP_TEST_LINK_TRAINING:
+		DRM_DEBUG_KMS("LINK_TRAINING test requested\n");
+		response = intel_dp_autotest_link_training(intel_dp);
+		break;
+	case DP_TEST_LINK_VIDEO_PATTERN:
+		DRM_DEBUG_KMS("TEST_PATTERN test requested\n");
+		response = intel_dp_autotest_video_pattern(intel_dp);
+		break;
+	case DP_TEST_LINK_EDID_READ:
+		DRM_DEBUG_KMS("EDID test requested\n");
+		response = intel_dp_autotest_edid(intel_dp);
+		break;
+	case DP_TEST_LINK_PHY_TEST_PATTERN:
+		DRM_DEBUG_KMS("PHY_PATTERN test requested\n");
+		response = intel_dp_autotest_phy_pattern(intel_dp);
+		break;
+		/* FAUX is optional in DP 1.2*/
+	case DP_TEST_LINK_FAUX_PATTERN:
+		DRM_DEBUG_KMS("FAUX_PATTERN testing not supported\n");
+		break;
+	default:
+		DRM_DEBUG_KMS("Invalid test request '%02x'\n", rxdata);
+		break;
+	}
+	status = drm_dp_dpcd_write(&intel_dp->aux,
+				   DP_TEST_RESPONSE,
+				   &response, 1);
+	if (status != 0)
+		DRM_DEBUG_KMS("Could not write test response to sink\n");
+
+	intel_dp->compliance_testing_active = 0;
 }
 
 static int
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 588b618..d1a807a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -638,6 +638,10 @@ struct intel_dp {
 		struct mutex mutex;
 	} drrs_state;
 
+	/* Displayport compliance testing */
+	unsigned long compliance_test_data;
+	bool compliance_testing_active;
+
 };
 
 struct intel_digital_port {
-- 
1.9.1

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

[PATCH 02/17] drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing

[Todd Previte]

Move the DPCD read to the top and check for an interrupt from the sink to catch
Displayport automated testing requests necessary to support Displayport compliance
testing. The checks for active connectors and link status are moved below the
check for the interrupt.

Adds a check at the top to verify that the device is connected. This is necessary
for DP compliance testing to ensure that test requests are captured and acknowledged.
If a test request is present during a connected->disconnected transition,
the test code will attempt to execute even though the connection has been disabled,
resulting in a faied test.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3dc92a3..1b452cc 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3890,21 +3890,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	if (!intel_encoder->connectors_active)
-		return;
-
-	if (WARN_ON(!intel_encoder->base.crtc))
-		return;
-
-	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
-		return;
-
-	/* Try to read receiver status if the link appears to be up */
-	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+	/* Bail if not connected */
+	if (intel_dp->attached_connector->base.status !=
+	    connector_status_connected) {
+		DRM_DEBUG_KMS("Not connected\n");
 		return;
 	}
 
-	/* Now read the DPCD to see if it's actually running */
+	/* Attempt to read the DPCD */
 	if (!intel_dp_get_dpcd(intel_dp)) {
 		return;
 	}
@@ -3916,13 +3909,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 		drm_dp_dpcd_writeb(&intel_dp->aux,
 				   DP_DEVICE_SERVICE_IRQ_VECTOR,
 				   sink_irq_vector);
-
 		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
 			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");
 	}
 
+	if (!intel_encoder->connectors_active)
+		return;
+
+	if (WARN_ON(!intel_encoder->base.crtc))
+		return;
+
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+		return;
+
+	/* Try to read receiver status if the link appears to be up */
+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+		return;
+	}
+
 	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);
-- 
1.9.1

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

[PATCH 03/17] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Todd Previte]

The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
specifies that repeated AUX transactions after a failure (no response /
invalid response) must have a minimum delay of 400us before the resend can
occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.

V2:
- Changed udelay() to usleep_range()

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1b452cc..b6f5a72 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -876,9 +876,14 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
 				   DP_AUX_CH_CTL_RECEIVE_ERROR);
 
+			/* DP compliance requires 400us delay for errors
+			   and timeouts (DP CTS 1.2 Core Rev 1.1, 4.2.1.1
+			   & 4.2.1.2) */
 			if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
-				      DP_AUX_CH_CTL_RECEIVE_ERROR))
+				      DP_AUX_CH_CTL_RECEIVE_ERROR)) {
+				usleep_range(400, 500);
 				continue;
+			}
 			if (status & DP_AUX_CH_CTL_DONE)
 				break;
 		}
-- 
1.9.1

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

[PATCH 04/17] drm/i915: Add debugfs information for Displayport compliance testing

[Todd Previte]

This patch was part of "[PATCH 05/10] drm/i915: Add debugfs interface for
Displayport debug and compliance testing". That patch has been split into
smaller patches for ease of review and integration.

This patch contains the definitions/declarations for some of the constants
and data structures added to support debugfs output for Displayport compliance
testing.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 41 +++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 479e0c1..65b4f5e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -33,6 +33,7 @@
 #include <linux/slab.h>
 #include <linux/export.h>
 #include <linux/list_sort.h>
+#include <linux/string.h>
 #include <asm/msr-index.h>
 #include <drm/drmP.h>
 #include "intel_drv.h"
@@ -51,6 +52,46 @@ static const char *yesno(int v)
 	return v ? "yes" : "no";
 }
 
+#define DP_PARAMETER_COUNT		8
+#define MAX_DP_CONFIG_LINE_COUNT	64
+
+enum dp_config_param {
+	DP_CONFIG_PARAM_INVALID = -1,
+	DP_CONFIG_PARAM_CONNECTOR = 0,
+	DP_CONFIG_PARAM_LINK_RATE,
+	DP_CONFIG_PARAM_LANE_COUNT,
+	DP_CONFIG_PARAM_VOLTAGE_SWING,
+	DP_CONFIG_PARAM_PREEMPHASIS,
+	DP_CONFIG_PARAM_HRES,
+	DP_CONFIG_PARAM_VRES,
+	DP_CONFIG_PARAM_BPP
+};
+
+struct dp_config {
+	enum dp_config_param type;
+	unsigned long value;
+};
+
+const char *dp_conf_tokens[DP_PARAMETER_COUNT] = {
+		 "Connector",
+		 "Link Rate",
+		 "Lane Count",
+		 "Voltage Swing Level",
+		 "Preemphasis Level",
+		 "Horizontal Resolution",
+		 "Vertical Resolution",
+		 "Bits Per Pixel"
+};
+/* Strings for writing out dp_configs */
+#define DP_CONF_STR_CONNECTOR	"Connector:             %s\n"
+#define DP_CONF_STR_LINKRATE	"Link Rate:             %02x\n"
+#define DP_CONF_STR_LANES	"Lane Count:            %u\n"
+#define DP_CONF_STR_VSWING	"Voltage Swing Level:   %u\n"
+#define DP_CONF_STR_PREEMP	"Preemphasis Level:     %u\n"
+#define DP_CONF_STR_HRES	"Horizontal Resolution: %d\n"
+#define DP_CONF_STR_VRES	"Vertical Resolution:   %d\n"
+#define DP_CONF_STR_BPP		"Bits Per Pixel:        %u\n"
+
 /* As the drm_debugfs_init() routines are called before dev->dev_private is
  * allocated we need to hook into the minor for release. */
 static int
-- 
1.9.1

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

[PATCH 05/17] drm/i915: Add file ops for Displayport configuration in debugfs

[Todd Previte]

This patch was also part of "[PATCH 05/10] drm/i915: Add debugfs interface
for Displayport debug and compliance testing". Adds file operations structures
for Displayport configuration in debugfs for compliance testing. Also adds
the declarations for the open() and write() functions listed in the file ops
structure.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 65b4f5e..feae100 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3694,6 +3694,28 @@ static const struct file_operations i915_display_crc_ctl_fops = {
 	.write = display_crc_ctl_write
 };
 
+static int displayport_config_ctl_open(struct inode *inode,
+				       struct file *file)
+{
+	return 0;
+}
+
+static ssize_t displayport_config_ctl_write(struct file *file,
+					    const char __user *ubuf,
+					    size_t len, loff_t *offp)
+{
+	return 0;
+}
+
+static const struct file_operations i915_displayport_config_ctl_fops = {
+	.owner = THIS_MODULE,
+	.open = displayport_config_ctl_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+	.write = displayport_config_ctl_write
+};
+
 static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
 {
 	struct drm_device *dev = m->private;
@@ -4446,6 +4468,7 @@ static const struct i915_debugfs_files {
 	{"i915_spr_wm_latency", &i915_spr_wm_latency_fops},
 	{"i915_cur_wm_latency", &i915_cur_wm_latency_fops},
 	{"i915_fbc_false_color", &i915_fbc_fc_fops},
+	{"i915_displayport_config_ctl", &i915_displayport_config_ctl_fops}
 };
 
 void intel_display_crc_init(struct drm_device *dev)
-- 
1.9.1

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

[PATCH 06/17] drm/i915: Add support functions in debugfs for handling Displayport compliance configuration

[Todd Previte]

This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs interface
for Displayport debug  and compliance testing". Adds two support functions for
handling Displayport configuration parameters that are used for compliance
testing.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 48 +++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index feae100..ce091c1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3694,6 +3694,54 @@ static const struct file_operations i915_display_crc_ctl_fops = {
 	.write = display_crc_ctl_write
 };
 
+enum dp_config_param displayport_get_config_param_type(char *input_string)
+{
+	enum dp_config_param param_type = DP_CONFIG_PARAM_INVALID;
+	int i = 0;
+
+	for (i = 0; i < DP_PARAMETER_COUNT; i++) {
+		if (strncmp(input_string, dp_conf_tokens[i],
+			    strlen(dp_conf_tokens[i])) == 0) {
+			param_type = (enum dp_config_param) i;
+			break;
+		}
+	}
+	return param_type;
+}
+
+int value_for_config_param(enum dp_config_param param,
+			   char *input_string,
+			   void *output_value)
+{
+	int status = 0;
+	unsigned int *out_ptr = (unsigned int *)output_value;
+	char *index = input_string;
+
+	switch (param) {
+	case DP_CONFIG_PARAM_CONNECTOR:
+		output_value = (char *)index;
+		break;
+	case DP_CONFIG_PARAM_LINK_RATE:
+	case DP_CONFIG_PARAM_LANE_COUNT:
+	case DP_CONFIG_PARAM_VOLTAGE_SWING:
+	case DP_CONFIG_PARAM_PREEMPHASIS:
+		status = kstrtol(index, 16, (long *)out_ptr);
+		break;
+	case DP_CONFIG_PARAM_HRES:
+	case DP_CONFIG_PARAM_VRES:
+	case DP_CONFIG_PARAM_BPP:
+		status = kstrtol(index, 10, (long *)out_ptr);
+		break;
+	default:
+		/* Unhandled case */
+		status = -EINVAL;
+		*out_ptr = 0;
+		break;
+	}
+
+	return status;
+}
+
 static int displayport_config_ctl_open(struct inode *inode,
 				       struct file *file)
 {
-- 
1.9.1

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

[PATCH 07/17] drm/i915: Add and implement the debugfs 'show' functions for Displayport compliance

[Todd Previte]

This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs
interface for Displayport debug and compliance testing". This patch implements
the 'show' functions for the debugfs interface for Displayport compliance
testing.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 85 +++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index ce091c1..184797d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3694,6 +3694,56 @@ static const struct file_operations i915_display_crc_ctl_fops = {
 	.write = display_crc_ctl_write
 };
 
+static void displayport_show_config_info(struct seq_file *m,
+			  struct intel_connector *intel_connector)
+{
+	struct intel_encoder *intel_encoder = intel_connector->encoder;
+	struct intel_dp *intel_dp;
+	struct intel_crtc *icrtc;
+	struct intel_crtc_config *crtc_config;
+	char *conn_name = intel_connector->base.name;
+	int pipe_bpp, hres, vres;
+	uint8_t vs[4], pe[4];
+	struct drm_display_mode *mode;
+	int i = 0;
+
+	if (intel_encoder) {
+		intel_dp = enc_to_intel_dp(&intel_encoder->base);
+		for (i = 0; i < intel_dp->lane_count; i++) {
+			vs[i] = intel_dp->train_set[i]
+				& DP_TRAIN_VOLTAGE_SWING_MASK;
+			pe[i] = (intel_dp->train_set[i]
+			      & DP_TRAIN_PRE_EMPHASIS_MASK) >> 3;
+		}
+		if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) {
+			if (intel_encoder->new_crtc) {
+				crtc_config = &intel_encoder->new_crtc->config;
+				pipe_bpp = crtc_config->pipe_bpp;
+				mode = &crtc_config->adjusted_mode;
+				hres = mode->hdisplay;
+				vres = mode->vdisplay;
+			} else if (intel_encoder->base.crtc) {
+				icrtc = to_intel_crtc(intel_encoder->base.crtc);
+				pipe_bpp = icrtc->config.pipe_bpp;
+				mode = &icrtc->config.adjusted_mode;
+				hres = mode->hdisplay;
+				vres = mode->vdisplay;
+			} else {
+				pipe_bpp = 0;
+				hres = vres = 0;
+			}
+			seq_printf(m, DP_CONF_STR_CONNECTOR, conn_name);
+			seq_printf(m, DP_CONF_STR_LINKRATE, intel_dp->link_bw);
+			seq_printf(m, DP_CONF_STR_LANES, intel_dp->lane_count);
+			seq_printf(m, DP_CONF_STR_VSWING, vs[0]);
+			seq_printf(m, DP_CONF_STR_PREEMP, pe[0]);
+			seq_printf(m, DP_CONF_STR_HRES, hres);
+			seq_printf(m, DP_CONF_STR_VRES, vres);
+			seq_printf(m, DP_CONF_STR_BPP, pipe_bpp);
+		}
+	}
+}
+
 enum dp_config_param displayport_get_config_param_type(char *input_string)
 {
 	enum dp_config_param param_type = DP_CONFIG_PARAM_INVALID;
@@ -3742,6 +3792,41 @@ int value_for_config_param(enum dp_config_param param,
 	return status;
 }
 
+static int displayport_config_ctl_show(struct seq_file *m, void *data)
+{
+	struct drm_device *dev = m->private;
+	struct drm_connector *drm_connector;
+	struct intel_encoder *encoder;
+	struct intel_connector *connector;
+
+	if (!dev)
+		return -ENODEV;
+
+	list_for_each_entry(drm_connector,
+			    &dev->mode_config.connector_list,
+			    head) {
+		connector = to_intel_connector(drm_connector);
+		encoder = connector->encoder;
+		if (drm_connector->connector_type ==
+		    DRM_MODE_CONNECTOR_DisplayPort) {
+			if (encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
+			    encoder->type == INTEL_OUTPUT_UNKNOWN) {
+				if (drm_connector->status ==
+				    connector_status_connected) {
+					displayport_show_config_info(m,
+								     connector);
+				} else {
+					seq_printf(m, DP_CONF_STR_CONNECTOR,
+						   encoder->base.name);
+					seq_puts(m, "disconnected\n");
+				}
+			}
+		}
+	}
+
+	return 0;
+}
+
 static int displayport_config_ctl_open(struct inode *inode,
 				       struct file *file)
 {
-- 
1.9.1

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

[PATCH 08/17] drm/i915: Add Displayport link configuration structure

[Todd Previte]

This patch was previously part of "[PATCH 07/10] drm/i915: Add structures for
Displayport compliance testing parameters". Adds a struct to maintain link
configuration data for Displayport compliance testing. The members added to
the intel_dp struct are for compliance testing purposes only and should not
be used during normal operations.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d1a807a..5a8b1d6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -577,6 +577,18 @@ enum edp_drrs_refresh_rate_type {
 	DRRS_MAX_RR, /* RR count */
 };
 
+struct intel_dp_link_config {
+	uint8_t lane_count;
+	uint8_t link_rate;
+	uint8_t vswing_level;
+	uint8_t preemp_level;
+	uint32_t hres;
+	uint32_t vres;
+	uint8_t bits_per_pixel;
+	struct drm_display_mode *compliance_mode;
+	char connector_name[12];
+};
+
 struct intel_dp {
 	uint32_t output_reg;
 	uint32_t aux_ch_ctl_reg;
@@ -641,6 +653,8 @@ struct intel_dp {
 	/* Displayport compliance testing */
 	unsigned long compliance_test_data;
 	bool compliance_testing_active;
+	struct intel_dp_link_config compliance_config;
+	struct intel_dp_link_config saved_config;
 
 };
 
-- 
1.9.1

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

[PATCH 09/17] drm/i915: Add config parsing utilities in debugfs for Displayport compliance

[Todd Previte]

This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs
interface for Displayport debug and compliance testing". This patch adds two
functions to handle parsing of Displayport configuration information as it
formatted in the debugfs file. It is used to process incoming configuration
changes from the userspace compliance application during testing.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 128 ++++++++++++++++++++++++++++++++++++
 1 file changed, 128 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 184797d..55b6da5 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3792,6 +3792,134 @@ int value_for_config_param(enum dp_config_param param,
 	return status;
 }
 
+int tokenize_dp_config(char *input_buffer, char *output[])
+{
+	char *base = input_buffer, *index, *end;
+	int line_count = 0;
+	int i = 0, len = 0;
+	int done = 0;
+
+	if (!input_buffer)
+		return 0;
+
+	while (!done) {
+		index = strpbrk(base, ":");
+		if (index) {
+			len = index - base;
+			*index = '\0';
+			index++;
+			/* Save the type string */
+			output[i] = base;
+			i++;
+			line_count++;
+			end = strpbrk(index, "\n\0");
+			if (end) {
+				*end = '\0';
+				/* Eat up whitespace */
+				while (*index <= 0x20)
+					index++;
+				output[i] = index;
+				i++;
+				line_count++;
+			} else
+				done = 1;
+			/* Move to the next section of the string */
+			base = end + 1;
+		} else
+			done = 1;
+	}
+	return line_count;
+}
+
+static int displayport_parse_config(char *input_buffer,
+				    ssize_t buffer_size,
+				    struct intel_dp *intel_dp)
+{
+	int status = 0;
+	char *lines[MAX_DP_CONFIG_LINE_COUNT];
+	int i = 0;
+	struct dp_config parms[DP_PARAMETER_COUNT];
+	int line_count = 0;
+	char *buffer = input_buffer;
+	enum dp_config_param parm_type;
+	unsigned long parm_value;
+
+	line_count = tokenize_dp_config(buffer, lines);
+
+	if (line_count == 0) {
+		DRM_DEBUG_DRIVER("No lines to process\n");
+		return 0;
+	}
+
+	for (i = 0; i < line_count; i += 2) {
+		parm_type = displayport_get_config_param_type(lines[i]);
+		if (parm_type != DP_CONFIG_PARAM_INVALID) {
+			status = value_for_config_param(parm_type,
+							lines[i+1],
+							&parm_value);
+			if (status == 0) {
+				parms[parm_type].type = parm_type;
+				parms[parm_type].value = parm_value;
+			}
+		}
+	}
+
+	if (parms[DP_CONFIG_PARAM_LINK_RATE].value == 0x06 ||
+	    parms[DP_CONFIG_PARAM_LINK_RATE].value == 0x0a ||
+	    parms[DP_CONFIG_PARAM_LINK_RATE].value == 0x14) {
+		intel_dp->compliance_config.link_rate =
+			parms[DP_CONFIG_PARAM_LINK_RATE].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_LANE_COUNT].value == 0x01 ||
+	    parms[DP_CONFIG_PARAM_LANE_COUNT].value == 0x02 ||
+	    parms[DP_CONFIG_PARAM_LANE_COUNT].value == 0x04) {
+		intel_dp->compliance_config.lane_count =
+			parms[DP_CONFIG_PARAM_LANE_COUNT].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_VOLTAGE_SWING].value <= 0x03 &&
+	    parms[DP_CONFIG_PARAM_VOLTAGE_SWING].value >= 0x00) {
+		intel_dp->compliance_config.vswing_level =
+			parms[DP_CONFIG_PARAM_VOLTAGE_SWING].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_PREEMPHASIS].value <= 0x03 &&
+	    parms[DP_CONFIG_PARAM_PREEMPHASIS].value >= 0x00) {
+		intel_dp->compliance_config.preemp_level =
+			parms[DP_CONFIG_PARAM_PREEMPHASIS].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_BPP].value == 18 ||
+	    parms[DP_CONFIG_PARAM_BPP].value == 24 ||
+	    parms[DP_CONFIG_PARAM_BPP].value == 30 ||
+	    parms[DP_CONFIG_PARAM_BPP].value == 36) {
+		intel_dp->compliance_config.bits_per_pixel =
+			parms[DP_CONFIG_PARAM_PREEMPHASIS].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_HRES].value > 0 &&
+	    parms[DP_CONFIG_PARAM_HRES].value <= 8192) {
+		intel_dp->compliance_config.hres =
+			parms[DP_CONFIG_PARAM_HRES].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_VRES].value > 0 &&
+	    parms[DP_CONFIG_PARAM_VRES].value <= 8192) {
+		intel_dp->compliance_config.vres =
+			parms[DP_CONFIG_PARAM_VRES].value;
+	} else
+		return -EINVAL;
+
+	return status;
+}
+
 static int displayport_config_ctl_show(struct seq_file *m, void *data)
 {
 	struct drm_device *dev = m->private;
-- 
1.9.1

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

[PATCH 10/17] drm/i915: Implement the 'open' and 'write' debugfs functions for Displayport compliance

[Todd Previte]

This patch is a combination of sections out of the following two previous
patches:
[PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and
compliance testing
[PATCH 07/10] drm/i915: Add structures for Displayport compliance testing
parameters

This patch implements the debugfs functions for 'open' and 'write' as they
are specified in the file ops structure. The 'open' function outputs
Displayport data to the appropriate debugfs file while the 'write' function
handles parsing of user data written to that same file.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 66 +++++++++++++++++++++++++++++++++++--
 1 file changed, 64 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 55b6da5..5eb6c24 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3958,14 +3958,76 @@ static int displayport_config_ctl_show(struct seq_file *m, void *data)
 static int displayport_config_ctl_open(struct inode *inode,
 				       struct file *file)
 {
-	return 0;
+	struct drm_device *dev = inode->i_private;
+
+	return single_open(file, displayport_config_ctl_show, dev);
 }
 
 static ssize_t displayport_config_ctl_write(struct file *file,
 					    const char __user *ubuf,
 					    size_t len, loff_t *offp)
 {
-	return 0;
+	char *input_buffer;
+	int status = 0;
+	struct seq_file *m;
+	struct drm_device *dev;
+	struct drm_connector *connector;
+	struct drm_encoder *drm_enc;
+	struct intel_encoder *intel_encoder;
+	struct intel_connector *intel_connector;
+	struct intel_dp *intel_dp;
+
+	m = file->private_data;
+	if (!m) {
+		status = -ENODEV;
+		return status;
+	}
+	dev = m->private;
+
+	if (!dev) {
+		status = -ENODEV;
+		return status;
+	}
+
+	if (len == 0)
+		return 0;
+
+	input_buffer = kmalloc(len + 1, GFP_KERNEL);
+	if (!input_buffer)
+		return -ENOMEM;
+
+	if (copy_from_user(input_buffer, ubuf, len)) {
+		status = -EFAULT;
+		goto out;
+	}
+
+	input_buffer[len] = '\0';
+	DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+		intel_connector = to_intel_connector(connector);
+		intel_encoder = intel_connector->encoder;
+		drm_enc = &intel_encoder->base;
+		if (connector->connector_type ==
+		    DRM_MODE_CONNECTOR_DisplayPort) {
+			if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
+			    intel_encoder->type == INTEL_OUTPUT_UNKNOWN) {
+				intel_dp = enc_to_intel_dp(drm_enc);
+				status = displayport_parse_config(input_buffer,
+								  len,
+								  intel_dp);
+				if (status < 0)
+					goto out;
+			}
+		}
+	}
+out:
+	kfree(input_buffer);
+	if (status < 0)
+		return status;
+
+	*offp += len;
+	return len;
 }
 
 static const struct file_operations i915_displayport_config_ctl_fops = {
-- 
1.9.1

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

[PATCH 11/17] drm/i915: Update the EDID automated compliance test function

[Todd Previte]

Updates the EDID compliance test function to perform the EDID read as
required by the tests. This read needs to take place in the kernel for
reasons of speed and efficiency. The results of the EDID read are handed
off to userspace so that the remainder of the test can be conducted there.

V2:
- Addressed mailing list feedback
- Removed excess debug messages
- Removed extraneous comments
- Fixed formatting issues (line length > 80)
- Updated the debug message in compute_edid_checksum to output hex values
  instead of decimal

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b6f5a72..2a13124 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -40,6 +40,13 @@
 
 #define DP_LINK_CHECK_TIMEOUT	(10 * 1000)
 
+/* Compliance test status bits  */
+#define  INTEL_DP_EDID_OK		(0<<0)
+#define  INTEL_DP_EDID_CORRUPT		(1<<0)
+#define  INTEL_DP_RESOLUTION_PREFERRED	(1<<2)
+#define  INTEL_DP_RESOLUTION_STANDARD	(1<<3)
+#define  INTEL_DP_RESOLUTION_FAILSAFE	(1<<4)
+
 struct dp_link_dpll {
 	int link_bw;
 	struct dpll dpll;
@@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 	return test_result;
 }
 
+static bool intel_dp_compute_edid_checksum(uint8_t *edid_data,
+					   uint8_t *edid_checksum)
+{
+	uint32_t byte_total = 0;
+	uint8_t i = 0;
+	bool edid_ok = true;
+
+	/* Don't include last byte (the checksum) in the computation */
+	for (i = 0; i < EDID_LENGTH - 2; i++)
+		byte_total += edid_data[i];
+
+	*edid_checksum = 256 - (byte_total % 256);
+
+	if (*edid_checksum != edid_data[EDID_LENGTH - 1]) {
+		DRM_DEBUG_KMS("Invalid EDID checksum %02x, should be %02x\n",
+			      edid_data[EDID_LENGTH - 40 - 1], *edid_checksum);
+		edid_ok = false;
+	}
+
+	return edid_ok;
+}
+
 static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
 {
-	uint8_t test_result = DP_TEST_NAK;
+	struct drm_connector *connector = &intel_dp->attached_connector->base;
+	struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+	struct edid *edid_read = NULL;
+	uint8_t *edid_data = NULL;
+	uint8_t test_result = DP_TEST_NAK, checksum = 0;
+	uint32_t ret = 0;
+
+	intel_dp->aux.i2c_nack_count = 0;
+	intel_dp->aux.i2c_defer_count = 0;
+
+	edid_read = drm_get_edid(connector, adapter);
+
+	if (edid_read == NULL) {
+		/* Check for NACKs/DEFERs, use failsafe if detected
+		   (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
+		if (intel_dp->aux.i2c_nack_count > 0 ||
+			intel_dp->aux.i2c_defer_count > 0)
+			DRM_DEBUG_KMS("EDID read had %d NACKs, %d DEFERs\n",
+				      intel_dp->aux.i2c_nack_count,
+				      intel_dp->aux.i2c_defer_count);
+		intel_dp->compliance_test_data = INTEL_DP_EDID_CORRUPT |
+						 INTEL_DP_RESOLUTION_FAILSAFE;
+	} else {
+		edid_data = (uint8_t *) edid_read;
+
+		if (intel_dp_compute_edid_checksum(edid_data, &checksum)) {
+			ret = drm_dp_dpcd_write(&intel_dp->aux,
+						DP_TEST_EDID_CHECKSUM,
+						&edid_read->checksum, 1);
+			test_result = DP_TEST_ACK |
+				      DP_TEST_EDID_CHECKSUM_WRITE;
+			intel_dp->compliance_test_data =
+				INTEL_DP_EDID_OK |
+				INTEL_DP_RESOLUTION_PREFERRED;
+		} else {
+			/* Invalid checksum - EDID corruption detection */
+			intel_dp->compliance_test_data =
+				INTEL_DP_EDID_CORRUPT |
+				INTEL_DP_RESOLUTION_FAILSAFE;
+		}
+	}
+
 	return test_result;
 }
 
-- 
1.9.1

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

[PATCH 12/17] drm/i915: Update intel_dp_compute_config() to handle compliance test requests

[Todd Previte]

Adds provisions in intel_dp_compute_config() to accommodate compliance
testing. Mostly this invovles circumventing the automatic link configuration
parameters and allowing the compliance code to set those parameters as
required by the tests.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2a13124..4a55ca6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1189,6 +1189,21 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	pipe_config->has_drrs = false;
 	pipe_config->has_audio = intel_dp->has_audio;
 
+	/* Compliance testing should skip most of this function */
+	if (!is_edp(intel_dp) && intel_dp->compliance_testing_active) {
+		bpp = intel_dp->compliance_config.bits_per_pixel;
+		lane_count = intel_dp->compliance_config.lane_count;
+		clock = intel_dp->compliance_config.link_rate >> 3;
+		/* Assign here and skip at the end - ensures correct values */
+		intel_dp->link_bw = bws[clock];
+		intel_dp->lane_count = lane_count;
+		pipe_config->pipe_bpp = bpp;
+		pipe_config->port_clock =
+			drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
+
+		goto compliance_exit;
+	}
+
 	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
 		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
 				       adjusted_mode);
@@ -1275,6 +1290,7 @@ found:
 	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
 		      mode_rate, link_avail);
 
+compliance_exit:
 	intel_link_compute_m_n(bpp, lane_count,
 			       adjusted_mode->crtc_clock,
 			       pipe_config->port_clock,
-- 
1.9.1

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

[PATCH 13/17] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation

[Todd Previte]

Moves the non-MST case out of the if-statement and places it at the beginning
of the function to handle HPD events for SST mode. The reasoning behind this
is to accommodate link status checks for compliance testing. Some test devices
use long pulses to perform test requests so link status must be checked
regardless of the pulse width for the SST operational mode.

This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
previous compliance testing patch sequence. Review feedback on that patch
indicated that updating intel_dp_hot_plug() was not the correct place for
the test handler.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 4a55ca6..73014d8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
+	if (!intel_dp->is_mst) {
+		/*
+		 *  Pulse width doesn't matter for SST mode
+		 *  Handle the HPD event now
+		*/
+		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+		intel_dp_check_link_status(intel_dp);
+		drm_modeset_unlock(&dev->mode_config.connection_mutex);
+		ret = false;
+		goto put_power;
+	}
+
 	if (long_hpd) {
 
 		if (HAS_PCH_SPLIT(dev)) {
@@ -4637,16 +4649,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
 				goto mst_fail;
 		}
-
-		if (!intel_dp->is_mst) {
-			/*
-			 * we'll check the link status via the normal hot plug path later -
-			 * but for short hpds we should check it now
-			 */
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-		}
 	}
 	ret = false;
 	goto put_power;
-- 
1.9.1

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

[PATCH 14/17] drm/i915: Add debugfs function to check connector status for compliance testing

[Todd Previte]

Adds a function to check the status of a Displayport connector. This function
simplifies the Displayport compliance testing interface in debugfs by providing
a single method for determining the status of a connector during Displayport
compliance testing. This function checks whether or not the connector is a
Displayport connector and whether or not that connector is active.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5eb6c24..978ddd1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3920,6 +3920,35 @@ static int displayport_parse_config(char *input_buffer,
 	return status;
 }
 
+static bool dp_connector_is_valid(struct drm_connector *connector,
+				  bool check_active)
+{
+	struct intel_encoder *intel_encoder;
+	struct intel_connector *intel_connector;
+
+	intel_connector = to_intel_connector(connector);
+	intel_encoder = intel_connector->encoder;
+
+	if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort)
+		goto no;
+
+	if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT &&
+	    intel_encoder->type != INTEL_OUTPUT_UNKNOWN)
+		goto no;
+
+	if (check_active) {
+		if (connector->status == connector_status_connected)
+			goto yes;
+		else
+			goto no;
+	}
+
+yes:
+	return true;
+no:
+	return false;
+}
+
 static int displayport_config_ctl_show(struct seq_file *m, void *data)
 {
 	struct drm_device *dev = m->private;
-- 
1.9.1

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

[PATCH 15/17] drm/i915: Update debugfs functions for Displayport compliance testing

[Todd Previte]

Updates displayport_config_ctl_write and displayport_config_ctl_show to
use the dp_connector_is_valid() function. This saves on code and improves
maintainability by unifying the code in a single, common path.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 42 +++++++++++++------------------------
 1 file changed, 14 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 978ddd1..846b2fb 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3955,6 +3955,7 @@ static int displayport_config_ctl_show(struct seq_file *m, void *data)
 	struct drm_connector *drm_connector;
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
+	char *conn_name;
 
 	if (!dev)
 		return -ENODEV;
@@ -3964,23 +3965,14 @@ static int displayport_config_ctl_show(struct seq_file *m, void *data)
 			    head) {
 		connector = to_intel_connector(drm_connector);
 		encoder = connector->encoder;
-		if (drm_connector->connector_type ==
-		    DRM_MODE_CONNECTOR_DisplayPort) {
-			if (encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
-			    encoder->type == INTEL_OUTPUT_UNKNOWN) {
-				if (drm_connector->status ==
-				    connector_status_connected) {
-					displayport_show_config_info(m,
-								     connector);
-				} else {
-					seq_printf(m, DP_CONF_STR_CONNECTOR,
-						   encoder->base.name);
-					seq_puts(m, "disconnected\n");
-				}
-			}
+		conn_name = connector->base.name;
+		if (dp_connector_is_valid(drm_connector, 1)) {
+			displayport_show_config_info(m, connector);
+		} else {
+			seq_printf(m, DP_CONF_STR_CONNECTOR, conn_name);
+			seq_puts(m, "disconnected\n");
 		}
 	}
-
 	return 0;
 }
 
@@ -4001,7 +3993,6 @@ static ssize_t displayport_config_ctl_write(struct file *file,
 	struct seq_file *m;
 	struct drm_device *dev;
 	struct drm_connector *connector;
-	struct drm_encoder *drm_enc;
 	struct intel_encoder *intel_encoder;
 	struct intel_connector *intel_connector;
 	struct intel_dp *intel_dp;
@@ -4036,18 +4027,13 @@ static ssize_t displayport_config_ctl_write(struct file *file,
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
 		intel_connector = to_intel_connector(connector);
 		intel_encoder = intel_connector->encoder;
-		drm_enc = &intel_encoder->base;
-		if (connector->connector_type ==
-		    DRM_MODE_CONNECTOR_DisplayPort) {
-			if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
-			    intel_encoder->type == INTEL_OUTPUT_UNKNOWN) {
-				intel_dp = enc_to_intel_dp(drm_enc);
-				status = displayport_parse_config(input_buffer,
-								  len,
-								  intel_dp);
-				if (status < 0)
-					goto out;
-			}
+		if (dp_connector_is_valid(connector, 0)) {
+			intel_dp = enc_to_intel_dp(&intel_encoder->base);
+			status = displayport_parse_config(input_buffer,
+							  len,
+							  intel_dp);
+			if (status < 0)
+				goto out;
 		}
 	}
 out:
-- 
1.9.1

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

[PATCH 16/17] drm/i915: Add new debugfs file for Displaypor compliance test control

[Todd Previte]

Adds a new file for controlling Displayport compliance testing via the debugfs
interface. Adds two functions, 'open' and 'show', as well as the file operations
structure to support reading the file from userspace. The new file is called
'i915_dp_test_ctl' and contains 4 fields:
  - Connector name
  - Test data
  - Test active flag
  - Test response
These four fields are used to control communication and operation between the
userspace application and the kernel during compliance testing. A new variable
is also added to the intel_dp struct to hold the response code from the user
application. This response code is then sent to the sink device upon completion
of the test.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 66 ++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h    |  1 +
 2 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 846b2fb..d2cd684 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -54,6 +54,7 @@ static const char *yesno(int v)
 
 #define DP_PARAMETER_COUNT		8
 #define MAX_DP_CONFIG_LINE_COUNT	64
+#define DP_TEST_CTRL_LINE_COUNT		4
 
 enum dp_config_param {
 	DP_CONFIG_PARAM_INVALID = -1,
@@ -67,6 +68,14 @@ enum dp_config_param {
 	DP_CONFIG_PARAM_BPP
 };
 
+enum dp_ctrl_param {
+	DP_CTRL_PARAM_INVALID = -1,
+	DP_CTRL_PARAM_CONNECTOR = 0,
+	DP_CTRL_PARAM_TEST_DATA = 1,
+	DP_CTRL_PARAM_TEST_ACTIVE = 2,
+	DP_CTRL_PARAM_TEST_RESPONSE = 3
+};
+
 struct dp_config {
 	enum dp_config_param type;
 	unsigned long value;
@@ -4054,6 +4063,60 @@ static const struct file_operations i915_displayport_config_ctl_fops = {
 	.write = displayport_config_ctl_write
 };
 
+static int displayport_test_ctl_show(struct seq_file *m, void *data)
+{
+	struct drm_device *dev = m->private;
+	struct drm_connector *connector;
+	struct intel_encoder *intel_encoder;
+	struct intel_connector *intel_connector;
+	struct intel_dp *intel_dp;
+
+	if (!dev)
+		return -ENODEV;
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+		intel_connector = to_intel_connector(connector);
+		intel_encoder = intel_connector->encoder;
+
+		seq_printf(m, DP_CONF_STR_CONNECTOR,
+			   intel_connector->base.name);
+
+		if (dp_connector_is_valid(connector, 1)) {
+			intel_encoder = intel_connector->encoder;
+			intel_dp = enc_to_intel_dp(&intel_encoder->base);
+			/* Compliance test data informs userspace about current
+			   request
+			*/
+			seq_printf(m, "Test Data  : %08lx\n",
+				   intel_dp->compliance_test_data);
+			seq_printf(m, "Test Active: %02x\n",
+				   intel_dp->compliance_testing_active);
+			seq_printf(m, "Test Response: %02lx\n",
+				   intel_dp->compliance_test_response);
+		} else {
+			seq_puts(m, "disconnected\n");
+		}
+	}
+
+	return 0;
+}
+
+static int displayport_test_ctl_open(struct inode *inode,
+				  struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+
+	return single_open(file, displayport_test_ctl_show, dev);
+}
+
+static const struct file_operations i915_dp_test_ctl_fops = {
+	.owner = THIS_MODULE,
+	.open = displayport_test_ctl_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
 static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
 {
 	struct drm_device *dev = m->private;
@@ -4806,7 +4869,8 @@ static const struct i915_debugfs_files {
 	{"i915_spr_wm_latency", &i915_spr_wm_latency_fops},
 	{"i915_cur_wm_latency", &i915_cur_wm_latency_fops},
 	{"i915_fbc_false_color", &i915_fbc_fc_fops},
-	{"i915_displayport_config_ctl", &i915_displayport_config_ctl_fops}
+	{"i915_displayport_config_ctl", &i915_displayport_config_ctl_fops},
+	{"i915_dp_test_ctl", &i915_dp_test_ctl_fops}
 };
 
 void intel_display_crc_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5a8b1d6..eff3a14 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -653,6 +653,7 @@ struct intel_dp {
 	/* Displayport compliance testing */
 	unsigned long compliance_test_data;
 	bool compliance_testing_active;
+	unsigned long compliance_test_response;
 	struct intel_dp_link_config compliance_config;
 	struct intel_dp_link_config saved_config;
 
-- 
1.9.1

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

[PATCH 17/17] drm/i915: Add debugfs write and test param parsing function for DP test control

[Todd Previte]

Adds and implements the 'write' function for the debugfs i915_dp_test_ctrl file.
Also adds in the required parsing function to read in the data from the file
once the user app has written its data to it.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 103 ++++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d2cd684..091d62b 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4063,6 +4063,46 @@ static const struct file_operations i915_displayport_config_ctl_fops = {
 	.write = displayport_config_ctl_write
 };
 
+static int displayport_parse_test_ctl(char *input_buffer,
+				      ssize_t buffer_size,
+				      struct intel_dp *intel_dp)
+{
+	char *ctrl_lines[DP_TEST_CTRL_LINE_COUNT];
+	int line_count;
+	int test_data;
+	int active;
+	int response;
+	char *conn_name; /* Connector name in the file */
+	char *dp_name;	 /* Connector name in the intel_dp struct */
+
+	if (!input_buffer)
+		return -EIO;
+
+	line_count = tokenize_dp_config(input_buffer, ctrl_lines);
+	if (line_count != DP_TEST_CTRL_LINE_COUNT)
+		return -EIO;
+
+	conn_name = ctrl_lines[DP_CTRL_PARAM_CONNECTOR];
+	dp_name = intel_dp->attached_connector->base.name;
+
+	if (strncmp(conn_name, dp_name, strlen(dp_name)) == 0) {
+		kstrtol(ctrl_lines[DP_CTRL_PARAM_TEST_DATA],
+			16,
+			(long *)&test_data);
+		kstrtol(ctrl_lines[DP_CTRL_PARAM_TEST_ACTIVE],
+			16,
+			(long *)&active);
+		kstrtol(ctrl_lines[DP_CTRL_PARAM_TEST_RESPONSE],
+			16,
+			(long *)&response);
+	} else {
+		DRM_DEBUG_DRIVER("Connector names don't match\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static int displayport_test_ctl_show(struct seq_file *m, void *data)
 {
 	struct drm_device *dev = m->private;
@@ -4109,9 +4149,72 @@ static int displayport_test_ctl_open(struct inode *inode,
 	return single_open(file, displayport_test_ctl_show, dev);
 }
 
+static ssize_t displayport_test_ctl_write(struct file *file,
+					    const char __user *ubuf,
+					    size_t len, loff_t *offp)
+{
+	char *input_buffer;
+	int status = 0;
+	struct seq_file *m;
+	struct drm_device *dev;
+	struct drm_connector *connector;
+	struct intel_encoder *intel_encoder;
+	struct intel_connector *intel_connector;
+	struct intel_dp *intel_dp;
+
+	m = file->private_data;
+	if (!m) {
+		status = -ENODEV;
+		return status;
+	}
+	dev = m->private;
+
+	if (!dev) {
+		status = -ENODEV;
+		return status;
+	}
+
+	if (len == 0)
+		return 0;
+
+	input_buffer = kmalloc(len + 1, GFP_KERNEL);
+	if (!input_buffer)
+		return -ENOMEM;
+
+	if (copy_from_user(input_buffer, ubuf, len)) {
+		status = -EFAULT;
+		goto out;
+	}
+
+	input_buffer[len] = '\0';
+	DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+		intel_connector = to_intel_connector(connector);
+		intel_encoder = intel_connector->encoder;
+		if (dp_connector_is_valid(connector, 0)) {
+			intel_dp = enc_to_intel_dp(&intel_encoder->base);
+			status = displayport_parse_test_ctl(input_buffer,
+							    len,
+							    intel_dp);
+			if (status < 0)
+				goto out;
+		}
+	}
+out:
+	kfree(input_buffer);
+	if (status < 0)
+		return status;
+
+	*offp += len;
+	return len;
+
+}
+
 static const struct file_operations i915_dp_test_ctl_fops = {
 	.owner = THIS_MODULE,
 	.open = displayport_test_ctl_open,
+	.write = displayport_test_ctl_write,
 	.read = seq_read,
 	.llseek = seq_lseek,
 	.release = single_release,
-- 
1.9.1

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

Re: [PATCH 01/17] drm/i915: Add automated testing support for Displayport compliance testing

[Paulo Zanoni]

2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>:
> Add the skeleton framework for supporting automation for Displayport compliance
> testing. This patch adds the necessary framework for the source device to
> appropriately respond to test automation requests from a sink device.
>
> V2:
> - Addressed previous mailing list feedback
> - Fixed compilation issue (struct members declared in a later patch)
> - Updated debug messages to be more accurate
> - Added status checks for the DPCD read/write calls
> - Removed excess comments and debug messages
> - Fixed debug message compilation warnings
> - Fixed compilation issue with missing variables
> - Updated link training autotest to ACK
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 72 +++++++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h |  4 +++
>  2 files changed, 72 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 3fc3296..3dc92a3 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3744,11 +3744,75 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>         return true;
>  }
>
> -static void
> -intel_dp_handle_test_request(struct intel_dp *intel_dp)
> +static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
> +{
> +       uint8_t test_result = DP_TEST_ACK;
> +       return test_result;
> +}
> +
> +static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
> +{
> +       uint8_t test_result = DP_TEST_NAK;
> +       return test_result;
> +}
> +
> +static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
> +{
> +       uint8_t test_result = DP_TEST_NAK;
> +       return test_result;
> +}
> +
> +static uint8_t intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
> +{
> +       uint8_t test_result = DP_TEST_NAK;
> +       return test_result;
> +}
> +
> +static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  {
> -       /* NAK by default */
> -       drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK);
> +       uint8_t response = DP_TEST_NAK;
> +       uint8_t rxdata = 0;
> +       int status = 0;
> +
> +       status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
> +       if (status != 0) {

Why are we checking for zero here? In the "happy case", shouldn't this
function return 1? To my understanding, we would be ignoring all test
requests from the users, which means you wouldn't be able to test
anything in your series at all... I see that you don't change this
line at all in the rest of your series, so maybe I'm just crazy and
failing to notice some detail...


> +               response = DP_TEST_NAK;
> +               DRM_DEBUG_KMS("Could not read test request from sink\n");

You assign a value to "response" but don't do anything to it.
Shouldn't we be trying to send the NAK in this case? If yes, then the
code is missing, if no, then I guess we can remove the "response"
assignment (well, we could remove it in both cases since it's already
initialized to DP_TEST_NAK anyway).


> +               return;
> +       }
> +
> +       switch (rxdata) {
> +       case DP_TEST_LINK_TRAINING:
> +               DRM_DEBUG_KMS("LINK_TRAINING test requested\n");
> +               response = intel_dp_autotest_link_training(intel_dp);
> +               break;
> +       case DP_TEST_LINK_VIDEO_PATTERN:
> +               DRM_DEBUG_KMS("TEST_PATTERN test requested\n");
> +               response = intel_dp_autotest_video_pattern(intel_dp);
> +               break;
> +       case DP_TEST_LINK_EDID_READ:
> +               DRM_DEBUG_KMS("EDID test requested\n");
> +               response = intel_dp_autotest_edid(intel_dp);
> +               break;
> +       case DP_TEST_LINK_PHY_TEST_PATTERN:
> +               DRM_DEBUG_KMS("PHY_PATTERN test requested\n");
> +               response = intel_dp_autotest_phy_pattern(intel_dp);
> +               break;
> +               /* FAUX is optional in DP 1.2*/
> +       case DP_TEST_LINK_FAUX_PATTERN:
> +               DRM_DEBUG_KMS("FAUX_PATTERN testing not supported\n");
> +               break;
> +       default:
> +               DRM_DEBUG_KMS("Invalid test request '%02x'\n", rxdata);
> +               break;
> +       }
> +       status = drm_dp_dpcd_write(&intel_dp->aux,
> +                                  DP_TEST_RESPONSE,
> +                                  &response, 1);
> +       if (status != 0)

Same here...


> +               DRM_DEBUG_KMS("Could not write test response to sink\n");
> +
> +       intel_dp->compliance_testing_active = 0;
>  }
>
>  static int
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 588b618..d1a807a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -638,6 +638,10 @@ struct intel_dp {
>                 struct mutex mutex;
>         } drrs_state;
>
> +       /* Displayport compliance testing */
> +       unsigned long compliance_test_data;
> +       bool compliance_testing_active;


Not a change request, but just a note: usually it's better to just add
new field/members in the patches that actually start using them.
Because sometimes we merge the first patches before the others, and we
may decide to change the later patches so they stop using those
fields, so we risk ending with unused space. Also, adding a field just
in the patch that uses it allows the reviewer to check if the chosen
type, name and location are appropriate, etc.


> +
>  };
>
>  struct intel_digital_port {
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 04/17] drm/i915: Add debugfs information for Displayport compliance testing

[Jani Nikula]

On Thu, 11 Dec 2014, Todd Previte <tprevite@gmail.com> wrote:
> This patch was part of "[PATCH 05/10] drm/i915: Add debugfs interface for
> Displayport debug and compliance testing". That patch has been split into
> smaller patches for ease of review and integration.
>
> This patch contains the definitions/declarations for some of the constants
> and data structures added to support debugfs output for Displayport compliance
> testing.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 41 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 479e0c1..65b4f5e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -33,6 +33,7 @@
>  #include <linux/slab.h>
>  #include <linux/export.h>
>  #include <linux/list_sort.h>
> +#include <linux/string.h>
>  #include <asm/msr-index.h>
>  #include <drm/drmP.h>
>  #include "intel_drv.h"
> @@ -51,6 +52,46 @@ static const char *yesno(int v)
>  	return v ? "yes" : "no";
>  }
>  
> +#define DP_PARAMETER_COUNT		8
> +#define MAX_DP_CONFIG_LINE_COUNT	64
> +
> +enum dp_config_param {
> +	DP_CONFIG_PARAM_INVALID = -1,
> +	DP_CONFIG_PARAM_CONNECTOR = 0,
> +	DP_CONFIG_PARAM_LINK_RATE,
> +	DP_CONFIG_PARAM_LANE_COUNT,
> +	DP_CONFIG_PARAM_VOLTAGE_SWING,
> +	DP_CONFIG_PARAM_PREEMPHASIS,
> +	DP_CONFIG_PARAM_HRES,
> +	DP_CONFIG_PARAM_VRES,
> +	DP_CONFIG_PARAM_BPP

If you start the enum with DP_CONFIG_PARAM_CONNECTOR, you can add
DP_PARAMETER_COUNT here which will automatically be correct, and then
add DP_CONFIG_PARAM_INVALID = -1 after that.

> +};
> +
> +struct dp_config {
> +	enum dp_config_param type;
> +	unsigned long value;
> +};
> +
> +const char *dp_conf_tokens[DP_PARAMETER_COUNT] = {
> +		 "Connector",
> +		 "Link Rate",
> +		 "Lane Count",
> +		 "Voltage Swing Level",
> +		 "Preemphasis Level",
> +		 "Horizontal Resolution",
> +		 "Vertical Resolution",
> +		 "Bits Per Pixel"

It's customary to initialize these with designated initializers so that
the order here is not tied to the enum. I.e.

	[DP_CONFIG_PARAM_CONNECTOR] = "Connector",

> +};
> +/* Strings for writing out dp_configs */
> +#define DP_CONF_STR_CONNECTOR	"Connector:             %s\n"
> +#define DP_CONF_STR_LINKRATE	"Link Rate:             %02x\n"
> +#define DP_CONF_STR_LANES	"Lane Count:            %u\n"
> +#define DP_CONF_STR_VSWING	"Voltage Swing Level:   %u\n"
> +#define DP_CONF_STR_PREEMP	"Preemphasis Level:     %u\n"
> +#define DP_CONF_STR_HRES	"Horizontal Resolution: %d\n"
> +#define DP_CONF_STR_VRES	"Vertical Resolution:   %d\n"
> +#define DP_CONF_STR_BPP		"Bits Per Pixel:        %u\n"

I really don't like format strings as defines, because it moves the
format string away from the parameters that need to match the format.

A better option might be to have helper wrappers for seq_printf that
take the required parameters.

BR,
Jani.



> +
>  /* As the drm_debugfs_init() routines are called before dev->dev_private is
>   * allocated we need to hook into the minor for release. */
>  static int
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 02/17] drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing

[Paulo Zanoni]

2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>:
> Move the DPCD read to the top and check for an interrupt from the sink to catch
> Displayport automated testing requests necessary to support Displayport compliance
> testing. The checks for active connectors and link status are moved below the
> check for the interrupt.

Why exactly is this needed?

>
> Adds a check at the top to verify that the device is connected. This is necessary
> for DP compliance testing to ensure that test requests are captured and acknowledged.

Why exactly? Can you please describe in terms of how the code is
executed in each case and what is missing?

Since there appears to be 2 different changes, shouldn't this patch be
split into 2 different patches?

> If a test request is present during a connected->disconnected transition,
> the test code will attempt to execute even though the connection has been disabled,
> resulting in a faied test.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 3dc92a3..1b452cc 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3890,21 +3890,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>
>         WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>
> -       if (!intel_encoder->connectors_active)
> -               return;
> -
> -       if (WARN_ON(!intel_encoder->base.crtc))
> -               return;
> -
> -       if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> -               return;
> -
> -       /* Try to read receiver status if the link appears to be up */
> -       if (!intel_dp_get_link_status(intel_dp, link_status)) {
> +       /* Bail if not connected */

Bikeshed: I don't see the reason for the comment above :)

Other than that, the patch looks fine. Let's see what PRTS will say about it.


> +       if (intel_dp->attached_connector->base.status !=
> +           connector_status_connected) {
> +               DRM_DEBUG_KMS("Not connected\n");
>                 return;
>         }
>
> -       /* Now read the DPCD to see if it's actually running */
> +       /* Attempt to read the DPCD */
>         if (!intel_dp_get_dpcd(intel_dp)) {
>                 return;
>         }
> @@ -3916,13 +3909,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>                 drm_dp_dpcd_writeb(&intel_dp->aux,
>                                    DP_DEVICE_SERVICE_IRQ_VECTOR,
>                                    sink_irq_vector);
> -
>                 if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
>                         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");
>         }
>
> +       if (!intel_encoder->connectors_active)
> +               return;
> +
> +       if (WARN_ON(!intel_encoder->base.crtc))
> +               return;
> +
> +       if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +               return;
> +
> +       /* Try to read receiver status if the link appears to be up */
> +       if (!intel_dp_get_link_status(intel_dp, link_status)) {
> +               return;
> +       }
> +
>         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);
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 03/17] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Paulo Zanoni]

2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>:
> The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
> specifies that repeated AUX transactions after a failure (no response /
> invalid response) must have a minimum delay of 400us before the resend can
> occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.
>
> V2:
> - Changed udelay() to usleep_range()
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1b452cc..b6f5a72 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -876,9 +876,14 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>                                    DP_AUX_CH_CTL_TIME_OUT_ERROR |
>                                    DP_AUX_CH_CTL_RECEIVE_ERROR);
>
> +                       /* DP compliance requires 400us delay for errors
> +                          and timeouts (DP CTS 1.2 Core Rev 1.1, 4.2.1.1
> +                          & 4.2.1.2) */
>                         if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
> -                                     DP_AUX_CH_CTL_RECEIVE_ERROR))
> +                                     DP_AUX_CH_CTL_RECEIVE_ERROR)) {
> +                               usleep_range(400, 500);

One thing to notice is that if we get a TIME_OUT_ERROR I guess it
means we already waited our standard timeout (which is either 400, 600
or 1600, depending on the platform), so shouldn't we just do the
usleep() after the RECEIVE_ERROR error?

>                                 continue;
> +                       }
>                         if (status & DP_AUX_CH_CTL_DONE)
>                                 break;
>                 }
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 06/17] drm/i915: Add support functions in debugfs for handling Displayport compliance configuration

[Paulo Zanoni]

2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>:
> This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs interface
> for Displayport debug  and compliance testing". Adds two support functions for
> handling Displayport configuration parameters that are used for compliance
> testing.
>

First of all, a little comment about not only this patch but others
too: you had the big 05/10 patch in the previous series, and even
though it was big, I could review it since the new functions included
their callers and everything. Now, I see you have patch 4 where you
add just a few definitions without users. Then in patch 5 you add a
debugfs file that does nothing; here in patch 6 you add 2 new
functions without callers. It's kinda hard to review these things
without knowing how they are used. Also, if at some later point of the
review we decide to change, for example, patch 08/17, maybe some of
the stuff added by patch 05 or 06 will have to be changed. I usually
try to split patches into "minimal reviewable bisectable
self-contained" pieces, where each patch moves the overall code a
single step and add all the required structures/definitions. This way,
every added function, definition and struct field has a user.

For example, the 2 functions added by this patch should probably be
static - judging by the fact that you don't export them in a header
file -, but if you go and declare them as static, then you'll get
compiler warnings complaining that they are declared static but are
not used.

> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 48 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index feae100..ce091c1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3694,6 +3694,54 @@ static const struct file_operations i915_display_crc_ctl_fops = {
>         .write = display_crc_ctl_write
>  };
>
> +enum dp_config_param displayport_get_config_param_type(char *input_string)
> +{
> +       enum dp_config_param param_type = DP_CONFIG_PARAM_INVALID;
> +       int i = 0;
> +
> +       for (i = 0; i < DP_PARAMETER_COUNT; i++) {
> +               if (strncmp(input_string, dp_conf_tokens[i],
> +                           strlen(dp_conf_tokens[i])) == 0) {

I guess we never really did a high-level review of your debugfs
protocol, and I can't really see everything since there's no
user-space side here. From this function, I can imagine you're using a
string protocol. Is there any major advantage of using a string
protocol compared to using a binary one? By using binary, we would be
able to avoid these string operations which add a lot of complexity
and danger, both on the user and kernel side: if both shared the same
.h containing the right enums, they would just be able to pass the
enums around. Well, that's just a suggestion in case you think it's
better, not a requirement for you to rewrite everything.


> +                       param_type = (enum dp_config_param) i;
> +                       break;
> +               }
> +       }
> +       return param_type;
> +}
> +
> +int value_for_config_param(enum dp_config_param param,
> +                          char *input_string,
> +                          void *output_value)
> +{

This function is a little weird since it can return pointers to
different types of things. I would prefer if we kept simple and
avoided the " void *"  here., even if that meant a little more code.
But, well, let me see in the later patches how this is used first.

Besides the bikeshed, the code looks correct.

> +       int status = 0;
> +       unsigned int *out_ptr = (unsigned int *)output_value;
> +       char *index = input_string;
> +
> +       switch (param) {
> +       case DP_CONFIG_PARAM_CONNECTOR:
> +               output_value = (char *)index;
> +               break;
> +       case DP_CONFIG_PARAM_LINK_RATE:
> +       case DP_CONFIG_PARAM_LANE_COUNT:
> +       case DP_CONFIG_PARAM_VOLTAGE_SWING:
> +       case DP_CONFIG_PARAM_PREEMPHASIS:
> +               status = kstrtol(index, 16, (long *)out_ptr);
> +               break;
> +       case DP_CONFIG_PARAM_HRES:
> +       case DP_CONFIG_PARAM_VRES:
> +       case DP_CONFIG_PARAM_BPP:
> +               status = kstrtol(index, 10, (long *)out_ptr);
> +               break;
> +       default:
> +               /* Unhandled case */
> +               status = -EINVAL;
> +               *out_ptr = 0;
> +               break;
> +       }
> +
> +       return status;
> +}
> +
>  static int displayport_config_ctl_open(struct inode *inode,
>                                        struct file *file)
>  {
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 17/17] drm/i915: Add debugfs write and test param parsing function for DP test control

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK              +5                 360/366              365/366
SNB                                  448/450              448/450
IVB                 -1              497/498              496/498
BYT                                  289/289              289/289
HSW                                  563/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 ILK  igt_drv_suspend_fence-restore-untiled      DMESG_WARN(1, M26)PASS(7, M37M26)      PASS(1, M37)
 ILK  igt_kms_flip_bcs-flip-vs-modeset-interruptible      DMESG_WARN(1, M26)PASS(7, M37M26)      PASS(1, M37)
 ILK  igt_kms_flip_busy-flip-interruptible      DMESG_WARN(2, M26)PASS(6, M37M26)      PASS(1, M37)
 ILK  igt_kms_flip_flip-vs-rmfb-interruptible      DMESG_WARN(1, M26)PASS(7, M37M26)      PASS(1, M37)
 ILK  igt_kms_flip_rcs-flip-vs-dpms      DMESG_WARN(1, M26)PASS(6, M37M26)      PASS(1, M37)
*IVB  igt_kms_cursor_crc_cursor-64x64-sliding      FAIL(1, M21)PASS(1, M34)      DMESG_WARN(1, M21)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/17] drm/i915: Add and implement the debugfs 'show' functions for Displayport compliance

[Paulo Zanoni]

2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>:
> This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs
> interface for Displayport debug and compliance testing". This patch implements
> the 'show' functions for the debugfs interface for Displayport compliance
> testing.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 85 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index ce091c1..184797d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3694,6 +3694,56 @@ static const struct file_operations i915_display_crc_ctl_fops = {
>         .write = display_crc_ctl_write
>  };
>
> +static void displayport_show_config_info(struct seq_file *m,
> +                         struct intel_connector *intel_connector)
> +{
> +       struct intel_encoder *intel_encoder = intel_connector->encoder;
> +       struct intel_dp *intel_dp;
> +       struct intel_crtc *icrtc;

Please don't use "icrtc": the current trend is to call intel_crtc as
just "crtc". In the past, "struct drm_crtc" would be "crtc" and
"struct intel_crtc" would be "intel_crtc", but I don't remember seeing
icrtc.

> +       struct intel_crtc_config *crtc_config;
> +       char *conn_name = intel_connector->base.name;
> +       int pipe_bpp, hres, vres;
> +       uint8_t vs[4], pe[4];
> +       struct drm_display_mode *mode;
> +       int i = 0;
> +
> +       if (intel_encoder) {

Bikeshedding: since the whole function implementation is inside the
"if" statement, we usually prefer to just "if (!intel_encoder)
return;", because that saves one indentation level.


> +               intel_dp = enc_to_intel_dp(&intel_encoder->base);
> +               for (i = 0; i < intel_dp->lane_count; i++) {
> +                       vs[i] = intel_dp->train_set[i]
> +                               & DP_TRAIN_VOLTAGE_SWING_MASK;
> +                       pe[i] = (intel_dp->train_set[i]
> +                             & DP_TRAIN_PRE_EMPHASIS_MASK) >> 3;

Please use DP_TRAIN_PRE_EMPHASIS_SHIFT instead of the hardcoded "3".


> +               }
> +               if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) {
> +                       if (intel_encoder->new_crtc) {
> +                               crtc_config = &intel_encoder->new_crtc->config;
> +                               pipe_bpp = crtc_config->pipe_bpp;
> +                               mode = &crtc_config->adjusted_mode;
> +                               hres = mode->hdisplay;
> +                               vres = mode->vdisplay;
> +                       } else if (intel_encoder->base.crtc) {
> +                               icrtc = to_intel_crtc(intel_encoder->base.crtc);
> +                               pipe_bpp = icrtc->config.pipe_bpp;
> +                               mode = &icrtc->config.adjusted_mode;
> +                               hres = mode->hdisplay;
> +                               vres = mode->vdisplay;
> +                       } else {
> +                               pipe_bpp = 0;
> +                               hres = vres = 0;
> +                       }

Why do you have this new_crtc vs current_crtc vs no crtc distinction
here? Is it really necessary? Shouldn't the "DP testing debugfs
protocol" establish when exactly the information should be queried,
and then give some errors in case information is being requested at
the wrong time?


> +                       seq_printf(m, DP_CONF_STR_CONNECTOR, conn_name);
> +                       seq_printf(m, DP_CONF_STR_LINKRATE, intel_dp->link_bw);
> +                       seq_printf(m, DP_CONF_STR_LANES, intel_dp->lane_count);
> +                       seq_printf(m, DP_CONF_STR_VSWING, vs[0]);
> +                       seq_printf(m, DP_CONF_STR_PREEMP, pe[0]);
> +                       seq_printf(m, DP_CONF_STR_HRES, hres);
> +                       seq_printf(m, DP_CONF_STR_VRES, vres);
> +                       seq_printf(m, DP_CONF_STR_BPP, pipe_bpp);
> +               }
> +       }
> +}
> +
>  enum dp_config_param displayport_get_config_param_type(char *input_string)
>  {
>         enum dp_config_param param_type = DP_CONFIG_PARAM_INVALID;
> @@ -3742,6 +3792,41 @@ int value_for_config_param(enum dp_config_param param,
>         return status;
>  }
>
> +static int displayport_config_ctl_show(struct seq_file *m, void *data)
> +{
> +       struct drm_device *dev = m->private;
> +       struct drm_connector *drm_connector;
> +       struct intel_encoder *encoder;
> +       struct intel_connector *connector;

The standard is to make "struct drm_connector *connector" and "struct
intel_connector *intel_connector", or "struct intel_connector
*connector" and then always use &connector->base for when
drm_connector is needed. Like the encoders and crtcs.


> +
> +       if (!dev)
> +               return -ENODEV;
> +
> +       list_for_each_entry(drm_connector,
> +                           &dev->mode_config.connector_list,
> +                           head) {
> +               connector = to_intel_connector(drm_connector);
> +               encoder = connector->encoder;
> +               if (drm_connector->connector_type ==
> +                   DRM_MODE_CONNECTOR_DisplayPort) {
> +                       if (encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
> +                           encoder->type == INTEL_OUTPUT_UNKNOWN) {

If the connector is DP, I think you don't need to check the encoder
type (because you check for "connected" below). Also, you could have
used "&&" instead of nesting if statements, which would save one
indentation level.


> +                               if (drm_connector->status ==
> +                                   connector_status_connected) {
> +                                       displayport_show_config_info(m,
> +                                                                    connector);
> +                               } else {
> +                                       seq_printf(m, DP_CONF_STR_CONNECTOR,
> +                                                  encoder->base.name);
> +                                       seq_puts(m, "disconnected\n");
> +                               }
> +                       }
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  static int displayport_config_ctl_open(struct inode *inode,
>                                        struct file *file)
>  {
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 11/17] drm/i915: Update the EDID automated compliance test function

[Paulo Zanoni]

2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>:
> Updates the EDID compliance test function to perform the EDID read as
> required by the tests. This read needs to take place in the kernel for
> reasons of speed and efficiency. The results of the EDID read are handed
> off to userspace so that the remainder of the test can be conducted there.
>
> V2:
> - Addressed mailing list feedback
> - Removed excess debug messages
> - Removed extraneous comments
> - Fixed formatting issues (line length > 80)
> - Updated the debug message in compute_edid_checksum to output hex values
>   instead of decimal
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b6f5a72..2a13124 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -40,6 +40,13 @@
>
>  #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)
>
> +/* Compliance test status bits  */
> +#define  INTEL_DP_EDID_OK              (0<<0)
> +#define  INTEL_DP_EDID_CORRUPT         (1<<0)
> +#define  INTEL_DP_RESOLUTION_PREFERRED (1<<2)
> +#define  INTEL_DP_RESOLUTION_STANDARD  (1<<3)
> +#define  INTEL_DP_RESOLUTION_FAILSAFE  (1<<4)
> +
>  struct dp_link_dpll {
>         int link_bw;
>         struct dpll dpll;
> @@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>         return test_result;
>  }
>
> +static bool intel_dp_compute_edid_checksum(uint8_t *edid_data,
> +                                          uint8_t *edid_checksum)
> +{
> +       uint32_t byte_total = 0;
> +       uint8_t i = 0;
> +       bool edid_ok = true;
> +
> +       /* Don't include last byte (the checksum) in the computation */
> +       for (i = 0; i < EDID_LENGTH - 2; i++)

Shouldn't this be "i < EDID_LENGHT - 1"?


> +               byte_total += edid_data[i];
> +
> +       *edid_checksum = 256 - (byte_total % 256);
> +
> +       if (*edid_checksum != edid_data[EDID_LENGTH - 1]) {
> +               DRM_DEBUG_KMS("Invalid EDID checksum %02x, should be %02x\n",
> +                             edid_data[EDID_LENGTH - 40 - 1], *edid_checksum);
> +               edid_ok = false;
> +       }
> +
> +       return edid_ok;
> +}
> +
>  static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
>  {
> -       uint8_t test_result = DP_TEST_NAK;
> +       struct drm_connector *connector = &intel_dp->attached_connector->base;
> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
> +       struct edid *edid_read = NULL;
> +       uint8_t *edid_data = NULL;
> +       uint8_t test_result = DP_TEST_NAK, checksum = 0;
> +       uint32_t ret = 0;
> +
> +       intel_dp->aux.i2c_nack_count = 0;
> +       intel_dp->aux.i2c_defer_count = 0;
> +
> +       edid_read = drm_get_edid(connector, adapter);
> +
> +       if (edid_read == NULL) {
> +               /* Check for NACKs/DEFERs, use failsafe if detected
> +                  (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
> +               if (intel_dp->aux.i2c_nack_count > 0 ||
> +                       intel_dp->aux.i2c_defer_count > 0)
> +                       DRM_DEBUG_KMS("EDID read had %d NACKs, %d DEFERs\n",
> +                                     intel_dp->aux.i2c_nack_count,
> +                                     intel_dp->aux.i2c_defer_count);

Don't we need to use these _count values somehow, instead of just
printing them in the logs?

Everything else looks fine.

> +               intel_dp->compliance_test_data = INTEL_DP_EDID_CORRUPT |
> +                                                INTEL_DP_RESOLUTION_FAILSAFE;
> +       } else {
> +               edid_data = (uint8_t *) edid_read;
> +
> +               if (intel_dp_compute_edid_checksum(edid_data, &checksum)) {
> +                       ret = drm_dp_dpcd_write(&intel_dp->aux,
> +                                               DP_TEST_EDID_CHECKSUM,
> +                                               &edid_read->checksum, 1);
> +                       test_result = DP_TEST_ACK |
> +                                     DP_TEST_EDID_CHECKSUM_WRITE;
> +                       intel_dp->compliance_test_data =
> +                               INTEL_DP_EDID_OK |
> +                               INTEL_DP_RESOLUTION_PREFERRED;
> +               } else {
> +                       /* Invalid checksum - EDID corruption detection */
> +                       intel_dp->compliance_test_data =
> +                               INTEL_DP_EDID_CORRUPT |
> +                               INTEL_DP_RESOLUTION_FAILSAFE;
> +               }
> +       }
> +
>         return test_result;
>  }
>
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 12/17] drm/i915: Update intel_dp_compute_config() to handle compliance test requests

[Paulo Zanoni]

2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>:
> Adds provisions in intel_dp_compute_config() to accommodate compliance
> testing. Mostly this invovles circumventing the automatic link configuration
> parameters and allowing the compliance code to set those parameters as
> required by the tests.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 2a13124..4a55ca6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1189,6 +1189,21 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>         pipe_config->has_drrs = false;
>         pipe_config->has_audio = intel_dp->has_audio;
>
> +       /* Compliance testing should skip most of this function */
> +       if (!is_edp(intel_dp) && intel_dp->compliance_testing_active) {

I couldn't find any patch on your series that flips
intel_dp->compliance_testing_active to true, which is weird since it
would prevent us from testing the code.

Also, if we can make sure that we never set compliance_testing_active
to true on eDP, we can remove the is_edp() check.

> +               bpp = intel_dp->compliance_config.bits_per_pixel;
> +               lane_count = intel_dp->compliance_config.lane_count;
> +               clock = intel_dp->compliance_config.link_rate >> 3;
> +               /* Assign here and skip at the end - ensures correct values */
> +               intel_dp->link_bw = bws[clock];
> +               intel_dp->lane_count = lane_count;
> +               pipe_config->pipe_bpp = bpp;
> +               pipe_config->port_clock =
> +                       drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
> +
> +               goto compliance_exit;
> +       }
> +
>         if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
>                 intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
>                                        adjusted_mode);
> @@ -1275,6 +1290,7 @@ found:
>         DRM_DEBUG_KMS("DP link bw required %i available %i\n",
>                       mode_rate, link_avail);
>
> +compliance_exit:

Don't we need to move the color range adjustments to this point?

>         intel_link_compute_m_n(bpp, lane_count,
>                                adjusted_mode->crtc_clock,
>                                pipe_config->port_clock,
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 13/17] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation

[Paulo Zanoni]

2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>:
> Moves the non-MST case out of the if-statement and places it at the beginning
> of the function to handle HPD events for SST mode. The reasoning behind this
> is to accommodate link status checks for compliance testing. Some test devices
> use long pulses to perform test requests so link status must be checked
> regardless of the pulse width for the SST operational mode.

Can you please elaborate a little more on what do you see on these
devices? The test spec is very clear about short vs long HPD pulses,
so it's hard to believe a test device would get this wrong. We have
some registers on the PCH that allow us to redefine short vs long
durations. Have you tried to play with them?

More below:

>
> This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
> previous compliance testing patch sequence. Review feedback on that patch
> indicated that updating intel_dp_hot_plug() was not the correct place for
> the test handler.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 4a55ca6..73014d8 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>         power_domain = intel_display_port_power_domain(intel_encoder);
>         intel_display_power_get(dev_priv, power_domain);
>
> +       if (!intel_dp->is_mst) {
> +               /*
> +                *  Pulse width doesn't matter for SST mode
> +                *  Handle the HPD event now
> +               */
> +               drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +               intel_dp_check_link_status(intel_dp);

The very first thing intel_dp_check_link_status() does is to return in
case "connector->base.status != connected". If we're getting a long
HPD, it doesn't seem make sense to check this field because the status
might be changing due to the long HPD.

> +               drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +               ret = false;
> +               goto put_power;
> +       }
> +
>         if (long_hpd) {
>
>                 if (HAS_PCH_SPLIT(dev)) {
> @@ -4637,16 +4649,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>                         if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
>                                 goto mst_fail;
>                 }
> -
> -               if (!intel_dp->is_mst) {
> -                       /*
> -                        * we'll check the link status via the normal hot plug path later -
> -                        * but for short hpds we should check it now
> -                        */
> -                       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -                       intel_dp_check_link_status(intel_dp);
> -                       drm_modeset_unlock(&dev->mode_config.connection_mutex);
> -               }
>         }
>         ret = false;
>         goto put_power;
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 14/17] drm/i915: Add debugfs function to check connector status for compliance testing

[Paulo Zanoni]

2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>:
> Adds a function to check the status of a Displayport connector. This function
> simplifies the Displayport compliance testing interface in debugfs by providing
> a single method for determining the status of a connector during Displayport
> compliance testing. This function checks whether or not the connector is a
> Displayport connector and whether or not that connector is active.
>

I see patches 14 and 15 rewrite some code that was added by patches 7
and 10. Instead of doing this, please make sure patch 7 already
contains your dp_connector_is_valid() function, then patch 10 just
calls it.

Also, please remove those "goto" and just use plain "return".

> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 5eb6c24..978ddd1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3920,6 +3920,35 @@ static int displayport_parse_config(char *input_buffer,
>         return status;
>  }
>
> +static bool dp_connector_is_valid(struct drm_connector *connector,
> +                                 bool check_active)
> +{
> +       struct intel_encoder *intel_encoder;
> +       struct intel_connector *intel_connector;
> +
> +       intel_connector = to_intel_connector(connector);
> +       intel_encoder = intel_connector->encoder;
> +
> +       if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort)
> +               goto no;
> +
> +       if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT &&
> +           intel_encoder->type != INTEL_OUTPUT_UNKNOWN)
> +               goto no;
> +
> +       if (check_active) {
> +               if (connector->status == connector_status_connected)
> +                       goto yes;
> +               else
> +                       goto no;
> +       }
> +
> +yes:
> +       return true;
> +no:
> +       return false;
> +}
> +
>  static int displayport_config_ctl_show(struct seq_file *m, void *data)
>  {
>         struct drm_device *dev = m->private;
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 07/17] drm/i915: Add and implement the debugfs 'show' functions for Displayport compliance

[Daniel Vetter]

On Tue, Dec 16, 2014 at 05:00:38PM -0200, Paulo Zanoni wrote:
> 2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>:
> > +               if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) {
> > +                       if (intel_encoder->new_crtc) {
> > +                               crtc_config = &intel_encoder->new_crtc->config;
> > +                               pipe_bpp = crtc_config->pipe_bpp;
> > +                               mode = &crtc_config->adjusted_mode;
> > +                               hres = mode->hdisplay;
> > +                               vres = mode->vdisplay;
> > +                       } else if (intel_encoder->base.crtc) {
> > +                               icrtc = to_intel_crtc(intel_encoder->base.crtc);
> > +                               pipe_bpp = icrtc->config.pipe_bpp;
> > +                               mode = &icrtc->config.adjusted_mode;
> > +                               hres = mode->hdisplay;
> > +                               vres = mode->vdisplay;
> > +                       } else {
> > +                               pipe_bpp = 0;
> > +                               hres = vres = 0;
> > +                       }
> 
> Why do you have this new_crtc vs current_crtc vs no crtc distinction
> here? Is it really necessary? Shouldn't the "DP testing debugfs
> protocol" establish when exactly the information should be queried,
> and then give some errors in case information is being requested at
> the wrong time?

Presuming adequate locking exists (haven't checked tbh) new_* pointers always
match the equivalent base pointers. new_ pointers/structures are
exclusively for modeset code (and specifically state computation). I guess
the new_crtc case needs to be dropped here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/17] drm/i915: Update the EDID automated compliance test function

[Daniel Vetter]

On Wed, Dec 17, 2014 at 02:25:42PM -0200, Paulo Zanoni wrote:
> 2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>:
> > Updates the EDID compliance test function to perform the EDID read as
> > required by the tests. This read needs to take place in the kernel for
> > reasons of speed and efficiency. The results of the EDID read are handed
> > off to userspace so that the remainder of the test can be conducted there.
> >
> > V2:
> > - Addressed mailing list feedback
> > - Removed excess debug messages
> > - Removed extraneous comments
> > - Fixed formatting issues (line length > 80)
> > - Updated the debug message in compute_edid_checksum to output hex values
> >   instead of decimal
> >
> > Signed-off-by: Todd Previte <tprevite@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 71 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index b6f5a72..2a13124 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -40,6 +40,13 @@
> >
> >  #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)
> >
> > +/* Compliance test status bits  */
> > +#define  INTEL_DP_EDID_OK              (0<<0)
> > +#define  INTEL_DP_EDID_CORRUPT         (1<<0)
> > +#define  INTEL_DP_RESOLUTION_PREFERRED (1<<2)
> > +#define  INTEL_DP_RESOLUTION_STANDARD  (1<<3)
> > +#define  INTEL_DP_RESOLUTION_FAILSAFE  (1<<4)
> > +
> >  struct dp_link_dpll {
> >         int link_bw;
> >         struct dpll dpll;
> > @@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
> >         return test_result;
> >  }
> >
> > +static bool intel_dp_compute_edid_checksum(uint8_t *edid_data,
> > +                                          uint8_t *edid_checksum)
> > +{
> > +       uint32_t byte_total = 0;
> > +       uint8_t i = 0;
> > +       bool edid_ok = true;
> > +
> > +       /* Don't include last byte (the checksum) in the computation */
> > +       for (i = 0; i < EDID_LENGTH - 2; i++)
> 
> Shouldn't this be "i < EDID_LENGHT - 1"?
> 
> 
> > +               byte_total += edid_data[i];
> > +
> > +       *edid_checksum = 256 - (byte_total % 256);
> > +
> > +       if (*edid_checksum != edid_data[EDID_LENGTH - 1]) {
> > +               DRM_DEBUG_KMS("Invalid EDID checksum %02x, should be %02x\n",
> > +                             edid_data[EDID_LENGTH - 40 - 1], *edid_checksum);
> > +               edid_ok = false;
> > +       }
> > +
> > +       return edid_ok;
> > +}
> > +
> >  static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
> >  {
> > -       uint8_t test_result = DP_TEST_NAK;
> > +       struct drm_connector *connector = &intel_dp->attached_connector->base;
> > +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
> > +       struct edid *edid_read = NULL;
> > +       uint8_t *edid_data = NULL;
> > +       uint8_t test_result = DP_TEST_NAK, checksum = 0;
> > +       uint32_t ret = 0;
> > +
> > +       intel_dp->aux.i2c_nack_count = 0;
> > +       intel_dp->aux.i2c_defer_count = 0;
> > +
> > +       edid_read = drm_get_edid(connector, adapter);
> > +
> > +       if (edid_read == NULL) {
> > +               /* Check for NACKs/DEFERs, use failsafe if detected
> > +                  (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
> > +               if (intel_dp->aux.i2c_nack_count > 0 ||
> > +                       intel_dp->aux.i2c_defer_count > 0)
> > +                       DRM_DEBUG_KMS("EDID read had %d NACKs, %d DEFERs\n",
> > +                                     intel_dp->aux.i2c_nack_count,
> > +                                     intel_dp->aux.i2c_defer_count);
> 
> Don't we need to use these _count values somehow, instead of just
> printing them in the logs?
> 
> Everything else looks fine.
> 
> > +               intel_dp->compliance_test_data = INTEL_DP_EDID_CORRUPT |
> > +                                                INTEL_DP_RESOLUTION_FAILSAFE;
> > +       } else {
> > +               edid_data = (uint8_t *) edid_read;
> > +
> > +               if (intel_dp_compute_edid_checksum(edid_data, &checksum)) {
> > +                       ret = drm_dp_dpcd_write(&intel_dp->aux,
> > +                                               DP_TEST_EDID_CHECKSUM,
> > +                                               &edid_read->checksum, 1);
> > +                       test_result = DP_TEST_ACK |
> > +                                     DP_TEST_EDID_CHECKSUM_WRITE;
> > +                       intel_dp->compliance_test_data =
> > +                               INTEL_DP_EDID_OK |
> > +                               INTEL_DP_RESOLUTION_PREFERRED;
> > +               } else {
> > +                       /* Invalid checksum - EDID corruption detection */
> > +                       intel_dp->compliance_test_data =
> > +                               INTEL_DP_EDID_CORRUPT |
> > +                               INTEL_DP_RESOLUTION_FAILSAFE;

Just something random I've spotted while driving by: drm_get_edid does all
the checksum stuff for you already (it retries up to 4 times if the
checkusm is off and also checks a few other things). We should never reach
this case and the checksum function is essentially dead code.

Or do I miss something?
-Daniel

> > +               }
> > +       }
> > +
> >         return test_result;
> >  }
> >
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/17] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation

[Daniel Vetter]

On Wed, Dec 17, 2014 at 03:57:21PM -0200, Paulo Zanoni wrote:
> 2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>:
> > Moves the non-MST case out of the if-statement and places it at the beginning
> > of the function to handle HPD events for SST mode. The reasoning behind this
> > is to accommodate link status checks for compliance testing. Some test devices
> > use long pulses to perform test requests so link status must be checked
> > regardless of the pulse width for the SST operational mode.
> 
> Can you please elaborate a little more on what do you see on these
> devices? The test spec is very clear about short vs long HPD pulses,
> so it's hard to believe a test device would get this wrong. We have
> some registers on the PCH that allow us to redefine short vs long
> durations. Have you tried to play with them?
> 
> More below:
> 
> >
> > This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
> > previous compliance testing patch sequence. Review feedback on that patch
> > indicated that updating intel_dp_hot_plug() was not the correct place for
> > the test handler.
> >
> > Signed-off-by: Todd Previte <tprevite@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++----------
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 4a55ca6..73014d8 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> >         power_domain = intel_display_port_power_domain(intel_encoder);
> >         intel_display_power_get(dev_priv, power_domain);
> >
> > +       if (!intel_dp->is_mst) {
> > +               /*
> > +                *  Pulse width doesn't matter for SST mode
> > +                *  Handle the HPD event now
> > +               */
> > +               drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > +               intel_dp_check_link_status(intel_dp);
> 
> The very first thing intel_dp_check_link_status() does is to return in
> case "connector->base.status != connected". If we're getting a long
> HPD, it doesn't seem make sense to check this field because the status
> might be changing due to the long HPD.

I don't think we can unconditionally run SST hpd logic before we've
correctly handled mst mode. It likely screws up the accounting.
> 
> > +               drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > +               ret = false;
> > +               goto put_power;
> > +       }
> > +
> >         if (long_hpd) {
> >
> >                 if (HAS_PCH_SPLIT(dev)) {
> > @@ -4637,16 +4649,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> >                         if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
> >                                 goto mst_fail;
> >                 }
> > -
> > -               if (!intel_dp->is_mst) {
> > -                       /*
> > -                        * we'll check the link status via the normal hot plug path later -
> > -                        * but for short hpds we should check it now
> > -                        */

Just aside: The above comment is outdated and can be remove. This is now
the only place where we handle link retraining. The function could be made
static and dropped from headers, too.
-Daniel

> > -                       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > -                       intel_dp_check_link_status(intel_dp);
> > -                       drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > -               }
> >         }
> >         ret = false;
> >         goto put_power;
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/17] drm/i915: Update intel_dp_compute_config() to handle compliance test requests

[Clint Taylor]

On 12/17/2014 09:04 AM, Paulo Zanoni wrote:
> 2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>:
>> Adds provisions in intel_dp_compute_config() to accommodate compliance
>> testing. Mostly this invovles circumventing the automatic link configuration
>> parameters and allowing the compliance code to set those parameters as
>> required by the tests.
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 2a13124..4a55ca6 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1189,6 +1189,21 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>          pipe_config->has_drrs = false;
>>          pipe_config->has_audio = intel_dp->has_audio;
>>
>> +       /* Compliance testing should skip most of this function */
>> +       if (!is_edp(intel_dp) && intel_dp->compliance_testing_active) {
>
> I couldn't find any patch on your series that flips
> intel_dp->compliance_testing_active to true, which is weird since it
> would prevent us from testing the code.
>
> Also, if we can make sure that we never set compliance_testing_active
> to true on eDP, we can remove the is_edp() check.

Why would we not allow automation compliance testing on eDP? There are 
automation tests and fixtures from Unigraf and Agilent for eDP.

-Clint

>
>> +               bpp = intel_dp->compliance_config.bits_per_pixel;
>> +               lane_count = intel_dp->compliance_config.lane_count;
>> +               clock = intel_dp->compliance_config.link_rate >> 3;
>> +               /* Assign here and skip at the end - ensures correct values */
>> +               intel_dp->link_bw = bws[clock];
>> +               intel_dp->lane_count = lane_count;
>> +               pipe_config->pipe_bpp = bpp;
>> +               pipe_config->port_clock =
>> +                       drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
>> +
>> +               goto compliance_exit;
>> +       }
>> +
>>          if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
>>                  intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
>>                                         adjusted_mode);
>> @@ -1275,6 +1290,7 @@ found:
>>          DRM_DEBUG_KMS("DP link bw required %i available %i\n",
>>                        mode_rate, link_avail);
>>
>> +compliance_exit:
>
> Don't we need to move the color range adjustments to this point?
>
>>          intel_link_compute_m_n(bpp, lane_count,
>>                                 adjusted_mode->crtc_clock,
>>                                 pipe_config->port_clock,
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>

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

Re: [PATCH 01/17] drm/i915: Add automated testing support for Displayport compliance testing

[Todd Previte]

On 12/12/14 1:25 PM, Paulo Zanoni wrote:
> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>:
>> Add the skeleton framework for supporting automation for Displayport compliance
>> testing. This patch adds the necessary framework for the source device to
>> appropriately respond to test automation requests from a sink device.
>>
>> V2:
>> - Addressed previous mailing list feedback
>> - Fixed compilation issue (struct members declared in a later patch)
>> - Updated debug messages to be more accurate
>> - Added status checks for the DPCD read/write calls
>> - Removed excess comments and debug messages
>> - Fixed debug message compilation warnings
>> - Fixed compilation issue with missing variables
>> - Updated link training autotest to ACK
>>
>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c  | 72 +++++++++++++++++++++++++++++++++++++---
>>   drivers/gpu/drm/i915/intel_drv.h |  4 +++
>>   2 files changed, 72 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 3fc3296..3dc92a3 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3744,11 +3744,75 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>>          return true;
>>   }
>>
>> -static void
>> -intel_dp_handle_test_request(struct intel_dp *intel_dp)
>> +static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_ACK;
>> +       return test_result;
>> +}
>> +
>> +static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +static uint8_t intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>>   {
>> -       /* NAK by default */
>> -       drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK);
>> +       uint8_t response = DP_TEST_NAK;
>> +       uint8_t rxdata = 0;
>> +       int status = 0;
>> +
>> +       status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
>> +       if (status != 0) {
> Why are we checking for zero here? In the "happy case", shouldn't this
> function return 1? To my understanding, we would be ignoring all test
> requests from the users, which means you wouldn't be able to test
> anything in your series at all... I see that you don't change this
> line at all in the rest of your series, so maybe I'm just crazy and
> failing to notice some detail...
>
>
>> +               response = DP_TEST_NAK;
>> +               DRM_DEBUG_KMS("Could not read test request from sink\n");
> You assign a value to "response" but don't do anything to it.
> Shouldn't we be trying to send the NAK in this case? If yes, then the
> code is missing, if no, then I guess we can remove the "response"
> assignment (well, we could remove it in both cases since it's already
> initialized to DP_TEST_NAK anyway).
Good catches on these two - thanks Paulo. They've been fixed in V3.

>> +               return;
>> +       }
>> +
>> +       switch (rxdata) {
>> +       case DP_TEST_LINK_TRAINING:
>> +               DRM_DEBUG_KMS("LINK_TRAINING test requested\n");
>> +               response = intel_dp_autotest_link_training(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_VIDEO_PATTERN:
>> +               DRM_DEBUG_KMS("TEST_PATTERN test requested\n");
>> +               response = intel_dp_autotest_video_pattern(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_EDID_READ:
>> +               DRM_DEBUG_KMS("EDID test requested\n");
>> +               response = intel_dp_autotest_edid(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_PHY_TEST_PATTERN:
>> +               DRM_DEBUG_KMS("PHY_PATTERN test requested\n");
>> +               response = intel_dp_autotest_phy_pattern(intel_dp);
>> +               break;
>> +               /* FAUX is optional in DP 1.2*/
>> +       case DP_TEST_LINK_FAUX_PATTERN:
>> +               DRM_DEBUG_KMS("FAUX_PATTERN testing not supported\n");
>> +               break;
>> +       default:
>> +               DRM_DEBUG_KMS("Invalid test request '%02x'\n", rxdata);
>> +               break;
>> +       }
>> +       status = drm_dp_dpcd_write(&intel_dp->aux,
>> +                                  DP_TEST_RESPONSE,
>> +                                  &response, 1);
>> +       if (status != 0)
> Same here...
>
Same as above. Fixed in V3.
>> +               DRM_DEBUG_KMS("Could not write test response to sink\n");
>> +
>> +       intel_dp->compliance_testing_active = 0;
>>   }
>>
>>   static int
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 588b618..d1a807a 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -638,6 +638,10 @@ struct intel_dp {
>>                  struct mutex mutex;
>>          } drrs_state;
>>
>> +       /* Displayport compliance testing */
>> +       unsigned long compliance_test_data;
>> +       bool compliance_testing_active;
> Not a change request, but just a note: usually it's better to just add
> new field/members in the patches that actually start using them.
> Because sometimes we merge the first patches before the others, and we
> may decide to change the later patches so they stop using those
> fields, so we risk ending with unused space. Also, adding a field just
> in the patch that uses it allows the reviewer to check if the chosen
> type, name and location are appropriate, etc.
>
Ok I'll keep this in mind moving forward. Thanks Paulo!

>> +
>>   };
>>
>>   struct intel_digital_port {
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

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

Re: [PATCH 02/17] drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing

[Todd Previte]

On 12/15/2014 9:36 AM, Paulo Zanoni wrote:
> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>:
>> Move the DPCD read to the top and check for an interrupt from the sink to catch
>> Displayport automated testing requests necessary to support Displayport compliance
>> testing. The checks for active connectors and link status are moved below the
>> check for the interrupt.
> Why exactly is this needed?
The main reason for doing this is to make sure that a test request isn't 
missed. Checking for the status of the encoder/crtc isn't necessary for 
some test cases (AUX channel tests are one example) and without moving 
the check for the interrupt, these tests may not execute if one of those 
checks fails. Additionally, if reading the DPCD fails, regardless of 
whether or not testing is happening, there's no way to train the link 
since configurations and status can't be read, nor can link training 
parameters be written.


>> Adds a check at the top to verify that the device is connected. This is necessary
>> for DP compliance testing to ensure that test requests are captured and acknowledged.
> Why exactly? Can you please describe in terms of how the code is
> executed in each case and what is missing?
This patch is actually both a bug fix and a component of compliance 
testing. Because HPD events are received both on connect and disconnect 
actions, it's vital that we don't try and train the link when we're 
transitioning from connected->disconnected. That results in errors and 
warning in the logs from failed AUX transactions and can trigger the 
WARN for the check of !base.crtc. By making the check at the beginning 
to see if the connection is truly active, those problems are avoided and 
testing / link training will only be attempted when there is a valid 
Displayport connection.

> Since there appears to be 2 different changes, shouldn't this patch be
> split into 2 different patches?
It can be split into two if that will make it easier to upstream. The 
changes are separate but related, which is why I grouped them into a 
single patch.

>> If a test request is present during a connected->disconnected transition,
>> the test code will attempt to execute even though the connection has been disabled,
>> resulting in a faied test.
>>
>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 32 +++++++++++++++++++-------------
>>   1 file changed, 19 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 3dc92a3..1b452cc 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3890,21 +3890,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>
>>          WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>
>> -       if (!intel_encoder->connectors_active)
>> -               return;
>> -
>> -       if (WARN_ON(!intel_encoder->base.crtc))
>> -               return;
>> -
>> -       if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> -               return;
>> -
>> -       /* Try to read receiver status if the link appears to be up */
>> -       if (!intel_dp_get_link_status(intel_dp, link_status)) {
>> +       /* Bail if not connected */
> Bikeshed: I don't see the reason for the comment above :)
>
> Other than that, the patch looks fine. Let's see what PRTS will say about it.

Fair enough. Comment deleted. :) Fix will be in patch V3. The above 
responses to your questions were also included in the patch notes for 
further explanation of this patch.

>> +       if (intel_dp->attached_connector->base.status !=
>> +           connector_status_connected) {
>> +               DRM_DEBUG_KMS("Not connected\n");
>>                  return;
>>          }
>>
>> -       /* Now read the DPCD to see if it's actually running */
>> +       /* Attempt to read the DPCD */
>>          if (!intel_dp_get_dpcd(intel_dp)) {
>>                  return;
>>          }
>> @@ -3916,13 +3909,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>                  drm_dp_dpcd_writeb(&intel_dp->aux,
>>                                     DP_DEVICE_SERVICE_IRQ_VECTOR,
>>                                     sink_irq_vector);
>> -
>>                  if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
>>                          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");
>>          }
>>
>> +       if (!intel_encoder->connectors_active)
>> +               return;
>> +
>> +       if (WARN_ON(!intel_encoder->base.crtc))
>> +               return;
>> +
>> +       if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> +               return;
>> +
>> +       /* Try to read receiver status if the link appears to be up */
>> +       if (!intel_dp_get_link_status(intel_dp, link_status)) {
>> +               return;
>> +       }
>> +
>>          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);
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 03/17] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Todd Previte]

On 12/15/2014 10:35 AM, Paulo Zanoni wrote:
> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>:
>> The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
>> specifies that repeated AUX transactions after a failure (no response /
>> invalid response) must have a minimum delay of 400us before the resend can
>> occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.
>>
>> V2:
>> - Changed udelay() to usleep_range()
>>
>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 1b452cc..b6f5a72 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -876,9 +876,14 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>>                                     DP_AUX_CH_CTL_TIME_OUT_ERROR |
>>                                     DP_AUX_CH_CTL_RECEIVE_ERROR);
>>
>> +                       /* DP compliance requires 400us delay for errors
>> +                          and timeouts (DP CTS 1.2 Core Rev 1.1, 4.2.1.1
>> +                          & 4.2.1.2) */
>>                          if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> -                                     DP_AUX_CH_CTL_RECEIVE_ERROR))
>> +                                     DP_AUX_CH_CTL_RECEIVE_ERROR)) {
>> +                               usleep_range(400, 500);
> One thing to notice is that if we get a TIME_OUT_ERROR I guess it
> means we already waited our standard timeout (which is either 400, 600
> or 1600, depending on the platform), so shouldn't we just do the
> usleep() after the RECEIVE_ERROR error?
Capture of IRC discussion:

It seems reasonable to just continue to the next iteration if the HW has 
already hit a timeout condition. Testing has determined that it is not 
necessary to handle the timeout case, since the HW has already waited 
the required amount of time. That case has been removed and the comment 
has been updated for V3.

>>                                  continue;
>> +                       }
>>                          if (status & DP_AUX_CH_CTL_DONE)
>>                                  break;
>>                  }
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 04/17] drm/i915: Add debugfs information for Displayport compliance testing

[Todd Previte]

On 12/15/14 6:11 AM, Jani Nikula wrote:
> On Thu, 11 Dec 2014, Todd Previte<tprevite@gmail.com>  wrote:
>> This patch was part of "[PATCH 05/10] drm/i915: Add debugfs interface for
>> Displayport debug and compliance testing". That patch has been split into
>> smaller patches for ease of review and integration.
>>
>> This patch contains the definitions/declarations for some of the constants
>> and data structures added to support debugfs output for Displayport compliance
>> testing.
>>
>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c | 41 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 41 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 479e0c1..65b4f5e 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -33,6 +33,7 @@
>>   #include <linux/slab.h>
>>   #include <linux/export.h>
>>   #include <linux/list_sort.h>
>> +#include <linux/string.h>
>>   #include <asm/msr-index.h>
>>   #include <drm/drmP.h>
>>   #include "intel_drv.h"
>> @@ -51,6 +52,46 @@ static const char *yesno(int v)
>>   	return v ? "yes" : "no";
>>   }
>>   
>> +#define DP_PARAMETER_COUNT		8
>> +#define MAX_DP_CONFIG_LINE_COUNT	64
>> +
>> +enum dp_config_param {
>> +	DP_CONFIG_PARAM_INVALID = -1,
>> +	DP_CONFIG_PARAM_CONNECTOR = 0,
>> +	DP_CONFIG_PARAM_LINK_RATE,
>> +	DP_CONFIG_PARAM_LANE_COUNT,
>> +	DP_CONFIG_PARAM_VOLTAGE_SWING,
>> +	DP_CONFIG_PARAM_PREEMPHASIS,
>> +	DP_CONFIG_PARAM_HRES,
>> +	DP_CONFIG_PARAM_VRES,
>> +	DP_CONFIG_PARAM_BPP
> If you start the enum with DP_CONFIG_PARAM_CONNECTOR, you can add
> DP_PARAMETER_COUNT here which will automatically be correct, and then
> add DP_CONFIG_PARAM_INVALID = -1 after that.

Good point. That change has been integrated into V3.
>> +};
>> +
>> +struct dp_config {
>> +	enum dp_config_param type;
>> +	unsigned long value;
>> +};
>> +
>> +const char *dp_conf_tokens[DP_PARAMETER_COUNT] = {
>> +		 "Connector",
>> +		 "Link Rate",
>> +		 "Lane Count",
>> +		 "Voltage Swing Level",
>> +		 "Preemphasis Level",
>> +		 "Horizontal Resolution",
>> +		 "Vertical Resolution",
>> +		 "Bits Per Pixel"
> It's customary to initialize these with designated initializers so that
> the order here is not tied to the enum. I.e.
>
> 	[DP_CONFIG_PARAM_CONNECTOR] = "Connector",
Makes sense. Fixed for V3.
>> +};
>> +/* Strings for writing out dp_configs */
>> +#define DP_CONF_STR_CONNECTOR	"Connector:             %s\n"
>> +#define DP_CONF_STR_LINKRATE	"Link Rate:             %02x\n"
>> +#define DP_CONF_STR_LANES	"Lane Count:            %u\n"
>> +#define DP_CONF_STR_VSWING	"Voltage Swing Level:   %u\n"
>> +#define DP_CONF_STR_PREEMP	"Preemphasis Level:     %u\n"
>> +#define DP_CONF_STR_HRES	"Horizontal Resolution: %d\n"
>> +#define DP_CONF_STR_VRES	"Vertical Resolution:   %d\n"
>> +#define DP_CONF_STR_BPP		"Bits Per Pixel:        %u\n"
> I really don't like format strings as defines, because it moves the
> format string away from the parameters that need to match the format.
>
> A better option might be to have helper wrappers for seq_printf that
> take the required parameters.
>
> BR,
> Jani.
Fair enough. I added  a function to wrap the seq_printf function and one 
to initialize the tokens
instead of having them are straight #defines. Other patches further on 
in the sequence will need to
be adjusted to accommodate these changes.

>> +
>>   /* As the drm_debugfs_init() routines are called before dev->dev_private is
>>    * allocated we need to hook into the minor for release. */
>>   static int
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 07/17] drm/i915: Add and implement the debugfs 'show' functions for Displayport compliance

[Todd Previte]

Responses inline below. Any changes have been rolled into the monster 
patch.

-T

On 12/16/14 12:00 PM, Paulo Zanoni wrote:
> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>:
>> This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs
>> interface for Displayport debug and compliance testing". This patch implements
>> the 'show' functions for the debugfs interface for Displayport compliance
>> testing.
>>
>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c | 85 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 85 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index ce091c1..184797d 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -3694,6 +3694,56 @@ static const struct file_operations i915_display_crc_ctl_fops = {
>>          .write = display_crc_ctl_write
>>   };
>>
>> +static void displayport_show_config_info(struct seq_file *m,
>> +                         struct intel_connector *intel_connector)
>> +{
>> +       struct intel_encoder *intel_encoder = intel_connector->encoder;
>> +       struct intel_dp *intel_dp;
>> +       struct intel_crtc *icrtc;
> Please don't use "icrtc": the current trend is to call intel_crtc as
> just "crtc". In the past, "struct drm_crtc" would be "crtc" and
> "struct intel_crtc" would be "intel_crtc", but I don't remember seeing
> icrtc.
Just an attempt to get the code under the 80 char line limit. With the 
other changes below it's unnecessary anyways. It's fixed in V3.
>> +       struct intel_crtc_config *crtc_config;
>> +       char *conn_name = intel_connector->base.name;
>> +       int pipe_bpp, hres, vres;
>> +       uint8_t vs[4], pe[4];
>> +       struct drm_display_mode *mode;
>> +       int i = 0;
>> +
>> +       if (intel_encoder) {
> Bikeshedding: since the whole function implementation is inside the
> "if" statement, we usually prefer to just "if (!intel_encoder)
> return;", because that saves one indentation level.
Changed for V3. Also changed the intel_encode->type check to the same 
thing, again saving a level of indent.
>
>> +               intel_dp = enc_to_intel_dp(&intel_encoder->base);
>> +               for (i = 0; i < intel_dp->lane_count; i++) {
>> +                       vs[i] = intel_dp->train_set[i]
>> +                               & DP_TRAIN_VOLTAGE_SWING_MASK;
>> +                       pe[i] = (intel_dp->train_set[i]
>> +                             & DP_TRAIN_PRE_EMPHASIS_MASK) >> 3;
> Please use DP_TRAIN_PRE_EMPHASIS_SHIFT instead of the hardcoded "3".
Fixed.
>> +               }
>> +               if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) {
>> +                       if (intel_encoder->new_crtc) {
>> +                               crtc_config = &intel_encoder->new_crtc->config;
>> +                               pipe_bpp = crtc_config->pipe_bpp;
>> +                               mode = &crtc_config->adjusted_mode;
>> +                               hres = mode->hdisplay;
>> +                               vres = mode->vdisplay;
>> +                       } else if (intel_encoder->base.crtc) {
>> +                               icrtc = to_intel_crtc(intel_encoder->base.crtc);
>> +                               pipe_bpp = icrtc->config.pipe_bpp;
>> +                               mode = &icrtc->config.adjusted_mode;
>> +                               hres = mode->hdisplay;
>> +                               vres = mode->vdisplay;
>> +                       } else {
>> +                               pipe_bpp = 0;
>> +                               hres = vres = 0;
>> +                       }
> Why do you have this new_crtc vs current_crtc vs no crtc distinction
> here? Is it really necessary? Shouldn't the "DP testing debugfs
> protocol" establish when exactly the information should be queried,
> and then give some errors in case information is being requested at
> the wrong time?
That code is a hold-over from when I was making sure I wasn't missing 
test events because of invalid data structures. Part of the problem is 
that link training is tied to mode sets, which is where this code 
originally came from as I recall. In any case, I've removed the new_crtc 
case (in light of Daniel's reply) so it shouldn't be an issue.
>> +                       seq_printf(m, DP_CONF_STR_CONNECTOR, conn_name);
>> +                       seq_printf(m, DP_CONF_STR_LINKRATE, intel_dp->link_bw);
>> +                       seq_printf(m, DP_CONF_STR_LANES, intel_dp->lane_count);
>> +                       seq_printf(m, DP_CONF_STR_VSWING, vs[0]);
>> +                       seq_printf(m, DP_CONF_STR_PREEMP, pe[0]);
>> +                       seq_printf(m, DP_CONF_STR_HRES, hres);
>> +                       seq_printf(m, DP_CONF_STR_VRES, vres);
>> +                       seq_printf(m, DP_CONF_STR_BPP, pipe_bpp);
>> +               }
>> +       }
>> +}
>> +
>>   enum dp_config_param displayport_get_config_param_type(char *input_string)
>>   {
>>          enum dp_config_param param_type = DP_CONFIG_PARAM_INVALID;
>> @@ -3742,6 +3792,41 @@ int value_for_config_param(enum dp_config_param param,
>>          return status;
>>   }
>>
>> +static int displayport_config_ctl_show(struct seq_file *m, void *data)
>> +{
>> +       struct drm_device *dev = m->private;
>> +       struct drm_connector *drm_connector;
>> +       struct intel_encoder *encoder;
>> +       struct intel_connector *connector;
> The standard is to make "struct drm_connector *connector" and "struct
> intel_connector *intel_connector", or "struct intel_connector
> *connector" and then always use &connector->base for when
> drm_connector is needed. Like the encoders and crtcs.
Fair enough. Fixed in V3.
>> +
>> +       if (!dev)
>> +               return -ENODEV;
>> +
>> +       list_for_each_entry(drm_connector,
>> +                           &dev->mode_config.connector_list,
>> +                           head) {
>> +               connector = to_intel_connector(drm_connector);
>> +               encoder = connector->encoder;
>> +               if (drm_connector->connector_type ==
>> +                   DRM_MODE_CONNECTOR_DisplayPort) {
>> +                       if (encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
>> +                           encoder->type == INTEL_OUTPUT_UNKNOWN) {
> If the connector is DP, I think you don't need to check the encoder
> type (because you check for "connected" below). Also, you could have
> used "&&" instead of nesting if statements, which would save one
> indentation level.
I changed the code around to save some indentation levels and address 
this particular issue. There was an issue I ran across where just using 
the DRM connector wasn't enough, but I haven't been able to reproduce 
that problem with the current code. So this has been removed.
>> +                               if (drm_connector->status ==
>> +                                   connector_status_connected) {
>> +                                       displayport_show_config_info(m,
>> +                                                                    connector);
>> +                               } else {
>> +                                       seq_printf(m, DP_CONF_STR_CONNECTOR,
>> +                                                  encoder->base.name);
>> +                                       seq_puts(m, "disconnected\n");
>> +                               }
>> +                       }
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>   static int displayport_config_ctl_open(struct inode *inode,
>>                                         struct file *file)
>>   {
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

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

Re: [PATCH 07/17] drm/i915: Add and implement the debugfs 'show' functions for Displayport compliance

[Todd Previte]

On 12/17/14 1:12 PM, Daniel Vetter wrote:
> On Tue, Dec 16, 2014 at 05:00:38PM -0200, Paulo Zanoni wrote:
>> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>:
>>> +               if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) {
>>> +                       if (intel_encoder->new_crtc) {
>>> +                               crtc_config = &intel_encoder->new_crtc->config;
>>> +                               pipe_bpp = crtc_config->pipe_bpp;
>>> +                               mode = &crtc_config->adjusted_mode;
>>> +                               hres = mode->hdisplay;
>>> +                               vres = mode->vdisplay;
>>> +                       } else if (intel_encoder->base.crtc) {
>>> +                               icrtc = to_intel_crtc(intel_encoder->base.crtc);
>>> +                               pipe_bpp = icrtc->config.pipe_bpp;
>>> +                               mode = &icrtc->config.adjusted_mode;
>>> +                               hres = mode->hdisplay;
>>> +                               vres = mode->vdisplay;
>>> +                       } else {
>>> +                               pipe_bpp = 0;
>>> +                               hres = vres = 0;
>>> +                       }
>> Why do you have this new_crtc vs current_crtc vs no crtc distinction
>> here? Is it really necessary? Shouldn't the "DP testing debugfs
>> protocol" establish when exactly the information should be queried,
>> and then give some errors in case information is being requested at
>> the wrong time?
> Presuming adequate locking exists (haven't checked tbh) new_* pointers always
> match the equivalent base pointers. new_ pointers/structures are
> exclusively for modeset code (and specifically state computation). I guess
> the new_crtc case needs to be dropped here.
> -Daniel
I tested this again with the new_crtc case removed and it appears to 
work correctly. So the new_crtc case has been removed. The changes have 
been integrated into the monster patch.

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

Re: [PATCH 11/17] drm/i915: Update the EDID automated compliance test function

[Todd Previte]

On 12/17/14 9:25 AM, Paulo Zanoni wrote:
> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>:
>> Updates the EDID compliance test function to perform the EDID read as
>> required by the tests. This read needs to take place in the kernel for
>> reasons of speed and efficiency. The results of the EDID read are handed
>> off to userspace so that the remainder of the test can be conducted there.
>>
>> V2:
>> - Addressed mailing list feedback
>> - Removed excess debug messages
>> - Removed extraneous comments
>> - Fixed formatting issues (line length > 80)
>> - Updated the debug message in compute_edid_checksum to output hex values
>>    instead of decimal
>>
>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 71 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index b6f5a72..2a13124 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -40,6 +40,13 @@
>>
>>   #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)
>>
>> +/* Compliance test status bits  */
>> +#define  INTEL_DP_EDID_OK              (0<<0)
>> +#define  INTEL_DP_EDID_CORRUPT         (1<<0)
>> +#define  INTEL_DP_RESOLUTION_PREFERRED (1<<2)
>> +#define  INTEL_DP_RESOLUTION_STANDARD  (1<<3)
>> +#define  INTEL_DP_RESOLUTION_FAILSAFE  (1<<4)
>> +
>>   struct dp_link_dpll {
>>          int link_bw;
>>          struct dpll dpll;
>> @@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>>          return test_result;
>>   }
>>
>> +static bool intel_dp_compute_edid_checksum(uint8_t *edid_data,
>> +                                          uint8_t *edid_checksum)
>> +{
>> +       uint32_t byte_total = 0;
>> +       uint8_t i = 0;
>> +       bool edid_ok = true;
>> +
>> +       /* Don't include last byte (the checksum) in the computation */
>> +       for (i = 0; i < EDID_LENGTH - 2; i++)
> Shouldn't this be "i < EDID_LENGHT - 1"?
Nope. :) EDID_LENGTH is defined as 128, so 128 would be off the end of 
the array. EDID_LENGTH - 1 would be the checksum itself, which I don't 
want to include when computing the checksum. So the last valid byte is 
actually byte 126, which is EDID_LENGTH - 2.
>> +               byte_total += edid_data[i];
>> +
>> +       *edid_checksum = 256 - (byte_total % 256);
>> +
>> +       if (*edid_checksum != edid_data[EDID_LENGTH - 1]) {
>> +               DRM_DEBUG_KMS("Invalid EDID checksum %02x, should be %02x\n",
>> +                             edid_data[EDID_LENGTH - 40 - 1], *edid_checksum);
>> +               edid_ok = false;
>> +       }
>> +
>> +       return edid_ok;
>> +}
>> +
>>   static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
>>   {
>> -       uint8_t test_result = DP_TEST_NAK;
>> +       struct drm_connector *connector = &intel_dp->attached_connector->base;
>> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
>> +       struct edid *edid_read = NULL;
>> +       uint8_t *edid_data = NULL;
>> +       uint8_t test_result = DP_TEST_NAK, checksum = 0;
>> +       uint32_t ret = 0;
>> +
>> +       intel_dp->aux.i2c_nack_count = 0;
>> +       intel_dp->aux.i2c_defer_count = 0;
>> +
>> +       edid_read = drm_get_edid(connector, adapter);
>> +
>> +       if (edid_read == NULL) {
>> +               /* Check for NACKs/DEFERs, use failsafe if detected
>> +                  (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
>> +               if (intel_dp->aux.i2c_nack_count > 0 ||
>> +                       intel_dp->aux.i2c_defer_count > 0)
>> +                       DRM_DEBUG_KMS("EDID read had %d NACKs, %d DEFERs\n",
>> +                                     intel_dp->aux.i2c_nack_count,
>> +                                     intel_dp->aux.i2c_defer_count);
> Don't we need to use these _count values somehow, instead of just
> printing them in the logs?
>
> Everything else looks fine.
In this instance, no. All that is required is for the source device to 
appropriately respond to receiving an I2C DEFER or NACK from the sink 
device. If those counters are above 0 for purposes of compliance 
testing, that triggers the corrupt/invalid EDID response from the test 
mechanism.

In reality, those counters could be tracked for a variety of reasons; 
one example is that they could used to slow down I2C transactions for 
sink devices with high response latencies, so that the DEFERs or NACKs 
could be reduced or avoided entirely.
>> +               intel_dp->compliance_test_data = INTEL_DP_EDID_CORRUPT |
>> +                                                INTEL_DP_RESOLUTION_FAILSAFE;
>> +       } else {
>> +               edid_data = (uint8_t *) edid_read;
>> +
>> +               if (intel_dp_compute_edid_checksum(edid_data, &checksum)) {
>> +                       ret = drm_dp_dpcd_write(&intel_dp->aux,
>> +                                               DP_TEST_EDID_CHECKSUM,
>> +                                               &edid_read->checksum, 1);
>> +                       test_result = DP_TEST_ACK |
>> +                                     DP_TEST_EDID_CHECKSUM_WRITE;
>> +                       intel_dp->compliance_test_data =
>> +                               INTEL_DP_EDID_OK |
>> +                               INTEL_DP_RESOLUTION_PREFERRED;
>> +               } else {
>> +                       /* Invalid checksum - EDID corruption detection */
>> +                       intel_dp->compliance_test_data =
>> +                               INTEL_DP_EDID_CORRUPT |
>> +                               INTEL_DP_RESOLUTION_FAILSAFE;
>> +               }
>> +       }
>> +
>>          return test_result;
>>   }
>>
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

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

Re: [PATCH 12/17] drm/i915: Update intel_dp_compute_config() to handle compliance test requests

[Todd Previte]

On 1/7/15 12:28 PM, Clint Taylor wrote:
> On 12/17/2014 09:04 AM, Paulo Zanoni wrote:
>> 2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>:
>>> Adds provisions in intel_dp_compute_config() to accommodate compliance
>>> testing. Mostly this invovles circumventing the automatic link 
>>> configuration
>>> parameters and allowing the compliance code to set those parameters as
>>> required by the tests.
>>>
>>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++++++++++
>>>   1 file changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c 
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index 2a13124..4a55ca6 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -1189,6 +1189,21 @@ intel_dp_compute_config(struct intel_encoder 
>>> *encoder,
>>>          pipe_config->has_drrs = false;
>>>          pipe_config->has_audio = intel_dp->has_audio;
>>>
>>> +       /* Compliance testing should skip most of this function */
>>> +       if (!is_edp(intel_dp) && intel_dp->compliance_testing_active) {
>>
>> I couldn't find any patch on your series that flips
>> intel_dp->compliance_testing_active to true, which is weird since it
>> would prevent us from testing the code.
>>
>> Also, if we can make sure that we never set compliance_testing_active
>> to true on eDP, we can remove the is_edp() check.
>
> Why would we not allow automation compliance testing on eDP? There are 
> automation tests and fixtures from Unigraf and Agilent for eDP.
>
> -Clint
eDP has different testing requirements and is a completely different 
specification than the one for regular Displayport. This patch set is 
for external Displayport connections only in accordance with the 
Displayport Link CTS 1.2 Core rev 1.1a document.

>
>>
>>> +               bpp = intel_dp->compliance_config.bits_per_pixel;
>>> +               lane_count = intel_dp->compliance_config.lane_count;
>>> +               clock = intel_dp->compliance_config.link_rate >> 3;
>>> +               /* Assign here and skip at the end - ensures correct 
>>> values */
>>> +               intel_dp->link_bw = bws[clock];
>>> +               intel_dp->lane_count = lane_count;
>>> +               pipe_config->pipe_bpp = bpp;
>>> +               pipe_config->port_clock =
>>> + drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
>>> +
>>> +               goto compliance_exit;
>>> +       }
>>> +
>>>          if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
>>> intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
>>>                                         adjusted_mode);
>>> @@ -1275,6 +1290,7 @@ found:
>>>          DRM_DEBUG_KMS("DP link bw required %i available %i\n",
>>>                        mode_rate, link_avail);
>>>
>>> +compliance_exit:
>>
>> Don't we need to move the color range adjustments to this point?
>>
>>>          intel_link_compute_m_n(bpp, lane_count,
>>>                                 adjusted_mode->crtc_clock,
>>>                                 pipe_config->port_clock,
>>> -- 
>>> 1.9.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>

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

Re: [PATCH 13/17] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation

[Todd Previte]

[-- Attachment #1.1: Type: text/plain, Size: 5153 bytes --]

On 12/17/14 10:57 AM, Paulo Zanoni wrote:
> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>:
>> Moves the non-MST case out of the if-statement and places it at the beginning
>> of the function to handle HPD events for SST mode. The reasoning behind this
>> is to accommodate link status checks for compliance testing. Some test devices
>> use long pulses to perform test requests so link status must be checked
>> regardless of the pulse width for the SST operational mode.
> Can you please elaborate a little more on what do you see on these
> devices? The test spec is very clear about short vs long HPD pulses,
> so it's hard to believe a test device would get this wrong. We have
> some registers on the PCH that allow us to redefine short vs long
> durations. Have you tried to play with them?
>
> More below:
The issue is not in differentiating between the two pulse widths. The 
problem is that compliance testing mixes the two together, i.e. some 
tests are IRQ events where the source has to "service" the sink while 
other tests are examining the hot plug detection and response 
functionality. So when it comes to compliance testing, they both have to 
be handled and checked to see if there's a test request coming in or if 
it's a real HPD event of some kind. The current implementation only 
checks SST mode for the short pulse case.

In light of Daniel's comment, though, that makes this code even more 
broken. The best place to handle this (as I mention in my response to 
Daniel) is to place the SST case after the mst_fail tag. That way, once 
it is determined that MST mode is not in use, the SST handler can be 
invoked and events responded to appropriately.


>> This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
>> previous compliance testing patch sequence. Review feedback on that patch
>> indicated that updating intel_dp_hot_plug() was not the correct place for
>> the test handler.
>>
>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++----------
>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 4a55ca6..73014d8 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>          power_domain = intel_display_port_power_domain(intel_encoder);
>>          intel_display_power_get(dev_priv, power_domain);
>>
>> +       if (!intel_dp->is_mst) {
>> +               /*
>> +                *  Pulse width doesn't matter for SST mode
>> +                *  Handle the HPD event now
>> +               */
>> +               drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> +               intel_dp_check_link_status(intel_dp);
> The very first thing intel_dp_check_link_status() does is to return in
> case "connector->base.status != connected". If we're getting a long
> HPD, it doesn't seem make sense to check this field because the status
> might be changing due to the long HPD.
Long pulses are either connect OR disconnect events. In the case of the 
test device, what happens is that it's already connected, so HPD is 
asserted. It pulses the HPD line low for > 2ms and then reasserts it 
sometime later once the source device has disabled the main link. So it 
becomes a 2-fold event for us - the initial connected->disconnected 
transition and the following disconnected->connected transition. Both of 
those invoke our IRQ handler and cause neat and exciting things to 
happen. For compliance testing, the disconnect event needs to be ignored 
(by the test code, anyways) and the connect event needs to be checked 
for TEST_REQUEST=1. Thus, it absolutely makes sense to check this here.

>> +               drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +               ret = false;
>> +               goto put_power;
>> +       }
>> +
>>          if (long_hpd) {
>>
>>                  if (HAS_PCH_SPLIT(dev)) {
>> @@ -4637,16 +4649,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>                          if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
>>                                  goto mst_fail;
>>                  }
>> -
>> -               if (!intel_dp->is_mst) {
>> -                       /*
>> -                        * we'll check the link status via the normal hot plug path later -
>> -                        * but for short hpds we should check it now
>> -                        */
>> -                       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> -                       intel_dp_check_link_status(intel_dp);
>> -                       drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> -               }
>>          }
>>          ret = false;
>>          goto put_power;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>


[-- Attachment #1.2: Type: text/html, Size: 6469 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 13/17] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation

[Todd Previte]

On 12/17/14 1:30 PM, Daniel Vetter wrote:
> On Wed, Dec 17, 2014 at 03:57:21PM -0200, Paulo Zanoni wrote:
>> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>:
>>> Moves the non-MST case out of the if-statement and places it at the beginning
>>> of the function to handle HPD events for SST mode. The reasoning behind this
>>> is to accommodate link status checks for compliance testing. Some test devices
>>> use long pulses to perform test requests so link status must be checked
>>> regardless of the pulse width for the SST operational mode.
>> Can you please elaborate a little more on what do you see on these
>> devices? The test spec is very clear about short vs long HPD pulses,
>> so it's hard to believe a test device would get this wrong. We have
>> some registers on the PCH that allow us to redefine short vs long
>> durations. Have you tried to play with them?
>>
>> More below:
>>
>>> This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
>>> previous compliance testing patch sequence. Review feedback on that patch
>>> indicated that updating intel_dp_hot_plug() was not the correct place for
>>> the test handler.
>>>
>>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++----------
>>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 4a55ca6..73014d8 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>>          power_domain = intel_display_port_power_domain(intel_encoder);
>>>          intel_display_power_get(dev_priv, power_domain);
>>>
>>> +       if (!intel_dp->is_mst) {
>>> +               /*
>>> +                *  Pulse width doesn't matter for SST mode
>>> +                *  Handle the HPD event now
>>> +               */
>>> +               drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>> +               intel_dp_check_link_status(intel_dp);
>> The very first thing intel_dp_check_link_status() does is to return in
>> case "connector->base.status != connected". If we're getting a long
>> HPD, it doesn't seem make sense to check this field because the status
>> might be changing due to the long HPD.
> I don't think we can unconditionally run SST hpd logic before we've
> correctly handled mst mode. It likely screws up the accounting.

Upon further review, it looks like the best solution here is to place 
the SST code in the mst_fail case as the 'else' clause. That way SST 
mode stuff gets handled regardless and if a need arises in the future 
where differentiating between short and long becomes necessary, there's 
a place to handle it. This will be in V3.

>>> +               drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>> +               ret = false;
>>> +               goto put_power;
>>> +       }
>>> +
>>>          if (long_hpd) {
>>>
>>>                  if (HAS_PCH_SPLIT(dev)) {
>>> @@ -4637,16 +4649,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>>                          if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
>>>                                  goto mst_fail;
>>>                  }
>>> -
>>> -               if (!intel_dp->is_mst) {
>>> -                       /*
>>> -                        * we'll check the link status via the normal hot plug path later -
>>> -                        * but for short hpds we should check it now
>>> -                        */
> Just aside: The above comment is outdated and can be remove. This is now
> the only place where we handle link retraining. The function could be made
> static and dropped from headers, too.
> -Daniel
Can't be static and pulled from the header. It's called in intel_ddi.c 
as well.

But the comment and the code is gone, in light of the above changes.

>>> -                       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>> -                       intel_dp_check_link_status(intel_dp);
>>> -                       drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>> -               }
>>>          }
>>>          ret = false;
>>>          goto put_power;
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> -- 
>> Paulo Zanoni
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 14/17] drm/i915: Add debugfs function to check connector status for compliance testing

[Todd Previte]

On 12/17/14 11:03 AM, Paulo Zanoni wrote:
> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>:
>> Adds a function to check the status of a Displayport connector. This function
>> simplifies the Displayport compliance testing interface in debugfs by providing
>> a single method for determining the status of a connector during Displayport
>> compliance testing. This function checks whether or not the connector is a
>> Displayport connector and whether or not that connector is active.
>>
> I see patches 14 and 15 rewrite some code that was added by patches 7
> and 10. Instead of doing this, please make sure patch 7 already
> contains your dp_connector_is_valid() function, then patch 10 just
> calls it.
More code for the monster patch... ;) These have been added in for V3.
>
> Also, please remove those "goto" and just use plain "return".
Done. It was an attempt to make the code cleaner and have a single point 
of exit, but in reality, it's not necessary and doesn't achieve its 
intended goal. They've been replaced with returns for V3.

>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 5eb6c24..978ddd1 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -3920,6 +3920,35 @@ static int displayport_parse_config(char *input_buffer,
>>          return status;
>>   }
>>
>> +static bool dp_connector_is_valid(struct drm_connector *connector,
>> +                                 bool check_active)
>> +{
>> +       struct intel_encoder *intel_encoder;
>> +       struct intel_connector *intel_connector;
>> +
>> +       intel_connector = to_intel_connector(connector);
>> +       intel_encoder = intel_connector->encoder;
>> +
>> +       if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort)
>> +               goto no;
>> +
>> +       if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT &&
>> +           intel_encoder->type != INTEL_OUTPUT_UNKNOWN)
>> +               goto no;
>> +
>> +       if (check_active) {
>> +               if (connector->status == connector_status_connected)
>> +                       goto yes;
>> +               else
>> +                       goto no;
>> +       }
>> +
>> +yes:
>> +       return true;
>> +no:
>> +       return false;
>> +}
>> +
>>   static int displayport_config_ctl_show(struct seq_file *m, void *data)
>>   {
>>          struct drm_device *dev = m->private;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

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

Re: [PATCH 14/17] drm/i915: Add debugfs function to check connector status for compliance testing

[Todd Previte]

After reviewing the code again, with the changes from the previous 
comments, it looks like the dp_connector_is_valid() function is 
superfluous. So this patch and patch 15 can be disregarded. I've added 
an additional line to the config_ctl_write function to also check to 
make sure the connector is active before performing the write 
operations. The commit message for the monster patch where these are 
implemented has been updated to reflect this change as well.

-T

On 2/18/15 10:08 AM, Todd Previte wrote:
> On 12/17/14 11:03 AM, Paulo Zanoni wrote:
>> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>:
>>> Adds a function to check the status of a Displayport connector. This
>>> function
>>> simplifies the Displayport compliance testing interface in debugfs
>>> by providing
>>> a single method for determining the status of a connector during
>>> Displayport
>>> compliance testing. This function checks whether or not the
>>> connector is a
>>> Displayport connector and whether or not that connector is active.
>>>
>> I see patches 14 and 15 rewrite some code that was added by patches 7
>> and 10. Instead of doing this, please make sure patch 7 already
>> contains your dp_connector_is_valid() function, then patch 10 just
>> calls it.
> More code for the monster patch... ;) These have been added in for V3.
>>
>> Also, please remove those "goto" and just use plain "return".
> Done. It was an attempt to make the code cleaner and have a single
> point of exit, but in reality, it's not necessary and doesn't achieve
> its intended goal. They've been replaced with returns for V3.
>
>>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_debugfs.c | 29
>>> +++++++++++++++++++++++++++++
>>>   1 file changed, 29 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 5eb6c24..978ddd1 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -3920,6 +3920,35 @@ static int displayport_parse_config(char
>>> *input_buffer,
>>>          return status;
>>>   }
>>>
>>> +static bool dp_connector_is_valid(struct drm_connector *connector,
>>> +                                 bool check_active)
>>> +{
>>> +       struct intel_encoder *intel_encoder;
>>> +       struct intel_connector *intel_connector;
>>> +
>>> +       intel_connector = to_intel_connector(connector);
>>> +       intel_encoder = intel_connector->encoder;
>>> +
>>> +       if (connector->connector_type !=
>>> DRM_MODE_CONNECTOR_DisplayPort)
>>> +               goto no;
>>> +
>>> +       if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT &&
>>> +           intel_encoder->type != INTEL_OUTPUT_UNKNOWN)
>>> +               goto no;
>>> +
>>> +       if (check_active) {
>>> +               if (connector->status == connector_status_connected)
>>> +                       goto yes;
>>> +               else
>>> +                       goto no;
>>> +       }
>>> +
>>> +yes:
>>> +       return true;
>>> +no:
>>> +       return false;
>>> +}
>>> +
>>>   static int displayport_config_ctl_show(struct seq_file *m, void
>>> *data)
>>>   {
>>>          struct drm_device *dev = m->private;
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>

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

Re: [PATCH 11/17] drm/i915: Update the EDID automated compliance test function

[Daniel Vetter]

Readding intel-gfx.

On Wed, Feb 18, 2015 at 5:57 PM, Todd Previte <tprevite@gmail.com> wrote:
> On 12/17/14 1:20 PM, Daniel Vetter wrote:
>> Just something random I've spotted while driving by: drm_get_edid does all
>> the checksum stuff for you already (it retries up to 4 times if the
>> checkusm is off and also checks a few other things). We should never reach
>> this case and the checksum function is essentially dead code.
>>
>> Or do I miss something?
>> -Daniel
>>
>
> The data I have is that when connected to the DPR-100, the logs show that
> the EDID checksum is incorrect from the DRM routines, which is why I went
> and wrote this one. This is most likely a problem with the DPR-100 (that
> they won't fix since they're not updating it anymore) and possibly still a
> problem with the DPR-120 (which I can't test because I don't have one). So
> this code is necessary in order to pass the EDID tests when using the
> DPR-100.
>
> That said, it's also unlikely that the DRM routines are wholly wrong, since
> that would result in many, many errors in the logs for all the shipping DP
> monitors out there. So this is one case where special code is necessary in
> order to pass the tests with a particular test device.

If that's the case then we need to add some quirks to the core edid
code (we have plenty of those already, shipping hw tends to be broken
in all sorts of interesting ways). At least I don't see a point in
passing compliance in such an important aspect as validating sink
behaviour by adding special-purpose code instead of the code we use
for real hw.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/17] drm/i915: Update the EDID automated compliance test function

[Daniel Vetter]

On Wed, Dec 10, 2014 at 04:53:11PM -0700, Todd Previte wrote:
> Updates the EDID compliance test function to perform the EDID read as
> required by the tests. This read needs to take place in the kernel for
> reasons of speed and efficiency. The results of the EDID read are handed
> off to userspace so that the remainder of the test can be conducted there.
> 
> V2:
> - Addressed mailing list feedback
> - Removed excess debug messages
> - Removed extraneous comments
> - Fixed formatting issues (line length > 80)
> - Updated the debug message in compute_edid_checksum to output hex values
>   instead of decimal
> 
> Signed-off-by: Todd Previte <tprevite@gmail.com>

Returning the abstract discussion about where to put the edid checksum
checks back to the concrete patch at hand.

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b6f5a72..2a13124 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -40,6 +40,13 @@
>  
>  #define DP_LINK_CHECK_TIMEOUT	(10 * 1000)
>  
> +/* Compliance test status bits  */
> +#define  INTEL_DP_EDID_OK		(0<<0)
> +#define  INTEL_DP_EDID_CORRUPT		(1<<0)
> +#define  INTEL_DP_RESOLUTION_PREFERRED	(1<<2)
> +#define  INTEL_DP_RESOLUTION_STANDARD	(1<<3)
> +#define  INTEL_DP_RESOLUTION_FAILSAFE	(1<<4)
> +
>  struct dp_link_dpll {
>  	int link_bw;
>  	struct dpll dpll;
> @@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>  	return test_result;
>  }
>  
> +static bool intel_dp_compute_edid_checksum(uint8_t *edid_data,
> +					   uint8_t *edid_checksum)
> +{
> +	uint32_t byte_total = 0;
> +	uint8_t i = 0;
> +	bool edid_ok = true;
> +
> +	/* Don't include last byte (the checksum) in the computation */
> +	for (i = 0; i < EDID_LENGTH - 2; i++)
> +		byte_total += edid_data[i];
> +
> +	*edid_checksum = 256 - (byte_total % 256);
> +
> +	if (*edid_checksum != edid_data[EDID_LENGTH - 1]) {
> +		DRM_DEBUG_KMS("Invalid EDID checksum %02x, should be %02x\n",
> +			      edid_data[EDID_LENGTH - 40 - 1], *edid_checksum);
> +		edid_ok = false;
> +	}
> +
> +	return edid_ok;
> +}
> +
>  static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
>  {
> -	uint8_t test_result = DP_TEST_NAK;
> +	struct drm_connector *connector = &intel_dp->attached_connector->base;
> +	struct i2c_adapter *adapter = &intel_dp->aux.ddc;
> +	struct edid *edid_read = NULL;
> +	uint8_t *edid_data = NULL;
> +	uint8_t test_result = DP_TEST_NAK, checksum = 0;
> +	uint32_t ret = 0;
> +
> +	intel_dp->aux.i2c_nack_count = 0;
> +	intel_dp->aux.i2c_defer_count = 0;
> +
> +	edid_read = drm_get_edid(connector, adapter);
> +
> +	if (edid_read == NULL) {

So if the edid core thinks your edid is foul (e.g. checksum mismatch) you
wont ever see the edid and land in this case.

> +		/* Check for NACKs/DEFERs, use failsafe if detected
> +		   (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
> +		if (intel_dp->aux.i2c_nack_count > 0 ||
> +			intel_dp->aux.i2c_defer_count > 0)
> +			DRM_DEBUG_KMS("EDID read had %d NACKs, %d DEFERs\n",
> +				      intel_dp->aux.i2c_nack_count,
> +				      intel_dp->aux.i2c_defer_count);
> +		intel_dp->compliance_test_data = INTEL_DP_EDID_CORRUPT |
> +						 INTEL_DP_RESOLUTION_FAILSAFE;

And return the above error condition of "corrupt edid" to the tester.

> +	} else {
> +		edid_data = (uint8_t *) edid_read;
> +
> +		if (intel_dp_compute_edid_checksum(edid_data, &checksum)) {
> +			ret = drm_dp_dpcd_write(&intel_dp->aux,
> +						DP_TEST_EDID_CHECKSUM,
> +						&edid_read->checksum, 1);
> +			test_result = DP_TEST_ACK |
> +				      DP_TEST_EDID_CHECKSUM_WRITE;
> +			intel_dp->compliance_test_data =
> +				INTEL_DP_EDID_OK |
> +				INTEL_DP_RESOLUTION_PREFERRED;
> +		} else {

Which means except when there is some difference between your checksum
code and the drm_edid.c checksum code this case here is dead code. And if
there _is_ a difference then they'll never agree, so the "edid ok" case
above can't happen.  Either way one of the two sides of this if is dead
code (at least if I haven't missed something).

That's essentially the reason why I think we should fixup the checksum
code in drm_edid.c and drop this.
-Daniel

> +			/* Invalid checksum - EDID corruption detection */
> +			intel_dp->compliance_test_data =
> +				INTEL_DP_EDID_CORRUPT |
> +				INTEL_DP_RESOLUTION_FAILSAFE;
> +		}
> +	}
> +
>  	return test_result;
>  }
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/17] drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing

[Paulo Zanoni]

2015-02-18 14:36 GMT-02:00 Todd Previte <tprevite@gmail.com>:
>
> On 12/15/2014 9:36 AM, Paulo Zanoni wrote:
>>
>> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>:
>>>
>>> Move the DPCD read to the top and check for an interrupt from the sink to
>>> catch
>>> Displayport automated testing requests necessary to support Displayport
>>> compliance
>>> testing. The checks for active connectors and link status are moved below
>>> the
>>> check for the interrupt.
>>
>> Why exactly is this needed?
>
> The main reason for doing this is to make sure that a test request isn't
> missed. Checking for the status of the encoder/crtc isn't necessary for some
> test cases (AUX channel tests are one example) and without moving the check
> for the interrupt, these tests may not execute if one of those checks fails.
> Additionally, if reading the DPCD fails, regardless of whether or not
> testing is happening, there's no way to train the link since configurations
> and status can't be read, nor can link training parameters be written.
>
>
>>> Adds a check at the top to verify that the device is connected. This is
>>> necessary
>>> for DP compliance testing to ensure that test requests are captured and
>>> acknowledged.
>>
>> Why exactly? Can you please describe in terms of how the code is
>> executed in each case and what is missing?
>
> This patch is actually both a bug fix and a component of compliance testing.
> Because HPD events are received both on connect and disconnect actions, it's
> vital that we don't try and train the link when we're transitioning from
> connected->disconnected. That results in errors and warning in the logs from
> failed AUX transactions and can trigger the WARN for the check of
> !base.crtc. By making the check at the beginning to see if the connection is
> truly active, those problems are avoided and testing / link training will
> only be attempted when there is a valid Displayport connection.
>
>> Since there appears to be 2 different changes, shouldn't this patch be
>> split into 2 different patches?
>
> It can be split into two if that will make it easier to upstream. The
> changes are separate but related, which is why I grouped them into a single
> patch.

The big reason for splitting is the bisectability. In case we ever
bisect a bug to this patch, we'll probably have to ask the user to
test additional patches so we can check which of the logical changes
actually introduced the problem. Anyway, since it's still a small
patch, I don't think it's a big problem, so splitting is not a hard
requirement IMHO.

>
>
>>> If a test request is present during a connected->disconnected transition,
>>> the test code will attempt to execute even though the connection has been
>>> disabled,
>>> resulting in a faied test.
>>>
>>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c | 32 +++++++++++++++++++-------------
>>>   1 file changed, 19 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index 3dc92a3..1b452cc 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -3890,21 +3890,14 @@ intel_dp_check_link_status(struct intel_dp
>>> *intel_dp)
>>>
>>>
>>> WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>>
>>> -       if (!intel_encoder->connectors_active)
>>> -               return;
>>> -
>>> -       if (WARN_ON(!intel_encoder->base.crtc))
>>> -               return;
>>> -
>>> -       if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>>> -               return;
>>> -
>>> -       /* Try to read receiver status if the link appears to be up */
>>> -       if (!intel_dp_get_link_status(intel_dp, link_status)) {
>>> +       /* Bail if not connected */
>>
>> Bikeshed: I don't see the reason for the comment above :)
>>
>> Other than that, the patch looks fine. Let's see what PRTS will say about
>> it.
>
>
> Fair enough. Comment deleted. :) Fix will be in patch V3. The above
> responses to your questions were also included in the patch notes for
> further explanation of this patch.
>
>
>>> +       if (intel_dp->attached_connector->base.status !=
>>> +           connector_status_connected) {
>>> +               DRM_DEBUG_KMS("Not connected\n");
>>>                  return;
>>>          }
>>>
>>> -       /* Now read the DPCD to see if it's actually running */
>>> +       /* Attempt to read the DPCD */
>>>          if (!intel_dp_get_dpcd(intel_dp)) {
>>>                  return;
>>>          }
>>> @@ -3916,13 +3909,26 @@ intel_dp_check_link_status(struct intel_dp
>>> *intel_dp)
>>>                  drm_dp_dpcd_writeb(&intel_dp->aux,
>>>                                     DP_DEVICE_SERVICE_IRQ_VECTOR,
>>>                                     sink_irq_vector);
>>> -
>>>                  if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
>>>                          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");
>>>          }
>>>
>>> +       if (!intel_encoder->connectors_active)
>>> +               return;
>>> +
>>> +       if (WARN_ON(!intel_encoder->base.crtc))
>>> +               return;
>>> +
>>> +       if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>>> +               return;
>>> +
>>> +       /* Try to read receiver status if the link appears to be up */
>>> +       if (!intel_dp_get_link_status(intel_dp, link_status)) {
>>> +               return;
>>> +       }
>>> +
>>>          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);
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>



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