[intel-gfx][PATCH V5] Displayport compliance testing V5

[intel-gfx][PATCH V5] Displayport compliance testing V5

[Todd Previte]

This is the 5th iteration of the Displayport compliance testing patch set for 
performing compliance testing operations of the i915 driver. High level changes 
are listed below, with the specifics for each patch listed in the commit messages.

Kernel:

Changes for V4:
- Removed the code for link configuration in debugfs. It wasn’t used in this 
  patch set so there was no need to add 500+ lines of code to the kernel. It may  
  be reintroduced in a future patch if it becomes necessary to support link 
  configuration testing for Displayport compliance
- Merged working changes in to the kernel code to keep the patches smaller and
  as discrete, testable units. This included moving variables around between 
  patches such that their declaration and use appears in the same patch. One 
  variable was removed entirely as it was no longer necessary.
- Changed the debugfs interface for test control. Previously test control was 
  handled by a single file that contained tags and values that were parsed and 
  used by both the user app and the kernel. This required a lot of parsing code    
  on both sides of the equation. That has been eliminated in favor of 3 separate,
  single value files for test type, test data and testing active. This reduces 
  the overhead of polling on test_active in the user app as well as eliminating 
  the need to parse a single monolithic file every time it checks the flag 
  (currently 1ms intervals). The net result is a more responsible app and a lot 
  less code on both sides.

Changes for V5:
- Removed a duplicate EDID read from the EDID auto test function
- Added a failsafe check in the I2C DEFER check to make sure a misbhaving
  device wouldn’t cause an infinite loop
- Shuffled around some variable declarations and assignments to put them in
  the correct patches and places
- Fixed a commit message that was no longer accurate
- Removed the main stream disable code from check_link_status as this doesn’t
  play nice with a number of systems. A replacement for this will be done in
  the user app when necessary.

Userspace app:

The userspace app can be found here:

https://github.com/tprevite/intel-gpu-tools/tree/dp_compliance

The user app has the following requirements:
- Must be executed as root from the command line
- No other display managers can be running. Must be in console mode.
- Only the test device should be connected to one of the external Displayport 
  ports. No other displays should be connected via Displayport. eDP displays are 
  ignored by the compliance code so they should operate normally.

Previous versions of the user app disabled all displays on the DUT. That is no 
longer necessary in order to execute Displayport compliance testing. The app can 
be run remotely via an SSH login or directly on the DUT at the preference of the 
operator. Aside from starting and stopping the app itself, no direct interaction 
is necessary at this time on the part of the test operator to perform compliance 
testing.

Some of the tests will still pass if the user app isn't running. The primary   
purpose of the user space application is to handle the heavy lifting of the
mode sets required to set the specific display resolutions for the tests. 

Changes for V2:
	- Removed unnecessary memcpy()s and replaced with assignments
	- Updated the README
	- Moved the menu code around and reworded it to be more accurate
	- Added a “NONE” display mode to disable the main link

Note that as of this moment, the Github version is out of date. This patch is the 
most current version but Github will be updated shortly.

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

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

V4:
- Moved declaration of type_type here
- Removed declaration of test_data (moved to a later patch)
- Added reset to 0 for compliance test variables

V5:
- Moved test_active variable declaration and initialization out of
  this patch and into the patch where it's used
- Changed variable name compliance_testing_active to
  compliance_test_active to unify the naming convention
- Added initialization for compliance_test_type variable

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index eea9e36..ae0fb98 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3746,11 +3746,78 @@ 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;
+
+	intel_dp->compliance_test_type = 0;
+	intel_dp->aux.i2c_nack_count = 0;
+	intel_dp->aux.i2c_defer_count = 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");
+		intel_dp->compliance_test_type = DP_TEST_LINK_TRAINING;
+		response = intel_dp_autotest_link_training(intel_dp);
+		break;
+	case DP_TEST_LINK_VIDEO_PATTERN:
+		DRM_DEBUG_KMS("TEST_PATTERN test requested\n");
+		intel_dp->compliance_test_type = DP_TEST_LINK_VIDEO_PATTERN;
+		response = intel_dp_autotest_video_pattern(intel_dp);
+		break;
+	case DP_TEST_LINK_EDID_READ:
+		DRM_DEBUG_KMS("EDID test requested\n");
+		intel_dp->compliance_test_type = DP_TEST_LINK_EDID_READ;
+		response = intel_dp_autotest_edid(intel_dp);
+		break;
+	case DP_TEST_LINK_PHY_TEST_PATTERN:
+		DRM_DEBUG_KMS("PHY_PATTERN test requested\n");
+		intel_dp->compliance_test_type = DP_TEST_LINK_PHY_TEST_PATTERN;
+		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..ab811e5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -647,6 +647,9 @@ struct intel_dp {
 				     bool has_aux_irq,
 				     int send_bytes,
 				     uint32_t aux_clock_divider);
+
+	/* Displayport compliance testing */
+	unsigned long compliance_test_type;
 };
 
 struct intel_digital_port {
-- 
1.9.1

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

[PATCH 02/11] drm/i915: Ignore disconnected Displayport connectors in check_link_status

[Todd Previte]

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 device is
disconnected, 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.

V1:
- Earlier review feedback recommended the original check_link_status
  patch be split into two. This is the first patch of that split.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ae0fb98..30cd433 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3895,21 +3895,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;
 	}
-- 
1.9.1

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

[PATCH 03/11] drm/i915: Move the DPCD read further up in intel_dp_check_link_status()

[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.

V1:
- This is the second part of the single-patch split previously
  mentioned.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 30cd433..23184b0 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3913,13 +3913,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 04/11] drm/i915: Add EDID read in intel_dp_check_link_status() for Link CTS 4.2.2.1

[Todd Previte]

Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the
Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for
all HPD plug events. To reduce the amount of code, this EDID read is also
used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual
support for these tests is implemented in later patches in this series.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 23184b0..a5dfaff 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3890,6 +3890,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
+	struct drm_connector *connector = &intel_dp->attached_connector->base;
+	struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+	struct edid *edid_read = NULL;
 	u8 sink_irq_vector;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
@@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 		return;
 	}
 
+	/* Displayport Link CTS Core 1.2 rev1.1 EDID testing
+	 * 4.2.2.1 - EDID read required for all HPD events
+         */
+        edid_read = drm_get_edid(connector, adapter);
+        if (!edid_read || connector->edid_header_corrupt == 1) {
+                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
+        }
+
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
 	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
-- 
1.9.1

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

[PATCH 05/11] 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.

Also, the check for DP_AUX_CH_CTL_TIME_OUT_ERROR has been moved out into a
separate case. This case just continues with the next iteration of the loop
as the HW has already waited the required amount of time.

V2:
- Changed udelay() to usleep_range()
V3:
- Removed extraneous check for timeout
- Updated comment to reflect this change
V4:
- Reformatted a comment
V5:
- Added separate check for HW timeout on AUX transactions. A message
  is logged upon detection of this case.
V6:
- Add continue statement to HW timeout detect case
- Remove the log message indicating a timeout has been
  detected (review feedback)
V7:
- Updated the commit message to remove verbage about the HW timeout
  case that is no longer valid.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a5dfaff..77b6b15 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -877,9 +877,18 @@ 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))
+			if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR)
 				continue;
+
+			/* 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 06/11] drm/i915: Add supporting structure for Displayport Link CTS test 4.2.2.6

[Todd Previte]

Displayport compliance test 4.2.2.6 requires that a source device be capable of
detecting a corrupt EDID. The test specification states that the sink device
sets up the EDID with an invalid checksum. To do this, the sink sets up an
invalid EDID header, expecting the source device to generate the checksum and
compare it to the value stored in the last byte of the block data.

Unfortunately, the DRM EDID reading and parsing functions are actually too good
in this case; the header is fixed before the checksum is computed and thus the
code never sees the invalid checksum. This results in a failure to pass the
compliance test.

To correct this issue, a checksum is generated when the EDID header is detected
as corrupted. If the checksum is invalid, it sets the header_corrupt flag and
logs the errors. In the case of a more seriously damaged header (fixup score
less than the threshold) the code does not generate the checksum but does set
the header_corrupt flag.

V2:
- Removed the static bool global
- Added a bool to the drm_connector struct to reaplce the static one for
  holding the status of raw edid header corruption detection
- Modified the function signature of the is_valid function to take an
  additional parameter to store the corruption detected value
- Fixed the other callers of the above is_valid function
V3:
- Updated the commit message to be more clear about what and why this
  patch does what it does.
- Added comment in code to clarify the operations there
- Removed compliance variable and check_link_status update; those
  have been moved to a later patch
- Removed variable assignment from the bottom of the test handler

Signed-off-by: Todd Previte <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_edid.c        | 31 ++++++++++++++++++++++++++-----
 drivers/gpu/drm/drm_edid_load.c   |  7 +++++--
 drivers/gpu/drm/i2c/tda998x_drv.c |  4 ++--
 include/drm/drm_crtc.h            |  8 +++++++-
 4 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 53bc7a6..12e5be7 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1005,7 +1005,6 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
 	for (i = 0; i < sizeof(edid_header); i++)
 		if (raw_edid[i] == edid_header[i])
 			score++;
-
 	return score;
 }
 EXPORT_SYMBOL(drm_edid_header_is_valid);
@@ -1047,7 +1046,8 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
  *
  * Return: True if the block is valid, false otherwise.
  */
-bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
+bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
+			  bool *header_corrupt)
 {
 	u8 csum;
 	struct edid *edid = (struct edid *)raw_edid;
@@ -1062,9 +1062,27 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
 		int score = drm_edid_header_is_valid(raw_edid);
 		if (score == 8) ;
 		else if (score >= edid_fixup) {
+			/* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6
+			 * In order to properly generate the invalid checksum
+			 * required for this test, it must be generated using
+			 * the raw EDID data. Otherwise, the fix-up code here
+			 * will correct the problem, the checksum is then correct
+			 * and the test fails
+			 */
+			csum = drm_edid_block_checksum(raw_edid);
+			if (csum) {
+				DRM_DEBUG_DRIVER("Invalid EDID header, score = %d\n", score);
+				DRM_DEBUG_DRIVER("Invalid EDID checksum %d\n", csum);
+				if (header_corrupt)
+					*header_corrupt = 1;
+			}
 			DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
 			memcpy(raw_edid, edid_header, sizeof(edid_header));
 		} else {
+			if (header_corrupt) {
+				DRM_DEBUG_DRIVER("Invalid EDID header\n");
+				*header_corrupt = 1;
+			}
 			goto bad;
 		}
 	}
@@ -1129,7 +1147,7 @@ bool drm_edid_is_valid(struct edid *edid)
 		return false;
 
 	for (i = 0; i <= edid->extensions; i++)
-		if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
+		if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true, NULL))
 			return false;
 
 	return true;
@@ -1232,7 +1250,8 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	for (i = 0; i < 4; i++) {
 		if (get_edid_block(data, block, 0, EDID_LENGTH))
 			goto out;
-		if (drm_edid_block_valid(block, 0, print_bad_edid))
+		if (drm_edid_block_valid(block, 0, print_bad_edid,
+					 &connector->edid_header_corrupt))
 			break;
 		if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
 			connector->null_edid_counter++;
@@ -1257,7 +1276,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 				  block + (valid_extensions + 1) * EDID_LENGTH,
 				  j, EDID_LENGTH))
 				goto out;
-			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
+			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j,
+						 print_bad_edid,
+						 &connector->edid_header_corrupt)) {
 				valid_extensions++;
 				break;
 			}
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 732cb6f..1505494 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -216,7 +216,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 		goto out;
 	}
 
-	if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
+	if (!drm_edid_block_valid(edid, 0, print_bad_edid,
+				  &connector->edid_header_corrupt)) {
 		connector->bad_edid_counter++;
 		DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
 		    name);
@@ -229,7 +230,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 		if (i != valid_extensions + 1)
 			memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
 			    edid + i * EDID_LENGTH, EDID_LENGTH);
-		if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, print_bad_edid))
+		if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, 
+					 print_bad_edid,
+					 &connector->edid_header_corrupt))
 			valid_extensions++;
 	}
 
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index a9041d1..9c3d6b3 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1106,7 +1106,7 @@ static uint8_t *do_get_edid(struct tda998x_priv *priv)
 	if (read_edid_block(priv, block, 0))
 		goto fail;
 
-	if (!drm_edid_block_valid(block, 0, print_bad_edid))
+	if (!drm_edid_block_valid(block, 0, print_bad_edid, NULL))
 		goto fail;
 
 	/* if there's no extensions, we're done */
@@ -1123,7 +1123,7 @@ static uint8_t *do_get_edid(struct tda998x_priv *priv)
 		if (read_edid_block(priv, ext_block, j))
 			goto fail;
 
-		if (!drm_edid_block_valid(ext_block, j, print_bad_edid))
+		if (!drm_edid_block_valid(ext_block, j, print_bad_edid, NULL))
 			goto fail;
 
 		valid_extensions++;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 0261417..e31a4b3 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -719,6 +719,11 @@ struct drm_connector {
 	int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */
 	unsigned bad_edid_counter;
 
+	/* Flag for raw EDID header corruption - used in Displayport compliance testing
+	 * Displayport Link CTS Core 1.2 rev1.1 4.2.2.6
+	 */
+	bool edid_header_corrupt;
+
 	struct dentry *debugfs_entry;
 
 	struct drm_connector_state *state;
@@ -1436,7 +1441,8 @@ extern void drm_set_preferred_mode(struct drm_connector *connector,
 				   int hpref, int vpref);
 
 extern int drm_edid_header_is_valid(const u8 *raw_edid);
-extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid);
+extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, bool *header_corrupt);
+
 extern bool drm_edid_is_valid(struct edid *edid);
 
 extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
-- 
1.9.1

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

[PATCH 07/11] drm/i915: Update intel_dp_hpd_pulse() 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
V5:
- Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8
V6:
- Reformatted a comment

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 77b6b15..ba2da44 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4572,29 +4572,26 @@ 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;
 
 	goto put_power;
 mst_fail:
-	/* if we were in MST mode, and device is not there get out of MST mode */
 	if (intel_dp->is_mst) {
+		/* if we were in MST mode, and device is not there get out of MST mode */
 		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);
 	}
 put_power:
+	/* SST mode - handle short/long pulses here */
+	if (!intel_dp->is_mst) {
+		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;
+	}
 	intel_display_power_put(dev_priv, power_domain);
 
 	return ret;
-- 
1.9.1

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

[PATCH 08/11] drm/i915: Support EDID compliance tests with the intel_dp_autotest_edid() 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
- Updated to use the raw header corruption detection mechanism
- Moved the declaration of the test_data variable here
V5:
- Update test active flag variable name to match the change in the
  first patch of the series.
- Relocated the test active flag declaration and initialization
  to this patch
V6:
- Updated to use the new flag for raw EDID header corruption
- Removed the extra EDID read from the autotest function
- Added the edid_checksum variable to struct intel_dp so that the
  autotest function can write it to the sink device
- Moved the update to the hpd_pulse function to another patch
- Removed extraneous constants

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ba2da44..e5d39ef 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -41,6 +41,12 @@
 
 #define DP_LINK_CHECK_TIMEOUT	(10 * 1000)
 
+/* Compliance test status bits  */
+#define INTEL_DP_RESOLUTION_SHIFT_MASK	4
+#define INTEL_DP_RESOLUTION_PREFERRED	(1 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define INTEL_DP_RESOLUTION_STANDARD	(2 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define INTEL_DP_RESOLUTION_FAILSAFE	(3 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+
 struct dp_link_dpll {
 	int link_bw;
 	struct dpll dpll;
@@ -3770,6 +3776,35 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
 {
 	uint8_t test_result = DP_TEST_NAK;
+	uint32_t ret = 0;
+
+	if (intel_dp->compliance_edid_invalid ||
+	    intel_dp->aux.i2c_defer_count > 6) {
+		/* 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_RESOLUTION_FAILSAFE;
+	} else {
+		ret = drm_dp_dpcd_write(&intel_dp->aux,
+					DP_TEST_EDID_CHECKSUM,
+					&intel_dp->compliance_edid_checksum, 1);
+		if (ret <= 0)
+			DRM_DEBUG_DRIVER("Failed to write EDID checksum\n");
+		else
+			DRM_DEBUG_DRIVER("EDID checksum written to sink\n");
+
+		test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
+		intel_dp->compliance_test_data = INTEL_DP_RESOLUTION_STANDARD;
+	}
+
+	/* Set test active flag here so userspace doesn't interrupt things */
+	intel_dp->compliance_test_active = 1;
+
 	return test_result;
 }
 
@@ -3785,7 +3820,10 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 	uint8_t rxdata = 0;
 	int status = 0;
 
+	intel_dp->compliance_test_active = 0;
 	intel_dp->compliance_test_type = 0;
+	intel_dp->compliance_test_data = 0;
+
 	intel_dp->aux.i2c_nack_count = 0;
 	intel_dp->aux.i2c_defer_count = 0;
 
@@ -3920,11 +3958,21 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
 	/* Displayport Link CTS Core 1.2 rev1.1 EDID testing
 	 * 4.2.2.1 - EDID read required for all HPD events
+	 * Also used for 4.2.2.3, 4.2.2.4 4.2.2.5 and 4.2.2.6
+	 *   - compliance_edid_invalid flag used for 4.2.2.6
+	 *   - compliance_edid_checksum used for 4.2.2.3
          */
         edid_read = drm_get_edid(connector, adapter);
+	intel_dp->compliance_edid_checksum = edid_read->checksum;
         if (!edid_read || connector->edid_header_corrupt == 1) {
                 DRM_DEBUG_DRIVER("Invalid EDID detected\n");
+                intel_dp->compliance_edid_invalid = 1;
+		intel_dp->compliance_edid_checksum = 0;
         }
+	else {
+		intel_dp->compliance_edid_invalid = 0;
+		intel_dp->compliance_edid_checksum = edid_read->checksum;
+	}
 
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ab811e5..23030a1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -650,6 +650,10 @@ struct intel_dp {
 
 	/* Displayport compliance testing */
 	unsigned long compliance_test_type;
+	unsigned long compliance_test_data;
+	bool compliance_test_active;
+	bool compliance_edid_invalid;
+	uint8_t compliance_edid_checksum;
 };
 
 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 09/11] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Todd Previte]

For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
device must attempt at least 7 times to read the EDID when it receives an
I2C defer. The normal DRM code makes only 7 retries, regardless of whether
or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
since there are native defers interspersed with the I2C defers which
results in less than 7 EDID read attempts.

The solution is to decrement the retry counter when an I2C DEFER is returned
such that another read attempt will be made. This situation should normally
only occur in compliance testing, however, as a worse case real-world
scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
for a single transaction to complete. The net result is a slightly slower
response to an EDID read that shouldn't significantly impact overall
performance.

V2:
- Added a check on the number of I2C Defers to limit the number
  of times that the retries variable will be decremented. This
  is to address review feedback regarding possible infinite loops
  from misbehaving sink devices.
V3:
- Fixed the limit value to 7 instead of 8 to get the correct retry
  count.
- Combined the increment of the defer count into the if-statement

Signed-off-by: Todd Previte <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_dp_helper.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 79968e3..9ecfd27 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -468,7 +468,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 
 		case DP_AUX_I2C_REPLY_DEFER:
 			DRM_DEBUG_KMS("I2C defer\n");
-			aux->i2c_defer_count++;
+			/* DP Compliance Test 4.2.2.5 Requirement:
+			 * Must have at least 7 retries for I2C defers on the
+			 * transaction to pass this test
+			 */
+			if (++aux->i2c_defer_count < 7)
+				retry = 0;
 			usleep_range(400, 500);
 			continue;
 
-- 
1.9.1

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

[PATCH 10/11] drm/i915: Add debugfs test control files for Displayport compliance testing

[Todd Previte]

This patch adds 3 debugfs files for handling Displayport compliance testing
and supercedes the previous patches that implemented debugfs support for
compliance testing. Those patches were:

- [PATCH 04/17] drm/i915: Add debugfs functions for Displayport
                          compliance testing
- [PATCH 08/17] drm/i915: Add new debugfs file for Displayport
                          compliance test control
- [PATCH 09/17] drm/i915: Add debugfs write and test param parsing
                          functions for DP test control

This new patch simplifies the debugfs implementation by places a single
test control value into an individual file. Each file is readable by
the usersapce application and the test_active file is writable to
indicate to the kernel when userspace has completed its portion of the
test sequence.

Replacing the previous files simplifies operation and speeds response
time for the user app, as it is required to poll on the test_active file
in order to determine when it needs to begin its operations.

V2:
- Updated the test active variable name to match the change in
  the initial patch of the series

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b315f01..b539279 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3701,6 +3701,211 @@ static const struct file_operations i915_display_crc_ctl_fops = {
 	.write = display_crc_ctl_write
 };
 
+static ssize_t i915_displayport_test_active_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;
+	int val = 0;
+
+	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 = kstrtoint(input_buffer, 10, &val);
+			if (status < 0)
+				goto out;
+			DRM_DEBUG_DRIVER("Got %d for test active\n", val);
+			/* To prevent erroneous activation of the compliance
+			 * testing code, only accept an actual value of 1 here
+			 */
+			if (val == 1)
+				intel_dp->compliance_test_active = 1;
+			else
+				intel_dp->compliance_test_active = 0;
+		}
+	}
+out:
+	kfree(input_buffer);
+	if (status < 0)
+		return status;
+
+	*offp += len;
+	return len;
+}
+
+static int i915_displayport_test_active_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 &&
+		    connector->encoder != NULL) {
+			intel_dp = enc_to_intel_dp(connector->encoder);
+			if (intel_dp->compliance_test_active)
+				seq_puts(m, "1");
+			else
+				seq_puts(m, "0");
+		} else
+			seq_puts(m, "0");
+	}
+
+	return 0;
+}
+
+static int i915_displayport_test_active_open(struct inode *inode,
+				       struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+
+	return single_open(file, i915_displayport_test_active_show, dev);
+}
+
+static const struct file_operations i915_displayport_test_active_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_displayport_test_active_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+	.write = i915_displayport_test_active_write
+};
+
+static int i915_displayport_test_data_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 &&
+		    connector->encoder != NULL) {
+			intel_dp = enc_to_intel_dp(connector->encoder);
+			seq_printf(m, "%lx", intel_dp->compliance_test_data);
+		} else
+			seq_puts(m, "0");
+	}
+
+	return 0;
+}
+static int i915_displayport_test_data_open(struct inode *inode,
+				       struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+
+	return single_open(file, i915_displayport_test_data_show, dev);
+}
+
+static const struct file_operations i915_displayport_test_data_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_displayport_test_data_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release
+};
+
+static int i915_displayport_test_type_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 &&
+		    connector->encoder != NULL) {
+			intel_dp = enc_to_intel_dp(connector->encoder);
+			seq_printf(m, "%02lx", intel_dp->compliance_test_type);
+		} else
+			seq_puts(m, "0");
+	}
+
+	return 0;
+}
+
+static int i915_displayport_test_type_open(struct inode *inode,
+				       struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+
+	return single_open(file, i915_displayport_test_type_show, dev);
+}
+
+static const struct file_operations i915_displayport_test_type_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_displayport_test_type_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;
@@ -4450,6 +4655,9 @@ 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_dp_test_data", &i915_displayport_test_data_fops},
+	{"i915_dp_test_type", &i915_displayport_test_type_fops},
+	{"i915_dp_test_active", &i915_displayport_test_active_fops}
 };
 
 void intel_display_crc_init(struct drm_device *dev)
-- 
1.9.1

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

[PATCH 11/11] drm: Fix the 'native defer' message in drm_dp_i2c_do_msg()

[Todd Previte]

The debug message is missing a newline at the end and it makes the
logs hard to read when a device defers a lot. Simple 2-character fix
adds the newline at the end.

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

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 9ecfd27..4ac416e 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -433,7 +433,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 			return -EREMOTEIO;
 
 		case DP_AUX_NATIVE_REPLY_DEFER:
-			DRM_DEBUG_KMS("native defer");
+			DRM_DEBUG_KMS("native defer\n");
 			/*
 			 * We could check for I2C bit rate capabilities and if
 			 * available adjust this interval. We could also be
-- 
1.9.1

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

Re: [PATCH 11/11] drm: Fix the 'native defer' message in drm_dp_i2c_do_msg()

[Alex Deucher]

On Fri, Apr 10, 2015 at 12:12 PM, Todd Previte <tprevite@gmail.com> wrote:
> The debug message is missing a newline at the end and it makes the
> logs hard to read when a device defers a lot. Simple 2-character fix
> adds the newline at the end.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> Cc: dri-devel@lists.freedesktop.org

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/drm_dp_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 9ecfd27..4ac416e 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -433,7 +433,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>                         return -EREMOTEIO;
>
>                 case DP_AUX_NATIVE_REPLY_DEFER:
> -                       DRM_DEBUG_KMS("native defer");
> +                       DRM_DEBUG_KMS("native defer\n");
>                         /*
>                          * We could check for I2C bit rate capabilities and if
>                          * available adjust this interval. We could also be
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/11] drm/i915: Add supporting structure for Displayport Link CTS test 4.2.2.6

[Todd Previte]

Easy enough to do. Tag removed and updated patch posted. Thanks Emil!

On 4/10/2015 10:45 AM, Emil Velikov wrote:
> Hi Todd
>
> On 10/04/15 16:12, Todd Previte wrote:
>> Displayport compliance test 4.2.2.6 requires that a source device be capable of
>> detecting a corrupt EDID. The test specification states that the sink device
>> sets up the EDID with an invalid checksum. To do this, the sink sets up an
>> invalid EDID header, expecting the source device to generate the checksum and
>> compare it to the value stored in the last byte of the block data.
>>
>> Unfortunately, the DRM EDID reading and parsing functions are actually too good
>> in this case; the header is fixed before the checksum is computed and thus the
>> code never sees the invalid checksum. This results in a failure to pass the
>> compliance test.
>>
>> To correct this issue, a checksum is generated when the EDID header is detected
>> as corrupted. If the checksum is invalid, it sets the header_corrupt flag and
>> logs the errors. In the case of a more seriously damaged header (fixup score
>> less than the threshold) the code does not generate the checksum but does set
>> the header_corrupt flag.
>>
>> V2:
>> - Removed the static bool global
>> - Added a bool to the drm_connector struct to reaplce the static one for
>>    holding the status of raw edid header corruption detection
>> - Modified the function signature of the is_valid function to take an
>>    additional parameter to store the corruption detected value
>> - Fixed the other callers of the above is_valid function
>> V3:
>> - Updated the commit message to be more clear about what and why this
>>    patch does what it does.
>> - Added comment in code to clarify the operations there
>> - Removed compliance variable and check_link_status update; those
>>    have been moved to a later patch
>> - Removed variable assignment from the bottom of the test handler
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> Cc: dri-devel@lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/drm_edid.c        | 31 ++++++++++++++++++++++++++-----
>>   drivers/gpu/drm/drm_edid_load.c   |  7 +++++--
>>   drivers/gpu/drm/i2c/tda998x_drv.c |  4 ++--
>>   include/drm/drm_crtc.h            |  8 +++++++-
>>   4 files changed, 40 insertions(+), 10 deletions(-)
>>
> Neither this nor patch 09/11 seems to be i915 specific. If you're doing
> another revision you might want to use just "drm:".
>
> Cheers,
> Emil

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

[PATCH 06/13] drm: Add supporting structure for Displayport Link CTS test 4.2.2.6

[Todd Previte]

Displayport compliance test 4.2.2.6 requires that a source device be capable of
detecting a corrupt EDID. The test specification states that the sink device
sets up the EDID with an invalid checksum. To do this, the sink sets up an
invalid EDID header, expecting the source device to generate the checksum and
compare it to the value stored in the last byte of the block data.

Unfortunately, the DRM EDID reading and parsing functions are actually too good
in this case; the header is fixed before the checksum is computed and thus the
code never sees the invalid checksum. This results in a failure to pass the
compliance test.

To correct this issue, a checksum is generated when the EDID header is detected
as corrupted. If the checksum is invalid, it sets the header_corrupt flag and
logs the errors. In the case of a more seriously damaged header (fixup score
less than the threshold) the code does not generate the checksum but does set
the header_corrupt flag.

V2:
- Removed the static bool global
- Added a bool to the drm_connector struct to reaplce the static one for
  holding the status of raw edid header corruption detection
- Modified the function signature of the is_valid function to take an
  additional parameter to store the corruption detected value
- Fixed the other callers of the above is_valid function
V3:
- Updated the commit message to be more clear about what and why this
  patch does what it does.
- Added comment in code to clarify the operations there
- Removed compliance variable and check_link_status update; those
  have been moved to a later patch
- Removed variable assignment from the bottom of the test handler
V4:
- Removed i915 tag from subject line as the patch is not i915-specific

Signed-off-by: Todd Previte <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_edid.c        | 31 ++++++++++++++++++++++++++-----
 drivers/gpu/drm/drm_edid_load.c   |  7 +++++--
 drivers/gpu/drm/i2c/tda998x_drv.c |  4 ++--
 include/drm/drm_crtc.h            |  8 +++++++-
 4 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 53bc7a6..12e5be7 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1005,7 +1005,6 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
 	for (i = 0; i < sizeof(edid_header); i++)
 		if (raw_edid[i] == edid_header[i])
 			score++;
-
 	return score;
 }
 EXPORT_SYMBOL(drm_edid_header_is_valid);
@@ -1047,7 +1046,8 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
  *
  * Return: True if the block is valid, false otherwise.
  */
-bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
+bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
+			  bool *header_corrupt)
 {
 	u8 csum;
 	struct edid *edid = (struct edid *)raw_edid;
@@ -1062,9 +1062,27 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
 		int score = drm_edid_header_is_valid(raw_edid);
 		if (score == 8) ;
 		else if (score >= edid_fixup) {
+			/* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6
+			 * In order to properly generate the invalid checksum
+			 * required for this test, it must be generated using
+			 * the raw EDID data. Otherwise, the fix-up code here
+			 * will correct the problem, the checksum is then correct
+			 * and the test fails
+			 */
+			csum = drm_edid_block_checksum(raw_edid);
+			if (csum) {
+				DRM_DEBUG_DRIVER("Invalid EDID header, score = %d\n", score);
+				DRM_DEBUG_DRIVER("Invalid EDID checksum %d\n", csum);
+				if (header_corrupt)
+					*header_corrupt = 1;
+			}
 			DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
 			memcpy(raw_edid, edid_header, sizeof(edid_header));
 		} else {
+			if (header_corrupt) {
+				DRM_DEBUG_DRIVER("Invalid EDID header\n");
+				*header_corrupt = 1;
+			}
 			goto bad;
 		}
 	}
@@ -1129,7 +1147,7 @@ bool drm_edid_is_valid(struct edid *edid)
 		return false;
 
 	for (i = 0; i <= edid->extensions; i++)
-		if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
+		if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true, NULL))
 			return false;
 
 	return true;
@@ -1232,7 +1250,8 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	for (i = 0; i < 4; i++) {
 		if (get_edid_block(data, block, 0, EDID_LENGTH))
 			goto out;
-		if (drm_edid_block_valid(block, 0, print_bad_edid))
+		if (drm_edid_block_valid(block, 0, print_bad_edid,
+					 &connector->edid_header_corrupt))
 			break;
 		if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
 			connector->null_edid_counter++;
@@ -1257,7 +1276,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 				  block + (valid_extensions + 1) * EDID_LENGTH,
 				  j, EDID_LENGTH))
 				goto out;
-			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
+			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j,
+						 print_bad_edid,
+						 &connector->edid_header_corrupt)) {
 				valid_extensions++;
 				break;
 			}
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 732cb6f..1505494 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -216,7 +216,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 		goto out;
 	}
 
-	if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
+	if (!drm_edid_block_valid(edid, 0, print_bad_edid,
+				  &connector->edid_header_corrupt)) {
 		connector->bad_edid_counter++;
 		DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
 		    name);
@@ -229,7 +230,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 		if (i != valid_extensions + 1)
 			memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
 			    edid + i * EDID_LENGTH, EDID_LENGTH);
-		if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, print_bad_edid))
+		if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, 
+					 print_bad_edid,
+					 &connector->edid_header_corrupt))
 			valid_extensions++;
 	}
 
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index a9041d1..9c3d6b3 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1106,7 +1106,7 @@ static uint8_t *do_get_edid(struct tda998x_priv *priv)
 	if (read_edid_block(priv, block, 0))
 		goto fail;
 
-	if (!drm_edid_block_valid(block, 0, print_bad_edid))
+	if (!drm_edid_block_valid(block, 0, print_bad_edid, NULL))
 		goto fail;
 
 	/* if there's no extensions, we're done */
@@ -1123,7 +1123,7 @@ static uint8_t *do_get_edid(struct tda998x_priv *priv)
 		if (read_edid_block(priv, ext_block, j))
 			goto fail;
 
-		if (!drm_edid_block_valid(ext_block, j, print_bad_edid))
+		if (!drm_edid_block_valid(ext_block, j, print_bad_edid, NULL))
 			goto fail;
 
 		valid_extensions++;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 0261417..e31a4b3 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -719,6 +719,11 @@ struct drm_connector {
 	int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */
 	unsigned bad_edid_counter;
 
+	/* Flag for raw EDID header corruption - used in Displayport compliance testing
+	 * Displayport Link CTS Core 1.2 rev1.1 4.2.2.6
+	 */
+	bool edid_header_corrupt;
+
 	struct dentry *debugfs_entry;
 
 	struct drm_connector_state *state;
@@ -1436,7 +1441,8 @@ extern void drm_set_preferred_mode(struct drm_connector *connector,
 				   int hpref, int vpref);
 
 extern int drm_edid_header_is_valid(const u8 *raw_edid);
-extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid);
+extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, bool *header_corrupt);
+
 extern bool drm_edid_is_valid(struct edid *edid);
 
 extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
-- 
1.9.1

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

[PATCH 09/13] drm: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Todd Previte]

For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
device must attempt at least 7 times to read the EDID when it receives an
I2C defer. The normal DRM code makes only 7 retries, regardless of whether
or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
since there are native defers interspersed with the I2C defers which
results in less than 7 EDID read attempts.

The solution is to decrement the retry counter when an I2C DEFER is returned
such that another read attempt will be made. This situation should normally
only occur in compliance testing, however, as a worse case real-world
scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
for a single transaction to complete. The net result is a slightly slower
response to an EDID read that shouldn't significantly impact overall
performance.

V2:
- Added a check on the number of I2C Defers to limit the number
  of times that the retries variable will be decremented. This
  is to address review feedback regarding possible infinite loops
  from misbehaving sink devices.
V3:
- Fixed the limit value to 7 instead of 8 to get the correct retry
  count.
- Combined the increment of the defer count into the if-statement
V4:
- Removed i915 tag from subject as the patch is not i915-specific

Signed-off-by: Todd Previte <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_dp_helper.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 79968e3..9ecfd27 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -468,7 +468,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 
 		case DP_AUX_I2C_REPLY_DEFER:
 			DRM_DEBUG_KMS("I2C defer\n");
-			aux->i2c_defer_count++;
+			/* DP Compliance Test 4.2.2.5 Requirement:
+			 * Must have at least 7 retries for I2C defers on the
+			 * transaction to pass this test
+			 */
+			if (++aux->i2c_defer_count < 7)
+				retry = 0;
 			usleep_range(400, 500);
 			continue;
 
-- 
1.9.1

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

[PATCH 08/13] drm/i915: Support EDID compliance tests with the intel_dp_autotest_edid() 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
- Updated to use the raw header corruption detection mechanism
- Moved the declaration of the test_data variable here
V5:
- Update test active flag variable name to match the change in the
  first patch of the series.
- Relocated the test active flag declaration and initialization
  to this patch
V6:
- Updated to use the new flag for raw EDID header corruption
- Removed the extra EDID read from the autotest function
- Added the edid_checksum variable to struct intel_dp so that the
  autotest function can write it to the sink device
- Moved the update to the hpd_pulse function to another patch
- Removed extraneous constants
V7:
- Fixed erroneous placement of the checksum assignment. In some cases
  such as when the EDID read fails and is NULL, this causes a NULL ptr
  dereference in the kernel. Bad news. Fixed now.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ba2da44..3bfdc40 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -41,6 +41,12 @@
 
 #define DP_LINK_CHECK_TIMEOUT	(10 * 1000)
 
+/* Compliance test status bits  */
+#define INTEL_DP_RESOLUTION_SHIFT_MASK	4
+#define INTEL_DP_RESOLUTION_PREFERRED	(1 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define INTEL_DP_RESOLUTION_STANDARD	(2 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define INTEL_DP_RESOLUTION_FAILSAFE	(3 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+
 struct dp_link_dpll {
 	int link_bw;
 	struct dpll dpll;
@@ -3770,6 +3776,35 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
 {
 	uint8_t test_result = DP_TEST_NAK;
+	uint32_t ret = 0;
+
+	if (intel_dp->compliance_edid_invalid ||
+	    intel_dp->aux.i2c_defer_count > 6) {
+		/* 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_RESOLUTION_FAILSAFE;
+	} else {
+		ret = drm_dp_dpcd_write(&intel_dp->aux,
+					DP_TEST_EDID_CHECKSUM,
+					&intel_dp->compliance_edid_checksum, 1);
+		if (ret <= 0)
+			DRM_DEBUG_DRIVER("Failed to write EDID checksum\n");
+		else
+			DRM_DEBUG_DRIVER("EDID checksum written to sink\n");
+
+		test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
+		intel_dp->compliance_test_data = INTEL_DP_RESOLUTION_STANDARD;
+	}
+
+	/* Set test active flag here so userspace doesn't interrupt things */
+	intel_dp->compliance_test_active = 1;
+
 	return test_result;
 }
 
@@ -3785,7 +3820,10 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 	uint8_t rxdata = 0;
 	int status = 0;
 
+	intel_dp->compliance_test_active = 0;
 	intel_dp->compliance_test_type = 0;
+	intel_dp->compliance_test_data = 0;
+
 	intel_dp->aux.i2c_nack_count = 0;
 	intel_dp->aux.i2c_defer_count = 0;
 
@@ -3920,11 +3958,19 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
 	/* Displayport Link CTS Core 1.2 rev1.1 EDID testing
 	 * 4.2.2.1 - EDID read required for all HPD events
+	 * Also used for 4.2.2.3, 4.2.2.4 4.2.2.5 and 4.2.2.6
+	 *   - compliance_edid_invalid flag used for 4.2.2.6
+	 *   - compliance_edid_checksum used for 4.2.2.3
          */
+	intel_dp->compliance_edid_invalid = 0;
         edid_read = drm_get_edid(connector, adapter);
         if (!edid_read || connector->edid_header_corrupt == 1) {
                 DRM_DEBUG_DRIVER("Invalid EDID detected\n");
+                intel_dp->compliance_edid_invalid = 1;
+		intel_dp->compliance_edid_checksum = 0;
         }
+	else
+		intel_dp->compliance_edid_checksum = edid_read->checksum;
 
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ab811e5..23030a1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -650,6 +650,10 @@ struct intel_dp {
 
 	/* Displayport compliance testing */
 	unsigned long compliance_test_type;
+	unsigned long compliance_test_data;
+	bool compliance_test_active;
+	bool compliance_edid_invalid;
+	uint8_t compliance_edid_checksum;
 };
 
 struct intel_digital_port {
-- 
1.9.1

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

Re: [PATCH 06/11] drm/i915: Add supporting structure for Displayport Link CTS test 4.2.2.6

[Emil Velikov]

Hi Todd

On 10/04/15 16:12, Todd Previte wrote:
> Displayport compliance test 4.2.2.6 requires that a source device be capable of
> detecting a corrupt EDID. The test specification states that the sink device
> sets up the EDID with an invalid checksum. To do this, the sink sets up an
> invalid EDID header, expecting the source device to generate the checksum and
> compare it to the value stored in the last byte of the block data.
> 
> Unfortunately, the DRM EDID reading and parsing functions are actually too good
> in this case; the header is fixed before the checksum is computed and thus the
> code never sees the invalid checksum. This results in a failure to pass the
> compliance test.
> 
> To correct this issue, a checksum is generated when the EDID header is detected
> as corrupted. If the checksum is invalid, it sets the header_corrupt flag and
> logs the errors. In the case of a more seriously damaged header (fixup score
> less than the threshold) the code does not generate the checksum but does set
> the header_corrupt flag.
> 
> V2:
> - Removed the static bool global
> - Added a bool to the drm_connector struct to reaplce the static one for
>   holding the status of raw edid header corruption detection
> - Modified the function signature of the is_valid function to take an
>   additional parameter to store the corruption detected value
> - Fixed the other callers of the above is_valid function
> V3:
> - Updated the commit message to be more clear about what and why this
>   patch does what it does.
> - Added comment in code to clarify the operations there
> - Removed compliance variable and check_link_status update; those
>   have been moved to a later patch
> - Removed variable assignment from the bottom of the test handler
> 
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_edid.c        | 31 ++++++++++++++++++++++++++-----
>  drivers/gpu/drm/drm_edid_load.c   |  7 +++++--
>  drivers/gpu/drm/i2c/tda998x_drv.c |  4 ++--
>  include/drm/drm_crtc.h            |  8 +++++++-
>  4 files changed, 40 insertions(+), 10 deletions(-)
> 
Neither this nor patch 09/11 seems to be i915 specific. If you're doing
another revision you might want to use just "drm:".

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

Re: [PATCH 04/11] drm/i915: Add EDID read in intel_dp_check_link_status() for Link CTS 4.2.2.1

[Paulo Zanoni]

2015-04-10 13:12 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the
> Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for
> all HPD plug events. To reduce the amount of code, this EDID read is also
> used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual
> support for these tests is implemented in later patches in this series.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>

drivers/gpu/drm/i915/intel_dp.c: In function ‘intel_dp_check_link_status’:
drivers/gpu/drm/i915/intel_dp.c:4140:36: error: ‘struct drm_connector’
has no member named ‘edid_header_corrupt’
         if (!edid_read || connector->edid_header_corrupt == 1) {

scripts/Makefile.build:258: recipe for target
'drivers/gpu/drm/i915/intel_dp.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_dp.o] Error 1

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 23184b0..a5dfaff 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3890,6 +3890,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  {
>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
>         struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> +       struct drm_connector *connector = &intel_dp->attached_connector->base;
> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
> +       struct edid *edid_read = NULL;
>         u8 sink_irq_vector;
>         u8 link_status[DP_LINK_STATUS_SIZE];
>
> @@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>                 return;
>         }
>
> +       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
> +        * 4.2.2.1 - EDID read required for all HPD events
> +         */
> +        edid_read = drm_get_edid(connector, adapter);
> +        if (!edid_read || connector->edid_header_corrupt == 1) {
> +                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
> +        }
> +
>         /* Try to read the source of the interrupt */
>         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>             intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

[PATCH 04/13] drm/i915: Add EDID read in intel_dp_check_link_status() for Link CTS 4.2.2.1

[Todd Previte]

Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the
Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for
all HPD plug events. To reduce the amount of code, this EDID read is also
used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual
support for these tests is implemented in later patches in this series.

V2:
- Fixed compilation error introduced during rework

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 23184b0..75df3e2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3890,6 +3890,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
+	struct drm_connector *connector = &intel_dp->attached_connector->base;
+	struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+	struct edid *edid_read = NULL;
 	u8 sink_irq_vector;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
@@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 		return;
 	}
 
+	/* Displayport Link CTS Core 1.2 rev1.1 EDID testing
+	 * 4.2.2.1 - EDID read required for all HPD events
+         */
+        edid_read = drm_get_edid(connector, adapter);
+        if (!edid_read) {
+                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
+        }
+
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
 	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
-- 
1.9.1

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

[PATCH 06/13] drm: Add supporting structure for Displayport Link CTS test 4.2.2.6

[Todd Previte]

Displayport compliance test 4.2.2.6 requires that a source device be capable of
detecting a corrupt EDID. The test specification states that the sink device
sets up the EDID with an invalid checksum. To do this, the sink sets up an
invalid EDID header, expecting the source device to generate the checksum and
compare it to the value stored in the last byte of the block data.

Unfortunately, the DRM EDID reading and parsing functions are actually too good
in this case; the header is fixed before the checksum is computed and thus the
code never sees the invalid checksum. This results in a failure to pass the
compliance test.

To correct this issue, a checksum is generated when the EDID header is detected
as corrupted. If the checksum is invalid, it sets the header_corrupt flag and
logs the errors. In the case of a more seriously damaged header (fixup score
less than the threshold) the code does not generate the checksum but does set
the header_corrupt flag.

V2:
- Removed the static bool global
- Added a bool to the drm_connector struct to reaplce the static one for
  holding the status of raw edid header corruption detection
- Modified the function signature of the is_valid function to take an
  additional parameter to store the corruption detected value
- Fixed the other callers of the above is_valid function
V3:
- Updated the commit message to be more clear about what and why this
  patch does what it does.
- Added comment in code to clarify the operations there
- Removed compliance variable and check_link_status update; those
  have been moved to a later patch
- Removed variable assignment from the bottom of the test handler
V4:
- Removed i915 tag from subject line as the patch is not i915-specific
V5:
- Moved code causing a compilation error to this patch where the variable
  is actually declared

Signed-off-by: Todd Previte <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_edid.c        | 31 ++++++++++++++++++++++++++-----
 drivers/gpu/drm/drm_edid_load.c   |  7 +++++--
 drivers/gpu/drm/i2c/tda998x_drv.c |  4 ++--
 drivers/gpu/drm/i915/intel_dp.c   |  2 +-
 include/drm/drm_crtc.h            |  8 +++++++-
 5 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 53bc7a6..12e5be7 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1005,7 +1005,6 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
 	for (i = 0; i < sizeof(edid_header); i++)
 		if (raw_edid[i] == edid_header[i])
 			score++;
-
 	return score;
 }
 EXPORT_SYMBOL(drm_edid_header_is_valid);
@@ -1047,7 +1046,8 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
  *
  * Return: True if the block is valid, false otherwise.
  */
-bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
+bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
+			  bool *header_corrupt)
 {
 	u8 csum;
 	struct edid *edid = (struct edid *)raw_edid;
@@ -1062,9 +1062,27 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
 		int score = drm_edid_header_is_valid(raw_edid);
 		if (score == 8) ;
 		else if (score >= edid_fixup) {
+			/* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6
+			 * In order to properly generate the invalid checksum
+			 * required for this test, it must be generated using
+			 * the raw EDID data. Otherwise, the fix-up code here
+			 * will correct the problem, the checksum is then correct
+			 * and the test fails
+			 */
+			csum = drm_edid_block_checksum(raw_edid);
+			if (csum) {
+				DRM_DEBUG_DRIVER("Invalid EDID header, score = %d\n", score);
+				DRM_DEBUG_DRIVER("Invalid EDID checksum %d\n", csum);
+				if (header_corrupt)
+					*header_corrupt = 1;
+			}
 			DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
 			memcpy(raw_edid, edid_header, sizeof(edid_header));
 		} else {
+			if (header_corrupt) {
+				DRM_DEBUG_DRIVER("Invalid EDID header\n");
+				*header_corrupt = 1;
+			}
 			goto bad;
 		}
 	}
@@ -1129,7 +1147,7 @@ bool drm_edid_is_valid(struct edid *edid)
 		return false;
 
 	for (i = 0; i <= edid->extensions; i++)
-		if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
+		if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true, NULL))
 			return false;
 
 	return true;
@@ -1232,7 +1250,8 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	for (i = 0; i < 4; i++) {
 		if (get_edid_block(data, block, 0, EDID_LENGTH))
 			goto out;
-		if (drm_edid_block_valid(block, 0, print_bad_edid))
+		if (drm_edid_block_valid(block, 0, print_bad_edid,
+					 &connector->edid_header_corrupt))
 			break;
 		if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
 			connector->null_edid_counter++;
@@ -1257,7 +1276,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 				  block + (valid_extensions + 1) * EDID_LENGTH,
 				  j, EDID_LENGTH))
 				goto out;
-			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
+			if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j,
+						 print_bad_edid,
+						 &connector->edid_header_corrupt)) {
 				valid_extensions++;
 				break;
 			}
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 732cb6f..1505494 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -216,7 +216,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 		goto out;
 	}
 
-	if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
+	if (!drm_edid_block_valid(edid, 0, print_bad_edid,
+				  &connector->edid_header_corrupt)) {
 		connector->bad_edid_counter++;
 		DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
 		    name);
@@ -229,7 +230,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 		if (i != valid_extensions + 1)
 			memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
 			    edid + i * EDID_LENGTH, EDID_LENGTH);
-		if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, print_bad_edid))
+		if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, 
+					 print_bad_edid,
+					 &connector->edid_header_corrupt))
 			valid_extensions++;
 	}
 
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index a9041d1..9c3d6b3 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1106,7 +1106,7 @@ static uint8_t *do_get_edid(struct tda998x_priv *priv)
 	if (read_edid_block(priv, block, 0))
 		goto fail;
 
-	if (!drm_edid_block_valid(block, 0, print_bad_edid))
+	if (!drm_edid_block_valid(block, 0, print_bad_edid, NULL))
 		goto fail;
 
 	/* if there's no extensions, we're done */
@@ -1123,7 +1123,7 @@ static uint8_t *do_get_edid(struct tda998x_priv *priv)
 		if (read_edid_block(priv, ext_block, j))
 			goto fail;
 
-		if (!drm_edid_block_valid(ext_block, j, print_bad_edid))
+		if (!drm_edid_block_valid(ext_block, j, print_bad_edid, NULL))
 			goto fail;
 
 		valid_extensions++;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e1d6e79..77b6b15 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3922,7 +3922,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	 * 4.2.2.1 - EDID read required for all HPD events
          */
         edid_read = drm_get_edid(connector, adapter);
-        if (!edid_read) {
+        if (!edid_read || connector->edid_header_corrupt == 1) {
                 DRM_DEBUG_DRIVER("Invalid EDID detected\n");
         }
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 0261417..e31a4b3 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -719,6 +719,11 @@ struct drm_connector {
 	int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */
 	unsigned bad_edid_counter;
 
+	/* Flag for raw EDID header corruption - used in Displayport compliance testing
+	 * Displayport Link CTS Core 1.2 rev1.1 4.2.2.6
+	 */
+	bool edid_header_corrupt;
+
 	struct dentry *debugfs_entry;
 
 	struct drm_connector_state *state;
@@ -1436,7 +1441,8 @@ extern void drm_set_preferred_mode(struct drm_connector *connector,
 				   int hpref, int vpref);
 
 extern int drm_edid_header_is_valid(const u8 *raw_edid);
-extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid);
+extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, bool *header_corrupt);
+
 extern bool drm_edid_is_valid(struct edid *edid);
 
 extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
-- 
1.9.1

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

Re: [PATCH 04/11] drm/i915: Add EDID read in intel_dp_check_link_status() for Link CTS 4.2.2.1

[Todd Previte]

On 4/13/15 7:10 AM, Paulo Zanoni wrote:
> 2015-04-10 13:12 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the
>> Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for
>> all HPD plug events. To reduce the amount of code, this EDID read is also
>> used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual
>> support for these tests is implemented in later patches in this series.
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
> drivers/gpu/drm/i915/intel_dp.c: In function ‘intel_dp_check_link_status’:
> drivers/gpu/drm/i915/intel_dp.c:4140:36: error: ‘struct drm_connector’
> has no member named ‘edid_header_corrupt’
>           if (!edid_read || connector->edid_header_corrupt == 1) {
>
> scripts/Makefile.build:258: recipe for target
> 'drivers/gpu/drm/i915/intel_dp.o' failed
> make[4]: *** [drivers/gpu/drm/i915/intel_dp.o] Error 1
Gah. That happened while shuffling stuff around between the patches. All 
fixed. The logical OR in
the if-statement has been moved to patch 6 where that variable is 
actually declared. Both patches
have already been posted to the list.

I also went back through the entire patch set and verified they all 
compile at every stage, so this
shouldn't happen again.
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 23184b0..a5dfaff 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3890,6 +3890,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   {
>>          struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>          struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>> +       struct drm_connector *connector = &intel_dp->attached_connector->base;
>> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
>> +       struct edid *edid_read = NULL;
>>          u8 sink_irq_vector;
>>          u8 link_status[DP_LINK_STATUS_SIZE];
>>
>> @@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>                  return;
>>          }
>>
>> +       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
>> +        * 4.2.2.1 - EDID read required for all HPD events
>> +         */
>> +        edid_read = drm_get_edid(connector, adapter);
>> +        if (!edid_read || connector->edid_header_corrupt == 1) {
>> +                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
>> +        }
>> +
>>          /* Try to read the source of the interrupt */
>>          if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>              intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

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

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

[Paulo Zanoni]

2015-04-10 13:12 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> Add the skeleton framework for supporting automation for Displayport compliance
> testing. This patch adds the necessary framework for the source device to
> appropriately respond to test automation requests from a sink device.
>
> 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
>
> V4:
> - Moved declaration of type_type here
> - Removed declaration of test_data (moved to a later patch)
> - Added reset to 0 for compliance test variables
>
> V5:
> - Moved test_active variable declaration and initialization out of
>   this patch and into the patch where it's used
> - Changed variable name compliance_testing_active to
>   compliance_test_active to unify the naming convention
> - Added initialization for compliance_test_type variable
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 75 +++++++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h |  3 ++
>  2 files changed, 74 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index eea9e36..ae0fb98 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3746,11 +3746,78 @@ 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;
> +
> +       intel_dp->compliance_test_type = 0;
> +       intel_dp->aux.i2c_nack_count = 0;
> +       intel_dp->aux.i2c_defer_count = 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");
> +               intel_dp->compliance_test_type = DP_TEST_LINK_TRAINING;
> +               response = intel_dp_autotest_link_training(intel_dp);
> +               break;
> +       case DP_TEST_LINK_VIDEO_PATTERN:
> +               DRM_DEBUG_KMS("TEST_PATTERN test requested\n");
> +               intel_dp->compliance_test_type = DP_TEST_LINK_VIDEO_PATTERN;
> +               response = intel_dp_autotest_video_pattern(intel_dp);
> +               break;
> +       case DP_TEST_LINK_EDID_READ:
> +               DRM_DEBUG_KMS("EDID test requested\n");
> +               intel_dp->compliance_test_type = DP_TEST_LINK_EDID_READ;
> +               response = intel_dp_autotest_edid(intel_dp);
> +               break;
> +       case DP_TEST_LINK_PHY_TEST_PATTERN:
> +               DRM_DEBUG_KMS("PHY_PATTERN test requested\n");
> +               intel_dp->compliance_test_type = DP_TEST_LINK_PHY_TEST_PATTERN;
> +               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..ab811e5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -647,6 +647,9 @@ struct intel_dp {
>                                      bool has_aux_irq,
>                                      int send_bytes,
>                                      uint32_t aux_clock_divider);
> +
> +       /* Displayport compliance testing */
> +       unsigned long compliance_test_type;
>  };
>
>  struct intel_digital_port {
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

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

[Paulo Zanoni]

2015-04-10 13:12 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
> specifies that repeated AUX transactions after a failure (no response /
> invalid response) must have a minimum delay of 400us before the resend can
> occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.
>
> Also, the check for DP_AUX_CH_CTL_TIME_OUT_ERROR has been moved out into a
> separate case. This case just continues with the next iteration of the loop
> as the HW has already waited the required amount of time.
>
> V2:
> - Changed udelay() to usleep_range()
> V3:
> - Removed extraneous check for timeout
> - Updated comment to reflect this change
> V4:
> - Reformatted a comment
> V5:
> - Added separate check for HW timeout on AUX transactions. A message
>   is logged upon detection of this case.
> V6:
> - Add continue statement to HW timeout detect case
> - Remove the log message indicating a timeout has been
>   detected (review feedback)
> V7:
> - Updated the commit message to remove verbage about the HW timeout
>   case that is no longer valid.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a5dfaff..77b6b15 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -877,9 +877,18 @@ 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))
> +                       if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR)
>                                 continue;
> +
> +                       /* 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



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

Re: [PATCH 06/13] drm: Add supporting structure for Displayport Link CTS test 4.2.2.6

[Paulo Zanoni]

2015-04-13 11:53 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> Displayport compliance test 4.2.2.6 requires that a source device be capable of
> detecting a corrupt EDID. The test specification states that the sink device
> sets up the EDID with an invalid checksum. To do this, the sink sets up an
> invalid EDID header, expecting the source device to generate the checksum and
> compare it to the value stored in the last byte of the block data.
>
> Unfortunately, the DRM EDID reading and parsing functions are actually too good
> in this case; the header is fixed before the checksum is computed and thus the
> code never sees the invalid checksum. This results in a failure to pass the
> compliance test.
>
> To correct this issue, a checksum is generated when the EDID header is detected
> as corrupted. If the checksum is invalid, it sets the header_corrupt flag and
> logs the errors. In the case of a more seriously damaged header (fixup score
> less than the threshold) the code does not generate the checksum but does set
> the header_corrupt flag.
>
> V2:
> - Removed the static bool global
> - Added a bool to the drm_connector struct to reaplce the static one for
>   holding the status of raw edid header corruption detection
> - Modified the function signature of the is_valid function to take an
>   additional parameter to store the corruption detected value
> - Fixed the other callers of the above is_valid function
> V3:
> - Updated the commit message to be more clear about what and why this
>   patch does what it does.
> - Added comment in code to clarify the operations there
> - Removed compliance variable and check_link_status update; those
>   have been moved to a later patch
> - Removed variable assignment from the bottom of the test handler
> V4:
> - Removed i915 tag from subject line as the patch is not i915-specific
> V5:
> - Moved code causing a compilation error to this patch where the variable
>   is actually declared
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_edid.c        | 31 ++++++++++++++++++++++++++-----
>  drivers/gpu/drm/drm_edid_load.c   |  7 +++++--
>  drivers/gpu/drm/i2c/tda998x_drv.c |  4 ++--
>  drivers/gpu/drm/i915/intel_dp.c   |  2 +-
>  include/drm/drm_crtc.h            |  8 +++++++-
>  5 files changed, 41 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 53bc7a6..12e5be7 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1005,7 +1005,6 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
>         for (i = 0; i < sizeof(edid_header); i++)
>                 if (raw_edid[i] == edid_header[i])
>                         score++;
> -
>         return score;
>  }
>  EXPORT_SYMBOL(drm_edid_header_is_valid);

Bad chunk...


> @@ -1047,7 +1046,8 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
>   *
>   * Return: True if the block is valid, false otherwise.
>   */
> -bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
> +bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
> +                         bool *header_corrupt)

Need to add the new parameter description to the documentation above.


>  {
>         u8 csum;
>         struct edid *edid = (struct edid *)raw_edid;
> @@ -1062,9 +1062,27 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
>                 int score = drm_edid_header_is_valid(raw_edid);
>                 if (score == 8) ;
>                 else if (score >= edid_fixup) {
> +                       /* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6
> +                        * In order to properly generate the invalid checksum
> +                        * required for this test, it must be generated using
> +                        * the raw EDID data. Otherwise, the fix-up code here
> +                        * will correct the problem, the checksum is then correct
> +                        * and the test fails
> +                        */
> +                       csum = drm_edid_block_checksum(raw_edid);
> +                       if (csum) {
> +                               DRM_DEBUG_DRIVER("Invalid EDID header, score = %d\n", score);
> +                               DRM_DEBUG_DRIVER("Invalid EDID checksum %d\n", csum);

No one on this file uses DRM_DEBUG_DRIVER (you use 2 calls here and one below).

Also, during "normal operation" we try to calculate the checksum based
on the fixed EDID header, so if we also print these messages here
we're always going to have a message complaining about invalid
checksum: either this one or the other that's already there. My
bikeshed would be to just remove the messages you added here and below
to not confuse users. Let's assume that the bad header is due to some
communication/corruption error, and the HW manufacturers did not
program an EDID with a bad header and a correct checksum based on bad
header :)


> +                               if (header_corrupt)
> +                                       *header_corrupt = 1;
> +                       }
>                         DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
>                         memcpy(raw_edid, edid_header, sizeof(edid_header));
>                 } else {
> +                       if (header_corrupt) {
> +                               DRM_DEBUG_DRIVER("Invalid EDID header\n");
> +                               *header_corrupt = 1;
> +                       }

We don't ever set header_corrupt back to false, so if we ever get a
corrupt header once, the header_corrupt flag will stay there even if
it gets fixed somehow (such as a DP compliance tester starting to
submit the correct header).


>                         goto bad;
>                 }
>         }
> @@ -1129,7 +1147,7 @@ bool drm_edid_is_valid(struct edid *edid)
>                 return false;
>
>         for (i = 0; i <= edid->extensions; i++)
> -               if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
> +               if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true, NULL))
>                         return false;
>
>         return true;
> @@ -1232,7 +1250,8 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>         for (i = 0; i < 4; i++) {
>                 if (get_edid_block(data, block, 0, EDID_LENGTH))
>                         goto out;
> -               if (drm_edid_block_valid(block, 0, print_bad_edid))
> +               if (drm_edid_block_valid(block, 0, print_bad_edid,
> +                                        &connector->edid_header_corrupt))
>                         break;
>                 if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
>                         connector->null_edid_counter++;
> @@ -1257,7 +1276,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>                                   block + (valid_extensions + 1) * EDID_LENGTH,
>                                   j, EDID_LENGTH))
>                                 goto out;
> -                       if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
> +                       if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j,
> +                                                print_bad_edid,
> +                                                &connector->edid_header_corrupt)) {

You can use NULL here since we're not handling block 0 anymore.


>                                 valid_extensions++;
>                                 break;
>                         }
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 732cb6f..1505494 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -216,7 +216,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
>                 goto out;
>         }
>
> -       if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
> +       if (!drm_edid_block_valid(edid, 0, print_bad_edid,
> +                                 &connector->edid_header_corrupt)) {
>                 connector->bad_edid_counter++;
>                 DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
>                     name);
> @@ -229,7 +230,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
>                 if (i != valid_extensions + 1)
>                         memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
>                             edid + i * EDID_LENGTH, EDID_LENGTH);
> -               if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, print_bad_edid))
> +               if (drm_edid_block_valid(edid + i * EDID_LENGTH, i,
> +                                        print_bad_edid,
> +                                        &connector->edid_header_corrupt))

Same here: not iterating over block 0.

Btw, why can't we just use NULL on edid_load?


>                         valid_extensions++;
>         }
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index a9041d1..9c3d6b3 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1106,7 +1106,7 @@ static uint8_t *do_get_edid(struct tda998x_priv *priv)
>         if (read_edid_block(priv, block, 0))
>                 goto fail;
>
> -       if (!drm_edid_block_valid(block, 0, print_bad_edid))
> +       if (!drm_edid_block_valid(block, 0, print_bad_edid, NULL))
>                 goto fail;
>
>         /* if there's no extensions, we're done */
> @@ -1123,7 +1123,7 @@ static uint8_t *do_get_edid(struct tda998x_priv *priv)
>                 if (read_edid_block(priv, ext_block, j))
>                         goto fail;
>
> -               if (!drm_edid_block_valid(ext_block, j, print_bad_edid))
> +               if (!drm_edid_block_valid(ext_block, j, print_bad_edid, NULL))
>                         goto fail;
>
>                 valid_extensions++;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e1d6e79..77b6b15 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3922,7 +3922,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>          * 4.2.2.1 - EDID read required for all HPD events
>           */
>          edid_read = drm_get_edid(connector, adapter);
> -        if (!edid_read) {
> +        if (!edid_read || connector->edid_header_corrupt == 1) {
>                  DRM_DEBUG_DRIVER("Invalid EDID detected\n");
>          }
>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 0261417..e31a4b3 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -719,6 +719,11 @@ struct drm_connector {
>         int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */
>         unsigned bad_edid_counter;
>
> +       /* Flag for raw EDID header corruption - used in Displayport compliance testing
> +        * Displayport Link CTS Core 1.2 rev1.1 4.2.2.6
> +        */
> +       bool edid_header_corrupt;
> +
>         struct dentry *debugfs_entry;
>
>         struct drm_connector_state *state;
> @@ -1436,7 +1441,8 @@ extern void drm_set_preferred_mode(struct drm_connector *connector,
>                                    int hpref, int vpref);
>
>  extern int drm_edid_header_is_valid(const u8 *raw_edid);
> -extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid);
> +extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, bool *header_corrupt);
> +
>  extern bool drm_edid_is_valid(struct edid *edid);
>
>  extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel



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

Re: [PATCH 07/11] drm/i915: Update intel_dp_hpd_pulse() for non-MST operation

[Paulo Zanoni]

2015-04-10 13:12 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> 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
> V5:
> - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8
> V6:
> - Reformatted a comment
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 77b6b15..ba2da44 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4572,29 +4572,26 @@ 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);
> -               }
>         }

Shouldn't the code be moved to exactly this spot instead of after the
put_power label? Why would we want to call check_link_status in case
we goto mst_fail? In case there is a valid reason, maybe it would be
better to do a big reorganization of this function because it's going
to start looking very weird - or at least rename the labels.

Also, for the long_hpd case, I see that check_link_status() will redo
some of the stuff we already did on this function, such as get_dpcd().
And if you follow my advice on patch 2, you will end up having even
more repeated code. I think you could try to do a careful analysis
here to make sure we're not calling stuff twice here, especially since
some of those operations are potentially slow.

>
>         ret = IRQ_HANDLED;
>
>         goto put_power;
>  mst_fail:
> -       /* if we were in MST mode, and device is not there get out of MST mode */
>         if (intel_dp->is_mst) {
> +               /* if we were in MST mode, and device is not there get out of MST mode */

I don't see the need for changes such as the one above - I saw similar
cases in other patches you submitted. I often use git blame on
comments in order to be able to see the whole context of the change,
and a simple change like this makes it harder to blame. Also, you're
not even fixing the 80 column problem here. And I do prefer the
comment on top of the if statement.


>                 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);
>         }
>  put_power:
> +       /* SST mode - handle short/long pulses here */
> +       if (!intel_dp->is_mst) {
> +               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;
> +       }
>         intel_display_power_put(dev_priv, power_domain);
>
>         return ret;
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 08/13] drm/i915: Support EDID compliance tests with the intel_dp_autotest_edid() function

[Paulo Zanoni]

2015-04-10 14:42 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> Updates the EDID compliance test function to perform the EDID read as
> required by the tests. This read needs to take place in the kernel for
> reasons of speed and efficiency. The results of the EDID read 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
> - Updated to use the raw header corruption detection mechanism
> - Moved the declaration of the test_data variable here
> V5:
> - Update test active flag variable name to match the change in the
>   first patch of the series.
> - Relocated the test active flag declaration and initialization
>   to this patch
> V6:
> - Updated to use the new flag for raw EDID header corruption
> - Removed the extra EDID read from the autotest function
> - Added the edid_checksum variable to struct intel_dp so that the
>   autotest function can write it to the sink device
> - Moved the update to the hpd_pulse function to another patch
> - Removed extraneous constants
> V7:
> - Fixed erroneous placement of the checksum assignment. In some cases
>   such as when the EDID read fails and is NULL, this causes a NULL ptr
>   dereference in the kernel. Bad news. Fixed now.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 46 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  4 ++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index ba2da44..3bfdc40 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -41,6 +41,12 @@
>
>  #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)
>
> +/* Compliance test status bits  */
> +#define INTEL_DP_RESOLUTION_SHIFT_MASK 4
> +#define INTEL_DP_RESOLUTION_PREFERRED  (1 << INTEL_DP_RESOLUTION_SHIFT_MASK)
> +#define INTEL_DP_RESOLUTION_STANDARD   (2 << INTEL_DP_RESOLUTION_SHIFT_MASK)
> +#define INTEL_DP_RESOLUTION_FAILSAFE   (3 << INTEL_DP_RESOLUTION_SHIFT_MASK)
> +
>  struct dp_link_dpll {
>         int link_bw;
>         struct dpll dpll;
> @@ -3770,6 +3776,35 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>  static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
>  {
>         uint8_t test_result = DP_TEST_NAK;
> +       uint32_t ret = 0;
> +
> +       if (intel_dp->compliance_edid_invalid ||
> +           intel_dp->aux.i2c_defer_count > 6) {
> +               /* 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_RESOLUTION_FAILSAFE;

Since this case is expected to happen, shouldn't we return something
that's not DP_TEST_NAK here?

Also, we should try to write the checksum on this case too, shouldn't we?

I see on 4.2.2.6 that the only happy case involves setting the
failsafe resolution, and optionally sending the checksum.

(sorry for not catching this earlier... I just didn't spot the problem)


> +       } else {
> +               ret = drm_dp_dpcd_write(&intel_dp->aux,
> +                                       DP_TEST_EDID_CHECKSUM,
> +                                       &intel_dp->compliance_edid_checksum, 1);
> +               if (ret <= 0)
> +                       DRM_DEBUG_DRIVER("Failed to write EDID checksum\n");
> +               else
> +                       DRM_DEBUG_DRIVER("EDID checksum written to sink\n");
> +
> +               test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
> +               intel_dp->compliance_test_data = INTEL_DP_RESOLUTION_STANDARD;
> +       }
> +
> +       /* Set test active flag here so userspace doesn't interrupt things */
> +       intel_dp->compliance_test_active = 1;
> +
>         return test_result;
>  }
>
> @@ -3785,7 +3820,10 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>         uint8_t rxdata = 0;
>         int status = 0;
>
> +       intel_dp->compliance_test_active = 0;
>         intel_dp->compliance_test_type = 0;
> +       intel_dp->compliance_test_data = 0;
> +
>         intel_dp->aux.i2c_nack_count = 0;
>         intel_dp->aux.i2c_defer_count = 0;
>
> @@ -3920,11 +3958,19 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>
>         /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
>          * 4.2.2.1 - EDID read required for all HPD events
> +        * Also used for 4.2.2.3, 4.2.2.4 4.2.2.5 and 4.2.2.6
> +        *   - compliance_edid_invalid flag used for 4.2.2.6
> +        *   - compliance_edid_checksum used for 4.2.2.3
>           */
> +       intel_dp->compliance_edid_invalid = 0;
>          edid_read = drm_get_edid(connector, adapter);
>          if (!edid_read || connector->edid_header_corrupt == 1) {
>                  DRM_DEBUG_DRIVER("Invalid EDID detected\n");
> +                intel_dp->compliance_edid_invalid = 1;
> +               intel_dp->compliance_edid_checksum = 0;
>          }
> +       else
> +               intel_dp->compliance_edid_checksum = edid_read->checksum;
>
>         /* Try to read the source of the interrupt */
>         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ab811e5..23030a1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -650,6 +650,10 @@ struct intel_dp {
>
>         /* Displayport compliance testing */
>         unsigned long compliance_test_type;
> +       unsigned long compliance_test_data;
> +       bool compliance_test_active;
> +       bool compliance_edid_invalid;
> +       uint8_t compliance_edid_checksum;
>  };
>
>  struct intel_digital_port {
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 04/13] drm/i915: Add EDID read in intel_dp_check_link_status() for Link CTS 4.2.2.1

[Paulo Zanoni]

2015-04-13 11:53 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the
> Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for
> all HPD plug events. To reduce the amount of code, this EDID read is also
> used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual
> support for these tests is implemented in later patches in this series.
>
> V2:
> - Fixed compilation error introduced during rework
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 23184b0..75df3e2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3890,6 +3890,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  {
>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
>         struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> +       struct drm_connector *connector = &intel_dp->attached_connector->base;
> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
> +       struct edid *edid_read = NULL;
>         u8 sink_irq_vector;
>         u8 link_status[DP_LINK_STATUS_SIZE];
>
> @@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>                 return;
>         }
>
> +       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
> +        * 4.2.2.1 - EDID read required for all HPD events
> +         */
> +        edid_read = drm_get_edid(connector, adapter);
> +        if (!edid_read) {
> +                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
> +        }
> +

We already briefly discussed this patch in private, so I'm going to
summarize the discussion and also add some more points here.

Frist, the actual detailed review: the indentation here is using
spaces and we're leaking the EDID. This will cause rebases to a few of
the next patches.

Back to the hight level architecture: your initial versions of the
series contained just 1 extra EDID read, and it was contained inside
the compliance testing function. Then the versions submitted a few
days ago had 2 extra EDID reads, then after some discussion you
reduced to 1 extra EDID read (the one on this patch). I previously
asked "But what about the automatic EDID read we do when we get a
hotplug? Can't we just rely on it?". I got some answers to the
question, but I was not really convinced.

Yesterday I was arguing that this extra EDID read is going to add a
small delay to every hotplug event we get, so my initial suggestion
was to organize the compliance testing in a way that would require the
user space program to call the GetResources() IOCTL to force the EDID
when needed. Your argument was that then the DP compliance testing
procedure would be testing our app for compliance, not the Kernel.

But today I decided to finally do some debugging regarding this, and I
was able to confirm that we do follow the DP requirements: we do have
an automatic EDID read done by the Kernel whenever we do a hotplug:
i915_hotplug_work_func() calls intel_dp_detect(), which ends calling
drm_get_edid() at some point. This function also does other stuff that
is required by the compliance testing, such as the DPCD reads.

Now there's a problem with using i915_hotplug_work_func(), which could
the reason why you rejected it: it only happens after
intel_dp_hpd_pulse(), which means that we only really do the EDID read
after intel_dp_handle_test_request().

I consider i915_hotplug_work_func() a fundamental part of our DP
framework, and the DP compliance testing seems to be just ignoring its
existence. So my idea for a solution here would be to make
intel_dp_handle_test_request() run on its own delayed work function.
It would wait for both i915_digport_work_func() and
i915_hotplug_work_func() to finish, and only then it would do the
normal processing. With this, we would be able to avoid the edid read
on this patch, we would maybe be able to avoid at least part of patch
2, we would maybe be able to completely avoid patch 7, and then on
patch 8 we would start touching intel_dp_get_edid() instead.

I know this is sort of a fundamental change that is being requested a
little late in the review process, and it can be frustrating, but this
aspect of the code only recently changed (I was fine with the EDID
reads just in the compliance testing function), and since the DP
compliance code is quite complex, it took me a while to realize
everything that's going on and what is the purpose of each piece. I
also think that, since this idea will allow the compliance testing to
take into consideration the work done by i915_hotplug_work_func(),
compliance testing will better reflect the behavior that is actually
done by the Kernel when DP devices are plugged/unplugged. And I did
ask about those new EDID reads as soon as I started reviewing the
patch that introduced them.

Now, since I know how frustrating it is to have to change a
significant portion of the code once again, I will leave to the
maintainers the decision of whether the current proposed
implementation is acceptable or if we want to make the DP compliance
testing code take into consideration the work done by
i915_hotplug_work_func(). I would also like to know your opinion on
this. Maybe my idea just doesn't make sense because of something else
I didn't realize :)

>         /* Try to read the source of the interrupt */
>         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>             intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 07/11] drm/i915: Update intel_dp_hpd_pulse() for non-MST operation

[Todd Previte]

On 4/14/15 4:29 AM, Paulo Zanoni wrote:
> 2015-04-10 13:12 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> 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
>> V5:
>> - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8
>> V6:
>> - Reformatted a comment
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++-----------
>>   1 file changed, 8 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 77b6b15..ba2da44 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4572,29 +4572,26 @@ 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);
>> -               }
>>          }
> Shouldn't the code be moved to exactly this spot instead of after the
> put_power label? Why would we want to call check_link_status in case
> we goto mst_fail? In case there is a valid reason, maybe it would be
> better to do a big reorganization of this function because it's going
> to start looking very weird - or at least rename the labels.
No because then you don't get long pulses, only short ones. The 
put_power case is where this belongs, unless you want to duplicate code 
in both the long_pulse and the else clause. There is a separate 
mst_check_link_status call so this one is specific to SST mode. There is 
also a check to make sure it doesn't get called when MST is active and 
MST has hit a failure mode, so that is a non-issue.
> Also, for the long_hpd case, I see that check_link_status() will redo
> some of the stuff we already did on this function, such as get_dpcd().
> And if you follow my advice on patch 2, you will end up having even
> more repeated code. I think you could try to do a careful analysis
> here to make sure we're not calling stuff twice here, especially since
> some of those operations are potentially slow.
I see a couple places where the code is duplicated, specifically the 
connection check (which I encapsulated in a function and I'll likely 
roll forward into this one since it makes things more clear) and the 
DPCD read in the long pulse case. I removed the code in 
check_link_status for both of these things and it still passes 
compliance. Good catch Paulo. This has been fixed and tested and will be 
in the updated patch posted shortly.
>>          ret = IRQ_HANDLED;
>>
>>          goto put_power;
>>   mst_fail:
>> -       /* if we were in MST mode, and device is not there get out of MST mode */
>>          if (intel_dp->is_mst) {
>> +               /* if we were in MST mode, and device is not there get out of MST mode */
> I don't see the need for changes such as the one above - I saw similar
> cases in other patches you submitted. I often use git blame on
> comments in order to be able to see the whole context of the change,
> and a simple change like this makes it harder to blame. Also, you're
> not even fixing the 80 column problem here. And I do prefer the
> comment on top of the if statement.
This is just an artifact of moving things around, as it likely was in 
the other patches. The only reason I will move comments is to clarify 
what they pertain to if code is moving around it. It's back where it 
belongs now so it doesn't even show up in the patch. Fixed for the next 
version.
>
>>                  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);
>>          }
>>   put_power:
>> +       /* SST mode - handle short/long pulses here */
>> +       if (!intel_dp->is_mst) {
>> +               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;
>> +       }
>>          intel_display_power_put(dev_priv, power_domain);
>>
>>          return ret;
>> --
>> 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 08/13] drm/i915: Support EDID compliance tests with the intel_dp_autotest_edid() function

[Todd Previte]

On 4/14/15 6:35 AM, Paulo Zanoni wrote:
> 2015-04-10 14:42 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> Updates the EDID compliance test function to perform the EDID read as
>> required by the tests. This read needs to take place in the kernel for
>> reasons of speed and efficiency. The results of the EDID read 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
>> - Updated to use the raw header corruption detection mechanism
>> - Moved the declaration of the test_data variable here
>> V5:
>> - Update test active flag variable name to match the change in the
>>    first patch of the series.
>> - Relocated the test active flag declaration and initialization
>>    to this patch
>> V6:
>> - Updated to use the new flag for raw EDID header corruption
>> - Removed the extra EDID read from the autotest function
>> - Added the edid_checksum variable to struct intel_dp so that the
>>    autotest function can write it to the sink device
>> - Moved the update to the hpd_pulse function to another patch
>> - Removed extraneous constants
>> V7:
>> - Fixed erroneous placement of the checksum assignment. In some cases
>>    such as when the EDID read fails and is NULL, this causes a NULL ptr
>>    dereference in the kernel. Bad news. Fixed now.
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c  | 46 ++++++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h |  4 ++++
>>   2 files changed, 50 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index ba2da44..3bfdc40 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -41,6 +41,12 @@
>>
>>   #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)
>>
>> +/* Compliance test status bits  */
>> +#define INTEL_DP_RESOLUTION_SHIFT_MASK 4
>> +#define INTEL_DP_RESOLUTION_PREFERRED  (1 << INTEL_DP_RESOLUTION_SHIFT_MASK)
>> +#define INTEL_DP_RESOLUTION_STANDARD   (2 << INTEL_DP_RESOLUTION_SHIFT_MASK)
>> +#define INTEL_DP_RESOLUTION_FAILSAFE   (3 << INTEL_DP_RESOLUTION_SHIFT_MASK)
>> +
>>   struct dp_link_dpll {
>>          int link_bw;
>>          struct dpll dpll;
>> @@ -3770,6 +3776,35 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>>   static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
>>   {
>>          uint8_t test_result = DP_TEST_NAK;
>> +       uint32_t ret = 0;
>> +
>> +       if (intel_dp->compliance_edid_invalid ||
>> +           intel_dp->aux.i2c_defer_count > 6) {
>> +               /* 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_RESOLUTION_FAILSAFE;
> Since this case is expected to happen, shouldn't we return something
> that's not DP_TEST_NAK here?
The spec does not state whether or not we should ACK or NAK this case. 
This value is written to the sink device as the response, so for 
compliance testing purposes, the test device does not appear to care 
whether it's ACK or NAK.
>
> Also, we should try to write the checksum on this case too, shouldn't we?
>
> I see on 4.2.2.6 that the only happy case involves setting the
> failsafe resolution, and optionally sending the checksum.
>
> (sorry for not catching this earlier... I just didn't spot the problem)
Correct. Writing the bad checksum is optional, so I opted not to. Less 
code, less AUX transactions.
>
>> +       } else {
>> +               ret = drm_dp_dpcd_write(&intel_dp->aux,
>> +                                       DP_TEST_EDID_CHECKSUM,
>> +                                       &intel_dp->compliance_edid_checksum, 1);
>> +               if (ret <= 0)
>> +                       DRM_DEBUG_DRIVER("Failed to write EDID checksum\n");
>> +               else
>> +                       DRM_DEBUG_DRIVER("EDID checksum written to sink\n");
>> +
>> +               test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
>> +               intel_dp->compliance_test_data = INTEL_DP_RESOLUTION_STANDARD;
>> +       }
>> +
>> +       /* Set test active flag here so userspace doesn't interrupt things */
>> +       intel_dp->compliance_test_active = 1;
>> +
>>          return test_result;
>>   }
>>
>> @@ -3785,7 +3820,10 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>>          uint8_t rxdata = 0;
>>          int status = 0;
>>
>> +       intel_dp->compliance_test_active = 0;
>>          intel_dp->compliance_test_type = 0;
>> +       intel_dp->compliance_test_data = 0;
>> +
>>          intel_dp->aux.i2c_nack_count = 0;
>>          intel_dp->aux.i2c_defer_count = 0;
>>
>> @@ -3920,11 +3958,19 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>
>>          /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
>>           * 4.2.2.1 - EDID read required for all HPD events
>> +        * Also used for 4.2.2.3, 4.2.2.4 4.2.2.5 and 4.2.2.6
>> +        *   - compliance_edid_invalid flag used for 4.2.2.6
>> +        *   - compliance_edid_checksum used for 4.2.2.3
>>            */
>> +       intel_dp->compliance_edid_invalid = 0;
>>           edid_read = drm_get_edid(connector, adapter);
>>           if (!edid_read || connector->edid_header_corrupt == 1) {
>>                   DRM_DEBUG_DRIVER("Invalid EDID detected\n");
>> +                intel_dp->compliance_edid_invalid = 1;
>> +               intel_dp->compliance_edid_checksum = 0;
>>           }
>> +       else
>> +               intel_dp->compliance_edid_checksum = edid_read->checksum;
>>
>>          /* Try to read the source of the interrupt */
>>          if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index ab811e5..23030a1 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -650,6 +650,10 @@ struct intel_dp {
>>
>>          /* Displayport compliance testing */
>>          unsigned long compliance_test_type;
>> +       unsigned long compliance_test_data;
>> +       bool compliance_test_active;
>> +       bool compliance_edid_invalid;
>> +       uint8_t compliance_edid_checksum;
>>   };
>>
>>   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 03/11] drm/i915: Move the DPCD read further up in intel_dp_check_link_status()

[Todd Previte]

Trying again and adding the list this time instead of just replying to 
myself....

This is just an artifact of moving things around, as it likely was in 
the other patches. The only reason I will move comments is to clarify 
what they pertain to if code is moving around it. In any case, it's back 
where it belongs in the updated patch.

-T

On 4/10/15 9:12 AM, Todd Previte wrote:
> 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.
>
> V1:
> - This is the second part of the single-patch split previously
>    mentioned.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 30cd433..23184b0 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3913,13 +3913,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);

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

Re: [PATCH 07/11] drm/i915: Update intel_dp_hpd_pulse() for non-MST operation

[Paulo Zanoni]

2015-04-14 14:36 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>
>
> On 4/14/15 4:29 AM, Paulo Zanoni wrote:
>>
>> 2015-04-10 13:12 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>>>
>>> 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
>>> V5:
>>> - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8
>>> V6:
>>> - Reformatted a comment
>>>
>>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++-----------
>>>   1 file changed, 8 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index 77b6b15..ba2da44 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -4572,29 +4572,26 @@ 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);
>>> -               }
>>>          }
>>
>> Shouldn't the code be moved to exactly this spot instead of after the
>> put_power label? Why would we want to call check_link_status in case
>> we goto mst_fail? In case there is a valid reason, maybe it would be
>> better to do a big reorganization of this function because it's going
>> to start looking very weird - or at least rename the labels.
>
> No because then you don't get long pulses, only short ones.

No, what I mean is:

if (long_hpd) {
    ... code ...
} else {
    ... code ....
}

if (!intel_dp->is_mst) {
    drm_modeset_lock(...)
    ... code ...
}

mst_fail:
    ... code ...

The other problem I point is: imagine we're SST and we get a long_hpd.
Then we run ibx_digital_port_connected(), and since the monitor is
disconnected we "goto mst_fail". We'll end up running
intel_dp_check_link_status() before returning, but we really shouldn't
run it since we know the monitor is disconnected.

> The put_power
> case is where this belongs, unless you want to duplicate code in both the
> long_pulse and the else clause. There is a separate mst_check_link_status
> call so this one is specific to SST mode. There is also a check to make sure
> it doesn't get called when MST is active and MST has hit a failure mode, so
> that is a non-issue.
>>
>> Also, for the long_hpd case, I see that check_link_status() will redo
>> some of the stuff we already did on this function, such as get_dpcd().
>> And if you follow my advice on patch 2, you will end up having even
>> more repeated code. I think you could try to do a careful analysis
>> here to make sure we're not calling stuff twice here, especially since
>> some of those operations are potentially slow.
>
> I see a couple places where the code is duplicated, specifically the
> connection check (which I encapsulated in a function and I'll likely roll
> forward into this one since it makes things more clear) and the DPCD read in
> the long pulse case. I removed the code in check_link_status for both of
> these things and it still passes compliance. Good catch Paulo. This has been
> fixed and tested and will be in the updated patch posted shortly.
>>>
>>>          ret = IRQ_HANDLED;
>>>
>>>          goto put_power;
>>>   mst_fail:
>>> -       /* if we were in MST mode, and device is not there get out of MST
>>> mode */
>>>          if (intel_dp->is_mst) {
>>> +               /* if we were in MST mode, and device is not there get
>>> out of MST mode */
>>
>> I don't see the need for changes such as the one above - I saw similar
>> cases in other patches you submitted. I often use git blame on
>> comments in order to be able to see the whole context of the change,
>> and a simple change like this makes it harder to blame. Also, you're
>> not even fixing the 80 column problem here. And I do prefer the
>> comment on top of the if statement.
>
> This is just an artifact of moving things around, as it likely was in the
> other patches. The only reason I will move comments is to clarify what they
> pertain to if code is moving around it. It's back where it belongs now so it
> doesn't even show up in the patch. Fixed for the next version.
>
>>
>>>                  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);
>>>          }
>>>   put_power:
>>> +       /* SST mode - handle short/long pulses here */
>>> +       if (!intel_dp->is_mst) {
>>> +               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;
>>> +       }
>>>          intel_display_power_put(dev_priv, power_domain);
>>>
>>>          return ret;
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>



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

Re: [PATCH 07/11] drm/i915: Update intel_dp_hpd_pulse() for non-MST operation

[Todd Previte]

On 4/14/15 12:00 PM, Paulo Zanoni wrote:
> 2015-04-14 14:36 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>>
>> On 4/14/15 4:29 AM, Paulo Zanoni wrote:
>>> 2015-04-10 13:12 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>>>> 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
>>>> V5:
>>>> - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8
>>>> V6:
>>>> - Reformatted a comment
>>>>
>>>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++-----------
>>>>    1 file changed, 8 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 77b6b15..ba2da44 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -4572,29 +4572,26 @@ 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);
>>>> -               }
>>>>           }
>>> Shouldn't the code be moved to exactly this spot instead of after the
>>> put_power label? Why would we want to call check_link_status in case
>>> we goto mst_fail? In case there is a valid reason, maybe it would be
>>> better to do a big reorganization of this function because it's going
>>> to start looking very weird - or at least rename the labels.
>> No because then you don't get long pulses, only short ones.
> No, what I mean is:
>
> if (long_hpd) {
>      ... code ...
> } else {
>      ... code ....
> }
>
> if (!intel_dp->is_mst) {
>      drm_modeset_lock(...)
>      ... code ...
> }
>
> mst_fail:
>      ... code ...
>
> The other problem I point is: imagine we're SST and we get a long_hpd.
> Then we run ibx_digital_port_connected(), and since the monitor is
> disconnected we "goto mst_fail". We'll end up running
> intel_dp_check_link_status() before returning, but we really shouldn't
> run it since we know the monitor is disconnected.
>
I see what you're saying, however under normal operation for SST (when 
connected and everything is working) the code will hit this line:

if (!intel_dp_probe_mst(intel_dp))

And proceed to the mst_fail block, thus skipping that block of code 
entirely and missing the SST handler. The result is a missed long pulse 
for the SST case.

Your second point has validity though. This can be addressed with a 
"connected" flag just before the if (long_pulse) statement:

     connected = intel_dp_digital_port_connected(intel_dp);
     if (!connected)
         goto mst_fail;

The pulse handler for the most part can then be skipped, since the 
device is gone. In mst_fail, the MST topology manager is updated though, 
so that still has to happen. With the SST pulse handler in put_power, it 
can simply fall through now and hit the updated if-statement there which is:

if (!intel_dp->is_mst && connected) {
         ... code ...
}

And all should be well for a disconnected device as well as normal 
operational modes of SST and MST.

Oh the intel_dp_digital_port_connected() function is just the 
encapsulation of this code from the long_pulse segment:

         if (HAS_PCH_SPLIT(dev)) {
             if (!ibx_digital_port_connected(dev_priv, intel_dig_port))
                 goto mst_fail;
         } else {
             if (g4x_digital_port_connected(dev, intel_dig_port) != 1)
                 goto mst_fail;
         }

And it's been added to the updated patch.

>> The put_power
>> case is where this belongs, unless you want to duplicate code in both the
>> long_pulse and the else clause. There is a separate mst_check_link_status
>> call so this one is specific to SST mode. There is also a check to make sure
>> it doesn't get called when MST is active and MST has hit a failure mode, so
>> that is a non-issue.
>>> Also, for the long_hpd case, I see that check_link_status() will redo
>>> some of the stuff we already did on this function, such as get_dpcd().
>>> And if you follow my advice on patch 2, you will end up having even
>>> more repeated code. I think you could try to do a careful analysis
>>> here to make sure we're not calling stuff twice here, especially since
>>> some of those operations are potentially slow.
>> I see a couple places where the code is duplicated, specifically the
>> connection check (which I encapsulated in a function and I'll likely roll
>> forward into this one since it makes things more clear) and the DPCD read in
>> the long pulse case. I removed the code in check_link_status for both of
>> these things and it still passes compliance. Good catch Paulo. This has been
>> fixed and tested and will be in the updated patch posted shortly.
>>>>           ret = IRQ_HANDLED;
>>>>
>>>>           goto put_power;
>>>>    mst_fail:
>>>> -       /* if we were in MST mode, and device is not there get out of MST
>>>> mode */
>>>>           if (intel_dp->is_mst) {
>>>> +               /* if we were in MST mode, and device is not there get
>>>> out of MST mode */
>>> I don't see the need for changes such as the one above - I saw similar
>>> cases in other patches you submitted. I often use git blame on
>>> comments in order to be able to see the whole context of the change,
>>> and a simple change like this makes it harder to blame. Also, you're
>>> not even fixing the 80 column problem here. And I do prefer the
>>> comment on top of the if statement.
>> This is just an artifact of moving things around, as it likely was in the
>> other patches. The only reason I will move comments is to clarify what they
>> pertain to if code is moving around it. It's back where it belongs now so it
>> doesn't even show up in the patch. Fixed for the next version.
>>
>>>>                   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);
>>>>           }
>>>>    put_power:
>>>> +       /* SST mode - handle short/long pulses here */
>>>> +       if (!intel_dp->is_mst) {
>>>> +               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;
>>>> +       }
>>>>           intel_display_power_put(dev_priv, power_domain);
>>>>
>>>>           return ret;
>>>> --
>>>> 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 06/13] drm: Add supporting structure for Displayport Link CTS test 4.2.2.6

[Todd Previte]

On 4/13/15 3:18 PM, Paulo Zanoni wrote:
> 2015-04-13 11:53 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> Displayport compliance test 4.2.2.6 requires that a source device be capable of
>> detecting a corrupt EDID. The test specification states that the sink device
>> sets up the EDID with an invalid checksum. To do this, the sink sets up an
>> invalid EDID header, expecting the source device to generate the checksum and
>> compare it to the value stored in the last byte of the block data.
>>
>> Unfortunately, the DRM EDID reading and parsing functions are actually too good
>> in this case; the header is fixed before the checksum is computed and thus the
>> code never sees the invalid checksum. This results in a failure to pass the
>> compliance test.
>>
>> To correct this issue, a checksum is generated when the EDID header is detected
>> as corrupted. If the checksum is invalid, it sets the header_corrupt flag and
>> logs the errors. In the case of a more seriously damaged header (fixup score
>> less than the threshold) the code does not generate the checksum but does set
>> the header_corrupt flag.
>>
>> V2:
>> - Removed the static bool global
>> - Added a bool to the drm_connector struct to reaplce the static one for
>>    holding the status of raw edid header corruption detection
>> - Modified the function signature of the is_valid function to take an
>>    additional parameter to store the corruption detected value
>> - Fixed the other callers of the above is_valid function
>> V3:
>> - Updated the commit message to be more clear about what and why this
>>    patch does what it does.
>> - Added comment in code to clarify the operations there
>> - Removed compliance variable and check_link_status update; those
>>    have been moved to a later patch
>> - Removed variable assignment from the bottom of the test handler
>> V4:
>> - Removed i915 tag from subject line as the patch is not i915-specific
>> V5:
>> - Moved code causing a compilation error to this patch where the variable
>>    is actually declared
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> Cc: dri-devel@lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/drm_edid.c        | 31 ++++++++++++++++++++++++++-----
>>   drivers/gpu/drm/drm_edid_load.c   |  7 +++++--
>>   drivers/gpu/drm/i2c/tda998x_drv.c |  4 ++--
>>   drivers/gpu/drm/i915/intel_dp.c   |  2 +-
>>   include/drm/drm_crtc.h            |  8 +++++++-
>>   5 files changed, 41 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 53bc7a6..12e5be7 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -1005,7 +1005,6 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
>>          for (i = 0; i < sizeof(edid_header); i++)
>>                  if (raw_edid[i] == edid_header[i])
>>                          score++;
>> -
>>          return score;
>>   }
>>   EXPORT_SYMBOL(drm_edid_header_is_valid);
> Bad chunk...
Fixed
>
>> @@ -1047,7 +1046,8 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
>>    *
>>    * Return: True if the block is valid, false otherwise.
>>    */
>> -bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
>> +bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
>> +                         bool *header_corrupt)
> Need to add the new parameter description to the documentation above.
Done.
>
>>   {
>>          u8 csum;
>>          struct edid *edid = (struct edid *)raw_edid;
>> @@ -1062,9 +1062,27 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
>>                  int score = drm_edid_header_is_valid(raw_edid);
>>                  if (score == 8) ;
>>                  else if (score >= edid_fixup) {
>> +                       /* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6
>> +                        * In order to properly generate the invalid checksum
>> +                        * required for this test, it must be generated using
>> +                        * the raw EDID data. Otherwise, the fix-up code here
>> +                        * will correct the problem, the checksum is then correct
>> +                        * and the test fails
>> +                        */
>> +                       csum = drm_edid_block_checksum(raw_edid);
>> +                       if (csum) {
>> +                               DRM_DEBUG_DRIVER("Invalid EDID header, score = %d\n", score);
>> +                               DRM_DEBUG_DRIVER("Invalid EDID checksum %d\n", csum);
> No one on this file uses DRM_DEBUG_DRIVER (you use 2 calls here and one below).
>
> Also, during "normal operation" we try to calculate the checksum based
> on the fixed EDID header, so if we also print these messages here
> we're always going to have a message complaining about invalid
> checksum: either this one or the other that's already there. My
> bikeshed would be to just remove the messages you added here and below
> to not confuse users. Let's assume that the bad header is due to some
> communication/corruption error, and the HW manufacturers did not
> program an EDID with a bad header and a correct checksum based on bad
> header :)
Works for me. Messages have been removed.
>
>> +                               if (header_corrupt)
>> +                                       *header_corrupt = 1;
>> +                       }
>>                          DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
>>                          memcpy(raw_edid, edid_header, sizeof(edid_header));
>>                  } else {
>> +                       if (header_corrupt) {
>> +                               DRM_DEBUG_DRIVER("Invalid EDID header\n");
>> +                               *header_corrupt = 1;
>> +                       }
> We don't ever set header_corrupt back to false, so if we ever get a
> corrupt header once, the header_corrupt flag will stay there even if
> it gets fixed somehow (such as a DP compliance tester starting to
> submit the correct header).
This is fixed as well. The flag now is reset to 0 at the end of the test 
handler.
>
>>                          goto bad;
>>                  }
>>          }
>> @@ -1129,7 +1147,7 @@ bool drm_edid_is_valid(struct edid *edid)
>>                  return false;
>>
>>          for (i = 0; i <= edid->extensions; i++)
>> -               if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
>> +               if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true, NULL))
>>                          return false;
>>
>>          return true;
>> @@ -1232,7 +1250,8 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>>          for (i = 0; i < 4; i++) {
>>                  if (get_edid_block(data, block, 0, EDID_LENGTH))
>>                          goto out;
>> -               if (drm_edid_block_valid(block, 0, print_bad_edid))
>> +               if (drm_edid_block_valid(block, 0, print_bad_edid,
>> +                                        &connector->edid_header_corrupt))
>>                          break;
>>                  if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
>>                          connector->null_edid_counter++;
>> @@ -1257,7 +1276,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>>                                    block + (valid_extensions + 1) * EDID_LENGTH,
>>                                    j, EDID_LENGTH))
>>                                  goto out;
>> -                       if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
>> +                       if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j,
>> +                                                print_bad_edid,
>> +                                                &connector->edid_header_corrupt)) {
> You can use NULL here since we're not handling block 0 anymore.
Fixed.
>
>>                                  valid_extensions++;
>>                                  break;
>>                          }
>> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
>> index 732cb6f..1505494 100644
>> --- a/drivers/gpu/drm/drm_edid_load.c
>> +++ b/drivers/gpu/drm/drm_edid_load.c
>> @@ -216,7 +216,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
>>                  goto out;
>>          }
>>
>> -       if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
>> +       if (!drm_edid_block_valid(edid, 0, print_bad_edid,
>> +                                 &connector->edid_header_corrupt)) {
>>                  connector->bad_edid_counter++;
>>                  DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
>>                      name);
>> @@ -229,7 +230,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
>>                  if (i != valid_extensions + 1)
>>                          memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
>>                              edid + i * EDID_LENGTH, EDID_LENGTH);
>> -               if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, print_bad_edid))
>> +               if (drm_edid_block_valid(edid + i * EDID_LENGTH, i,
>> +                                        print_bad_edid,
>> +                                        &connector->edid_header_corrupt))
> Same here: not iterating over block 0.
Fixed
> Btw, why can't we just use NULL on edid_load?
I think it's at too high level again and the fix up code will get in the 
way.
>
>>                          valid_extensions++;
>>          }
>>
>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
>> index a9041d1..9c3d6b3 100644
>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
>> @@ -1106,7 +1106,7 @@ static uint8_t *do_get_edid(struct tda998x_priv *priv)
>>          if (read_edid_block(priv, block, 0))
>>                  goto fail;
>>
>> -       if (!drm_edid_block_valid(block, 0, print_bad_edid))
>> +       if (!drm_edid_block_valid(block, 0, print_bad_edid, NULL))
>>                  goto fail;
>>
>>          /* if there's no extensions, we're done */
>> @@ -1123,7 +1123,7 @@ static uint8_t *do_get_edid(struct tda998x_priv *priv)
>>                  if (read_edid_block(priv, ext_block, j))
>>                          goto fail;
>>
>> -               if (!drm_edid_block_valid(ext_block, j, print_bad_edid))
>> +               if (!drm_edid_block_valid(ext_block, j, print_bad_edid, NULL))
>>                          goto fail;
>>
>>                  valid_extensions++;
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index e1d6e79..77b6b15 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3922,7 +3922,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>           * 4.2.2.1 - EDID read required for all HPD events
>>            */
>>           edid_read = drm_get_edid(connector, adapter);
>> -        if (!edid_read) {
>> +        if (!edid_read || connector->edid_header_corrupt == 1) {
>>                   DRM_DEBUG_DRIVER("Invalid EDID detected\n");
>>           }
>>
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 0261417..e31a4b3 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -719,6 +719,11 @@ struct drm_connector {
>>          int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */
>>          unsigned bad_edid_counter;
>>
>> +       /* Flag for raw EDID header corruption - used in Displayport compliance testing
>> +        * Displayport Link CTS Core 1.2 rev1.1 4.2.2.6
>> +        */
>> +       bool edid_header_corrupt;
>> +
>>          struct dentry *debugfs_entry;
>>
>>          struct drm_connector_state *state;
>> @@ -1436,7 +1441,8 @@ extern void drm_set_preferred_mode(struct drm_connector *connector,
>>                                     int hpref, int vpref);
>>
>>   extern int drm_edid_header_is_valid(const u8 *raw_edid);
>> -extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid);
>> +extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, bool *header_corrupt);
>> +
>>   extern bool drm_edid_is_valid(struct edid *edid);
>>
>>   extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

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

Re: [PATCH 04/13] drm/i915: Add EDID read in intel_dp_check_link_status() for Link CTS 4.2.2.1

[Daniel Vetter]

On Tue, Apr 14, 2015 at 01:53:21PM -0300, Paulo Zanoni wrote:
> ... I will leave to the > maintainers the decision of whether the
> current proposed implementation is acceptable or [not] ...

So this is pretty much a lose-lose proposition for me. I chatted a bit
with Todd about this topic and I don't really have a lot of clue here. So
me making a call will mostly end up in a coin-tossing but and so will
definitely piss off at least someone.

I need to ponder this some more, but I'd really prefer if you can just
come up with a plan yourself. Without knowing all the details I'd guess
adding a FIXME comment and investigating this after patches have landed
(assuming it does indeed happen) seems acceptable to me. But that's really
just my totally ignorant opinion ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/13] drm/i915: Add EDID read in intel_dp_check_link_status() for Link CTS 4.2.2.1

[Todd Previte]

On 4/14/2015 9:53 AM, Paulo Zanoni wrote:
> 2015-04-13 11:53 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the
>> Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for
>> all HPD plug events. To reduce the amount of code, this EDID read is also
>> used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual
>> support for these tests is implemented in later patches in this series.
>>
>> V2:
>> - Fixed compilation error introduced during rework
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 23184b0..75df3e2 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3890,6 +3890,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   {
>>          struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>          struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>> +       struct drm_connector *connector = &intel_dp->attached_connector->base;
>> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
>> +       struct edid *edid_read = NULL;
>>          u8 sink_irq_vector;
>>          u8 link_status[DP_LINK_STATUS_SIZE];
>>
>> @@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>                  return;
>>          }
>>
>> +       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
>> +        * 4.2.2.1 - EDID read required for all HPD events
>> +         */
>> +        edid_read = drm_get_edid(connector, adapter);
>> +        if (!edid_read) {
>> +                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
>> +        }
>> +
> We already briefly discussed this patch in private, so I'm going to
> summarize the discussion and also add some more points here.
>
> Frist, the actual detailed review: the indentation here is using
> spaces and we're leaking the EDID. This will cause rebases to a few of
> the next patches.
>
> Back to the hight level architecture: your initial versions of the
> series contained just 1 extra EDID read, and it was contained inside
> the compliance testing function. Then the versions submitted a few
> days ago had 2 extra EDID reads, then after some discussion you
> reduced to 1 extra EDID read (the one on this patch). I previously
> asked "But what about the automatic EDID read we do when we get a
> hotplug? Can't we just rely on it?". I got some answers to the
> question, but I was not really convinced.
>
> Yesterday I was arguing that this extra EDID read is going to add a
> small delay to every hotplug event we get, so my initial suggestion
> was to organize the compliance testing in a way that would require the
> user space program to call the GetResources() IOCTL to force the EDID
> when needed. Your argument was that then the DP compliance testing
> procedure would be testing our app for compliance, not the Kernel.
>
> But today I decided to finally do some debugging regarding this, and I
> was able to confirm that we do follow the DP requirements: we do have
> an automatic EDID read done by the Kernel whenever we do a hotplug:
> i915_hotplug_work_func() calls intel_dp_detect(), which ends calling
> drm_get_edid() at some point. This function also does other stuff that
> is required by the compliance testing, such as the DPCD reads.
>
> Now there's a problem with using i915_hotplug_work_func(), which could
> the reason why you rejected it: it only happens after
> intel_dp_hpd_pulse(), which means that we only really do the EDID read
> after intel_dp_handle_test_request().
>
> I consider i915_hotplug_work_func() a fundamental part of our DP
> framework, and the DP compliance testing seems to be just ignoring its
> existence. So my idea for a solution here would be to make
> intel_dp_handle_test_request() run on its own delayed work function.
> It would wait for both i915_digport_work_func() and
> i915_hotplug_work_func() to finish, and only then it would do the
> normal processing. With this, we would be able to avoid the edid read
> on this patch, we would maybe be able to avoid at least part of patch
> 2, we would maybe be able to completely avoid patch 7, and then on
> patch 8 we would start touching intel_dp_get_edid() instead.
>
> I know this is sort of a fundamental change that is being requested a
> little late in the review process, and it can be frustrating, but this
> aspect of the code only recently changed (I was fine with the EDID
> reads just in the compliance testing function), and since the DP
> compliance code is quite complex, it took me a while to realize
> everything that's going on and what is the purpose of each piece. I
> also think that, since this idea will allow the compliance testing to
> take into consideration the work done by i915_hotplug_work_func(),
> compliance testing will better reflect the behavior that is actually
> done by the Kernel when DP devices are plugged/unplugged. And I did
> ask about those new EDID reads as soon as I started reviewing the
> patch that introduced them.
>
> Now, since I know how frustrating it is to have to change a
> significant portion of the code once again, I will leave to the
> maintainers the decision of whether the current proposed
> implementation is acceptable or if we want to make the DP compliance
> testing code take into consideration the work done by
> i915_hotplug_work_func(). I would also like to know your opinion on
> this. Maybe my idea just doesn't make sense because of something else
> I didn't realize :)
I don't think this is a good idea. The work loop aspect seems like a 
very complex solution solution to a problem that is relatively simple. 
In a discussion with Daniel, he indicated that adding a work loop is 
something to be avoided unless it's *really* necessary, as they are 
prone to race conditions. In this case, I just don't see that it's 
necessary. Additionally, you have been keen to note invasive solutions 
and this seems like a highly invasive solution especially in light of 
having a viable one right here.

This solution adds very little overhead along the HPD path and that 
overhead is a single read of the EDID for each event. To further address 
any concerns about performance, for V6 I've propagated the long_hpd flag 
forward into check_link_status so that the EDID read is only called for 
a hot plug event, not short pulses. Another plus to this solution is 
that if/when this needs to moved to userspace, this one is a lot easier 
to unwind and pull out than the work loop you're suggesting.

With respect to the user space solution, that's something worth 
investigating in the future. I have some concerns about doing that, the 
primary one being the ability to detect the corrupted header, but I 
think it's something to consider for a follow-on update.


>>          /* Try to read the source of the interrupt */
>>          if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>              intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

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

Re: [PATCH 04/13] drm/i915: Add EDID read in intel_dp_check_link_status() for Link CTS 4.2.2.1

[Paulo Zanoni]

2015-04-15 12:37 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>
>
> On 4/14/2015 9:53 AM, Paulo Zanoni wrote:
>>
>> 2015-04-13 11:53 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>>>
>>> Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in
>>> the
>>> Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for
>>> all HPD plug events. To reduce the amount of code, this EDID read is also
>>> used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual
>>> support for these tests is implemented in later patches in this series.
>>>
>>> V2:
>>> - Fixed compilation error introduced during rework
>>>
>>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index 23184b0..75df3e2 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -3890,6 +3890,9 @@ intel_dp_check_link_status(struct intel_dp
>>> *intel_dp)
>>>   {
>>>          struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>          struct intel_encoder *intel_encoder =
>>> &dp_to_dig_port(intel_dp)->base;
>>> +       struct drm_connector *connector =
>>> &intel_dp->attached_connector->base;
>>> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
>>> +       struct edid *edid_read = NULL;
>>>          u8 sink_irq_vector;
>>>          u8 link_status[DP_LINK_STATUS_SIZE];
>>>
>>> @@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp
>>> *intel_dp)
>>>                  return;
>>>          }
>>>
>>> +       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
>>> +        * 4.2.2.1 - EDID read required for all HPD events
>>> +         */
>>> +        edid_read = drm_get_edid(connector, adapter);
>>> +        if (!edid_read) {
>>> +                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
>>> +        }
>>> +
>>
>> We already briefly discussed this patch in private, so I'm going to
>> summarize the discussion and also add some more points here.
>>
>> Frist, the actual detailed review: the indentation here is using
>> spaces and we're leaking the EDID. This will cause rebases to a few of
>> the next patches.
>>
>> Back to the hight level architecture: your initial versions of the
>> series contained just 1 extra EDID read, and it was contained inside
>> the compliance testing function. Then the versions submitted a few
>> days ago had 2 extra EDID reads, then after some discussion you
>> reduced to 1 extra EDID read (the one on this patch). I previously
>> asked "But what about the automatic EDID read we do when we get a
>> hotplug? Can't we just rely on it?". I got some answers to the
>> question, but I was not really convinced.
>>
>> Yesterday I was arguing that this extra EDID read is going to add a
>> small delay to every hotplug event we get, so my initial suggestion
>> was to organize the compliance testing in a way that would require the
>> user space program to call the GetResources() IOCTL to force the EDID
>> when needed. Your argument was that then the DP compliance testing
>> procedure would be testing our app for compliance, not the Kernel.
>>
>> But today I decided to finally do some debugging regarding this, and I
>> was able to confirm that we do follow the DP requirements: we do have
>> an automatic EDID read done by the Kernel whenever we do a hotplug:
>> i915_hotplug_work_func() calls intel_dp_detect(), which ends calling
>> drm_get_edid() at some point. This function also does other stuff that
>> is required by the compliance testing, such as the DPCD reads.
>>
>> Now there's a problem with using i915_hotplug_work_func(), which could
>> the reason why you rejected it: it only happens after
>> intel_dp_hpd_pulse(), which means that we only really do the EDID read
>> after intel_dp_handle_test_request().
>>
>> I consider i915_hotplug_work_func() a fundamental part of our DP
>> framework, and the DP compliance testing seems to be just ignoring its
>> existence. So my idea for a solution here would be to make
>> intel_dp_handle_test_request() run on its own delayed work function.
>> It would wait for both i915_digport_work_func() and
>> i915_hotplug_work_func() to finish, and only then it would do the
>> normal processing. With this, we would be able to avoid the edid read
>> on this patch, we would maybe be able to avoid at least part of patch
>> 2, we would maybe be able to completely avoid patch 7, and then on
>> patch 8 we would start touching intel_dp_get_edid() instead.
>>
>> I know this is sort of a fundamental change that is being requested a
>> little late in the review process, and it can be frustrating, but this
>> aspect of the code only recently changed (I was fine with the EDID
>> reads just in the compliance testing function), and since the DP
>> compliance code is quite complex, it took me a while to realize
>> everything that's going on and what is the purpose of each piece. I
>> also think that, since this idea will allow the compliance testing to
>> take into consideration the work done by i915_hotplug_work_func(),
>> compliance testing will better reflect the behavior that is actually
>> done by the Kernel when DP devices are plugged/unplugged. And I did
>> ask about those new EDID reads as soon as I started reviewing the
>> patch that introduced them.
>>
>> Now, since I know how frustrating it is to have to change a
>> significant portion of the code once again, I will leave to the
>> maintainers the decision of whether the current proposed
>> implementation is acceptable or if we want to make the DP compliance
>> testing code take into consideration the work done by
>> i915_hotplug_work_func(). I would also like to know your opinion on
>> this. Maybe my idea just doesn't make sense because of something else
>> I didn't realize :)
>
> I don't think this is a good idea. The work loop aspect seems like a very
> complex solution solution to a problem that is relatively simple. In a
> discussion with Daniel, he indicated that adding a work loop is something to
> be avoided unless it's *really* necessary, as they are prone to race
> conditions. In this case, I just don't see that it's necessary.

The workqueue thing was just an idea to implement a solution for the
real problem. I think we should be focusing about discussing the fact
that we're not taking i915_hotplug_work_func() into consideration when
doing the compliance testing, not on the fact that one of the possible
implementations could use a workqueue. I'd still like to hear your
arguments on that.


> Additionally, you have been keen to note invasive solutions and this seems
> like a highly invasive solution especially in light of having a viable one
> right here.
>
> This solution adds very little overhead along the HPD path and that overhead
> is a single read of the EDID for each event. To further address any concerns
> about performance, for V6 I've propagated the long_hpd flag forward into
> check_link_status so that the EDID read is only called for a hot plug event,
> not short pulses. Another plus to this solution is that if/when this needs
> to moved to userspace, this one is a lot easier to unwind and pull out than
> the work loop you're suggesting.
>
> With respect to the user space solution, that's something worth
> investigating in the future. I have some concerns about doing that, the
> primary one being the ability to detect the corrupted header, but I think
> it's something to consider for a follow-on update.
>
>
>
>>>          /* Try to read the source of the interrupt */
>>>          if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>>              intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>



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

Re: [PATCH 04/13] drm/i915: Add EDID read in intel_dp_check_link_status() for Link CTS 4.2.2.1

[Todd Previte]

On 4/15/2015 10:42 AM, Paulo Zanoni wrote:
> 2015-04-15 12:37 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>>
>> On 4/14/2015 9:53 AM, Paulo Zanoni wrote:
>>> 2015-04-13 11:53 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>>>> Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in
>>>> the
>>>> Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for
>>>> all HPD plug events. To reduce the amount of code, this EDID read is also
>>>> used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual
>>>> support for these tests is implemented in later patches in this series.
>>>>
>>>> V2:
>>>> - Fixed compilation error introduced during rework
>>>>
>>>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
>>>>    1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 23184b0..75df3e2 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -3890,6 +3890,9 @@ intel_dp_check_link_status(struct intel_dp
>>>> *intel_dp)
>>>>    {
>>>>           struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>           struct intel_encoder *intel_encoder =
>>>> &dp_to_dig_port(intel_dp)->base;
>>>> +       struct drm_connector *connector =
>>>> &intel_dp->attached_connector->base;
>>>> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
>>>> +       struct edid *edid_read = NULL;
>>>>           u8 sink_irq_vector;
>>>>           u8 link_status[DP_LINK_STATUS_SIZE];
>>>>
>>>> @@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp
>>>> *intel_dp)
>>>>                   return;
>>>>           }
>>>>
>>>> +       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
>>>> +        * 4.2.2.1 - EDID read required for all HPD events
>>>> +         */
>>>> +        edid_read = drm_get_edid(connector, adapter);
>>>> +        if (!edid_read) {
>>>> +                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
>>>> +        }
>>>> +
>>> We already briefly discussed this patch in private, so I'm going to
>>> summarize the discussion and also add some more points here.
>>>
>>> Frist, the actual detailed review: the indentation here is using
>>> spaces and we're leaking the EDID. This will cause rebases to a few of
>>> the next patches.
>>>
>>> Back to the hight level architecture: your initial versions of the
>>> series contained just 1 extra EDID read, and it was contained inside
>>> the compliance testing function. Then the versions submitted a few
>>> days ago had 2 extra EDID reads, then after some discussion you
>>> reduced to 1 extra EDID read (the one on this patch). I previously
>>> asked "But what about the automatic EDID read we do when we get a
>>> hotplug? Can't we just rely on it?". I got some answers to the
>>> question, but I was not really convinced.
>>>
>>> Yesterday I was arguing that this extra EDID read is going to add a
>>> small delay to every hotplug event we get, so my initial suggestion
>>> was to organize the compliance testing in a way that would require the
>>> user space program to call the GetResources() IOCTL to force the EDID
>>> when needed. Your argument was that then the DP compliance testing
>>> procedure would be testing our app for compliance, not the Kernel.
>>>
>>> But today I decided to finally do some debugging regarding this, and I
>>> was able to confirm that we do follow the DP requirements: we do have
>>> an automatic EDID read done by the Kernel whenever we do a hotplug:
>>> i915_hotplug_work_func() calls intel_dp_detect(), which ends calling
>>> drm_get_edid() at some point. This function also does other stuff that
>>> is required by the compliance testing, such as the DPCD reads.
>>>
>>> Now there's a problem with using i915_hotplug_work_func(), which could
>>> the reason why you rejected it: it only happens after
>>> intel_dp_hpd_pulse(), which means that we only really do the EDID read
>>> after intel_dp_handle_test_request().
>>>
>>> I consider i915_hotplug_work_func() a fundamental part of our DP
>>> framework, and the DP compliance testing seems to be just ignoring its
>>> existence. So my idea for a solution here would be to make
>>> intel_dp_handle_test_request() run on its own delayed work function.
>>> It would wait for both i915_digport_work_func() and
>>> i915_hotplug_work_func() to finish, and only then it would do the
>>> normal processing. With this, we would be able to avoid the edid read
>>> on this patch, we would maybe be able to avoid at least part of patch
>>> 2, we would maybe be able to completely avoid patch 7, and then on
>>> patch 8 we would start touching intel_dp_get_edid() instead.
>>>
>>> I know this is sort of a fundamental change that is being requested a
>>> little late in the review process, and it can be frustrating, but this
>>> aspect of the code only recently changed (I was fine with the EDID
>>> reads just in the compliance testing function), and since the DP
>>> compliance code is quite complex, it took me a while to realize
>>> everything that's going on and what is the purpose of each piece. I
>>> also think that, since this idea will allow the compliance testing to
>>> take into consideration the work done by i915_hotplug_work_func(),
>>> compliance testing will better reflect the behavior that is actually
>>> done by the Kernel when DP devices are plugged/unplugged. And I did
>>> ask about those new EDID reads as soon as I started reviewing the
>>> patch that introduced them.
>>>
>>> Now, since I know how frustrating it is to have to change a
>>> significant portion of the code once again, I will leave to the
>>> maintainers the decision of whether the current proposed
>>> implementation is acceptable or if we want to make the DP compliance
>>> testing code take into consideration the work done by
>>> i915_hotplug_work_func(). I would also like to know your opinion on
>>> this. Maybe my idea just doesn't make sense because of something else
>>> I didn't realize :)
>> I don't think this is a good idea. The work loop aspect seems like a very
>> complex solution solution to a problem that is relatively simple. In a
>> discussion with Daniel, he indicated that adding a work loop is something to
>> be avoided unless it's *really* necessary, as they are prone to race
>> conditions. In this case, I just don't see that it's necessary.
> The workqueue thing was just an idea to implement a solution for the
> real problem. I think we should be focusing about discussing the fact
> that we're not taking i915_hotplug_work_func() into consideration when
> doing the compliance testing, not on the fact that one of the possible
> implementations could use a workqueue. I'd still like to hear your
> arguments on that.
Fair enough.

So I've been looking into this and why the i915_hotplug_work_func wasn't 
part of this. It is, as you said, a relatively fundamental code path for 
Displayport through the driver. What I discovered was that this function 
is never called on HSW (my primary test vehicle), mainly because 
check_link_status() returns IRQ_HANDLED instead of IRQ_NONE. The work 
function for HSW is i915_digport_work_func, so when it gets the 
IRQ_HANDLED return code, it doesn't fall through to the legacy 
i915_hotplug_work_func handler. This is important because this handler 
calls intel_hpd_irq_event which is where the ->detect connector function 
is called. And intel_dp_detect() is where all the happy goodness for 
Displayport begins.

Up until I discovered this, I had mistakenly propagated that problem 
forward in to the SST case in intel_dp_hot_pulse() in patch 6 by 
returning IRQ_HANDLED instead of IRQ_NONE, which is what the code was 
doing for SST prior to patch 6. With this problem corrected (as it is in 
the latest update in patch set V6) the work functions are now called as 
they should be. The point being that this opens up the possibility of 
using elements along this path to pass compliance testing, thereby 
creating a more valid test case.

With this in mind, I am not opposed to using elements along that path to 
satisfy compliance requirements (that's the spirit of the tests, 
anyways) but as I've indicated, there are cases where we need to take 
special steps (like the edid_corrupt flag) in order to do the right 
things to pass the tests. I have concerns about trying to do that at 
this point, as it requires substantial rework to that code path that 
have a significant chance of breaking things. So to avoid that,  I 
propose that this patch be merged now so that a working solution is in 
place. This discussion should continue and we can decide where to put 
things in the hotplug_work path to satisfy the compliance requires over 
the course of some followup patches.

>> Additionally, you have been keen to note invasive solutions and this seems
>> like a highly invasive solution especially in light of having a viable one
>> right here.
>>
>> This solution adds very little overhead along the HPD path and that overhead
>> is a single read of the EDID for each event. To further address any concerns
>> about performance, for V6 I've propagated the long_hpd flag forward into
>> check_link_status so that the EDID read is only called for a hot plug event,
>> not short pulses. Another plus to this solution is that if/when this needs
>> to moved to userspace, this one is a lot easier to unwind and pull out than
>> the work loop you're suggesting.
>>
>> With respect to the user space solution, that's something worth
>> investigating in the future. I have some concerns about doing that, the
>> primary one being the ability to detect the corrupted header, but I think
>> it's something to consider for a follow-on update.
>>
>>
>>
>>>>           /* Try to read the source of the interrupt */
>>>>           if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>>>               intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
>>>> --
>>>> 1.9.1
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>>>
>
>

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

Re: [PATCH 04/13] drm/i915: Add EDID read in intel_dp_check_link_status() for Link CTS 4.2.2.1

[Daniel Vetter]

On Thu, Apr 16, 2015 at 08:41:33AM -0700, Todd Previte wrote:
> On 4/15/2015 10:42 AM, Paulo Zanoni wrote:
> >2015-04-15 12:37 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> >>On 4/14/2015 9:53 AM, Paulo Zanoni wrote:
> >>>2015-04-13 11:53 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> >>>>Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in
> >>>>the
> >>>>Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for
> >>>>all HPD plug events. To reduce the amount of code, this EDID read is also
> >>>>used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual
> >>>>support for these tests is implemented in later patches in this series.
> >>>>
> >>>>V2:
> >>>>- Fixed compilation error introduced during rework
> >>>>
> >>>>Signed-off-by: Todd Previte <tprevite@gmail.com>
> >>>>---
> >>>>   drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
> >>>>   1 file changed, 11 insertions(+)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/intel_dp.c
> >>>>b/drivers/gpu/drm/i915/intel_dp.c
> >>>>index 23184b0..75df3e2 100644
> >>>>--- a/drivers/gpu/drm/i915/intel_dp.c
> >>>>+++ b/drivers/gpu/drm/i915/intel_dp.c
> >>>>@@ -3890,6 +3890,9 @@ intel_dp_check_link_status(struct intel_dp
> >>>>*intel_dp)
> >>>>   {
> >>>>          struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >>>>          struct intel_encoder *intel_encoder =
> >>>>&dp_to_dig_port(intel_dp)->base;
> >>>>+       struct drm_connector *connector =
> >>>>&intel_dp->attached_connector->base;
> >>>>+       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
> >>>>+       struct edid *edid_read = NULL;
> >>>>          u8 sink_irq_vector;
> >>>>          u8 link_status[DP_LINK_STATUS_SIZE];
> >>>>
> >>>>@@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp
> >>>>*intel_dp)
> >>>>                  return;
> >>>>          }
> >>>>
> >>>>+       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
> >>>>+        * 4.2.2.1 - EDID read required for all HPD events
> >>>>+         */
> >>>>+        edid_read = drm_get_edid(connector, adapter);
> >>>>+        if (!edid_read) {
> >>>>+                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
> >>>>+        }
> >>>>+
> >>>We already briefly discussed this patch in private, so I'm going to
> >>>summarize the discussion and also add some more points here.
> >>>
> >>>Frist, the actual detailed review: the indentation here is using
> >>>spaces and we're leaking the EDID. This will cause rebases to a few of
> >>>the next patches.
> >>>
> >>>Back to the hight level architecture: your initial versions of the
> >>>series contained just 1 extra EDID read, and it was contained inside
> >>>the compliance testing function. Then the versions submitted a few
> >>>days ago had 2 extra EDID reads, then after some discussion you
> >>>reduced to 1 extra EDID read (the one on this patch). I previously
> >>>asked "But what about the automatic EDID read we do when we get a
> >>>hotplug? Can't we just rely on it?". I got some answers to the
> >>>question, but I was not really convinced.
> >>>
> >>>Yesterday I was arguing that this extra EDID read is going to add a
> >>>small delay to every hotplug event we get, so my initial suggestion
> >>>was to organize the compliance testing in a way that would require the
> >>>user space program to call the GetResources() IOCTL to force the EDID
> >>>when needed. Your argument was that then the DP compliance testing
> >>>procedure would be testing our app for compliance, not the Kernel.
> >>>
> >>>But today I decided to finally do some debugging regarding this, and I
> >>>was able to confirm that we do follow the DP requirements: we do have
> >>>an automatic EDID read done by the Kernel whenever we do a hotplug:
> >>>i915_hotplug_work_func() calls intel_dp_detect(), which ends calling
> >>>drm_get_edid() at some point. This function also does other stuff that
> >>>is required by the compliance testing, such as the DPCD reads.
> >>>
> >>>Now there's a problem with using i915_hotplug_work_func(), which could
> >>>the reason why you rejected it: it only happens after
> >>>intel_dp_hpd_pulse(), which means that we only really do the EDID read
> >>>after intel_dp_handle_test_request().
> >>>
> >>>I consider i915_hotplug_work_func() a fundamental part of our DP
> >>>framework, and the DP compliance testing seems to be just ignoring its
> >>>existence. So my idea for a solution here would be to make
> >>>intel_dp_handle_test_request() run on its own delayed work function.
> >>>It would wait for both i915_digport_work_func() and
> >>>i915_hotplug_work_func() to finish, and only then it would do the
> >>>normal processing. With this, we would be able to avoid the edid read
> >>>on this patch, we would maybe be able to avoid at least part of patch
> >>>2, we would maybe be able to completely avoid patch 7, and then on
> >>>patch 8 we would start touching intel_dp_get_edid() instead.
> >>>
> >>>I know this is sort of a fundamental change that is being requested a
> >>>little late in the review process, and it can be frustrating, but this
> >>>aspect of the code only recently changed (I was fine with the EDID
> >>>reads just in the compliance testing function), and since the DP
> >>>compliance code is quite complex, it took me a while to realize
> >>>everything that's going on and what is the purpose of each piece. I
> >>>also think that, since this idea will allow the compliance testing to
> >>>take into consideration the work done by i915_hotplug_work_func(),
> >>>compliance testing will better reflect the behavior that is actually
> >>>done by the Kernel when DP devices are plugged/unplugged. And I did
> >>>ask about those new EDID reads as soon as I started reviewing the
> >>>patch that introduced them.
> >>>
> >>>Now, since I know how frustrating it is to have to change a
> >>>significant portion of the code once again, I will leave to the
> >>>maintainers the decision of whether the current proposed
> >>>implementation is acceptable or if we want to make the DP compliance
> >>>testing code take into consideration the work done by
> >>>i915_hotplug_work_func(). I would also like to know your opinion on
> >>>this. Maybe my idea just doesn't make sense because of something else
> >>>I didn't realize :)
> >>I don't think this is a good idea. The work loop aspect seems like a very
> >>complex solution solution to a problem that is relatively simple. In a
> >>discussion with Daniel, he indicated that adding a work loop is something to
> >>be avoided unless it's *really* necessary, as they are prone to race
> >>conditions. In this case, I just don't see that it's necessary.
> >The workqueue thing was just an idea to implement a solution for the
> >real problem. I think we should be focusing about discussing the fact
> >that we're not taking i915_hotplug_work_func() into consideration when
> >doing the compliance testing, not on the fact that one of the possible
> >implementations could use a workqueue. I'd still like to hear your
> >arguments on that.
> Fair enough.
> 
> So I've been looking into this and why the i915_hotplug_work_func wasn't
> part of this. It is, as you said, a relatively fundamental code path for
> Displayport through the driver. What I discovered was that this function is
> never called on HSW (my primary test vehicle), mainly because
> check_link_status() returns IRQ_HANDLED instead of IRQ_NONE. The work
> function for HSW is i915_digport_work_func, so when it gets the IRQ_HANDLED
> return code, it doesn't fall through to the legacy i915_hotplug_work_func
> handler. This is important because this handler calls intel_hpd_irq_event
> which is where the ->detect connector function is called. And
> intel_dp_detect() is where all the happy goodness for Displayport begins.
> 
> Up until I discovered this, I had mistakenly propagated that problem forward
> in to the SST case in intel_dp_hot_pulse() in patch 6 by returning
> IRQ_HANDLED instead of IRQ_NONE, which is what the code was doing for SST
> prior to patch 6. With this problem corrected (as it is in the latest update
> in patch set V6) the work functions are now called as they should be. The
> point being that this opens up the possibility of using elements along this
> path to pass compliance testing, thereby creating a more valid test case.
> 
> With this in mind, I am not opposed to using elements along that path to
> satisfy compliance requirements (that's the spirit of the tests, anyways)
> but as I've indicated, there are cases where we need to take special steps
> (like the edid_corrupt flag) in order to do the right things to pass the
> tests. I have concerns about trying to do that at this point, as it requires
> substantial rework to that code path that have a significant chance of
> breaking things. So to avoid that,  I propose that this patch be merged now
> so that a working solution is in place. This discussion should continue and
> we can decide where to put things in the hotplug_work path to satisfy the
> compliance requires over the course of some followup patches.

I've looked a bit at all this and I think the other issue here is the
placement of intel_dp_handle_test_request in check_link_status. This has
been done almost 4 years ago in

commit a60f0e38d72a5e24085d6e7e27a4cadc20ae268a
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date:   Thu Oct 20 15:09:17 2011 -0700

    drm/i915: add DP test request handling

but never contained (up to this point) any functional code. It was however
dutifully moved around together with the other code. And way back the
placement even made some sense - check_link_status was called
unconditionally from our hot_plug handler. But since the MST rework (and a
few other things) happened that has been changed pretty radically and the
current place where the test request handler is called just doesn't seem
to make that much sense any more.

While I started pulling in patches I also noticed other places where we
duplicate existing logic (e.g. the dpcd read), so this isn't just about
the edid.

The other aspect here is that nowadays we do cache the edid for dp ports
aggressively, which means if we don't read the edid the kernel will indeed
keep on using a stale one. Hence there's a good risk that we don't just
have a minor piece of duct-tape to keep the somewhat strange expectations
of DP compliance testers happy but might be hiding a real bug in our DP
code. Giving how many we've had that seems fairly likely and I'm not happy
with sweeping this under the rug. This definitely needs a solid
analysis and explanation either way (i.e. whether this is a bug or just an
overly strict dp compliance tester requirement).

Finally there's the dp hotplug handling. Ever since MST support was merged
this has a been a lot of fun and took us a while to make it work correctly
- lots of deadlocks and other issues. And given the above we still seem to
have regressions due to MST support, or at least evidence for such. Since
this is a fairly fragile piece of code, which is also not that well-tested
(we don't have any MST hw in our test matrix yet) I prefer to keep changes
to a minimum. Merging these patches here first and then potentially
undoing them again because the bug has been the (mis)placement of the test
request handler in the MST patches feels too risky.

Given all that I'd like to hold off merging these patches that rework the
code around the check_link_status function until we have clarity here.

I've pulled in the other patches meanwhile which are reviewed and ready. I
also think we can pull in the drm_edid.c patch with the statistics code we
need for compliance testing ahead of resolving the above opens. And it
would be good to get some feedback from other (non-intel) drm developers
beforehand, the changes are quite invasive in some parts.

I've chatted a lot with Todd and Paulo and I think my decision here and
the rough plan laid out is the best choice I have from a technical point
of view.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/13] drm/i915: Add EDID read in intel_dp_check_link_status() for Link CTS 4.2.2.1

[Todd Previte]

On 4/16/2015 9:31 AM, Daniel Vetter wrote:
> On Thu, Apr 16, 2015 at 08:41:33AM -0700, Todd Previte wrote:
>> On 4/15/2015 10:42 AM, Paulo Zanoni wrote:
>>> 2015-04-15 12:37 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>>>> On 4/14/2015 9:53 AM, Paulo Zanoni wrote:
>>>>> 2015-04-13 11:53 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>>>>>> Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in
>>>>>> the
>>>>>> Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for
>>>>>> all HPD plug events. To reduce the amount of code, this EDID read is also
>>>>>> used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual
>>>>>> support for these tests is implemented in later patches in this series.
>>>>>>
>>>>>> V2:
>>>>>> - Fixed compilation error introduced during rework
>>>>>>
>>>>>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
>>>>>>    1 file changed, 11 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>>>> index 23184b0..75df3e2 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>>> @@ -3890,6 +3890,9 @@ intel_dp_check_link_status(struct intel_dp
>>>>>> *intel_dp)
>>>>>>    {
>>>>>>           struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>>           struct intel_encoder *intel_encoder =
>>>>>> &dp_to_dig_port(intel_dp)->base;
>>>>>> +       struct drm_connector *connector =
>>>>>> &intel_dp->attached_connector->base;
>>>>>> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
>>>>>> +       struct edid *edid_read = NULL;
>>>>>>           u8 sink_irq_vector;
>>>>>>           u8 link_status[DP_LINK_STATUS_SIZE];
>>>>>>
>>>>>> @@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp
>>>>>> *intel_dp)
>>>>>>                   return;
>>>>>>           }
>>>>>>
>>>>>> +       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
>>>>>> +        * 4.2.2.1 - EDID read required for all HPD events
>>>>>> +         */
>>>>>> +        edid_read = drm_get_edid(connector, adapter);
>>>>>> +        if (!edid_read) {
>>>>>> +                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
>>>>>> +        }
>>>>>> +
>>>>> We already briefly discussed this patch in private, so I'm going to
>>>>> summarize the discussion and also add some more points here.
>>>>>
>>>>> Frist, the actual detailed review: the indentation here is using
>>>>> spaces and we're leaking the EDID. This will cause rebases to a few of
>>>>> the next patches.
>>>>>
>>>>> Back to the hight level architecture: your initial versions of the
>>>>> series contained just 1 extra EDID read, and it was contained inside
>>>>> the compliance testing function. Then the versions submitted a few
>>>>> days ago had 2 extra EDID reads, then after some discussion you
>>>>> reduced to 1 extra EDID read (the one on this patch). I previously
>>>>> asked "But what about the automatic EDID read we do when we get a
>>>>> hotplug? Can't we just rely on it?". I got some answers to the
>>>>> question, but I was not really convinced.
>>>>>
>>>>> Yesterday I was arguing that this extra EDID read is going to add a
>>>>> small delay to every hotplug event we get, so my initial suggestion
>>>>> was to organize the compliance testing in a way that would require the
>>>>> user space program to call the GetResources() IOCTL to force the EDID
>>>>> when needed. Your argument was that then the DP compliance testing
>>>>> procedure would be testing our app for compliance, not the Kernel.
>>>>>
>>>>> But today I decided to finally do some debugging regarding this, and I
>>>>> was able to confirm that we do follow the DP requirements: we do have
>>>>> an automatic EDID read done by the Kernel whenever we do a hotplug:
>>>>> i915_hotplug_work_func() calls intel_dp_detect(), which ends calling
>>>>> drm_get_edid() at some point. This function also does other stuff that
>>>>> is required by the compliance testing, such as the DPCD reads.
>>>>>
>>>>> Now there's a problem with using i915_hotplug_work_func(), which could
>>>>> the reason why you rejected it: it only happens after
>>>>> intel_dp_hpd_pulse(), which means that we only really do the EDID read
>>>>> after intel_dp_handle_test_request().
>>>>>
>>>>> I consider i915_hotplug_work_func() a fundamental part of our DP
>>>>> framework, and the DP compliance testing seems to be just ignoring its
>>>>> existence. So my idea for a solution here would be to make
>>>>> intel_dp_handle_test_request() run on its own delayed work function.
>>>>> It would wait for both i915_digport_work_func() and
>>>>> i915_hotplug_work_func() to finish, and only then it would do the
>>>>> normal processing. With this, we would be able to avoid the edid read
>>>>> on this patch, we would maybe be able to avoid at least part of patch
>>>>> 2, we would maybe be able to completely avoid patch 7, and then on
>>>>> patch 8 we would start touching intel_dp_get_edid() instead.
>>>>>
>>>>> I know this is sort of a fundamental change that is being requested a
>>>>> little late in the review process, and it can be frustrating, but this
>>>>> aspect of the code only recently changed (I was fine with the EDID
>>>>> reads just in the compliance testing function), and since the DP
>>>>> compliance code is quite complex, it took me a while to realize
>>>>> everything that's going on and what is the purpose of each piece. I
>>>>> also think that, since this idea will allow the compliance testing to
>>>>> take into consideration the work done by i915_hotplug_work_func(),
>>>>> compliance testing will better reflect the behavior that is actually
>>>>> done by the Kernel when DP devices are plugged/unplugged. And I did
>>>>> ask about those new EDID reads as soon as I started reviewing the
>>>>> patch that introduced them.
>>>>>
>>>>> Now, since I know how frustrating it is to have to change a
>>>>> significant portion of the code once again, I will leave to the
>>>>> maintainers the decision of whether the current proposed
>>>>> implementation is acceptable or if we want to make the DP compliance
>>>>> testing code take into consideration the work done by
>>>>> i915_hotplug_work_func(). I would also like to know your opinion on
>>>>> this. Maybe my idea just doesn't make sense because of something else
>>>>> I didn't realize :)
>>>> I don't think this is a good idea. The work loop aspect seems like a very
>>>> complex solution solution to a problem that is relatively simple. In a
>>>> discussion with Daniel, he indicated that adding a work loop is something to
>>>> be avoided unless it's *really* necessary, as they are prone to race
>>>> conditions. In this case, I just don't see that it's necessary.
>>> The workqueue thing was just an idea to implement a solution for the
>>> real problem. I think we should be focusing about discussing the fact
>>> that we're not taking i915_hotplug_work_func() into consideration when
>>> doing the compliance testing, not on the fact that one of the possible
>>> implementations could use a workqueue. I'd still like to hear your
>>> arguments on that.
>> Fair enough.
>>
>> So I've been looking into this and why the i915_hotplug_work_func wasn't
>> part of this. It is, as you said, a relatively fundamental code path for
>> Displayport through the driver. What I discovered was that this function is
>> never called on HSW (my primary test vehicle), mainly because
>> check_link_status() returns IRQ_HANDLED instead of IRQ_NONE. The work
>> function for HSW is i915_digport_work_func, so when it gets the IRQ_HANDLED
>> return code, it doesn't fall through to the legacy i915_hotplug_work_func
>> handler. This is important because this handler calls intel_hpd_irq_event
>> which is where the ->detect connector function is called. And
>> intel_dp_detect() is where all the happy goodness for Displayport begins.
>>
>> Up until I discovered this, I had mistakenly propagated that problem forward
>> in to the SST case in intel_dp_hot_pulse() in patch 6 by returning
>> IRQ_HANDLED instead of IRQ_NONE, which is what the code was doing for SST
>> prior to patch 6. With this problem corrected (as it is in the latest update
>> in patch set V6) the work functions are now called as they should be. The
>> point being that this opens up the possibility of using elements along this
>> path to pass compliance testing, thereby creating a more valid test case.
>>
>> With this in mind, I am not opposed to using elements along that path to
>> satisfy compliance requirements (that's the spirit of the tests, anyways)
>> but as I've indicated, there are cases where we need to take special steps
>> (like the edid_corrupt flag) in order to do the right things to pass the
>> tests. I have concerns about trying to do that at this point, as it requires
>> substantial rework to that code path that have a significant chance of
>> breaking things. So to avoid that,  I propose that this patch be merged now
>> so that a working solution is in place. This discussion should continue and
>> we can decide where to put things in the hotplug_work path to satisfy the
>> compliance requires over the course of some followup patches.
> I've looked a bit at all this and I think the other issue here is the
> placement of intel_dp_handle_test_request in check_link_status. This has
> been done almost 4 years ago in
>
> commit a60f0e38d72a5e24085d6e7e27a4cadc20ae268a
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date:   Thu Oct 20 15:09:17 2011 -0700
>
>      drm/i915: add DP test request handling
>
> but never contained (up to this point) any functional code. It was however
> dutifully moved around together with the other code. And way back the
> placement even made some sense - check_link_status was called
> unconditionally from our hot_plug handler. But since the MST rework (and a
> few other things) happened that has been changed pretty radically and the
> current place where the test request handler is called just doesn't seem
> to make that much sense any more.
I will agree with you on this. In light of my earlier findings, I think 
that check_link_status may have become a superfluous function call, much 
like the old intel_dp_hot_plug() function. However, there is some code 
that needs to be carried forward into the normal work func path to make 
compliance testing work properly. Those things are as follows:
     - A fresh EDID read early in the HPD process
     - Detection of a corrupted EDID at the same point
     - A check of the Displayport IRQ_VECTOR register to determine if a 
test request has occurred

There is also one change that must be made to intel_dp_hpd_pulse(), and 
this is to make sure that it returns IRQ_NONE for the SST case such that 
the hotplug_work_func gets invoked. I believe that is the key to 
restoring working status to this code path for the reasons I detailed 
above.

As an aside, I think the obsolete functions should be removed instead of 
remaining in the code base. This should be done as a separate patch so 
that it's clear and bisectable should that become an issue in the future.

> While I started pulling in patches I also noticed other places where we
> duplicate existing logic (e.g. the dpcd read), so this isn't just about
> the edid.
Paulo noticed this as well and I was addressing it as it came up in 
review. In the patch churn some of those changes may have been lost or 
reverted, but this is something I will continue to fix as I work out 
this code.
> The other aspect here is that nowadays we do cache the edid for dp ports
> aggressively, which means if we don't read the edid the kernel will indeed
> keep on using a stale one. Hence there's a good risk that we don't just
> have a minor piece of duct-tape to keep the somewhat strange expectations
> of DP compliance testers happy but might be hiding a real bug in our DP
> code. Giving how many we've had that seems fairly likely and I'm not happy
> with sweeping this under the rug. This definitely needs a solid
> analysis and explanation either way (i.e. whether this is a bug or just an
> overly strict dp compliance tester requirement).
I think this can be resolved by starting to rework this in 
intel_dp_detect(). The DPCD and EDID reads/updates happen here, so this 
is the most logical place to start integrating the code from 
check_link_status().  I believe it can be done such that it satisfies 
both real world operations and compliance testing, with a minimum of 
special cases to accommodate the test apparatus.

>
> Finally there's the dp hotplug handling. Ever since MST support was merged
> this has a been a lot of fun and took us a while to make it work correctly
> - lots of deadlocks and other issues. And given the above we still seem to
> have regressions due to MST support, or at least evidence for such. Since
> this is a fairly fragile piece of code, which is also not that well-tested
> (we don't have any MST hw in our test matrix yet) I prefer to keep changes
> to a minimum. Merging these patches here first and then potentially
> undoing them again because the bug has been the (mis)placement of the test
> request handler in the MST patches feels too risky.
I can understand your point here. As I indicated above, I think the only 
real change required is to ensure that IRQ_NONE is returned for the SST 
case such that the correct work functions are invoked.
> Given all that I'd like to hold off merging these patches that rework the
> code around the check_link_status function until we have clarity here.
>
> I've pulled in the other patches meanwhile which are reviewed and ready. I
> also think we can pull in the drm_edid.c patch with the statistics code we
> need for compliance testing ahead of resolving the above opens. And it
> would be good to get some feedback from other (non-intel) drm developers
> beforehand, the changes are quite invasive in some parts.
>
> I've chatted a lot with Todd and Paulo and I think my decision here and
> the rough plan laid out is the best choice I have from a technical point
> of view.
>
> Thanks, Daniel

Thank you for the feedback, Daniel. I think this discussion needs to be 
carried forward into the V6 of the patch set to keep things current 
there. I'm going to begin working on the solution for this now.


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

Re: [PATCH 04/13] drm/i915: Add EDID read in intel_dp_check_link_status() for Link CTS 4.2.2.1

[Todd Previte]

On 4/16/15 9:31 AM, Daniel Vetter wrote:
> On Thu, Apr 16, 2015 at 08:41:33AM -0700, Todd Previte wrote:
>> On 4/15/2015 10:42 AM, Paulo Zanoni wrote:
>>> 2015-04-15 12:37 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>>>> On 4/14/2015 9:53 AM, Paulo Zanoni wrote:
>>>>> 2015-04-13 11:53 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>>>>>> Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in
>>>>>> the
>>>>>> Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for
>>>>>> all HPD plug events. To reduce the amount of code, this EDID read is also
>>>>>> used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual
>>>>>> support for these tests is implemented in later patches in this series.
>>>>>>
>>>>>> V2:
>>>>>> - Fixed compilation error introduced during rework
>>>>>>
>>>>>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
>>>>>>    1 file changed, 11 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>>>> index 23184b0..75df3e2 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>>> @@ -3890,6 +3890,9 @@ intel_dp_check_link_status(struct intel_dp
>>>>>> *intel_dp)
>>>>>>    {
>>>>>>           struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>>           struct intel_encoder *intel_encoder =
>>>>>> &dp_to_dig_port(intel_dp)->base;
>>>>>> +       struct drm_connector *connector =
>>>>>> &intel_dp->attached_connector->base;
>>>>>> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
>>>>>> +       struct edid *edid_read = NULL;
>>>>>>           u8 sink_irq_vector;
>>>>>>           u8 link_status[DP_LINK_STATUS_SIZE];
>>>>>>
>>>>>> @@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp
>>>>>> *intel_dp)
>>>>>>                   return;
>>>>>>           }
>>>>>>
>>>>>> +       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
>>>>>> +        * 4.2.2.1 - EDID read required for all HPD events
>>>>>> +         */
>>>>>> +        edid_read = drm_get_edid(connector, adapter);
>>>>>> +        if (!edid_read) {
>>>>>> +                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
>>>>>> +        }
>>>>>> +
>>>>> We already briefly discussed this patch in private, so I'm going to
>>>>> summarize the discussion and also add some more points here.
>>>>>
>>>>> Frist, the actual detailed review: the indentation here is using
>>>>> spaces and we're leaking the EDID. This will cause rebases to a few of
>>>>> the next patches.
>>>>>
>>>>> Back to the hight level architecture: your initial versions of the
>>>>> series contained just 1 extra EDID read, and it was contained inside
>>>>> the compliance testing function. Then the versions submitted a few
>>>>> days ago had 2 extra EDID reads, then after some discussion you
>>>>> reduced to 1 extra EDID read (the one on this patch). I previously
>>>>> asked "But what about the automatic EDID read we do when we get a
>>>>> hotplug? Can't we just rely on it?". I got some answers to the
>>>>> question, but I was not really convinced.
>>>>>
>>>>> Yesterday I was arguing that this extra EDID read is going to add a
>>>>> small delay to every hotplug event we get, so my initial suggestion
>>>>> was to organize the compliance testing in a way that would require the
>>>>> user space program to call the GetResources() IOCTL to force the EDID
>>>>> when needed. Your argument was that then the DP compliance testing
>>>>> procedure would be testing our app for compliance, not the Kernel.
>>>>>
>>>>> But today I decided to finally do some debugging regarding this, and I
>>>>> was able to confirm that we do follow the DP requirements: we do have
>>>>> an automatic EDID read done by the Kernel whenever we do a hotplug:
>>>>> i915_hotplug_work_func() calls intel_dp_detect(), which ends calling
>>>>> drm_get_edid() at some point. This function also does other stuff that
>>>>> is required by the compliance testing, such as the DPCD reads.
>>>>>
>>>>> Now there's a problem with using i915_hotplug_work_func(), which could
>>>>> the reason why you rejected it: it only happens after
>>>>> intel_dp_hpd_pulse(), which means that we only really do the EDID read
>>>>> after intel_dp_handle_test_request().
>>>>>
>>>>> I consider i915_hotplug_work_func() a fundamental part of our DP
>>>>> framework, and the DP compliance testing seems to be just ignoring its
>>>>> existence. So my idea for a solution here would be to make
>>>>> intel_dp_handle_test_request() run on its own delayed work function.
>>>>> It would wait for both i915_digport_work_func() and
>>>>> i915_hotplug_work_func() to finish, and only then it would do the
>>>>> normal processing. With this, we would be able to avoid the edid read
>>>>> on this patch, we would maybe be able to avoid at least part of patch
>>>>> 2, we would maybe be able to completely avoid patch 7, and then on
>>>>> patch 8 we would start touching intel_dp_get_edid() instead.
>>>>>
>>>>> I know this is sort of a fundamental change that is being requested a
>>>>> little late in the review process, and it can be frustrating, but this
>>>>> aspect of the code only recently changed (I was fine with the EDID
>>>>> reads just in the compliance testing function), and since the DP
>>>>> compliance code is quite complex, it took me a while to realize
>>>>> everything that's going on and what is the purpose of each piece. I
>>>>> also think that, since this idea will allow the compliance testing to
>>>>> take into consideration the work done by i915_hotplug_work_func(),
>>>>> compliance testing will better reflect the behavior that is actually
>>>>> done by the Kernel when DP devices are plugged/unplugged. And I did
>>>>> ask about those new EDID reads as soon as I started reviewing the
>>>>> patch that introduced them.
>>>>>
>>>>> Now, since I know how frustrating it is to have to change a
>>>>> significant portion of the code once again, I will leave to the
>>>>> maintainers the decision of whether the current proposed
>>>>> implementation is acceptable or if we want to make the DP compliance
>>>>> testing code take into consideration the work done by
>>>>> i915_hotplug_work_func(). I would also like to know your opinion on
>>>>> this. Maybe my idea just doesn't make sense because of something else
>>>>> I didn't realize :)
>>>> I don't think this is a good idea. The work loop aspect seems like a very
>>>> complex solution solution to a problem that is relatively simple. In a
>>>> discussion with Daniel, he indicated that adding a work loop is something to
>>>> be avoided unless it's *really* necessary, as they are prone to race
>>>> conditions. In this case, I just don't see that it's necessary.
>>> The workqueue thing was just an idea to implement a solution for the
>>> real problem. I think we should be focusing about discussing the fact
>>> that we're not taking i915_hotplug_work_func() into consideration when
>>> doing the compliance testing, not on the fact that one of the possible
>>> implementations could use a workqueue. I'd still like to hear your
>>> arguments on that.
>> Fair enough.
>>
>> So I've been looking into this and why the i915_hotplug_work_func wasn't
>> part of this. It is, as you said, a relatively fundamental code path for
>> Displayport through the driver. What I discovered was that this function is
>> never called on HSW (my primary test vehicle), mainly because
>> check_link_status() returns IRQ_HANDLED instead of IRQ_NONE. The work
>> function for HSW is i915_digport_work_func, so when it gets the IRQ_HANDLED
>> return code, it doesn't fall through to the legacy i915_hotplug_work_func
>> handler. This is important because this handler calls intel_hpd_irq_event
>> which is where the ->detect connector function is called. And
>> intel_dp_detect() is where all the happy goodness for Displayport begins.
>>
>> Up until I discovered this, I had mistakenly propagated that problem forward
>> in to the SST case in intel_dp_hot_pulse() in patch 6 by returning
>> IRQ_HANDLED instead of IRQ_NONE, which is what the code was doing for SST
>> prior to patch 6. With this problem corrected (as it is in the latest update
>> in patch set V6) the work functions are now called as they should be. The
>> point being that this opens up the possibility of using elements along this
>> path to pass compliance testing, thereby creating a more valid test case.
>>
>> With this in mind, I am not opposed to using elements along that path to
>> satisfy compliance requirements (that's the spirit of the tests, anyways)
>> but as I've indicated, there are cases where we need to take special steps
>> (like the edid_corrupt flag) in order to do the right things to pass the
>> tests. I have concerns about trying to do that at this point, as it requires
>> substantial rework to that code path that have a significant chance of
>> breaking things. So to avoid that,  I propose that this patch be merged now
>> so that a working solution is in place. This discussion should continue and
>> we can decide where to put things in the hotplug_work path to satisfy the
>> compliance requires over the course of some followup patches.
> I've looked a bit at all this and I think the other issue here is the
> placement of intel_dp_handle_test_request in check_link_status. This has
> been done almost 4 years ago in
>
> commit a60f0e38d72a5e24085d6e7e27a4cadc20ae268a
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date:   Thu Oct 20 15:09:17 2011 -0700
>
>      drm/i915: add DP test request handling
>
> but never contained (up to this point) any functional code. It was however
> dutifully moved around together with the other code. And way back the
> placement even made some sense - check_link_status was called
> unconditionally from our hot_plug handler. But since the MST rework (and a
> few other things) happened that has been changed pretty radically and the
> current place where the test request handler is called just doesn't seem
> to make that much sense any more.
Agreed. For V7, I moved it from here into intel_dp_detect. It's placed 
so that all the necessary operations to satisfy compliance testing 
happen before the test handler is called, so there's virtually no 
duplicated code at all now.
>
> While I started pulling in patches I also noticed other places where we
> duplicate existing logic (e.g. the dpcd read), so this isn't just about
> the edid.
>
> The other aspect here is that nowadays we do cache the edid for dp ports
> aggressively, which means if we don't read the edid the kernel will indeed
> keep on using a stale one. Hence there's a good risk that we don't just
> have a minor piece of duct-tape to keep the somewhat strange expectations
> of DP compliance testers happy but might be hiding a real bug in our DP
> code. Giving how many we've had that seems fairly likely and I'm not happy
> with sweeping this under the rug. This definitely needs a solid
> analysis and explanation either way (i.e. whether this is a bug or just an
> overly strict dp compliance tester requirement).
With the test handler being called from the regular hot plug path as 
above, this isn't really an issue anymore. All the EDID reads are real 
reads because they're occurring during a long pulse. And in that case, 
when intel_dp_detect is called, we unset the edid and read a fresh copy 
from the device. The compliance test handler uses also the real data 
from that instead of intermediate values as it did before.
> Finally there's the dp hotplug handling. Ever since MST support was merged
> this has a been a lot of fun and took us a while to make it work correctly
> - lots of deadlocks and other issues. And given the above we still seem to
> have regressions due to MST support, or at least evidence for such. Since
> this is a fairly fragile piece of code, which is also not that well-tested
> (we don't have any MST hw in our test matrix yet) I prefer to keep changes
> to a minimum. Merging these patches here first and then potentially
> undoing them again because the bug has been the (mis)placement of the test
> request handler in the MST patches feels too risky.
For V7, I removed all the changes to intel_dp_hpd_pulse as I found they 
were unnecessary once the test request code was moved into 
intel_dp_detect. The move of the test request handling from 
check_link_status to intel_dp_detect is the only code change on that 
path now.
> Given all that I'd like to hold off merging these patches that rework the
> code around the check_link_status function until we have clarity here.
>
> I've pulled in the other patches meanwhile which are reviewed and ready. I
> also think we can pull in the drm_edid.c patch with the statistics code we
> need for compliance testing ahead of resolving the above opens. And it
> would be good to get some feedback from other (non-intel) drm developers
> beforehand, the changes are quite invasive in some parts.
>
> I've chatted a lot with Todd and Paulo and I think my decision here and
> the rough plan laid out is the best choice I have from a technical point
> of view.
>
> Thanks, Daniel

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