Displayport Compliance Testing V3

Displayport Compliance Testing V3

[Todd Previte]

These are the kernel-side patches to enable Displayport compliance testing for 
the i915 driver. They establish a framework and operational parameters for 
compliance testing operations on the i915 driver. Structure is in place such 
that more tests can be added in the near future. Combined with the userspace 
application (currently being integrated into the intel-gnu-tools tree) these 
patches enable the following tests:
        4.2.1.1 Source DUT Retry on No-Reply During AUX Read after HPD Plug Event
        4.2.1.2 Source Retry on Invalid Reply During AUX Read after HPD Plug Event
        4.2.2.1 EDID Read upon HPD Plug Event
        4.2.2.2 DPCD Receiver Capability Read upon HPD Plug Event
        4.2.2.3 EDID Read
        4.2.2.4 EDID Read Failure #1: I2C-Over-AUX NACK
        4.2.2.5 EDID Read Failure #2: I2C-Over-AUX DEFER
        4.2.2.6 EDID Corruption Detection
Without the user app to set the required video mode, some of these tests will 
still pass. 4.2.1.1 and 4.2.1.2 do not require the user app, nor does 4.2.2.2. 
Some of the others may pass with warnings about the video mode not being set 
correctly, depending on your test device. Documentation for using the userspace 
app and an overview of this implementation is available with the user app.

drm/i915: Add debugfs write and test param parsing functions for DP test control
drm/i915: Add new debugfs file for Displaypor compliance test control
drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation
drm/i915: Update intel_dp_compute_config() to handle compliance test requests
drm/i915: Update the EDID automated compliance test function
drm/i915: Add debugfs functions for Displayport compliance testing
drm/i915: Add a delay in Displayport AUX transactions for compliance testing
drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing
drm/i915: Add automated testing support for Displayport compliance testing

drivers/gpu/drm/i915/i915_debugfs.c | 643 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_dp.c     | 249 ++++++++++++++++++++----
drivers/gpu/drm/i915/intel_drv.h    |  18 ++
3 files changed, 870 insertions(+), 40 deletions(-)

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

[PATCH 1/9] drm/i915: Add automated testing support for Displayport compliance testing

[Todd Previte]

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

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

V3:
- Fixed the checks on the DPCD return code to be <= 0
  rather than != 0
- Removed extraneous assignment of a NAK return code in the
  DPCD read failure case
- Changed the return in the DPCD read failure case to a goto
  to the exit point where the status code is written to the sink
- Removed FAUX test case since it's deprecated now
- Removed the compliance flag assignment in handle_test_request

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index eea9e36..25ffa7d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3746,11 +3746,70 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 	return true;
 }
 
-static void
-intel_dp_handle_test_request(struct intel_dp *intel_dp)
+static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_ACK;
+	return test_result;
+}
+
+static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_NAK;
+	return test_result;
+}
+
+static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_NAK;
+	return test_result;
+}
+
+static uint8_t intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_NAK;
+	return test_result;
+}
+
+static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 {
-	/* NAK by default */
-	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK);
+	uint8_t response = DP_TEST_NAK;
+	uint8_t rxdata = 0;
+	int status = 0;
+
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Could not read test request from sink\n");
+		goto update_status;
+	}
+
+	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;
+	default:
+		DRM_DEBUG_KMS("Invalid test request '%02x'\n", rxdata);
+		break;
+	}
+
+update_status:
+	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");
 }
 
 static int
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index eef79cc..74b30c1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -647,6 +647,10 @@ struct intel_dp {
 				     bool has_aux_irq,
 				     int send_bytes,
 				     uint32_t aux_clock_divider);
+
+	/* Displayport compliance testing */
+	unsigned long compliance_test_data;
+	bool compliance_testing_active;
 };
 
 struct intel_digital_port {
-- 
1.9.1

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

[PATCH 2/9] 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.

The main reason for doing this is to make sure that a test request isn't missed.
Checking for the status of the encoder/crtc isn't necessary for some test cases
(AUX channel tests are one example) and without moving the check for the
interrupt, these tests may not execute if one of those checks fails.
Additionally, if reading the DPCD fails, regardless of whether or not testing is
happening, there's no way to train the link since configurations and status
can't be read, nor can link training parameters be written.

Adds a check at the top to verify that the device is connected. This is
necessary for DP compliance testing to ensure that test requests are captured
and acknowledged. 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.

This patch is actually both a bug fix and a component of compliance testing.
Because HPD events are received both on connect and disconnect actions, it's
vital that we don't try and train the link when we're transitioning from
connected->disconnected. That results in errors and warning in the logs from
failed AUX transactions and can trigger the WARN for the check of !base.crtc.
By making the check at the beginning to see if the connection is truly active,
those problems are avoided and testing / link training will only be attempted
when there is a valid Displayport connection.

V2:
- N/A
V3:
- Removed extraneous comment
- Added responses to review feedback into the patch notes

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 25ffa7d..280ae7a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3887,21 +3887,13 @@ 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)) {
+	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;
 	}
@@ -3913,13 +3905,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 		drm_dp_dpcd_writeb(&intel_dp->aux,
 				   DP_DEVICE_SERVICE_IRQ_VECTOR,
 				   sink_irq_vector);
-
 		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
 			intel_dp_handle_test_request(intel_dp);
 		if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
 
+	if (!intel_encoder->connectors_active)
+		return;
+
+	if (WARN_ON(!intel_encoder->base.crtc))
+		return;
+
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+		return;
+
+	/* Try to read receiver status if the link appears to be up */
+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+		return;
+	}
+
 	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
 		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
 			      intel_encoder->base.name);
-- 
1.9.1

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

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

[Todd Previte]

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

V2:
- Changed udelay() to usleep_range()
V3:
- Removed extraneous check for timeout
- Updated comment to reflect this change

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 280ae7a..5d7b7f3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -877,9 +877,15 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
 				   DP_AUX_CH_CTL_RECEIVE_ERROR);
 
-			if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
-				      DP_AUX_CH_CTL_RECEIVE_ERROR))
+			/* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
+			     400us delay required for errors and timeouts
+			     Timeout errors from the HW already meet this
+			     requirement so skip to next iteration
+			*/
+			if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
+				usleep_range(400, 500);
 				continue;
+			}
 			if (status & DP_AUX_CH_CTL_DONE)
 				break;
 		}
-- 
1.9.1

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

[PATCH 4/9] drm/i915: Add debugfs functions for Displayport compliance testing

[Todd Previte]

This patch is the amalgamation of 7 patches from the V2 series. These
patches all involve the implementation of the debugfs mechanism for
handling Displayport compliance testing. The following are the commit
messages from those 7 patches (included here to try and preserve the
patch set history):

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

Originally, this patch was part of "[PATCH 05/10] drm/i915: Add debugfs
interface for Displayport debug and compliance testing".  It contained
definitions/declarations for some of the constants and data structures
added to support debugfs output for Displayport compliance
testing.
-------------------------------------------------------------------------------
[PATCH 05/17] drm/i915: Add file ops for Displayport configuration in debugfs

This patch was also part of "[PATCH 05/10] drm/i915: Add debugfs interface
for Displayport debug and compliance testing". This one added file operations
structures for Displayport configuration and the declarations for open() and
write() in the file ops structure.
-------------------------------------------------------------------------------
[PATCH 06/17] drm/i915: Add support functions in debugfs for handling
              Displayport compliance configuration

This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs interface
for Displayport debug  and compliance testing". It added two support functions
for handling Displayport configuration parameters that are used for compliance
testing.
-------------------------------------------------------------------------------
[PATCH 07/17] drm/i915: Add and implement the debugfs 'show' functions for
              Displayport compliance

This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs
interface for Displayport debug and compliance testing". This patch implemented
the show() function.
-------------------------------------------------------------------------------
[PATCH 08/17] drm/i915: Add Displayport link configuration structure

This patch was previously part of "[PATCH 07/10] drm/i915: Add structures for
Displayport compliance testing parameters". It added a struct to maintain link
configuration data. These were meant purely for Displayport compliance use.
-------------------------------------------------------------------------------
[PATCH 09/17] drm/i915: Add config parsing utilities in debugfs for Displayport
              compliance

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

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

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

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

The nature and scope of the contents of this patch make breaking it into
smaller chunks difficult. With everthing contained in a single patch, the
code is easier to review and understand.

That said, this monster of a patch does quite a few things. First, it adds
some necessary data structures for performing compliance testing operations
on the kernel side. Specifically, it adds link configuration parameters and
string tokens for the debugfs files. There are multiple utility functions
added here as well in support of the debugfs system for handling compliance
testing. Functions for printing strings to the files with the correct format,
parsing tokens, tokenizing strings and doing the grunt work for writing the
data to the debugfs files.

This patch also adds the basic debugfs structures to build the file for the
link configuration output as well as the file operations structures to handle
the open and show functions.

V2:
- N/A
V3:
- Merged patches 4-10 into a single patch per review feedback
- Added previous commit messages from the other commits integrated
  here to preserve the integrity of the patch sequence
- Adjusted the config_ctl_write function to only check the drm_connector
  struct for a) type == Displayport and b) status == active. This is
  to eliminate patches 14 and 15 from V2, which become unnecessary
  in light of this change.
- Fixed checkpatch.pl errors

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b315f01..b77574e 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,121 @@ static const char *yesno(int v)
 	return v ? "yes" : "no";
 }
 
+#define MAX_DP_CONFIG_LINE_COUNT	64
+
+enum dp_config_param {
+	DP_CONFIG_PARAM_CONNECTOR = 0,
+	DP_CONFIG_PARAM_CONNECTOR_ID,
+	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,
+	DP_PARAMETER_COUNT,
+	DP_CONFIG_PARAM_INVALID = -1
+};
+
+struct dp_config {
+	enum dp_config_param type;
+	unsigned long value;
+};
+
+static char *dp_get_config_str(int for_token)
+{
+	char *s;
+
+	switch (for_token) {
+	case DP_CONFIG_PARAM_CONNECTOR:
+		s = "Connector";
+		break;
+	case DP_CONFIG_PARAM_CONNECTOR_ID:
+		s = "Connector ID";
+		break;
+	case DP_CONFIG_PARAM_LINK_RATE:
+		s = "Link Rate";
+		break;
+	case DP_CONFIG_PARAM_LANE_COUNT:
+		s = "Lane Count";
+		break;
+	case DP_CONFIG_PARAM_VOLTAGE_SWING:
+		s = "Voltage Swing Level";
+		break;
+	case DP_CONFIG_PARAM_PREEMPHASIS:
+		s = "Preemphasis Level";
+		break;
+	case DP_CONFIG_PARAM_HRES:
+		s = "Horizontal Resolution";
+		break;
+	case DP_CONFIG_PARAM_VRES:
+		s = "Vertical Resolution";
+		break;
+	case DP_CONFIG_PARAM_BPP:
+		s = "Bits Per Pixel";
+		break;
+	default:
+		s = "";
+		break;
+	}
+
+	return s;
+}
+
+static void dp_print_string(struct seq_file *m,
+			    enum dp_config_param for_param,
+			    void *data)
+{
+	char *format_string;
+	unsigned long output_value;
+
+	switch (for_param) {
+	case DP_CONFIG_PARAM_CONNECTOR:
+		/* Special case for the string parameter */
+		format_string = "Connector:             %s\n";
+		seq_printf(m, format_string, (char *) data);
+		break;
+	case DP_CONFIG_PARAM_CONNECTOR_ID:
+		format_string = "Connector ID:          %u\n";
+		output_value = *(unsigned int *) data;
+		break;
+	case DP_CONFIG_PARAM_LINK_RATE:
+		format_string = "Link Rate:             %02x\n";
+		output_value = *(unsigned char *) data;
+		break;
+	case DP_CONFIG_PARAM_LANE_COUNT:
+		format_string = "Lane Count:            %u\n";
+		output_value = *(unsigned int *) data;
+		break;
+	case DP_CONFIG_PARAM_VOLTAGE_SWING:
+		format_string = "Voltage Swing Level:   %u\n";
+		output_value = *(unsigned char *) data;
+		break;
+	case DP_CONFIG_PARAM_PREEMPHASIS:
+		format_string = "Preemphasis Level:     %u\n";
+		output_value = *(unsigned char *) data;
+		break;
+	case DP_CONFIG_PARAM_HRES:
+		format_string = "Horizontal Resolution: %d\n";
+		output_value = *(int *) data;
+		break;
+	case DP_CONFIG_PARAM_VRES:
+		format_string = "Vertical Resolution:   %d\n";
+		output_value = *(int *) data;
+		break;
+	case DP_CONFIG_PARAM_BPP:
+		format_string = "Bits Per Pixel:        %u\n";
+		output_value = *(unsigned char *) data;
+		break;
+	default:
+		format_string = NULL;
+		break;
+	}
+
+	if (format_string != NULL && for_param != DP_CONFIG_PARAM_CONNECTOR)
+		seq_printf(m, format_string, output_value);
+}
+
 /* 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
@@ -3701,6 +3817,360 @@ static const struct file_operations i915_display_crc_ctl_fops = {
 	.write = display_crc_ctl_write
 };
 
+static void dp_show_config(struct seq_file *m,
+			   struct drm_connector *connector)
+{
+	struct intel_dp *intel_dp;
+	struct intel_crtc *crtc;
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct intel_encoder *intel_encoder = intel_connector->encoder;
+	int pipe_bpp, hres, vres;
+	uint8_t vs[4], pe[4];
+	struct drm_display_mode *mode;
+	int i = 0;
+
+	if (!intel_encoder)
+		return;
+
+	if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT)
+		return;
+
+	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) >>
+			DP_TRAIN_PRE_EMPHASIS_SHIFT;
+	}
+
+	/* Output mode info only if a valid CRTC exists */
+	if (intel_encoder->base.crtc) {
+		crtc = to_intel_crtc(intel_encoder->base.crtc);
+		pipe_bpp = crtc->config->pipe_bpp;
+		mode = &crtc->config->base.adjusted_mode;
+		hres = mode->hdisplay;
+		vres = mode->vdisplay;
+	} else {
+		pipe_bpp = 0;
+		hres = vres = 0;
+	}
+
+	dp_print_string(m,
+			DP_CONFIG_PARAM_CONNECTOR,
+			connector->name);
+	dp_print_string(m,
+			DP_CONFIG_PARAM_CONNECTOR_ID,
+			&connector->base.id);
+	dp_print_string(m,
+			DP_CONFIG_PARAM_LINK_RATE,
+			&intel_dp->link_bw);
+	dp_print_string(m,
+			DP_CONFIG_PARAM_LANE_COUNT,
+			&intel_dp->lane_count);
+	dp_print_string(m,
+			DP_CONFIG_PARAM_VOLTAGE_SWING,
+			&vs[0]);
+	dp_print_string(m,
+			DP_CONFIG_PARAM_PREEMPHASIS,
+			&pe[0]);
+	dp_print_string(m,
+			DP_CONFIG_PARAM_HRES,
+			&hres);
+	dp_print_string(m,
+			DP_CONFIG_PARAM_VRES,
+			&vres);
+	dp_print_string(m,
+			DP_CONFIG_PARAM_BPP,
+			&pipe_bpp);
+}
+
+enum dp_config_param dp_get_param_type(char *input_string)
+{
+	enum dp_config_param param_type = DP_CONFIG_PARAM_INVALID;
+	char *s;
+	int i = 0;
+
+	for (i = 0; i < DP_PARAMETER_COUNT; i++) {
+		s = dp_get_config_str(i);
+		if (strncmp(input_string, s,
+			    strlen(s)) == 0) {
+			param_type = (enum dp_config_param) i;
+			break;
+		}
+	}
+	return param_type;
+}
+
+int dp_get_param_value(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 dp_tokenize_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 dp_parse_config(char *input_buffer,
+			   ssize_t buffer_size,
+			   struct intel_dp *intel_dp)
+{
+	int status = 0;
+	char *lines[MAX_DP_CONFIG_LINE_COUNT];
+	int i = 0;
+	struct dp_config parms[DP_PARAMETER_COUNT];
+	int line_count = 0;
+	char *buffer = input_buffer;
+	enum dp_config_param parm_type;
+	unsigned long parm_value;
+
+	line_count = dp_tokenize_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 = dp_get_param_type(lines[i]);
+		if (parm_type != DP_CONFIG_PARAM_INVALID) {
+			status = dp_get_param_value(parm_type,
+							lines[i+1],
+							&parm_value);
+			if (status == 0) {
+				parms[parm_type].type = parm_type;
+				parms[parm_type].value = parm_value;
+			}
+		}
+	}
+
+	if (parms[DP_CONFIG_PARAM_LINK_RATE].value == 0x06 ||
+	    parms[DP_CONFIG_PARAM_LINK_RATE].value == 0x0a ||
+	    parms[DP_CONFIG_PARAM_LINK_RATE].value == 0x14) {
+		intel_dp->compliance_config.link_rate =
+			parms[DP_CONFIG_PARAM_LINK_RATE].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_LANE_COUNT].value == 0x01 ||
+	    parms[DP_CONFIG_PARAM_LANE_COUNT].value == 0x02 ||
+	    parms[DP_CONFIG_PARAM_LANE_COUNT].value == 0x04) {
+		intel_dp->compliance_config.lane_count =
+			parms[DP_CONFIG_PARAM_LANE_COUNT].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_VOLTAGE_SWING].value <= 0x03 &&
+	    parms[DP_CONFIG_PARAM_VOLTAGE_SWING].value >= 0x00) {
+		intel_dp->compliance_config.vswing_level =
+			parms[DP_CONFIG_PARAM_VOLTAGE_SWING].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_PREEMPHASIS].value <= 0x03 &&
+	    parms[DP_CONFIG_PARAM_PREEMPHASIS].value >= 0x00) {
+		intel_dp->compliance_config.preemp_level =
+			parms[DP_CONFIG_PARAM_PREEMPHASIS].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_BPP].value == 18 ||
+	    parms[DP_CONFIG_PARAM_BPP].value == 24 ||
+	    parms[DP_CONFIG_PARAM_BPP].value == 30 ||
+	    parms[DP_CONFIG_PARAM_BPP].value == 36) {
+		intel_dp->compliance_config.bits_per_pixel =
+			parms[DP_CONFIG_PARAM_PREEMPHASIS].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_HRES].value > 0 &&
+	    parms[DP_CONFIG_PARAM_HRES].value <= 8192) {
+		intel_dp->compliance_config.hres =
+			parms[DP_CONFIG_PARAM_HRES].value;
+	} else
+		return -EINVAL;
+
+	if (parms[DP_CONFIG_PARAM_VRES].value > 0 &&
+	    parms[DP_CONFIG_PARAM_VRES].value <= 8192) {
+		intel_dp->compliance_config.vres =
+			parms[DP_CONFIG_PARAM_VRES].value;
+	} else
+		return -EINVAL;
+
+	return status;
+}
+
+static int i915_displayport_config_ctl_show(struct seq_file *m, void *data)
+{
+	struct drm_device *dev = m->private;
+	struct drm_connector *connector;
+	struct list_head *connector_list = &dev->mode_config.connector_list;
+
+	if (!dev)
+		return -ENODEV;
+
+	list_for_each_entry(connector, connector_list, head) {
+
+		if (connector->connector_type !=
+		    DRM_MODE_CONNECTOR_DisplayPort)
+			continue;
+
+		if (connector->status == connector_status_connected) {
+			dp_show_config(m, connector);
+		} else {
+			dp_print_string(m,
+					DP_CONFIG_PARAM_CONNECTOR,
+					connector->name);
+			dp_print_string(m,
+					DP_CONFIG_PARAM_CONNECTOR_ID,
+					&connector->base.id);
+			seq_puts(m, "disconnected\n");
+		}
+	}
+
+	return 0;
+}
+
+static int i915_displayport_config_ctl_open(struct inode *inode,
+				       struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+
+	return single_open(file, i915_displayport_config_ctl_show, dev);
+}
+
+static ssize_t i915_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 list_head *connector_list;
+	struct intel_dp *intel_dp;
+
+	m = file->private_data;
+	if (!m) {
+		status = -ENODEV;
+		return status;
+	}
+	dev = m->private;
+
+	if (!dev) {
+		status = -ENODEV;
+		return status;
+	}
+	connector_list = &dev->mode_config.connector_list;
+
+	if (len == 0)
+		return 0;
+
+	input_buffer = kmalloc(len + 1, GFP_KERNEL);
+	if (!input_buffer)
+		return -ENOMEM;
+
+	if (copy_from_user(input_buffer, ubuf, len)) {
+		status = -EFAULT;
+		goto out;
+	}
+
+	input_buffer[len] = '\0';
+	DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
+
+	list_for_each_entry(connector, connector_list, head) {
+		if (connector->connector_type ==
+		    DRM_MODE_CONNECTOR_DisplayPort &&
+		    connector->status == connector_status_connected) {
+			intel_dp = enc_to_intel_dp(connector->encoder);
+			status = dp_parse_config(input_buffer, len, intel_dp);
+			if (status < 0)
+				goto out;
+		}
+	}
+out:
+	kfree(input_buffer);
+	if (status < 0)
+		return status;
+
+	*offp += len;
+	return len;
+}
+
+static const struct file_operations i915_displayport_config_ctl_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_displayport_config_ctl_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+	.write = i915_displayport_config_ctl_write
+};
+
 static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
 {
 	struct drm_device *dev = m->private;
@@ -4450,6 +4920,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_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 74b30c1..c813d3c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -592,6 +592,18 @@ struct intel_hdmi {
 struct intel_dp_mst_encoder;
 #define DP_MAX_DOWNSTREAM_PORTS		0x10
 
+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;
@@ -651,6 +663,7 @@ struct intel_dp {
 	/* Displayport compliance testing */
 	unsigned long compliance_test_data;
 	bool compliance_testing_active;
+	struct intel_dp_link_config compliance_config;
 };
 
 struct intel_digital_port {
-- 
1.9.1

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

[PATCH 5/9] 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 operations
are handed off to userspace so that the userspace app can set the display
mode appropriately for the test response.

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

V2:
- Addressed mailing list feedback
- Removed excess debug messages
- Removed extraneous comments
- Fixed formatting issues (line length > 80)
- Updated the debug message in compute_edid_checksum to output hex values
  instead of decimal
V3:
- Addressed more list feedback
- Added the test_active flag to the autotest function
- Removed test_active flag from handler
- Added failsafe check on the compliance test active flag
  at the end of the test handler
- Fixed checkpatch.pl issues

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5d7b7f3..e69cdad 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)
 
+/* 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;
@@ -3764,9 +3771,76 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 	return test_result;
 }
 
+static bool intel_dp_compute_edid_checksum(uint8_t *edid_data,
+					   uint8_t *edid_checksum)
+{
+	uint32_t byte_total = 0;
+	uint8_t i = 0;
+	bool edid_ok = true;
+
+	/* Don't include last byte (the checksum) in the computation */
+	for (i = 0; i < EDID_LENGTH - 2; i++)
+		byte_total += edid_data[i];
+
+	*edid_checksum = 256 - (byte_total % 256);
+
+	if (*edid_checksum != edid_data[EDID_LENGTH - 1]) {
+		DRM_DEBUG_KMS("Invalid EDID checksum %02x, should be %02x\n",
+			      edid_data[EDID_LENGTH - 40 - 1], *edid_checksum);
+		edid_ok = false;
+	}
+
+	return edid_ok;
+}
+
 static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
 {
-	uint8_t test_result = DP_TEST_NAK;
+	struct drm_connector *connector = &intel_dp->attached_connector->base;
+	struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+	struct edid *edid_read = NULL;
+	uint8_t *edid_data = NULL;
+	uint8_t test_result = DP_TEST_NAK, checksum = 0;
+	uint32_t ret = 0;
+
+	intel_dp->aux.i2c_nack_count = 0;
+	intel_dp->aux.i2c_defer_count = 0;
+
+
+	edid_read = drm_get_edid(connector, adapter);
+
+	if (edid_read == NULL) {
+		/* Check for NACKs/DEFERs, use failsafe if detected
+		   (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
+		if (intel_dp->aux.i2c_nack_count > 0 ||
+			intel_dp->aux.i2c_defer_count > 0)
+			DRM_DEBUG_KMS("EDID read had %d NACKs, %d DEFERs\n",
+				      intel_dp->aux.i2c_nack_count,
+				      intel_dp->aux.i2c_defer_count);
+		intel_dp->compliance_test_data = INTEL_DP_EDID_CORRUPT |
+						 INTEL_DP_RESOLUTION_FAILSAFE;
+	} else {
+		edid_data = (uint8_t *) edid_read;
+
+		if (intel_dp_compute_edid_checksum(edid_data, &checksum)) {
+			ret = drm_dp_dpcd_write(&intel_dp->aux,
+						DP_TEST_EDID_CHECKSUM,
+						&edid_read->checksum, 1);
+			test_result = DP_TEST_ACK |
+				      DP_TEST_EDID_CHECKSUM_WRITE;
+			intel_dp->compliance_test_data =
+				INTEL_DP_EDID_OK |
+				INTEL_DP_RESOLUTION_PREFERRED;
+		} else {
+			/* Invalid checksum - EDID corruption detection */
+			intel_dp->compliance_test_data =
+				INTEL_DP_EDID_CORRUPT |
+				INTEL_DP_RESOLUTION_FAILSAFE;
+		}
+	}
+
+	/* Set test active flag here so userspace doesn't interrupt things */
+	intel_dp->compliance_testing_active = 1;
+
 	return test_result;
 }
 
@@ -3781,6 +3855,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 	uint8_t response = DP_TEST_NAK;
 	uint8_t rxdata = 0;
 	int status = 0;
+	int failsafe_counter = 0;
 
 	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
 	if (status <= 0) {
@@ -3816,6 +3891,18 @@ update_status:
 				   &response, 1);
 	if (status <= 0)
 		DRM_DEBUG_KMS("Could not write test response to sink\n");
+
+	/* Failsafe check on compliance testing flag */
+	while (intel_dp->compliance_testing_active) {
+		usleep_range(100, 150);
+		failsafe_counter++;
+		/*
+		   If userspace still hasn't responded after 20 iterations,
+		   chances are it's not going to, so clear the flag and move on
+		*/
+		if (failsafe_counter > 20)
+			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

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

[Todd Previte]

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

V2:
- N/A
V3:
- Moved color range check down under the compliance_exit tag

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e69cdad..2d83f13 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1191,6 +1191,21 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	pipe_config->has_drrs = false;
 	pipe_config->has_audio = intel_dp->has_audio;
 
+	/* Compliance testing should skip most of this function */
+	if (!is_edp(intel_dp) && intel_dp->compliance_testing_active) {
+		bpp = intel_dp->compliance_config.bits_per_pixel;
+		lane_count = intel_dp->compliance_config.lane_count;
+		clock = intel_dp->compliance_config.link_rate >> 3;
+		/* Assign here and skip at the end - ensures correct values */
+		intel_dp->link_bw = bws[clock];
+		intel_dp->lane_count = lane_count;
+		pipe_config->pipe_bpp = bpp;
+		pipe_config->port_clock =
+			drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
+
+		goto compliance_exit;
+	}
+
 	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
 		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
 				       adjusted_mode);
@@ -1251,6 +1266,18 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	return false;
 
 found:
+	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);
+
+	DRM_DEBUG_KMS("DP link bw %02x lane count %d clock %d bpp %d\n",
+		      intel_dp->link_bw, intel_dp->lane_count,
+		      pipe_config->port_clock, bpp);
+	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
+		      mode_rate, link_avail);
+
+compliance_exit:
 	if (intel_dp->color_range_auto) {
 		/*
 		 * See:
@@ -1266,17 +1293,6 @@ found:
 	if (intel_dp->color_range)
 		pipe_config->limited_color_range = true;
 
-	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);
-
-	DRM_DEBUG_KMS("DP link bw %02x lane count %d clock %d bpp %d\n",
-		      intel_dp->link_bw, intel_dp->lane_count,
-		      pipe_config->port_clock, bpp);
-	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
-		      mode_rate, link_avail);
-
 	intel_link_compute_m_n(bpp, lane_count,
 			       adjusted_mode->crtc_clock,
 			       pipe_config->port_clock,
-- 
1.9.1

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

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

[Todd Previte]

Update the hot plug function to handle the SST case. Instead of placing
the SST case within the long/short pulse block, it is now handled after
determining that MST mode is not in use. This way, the topology management
layer can handle any MST-related operations while SST operations are still
correctly handled afterwards.

This patch also corrects the problem of SST mode only being handled in the
case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes
both short and long pulses are used by the different tests, thus both cases
need to be addressed for SST.

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

V2:
- N/A
V3:
- Place the SST mode link status check into the mst_fail case
- Remove obsolete comment regarding SST mode operation
- Removed an erroneous line of code that snuck in during rebasing

Signed-off-by: Todd Previte <tprevite@gmail.com>

Conflicts:
	drivers/gpu/drm/i915/intel_dp.c
---
 drivers/gpu/drm/i915/intel_dp.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2d83f13..041df81 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4653,16 +4653,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
 				goto mst_fail;
 		}
-
-		if (!intel_dp->is_mst) {
-			/*
-			 * we'll check the link status via the normal hot plug path later -
-			 * but for short hpds we should check it now
-			 */
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-		}
 	}
 
 	ret = IRQ_HANDLED;
@@ -4674,6 +4664,12 @@ mst_fail:
 		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
 		intel_dp->is_mst = false;
 		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
+	} else {
+		/* SST mode - handle short/long pulses here */
+		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+		intel_dp_check_link_status(intel_dp);
+		drm_modeset_unlock(&dev->mode_config.connection_mutex);
+		ret = IRQ_HANDLED;
 	}
 put_power:
 	intel_display_power_put(dev_priv, power_domain);
-- 
1.9.1

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

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

[Todd Previte]

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

V2:
- N/A
V3:
- Removed the reference to dp_connector_is_valid()
- Updated the show() function to match config_ctl_show()
- Updated the enums to match the format of the existing enums
  for the config parameters

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b77574e..45c0fde 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -68,6 +68,15 @@ enum dp_config_param {
 	DP_CONFIG_PARAM_INVALID = -1
 };
 
+enum dp_ctrl_param {
+	DP_CTRL_PARAM_CONNECTOR = 0,
+	DP_CTRL_PARAM_TEST_DATA,
+	DP_CTRL_PARAM_TEST_ACTIVE,
+	DP_CTRL_PARAM_TEST_RESPONSE,
+	DP_CTRL_PARAM_COUNT,
+	DP_CTRL_PARAM_INVALID = -1
+};
+
 struct dp_config {
 	enum dp_config_param type;
 	unsigned long value;
@@ -4171,6 +4180,63 @@ static const struct file_operations i915_displayport_config_ctl_fops = {
 	.write = i915_displayport_config_ctl_write
 };
 
+static int i915_displayport_test_ctl_show(struct seq_file *m, void *data)
+{
+	struct drm_device *dev = m->private;
+	struct drm_connector *connector;
+	struct list_head *connector_list = &dev->mode_config.connector_list;
+	struct intel_dp *intel_dp;
+
+	if (!dev)
+		return -ENODEV;
+
+	list_for_each_entry(connector, connector_list, head) {
+
+		if (connector->connector_type !=
+		    DRM_MODE_CONNECTOR_DisplayPort)
+			continue;
+
+		if (connector->status == connector_status_connected) {
+			intel_dp = enc_to_intel_dp(connector->encoder);
+			/* Compliance test data informs userspace about current
+			   request
+			*/
+			seq_printf(m, "Test Data  : %08lx\n",
+				   intel_dp->compliance_test_data);
+			seq_printf(m, "Test Active: %02x\n",
+				   intel_dp->compliance_testing_active);
+			seq_printf(m, "Test Response: %02lx\n",
+				   intel_dp->compliance_test_response);
+		} else {
+			dp_print_string(m,
+					DP_CONFIG_PARAM_CONNECTOR,
+					connector->name);
+			dp_print_string(m,
+					DP_CONFIG_PARAM_CONNECTOR_ID,
+					&connector->base.id);
+			seq_puts(m, "disconnected\n");
+		}
+	}
+
+	return 0;
+}
+
+static int i915_displayport_test_ctl_open(struct inode *inode,
+				  struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+
+	return single_open(file, i915_displayport_test_ctl_show, dev);
+}
+
+static const struct file_operations i915_dp_test_ctl_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_displayport_test_ctl_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
 static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
 {
 	struct drm_device *dev = m->private;
@@ -4920,7 +4986,8 @@ static const struct i915_debugfs_files {
 	{"i915_spr_wm_latency", &i915_spr_wm_latency_fops},
 	{"i915_cur_wm_latency", &i915_cur_wm_latency_fops},
 	{"i915_fbc_false_color", &i915_fbc_fc_fops},
-	{"i915_displayport_config_ctl", &i915_displayport_config_ctl_fops}
+	{"i915_displayport_config_ctl", &i915_displayport_config_ctl_fops},
+	{"i915_dp_test_ctl", &i915_dp_test_ctl_fops}
 };
 
 void intel_display_crc_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c813d3c..6af5880 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -663,6 +663,7 @@ struct intel_dp {
 	/* Displayport compliance testing */
 	unsigned long compliance_test_data;
 	bool compliance_testing_active;
+	unsigned long compliance_test_response;
 	struct intel_dp_link_config compliance_config;
 };
 
-- 
1.9.1

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

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

[Todd Previte]

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

V2:
- N/A
V3:
- Removed use of dp_connector_is_valid()
- Replaced with DRM connector check to match other functions
- Updated the dp_parse_test_ctl function to use the new enums
  instead of #defines

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 45c0fde..e180813 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4073,6 +4073,46 @@ static int dp_parse_config(char *input_buffer,
 	return status;
 }
 
+static int dp_parse_test_ctl(char *input_buffer,
+			     ssize_t buffer_size,
+			     struct intel_dp *intel_dp)
+{
+	char *ctrl_lines[DP_CTRL_PARAM_COUNT];
+	int line_count;
+	int test_data;
+	int active;
+	int response;
+	char *conn_name; /* Connector name in the file */
+	char *dp_name;	 /* Connector name in the intel_dp struct */
+
+	if (!input_buffer)
+		return -EIO;
+
+	line_count = dp_tokenize_config(input_buffer, ctrl_lines);
+	if (line_count != DP_CTRL_PARAM_COUNT)
+		return -EIO;
+
+	conn_name = ctrl_lines[DP_CTRL_PARAM_CONNECTOR];
+	dp_name = intel_dp->attached_connector->base.name;
+
+	if (strncmp(conn_name, dp_name, strlen(dp_name)) == 0) {
+		kstrtol(ctrl_lines[DP_CTRL_PARAM_TEST_DATA],
+			16,
+			(long *)&test_data);
+		kstrtol(ctrl_lines[DP_CTRL_PARAM_TEST_ACTIVE],
+			16,
+			(long *)&active);
+		kstrtol(ctrl_lines[DP_CTRL_PARAM_TEST_RESPONSE],
+			16,
+			(long *)&response);
+	} else {
+		DRM_DEBUG_DRIVER("Connector names don't match\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static int i915_displayport_config_ctl_show(struct seq_file *m, void *data)
 {
 	struct drm_device *dev = m->private;
@@ -4180,6 +4220,70 @@ static const struct file_operations i915_displayport_config_ctl_fops = {
 	.write = i915_displayport_config_ctl_write
 };
 
+static ssize_t i915_displayport_test_ctl_write(struct file *file,
+					    const char __user *ubuf,
+					    size_t len, loff_t *offp)
+{
+	char *input_buffer;
+	int status = 0;
+	struct seq_file *m;
+	struct drm_device *dev;
+	struct drm_connector *connector;
+	struct list_head *connector_list;
+	struct intel_dp *intel_dp;
+
+	m = file->private_data;
+	if (!m) {
+		status = -ENODEV;
+		return status;
+	}
+	dev = m->private;
+
+	if (!dev) {
+		status = -ENODEV;
+		return status;
+	}
+	connector_list = &dev->mode_config.connector_list;
+
+	if (len == 0)
+		return 0;
+
+	input_buffer = kmalloc(len + 1, GFP_KERNEL);
+	if (!input_buffer)
+		return -ENOMEM;
+
+	if (copy_from_user(input_buffer, ubuf, len)) {
+		status = -EFAULT;
+		goto out;
+	}
+
+	input_buffer[len] = '\0';
+	DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
+
+	list_for_each_entry(connector, connector_list, head) {
+
+		if (connector->connector_type !=
+		    DRM_MODE_CONNECTOR_DisplayPort)
+			continue;
+
+		if (connector->connector_type ==
+		    DRM_MODE_CONNECTOR_DisplayPort &&
+		    connector->status == connector_status_connected) {
+			intel_dp = enc_to_intel_dp(connector->encoder);
+			status = dp_parse_test_ctl(input_buffer, len, intel_dp);
+			if (status < 0)
+				goto out;
+		}
+	}
+out:
+	kfree(input_buffer);
+	if (status < 0)
+		return status;
+
+	*offp += len;
+	return len;
+}
+
 static int i915_displayport_test_ctl_show(struct seq_file *m, void *data)
 {
 	struct drm_device *dev = m->private;
@@ -4232,6 +4336,7 @@ static int i915_displayport_test_ctl_open(struct inode *inode,
 static const struct file_operations i915_dp_test_ctl_fops = {
 	.owner = THIS_MODULE,
 	.open = i915_displayport_test_ctl_open,
+	.write = i915_displayport_test_ctl_write,
 	.read = seq_read,
 	.llseek = seq_lseek,
 	.release = single_release,
-- 
1.9.1

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

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

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5792
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  277/277              277/277
ILK                                  313/313              313/313
SNB                                  309/309              309/309
IVB                                  382/382              382/382
BYT                                  296/296              296/296
HSW                                  425/425              425/425
BDW                 -1              318/318              317/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BDW  igt_gem_gtt_hog      PASS(10)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/9 V4] 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 operations
are handed off to userspace so that the userspace app can set the display
mode appropriately for the test response.

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

V2:
- Addressed mailing list feedback
- Removed excess debug messages
- Removed extraneous comments
- Fixed formatting issues (line length > 80)
- Updated the debug message in compute_edid_checksum to output hex values
  instead of decimal
V3:
- Addressed more list feedback
- Added the test_active flag to the autotest function
- Removed test_active flag from handler
- Added failsafe check on the compliance test active flag
  at the end of the test handler
- Fixed checkpatch.pl issues
V4:
- Removed the checksum computation function and its use as it has been
  rendered superfluous by changes to the core DRM EDID functions

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5d7b7f3..32f0fc5 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)
 
+/* 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;
@@ -3766,7 +3773,39 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 
 static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
 {
+	struct drm_connector *connector = &intel_dp->attached_connector->base;
+	struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+	struct edid *edid_read = NULL;
 	uint8_t test_result = DP_TEST_NAK;
+	uint32_t ret = 0;
+
+	intel_dp->aux.i2c_nack_count = 0;
+	intel_dp->aux.i2c_defer_count = 0;
+
+	edid_read = drm_get_edid(connector, adapter);
+
+	if (edid_read == NULL) {
+		/* Check for NACKs/DEFERs, use failsafe if detected
+		   (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
+		if (intel_dp->aux.i2c_nack_count > 0 ||
+			intel_dp->aux.i2c_defer_count > 0)
+			DRM_DEBUG_KMS("EDID read had %d NACKs, %d DEFERs\n",
+				      intel_dp->aux.i2c_nack_count,
+				      intel_dp->aux.i2c_defer_count);
+		intel_dp->compliance_test_data = INTEL_DP_EDID_CORRUPT |
+						 INTEL_DP_RESOLUTION_FAILSAFE;
+	} else {
+		ret = drm_dp_dpcd_write(&intel_dp->aux,
+					DP_TEST_EDID_CHECKSUM,
+					&edid_read->checksum, 1);
+		test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
+		intel_dp->compliance_test_data = INTEL_DP_EDID_OK | 
+						 INTEL_DP_RESOLUTION_PREFERRED;
+	}
+
+	/* Set test active flag here so userspace doesn't interrupt things */
+	intel_dp->compliance_testing_active = 1;
+
 	return test_result;
 }
 
@@ -3781,6 +3820,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 	uint8_t response = DP_TEST_NAK;
 	uint8_t rxdata = 0;
 	int status = 0;
+	int failsafe_counter = 0;
 
 	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
 	if (status <= 0) {
@@ -3816,6 +3856,18 @@ update_status:
 				   &response, 1);
 	if (status <= 0)
 		DRM_DEBUG_KMS("Could not write test response to sink\n");
+
+	/* Failsafe check on compliance testing flag */
+	while (intel_dp->compliance_testing_active) {
+		usleep_range(100, 150);
+		failsafe_counter++;
+		/*
+		   If userspace still hasn't responded after 20 iterations,
+		   chances are it's not going to, so clear the flag and move on
+		*/
+		if (failsafe_counter > 20)
+			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

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

[Todd Previte]

Update the hot plug function to handle the SST case. Instead of placing
the SST case within the long/short pulse block, it is now handled after
determining that MST mode is not in use. This way, the topology management
layer can handle any MST-related operations while SST operations are still
correctly handled afterwards.

This patch also corrects the problem of SST mode only being handled in the
case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes
both short and long pulses are used by the different tests, thus both cases
need to be addressed for SST.

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

For the SST case, the main stream is disabled for long HPD pulses as this
generally indicates either a connect/disconnect event or link failure. For
a number of case in compliance testing, the source is required to disable
the main link upon detection of a long HPD.

V2:
- N/A
V3:
- Place the SST mode link status check into the mst_fail case
- Remove obsolete comment regarding SST mode operation
- Removed an erroneous line of code that snuck in during rebasing
V4:
- Added a disable of the main stream (DP transport) for the long pulse case
  for SST to support compliance testing

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 080cc23..2460d14 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4618,16 +4618,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
 				goto mst_fail;
 		}
-
-		if (!intel_dp->is_mst) {
-			/*
-			 * we'll check the link status via the normal hot plug path later -
-			 * but for short hpds we should check it now
-			 */
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-		}
 	}
 
 	ret = IRQ_HANDLED;
@@ -4639,6 +4629,21 @@ mst_fail:
 		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
 		intel_dp->is_mst = false;
 		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
+	} else {
+		/* SST mode - handle short/long pulses here */
+		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+		/* Clear compliance testing flags/data here to prevent
+		 * false detection in userspace */
+		intel_dp->compliance_test_data = 0;
+		intel_dp->compliance_testing_active = 0;
+		/* For a long pulse in SST mode, disable the main link */
+		if (long_hpd) {
+			I915_WRITE(DP_TP_CTL(intel_dig_port->port),
+					      ~DP_TP_CTL_ENABLE);
+		}
+		intel_dp_check_link_status(intel_dp);
+		drm_modeset_unlock(&dev->mode_config.connection_mutex);
+		ret = IRQ_HANDLED;
 	}
 put_power:
 	intel_display_power_put(dev_priv, power_domain);
-- 
1.9.1

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

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

[Daniel Vetter]

On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote:
> Update the hot plug function to handle the SST case. Instead of placing
> the SST case within the long/short pulse block, it is now handled after
> determining that MST mode is not in use. This way, the topology management
> layer can handle any MST-related operations while SST operations are still
> correctly handled afterwards.
> 
> This patch also corrects the problem of SST mode only being handled in the
> case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes
> both short and long pulses are used by the different tests, thus both cases
> need to be addressed for SST.
> 
> This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
> previous compliance testing patch sequence. Review feedback on that patch
> indicated that updating intel_dp_hot_plug() was not the correct place for
> the test handler.
> 
> For the SST case, the main stream is disabled for long HPD pulses as this
> generally indicates either a connect/disconnect event or link failure. For
> a number of case in compliance testing, the source is required to disable
> the main link upon detection of a long HPD.
> 
> V2:
> - N/A
> V3:
> - Place the SST mode link status check into the mst_fail case
> - Remove obsolete comment regarding SST mode operation
> - Removed an erroneous line of code that snuck in during rebasing
> V4:
> - Added a disable of the main stream (DP transport) for the long pulse case
>   for SST to support compliance testing
> 
> Signed-off-by: Todd PRevite <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 080cc23..2460d14 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4618,16 +4618,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
>  				goto mst_fail;
>  		}
> -
> -		if (!intel_dp->is_mst) {
> -			/*
> -			 * we'll check the link status via the normal hot plug path later -
> -			 * but for short hpds we should check it now
> -			 */
> -			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -			intel_dp_check_link_status(intel_dp);
> -			drm_modeset_unlock(&dev->mode_config.connection_mutex);
> -		}
>  	}
>  
>  	ret = IRQ_HANDLED;
> @@ -4639,6 +4629,21 @@ mst_fail:
>  		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
>  		intel_dp->is_mst = false;
>  		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> +	} else {
> +		/* SST mode - handle short/long pulses here */
> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +		/* Clear compliance testing flags/data here to prevent
> +		 * false detection in userspace */
> +		intel_dp->compliance_test_data = 0;
> +		intel_dp->compliance_testing_active = 0;
> +		/* For a long pulse in SST mode, disable the main link */
> +		if (long_hpd) {
> +			I915_WRITE(DP_TP_CTL(intel_dig_port->port),
> +					      ~DP_TP_CTL_ENABLE);
> +		}

Disabling the  main link should be done in userspace. All long pulse
requests should be forwarded to userspace as a hotplug event. Userspace
can then react to that hotplug appropriately. This way we can again
exercise the normal operation of all our dp code.

If otoh this is again a case where compliance requirements are so strict
that they don't allow any lee-way (I can't come up with a scenario really)
then I think this should be in a completely separate commit with the full
explanation in the commit message.
-Daniel

> +		intel_dp_check_link_status(intel_dp);
> +		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +		ret = IRQ_HANDLED;
>  	}
>  put_power:
>  	intel_display_power_put(dev_priv, power_domain);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Jesse Barnes]

On 03/06/2015 08:34 AM, Daniel Vetter wrote:
> On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote:
>> Update the hot plug function to handle the SST case. Instead of placing
>> the SST case within the long/short pulse block, it is now handled after
>> determining that MST mode is not in use. This way, the topology management
>> layer can handle any MST-related operations while SST operations are still
>> correctly handled afterwards.
>>
>> This patch also corrects the problem of SST mode only being handled in the
>> case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes
>> both short and long pulses are used by the different tests, thus both cases
>> need to be addressed for SST.
>>
>> This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
>> previous compliance testing patch sequence. Review feedback on that patch
>> indicated that updating intel_dp_hot_plug() was not the correct place for
>> the test handler.
>>
>> For the SST case, the main stream is disabled for long HPD pulses as this
>> generally indicates either a connect/disconnect event or link failure. For
>> a number of case in compliance testing, the source is required to disable
>> the main link upon detection of a long HPD.
>>
>> V2:
>> - N/A
>> V3:
>> - Place the SST mode link status check into the mst_fail case
>> - Remove obsolete comment regarding SST mode operation
>> - Removed an erroneous line of code that snuck in during rebasing
>> V4:
>> - Added a disable of the main stream (DP transport) for the long pulse case
>>   for SST to support compliance testing
>>
>> Signed-off-by: Todd PRevite <tprevite@gmail.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 25 +++++++++++++++----------
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 080cc23..2460d14 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4618,16 +4618,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>  			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
>>  				goto mst_fail;
>>  		}
>> -
>> -		if (!intel_dp->is_mst) {
>> -			/*
>> -			 * we'll check the link status via the normal hot plug path later -
>> -			 * but for short hpds we should check it now
>> -			 */
>> -			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> -			intel_dp_check_link_status(intel_dp);
>> -			drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> -		}
>>  	}
>>  
>>  	ret = IRQ_HANDLED;
>> @@ -4639,6 +4629,21 @@ mst_fail:
>>  		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
>>  		intel_dp->is_mst = false;
>>  		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
>> +	} else {
>> +		/* SST mode - handle short/long pulses here */
>> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> +		/* Clear compliance testing flags/data here to prevent
>> +		 * false detection in userspace */
>> +		intel_dp->compliance_test_data = 0;
>> +		intel_dp->compliance_testing_active = 0;
>> +		/* For a long pulse in SST mode, disable the main link */
>> +		if (long_hpd) {
>> +			I915_WRITE(DP_TP_CTL(intel_dig_port->port),
>> +					      ~DP_TP_CTL_ENABLE);
>> +		}
> 
> Disabling the  main link should be done in userspace. All long pulse
> requests should be forwarded to userspace as a hotplug event. Userspace
> can then react to that hotplug appropriately. This way we can again
> exercise the normal operation of all our dp code.

What's your concern here?  Do you want to make sure we get coverage on
dp_link_down()?  It looks like that might be safe to use here instead of
flipping the disable bit directly.  Or did you want to go through the
whole pipe/port shutdown sequence as well?  If so, I think the dpms
tests will already cover that, separate from simple compliance.

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

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

[Daniel Vetter]

On Mon, Mar 09, 2015 at 08:34:49AM -0700, Jesse Barnes wrote:
> On 03/06/2015 08:34 AM, Daniel Vetter wrote:
> > On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote:
> >> Update the hot plug function to handle the SST case. Instead of placing
> >> the SST case within the long/short pulse block, it is now handled after
> >> determining that MST mode is not in use. This way, the topology management
> >> layer can handle any MST-related operations while SST operations are still
> >> correctly handled afterwards.
> >>
> >> This patch also corrects the problem of SST mode only being handled in the
> >> case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes
> >> both short and long pulses are used by the different tests, thus both cases
> >> need to be addressed for SST.
> >>
> >> This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
> >> previous compliance testing patch sequence. Review feedback on that patch
> >> indicated that updating intel_dp_hot_plug() was not the correct place for
> >> the test handler.
> >>
> >> For the SST case, the main stream is disabled for long HPD pulses as this
> >> generally indicates either a connect/disconnect event or link failure. For
> >> a number of case in compliance testing, the source is required to disable
> >> the main link upon detection of a long HPD.
> >>
> >> V2:
> >> - N/A
> >> V3:
> >> - Place the SST mode link status check into the mst_fail case
> >> - Remove obsolete comment regarding SST mode operation
> >> - Removed an erroneous line of code that snuck in during rebasing
> >> V4:
> >> - Added a disable of the main stream (DP transport) for the long pulse case
> >>   for SST to support compliance testing
> >>
> >> Signed-off-by: Todd PRevite <tprevite@gmail.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c | 25 +++++++++++++++----------
> >>  1 file changed, 15 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index 080cc23..2460d14 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -4618,16 +4618,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> >>  			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
> >>  				goto mst_fail;
> >>  		}
> >> -
> >> -		if (!intel_dp->is_mst) {
> >> -			/*
> >> -			 * we'll check the link status via the normal hot plug path later -
> >> -			 * but for short hpds we should check it now
> >> -			 */
> >> -			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >> -			intel_dp_check_link_status(intel_dp);
> >> -			drm_modeset_unlock(&dev->mode_config.connection_mutex);
> >> -		}
> >>  	}
> >>  
> >>  	ret = IRQ_HANDLED;
> >> @@ -4639,6 +4629,21 @@ mst_fail:
> >>  		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> >>  		intel_dp->is_mst = false;
> >>  		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> >> +	} else {
> >> +		/* SST mode - handle short/long pulses here */
> >> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >> +		/* Clear compliance testing flags/data here to prevent
> >> +		 * false detection in userspace */
> >> +		intel_dp->compliance_test_data = 0;
> >> +		intel_dp->compliance_testing_active = 0;
> >> +		/* For a long pulse in SST mode, disable the main link */
> >> +		if (long_hpd) {
> >> +			I915_WRITE(DP_TP_CTL(intel_dig_port->port),
> >> +					      ~DP_TP_CTL_ENABLE);
> >> +		}
> > 
> > Disabling the  main link should be done in userspace. All long pulse
> > requests should be forwarded to userspace as a hotplug event. Userspace
> > can then react to that hotplug appropriately. This way we can again
> > exercise the normal operation of all our dp code.
> 
> What's your concern here?  Do you want to make sure we get coverage on
> dp_link_down()?  It looks like that might be safe to use here instead of
> flipping the disable bit directly.  Or did you want to go through the
> whole pipe/port shutdown sequence as well?  If so, I think the dpms
> tests will already cover that, separate from simple compliance.

This is likely to upset the state checker, we've already had some fun with
killing the hard dp pipe disable that the hdp code occasionally did. Well,
still have. The other reason is that dp compliance testing with
special-case code is somewhat pointless, except when the compliance test
contracts what real-world experience forces us to do. For these exceptions
I'd like that we fully understand them and also document them. Disabling
the link on a full hot-unplug is something we can (and most DE actually
do) do.
-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 4/9] drm/i915: Add debugfs functions for Displayport compliance testing

[Jani Nikula]

On Thu, 19 Feb 2015, Todd Previte <tprevite@gmail.com> wrote:
> This patch is the amalgamation of 7 patches from the V2 series. These
> patches all involve the implementation of the debugfs mechanism for
> handling Displayport compliance testing. The following are the commit
> messages from those 7 patches (included here to try and preserve the
> patch set history):

I think there should be per connector debugfs files for this, instead of
doing it for all DP connectors.

See my dpcd debugfs patch [1] for an example. I wish that got reviewed
and merged...

BR,
Jani.


[1] http://mid.gmane.org/1424867645-21608-1-git-send-email-jani.nikula@intel.com





>
> -------------------------------------------------------------------------------
> [PATCH 04/17] drm/i915: Add debugfs information for Displayport compliance
>               testing
>
> Originally, this patch was part of "[PATCH 05/10] drm/i915: Add debugfs
> interface for Displayport debug and compliance testing".  It contained
> definitions/declarations for some of the constants and data structures
> added to support debugfs output for Displayport compliance
> testing.
> -------------------------------------------------------------------------------
> [PATCH 05/17] drm/i915: Add file ops for Displayport configuration in debugfs
>
> This patch was also part of "[PATCH 05/10] drm/i915: Add debugfs interface
> for Displayport debug and compliance testing". This one added file operations
> structures for Displayport configuration and the declarations for open() and
> write() in the file ops structure.
> -------------------------------------------------------------------------------
> [PATCH 06/17] drm/i915: Add support functions in debugfs for handling
>               Displayport compliance configuration
>
> This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs interface
> for Displayport debug  and compliance testing". It added two support functions
> for handling Displayport configuration parameters that are used for compliance
> testing.
> -------------------------------------------------------------------------------
> [PATCH 07/17] drm/i915: Add and implement the debugfs 'show' functions for
>               Displayport compliance
>
> This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs
> interface for Displayport debug and compliance testing". This patch implemented
> the show() function.
> -------------------------------------------------------------------------------
> [PATCH 08/17] drm/i915: Add Displayport link configuration structure
>
> This patch was previously part of "[PATCH 07/10] drm/i915: Add structures for
> Displayport compliance testing parameters". It added a struct to maintain link
> configuration data. These were meant purely for Displayport compliance use.
> -------------------------------------------------------------------------------
> [PATCH 09/17] drm/i915: Add config parsing utilities in debugfs for Displayport
>               compliance
>
> This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs
> interface for Displayport debug and compliance testing". This patch adds two
> functions to handle parsing of Displayport configuration information as it
> formatted in the debugfs file. It is used to process incoming configuration
> changes from the userspace compliance application during testing.
>
> -------------------------------------------------------------------------------
> [PATCH 10/17] drm/i915: Implement the 'open' and 'write' debugfs functions for
>               Displayport compliance
>
> This patch is a combination of sections out of the following two previous
> patches:
> [PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and
> compliance testing
> [PATCH 07/10] drm/i915: Add structures for Displayport compliance testing
> parameters
>
> This patch implements the debugfs functions for 'open' and 'write' as they
> are specified in the file ops structure. The 'open' function outputs
> Displayport data to the appropriate debugfs file while the 'write' function
> handles parsing of user data written to that same file.
> -------------------------------------------------------------------------------
>
> The nature and scope of the contents of this patch make breaking it into
> smaller chunks difficult. With everthing contained in a single patch, the
> code is easier to review and understand.
>
> That said, this monster of a patch does quite a few things. First, it adds
> some necessary data structures for performing compliance testing operations
> on the kernel side. Specifically, it adds link configuration parameters and
> string tokens for the debugfs files. There are multiple utility functions
> added here as well in support of the debugfs system for handling compliance
> testing. Functions for printing strings to the files with the correct format,
> parsing tokens, tokenizing strings and doing the grunt work for writing the
> data to the debugfs files.
>
> This patch also adds the basic debugfs structures to build the file for the
> link configuration output as well as the file operations structures to handle
> the open and show functions.
>
> V2:
> - N/A
> V3:
> - Merged patches 4-10 into a single patch per review feedback
> - Added previous commit messages from the other commits integrated
>   here to preserve the integrity of the patch sequence
> - Adjusted the config_ctl_write function to only check the drm_connector
>   struct for a) type == Displayport and b) status == active. This is
>   to eliminate patches 14 and 15 from V2, which become unnecessary
>   in light of this change.
> - Fixed checkpatch.pl errors
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 471 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h    |  13 +
>  2 files changed, 484 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index b315f01..b77574e 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,121 @@ static const char *yesno(int v)
>  	return v ? "yes" : "no";
>  }
>  
> +#define MAX_DP_CONFIG_LINE_COUNT	64
> +
> +enum dp_config_param {
> +	DP_CONFIG_PARAM_CONNECTOR = 0,
> +	DP_CONFIG_PARAM_CONNECTOR_ID,
> +	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,
> +	DP_PARAMETER_COUNT,
> +	DP_CONFIG_PARAM_INVALID = -1
> +};
> +
> +struct dp_config {
> +	enum dp_config_param type;
> +	unsigned long value;
> +};
> +
> +static char *dp_get_config_str(int for_token)
> +{
> +	char *s;
> +
> +	switch (for_token) {
> +	case DP_CONFIG_PARAM_CONNECTOR:
> +		s = "Connector";
> +		break;
> +	case DP_CONFIG_PARAM_CONNECTOR_ID:
> +		s = "Connector ID";
> +		break;
> +	case DP_CONFIG_PARAM_LINK_RATE:
> +		s = "Link Rate";
> +		break;
> +	case DP_CONFIG_PARAM_LANE_COUNT:
> +		s = "Lane Count";
> +		break;
> +	case DP_CONFIG_PARAM_VOLTAGE_SWING:
> +		s = "Voltage Swing Level";
> +		break;
> +	case DP_CONFIG_PARAM_PREEMPHASIS:
> +		s = "Preemphasis Level";
> +		break;
> +	case DP_CONFIG_PARAM_HRES:
> +		s = "Horizontal Resolution";
> +		break;
> +	case DP_CONFIG_PARAM_VRES:
> +		s = "Vertical Resolution";
> +		break;
> +	case DP_CONFIG_PARAM_BPP:
> +		s = "Bits Per Pixel";
> +		break;
> +	default:
> +		s = "";
> +		break;
> +	}
> +
> +	return s;
> +}
> +
> +static void dp_print_string(struct seq_file *m,
> +			    enum dp_config_param for_param,
> +			    void *data)
> +{
> +	char *format_string;
> +	unsigned long output_value;
> +
> +	switch (for_param) {
> +	case DP_CONFIG_PARAM_CONNECTOR:
> +		/* Special case for the string parameter */
> +		format_string = "Connector:             %s\n";
> +		seq_printf(m, format_string, (char *) data);
> +		break;
> +	case DP_CONFIG_PARAM_CONNECTOR_ID:
> +		format_string = "Connector ID:          %u\n";
> +		output_value = *(unsigned int *) data;
> +		break;
> +	case DP_CONFIG_PARAM_LINK_RATE:
> +		format_string = "Link Rate:             %02x\n";
> +		output_value = *(unsigned char *) data;
> +		break;
> +	case DP_CONFIG_PARAM_LANE_COUNT:
> +		format_string = "Lane Count:            %u\n";
> +		output_value = *(unsigned int *) data;
> +		break;
> +	case DP_CONFIG_PARAM_VOLTAGE_SWING:
> +		format_string = "Voltage Swing Level:   %u\n";
> +		output_value = *(unsigned char *) data;
> +		break;
> +	case DP_CONFIG_PARAM_PREEMPHASIS:
> +		format_string = "Preemphasis Level:     %u\n";
> +		output_value = *(unsigned char *) data;
> +		break;
> +	case DP_CONFIG_PARAM_HRES:
> +		format_string = "Horizontal Resolution: %d\n";
> +		output_value = *(int *) data;
> +		break;
> +	case DP_CONFIG_PARAM_VRES:
> +		format_string = "Vertical Resolution:   %d\n";
> +		output_value = *(int *) data;
> +		break;
> +	case DP_CONFIG_PARAM_BPP:
> +		format_string = "Bits Per Pixel:        %u\n";
> +		output_value = *(unsigned char *) data;
> +		break;
> +	default:
> +		format_string = NULL;
> +		break;
> +	}
> +
> +	if (format_string != NULL && for_param != DP_CONFIG_PARAM_CONNECTOR)
> +		seq_printf(m, format_string, output_value);
> +}
> +
>  /* 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
> @@ -3701,6 +3817,360 @@ static const struct file_operations i915_display_crc_ctl_fops = {
>  	.write = display_crc_ctl_write
>  };
>  
> +static void dp_show_config(struct seq_file *m,
> +			   struct drm_connector *connector)
> +{
> +	struct intel_dp *intel_dp;
> +	struct intel_crtc *crtc;
> +	struct intel_connector *intel_connector = to_intel_connector(connector);
> +	struct intel_encoder *intel_encoder = intel_connector->encoder;
> +	int pipe_bpp, hres, vres;
> +	uint8_t vs[4], pe[4];
> +	struct drm_display_mode *mode;
> +	int i = 0;
> +
> +	if (!intel_encoder)
> +		return;
> +
> +	if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT)
> +		return;
> +
> +	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) >>
> +			DP_TRAIN_PRE_EMPHASIS_SHIFT;
> +	}
> +
> +	/* Output mode info only if a valid CRTC exists */
> +	if (intel_encoder->base.crtc) {
> +		crtc = to_intel_crtc(intel_encoder->base.crtc);
> +		pipe_bpp = crtc->config->pipe_bpp;
> +		mode = &crtc->config->base.adjusted_mode;
> +		hres = mode->hdisplay;
> +		vres = mode->vdisplay;
> +	} else {
> +		pipe_bpp = 0;
> +		hres = vres = 0;
> +	}
> +
> +	dp_print_string(m,
> +			DP_CONFIG_PARAM_CONNECTOR,
> +			connector->name);
> +	dp_print_string(m,
> +			DP_CONFIG_PARAM_CONNECTOR_ID,
> +			&connector->base.id);
> +	dp_print_string(m,
> +			DP_CONFIG_PARAM_LINK_RATE,
> +			&intel_dp->link_bw);
> +	dp_print_string(m,
> +			DP_CONFIG_PARAM_LANE_COUNT,
> +			&intel_dp->lane_count);
> +	dp_print_string(m,
> +			DP_CONFIG_PARAM_VOLTAGE_SWING,
> +			&vs[0]);
> +	dp_print_string(m,
> +			DP_CONFIG_PARAM_PREEMPHASIS,
> +			&pe[0]);
> +	dp_print_string(m,
> +			DP_CONFIG_PARAM_HRES,
> +			&hres);
> +	dp_print_string(m,
> +			DP_CONFIG_PARAM_VRES,
> +			&vres);
> +	dp_print_string(m,
> +			DP_CONFIG_PARAM_BPP,
> +			&pipe_bpp);
> +}
> +
> +enum dp_config_param dp_get_param_type(char *input_string)
> +{
> +	enum dp_config_param param_type = DP_CONFIG_PARAM_INVALID;
> +	char *s;
> +	int i = 0;
> +
> +	for (i = 0; i < DP_PARAMETER_COUNT; i++) {
> +		s = dp_get_config_str(i);
> +		if (strncmp(input_string, s,
> +			    strlen(s)) == 0) {
> +			param_type = (enum dp_config_param) i;
> +			break;
> +		}
> +	}
> +	return param_type;
> +}
> +
> +int dp_get_param_value(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 dp_tokenize_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 dp_parse_config(char *input_buffer,
> +			   ssize_t buffer_size,
> +			   struct intel_dp *intel_dp)
> +{
> +	int status = 0;
> +	char *lines[MAX_DP_CONFIG_LINE_COUNT];
> +	int i = 0;
> +	struct dp_config parms[DP_PARAMETER_COUNT];
> +	int line_count = 0;
> +	char *buffer = input_buffer;
> +	enum dp_config_param parm_type;
> +	unsigned long parm_value;
> +
> +	line_count = dp_tokenize_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 = dp_get_param_type(lines[i]);
> +		if (parm_type != DP_CONFIG_PARAM_INVALID) {
> +			status = dp_get_param_value(parm_type,
> +							lines[i+1],
> +							&parm_value);
> +			if (status == 0) {
> +				parms[parm_type].type = parm_type;
> +				parms[parm_type].value = parm_value;
> +			}
> +		}
> +	}
> +
> +	if (parms[DP_CONFIG_PARAM_LINK_RATE].value == 0x06 ||
> +	    parms[DP_CONFIG_PARAM_LINK_RATE].value == 0x0a ||
> +	    parms[DP_CONFIG_PARAM_LINK_RATE].value == 0x14) {
> +		intel_dp->compliance_config.link_rate =
> +			parms[DP_CONFIG_PARAM_LINK_RATE].value;
> +	} else
> +		return -EINVAL;
> +
> +	if (parms[DP_CONFIG_PARAM_LANE_COUNT].value == 0x01 ||
> +	    parms[DP_CONFIG_PARAM_LANE_COUNT].value == 0x02 ||
> +	    parms[DP_CONFIG_PARAM_LANE_COUNT].value == 0x04) {
> +		intel_dp->compliance_config.lane_count =
> +			parms[DP_CONFIG_PARAM_LANE_COUNT].value;
> +	} else
> +		return -EINVAL;
> +
> +	if (parms[DP_CONFIG_PARAM_VOLTAGE_SWING].value <= 0x03 &&
> +	    parms[DP_CONFIG_PARAM_VOLTAGE_SWING].value >= 0x00) {
> +		intel_dp->compliance_config.vswing_level =
> +			parms[DP_CONFIG_PARAM_VOLTAGE_SWING].value;
> +	} else
> +		return -EINVAL;
> +
> +	if (parms[DP_CONFIG_PARAM_PREEMPHASIS].value <= 0x03 &&
> +	    parms[DP_CONFIG_PARAM_PREEMPHASIS].value >= 0x00) {
> +		intel_dp->compliance_config.preemp_level =
> +			parms[DP_CONFIG_PARAM_PREEMPHASIS].value;
> +	} else
> +		return -EINVAL;
> +
> +	if (parms[DP_CONFIG_PARAM_BPP].value == 18 ||
> +	    parms[DP_CONFIG_PARAM_BPP].value == 24 ||
> +	    parms[DP_CONFIG_PARAM_BPP].value == 30 ||
> +	    parms[DP_CONFIG_PARAM_BPP].value == 36) {
> +		intel_dp->compliance_config.bits_per_pixel =
> +			parms[DP_CONFIG_PARAM_PREEMPHASIS].value;
> +	} else
> +		return -EINVAL;
> +
> +	if (parms[DP_CONFIG_PARAM_HRES].value > 0 &&
> +	    parms[DP_CONFIG_PARAM_HRES].value <= 8192) {
> +		intel_dp->compliance_config.hres =
> +			parms[DP_CONFIG_PARAM_HRES].value;
> +	} else
> +		return -EINVAL;
> +
> +	if (parms[DP_CONFIG_PARAM_VRES].value > 0 &&
> +	    parms[DP_CONFIG_PARAM_VRES].value <= 8192) {
> +		intel_dp->compliance_config.vres =
> +			parms[DP_CONFIG_PARAM_VRES].value;
> +	} else
> +		return -EINVAL;
> +
> +	return status;
> +}
> +
> +static int i915_displayport_config_ctl_show(struct seq_file *m, void *data)
> +{
> +	struct drm_device *dev = m->private;
> +	struct drm_connector *connector;
> +	struct list_head *connector_list = &dev->mode_config.connector_list;
> +
> +	if (!dev)
> +		return -ENODEV;
> +
> +	list_for_each_entry(connector, connector_list, head) {
> +
> +		if (connector->connector_type !=
> +		    DRM_MODE_CONNECTOR_DisplayPort)
> +			continue;
> +
> +		if (connector->status == connector_status_connected) {
> +			dp_show_config(m, connector);
> +		} else {
> +			dp_print_string(m,
> +					DP_CONFIG_PARAM_CONNECTOR,
> +					connector->name);
> +			dp_print_string(m,
> +					DP_CONFIG_PARAM_CONNECTOR_ID,
> +					&connector->base.id);
> +			seq_puts(m, "disconnected\n");
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int i915_displayport_config_ctl_open(struct inode *inode,
> +				       struct file *file)
> +{
> +	struct drm_device *dev = inode->i_private;
> +
> +	return single_open(file, i915_displayport_config_ctl_show, dev);
> +}
> +
> +static ssize_t i915_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 list_head *connector_list;
> +	struct intel_dp *intel_dp;
> +
> +	m = file->private_data;
> +	if (!m) {
> +		status = -ENODEV;
> +		return status;
> +	}
> +	dev = m->private;
> +
> +	if (!dev) {
> +		status = -ENODEV;
> +		return status;
> +	}
> +	connector_list = &dev->mode_config.connector_list;
> +
> +	if (len == 0)
> +		return 0;
> +
> +	input_buffer = kmalloc(len + 1, GFP_KERNEL);
> +	if (!input_buffer)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(input_buffer, ubuf, len)) {
> +		status = -EFAULT;
> +		goto out;
> +	}
> +
> +	input_buffer[len] = '\0';
> +	DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
> +
> +	list_for_each_entry(connector, connector_list, head) {
> +		if (connector->connector_type ==
> +		    DRM_MODE_CONNECTOR_DisplayPort &&
> +		    connector->status == connector_status_connected) {
> +			intel_dp = enc_to_intel_dp(connector->encoder);
> +			status = dp_parse_config(input_buffer, len, intel_dp);
> +			if (status < 0)
> +				goto out;
> +		}
> +	}
> +out:
> +	kfree(input_buffer);
> +	if (status < 0)
> +		return status;
> +
> +	*offp += len;
> +	return len;
> +}
> +
> +static const struct file_operations i915_displayport_config_ctl_fops = {
> +	.owner = THIS_MODULE,
> +	.open = i915_displayport_config_ctl_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = single_release,
> +	.write = i915_displayport_config_ctl_write
> +};
> +
>  static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
>  {
>  	struct drm_device *dev = m->private;
> @@ -4450,6 +4920,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_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 74b30c1..c813d3c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -592,6 +592,18 @@ struct intel_hdmi {
>  struct intel_dp_mst_encoder;
>  #define DP_MAX_DOWNSTREAM_PORTS		0x10
>  
> +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;
> @@ -651,6 +663,7 @@ struct intel_dp {
>  	/* Displayport compliance testing */
>  	unsigned long compliance_test_data;
>  	bool compliance_testing_active;
> +	struct intel_dp_link_config compliance_config;
>  };
>  
>  struct intel_digital_port {
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Jesse Barnes]

On 03/09/2015 10:29 AM, Daniel Vetter wrote:
> On Mon, Mar 09, 2015 at 08:34:49AM -0700, Jesse Barnes wrote:
>> On 03/06/2015 08:34 AM, Daniel Vetter wrote:
>>> On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote:
>>>> +	} else {
>>>> +		/* SST mode - handle short/long pulses here */
>>>> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>>> +		/* Clear compliance testing flags/data here to prevent
>>>> +		 * false detection in userspace */
>>>> +		intel_dp->compliance_test_data = 0;
>>>> +		intel_dp->compliance_testing_active = 0;
>>>> +		/* For a long pulse in SST mode, disable the main link */
>>>> +		if (long_hpd) {
>>>> +			I915_WRITE(DP_TP_CTL(intel_dig_port->port),
>>>> +					      ~DP_TP_CTL_ENABLE);
>>>> +		}
>>>
>>> Disabling the  main link should be done in userspace. All long pulse
>>> requests should be forwarded to userspace as a hotplug event. Userspace
>>> can then react to that hotplug appropriately. This way we can again
>>> exercise the normal operation of all our dp code.
>>
>> What's your concern here?  Do you want to make sure we get coverage on
>> dp_link_down()?  It looks like that might be safe to use here instead of
>> flipping the disable bit directly.  Or did you want to go through the
>> whole pipe/port shutdown sequence as well?  If so, I think the dpms
>> tests will already cover that, separate from simple compliance.
> 
> This is likely to upset the state checker, we've already had some fun with
> killing the hard dp pipe disable that the hdp code occasionally did. Well,
> still have. The other reason is that dp compliance testing with
> special-case code is somewhat pointless, except when the compliance test
> contracts what real-world experience forces us to do. For these exceptions
> I'd like that we fully understand them and also document them. Disabling
> the link on a full hot-unplug is something we can (and most DE actually
> do) do.

If we end up hitting the checker while testing, then yeah it would spew.

But I thought this was mainly about testing the DP code, making sure we
can up/down links, train at different parameters, etc, not about going
through full mode sets all the time...

But either way, I agree we should be documenting this behavior so we
don't get stuck trying to figure it out later.

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

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

[Ville Syrjälä]

On Mon, Mar 09, 2015 at 12:07:31PM -0700, Jesse Barnes wrote:
> On 03/09/2015 10:29 AM, Daniel Vetter wrote:
> > On Mon, Mar 09, 2015 at 08:34:49AM -0700, Jesse Barnes wrote:
> >> On 03/06/2015 08:34 AM, Daniel Vetter wrote:
> >>> On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote:
> >>>> +	} else {
> >>>> +		/* SST mode - handle short/long pulses here */
> >>>> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >>>> +		/* Clear compliance testing flags/data here to prevent
> >>>> +		 * false detection in userspace */
> >>>> +		intel_dp->compliance_test_data = 0;
> >>>> +		intel_dp->compliance_testing_active = 0;
> >>>> +		/* For a long pulse in SST mode, disable the main link */
> >>>> +		if (long_hpd) {
> >>>> +			I915_WRITE(DP_TP_CTL(intel_dig_port->port),
> >>>> +					      ~DP_TP_CTL_ENABLE);
> >>>> +		}
> >>>
> >>> Disabling the  main link should be done in userspace. All long pulse
> >>> requests should be forwarded to userspace as a hotplug event. Userspace
> >>> can then react to that hotplug appropriately. This way we can again
> >>> exercise the normal operation of all our dp code.
> >>
> >> What's your concern here?  Do you want to make sure we get coverage on
> >> dp_link_down()?  It looks like that might be safe to use here instead of
> >> flipping the disable bit directly.  Or did you want to go through the
> >> whole pipe/port shutdown sequence as well?  If so, I think the dpms
> >> tests will already cover that, separate from simple compliance.
> > 
> > This is likely to upset the state checker, we've already had some fun with
> > killing the hard dp pipe disable that the hdp code occasionally did. Well,
> > still have. The other reason is that dp compliance testing with
> > special-case code is somewhat pointless, except when the compliance test
> > contracts what real-world experience forces us to do. For these exceptions
> > I'd like that we fully understand them and also document them. Disabling
> > the link on a full hot-unplug is something we can (and most DE actually
> > do) do.
> 
> If we end up hitting the checker while testing, then yeah it would spew.
> 
> But I thought this was mainly about testing the DP code, making sure we
> can up/down links, train at different parameters, etc, not about going
> through full mode sets all the time...
> 
> But either way, I agree we should be documenting this behavior so we
> don't get stuck trying to figure it out later.

I don't think we should be killing the port like this. It'll also kill
the pipe on some platforms and then we get all kinds of pipe stuck
warnings. So I think we'd definitely want a more graceful shutdown of
things.

I thought we actually discussed about going to the other direction, ie.
potentially allowing the link to brought up without the pipe and
enabling/disabling the pipe independently while the link remains up and
running?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drm/i915: Add debugfs functions for Displayport compliance testing

[Todd Previte]

On 3/9/2015 10:57 AM, Jani Nikula wrote:
> On Thu, 19 Feb 2015, Todd Previte <tprevite@gmail.com> wrote:
>> This patch is the amalgamation of 7 patches from the V2 series. These
>> patches all involve the implementation of the debugfs mechanism for
>> handling Displayport compliance testing. The following are the commit
>> messages from those 7 patches (included here to try and preserve the
>> patch set history):
> I think there should be per connector debugfs files for this, instead of
> doing it for all DP connectors.
>
> See my dpcd debugfs patch [1] for an example. I wish that got reviewed
> and merged...
>
> BR,
> Jani.

When it comes to compliance testing, you generally don't want anything 
else plugged in anyways. The user app is going to kill all the other 
displays, since it needs to be DRM master, so it's really kind of 
pointless. It also makes the implementation less complex. During 
development and testing, I've run the user app locally (logged in as 
root directly on the DUT) and over the network (ssh as root into the 
machine and launched the user app from the console) and the network 
method seems easier and more efficient to me. This is especially 
important as I can see the debug output from the user app right on the 
console instead of staring at a blank screen waiting for the test to 
complete.

That said, I think this is a reasonable request that can be addressed at 
a future time, once the base code is in place.  The architectural and 
design changes necessary for this would drag out the integration of 
basic compliance testing even longer and that's not something I believe 
we can afford to do right now.

I'll go look for your DPCD patch and get a review on there.

-T

>
> [1] http://mid.gmane.org/1424867645-21608-1-git-send-email-jani.nikula@intel.com
>
>
>
>
>
>> -------------------------------------------------------------------------------
>> [PATCH 04/17] drm/i915: Add debugfs information for Displayport compliance
>>                testing
>>
>> Originally, this patch was part of "[PATCH 05/10] drm/i915: Add debugfs
>> interface for Displayport debug and compliance testing".  It contained
>> definitions/declarations for some of the constants and data structures
>> added to support debugfs output for Displayport compliance
>> testing.
>> -------------------------------------------------------------------------------
>> [PATCH 05/17] drm/i915: Add file ops for Displayport configuration in debugfs
>>
>> This patch was also part of "[PATCH 05/10] drm/i915: Add debugfs interface
>> for Displayport debug and compliance testing". This one added file operations
>> structures for Displayport configuration and the declarations for open() and
>> write() in the file ops structure.
>> -------------------------------------------------------------------------------
>> [PATCH 06/17] drm/i915: Add support functions in debugfs for handling
>>                Displayport compliance configuration
>>
>> This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs interface
>> for Displayport debug  and compliance testing". It added two support functions
>> for handling Displayport configuration parameters that are used for compliance
>> testing.
>> -------------------------------------------------------------------------------
>> [PATCH 07/17] drm/i915: Add and implement the debugfs 'show' functions for
>>                Displayport compliance
>>
>> This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs
>> interface for Displayport debug and compliance testing". This patch implemented
>> the show() function.
>> -------------------------------------------------------------------------------
>> [PATCH 08/17] drm/i915: Add Displayport link configuration structure
>>
>> This patch was previously part of "[PATCH 07/10] drm/i915: Add structures for
>> Displayport compliance testing parameters". It added a struct to maintain link
>> configuration data. These were meant purely for Displayport compliance use.
>> -------------------------------------------------------------------------------
>> [PATCH 09/17] drm/i915: Add config parsing utilities in debugfs for Displayport
>>                compliance
>>
>> This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs
>> interface for Displayport debug and compliance testing". This patch adds two
>> functions to handle parsing of Displayport configuration information as it
>> formatted in the debugfs file. It is used to process incoming configuration
>> changes from the userspace compliance application during testing.
>>
>> -------------------------------------------------------------------------------
>> [PATCH 10/17] drm/i915: Implement the 'open' and 'write' debugfs functions for
>>                Displayport compliance
>>
>> This patch is a combination of sections out of the following two previous
>> patches:
>> [PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and
>> compliance testing
>> [PATCH 07/10] drm/i915: Add structures for Displayport compliance testing
>> parameters
>>
>> This patch implements the debugfs functions for 'open' and 'write' as they
>> are specified in the file ops structure. The 'open' function outputs
>> Displayport data to the appropriate debugfs file while the 'write' function
>> handles parsing of user data written to that same file.
>> -------------------------------------------------------------------------------
>>
>> The nature and scope of the contents of this patch make breaking it into
>> smaller chunks difficult. With everthing contained in a single patch, the
>> code is easier to review and understand.
>>
>> That said, this monster of a patch does quite a few things. First, it adds
>> some necessary data structures for performing compliance testing operations
>> on the kernel side. Specifically, it adds link configuration parameters and
>> string tokens for the debugfs files. There are multiple utility functions
>> added here as well in support of the debugfs system for handling compliance
>> testing. Functions for printing strings to the files with the correct format,
>> parsing tokens, tokenizing strings and doing the grunt work for writing the
>> data to the debugfs files.
>>
>> This patch also adds the basic debugfs structures to build the file for the
>> link configuration output as well as the file operations structures to handle
>> the open and show functions.
>>
>> V2:
>> - N/A
>> V3:
>> - Merged patches 4-10 into a single patch per review feedback
>> - Added previous commit messages from the other commits integrated
>>    here to preserve the integrity of the patch sequence
>> - Adjusted the config_ctl_write function to only check the drm_connector
>>    struct for a) type == Displayport and b) status == active. This is
>>    to eliminate patches 14 and 15 from V2, which become unnecessary
>>    in light of this change.
>> - Fixed checkpatch.pl errors
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c | 471 ++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h    |  13 +
>>   2 files changed, 484 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index b315f01..b77574e 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,121 @@ static const char *yesno(int v)
>>   	return v ? "yes" : "no";
>>   }
>>   
>> +#define MAX_DP_CONFIG_LINE_COUNT	64
>> +
>> +enum dp_config_param {
>> +	DP_CONFIG_PARAM_CONNECTOR = 0,
>> +	DP_CONFIG_PARAM_CONNECTOR_ID,
>> +	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,
>> +	DP_PARAMETER_COUNT,
>> +	DP_CONFIG_PARAM_INVALID = -1
>> +};
>> +
>> +struct dp_config {
>> +	enum dp_config_param type;
>> +	unsigned long value;
>> +};
>> +
>> +static char *dp_get_config_str(int for_token)
>> +{
>> +	char *s;
>> +
>> +	switch (for_token) {
>> +	case DP_CONFIG_PARAM_CONNECTOR:
>> +		s = "Connector";
>> +		break;
>> +	case DP_CONFIG_PARAM_CONNECTOR_ID:
>> +		s = "Connector ID";
>> +		break;
>> +	case DP_CONFIG_PARAM_LINK_RATE:
>> +		s = "Link Rate";
>> +		break;
>> +	case DP_CONFIG_PARAM_LANE_COUNT:
>> +		s = "Lane Count";
>> +		break;
>> +	case DP_CONFIG_PARAM_VOLTAGE_SWING:
>> +		s = "Voltage Swing Level";
>> +		break;
>> +	case DP_CONFIG_PARAM_PREEMPHASIS:
>> +		s = "Preemphasis Level";
>> +		break;
>> +	case DP_CONFIG_PARAM_HRES:
>> +		s = "Horizontal Resolution";
>> +		break;
>> +	case DP_CONFIG_PARAM_VRES:
>> +		s = "Vertical Resolution";
>> +		break;
>> +	case DP_CONFIG_PARAM_BPP:
>> +		s = "Bits Per Pixel";
>> +		break;
>> +	default:
>> +		s = "";
>> +		break;
>> +	}
>> +
>> +	return s;
>> +}
>> +
>> +static void dp_print_string(struct seq_file *m,
>> +			    enum dp_config_param for_param,
>> +			    void *data)
>> +{
>> +	char *format_string;
>> +	unsigned long output_value;
>> +
>> +	switch (for_param) {
>> +	case DP_CONFIG_PARAM_CONNECTOR:
>> +		/* Special case for the string parameter */
>> +		format_string = "Connector:             %s\n";
>> +		seq_printf(m, format_string, (char *) data);
>> +		break;
>> +	case DP_CONFIG_PARAM_CONNECTOR_ID:
>> +		format_string = "Connector ID:          %u\n";
>> +		output_value = *(unsigned int *) data;
>> +		break;
>> +	case DP_CONFIG_PARAM_LINK_RATE:
>> +		format_string = "Link Rate:             %02x\n";
>> +		output_value = *(unsigned char *) data;
>> +		break;
>> +	case DP_CONFIG_PARAM_LANE_COUNT:
>> +		format_string = "Lane Count:            %u\n";
>> +		output_value = *(unsigned int *) data;
>> +		break;
>> +	case DP_CONFIG_PARAM_VOLTAGE_SWING:
>> +		format_string = "Voltage Swing Level:   %u\n";
>> +		output_value = *(unsigned char *) data;
>> +		break;
>> +	case DP_CONFIG_PARAM_PREEMPHASIS:
>> +		format_string = "Preemphasis Level:     %u\n";
>> +		output_value = *(unsigned char *) data;
>> +		break;
>> +	case DP_CONFIG_PARAM_HRES:
>> +		format_string = "Horizontal Resolution: %d\n";
>> +		output_value = *(int *) data;
>> +		break;
>> +	case DP_CONFIG_PARAM_VRES:
>> +		format_string = "Vertical Resolution:   %d\n";
>> +		output_value = *(int *) data;
>> +		break;
>> +	case DP_CONFIG_PARAM_BPP:
>> +		format_string = "Bits Per Pixel:        %u\n";
>> +		output_value = *(unsigned char *) data;
>> +		break;
>> +	default:
>> +		format_string = NULL;
>> +		break;
>> +	}
>> +
>> +	if (format_string != NULL && for_param != DP_CONFIG_PARAM_CONNECTOR)
>> +		seq_printf(m, format_string, output_value);
>> +}
>> +
>>   /* 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
>> @@ -3701,6 +3817,360 @@ static const struct file_operations i915_display_crc_ctl_fops = {
>>   	.write = display_crc_ctl_write
>>   };
>>   
>> +static void dp_show_config(struct seq_file *m,
>> +			   struct drm_connector *connector)
>> +{
>> +	struct intel_dp *intel_dp;
>> +	struct intel_crtc *crtc;
>> +	struct intel_connector *intel_connector = to_intel_connector(connector);
>> +	struct intel_encoder *intel_encoder = intel_connector->encoder;
>> +	int pipe_bpp, hres, vres;
>> +	uint8_t vs[4], pe[4];
>> +	struct drm_display_mode *mode;
>> +	int i = 0;
>> +
>> +	if (!intel_encoder)
>> +		return;
>> +
>> +	if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT)
>> +		return;
>> +
>> +	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) >>
>> +			DP_TRAIN_PRE_EMPHASIS_SHIFT;
>> +	}
>> +
>> +	/* Output mode info only if a valid CRTC exists */
>> +	if (intel_encoder->base.crtc) {
>> +		crtc = to_intel_crtc(intel_encoder->base.crtc);
>> +		pipe_bpp = crtc->config->pipe_bpp;
>> +		mode = &crtc->config->base.adjusted_mode;
>> +		hres = mode->hdisplay;
>> +		vres = mode->vdisplay;
>> +	} else {
>> +		pipe_bpp = 0;
>> +		hres = vres = 0;
>> +	}
>> +
>> +	dp_print_string(m,
>> +			DP_CONFIG_PARAM_CONNECTOR,
>> +			connector->name);
>> +	dp_print_string(m,
>> +			DP_CONFIG_PARAM_CONNECTOR_ID,
>> +			&connector->base.id);
>> +	dp_print_string(m,
>> +			DP_CONFIG_PARAM_LINK_RATE,
>> +			&intel_dp->link_bw);
>> +	dp_print_string(m,
>> +			DP_CONFIG_PARAM_LANE_COUNT,
>> +			&intel_dp->lane_count);
>> +	dp_print_string(m,
>> +			DP_CONFIG_PARAM_VOLTAGE_SWING,
>> +			&vs[0]);
>> +	dp_print_string(m,
>> +			DP_CONFIG_PARAM_PREEMPHASIS,
>> +			&pe[0]);
>> +	dp_print_string(m,
>> +			DP_CONFIG_PARAM_HRES,
>> +			&hres);
>> +	dp_print_string(m,
>> +			DP_CONFIG_PARAM_VRES,
>> +			&vres);
>> +	dp_print_string(m,
>> +			DP_CONFIG_PARAM_BPP,
>> +			&pipe_bpp);
>> +}
>> +
>> +enum dp_config_param dp_get_param_type(char *input_string)
>> +{
>> +	enum dp_config_param param_type = DP_CONFIG_PARAM_INVALID;
>> +	char *s;
>> +	int i = 0;
>> +
>> +	for (i = 0; i < DP_PARAMETER_COUNT; i++) {
>> +		s = dp_get_config_str(i);
>> +		if (strncmp(input_string, s,
>> +			    strlen(s)) == 0) {
>> +			param_type = (enum dp_config_param) i;
>> +			break;
>> +		}
>> +	}
>> +	return param_type;
>> +}
>> +
>> +int dp_get_param_value(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 dp_tokenize_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 dp_parse_config(char *input_buffer,
>> +			   ssize_t buffer_size,
>> +			   struct intel_dp *intel_dp)
>> +{
>> +	int status = 0;
>> +	char *lines[MAX_DP_CONFIG_LINE_COUNT];
>> +	int i = 0;
>> +	struct dp_config parms[DP_PARAMETER_COUNT];
>> +	int line_count = 0;
>> +	char *buffer = input_buffer;
>> +	enum dp_config_param parm_type;
>> +	unsigned long parm_value;
>> +
>> +	line_count = dp_tokenize_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 = dp_get_param_type(lines[i]);
>> +		if (parm_type != DP_CONFIG_PARAM_INVALID) {
>> +			status = dp_get_param_value(parm_type,
>> +							lines[i+1],
>> +							&parm_value);
>> +			if (status == 0) {
>> +				parms[parm_type].type = parm_type;
>> +				parms[parm_type].value = parm_value;
>> +			}
>> +		}
>> +	}
>> +
>> +	if (parms[DP_CONFIG_PARAM_LINK_RATE].value == 0x06 ||
>> +	    parms[DP_CONFIG_PARAM_LINK_RATE].value == 0x0a ||
>> +	    parms[DP_CONFIG_PARAM_LINK_RATE].value == 0x14) {
>> +		intel_dp->compliance_config.link_rate =
>> +			parms[DP_CONFIG_PARAM_LINK_RATE].value;
>> +	} else
>> +		return -EINVAL;
>> +
>> +	if (parms[DP_CONFIG_PARAM_LANE_COUNT].value == 0x01 ||
>> +	    parms[DP_CONFIG_PARAM_LANE_COUNT].value == 0x02 ||
>> +	    parms[DP_CONFIG_PARAM_LANE_COUNT].value == 0x04) {
>> +		intel_dp->compliance_config.lane_count =
>> +			parms[DP_CONFIG_PARAM_LANE_COUNT].value;
>> +	} else
>> +		return -EINVAL;
>> +
>> +	if (parms[DP_CONFIG_PARAM_VOLTAGE_SWING].value <= 0x03 &&
>> +	    parms[DP_CONFIG_PARAM_VOLTAGE_SWING].value >= 0x00) {
>> +		intel_dp->compliance_config.vswing_level =
>> +			parms[DP_CONFIG_PARAM_VOLTAGE_SWING].value;
>> +	} else
>> +		return -EINVAL;
>> +
>> +	if (parms[DP_CONFIG_PARAM_PREEMPHASIS].value <= 0x03 &&
>> +	    parms[DP_CONFIG_PARAM_PREEMPHASIS].value >= 0x00) {
>> +		intel_dp->compliance_config.preemp_level =
>> +			parms[DP_CONFIG_PARAM_PREEMPHASIS].value;
>> +	} else
>> +		return -EINVAL;
>> +
>> +	if (parms[DP_CONFIG_PARAM_BPP].value == 18 ||
>> +	    parms[DP_CONFIG_PARAM_BPP].value == 24 ||
>> +	    parms[DP_CONFIG_PARAM_BPP].value == 30 ||
>> +	    parms[DP_CONFIG_PARAM_BPP].value == 36) {
>> +		intel_dp->compliance_config.bits_per_pixel =
>> +			parms[DP_CONFIG_PARAM_PREEMPHASIS].value;
>> +	} else
>> +		return -EINVAL;
>> +
>> +	if (parms[DP_CONFIG_PARAM_HRES].value > 0 &&
>> +	    parms[DP_CONFIG_PARAM_HRES].value <= 8192) {
>> +		intel_dp->compliance_config.hres =
>> +			parms[DP_CONFIG_PARAM_HRES].value;
>> +	} else
>> +		return -EINVAL;
>> +
>> +	if (parms[DP_CONFIG_PARAM_VRES].value > 0 &&
>> +	    parms[DP_CONFIG_PARAM_VRES].value <= 8192) {
>> +		intel_dp->compliance_config.vres =
>> +			parms[DP_CONFIG_PARAM_VRES].value;
>> +	} else
>> +		return -EINVAL;
>> +
>> +	return status;
>> +}
>> +
>> +static int i915_displayport_config_ctl_show(struct seq_file *m, void *data)
>> +{
>> +	struct drm_device *dev = m->private;
>> +	struct drm_connector *connector;
>> +	struct list_head *connector_list = &dev->mode_config.connector_list;
>> +
>> +	if (!dev)
>> +		return -ENODEV;
>> +
>> +	list_for_each_entry(connector, connector_list, head) {
>> +
>> +		if (connector->connector_type !=
>> +		    DRM_MODE_CONNECTOR_DisplayPort)
>> +			continue;
>> +
>> +		if (connector->status == connector_status_connected) {
>> +			dp_show_config(m, connector);
>> +		} else {
>> +			dp_print_string(m,
>> +					DP_CONFIG_PARAM_CONNECTOR,
>> +					connector->name);
>> +			dp_print_string(m,
>> +					DP_CONFIG_PARAM_CONNECTOR_ID,
>> +					&connector->base.id);
>> +			seq_puts(m, "disconnected\n");
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int i915_displayport_config_ctl_open(struct inode *inode,
>> +				       struct file *file)
>> +{
>> +	struct drm_device *dev = inode->i_private;
>> +
>> +	return single_open(file, i915_displayport_config_ctl_show, dev);
>> +}
>> +
>> +static ssize_t i915_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 list_head *connector_list;
>> +	struct intel_dp *intel_dp;
>> +
>> +	m = file->private_data;
>> +	if (!m) {
>> +		status = -ENODEV;
>> +		return status;
>> +	}
>> +	dev = m->private;
>> +
>> +	if (!dev) {
>> +		status = -ENODEV;
>> +		return status;
>> +	}
>> +	connector_list = &dev->mode_config.connector_list;
>> +
>> +	if (len == 0)
>> +		return 0;
>> +
>> +	input_buffer = kmalloc(len + 1, GFP_KERNEL);
>> +	if (!input_buffer)
>> +		return -ENOMEM;
>> +
>> +	if (copy_from_user(input_buffer, ubuf, len)) {
>> +		status = -EFAULT;
>> +		goto out;
>> +	}
>> +
>> +	input_buffer[len] = '\0';
>> +	DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
>> +
>> +	list_for_each_entry(connector, connector_list, head) {
>> +		if (connector->connector_type ==
>> +		    DRM_MODE_CONNECTOR_DisplayPort &&
>> +		    connector->status == connector_status_connected) {
>> +			intel_dp = enc_to_intel_dp(connector->encoder);
>> +			status = dp_parse_config(input_buffer, len, intel_dp);
>> +			if (status < 0)
>> +				goto out;
>> +		}
>> +	}
>> +out:
>> +	kfree(input_buffer);
>> +	if (status < 0)
>> +		return status;
>> +
>> +	*offp += len;
>> +	return len;
>> +}
>> +
>> +static const struct file_operations i915_displayport_config_ctl_fops = {
>> +	.owner = THIS_MODULE,
>> +	.open = i915_displayport_config_ctl_open,
>> +	.read = seq_read,
>> +	.llseek = seq_lseek,
>> +	.release = single_release,
>> +	.write = i915_displayport_config_ctl_write
>> +};
>> +
>>   static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
>>   {
>>   	struct drm_device *dev = m->private;
>> @@ -4450,6 +4920,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_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 74b30c1..c813d3c 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -592,6 +592,18 @@ struct intel_hdmi {
>>   struct intel_dp_mst_encoder;
>>   #define DP_MAX_DOWNSTREAM_PORTS		0x10
>>   
>> +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;
>> @@ -651,6 +663,7 @@ struct intel_dp {
>>   	/* Displayport compliance testing */
>>   	unsigned long compliance_test_data;
>>   	bool compliance_testing_active;
>> +	struct intel_dp_link_config compliance_config;
>>   };
>>   
>>   struct intel_digital_port {
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Jesse Barnes]

On 03/09/2015 02:04 PM, Ville Syrjälä wrote:
> On Mon, Mar 09, 2015 at 12:07:31PM -0700, Jesse Barnes wrote:
>> On 03/09/2015 10:29 AM, Daniel Vetter wrote:
>>> On Mon, Mar 09, 2015 at 08:34:49AM -0700, Jesse Barnes wrote:
>>>> On 03/06/2015 08:34 AM, Daniel Vetter wrote:
>>>>> On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote:
>>>>>> +	} else {
>>>>>> +		/* SST mode - handle short/long pulses here */
>>>>>> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>>>>> +		/* Clear compliance testing flags/data here to prevent
>>>>>> +		 * false detection in userspace */
>>>>>> +		intel_dp->compliance_test_data = 0;
>>>>>> +		intel_dp->compliance_testing_active = 0;
>>>>>> +		/* For a long pulse in SST mode, disable the main link */
>>>>>> +		if (long_hpd) {
>>>>>> +			I915_WRITE(DP_TP_CTL(intel_dig_port->port),
>>>>>> +					      ~DP_TP_CTL_ENABLE);
>>>>>> +		}
>>>>>
>>>>> Disabling the  main link should be done in userspace. All long pulse
>>>>> requests should be forwarded to userspace as a hotplug event. Userspace
>>>>> can then react to that hotplug appropriately. This way we can again
>>>>> exercise the normal operation of all our dp code.
>>>>
>>>> What's your concern here?  Do you want to make sure we get coverage on
>>>> dp_link_down()?  It looks like that might be safe to use here instead of
>>>> flipping the disable bit directly.  Or did you want to go through the
>>>> whole pipe/port shutdown sequence as well?  If so, I think the dpms
>>>> tests will already cover that, separate from simple compliance.
>>>
>>> This is likely to upset the state checker, we've already had some fun with
>>> killing the hard dp pipe disable that the hdp code occasionally did. Well,
>>> still have. The other reason is that dp compliance testing with
>>> special-case code is somewhat pointless, except when the compliance test
>>> contracts what real-world experience forces us to do. For these exceptions
>>> I'd like that we fully understand them and also document them. Disabling
>>> the link on a full hot-unplug is something we can (and most DE actually
>>> do) do.
>>
>> If we end up hitting the checker while testing, then yeah it would spew.
>>
>> But I thought this was mainly about testing the DP code, making sure we
>> can up/down links, train at different parameters, etc, not about going
>> through full mode sets all the time...
>>
>> But either way, I agree we should be documenting this behavior so we
>> don't get stuck trying to figure it out later.
> 
> I don't think we should be killing the port like this. It'll also kill
> the pipe on some platforms and then we get all kinds of pipe stuck
> warnings. So I think we'd definitely want a more graceful shutdown of
> things.

Does that affect current platforms?  Or just CTG and ILK?  I can guess
BYT & BSW might be affected, but I haven't tested.  As long as we just
up/down the port w/o anything else it might be able to work...

> I thought we actually discussed about going to the other direction, ie.
> potentially allowing the link to brought up without the pipe and
> enabling/disabling the pipe independently while the link remains up and
> running?

I guess I was thinking the reverse: that bringing up the port w/o a pipe
driving it would be more likely to cause problems, but I guess we'll
need testing.

Depending on what we find, we could change the logic to accommodate the
platforms we want to test (HSW+ and BYT+ I think, though we could limit
it to even newer ones if those are too tough to handle).

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

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

[Ville Syrjälä]

On Wed, Mar 11, 2015 at 11:37:23AM -0700, Jesse Barnes wrote:
> On 03/09/2015 02:04 PM, Ville Syrjälä wrote:
> > On Mon, Mar 09, 2015 at 12:07:31PM -0700, Jesse Barnes wrote:
> >> On 03/09/2015 10:29 AM, Daniel Vetter wrote:
> >>> On Mon, Mar 09, 2015 at 08:34:49AM -0700, Jesse Barnes wrote:
> >>>> On 03/06/2015 08:34 AM, Daniel Vetter wrote:
> >>>>> On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote:
> >>>>>> +	} else {
> >>>>>> +		/* SST mode - handle short/long pulses here */
> >>>>>> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >>>>>> +		/* Clear compliance testing flags/data here to prevent
> >>>>>> +		 * false detection in userspace */
> >>>>>> +		intel_dp->compliance_test_data = 0;
> >>>>>> +		intel_dp->compliance_testing_active = 0;
> >>>>>> +		/* For a long pulse in SST mode, disable the main link */
> >>>>>> +		if (long_hpd) {
> >>>>>> +			I915_WRITE(DP_TP_CTL(intel_dig_port->port),
> >>>>>> +					      ~DP_TP_CTL_ENABLE);
> >>>>>> +		}
> >>>>>
> >>>>> Disabling the  main link should be done in userspace. All long pulse
> >>>>> requests should be forwarded to userspace as a hotplug event. Userspace
> >>>>> can then react to that hotplug appropriately. This way we can again
> >>>>> exercise the normal operation of all our dp code.
> >>>>
> >>>> What's your concern here?  Do you want to make sure we get coverage on
> >>>> dp_link_down()?  It looks like that might be safe to use here instead of
> >>>> flipping the disable bit directly.  Or did you want to go through the
> >>>> whole pipe/port shutdown sequence as well?  If so, I think the dpms
> >>>> tests will already cover that, separate from simple compliance.
> >>>
> >>> This is likely to upset the state checker, we've already had some fun with
> >>> killing the hard dp pipe disable that the hdp code occasionally did. Well,
> >>> still have. The other reason is that dp compliance testing with
> >>> special-case code is somewhat pointless, except when the compliance test
> >>> contracts what real-world experience forces us to do. For these exceptions
> >>> I'd like that we fully understand them and also document them. Disabling
> >>> the link on a full hot-unplug is something we can (and most DE actually
> >>> do) do.
> >>
> >> If we end up hitting the checker while testing, then yeah it would spew.
> >>
> >> But I thought this was mainly about testing the DP code, making sure we
> >> can up/down links, train at different parameters, etc, not about going
> >> through full mode sets all the time...
> >>
> >> But either way, I agree we should be documenting this behavior so we
> >> don't get stuck trying to figure it out later.
> > 
> > I don't think we should be killing the port like this. It'll also kill
> > the pipe on some platforms and then we get all kinds of pipe stuck
> > warnings. So I think we'd definitely want a more graceful shutdown of
> > things.
> 
> Does that affect current platforms?  Or just CTG and ILK?  I can guess
> BYT & BSW might be affected, but I haven't tested.  As long as we just
> up/down the port w/o anything else it might be able to work...

I think you have that reversed. Old platforms were generally fine with
enabling/disabling ports while the pipe kept running, but I think that
changed at some point (with ilk I suppose), and killing the port is then
a bad idea.

That's at least the case with DP. I seem to recall we had at least stuck
pipes (and maybe even some lockups or something?) when we had the
dp_link_down() call in the hpd handler.

> 
> > I thought we actually discussed about going to the other direction, ie.
> > potentially allowing the link to brought up without the pipe and
> > enabling/disabling the pipe independently while the link remains up and
> > running?
> 
> I guess I was thinking the reverse: that bringing up the port w/o a pipe
> driving it would be more likely to cause problems, but I guess we'll
> need testing.

Bringing up the port on its own is perfectly fine. We do that for link
training already, and if you consider MST you'll notice it's the only
way really.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Wed, Mar 11, 2015 at 09:10:29PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 11, 2015 at 11:37:23AM -0700, Jesse Barnes wrote:
> > On 03/09/2015 02:04 PM, Ville Syrjälä wrote:
> > > On Mon, Mar 09, 2015 at 12:07:31PM -0700, Jesse Barnes wrote:
> > >> On 03/09/2015 10:29 AM, Daniel Vetter wrote:
> > >>> On Mon, Mar 09, 2015 at 08:34:49AM -0700, Jesse Barnes wrote:
> > >>>> On 03/06/2015 08:34 AM, Daniel Vetter wrote:
> > >>>>> On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote:
> > >>>>>> +	} else {
> > >>>>>> +		/* SST mode - handle short/long pulses here */
> > >>>>>> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > >>>>>> +		/* Clear compliance testing flags/data here to prevent
> > >>>>>> +		 * false detection in userspace */
> > >>>>>> +		intel_dp->compliance_test_data = 0;
> > >>>>>> +		intel_dp->compliance_testing_active = 0;
> > >>>>>> +		/* For a long pulse in SST mode, disable the main link */
> > >>>>>> +		if (long_hpd) {
> > >>>>>> +			I915_WRITE(DP_TP_CTL(intel_dig_port->port),
> > >>>>>> +					      ~DP_TP_CTL_ENABLE);
> > >>>>>> +		}
> > >>>>>
> > >>>>> Disabling the  main link should be done in userspace. All long pulse
> > >>>>> requests should be forwarded to userspace as a hotplug event. Userspace
> > >>>>> can then react to that hotplug appropriately. This way we can again
> > >>>>> exercise the normal operation of all our dp code.
> > >>>>
> > >>>> What's your concern here?  Do you want to make sure we get coverage on
> > >>>> dp_link_down()?  It looks like that might be safe to use here instead of
> > >>>> flipping the disable bit directly.  Or did you want to go through the
> > >>>> whole pipe/port shutdown sequence as well?  If so, I think the dpms
> > >>>> tests will already cover that, separate from simple compliance.
> > >>>
> > >>> This is likely to upset the state checker, we've already had some fun with
> > >>> killing the hard dp pipe disable that the hdp code occasionally did. Well,
> > >>> still have. The other reason is that dp compliance testing with
> > >>> special-case code is somewhat pointless, except when the compliance test
> > >>> contracts what real-world experience forces us to do. For these exceptions
> > >>> I'd like that we fully understand them and also document them. Disabling
> > >>> the link on a full hot-unplug is something we can (and most DE actually
> > >>> do) do.
> > >>
> > >> If we end up hitting the checker while testing, then yeah it would spew.
> > >>
> > >> But I thought this was mainly about testing the DP code, making sure we
> > >> can up/down links, train at different parameters, etc, not about going
> > >> through full mode sets all the time...
> > >>
> > >> But either way, I agree we should be documenting this behavior so we
> > >> don't get stuck trying to figure it out later.
> > > 
> > > I don't think we should be killing the port like this. It'll also kill
> > > the pipe on some platforms and then we get all kinds of pipe stuck
> > > warnings. So I think we'd definitely want a more graceful shutdown of
> > > things.
> > 
> > Does that affect current platforms?  Or just CTG and ILK?  I can guess
> > BYT & BSW might be affected, but I haven't tested.  As long as we just
> > up/down the port w/o anything else it might be able to work...
> 
> I think you have that reversed. Old platforms were generally fine with
> enabling/disabling ports while the pipe kept running, but I think that
> changed at some point (with ilk I suppose), and killing the port is then
> a bad idea.
> 
> That's at least the case with DP. I seem to recall we had at least stuck
> pipes (and maybe even some lockups or something?) when we had the
> dp_link_down() call in the hpd handler.

cpu edp on ilk+ and hsw+ dp are the ones that definitely get cranky if you
just kill the port. Not sure whether we've even seen hard hangs in dp
enabling on hsw, it's been a very long time ago. Well we've seen hard
hangs on hsw because of modeset bugs, but I don't remember whether it was
this on here too.

> > > I thought we actually discussed about going to the other direction, ie.
> > > potentially allowing the link to brought up without the pipe and
> > > enabling/disabling the pipe independently while the link remains up and
> > > running?
> > 
> > I guess I was thinking the reverse: that bringing up the port w/o a pipe
> > driving it would be more likely to cause problems, but I guess we'll
> > need testing.
> 
> Bringing up the port on its own is perfectly fine. We do that for link
> training already, and if you consider MST you'll notice it's the only
> way really.

Yeah on hsw+ and cpu edp we bring up the prot first iirc, then fire up the
pipe later on. Dunno what we do for external dp on ilk-ivb or what exactly
we should be doing - we iirc still have the occasionally dp port stuck bug
floating about.

In any case there's a very well defined sequence to go about things, and
anything else tends to anger the machines a lot. If we can't just use the
setCrtc ioctl from userspace we should at least reuse the link shutdown
machinery we have properly. Of course I'd prefer if we don't have to since
that would be less extra code to keep working.
-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