[intel-gfx] Displayport compliance testing

[intel-gfx] Displayport compliance testing

[Todd Previte]

The kernel side is responsible for the acknowledgement of the test requests and 
setup of the required parameters. It also handles the necessary AUX transactions 
for reading the EDID and DPCD as well as writing response codes or checksums as 
necessary. Performing these operations in userspace would add unnecessary delays 
and complicate the interface more than necessary. The userspace application then 
handles the large motions - frame buffer management, mode sets and link 
configuration. The following is an overview of the basic event handling for 
compliance testing inside the kernel:
        - The test device  signals the DUT with an HPD pulse. This can  be a 
          short or long pulse, depending on circumstances.
        - The interrupt generated by the HPD pulse invokes the test handler, 
          which reads the test device DPCD to determine what actions are necessary.
        - Once the test handler determines which test has been requested, it 
          invokes the kernel-side handler function and then signals the userspace 
          app. If no userspace app has been registered, the signal is ignored.
        - Test responses and status is written out / reported as necessary and 
          normal operation is resumed.

The userspace support application and additional documentation will be posted 
to the list for review soon.

[PATCH 01/10] 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.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 64c8e04..f7d4119 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3959,11 +3959,89 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 	return true;
 }
 
+/* Displayport compliance testing - Link training */
+static uint8_t
+intel_dp_autotest_link_training(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_NAK;
+	return test_result;
+}
+
+/* Displayport compliance testing - Video pattern testing */
+static uint8_t
+intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_NAK;
+	return test_result;
+}
+
+/* Displayport compliance testing - EDID operations */
+static uint8_t
+intel_dp_autotest_edid(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_NAK;
+	return test_result;
+}
+
+/* Displayport compliance testing - PHY pattern testing */
+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;
+
+	DRM_DEBUG_KMS("Received automated test request\n");
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
+
+	/* ACK/NAK response based on test function response
+	   Unimplemented/unsupported tests will NAK by default */
+	switch (rxdata) {
+	case DP_TEST_LINK_TRAINING:
+		DRM_DEBUG_KMS("Executing LINK_TRAINING request\n");
+		intel_dp->compliance_test_data = DP_TEST_LINK_TRAINING;
+		response = intel_dp_autotest_link_training(intel_dp);
+		break;
+	case DP_TEST_LINK_VIDEO_PATTERN:
+		DRM_DEBUG_KMS("Executing TEST_PATTERN request\n");
+		intel_dp->compliance_test_data = DP_TEST_LINK_VIDEO_PATTERN;
+		response = intel_dp_autotest_video_pattern(intel_dp);
+		break;
+	case DP_TEST_LINK_EDID_READ:
+		DRM_DEBUG_KMS("Executing EDID request\n");
+		intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ;
+		response = intel_dp_autotest_edid(intel_dp);
+		break;
+	case DP_TEST_LINK_PHY_TEST_PATTERN:
+		DRM_DEBUG_KMS("Executing PHY_PATTERN request\n");
+		intel_dp->compliance_test_data = DP_TEST_LINK_PHY_TEST_PATTERN;
+		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;
+	/* Unsupported test case or something went wrong */
+	default:
+		DRM_DEBUG_KMS("Unhandled test request\n");
+		break;
+	}
+	if (status != 0) {
+		response = DP_TEST_NAK;
+		DRM_DEBUG_KMS("Error %d processing test request\n", status);
+	}
+	status = drm_dp_dpcd_write(&intel_dp->aux,
+				   DP_TEST_RESPONSE,
+				   &response, 1);
+	intel_dp->compliance_testing_active = 0;
+
 }
 
 static int
-- 
1.9.1

[PATCH 02/10] drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs

[Todd Previte]

These counters are used for Displayort complinace testing to detect error conditions
when executing certain compliance tests. Currently these are used in the EDID tests
to determine if the video mode needs to be set to the preferred mode or the failsafe
mode.

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 2 ++
 include/drm/drm_dp_helper.h     | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 08e33b8..8353051 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 
 		case DP_AUX_I2C_REPLY_NACK:
 			DRM_DEBUG_KMS("I2C nack\n");
+			aux->i2c_nack_count++;
 			return -EREMOTEIO;
 
 		case DP_AUX_I2C_REPLY_DEFER:
 			DRM_DEBUG_KMS("I2C defer\n");
+			aux->i2c_defer_count++;
 			usleep_range(400, 500);
 			continue;
 
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 8edeed0..45f3ee8 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -551,6 +551,7 @@ struct drm_dp_aux {
 	struct mutex hw_mutex;
 	ssize_t (*transfer)(struct drm_dp_aux *aux,
 			    struct drm_dp_aux_msg *msg);
+	uint8_t i2c_nack_count, i2c_defer_count;
 };
 
 ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
-- 
1.9.1

[PATCH 03/10] 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 f7d4119..952dfa3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4119,21 +4119,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;
 	}
@@ -4145,13 +4138,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

[PATCH 04/10] 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.

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 952dfa3..a7acc61 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -799,9 +799,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)) {
+				udelay(400);
 				continue;
+			}
 			if (status & DP_AUX_CH_CTL_DONE)
 				break;
 		}
-- 
1.9.1

[PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing

[Todd Previte]

Adds an interface that allows for Displayport configuration information to be accessed
through debugfs. The information paramters provided here are as follows:
   Link rate
   Lane count
   Bits per pixel
   Voltage swing level
   Preemphasis level
   Display mode

These parameters are intended to be used by userspace applications that need access
to the link configuration of any active Displayport connection. Additionally,
applications can make adjustments to these parameters through this interface to
allow userspace application to have fine-grained control over link training
paramters.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 389 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c     |  13 +-
 2 files changed, 397 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index da4036d..2dada18 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
@@ -3505,6 +3546,353 @@ 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;
+	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) {
+				pipe_bpp = intel_encoder->new_crtc->config.pipe_bpp;
+				mode = &intel_encoder->new_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,
+				   intel_connector->base.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;
+	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;
+}
+
+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_link_config *dp_conf)
+{
+	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;
+			}
+		}
+	}
+
+	for (i = 1; i < DP_PARAMETER_COUNT; i++)
+		DRM_DEBUG_DRIVER("%s = %lu\n",
+					dp_conf_tokens[parms[i].type],
+					parms[i].value);
+
+	/* Validate any/all incoming parameters */
+	strcpy(dp_conf->connector_name,
+	       (char *)parms[DP_CONFIG_PARAM_CONNECTOR].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) {
+		dp_conf->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) {
+		dp_conf->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) {
+		dp_conf->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) {
+		dp_conf->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) {
+		dp_conf->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) {
+		dp_conf->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) {
+		dp_conf->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;
+	struct drm_connector *connector;
+	struct intel_encoder *intel_encoder;
+	struct intel_connector *intel_connector;
+
+	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;
+		if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
+			if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
+			    intel_encoder->type == INTEL_OUTPUT_UNKNOWN) {
+				if (connector->status == connector_status_connected) {
+					displayport_show_config_info(m, intel_connector);
+				} else {
+					seq_printf(m, DP_CONF_STR_CONNECTOR,
+						   intel_connector->base.name);
+					seq_printf(m, "disconnected\n");
+				}
+			}
+		}
+	}
+
+	return 0;
+}
+
+static int displayport_config_ctl_open(struct inode *inode,
+				       struct file *file)
+{
+	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)
+{
+	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;
+	struct intel_dp_link_config dp_conf;
+
+	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';
+	memset(&dp_conf, 0, sizeof(struct intel_dp_link_config));
+	DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
+	status = displayport_parse_config(input_buffer, len, &dp_conf);
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+		intel_connector = to_intel_connector(connector);
+		intel_encoder = intel_connector->encoder;
+		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(&intel_encoder->base);
+				memcpy(&intel_dp->compliance_config,
+				       &dp_conf,
+				       sizeof(struct intel_dp_link_config));
+			}
+		}
+	}
+
+
+out:
+	kfree(input_buffer);
+	if (status < 0)
+		return status;
+
+	*offp += len;
+	return len;
+}
+
+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[5])
 {
 	struct drm_device *dev = m->private;
@@ -4208,6 +4596,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)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a7acc61..b3ddd15 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3968,7 +3968,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 static uint8_t
 intel_dp_autotest_link_training(struct intel_dp *intel_dp)
 {
-	uint8_t test_result = DP_TEST_NAK;
+	uint8_t test_result = DP_TEST_ACK;
 	return test_result;
 }
 
@@ -4013,21 +4013,25 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)
 		DRM_DEBUG_KMS("Executing LINK_TRAINING request\n");
 		intel_dp->compliance_test_data = DP_TEST_LINK_TRAINING;
 		response = intel_dp_autotest_link_training(intel_dp);
+		status = intel_dp_signal_userspace(intel_dp);
 		break;
 	case DP_TEST_LINK_VIDEO_PATTERN:
 		DRM_DEBUG_KMS("Executing TEST_PATTERN request\n");
 		intel_dp->compliance_test_data = DP_TEST_LINK_VIDEO_PATTERN;
 		response = intel_dp_autotest_video_pattern(intel_dp);
+		status = intel_dp_signal_userspace(intel_dp);
 		break;
 	case DP_TEST_LINK_EDID_READ:
 		DRM_DEBUG_KMS("Executing EDID request\n");
 		intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ;
 		response = intel_dp_autotest_edid(intel_dp);
+		status = intel_dp_signal_userspace(intel_dp);
 		break;
 	case DP_TEST_LINK_PHY_TEST_PATTERN:
 		DRM_DEBUG_KMS("Executing PHY_PATTERN request\n");
 		intel_dp->compliance_test_data = DP_TEST_LINK_PHY_TEST_PATTERN;
 		response = intel_dp_autotest_phy_pattern(intel_dp);
+		status = intel_dp_signal_userspace(intel_dp);
 		break;
 		/* FAUX is optional in DP 1.2*/
 	case DP_TEST_LINK_FAUX_PATTERN:
@@ -4038,10 +4042,9 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)
 		DRM_DEBUG_KMS("Unhandled test request\n");
 		break;
 	}
-	if (status != 0) {
-		response = DP_TEST_NAK;
-		DRM_DEBUG_KMS("Error %d processing test request\n", status);
-	}
+	if (status != 0)
+		DRM_DEBUG_KMS("Status code %d processing test request\n",
+			      status);
 	status = drm_dp_dpcd_write(&intel_dp->aux,
 				   DP_TEST_RESPONSE,
 				   &response, 1);
-- 
1.9.1

[PATCH 06/10] drm/i915: Add debugfs interface and support functions to notify userspace apps for Displayport compliance testing

[Todd Previte]

Adds the capability for the driver to signal a userspace application for Displayport
compliance testing. The userspace app must write its PID to the appropriate debugfs file,
at which time the kernel will read and store that PID internally. PIDs are specified on a
per-connector basis.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 2dada18..d16612c 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 PID_BUFFER_SIZE			16
 
 enum dp_config_param {
 	DP_CONFIG_PARAM_INVALID = -1,
@@ -3884,6 +3885,126 @@ out:
 	return len;
 }
 
+static int displayport_user_pid_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;
+		DRM_DEBUG_DRIVER("Got connector %s with type %08x\n",
+				 connector->name, connector->connector_type);
+		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(&intel_encoder->base);
+				seq_printf(m, "%s: %lu\n",
+					   connector->name,
+					   intel_dp->compliance_app_pid);
+			}
+		}
+	}
+
+	return 0;
+}
+
+static int i915_displayport_user_pid_open(struct inode *inode,
+					  struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+
+	return single_open(file, displayport_user_pid_show, dev);
+}
+
+static ssize_t displayport_user_pid_write(struct file *file,
+					    const char __user *ubuf,
+					    size_t len, loff_t *offp)
+{
+	/* Userspace app writes its PID in here and is stored in the intel_dp
+	   struct for the connector. The PID is used when the driver receives
+	   an intrrupt for a compliance request to notify userspace of the
+	   event. */
+	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;
+	int status = 0;
+	long user_pid = 0;
+	char pidbuf[PID_BUFFER_SIZE+1];
+	char *start = pidbuf;
+
+	m = file->private_data;
+	if (!m) {
+		status = -ENODEV;
+		goto out;
+	}
+
+	dev = m->private;
+	if (!dev) {
+		status = -ENODEV;
+		goto out;
+	}
+
+	if (len > PID_BUFFER_SIZE) {
+		status = -EINVAL;
+		goto out;
+	}
+
+	if (copy_from_user(pidbuf, ubuf, len)) {
+		status = -EFAULT;
+		goto out;
+	}
+
+	pidbuf[len] = '\0';
+	/* Skip to separator */
+	while (*start != ':')
+		start++;
+	start++;
+	/* skip whitespace */
+	while (*start <= 0x20)
+		start++;
+	/* Get the PID now */
+	status = kstrtol(start, 10, &user_pid);
+	if (status < 0)
+		goto out;
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+		intel_connector = to_intel_connector(connector);
+		intel_encoder = intel_connector->encoder;
+		DRM_DEBUG_DRIVER("Got connector %s with type %08x\n",
+				 connector->name, connector->connector_type);
+		if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
+			if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
+			    intel_encoder->type == INTEL_OUTPUT_UNKNOWN) {
+				DRM_DEBUG_DRIVER("Pid %ld added to Connector %s\n",
+						 user_pid, connector->name);
+				intel_dp = enc_to_intel_dp(&intel_encoder->base);
+				intel_dp->compliance_app_pid = user_pid;
+			}
+		}
+	}
+out:
+	return status;
+}
+
+static const struct file_operations displayport_user_pid_fops = {
+	.owner = THIS_MODULE,
+	.write = displayport_user_pid_write,
+	.open = i915_displayport_user_pid_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release
+};
+
 static const struct file_operations i915_displayport_config_ctl_fops = {
 	.owner = THIS_MODULE,
 	.open = displayport_config_ctl_open,
@@ -4596,7 +4717,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_displayport_user_pid", &displayport_user_pid_fops}
 };
 
 void intel_display_crc_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b3ddd15..717cb5d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -39,6 +39,7 @@
 #include "i915_drv.h"
 
 #define DP_LINK_CHECK_TIMEOUT	(10 * 1000)
+#define SIG_DP_COMPLIANCE	47
 
 struct dp_link_dpll {
 	int link_bw;
@@ -157,6 +158,37 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp)
 	return min(source_max, sink_max);
 }
 
+int intel_dp_signal_userspace(struct intel_dp *intel_dp)
+{
+	int status = 0;
+	struct siginfo s_info;
+	struct task_struct *compliance_app;
+
+	/* Setup signal structure */
+	memset(&s_info, 0, sizeof(struct siginfo));
+	s_info.si_signo = SIG_DP_COMPLIANCE;
+	s_info.si_code = SI_QUEUE;
+	s_info.si_int = intel_dp->compliance_test_data;
+
+	rcu_read_lock();
+	compliance_app = pid_task(find_pid_ns(intel_dp->compliance_app_pid,
+					      &init_pid_ns), PIDTYPE_PID);
+	if (compliance_app == NULL) {
+		rcu_read_unlock();
+		DRM_DEBUG_DRIVER("Compliance test requested with no available userspace handler. Testing aborted\n");
+		status = -ENODEV;
+		goto out;
+	}
+	rcu_read_unlock();
+
+	/* Send signal */
+	status = send_sig_info(SIG_DP_COMPLIANCE, &s_info, compliance_app);
+	if (status < 0)
+		DRM_DEBUG_DRIVER("Error: Could not send signal\n");
+out:
+	return status;
+}
+
 /*
  * The units on the numbers in the next two are... bizarre.  Examples will
  * make it clearer; this one parallels an example in the eDP spec.
@@ -4006,6 +4038,8 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)
 	DRM_DEBUG_KMS("Received automated test request\n");
 	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
 
+	intel_dp->compliance_test_data = rxdata;
+
 	/* ACK/NAK response based on test function response
 	   Unimplemented/unsupported tests will NAK by default */
 	switch (rxdata) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e8f4839..29d51f9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -613,6 +613,10 @@ struct intel_dp {
 		enum edp_drrs_refresh_rate_type refresh_rate_type;
 		struct mutex mutex;
 	} drrs_state;
+	/* Displayport compliance testing */
+	unsigned long compliance_app_pid;
+	unsigned long compliance_test_data;
+	bool compliance_testing_active;
 
 };
 
-- 
1.9.1

[PATCH 07/10] drm/i915: Add structures for Displayport compliance testing parameters

[Todd Previte]

Adds a struct to hold link configuration parameters and several other
variables for use during Displayport compliance testing. Adding these
items here allows for efficient handling of compliance test data and
configuration parameters where necessary in the driver.

Also updates the debugfs interface to use these new parameters instead
of storing them in an intermediate structure. Values are stored on a
per-connector basis in the intel_dp struct.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d16612c..8fb3232 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3684,7 +3684,7 @@ int tokenize_dp_config(char *input_buffer, char *output[])
 
 static int displayport_parse_config(char *input_buffer,
 				    ssize_t buffer_size,
-				    struct intel_dp_link_config *dp_conf)
+				    struct intel_dp *intel_dp)
 {
 	int status = 0;
 	char *lines[MAX_DP_CONFIG_LINE_COUNT];
@@ -3715,19 +3715,11 @@ static int displayport_parse_config(char *input_buffer,
 		}
 	}
 
-	for (i = 1; i < DP_PARAMETER_COUNT; i++)
-		DRM_DEBUG_DRIVER("%s = %lu\n",
-					dp_conf_tokens[parms[i].type],
-					parms[i].value);
-
 	/* Validate any/all incoming parameters */
-	strcpy(dp_conf->connector_name,
-	       (char *)parms[DP_CONFIG_PARAM_CONNECTOR].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) {
-		dp_conf->link_rate =
+		intel_dp->compliance_config.link_rate =
 			parms[DP_CONFIG_PARAM_LINK_RATE].value;
 	} else
 		return -EINVAL;
@@ -3735,21 +3727,21 @@ static int displayport_parse_config(char *input_buffer,
 	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) {
-		dp_conf->lane_count =
+		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) {
-		dp_conf->vswing_level =
+		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) {
-		dp_conf->preemp_level =
+		intel_dp->compliance_config.preemp_level =
 			parms[DP_CONFIG_PARAM_PREEMPHASIS].value;
 	} else
 		return -EINVAL;
@@ -3758,21 +3750,21 @@ static int displayport_parse_config(char *input_buffer,
 	    parms[DP_CONFIG_PARAM_BPP].value == 24 ||
 	    parms[DP_CONFIG_PARAM_BPP].value == 30 ||
 	    parms[DP_CONFIG_PARAM_BPP].value == 36) {
-		dp_conf->bits_per_pixel =
+		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) {
-		dp_conf->hres =
+		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) {
-		dp_conf->vres =
+		intel_dp->compliance_config.vres =
 			parms[DP_CONFIG_PARAM_VRES].value;
 	} else
 		return -EINVAL;
@@ -3830,7 +3822,6 @@ static ssize_t displayport_config_ctl_write(struct file *file,
 	struct intel_encoder *intel_encoder;
 	struct intel_connector *intel_connector;
 	struct intel_dp *intel_dp;
-	struct intel_dp_link_config dp_conf;
 
 	m = file->private_data;
 	if (!m) {
@@ -3857,9 +3848,7 @@ static ssize_t displayport_config_ctl_write(struct file *file,
 	}
 
 	input_buffer[len] = '\0';
-	memset(&dp_conf, 0, sizeof(struct intel_dp_link_config));
 	DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
-	status = displayport_parse_config(input_buffer, len, &dp_conf);
 
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
 		intel_connector = to_intel_connector(connector);
@@ -3868,9 +3857,11 @@ static ssize_t displayport_config_ctl_write(struct file *file,
 			if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
 			    intel_encoder->type == INTEL_OUTPUT_UNKNOWN) {
 				intel_dp = enc_to_intel_dp(&intel_encoder->base);
-				memcpy(&intel_dp->compliance_config,
-				       &dp_conf,
-				       sizeof(struct intel_dp_link_config));
+				status = displayport_parse_config(input_buffer,
+								  len,
+								  intel_dp);
+				if (status < 0)
+					goto out;
 			}
 		}
 	}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 29d51f9..bd32d98 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -554,6 +554,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;
@@ -613,11 +625,13 @@ struct intel_dp {
 		enum edp_drrs_refresh_rate_type refresh_rate_type;
 		struct mutex mutex;
 	} drrs_state;
+
 	/* Displayport compliance testing */
 	unsigned long compliance_app_pid;
 	unsigned long compliance_test_data;
 	bool compliance_testing_active;
-
+	struct intel_dp_link_config compliance_config;
+	struct intel_dp_link_config saved_config;
 };
 
 struct intel_digital_port {
-- 
1.9.1

[PATCH 08/10] 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.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 717cb5d..b7bdb5f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -41,6 +41,13 @@
 #define DP_LINK_CHECK_TIMEOUT	(10 * 1000)
 #define SIG_DP_COMPLIANCE	47
 
+/* 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;
@@ -4012,11 +4019,81 @@ intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 	return test_result;
 }
 
-/* Displayport compliance testing - EDID operations */
-static uint8_t
-intel_dp_autotest_edid(struct intel_dp *intel_dp)
+static bool intel_dp_compute_edid_checksum(uint8_t *edid_data,
+					   uint8_t *edid_checksum)
 {
-	uint8_t test_result = DP_TEST_NAK;
+	uint32_t byte_total = 0;
+	uint8_t i = 0;
+	bool edid_ok = true;
+
+	/* Compute byte total w/o the checksum value*/
+	for (i = 0; i < EDID_LENGTH - 2; i++)
+		byte_total += edid_data[i];
+
+	DRM_DEBUG_KMS("EDID total = %d, EDID checksum =  %d\n",
+		      byte_total, edid_data[EDID_LENGTH - 34 - 1]);
+
+	/* Compute the checksum */
+	*edid_checksum = 256 - (byte_total % 256);
+
+	if (*edid_checksum != edid_data[EDID_LENGTH - 1]) {
+		DRM_DEBUG_KMS("Invalid EDID checksum %d, should be %d\n",
+			      edid_data[EDID_LENGTH - 40 - 1], *edid_checksum);
+		edid_ok = false;
+	}
+
+	return edid_ok;
+}
+
+
+/* Displayport compliance testing - EDID operations */
+static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
+{
+	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;
+
+	DRM_DEBUG_KMS("Displayport: EDID automated test\n");
+
+	/* Reset the NACK/DEFER counters */
+	intel_dp->aux.i2c_nack_count = 0;
+	intel_dp->aux.i2c_defer_count = 0;
+	/* Now read out the EDID */
+	edid_read = drm_get_edid(connector, adapter);
+
+	if (edid_read == NULL) {
+		/* Check for NACKs/DEFERs, goto 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)) {
+			/* Write the checksum to EDID checksum register */
+			ret = drm_dp_dpcd_write(&intel_dp->aux,
+						DP_TEST_EDID_CHECKSUM,
+						&edid_read->checksum, 1);
+			/* Reponse is ACK and and checksum written */
+			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 test */
+			intel_dp->compliance_test_data = INTEL_DP_EDID_CORRUPT |
+							 INTEL_DP_RESOLUTION_FAILSAFE;
+		}
+	}
+
 	return test_result;
 }
 
-- 
1.9.1

[PATCH 09/10] drm/i915: Update intel_dp_compute_config() to respond to Displayport compliance test requests appropriately

[Todd Previte]

Adds provisions in intel_dp_compute_config() to accommodate compliance testing. Mostly
this invovles circumventing the automatic link configuration paramters 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 b7bdb5f..9da948c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1117,6 +1117,21 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	pipe_config->has_drrs = false;
 	pipe_config->has_audio = intel_dp->has_audio;
 
+	/* Compliance testing can skip most of this function */
+	if (!is_edp(intel_dp) && intel_dp->compliance_testing_active) {
+		/* FIXME: Validate these parameters*/
+		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);
@@ -1203,6 +1218,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

[PATCH 10/10] drm/i915: Fix intel_dp_hot_plug()

[Todd Previte]

The hot plug function for DP appears to have been broken somewhere along the way. Without
this function being operational, hot plug events are not correctly received for compliance
testing. This patch implements the necessary functionality to resolve that issue.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9da948c..8005955 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4807,6 +4807,13 @@ static const struct drm_encoder_funcs intel_dp_enc_funcs = {
 void
 intel_dp_hot_plug(struct intel_encoder *intel_encoder)
 {
+	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
+	struct drm_device *dev = intel_encoder->base.dev;
+
+	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+	intel_dp_check_link_status(intel_dp);
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
 	return;
 }
 
-- 
1.9.1

Re: [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug()

[Chris Wilson]

On Thu, Oct 09, 2014 at 08:38:10AM -0700, Todd Previte wrote:
> The hot plug function for DP appears to have been broken somewhere along the way. Without
> this function being operational, hot plug events are not correctly received for compliance
> testing. This patch implements the necessary functionality to resolve that issue.

Perhaps a discussion of why it was removed and how the apparent conflict
should be resolved?

In particular citing the commit that gutted the function and cc'ing the
author (Dave Airlie) and reviwers (you) are in order.

commit 0e32b39ceed665bfa4a77a4bc307b6652b991632
Author: Dave Airlie <airlied@redhat.com>
Date:   Fri May 2 14:02:48 2014 +1000

    drm/i915: add DP 1.2 MST support (v0.7)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug()

[Dave Airlie]

On 10 October 2014 01:49, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Oct 09, 2014 at 08:38:10AM -0700, Todd Previte wrote:
>> The hot plug function for DP appears to have been broken somewhere along the way. Without
>> this function being operational, hot plug events are not correctly received for compliance
>> testing. This patch implements the necessary functionality to resolve that issue.
>
> Perhaps a discussion of why it was removed and how the apparent conflict
> should be resolved?
>
> In particular citing the commit that gutted the function and cc'ing the
> author (Dave Airlie) and reviwers (you) are in order.
>
> commit 0e32b39ceed665bfa4a77a4bc307b6652b991632
> Author: Dave Airlie <airlied@redhat.com>
> Date:   Fri May 2 14:02:48 2014 +1000
>
>     drm/i915: add DP 1.2 MST support (v0.7)

I'm sure this shouldn't be needed as we get short/long hotplug DP
events in the pulse handler,
not here.

Dave.

Re: [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug()

[Todd Previte]

On 10/9/14 8:38 PM, Dave Airlie wrote:
> On 10 October 2014 01:49, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> On Thu, Oct 09, 2014 at 08:38:10AM -0700, Todd Previte wrote:
>>> The hot plug function for DP appears to have been broken somewhere along the way. Without
>>> this function being operational, hot plug events are not correctly received for compliance
>>> testing. This patch implements the necessary functionality to resolve that issue.
>> Perhaps a discussion of why it was removed and how the apparent conflict
>> should be resolved?
>>
>> In particular citing the commit that gutted the function and cc'ing the
>> author (Dave Airlie) and reviwers (you) are in order.
>>
>> commit 0e32b39ceed665bfa4a77a4bc307b6652b991632
>> Author: Dave Airlie <airlied@redhat.com>
>> Date:   Fri May 2 14:02:48 2014 +1000
>>
>>      drm/i915: add DP 1.2 MST support (v0.7)
> I'm sure this shouldn't be needed as we get short/long hotplug DP
> events in the pulse handler,
> not here.
>
> Dave.
Sounds good. Looking at the HPD pulse handler, I think I can get the 
necessary code in there for the non-MST case to get compliance testing 
working with just the pulse handler. The short pulse will catch it 
already, but I need to add code to the long pulse segment in order to 
get the correct notification there.

-T

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

[Paulo Zanoni]

Hi

Since I already commented about the coding style on the previous
reviews, I'll ignore that aspect for the comments below.

2014-10-09 12:38 GMT-03: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.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 82 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 80 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 64c8e04..f7d4119 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3959,11 +3959,89 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>         return true;
>  }
>
> +/* Displayport compliance testing - Link training */
> +static uint8_t
> +intel_dp_autotest_link_training(struct intel_dp *intel_dp)
> +{
> +       uint8_t test_result = DP_TEST_NAK;
> +       return test_result;
> +}
> +
> +/* Displayport compliance testing - Video pattern testing */
> +static uint8_t
> +intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
> +{
> +       uint8_t test_result = DP_TEST_NAK;
> +       return test_result;
> +}
> +
> +/* Displayport compliance testing - EDID operations */
> +static uint8_t
> +intel_dp_autotest_edid(struct intel_dp *intel_dp)
> +{
> +       uint8_t test_result = DP_TEST_NAK;
> +       return test_result;
> +}
> +
> +/* Displayport compliance testing - PHY pattern testing */
> +static uint8_t
> +intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
> +{
> +       uint8_t test_result = DP_TEST_NAK;
> +       return test_result;
> +}

I guess a lot of people would have just made the code return NAK
without even defining/calling these functions above.

> +
>  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;
> +
> +       DRM_DEBUG_KMS("Received automated test request\n");
> +       status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
> +
> +       /* ACK/NAK response based on test function response
> +          Unimplemented/unsupported tests will NAK by default */
> +       switch (rxdata) {

You're reading rxdata without checking for "status" first.


> +       case DP_TEST_LINK_TRAINING:
> +               DRM_DEBUG_KMS("Executing LINK_TRAINING request\n");

As I said on the previous review of the same patch: we're lying here.
We won't execute anything yet.


> +               intel_dp->compliance_test_data = DP_TEST_LINK_TRAINING;
> +               response = intel_dp_autotest_link_training(intel_dp);
> +               break;
> +       case DP_TEST_LINK_VIDEO_PATTERN:
> +               DRM_DEBUG_KMS("Executing TEST_PATTERN request\n");
> +               intel_dp->compliance_test_data = DP_TEST_LINK_VIDEO_PATTERN;
> +               response = intel_dp_autotest_video_pattern(intel_dp);
> +               break;
> +       case DP_TEST_LINK_EDID_READ:
> +               DRM_DEBUG_KMS("Executing EDID request\n");
> +               intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ;
> +               response = intel_dp_autotest_edid(intel_dp);
> +               break;
> +       case DP_TEST_LINK_PHY_TEST_PATTERN:
> +               DRM_DEBUG_KMS("Executing PHY_PATTERN request\n");
> +               intel_dp->compliance_test_data = DP_TEST_LINK_PHY_TEST_PATTERN;
> +               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;
> +       /* Unsupported test case or something went wrong */

Is there a way to differentiate the "unsupported" and the "went wrong"
cases on dmesg?


> +       default:
> +               DRM_DEBUG_KMS("Unhandled test request\n");
> +               break;
> +       }
> +       if (status != 0) {
> +               response = DP_TEST_NAK;
> +               DRM_DEBUG_KMS("Error %d processing test request\n", status);
> +       }
> +       status = drm_dp_dpcd_write(&intel_dp->aux,
> +                                  DP_TEST_RESPONSE,
> +                                  &response, 1);
> +       intel_dp->compliance_testing_active = 0;
> +
>  }

And the most important thing: the patch doesn't compile. Please make
sure each patch in the series compiles and works. We do tons of git
bisections on our tree, that's really important.

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



-- 
Paulo Zanoni

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

[Daniel Vetter]

On Thu, Oct 09, 2014 at 08:38:01AM -0700, Todd Previte wrote:
> 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.
> 
> Signed-off-by: Todd Previte <tprevite@gmail.com>

Patch is missing the patch changelog here which details your changes
compared to the earlier version. This is especially important since Paulo
has already started reviewing your patches, so it's good practice to
mention all the things you've changed. And much more important, which
parts you didn't change - that _must_ happen, eithere in the commit
message or with a reply to Paulo's review. Also, please cc any people who
previously commented on a patch so that they have a chance to catch the
next round.

Without all that review iterations are a massive pain for reviewers since
they might miss you resends and have to re-read all the patches again
completely to check that you didn't miss anything.

Thanks, Daniel
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 82 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 80 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 64c8e04..f7d4119 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3959,11 +3959,89 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  	return true;
>  }
>  
> +/* Displayport compliance testing - Link training */
> +static uint8_t
> +intel_dp_autotest_link_training(struct intel_dp *intel_dp)
> +{
> +	uint8_t test_result = DP_TEST_NAK;
> +	return test_result;
> +}
> +
> +/* Displayport compliance testing - Video pattern testing */
> +static uint8_t
> +intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
> +{
> +	uint8_t test_result = DP_TEST_NAK;
> +	return test_result;
> +}
> +
> +/* Displayport compliance testing - EDID operations */
> +static uint8_t
> +intel_dp_autotest_edid(struct intel_dp *intel_dp)
> +{
> +	uint8_t test_result = DP_TEST_NAK;
> +	return test_result;
> +}
> +
> +/* Displayport compliance testing - PHY pattern testing */
> +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;
> +
> +	DRM_DEBUG_KMS("Received automated test request\n");
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
> +
> +	/* ACK/NAK response based on test function response
> +	   Unimplemented/unsupported tests will NAK by default */
> +	switch (rxdata) {
> +	case DP_TEST_LINK_TRAINING:
> +		DRM_DEBUG_KMS("Executing LINK_TRAINING request\n");
> +		intel_dp->compliance_test_data = DP_TEST_LINK_TRAINING;
> +		response = intel_dp_autotest_link_training(intel_dp);
> +		break;
> +	case DP_TEST_LINK_VIDEO_PATTERN:
> +		DRM_DEBUG_KMS("Executing TEST_PATTERN request\n");
> +		intel_dp->compliance_test_data = DP_TEST_LINK_VIDEO_PATTERN;
> +		response = intel_dp_autotest_video_pattern(intel_dp);
> +		break;
> +	case DP_TEST_LINK_EDID_READ:
> +		DRM_DEBUG_KMS("Executing EDID request\n");
> +		intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ;
> +		response = intel_dp_autotest_edid(intel_dp);
> +		break;
> +	case DP_TEST_LINK_PHY_TEST_PATTERN:
> +		DRM_DEBUG_KMS("Executing PHY_PATTERN request\n");
> +		intel_dp->compliance_test_data = DP_TEST_LINK_PHY_TEST_PATTERN;
> +		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;
> +	/* Unsupported test case or something went wrong */
> +	default:
> +		DRM_DEBUG_KMS("Unhandled test request\n");
> +		break;
> +	}
> +	if (status != 0) {
> +		response = DP_TEST_NAK;
> +		DRM_DEBUG_KMS("Error %d processing test request\n", status);
> +	}
> +	status = drm_dp_dpcd_write(&intel_dp->aux,
> +				   DP_TEST_RESPONSE,
> +				   &response, 1);
> +	intel_dp->compliance_testing_active = 0;
> +
>  }
>  
>  static int
> -- 
> 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

Re: [Intel-gfx] [PATCH 02/10] drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs

[Paulo Zanoni]

2014-10-09 12:38 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> These counters are used for Displayort complinace testing to detect error conditions
> when executing certain compliance tests. Currently these are used in the EDID tests
> to determine if the video mode needs to be set to the preferred mode or the failsafe
> mode.
>
> Cc: dri-devel@lists.freedesktop.org

I see that this patch and a few others in your series still have
unaddressed/unanswered review comments, given on the first time you
sent the patches. Please take a look at them.

> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 2 ++
>  include/drm/drm_dp_helper.h     | 1 +
>  2 files changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 08e33b8..8353051 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>
>                 case DP_AUX_I2C_REPLY_NACK:
>                         DRM_DEBUG_KMS("I2C nack\n");
> +                       aux->i2c_nack_count++;
>                         return -EREMOTEIO;
>
>                 case DP_AUX_I2C_REPLY_DEFER:
>                         DRM_DEBUG_KMS("I2C defer\n");
> +                       aux->i2c_defer_count++;
>                         usleep_range(400, 500);
>                         continue;
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 8edeed0..45f3ee8 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -551,6 +551,7 @@ struct drm_dp_aux {
>         struct mutex hw_mutex;
>         ssize_t (*transfer)(struct drm_dp_aux *aux,
>                             struct drm_dp_aux_msg *msg);
> +       uint8_t i2c_nack_count, i2c_defer_count;
>  };
>
>  ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing

[Daniel Vetter]

Hi Tood,

Paulo already mentioned it, so I'll just rehash quickly: I think the
points from the internal pre-review first need to be address before we can
dig into details. I know that the discussion there pettered out a bit, but
imo it's the patch authors responsibility to pick that up again and ping
people. One example I've noticed is the use of signals: We really want a
pollable file in debugfs (see e.g. the crc stuff). You can always still
redirect that to give you a signal, although I highly recommend to avoid
signals like the plague - they're simply too much a pain.

But now onto the real comment I wanted to make, see below.

On Thu, Oct 09, 2014 at 08:38:05AM -0700, Todd Previte wrote:
> Adds an interface that allows for Displayport configuration information to be accessed
> through debugfs. The information paramters provided here are as follows:
>    Link rate
>    Lane count
>    Bits per pixel
>    Voltage swing level
>    Preemphasis level
>    Display mode
> 
> These parameters are intended to be used by userspace applications that need access
> to the link configuration of any active Displayport connection. Additionally,
> applications can make adjustments to these parameters through this interface to
> allow userspace application to have fine-grained control over link training
> paramters.
> 
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 389 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c     |  13 +-
>  2 files changed, 397 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index da4036d..2dada18 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
> +};

I think it's better to expose this as drm properties on the DP connector.
This has a pile of upsides:
- We don't need to invent a new parser.
- Users can play with them to test out different theories.
- We could even use this to fix broken panels/boards without vbt or
  anything using our plan to set up the initial config with a dt firmware
  blob.

So would be a lot more useful than this private interface.

For the properties themselves I think we should go with explicit
enumerations with default value "automatic" and then an enumeration of all
values allowed by DP. For the naming of the properties I guess something
like DP_link_lanes, DP_link_clock, ... would look pretty. The properties
should be in dev->mode_config (since they're reallly generic) and I think
we want one function to register them all in one go in the drm_dp_helper.c
library. Maybe we could even put the values into the existing dp source
struct so that we don't have to reinvent the decode logic every single
time.

Now on the properties themselves I think they all make perfect sense
except:
- bpp: I've thought if we define lanes+clock plus the bpp from the
  framebuffer (we support 16bpp, 24bpp, 30bpp and 48bpp) we should be able
  to hit every possible combination. So I wonder in which precise
  corner-case the userspace program can't set up the link like it wants to
  without the bpp property.
- hres/vres: Seem unused and look like they're back from an earlier
  design. We should be able to set that through the mode we're setting
  with setCrtc.

With that plus Paulo&Jesse's internal comments addressed I think this is
moving into the right direction. Aside (probably just missed it): Where's
the userspace side of this?

Thanks, Daniel

> +
> +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
> @@ -3505,6 +3546,353 @@ 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;
> +	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) {
> +				pipe_bpp = intel_encoder->new_crtc->config.pipe_bpp;
> +				mode = &intel_encoder->new_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,
> +				   intel_connector->base.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;
> +	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;
> +}
> +
> +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_link_config *dp_conf)
> +{
> +	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;
> +			}
> +		}
> +	}
> +
> +	for (i = 1; i < DP_PARAMETER_COUNT; i++)
> +		DRM_DEBUG_DRIVER("%s = %lu\n",
> +					dp_conf_tokens[parms[i].type],
> +					parms[i].value);
> +
> +	/* Validate any/all incoming parameters */
> +	strcpy(dp_conf->connector_name,
> +	       (char *)parms[DP_CONFIG_PARAM_CONNECTOR].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) {
> +		dp_conf->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) {
> +		dp_conf->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) {
> +		dp_conf->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) {
> +		dp_conf->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) {
> +		dp_conf->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) {
> +		dp_conf->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) {
> +		dp_conf->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;
> +	struct drm_connector *connector;
> +	struct intel_encoder *intel_encoder;
> +	struct intel_connector *intel_connector;
> +
> +	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;
> +		if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
> +			if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
> +			    intel_encoder->type == INTEL_OUTPUT_UNKNOWN) {
> +				if (connector->status == connector_status_connected) {
> +					displayport_show_config_info(m, intel_connector);
> +				} else {
> +					seq_printf(m, DP_CONF_STR_CONNECTOR,
> +						   intel_connector->base.name);
> +					seq_printf(m, "disconnected\n");
> +				}
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int displayport_config_ctl_open(struct inode *inode,
> +				       struct file *file)
> +{
> +	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)
> +{
> +	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;
> +	struct intel_dp_link_config dp_conf;
> +
> +	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';
> +	memset(&dp_conf, 0, sizeof(struct intel_dp_link_config));
> +	DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
> +	status = displayport_parse_config(input_buffer, len, &dp_conf);
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> +		intel_connector = to_intel_connector(connector);
> +		intel_encoder = intel_connector->encoder;
> +		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(&intel_encoder->base);
> +				memcpy(&intel_dp->compliance_config,
> +				       &dp_conf,
> +				       sizeof(struct intel_dp_link_config));
> +			}
> +		}
> +	}
> +
> +
> +out:
> +	kfree(input_buffer);
> +	if (status < 0)
> +		return status;
> +
> +	*offp += len;
> +	return len;
> +}
> +
> +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[5])
>  {
>  	struct drm_device *dev = m->private;
> @@ -4208,6 +4596,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)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a7acc61..b3ddd15 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3968,7 +3968,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  static uint8_t
>  intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>  {
> -	uint8_t test_result = DP_TEST_NAK;
> +	uint8_t test_result = DP_TEST_ACK;
>  	return test_result;
>  }
>  
> @@ -4013,21 +4013,25 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  		DRM_DEBUG_KMS("Executing LINK_TRAINING request\n");
>  		intel_dp->compliance_test_data = DP_TEST_LINK_TRAINING;
>  		response = intel_dp_autotest_link_training(intel_dp);
> +		status = intel_dp_signal_userspace(intel_dp);
>  		break;
>  	case DP_TEST_LINK_VIDEO_PATTERN:
>  		DRM_DEBUG_KMS("Executing TEST_PATTERN request\n");
>  		intel_dp->compliance_test_data = DP_TEST_LINK_VIDEO_PATTERN;
>  		response = intel_dp_autotest_video_pattern(intel_dp);
> +		status = intel_dp_signal_userspace(intel_dp);
>  		break;
>  	case DP_TEST_LINK_EDID_READ:
>  		DRM_DEBUG_KMS("Executing EDID request\n");
>  		intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ;
>  		response = intel_dp_autotest_edid(intel_dp);
> +		status = intel_dp_signal_userspace(intel_dp);
>  		break;
>  	case DP_TEST_LINK_PHY_TEST_PATTERN:
>  		DRM_DEBUG_KMS("Executing PHY_PATTERN request\n");
>  		intel_dp->compliance_test_data = DP_TEST_LINK_PHY_TEST_PATTERN;
>  		response = intel_dp_autotest_phy_pattern(intel_dp);
> +		status = intel_dp_signal_userspace(intel_dp);
>  		break;
>  		/* FAUX is optional in DP 1.2*/
>  	case DP_TEST_LINK_FAUX_PATTERN:
> @@ -4038,10 +4042,9 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  		DRM_DEBUG_KMS("Unhandled test request\n");
>  		break;
>  	}
> -	if (status != 0) {
> -		response = DP_TEST_NAK;
> -		DRM_DEBUG_KMS("Error %d processing test request\n", status);
> -	}
> +	if (status != 0)
> +		DRM_DEBUG_KMS("Status code %d processing test request\n",
> +			      status);
>  	status = drm_dp_dpcd_write(&intel_dp->aux,
>  				   DP_TEST_RESPONSE,
>  				   &response, 1);
> -- 
> 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

Re: [PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing

[Paulo Zanoni]

2014-10-23 10:50 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> Hi Tood,
>
> Paulo already mentioned it, so I'll just rehash quickly: I think the
> points from the internal pre-review first need to be address before we can
> dig into details. I know that the discussion there pettered out a bit, but
> imo it's the patch authors responsibility to pick that up again and ping
> people. One example I've noticed is the use of signals: We really want a
> pollable file in debugfs (see e.g. the crc stuff). You can always still
> redirect that to give you a signal, although I highly recommend to avoid
> signals like the plague - they're simply too much a pain.
>
> But now onto the real comment I wanted to make, see below.
>
> On Thu, Oct 09, 2014 at 08:38:05AM -0700, Todd Previte wrote:
>> Adds an interface that allows for Displayport configuration information to be accessed
>> through debugfs. The information paramters provided here are as follows:
>>    Link rate
>>    Lane count
>>    Bits per pixel
>>    Voltage swing level
>>    Preemphasis level
>>    Display mode
>>
>> These parameters are intended to be used by userspace applications that need access
>> to the link configuration of any active Displayport connection. Additionally,
>> applications can make adjustments to these parameters through this interface to
>> allow userspace application to have fine-grained control over link training
>> paramters.
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c | 389 ++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_dp.c     |  13 +-
>>  2 files changed, 397 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index da4036d..2dada18 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
>> +};
>
> I think it's better to expose this as drm properties on the DP connector.
> This has a pile of upsides:
> - We don't need to invent a new parser.
> - Users can play with them to test out different theories.
> - We could even use this to fix broken panels/boards without vbt or
>   anything using our plan to set up the initial config with a dt firmware
>   blob.
>
> So would be a lot more useful than this private interface.
>
> For the properties themselves I think we should go with explicit
> enumerations with default value "automatic" and then an enumeration of all
> values allowed by DP. For the naming of the properties I guess something
> like DP_link_lanes, DP_link_clock, ... would look pretty. The properties
> should be in dev->mode_config (since they're reallly generic) and I think
> we want one function to register them all in one go in the drm_dp_helper.c
> library. Maybe we could even put the values into the existing dp source
> struct so that we don't have to reinvent the decode logic every single
> time.

I disagree. I do prefer the debugfs thing. Think about all the
examples of people that break their systems by passing
i915.enable_foo=1 or i915.enable_rc6=7 and then open bug reports...
With debug stuff exposed as DP properties, we're going to increase
that problem, and in a way that will make it even harder to detect. I
really really think these things should be exposed only to the debugfs
users, because then if the debugfs file is closed, we can just ignore
all the arguments and keep doing "normal" unaffected modesets.

If we don't want the parser, maybe we can make it a binary protocol:
we'lll still have to parse it, but it can get much easier.

>
> Now on the properties themselves I think they all make perfect sense
> except:
> - bpp: I've thought if we define lanes+clock plus the bpp from the
>   framebuffer (we support 16bpp, 24bpp, 30bpp and 48bpp) we should be able
>   to hit every possible combination. So I wonder in which precise
>   corner-case the userspace program can't set up the link like it wants to
>   without the bpp property.
> - hres/vres: Seem unused and look like they're back from an earlier
>   design. We should be able to set that through the mode we're setting
>   with setCrtc.
>
> With that plus Paulo&Jesse's internal comments addressed I think this is
> moving into the right direction. Aside (probably just missed it): Where's
> the userspace side of this?
>
> Thanks, Daniel
>
>> +
>> +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
>> @@ -3505,6 +3546,353 @@ 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;
>> +     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) {
>> +                             pipe_bpp = intel_encoder->new_crtc->config.pipe_bpp;
>> +                             mode = &intel_encoder->new_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,
>> +                                intel_connector->base.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;
>> +     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;
>> +}
>> +
>> +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_link_config *dp_conf)
>> +{
>> +     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;
>> +                     }
>> +             }
>> +     }
>> +
>> +     for (i = 1; i < DP_PARAMETER_COUNT; i++)
>> +             DRM_DEBUG_DRIVER("%s = %lu\n",
>> +                                     dp_conf_tokens[parms[i].type],
>> +                                     parms[i].value);
>> +
>> +     /* Validate any/all incoming parameters */
>> +     strcpy(dp_conf->connector_name,
>> +            (char *)parms[DP_CONFIG_PARAM_CONNECTOR].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) {
>> +             dp_conf->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) {
>> +             dp_conf->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) {
>> +             dp_conf->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) {
>> +             dp_conf->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) {
>> +             dp_conf->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) {
>> +             dp_conf->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) {
>> +             dp_conf->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;
>> +     struct drm_connector *connector;
>> +     struct intel_encoder *intel_encoder;
>> +     struct intel_connector *intel_connector;
>> +
>> +     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;
>> +             if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
>> +                     if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
>> +                         intel_encoder->type == INTEL_OUTPUT_UNKNOWN) {
>> +                             if (connector->status == connector_status_connected) {
>> +                                     displayport_show_config_info(m, intel_connector);
>> +                             } else {
>> +                                     seq_printf(m, DP_CONF_STR_CONNECTOR,
>> +                                                intel_connector->base.name);
>> +                                     seq_printf(m, "disconnected\n");
>> +                             }
>> +                     }
>> +             }
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int displayport_config_ctl_open(struct inode *inode,
>> +                                    struct file *file)
>> +{
>> +     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)
>> +{
>> +     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;
>> +     struct intel_dp_link_config dp_conf;
>> +
>> +     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';
>> +     memset(&dp_conf, 0, sizeof(struct intel_dp_link_config));
>> +     DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
>> +     status = displayport_parse_config(input_buffer, len, &dp_conf);
>> +
>> +     list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
>> +             intel_connector = to_intel_connector(connector);
>> +             intel_encoder = intel_connector->encoder;
>> +             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(&intel_encoder->base);
>> +                             memcpy(&intel_dp->compliance_config,
>> +                                    &dp_conf,
>> +                                    sizeof(struct intel_dp_link_config));
>> +                     }
>> +             }
>> +     }
>> +
>> +
>> +out:
>> +     kfree(input_buffer);
>> +     if (status < 0)
>> +             return status;
>> +
>> +     *offp += len;
>> +     return len;
>> +}
>> +
>> +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[5])
>>  {
>>       struct drm_device *dev = m->private;
>> @@ -4208,6 +4596,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)
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index a7acc61..b3ddd15 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3968,7 +3968,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>>  static uint8_t
>>  intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>>  {
>> -     uint8_t test_result = DP_TEST_NAK;
>> +     uint8_t test_result = DP_TEST_ACK;
>>       return test_result;
>>  }
>>
>> @@ -4013,21 +4013,25 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)
>>               DRM_DEBUG_KMS("Executing LINK_TRAINING request\n");
>>               intel_dp->compliance_test_data = DP_TEST_LINK_TRAINING;
>>               response = intel_dp_autotest_link_training(intel_dp);
>> +             status = intel_dp_signal_userspace(intel_dp);
>>               break;
>>       case DP_TEST_LINK_VIDEO_PATTERN:
>>               DRM_DEBUG_KMS("Executing TEST_PATTERN request\n");
>>               intel_dp->compliance_test_data = DP_TEST_LINK_VIDEO_PATTERN;
>>               response = intel_dp_autotest_video_pattern(intel_dp);
>> +             status = intel_dp_signal_userspace(intel_dp);
>>               break;
>>       case DP_TEST_LINK_EDID_READ:
>>               DRM_DEBUG_KMS("Executing EDID request\n");
>>               intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ;
>>               response = intel_dp_autotest_edid(intel_dp);
>> +             status = intel_dp_signal_userspace(intel_dp);
>>               break;
>>       case DP_TEST_LINK_PHY_TEST_PATTERN:
>>               DRM_DEBUG_KMS("Executing PHY_PATTERN request\n");
>>               intel_dp->compliance_test_data = DP_TEST_LINK_PHY_TEST_PATTERN;
>>               response = intel_dp_autotest_phy_pattern(intel_dp);
>> +             status = intel_dp_signal_userspace(intel_dp);
>>               break;
>>               /* FAUX is optional in DP 1.2*/
>>       case DP_TEST_LINK_FAUX_PATTERN:
>> @@ -4038,10 +4042,9 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)
>>               DRM_DEBUG_KMS("Unhandled test request\n");
>>               break;
>>       }
>> -     if (status != 0) {
>> -             response = DP_TEST_NAK;
>> -             DRM_DEBUG_KMS("Error %d processing test request\n", status);
>> -     }
>> +     if (status != 0)
>> +             DRM_DEBUG_KMS("Status code %d processing test request\n",
>> +                           status);
>>       status = drm_dp_dpcd_write(&intel_dp->aux,
>>                                  DP_TEST_RESPONSE,
>>                                  &response, 1);
>> --
>> 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



-- 
Paulo Zanoni

Re: [PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing

[Daniel Vetter]

On Thu, Oct 23, 2014 at 10:58:59AM -0200, Paulo Zanoni wrote:
> 2014-10-23 10:50 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> > Hi Tood,
> >
> > Paulo already mentioned it, so I'll just rehash quickly: I think the
> > points from the internal pre-review first need to be address before we can
> > dig into details. I know that the discussion there pettered out a bit, but
> > imo it's the patch authors responsibility to pick that up again and ping
> > people. One example I've noticed is the use of signals: We really want a
> > pollable file in debugfs (see e.g. the crc stuff). You can always still
> > redirect that to give you a signal, although I highly recommend to avoid
> > signals like the plague - they're simply too much a pain.
> >
> > But now onto the real comment I wanted to make, see below.
> >
> > On Thu, Oct 09, 2014 at 08:38:05AM -0700, Todd Previte wrote:
> >> Adds an interface that allows for Displayport configuration information to be accessed
> >> through debugfs. The information paramters provided here are as follows:
> >>    Link rate
> >>    Lane count
> >>    Bits per pixel
> >>    Voltage swing level
> >>    Preemphasis level
> >>    Display mode
> >>
> >> These parameters are intended to be used by userspace applications that need access
> >> to the link configuration of any active Displayport connection. Additionally,
> >> applications can make adjustments to these parameters through this interface to
> >> allow userspace application to have fine-grained control over link training
> >> paramters.
> >>
> >> Signed-off-by: Todd Previte <tprevite@gmail.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_debugfs.c | 389 ++++++++++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/i915/intel_dp.c     |  13 +-
> >>  2 files changed, 397 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >> index da4036d..2dada18 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
> >> +};
> >
> > I think it's better to expose this as drm properties on the DP connector.
> > This has a pile of upsides:
> > - We don't need to invent a new parser.
> > - Users can play with them to test out different theories.
> > - We could even use this to fix broken panels/boards without vbt or
> >   anything using our plan to set up the initial config with a dt firmware
> >   blob.
> >
> > So would be a lot more useful than this private interface.
> >
> > For the properties themselves I think we should go with explicit
> > enumerations with default value "automatic" and then an enumeration of all
> > values allowed by DP. For the naming of the properties I guess something
> > like DP_link_lanes, DP_link_clock, ... would look pretty. The properties
> > should be in dev->mode_config (since they're reallly generic) and I think
> > we want one function to register them all in one go in the drm_dp_helper.c
> > library. Maybe we could even put the values into the existing dp source
> > struct so that we don't have to reinvent the decode logic every single
> > time.
> 
> I disagree. I do prefer the debugfs thing. Think about all the
> examples of people that break their systems by passing
> i915.enable_foo=1 or i915.enable_rc6=7 and then open bug reports...
> With debug stuff exposed as DP properties, we're going to increase
> that problem, and in a way that will make it even harder to detect. I
> really really think these things should be exposed only to the debugfs
> users, because then if the debugfs file is closed, we can just ignore
> all the arguments and keep doing "normal" unaffected modesets.
> 
> If we don't want the parser, maybe we can make it a binary protocol:
> we'lll still have to parse it, but it can get much easier.

We already expose piles of funky stuff to users in these properties (e.g.
audio on hdmi). And contrary to the module options most of these will just
result in black screens if you don't understand what you're doing, so I
think the risk is low. There's also a bit of an issue with the current
interface as it's not clear which line corresponds to which DP interface.
Using properties would solve this. Also these options have a much more
direct impact - if you set them and the screen goes dark then the user
hopefully realizes that this is something they shouldn't touch.

For fbc and rc6 and all those the problem is that nothing really happens,
the system just becomes a bit more unstable and randomly crashes. Which
users then report.

But If you're really concerned about users doing crappy stuff we could add
a module option to drm_dp_helper.c which hides these, and then taint the
kernel if any user sets them. But I really think this is overkill.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Todd Previte]

Version 2 in process now. Responses in line below.


On 10/20/2014 10:48 AM, Paulo Zanoni wrote:
> Hi
>
> Since I already commented about the coding style on the previous
> reviews, I'll ignore that aspect for the comments below.
I think all the style issues have been resolved. Checkpatch.pl has 0 
errors, 0 warnings, so that should be good to go. Excess comments have 
also been removed. If I missed anything though, please let me know.

>
> 2014-10-09 12:38 GMT-03: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.
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 82 ++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 80 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 64c8e04..f7d4119 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3959,11 +3959,89 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>>          return true;
>>   }
>>
>> +/* Displayport compliance testing - Link training */
>> +static uint8_t
>> +intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +/* Displayport compliance testing - Video pattern testing */
>> +static uint8_t
>> +intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +/* Displayport compliance testing - EDID operations */
>> +static uint8_t
>> +intel_dp_autotest_edid(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +/* Displayport compliance testing - PHY pattern testing */
>> +static uint8_t
>> +intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
> I guess a lot of people would have just made the code return NAK
> without even defining/calling these functions above.

It came down to two ways of implementing this initial patch. I could 
have NAK'd in the handler and omitted these functions until they were 
fully implemented. Or I could do what I did here, which was to put the 
skeleton in place and add the flesh to bones when ready. To me, this 
seems to make more sense to me because it gets the structure in place 
and builds on it in subsequent patches.

>
>> +
>>   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;
>> +
>> +       DRM_DEBUG_KMS("Received automated test request\n");
>> +       status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
>> +
>> +       /* ACK/NAK response based on test function response
>> +          Unimplemented/unsupported tests will NAK by default */
>> +       switch (rxdata) {
> You're reading rxdata without checking for "status" first.
Good catch. Fixed. The status check at the end should have been up at 
the top as well.
>
>
>> +       case DP_TEST_LINK_TRAINING:
>> +               DRM_DEBUG_KMS("Executing LINK_TRAINING request\n");
> As I said on the previous review of the same patch: we're lying here.
> We won't execute anything yet.
I reread the previous comments and after reviewing the code, I can see 
how this could be misleading or confusing.
The message has been changed to "<test name> test requested".


>> +               intel_dp->compliance_test_data = DP_TEST_LINK_TRAINING;
>> +               response = intel_dp_autotest_link_training(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_VIDEO_PATTERN:
>> +               DRM_DEBUG_KMS("Executing TEST_PATTERN request\n");
>> +               intel_dp->compliance_test_data = DP_TEST_LINK_VIDEO_PATTERN;
>> +               response = intel_dp_autotest_video_pattern(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_EDID_READ:
>> +               DRM_DEBUG_KMS("Executing EDID request\n");
>> +               intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ;
>> +               response = intel_dp_autotest_edid(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_PHY_TEST_PATTERN:
>> +               DRM_DEBUG_KMS("Executing PHY_PATTERN request\n");
>> +               intel_dp->compliance_test_data = DP_TEST_LINK_PHY_TEST_PATTERN;
>> +               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;
>> +       /* Unsupported test case or something went wrong */
> Is there a way to differentiate the "unsupported" and the "went wrong"
> cases on dmesg?
I gave this one some thought and after looking at the spec and the code, 
the default case is there to catch any invalid test request. So I 
changed the debug message to reflect that - it now states that it has 
encountered and invalid test request and properly NAKs it.
>
>
>> +       default:
>> +               DRM_DEBUG_KMS("Unhandled test request\n");
>> +               break;
>> +       }
>> +       if (status != 0) {
>> +               response = DP_TEST_NAK;
>> +               DRM_DEBUG_KMS("Error %d processing test request\n", status);
>> +       }
>> +       status = drm_dp_dpcd_write(&intel_dp->aux,
>> +                                  DP_TEST_RESPONSE,
>> +                                  &response, 1);
>> +       intel_dp->compliance_testing_active = 0;
>> +
>>   }
> And the most important thing: the patch doesn't compile. Please make
> sure each patch in the series compiles and works. We do tons of git
> bisections on our tree, that's really important.
Yeah that was an artifact of multiple rebases and moving code around. 
I'll scrub the whole series again when I post version 2.

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

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

[Daniel Vetter]

On Thu, Oct 23, 2014 at 09:58:47AM -0700, Todd Previte wrote:
> On 10/20/2014 10:48 AM, Paulo Zanoni wrote:
> >2014-10-09 12:38 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> >>+/* Displayport compliance testing - PHY pattern testing */
> >>+static uint8_t
> >>+intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
> >>+{
> >>+       uint8_t test_result = DP_TEST_NAK;
> >>+       return test_result;
> >>+}
> >I guess a lot of people would have just made the code return NAK
> >without even defining/calling these functions above.
> 
> It came down to two ways of implementing this initial patch. I could have
> NAK'd in the handler and omitted these functions until they were fully
> implemented. Or I could do what I did here, which was to put the skeleton in
> place and add the flesh to bones when ready. To me, this seems to make more
> sense to me because it gets the structure in place and builds on it in
> subsequent patches.

Random drive-by comment: Imo rolling out the scaffolding in this patch
here looks ok. Personally I'd probably have done what Paulo suggested
too. Maybe even with a default switch that just yells with a
DRM_ERROR("Test not yet implement: %i\n", testcode) and then fill things
out step-by-step. But that's kinda a bikeshed ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 02/10] drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs

[Todd Previte]

To address previous feedback, I'll quote below and answer.

>It would be nice if you could cite on the commit message the name of
>the specification and the name of the test(s) that use it.

Done. For reference, in the Displayport Link CTS spec, tests 4.2.2.4 and 
4.2.2.5 are the ones that use these, specifically.

>Does it really need to be uint8_t? I see on patch 7 that you don't
>really write this value to a place that only accepts uint8_t-sized
>arguments, so I fear that if we get 256 NACKs or DEFERs we may end up
>doing the wrong thing.

There are no compliance tests that are concerned with the number of 
native DEFERs or NACKs received, hence why they are not addressed here.
There's no requirement for the size of this value and I selected uint8_t 
as the smallest reasonable size for it. It's only used to count the 
number of NACKs and DEFERs during compliance testing and it gets reset 
to 0 at the beginning of the test block where it's used in a later 
patch. It's unlikely that a case would occur during this compliance test 
where exactly 256 NACKs or DEFERs occur, but your point is well-taken. 
I'll make them uint32s and be done with it. I don't think 6 extra bytes 
is going to run the kernel out of memory. :)

>Also, why don't we need to count the native NACKs and DEFERs?

There are no compliance tests that are concerned with the number of 
native DEFERs or NACKs received.

New patch will be here shortly.

-T

On 10/21/2014 10:10 AM, Paulo Zanoni wrote:
> 2014-10-09 12:38 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> These counters are used for Displayort complinace testing to detect error conditions
>> when executing certain compliance tests. Currently these are used in the EDID tests
>> to determine if the video mode needs to be set to the preferred mode or the failsafe
>> mode.
>>
>> Cc: dri-devel@lists.freedesktop.org
> I see that this patch and a few others in your series still have
> unaddressed/unanswered review comments, given on the first time you
> sent the patches. Please take a look at them.
>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/drm_dp_helper.c | 2 ++
>>   include/drm/drm_dp_helper.h     | 1 +
>>   2 files changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>> index 08e33b8..8353051 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>>
>>                  case DP_AUX_I2C_REPLY_NACK:
>>                          DRM_DEBUG_KMS("I2C nack\n");
>> +                       aux->i2c_nack_count++;
>>                          return -EREMOTEIO;
>>
>>                  case DP_AUX_I2C_REPLY_DEFER:
>>                          DRM_DEBUG_KMS("I2C defer\n");
>> +                       aux->i2c_defer_count++;
>>                          usleep_range(400, 500);
>>                          continue;
>>
>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> index 8edeed0..45f3ee8 100644
>> --- a/include/drm/drm_dp_helper.h
>> +++ b/include/drm/drm_dp_helper.h
>> @@ -551,6 +551,7 @@ struct drm_dp_aux {
>>          struct mutex hw_mutex;
>>          ssize_t (*transfer)(struct drm_dp_aux *aux,
>>                              struct drm_dp_aux_msg *msg);
>> +       uint8_t i2c_nack_count, i2c_defer_count;
>>   };
>>
>>   ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

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

Re: [Intel-gfx] [PATCH 02/10] drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs

[Daniel Vetter]

On Tue, Nov 04, 2014 at 03:12:27PM -0700, Todd Previte wrote:
> >Does it really need to be uint8_t? I see on patch 7 that you don't
> >really write this value to a place that only accepts uint8_t-sized
> >arguments, so I fear that if we get 256 NACKs or DEFERs we may end up
> >doing the wrong thing.
> 
> There are no compliance tests that are concerned with the number of native
> DEFERs or NACKs received, hence why they are not addressed here.
> There's no requirement for the size of this value and I selected uint8_t as
> the smallest reasonable size for it. It's only used to count the number of
> NACKs and DEFERs during compliance testing and it gets reset to 0 at the
> beginning of the test block where it's used in a later patch. It's unlikely
> that a case would occur during this compliance test where exactly 256 NACKs
> or DEFERs occur, but your point is well-taken. I'll make them uint32s and be
> done with it. I don't think 6 extra bytes is going to run the kernel out of
> memory. :)

Du to struct alignement rules a lone uint8_t will actually occupy as much
as a plain unsigned. Also uint8_t in drivers has a bit the air of a
carefully picked size, usually in relation to some hw limit (e.g. register
size).

Imo just go with unsigned, it's clearer in intent and actually doesn't use
more space.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/10] 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

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 64c8e04..494ed26 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3959,11 +3959,77 @@ 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_NAK;
+	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",
+			      status);
+		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",
+			      status);
+
+	intel_dp->compliance_testing_active = 0;
 }
 
 static int
-- 
1.9.1

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

Re: [PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing

[Jesse Barnes]

On Thu, 23 Oct 2014 17:43:01 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Oct 23, 2014 at 10:58:59AM -0200, Paulo Zanoni wrote:
> > 2014-10-23 10:50 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> > > I think it's better to expose this as drm properties on the DP
> > > connector. This has a pile of upsides:
> > > - We don't need to invent a new parser.
> > > - Users can play with them to test out different theories.
> > > - We could even use this to fix broken panels/boards without vbt
> > > or anything using our plan to set up the initial config with a dt
> > > firmware blob.
> > >
> > > So would be a lot more useful than this private interface.
> > >
> > > For the properties themselves I think we should go with explicit
> > > enumerations with default value "automatic" and then an
> > > enumeration of all values allowed by DP. For the naming of the
> > > properties I guess something like DP_link_lanes,
> > > DP_link_clock, ... would look pretty. The properties should be in
> > > dev->mode_config (since they're reallly generic) and I think we
> > > want one function to register them all in one go in the
> > > drm_dp_helper.c library. Maybe we could even put the values into
> > > the existing dp source struct so that we don't have to reinvent
> > > the decode logic every single time.
> > 
> > I disagree. I do prefer the debugfs thing. Think about all the
> > examples of people that break their systems by passing
> > i915.enable_foo=1 or i915.enable_rc6=7 and then open bug reports...
> > With debug stuff exposed as DP properties, we're going to increase
> > that problem, and in a way that will make it even harder to detect.
> > I really really think these things should be exposed only to the
> > debugfs users, because then if the debugfs file is closed, we can
> > just ignore all the arguments and keep doing "normal" unaffected
> > modesets.
> > 
> > If we don't want the parser, maybe we can make it a binary protocol:
> > we'lll still have to parse it, but it can get much easier.
> 
> We already expose piles of funky stuff to users in these properties
> (e.g. audio on hdmi). And contrary to the module options most of
> these will just result in black screens if you don't understand what
> you're doing, so I think the risk is low. There's also a bit of an
> issue with the current interface as it's not clear which line
> corresponds to which DP interface. Using properties would solve this.
> Also these options have a much more direct impact - if you set them
> and the screen goes dark then the user hopefully realizes that this
> is something they shouldn't touch.
> 
> For fbc and rc6 and all those the problem is that nothing really
> happens, the system just becomes a bit more unstable and randomly
> crashes. Which users then report.
> 
> But If you're really concerned about users doing crappy stuff we
> could add a module option to drm_dp_helper.c which hides these, and
> then taint the kernel if any user sets them. But I really think this
> is overkill. -Daniel

Just chatted with Todd about the state of this patchset.  He's going to
post an update today or tomorrow and summarize the feedback from July
ad what he's done to address it, add changelogs, and address outstanding
review feedback from this posting.

I like the idea of exposing some DP stuff like lane count, preemph,
etc, as new properties, but I don't think it's reasonable to block the
testing stuff on it, for a few reasons:
  1) we should think pretty hard about how we want new ABI like
     this exposed
  2) the compliance spec changes pretty regularly, so keeping an
     unstable interface for it in debugfs may make sense anyway
  3) even if we decide to move the userland test code over to
     properties in the future, the fact that debugfs is unstable means
     we can drop it at that time

So I asked Todd to file a JIRA for the properties feature request,
since it's definitely worth looking at.

Which isn't to say the current interface doesn't have issues, just that
Todd shouldn't rewrite things again to include a new, stable ABI,
probably keeping compliance testing and additional DP test coverage out
of the tree for even longer.

Thoughts, objections?

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

Re: [PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing

[Daniel Vetter]

On Thu, Nov 13, 2014 at 7:44 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> > > I think it's better to expose this as drm properties on the DP
>> > > connector. This has a pile of upsides:
>> > > - We don't need to invent a new parser.
>> > > - Users can play with them to test out different theories.
>> > > - We could even use this to fix broken panels/boards without vbt
>> > > or anything using our plan to set up the initial config with a dt
>> > > firmware blob.
>> > >
>> > > So would be a lot more useful than this private interface.
>> > >
>> > > For the properties themselves I think we should go with explicit
>> > > enumerations with default value "automatic" and then an
>> > > enumeration of all values allowed by DP. For the naming of the
>> > > properties I guess something like DP_link_lanes,
>> > > DP_link_clock, ... would look pretty. The properties should be in
>> > > dev->mode_config (since they're reallly generic) and I think we
>> > > want one function to register them all in one go in the
>> > > drm_dp_helper.c library. Maybe we could even put the values into
>> > > the existing dp source struct so that we don't have to reinvent
>> > > the decode logic every single time.
>> >
>> > I disagree. I do prefer the debugfs thing. Think about all the
>> > examples of people that break their systems by passing
>> > i915.enable_foo=1 or i915.enable_rc6=7 and then open bug reports...
>> > With debug stuff exposed as DP properties, we're going to increase
>> > that problem, and in a way that will make it even harder to detect.
>> > I really really think these things should be exposed only to the
>> > debugfs users, because then if the debugfs file is closed, we can
>> > just ignore all the arguments and keep doing "normal" unaffected
>> > modesets.
>> >
>> > If we don't want the parser, maybe we can make it a binary protocol:
>> > we'lll still have to parse it, but it can get much easier.
>>
>> We already expose piles of funky stuff to users in these properties
>> (e.g. audio on hdmi). And contrary to the module options most of
>> these will just result in black screens if you don't understand what
>> you're doing, so I think the risk is low. There's also a bit of an
>> issue with the current interface as it's not clear which line
>> corresponds to which DP interface. Using properties would solve this.
>> Also these options have a much more direct impact - if you set them
>> and the screen goes dark then the user hopefully realizes that this
>> is something they shouldn't touch.
>>
>> For fbc and rc6 and all those the problem is that nothing really
>> happens, the system just becomes a bit more unstable and randomly
>> crashes. Which users then report.
>>
>> But If you're really concerned about users doing crappy stuff we
>> could add a module option to drm_dp_helper.c which hides these, and
>> then taint the kernel if any user sets them. But I really think this
>> is overkill. -Daniel
>
> Just chatted with Todd about the state of this patchset.  He's going to
> post an update today or tomorrow and summarize the feedback from July
> ad what he's done to address it, add changelogs, and address outstanding
> review feedback from this posting.
>
> I like the idea of exposing some DP stuff like lane count, preemph,
> etc, as new properties, but I don't think it's reasonable to block the
> testing stuff on it, for a few reasons:
>   1) we should think pretty hard about how we want new ABI like
>      this exposed
>   2) the compliance spec changes pretty regularly, so keeping an
>      unstable interface for it in debugfs may make sense anyway
>   3) even if we decide to move the userland test code over to
>      properties in the future, the fact that debugfs is unstable means
>      we can drop it at that time
>
> So I asked Todd to file a JIRA for the properties feature request,
> since it's definitely worth looking at.
>
> Which isn't to say the current interface doesn't have issues, just that
> Todd shouldn't rewrite things again to include a new, stable ABI,
> probably keeping compliance testing and additional DP test coverage out
> of the tree for even longer.
>
> Thoughts, objections?

Ok, so from a really, really high-level perspective the design review
from my side in July had one primary goal and a corollary from that:

- DP validation should exercise the same code paths (where possible)
than what actual customers use in DP mode sets. If we implement
completely separate codepaths (which the first iteration partially
did) then we validate a separate driver and not actually what
customers run. Which is fairly pointless imo.

- Corollary was that this means we should drive the full kernel stack
and so control the testing from userspace using the same interfaces as
any userspace would use for a modeset. And if that interface has gaps,
or our kernel side DP driver has gaps we'd need to fill them. debugfs
should only serve as the testing sideband between the DP tester (sink
device) and the userspace test program. So the kernel just passes
stuff back&forth like test requests (e.g. "pls set this mode on now
and show test pattern") and return test results (e.g. "I've read the
edid now, here's the checksum"). Of course for some test requests we
can directly answer them from the kernel (e.g. "tell me how many dp
aux failures you've seen"). Examples completely made up ;-)

So from that perpective adjusting DP link paramters is either a new
tuning bit, which really should be generally available. Or it
indicates a bug/omission in our DP link training or re-training logic.
In either case I don't think it's really pure test-related sideband
date.

I guess I didn't make this 100% clear in the original design review
what my goal is, or maybe Todd misunderstood things a bit.

Aside from all this (and now with my community hat on) just adding
code to get a sticker (labelled "passed DP validation") which is
separate code and not used by actual users is imo not useful for
merging upstream. But that's just my own opinion, not sure what's
Dave's stance here or whether there's much precendence either way.

So short answer: I still think exposing this with properties is the
right approach, presuming we really need it (and it's not just to
paper over a deficient link training logic in the kernel). I also
think it'll be less code since we can simplify the debugfs option
parser.
-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 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing

[Dave Airlie]

On 14 November 2014 06:44, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Nov 13, 2014 at 7:44 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>>> > > I think it's better to expose this as drm properties on the DP
>>> > > connector. This has a pile of upsides:
>>> > > - We don't need to invent a new parser.
>>> > > - Users can play with them to test out different theories.
>>> > > - We could even use this to fix broken panels/boards without vbt
>>> > > or anything using our plan to set up the initial config with a dt
>>> > > firmware blob.
>>> > >
>>> > > So would be a lot more useful than this private interface.
>>> > >
>>> > > For the properties themselves I think we should go with explicit
>>> > > enumerations with default value "automatic" and then an
>>> > > enumeration of all values allowed by DP. For the naming of the
>>> > > properties I guess something like DP_link_lanes,
>>> > > DP_link_clock, ... would look pretty. The properties should be in
>>> > > dev->mode_config (since they're reallly generic) and I think we
>>> > > want one function to register them all in one go in the
>>> > > drm_dp_helper.c library. Maybe we could even put the values into
>>> > > the existing dp source struct so that we don't have to reinvent
>>> > > the decode logic every single time.
>>> >
>>> > I disagree. I do prefer the debugfs thing. Think about all the
>>> > examples of people that break their systems by passing
>>> > i915.enable_foo=1 or i915.enable_rc6=7 and then open bug reports...
>>> > With debug stuff exposed as DP properties, we're going to increase
>>> > that problem, and in a way that will make it even harder to detect.
>>> > I really really think these things should be exposed only to the
>>> > debugfs users, because then if the debugfs file is closed, we can
>>> > just ignore all the arguments and keep doing "normal" unaffected
>>> > modesets.
>>> >
>>> > If we don't want the parser, maybe we can make it a binary protocol:
>>> > we'lll still have to parse it, but it can get much easier.
>>>
>>> We already expose piles of funky stuff to users in these properties
>>> (e.g. audio on hdmi). And contrary to the module options most of
>>> these will just result in black screens if you don't understand what
>>> you're doing, so I think the risk is low. There's also a bit of an
>>> issue with the current interface as it's not clear which line
>>> corresponds to which DP interface. Using properties would solve this.
>>> Also these options have a much more direct impact - if you set them
>>> and the screen goes dark then the user hopefully realizes that this
>>> is something they shouldn't touch.
>>>
>>> For fbc and rc6 and all those the problem is that nothing really
>>> happens, the system just becomes a bit more unstable and randomly
>>> crashes. Which users then report.
>>>
>>> But If you're really concerned about users doing crappy stuff we
>>> could add a module option to drm_dp_helper.c which hides these, and
>>> then taint the kernel if any user sets them. But I really think this
>>> is overkill. -Daniel
>>
>> Just chatted with Todd about the state of this patchset.  He's going to
>> post an update today or tomorrow and summarize the feedback from July
>> ad what he's done to address it, add changelogs, and address outstanding
>> review feedback from this posting.
>>
>> I like the idea of exposing some DP stuff like lane count, preemph,
>> etc, as new properties, but I don't think it's reasonable to block the
>> testing stuff on it, for a few reasons:
>>   1) we should think pretty hard about how we want new ABI like
>>      this exposed
>>   2) the compliance spec changes pretty regularly, so keeping an
>>      unstable interface for it in debugfs may make sense anyway
>>   3) even if we decide to move the userland test code over to
>>      properties in the future, the fact that debugfs is unstable means
>>      we can drop it at that time
>>
>> So I asked Todd to file a JIRA for the properties feature request,
>> since it's definitely worth looking at.
>>
>> Which isn't to say the current interface doesn't have issues, just that
>> Todd shouldn't rewrite things again to include a new, stable ABI,
>> probably keeping compliance testing and additional DP test coverage out
>> of the tree for even longer.
>>
>> Thoughts, objections?
>
> Ok, so from a really, really high-level perspective the design review
> from my side in July had one primary goal and a corollary from that:
>
> - DP validation should exercise the same code paths (where possible)
> than what actual customers use in DP mode sets. If we implement
> completely separate codepaths (which the first iteration partially
> did) then we validate a separate driver and not actually what
> customers run. Which is fairly pointless imo.
>
> - Corollary was that this means we should drive the full kernel stack
> and so control the testing from userspace using the same interfaces as
> any userspace would use for a modeset. And if that interface has gaps,
> or our kernel side DP driver has gaps we'd need to fill them. debugfs
> should only serve as the testing sideband between the DP tester (sink
> device) and the userspace test program. So the kernel just passes
> stuff back&forth like test requests (e.g. "pls set this mode on now
> and show test pattern") and return test results (e.g. "I've read the
> edid now, here's the checksum"). Of course for some test requests we
> can directly answer them from the kernel (e.g. "tell me how many dp
> aux failures you've seen"). Examples completely made up ;-)
>
> So from that perpective adjusting DP link paramters is either a new
> tuning bit, which really should be generally available. Or it
> indicates a bug/omission in our DP link training or re-training logic.
> In either case I don't think it's really pure test-related sideband
> date.
>
> I guess I didn't make this 100% clear in the original design review
> what my goal is, or maybe Todd misunderstood things a bit.
>
> Aside from all this (and now with my community hat on) just adding
> code to get a sticker (labelled "passed DP validation") which is
> separate code and not used by actual users is imo not useful for
> merging upstream. But that's just my own opinion, not sure what's
> Dave's stance here or whether there's much precendence either way.
>
> So short answer: I still think exposing this with properties is the
> right approach, presuming we really need it (and it's not just to
> paper over a deficient link training logic in the kernel). I also
> think it'll be less code since we can simplify the debugfs option
> parser.

Don't expose DP stuff in properties, I don't want users controlling
the parameters of the DP link in any way. I can't see any use
in userspace for controlling this stuff. So I'm happier with debugfs,
to avoid making an ABI we hate later.

Yes I do prefer we make DP validation go via the same paths,
but some parts of DP validation require things userspace shouldn't
be allowed setup, and for those we should have bypasses, everything
else should be done via normal channels.

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

Re: [PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing

[Jesse Barnes]

On Fri, 14 Nov 2014 07:00:17 +1000
Dave Airlie <airlied@gmail.com> wrote:

> > Aside from all this (and now with my community hat on) just adding
> > code to get a sticker (labelled "passed DP validation") which is
> > separate code and not used by actual users is imo not useful for
> > merging upstream. But that's just my own opinion, not sure what's
> > Dave's stance here or whether there's much precendence either way.
> >
> > So short answer: I still think exposing this with properties is the
> > right approach, presuming we really need it (and it's not just to
> > paper over a deficient link training logic in the kernel). I also
> > think it'll be less code since we can simplify the debugfs option
> > parser.  
> 
> Don't expose DP stuff in properties, I don't want users controlling
> the parameters of the DP link in any way. I can't see any use
> in userspace for controlling this stuff. So I'm happier with debugfs,
> to avoid making an ABI we hate later.
> 
> Yes I do prefer we make DP validation go via the same paths,
> but some parts of DP validation require things userspace shouldn't
> be allowed setup, and for those we should have bypasses, everything
> else should be done via normal channels.

Yeah, agreed on re-using code paths.  I just don't think it means we
have to re-use ioctl or property entry points.  The internals of the
debugfs file can just as easily call internal functions as the
ioctl/property code, so I think we can be covered that way too.

I guess we'll have to check out Todd's latest patches when he posts
them.

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