Displayport compliance testing V4

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

Signed-off-by Todd Previte <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c  | 75 +++++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h |  4 +++
 2 files changed, 75 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +71 -4

index eea9e36..960cc68 100644
@@ -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_testing_active = 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

drivers/gpu/drm/i915/intel_drv.h +0 -0

index eef79cc..e7b62be 100644

drivers/gpu/drm/i915/intel_drv.h +0 -1

 };
 
 struct intel_digital_port {
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

2015-03-31 14:14 GMT-03:00 Todd Previte <[email protected]>:

Quoted 127 lines

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

Signed-off-by Todd Previte <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c  | 75 +++++++++++++++++++++++++++++++++++++---
  drivers/gpu/drm/i915/intel_drv.h |  4 +++
  2 files changed, 75 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +71 -4

index eea9e36..960cc68 100644
@@ -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_testing_active = 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

drivers/gpu/drm/i915/intel_drv.h +0 -0

index eef79cc..e7b62be 100644

drivers/gpu/drm/i915/intel_drv.h struct intel_dp { · line 652

Quoted 6 lines

drivers/gpu/drm/i915/intel_drv.h +3 -0

@@ -647,6 +647,10 @@ struct intel_dp {
                                      bool has_aux_irq,
                                      int send_bytes,
                                      uint32_t aux_clock_divider);
+
+       /* Displayport compliance testing */
+       unsigned long compliance_test_type;

Shouldn't this have a default/initialized value? I see we assign values to it, but then we never assign back to a value that means "not testing anything". It's hard to see if this is a problem since this variable is not really used on this patch. Ideally, the definition and assignments should be placed on the patch that actually uses them (patch 8).

I also see that on patch 5 you change this to char instead of long, but you still don't use it for anything... This is a little confusing.

drivers/gpu/drm/i915/intel_drv.h struct intel_dp { · line 653

Quoted 1 line

drivers/gpu/drm/i915/intel_drv.h +1 -0

+       bool compliance_testing_active;

Same thing for this: ideally it should be defined on the patch that actually does something with the variable.

Also, one variable is compliance_test_ and the other is compliance_testING_ . It would be nice to keep both in the same "namespace".

Anyway, the comments above are probably bikesheds. I'll keep reviewing, so when I reach patch 8 I'll have a clearer view of these variables, then I can come back to this patch.

Quoted 10 lines

drivers/gpu/drm/i915/intel_drv.h +0 -1

  };
 
  struct intel_digital_port {
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

2015-03-31 14:14 GMT-03:00 Todd Previte <[email protected]>:

Quoted 131 lines

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

Signed-off-by Todd Previte <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c  | 75 +++++++++++++++++++++++++++++++++++++---
  drivers/gpu/drm/i915/intel_drv.h |  4 +++
  2 files changed, 75 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +71 -4

index eea9e36..960cc68 100644
@@ -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_testing_active = 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

drivers/gpu/drm/i915/intel_drv.h +1 -0

index eef79cc..e7b62be 100644
@@ -647,6 +647,10 @@ struct intel_dp {
                                      bool has_aux_irq,
                                      int send_bytes,
                                      uint32_t aux_clock_divider);
+

drivers/gpu/drm/i915/intel_drv.h struct intel_dp { · line 652

Quoted 2 lines

drivers/gpu/drm/i915/intel_drv.h +2 -0

+       /* Displayport compliance testing */
+       unsigned long compliance_test_type;

Shouldn't this have a default/initialized value? I see we assign values to it, but then we never assign back to a value that means "not testing anything". It's hard to see if this is a problem since this variable is not really used on this patch. Ideally, the definition and assignments should be placed on the patch that actually uses them (patch 8).

I also see that on patch 5 you change this to char instead of long, but you still don't use it for anything... This is a little confusing.

drivers/gpu/drm/i915/intel_drv.h struct intel_dp { · line 653

Quoted 1 line

drivers/gpu/drm/i915/intel_drv.h +1 -0

+       bool compliance_testing_active;

Same thing for this: ideally it should be defined on the patch that actually does something with the variable.

Also, one variable is compliance_test_ and the other is compliance_testING_ . It would be nice to keep both in the same "namespace".

Anyway, the comments above are probably bikesheds. I'll keep reviewing, so when I reach patch 8 I'll have a clearer view of these variables, then I can come back to this patch.

Quoted 10 lines

drivers/gpu/drm/i915/intel_drv.h +0 -1

  };
 
  struct intel_digital_port {
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

Signed-off-by Todd Previte <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +0 -0

index ed2f60c..dc87276 100644

drivers/gpu/drm/i915/intel_dp.c +0 -0

 			if (status & DP_AUX_CH_CTL_DONE)
 				break;
 		}

Signature · 6 lines

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 24 lines · Todd Previte

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

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

Signed-off-by Todd Previte <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++--
  1 file changed, 8 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +0 -0

index ed2f60c..dc87276 100644
@@ -877,9 +877,15 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
  				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
  				   DP_AUX_CH_CTL_RECEIVE_ERROR);
 

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 884

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp.c +5 -2

-			if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
-				      DP_AUX_CH_CTL_RECEIVE_ERROR))
+			/* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
+			     400us delay required for errors and timeouts
+			     Timeout errors from the HW already meet this
+			     requirement so skip to next iteration
+			*/

Weird format for multi line comment.

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 888

Quoted 5 lines

drivers/gpu/drm/i915/intel_dp.c +3 -0

+			if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
+				usleep_range(400, 500);
  				continue;
+			}

Where did the DP_AUX_CH_CTL_TIME_OUT_ERROR handling go?

Quoted 11 lines

drivers/gpu/drm/i915/intel_dp.c +0 -0

  			if (status & DP_AUX_CH_CTL_DONE)
  				break;
  		}

Signature · 6 lines

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 25 lines · Ville Syrjälä

Quoted 24 lines · Todd Previte

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

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

Signed-off-by Todd Previte <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++--
  1 file changed, 8 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +0 -0

index ed2f60c..dc87276 100644
@@ -877,9 +877,15 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
  				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
  				   DP_AUX_CH_CTL_RECEIVE_ERROR);
 

Quoted 8 lines

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 884

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp.c +5 -2

-			if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
-				      DP_AUX_CH_CTL_RECEIVE_ERROR))
+			/* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
+			     400us delay required for errors and timeouts
+			     Timeout errors from the HW already meet this
+			     requirement so skip to next iteration
+			*/

Weird format for multi line comment.

Yeah I had to squish it in there to keep it under 80 columns. Needs the '*' on the left side too though. I'll fix that and repost.

Quoted 6 lines

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 888

Quoted 5 lines

drivers/gpu/drm/i915/intel_dp.c +3 -0

+			if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
+				usleep_range(400, 500);
  				continue;
+			}

Where did the DP_AUX_CH_CTL_TIME_OUT_ERROR handling go?

As I recall, previous review feedback indicated that the timeout condition there was already accounted for.

on 12/15, Paulo commented:

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

When I checked the BSpec, that seemed to be the case so I removed the TIME_OUT_ERROR. Without this
code in place, we still pass the compliance tests for AUX transactions, one of which is for a no-reply transaction.
That case specifically should hit the TIME_OUT_ERROR if it was going to occur, I would think.

If you can give me a case where that becomes an issue, it's a simple fix to add it back in there.

Quoted 12 lines

Quoted 12 lines

drivers/gpu/drm/i915/intel_dp.c +0 -0

  			if (status & DP_AUX_CH_CTL_DONE)
  				break;
  		}

Signature · 6 lines

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 60 lines · Todd Previte

Quoted 25 lines · Ville Syrjälä

Quoted 24 lines · Todd Previte

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

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

Signed-off-by Todd Previte <[email protected]>

---
   drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++--
   1 file changed, 8 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +0 -0

index ed2f60c..dc87276 100644
@@ -877,9 +877,15 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
   				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
   				   DP_AUX_CH_CTL_RECEIVE_ERROR);
 

Quoted 8 lines

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 884

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp.c +5 -2

-			if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
-				      DP_AUX_CH_CTL_RECEIVE_ERROR))
+			/* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
+			     400us delay required for errors and timeouts
+			     Timeout errors from the HW already meet this
+			     requirement so skip to next iteration
+			*/

Weird format for multi line comment.

Yeah I had to squish it in there to keep it under 80 columns. Needs the '*' on the left side too though. I'll fix that and repost.

Quoted 5 lines

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 888

Quoted 4 lines

drivers/gpu/drm/i915/intel_dp.c +3 -0

+			if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
+				usleep_range(400, 500);
   				continue;
+			}

Where did the DP_AUX_CH_CTL_TIME_OUT_ERROR handling go?

As I recall, previous review feedback indicated that the timeout condition there was already accounted for.

on 12/15, Paulo commented:

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

When I checked the BSpec, that seemed to be the case so I removed the TIME_OUT_ERROR. Without this
code in place, we still pass the compliance tests for AUX transactions, one of which is for a no-reply transaction.
That case specifically should hit the TIME_OUT_ERROR if it was going to occur, I would think.

Quoted 2 lines

If you can give me a case where that becomes an issue, it's a simple fix to add it back in there.

Not doing the usleep for the timeout error does seem sane enough to me, but I didn't actually read through the specs to confirm that.

However my concern is that you no longer check the timeout error bit at all inside the loop and instead just check the done bit even when the timeout error may have happened. Now, I'm not sure both bits can actually be set at the same time, but if they can't the original error check was entirely redundant. So it would seem safer to keep checking both error bits before the done bit.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp.c +0 -0

   			if (status & DP_AUX_CH_CTL_DONE)
   				break;
   		}

Signature · 6 lines

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 80 lines · Todd Previte

Quoted 72 lines · Ville Syrjälä

Quoted 32 lines · Todd Previte

Quoted 30 lines

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

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

Signed-off-by Todd Previte <[email protected]>

---
   drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++--
   1 file changed, 8 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +1 -1

index ed2f60c..dc87276 100644
@@ -877,9 +877,15 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
                                   DP_AUX_CH_CTL_TIME_OUT_ERROR |
                                   DP_AUX_CH_CTL_RECEIVE_ERROR);
   -                    if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
-                                     DP_AUX_CH_CTL_RECEIVE_ERROR))
+                       /* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2

Quoted 8 lines

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 884

Quoted 6 lines

drivers/gpu/drm/i915/intel_dp.c +1 -0

+                            400us delay required for errors and

timeouts
+ Timeout errors from the HW already meet
this

drivers/gpu/drm/i915/intel_dp.c +2 -0

+                            requirement so skip to next iteration
+                       */

Weird format for multi line comment.

Yeah I had to squish it in there to keep it under 80 columns. Needs the '*' on the left side too though. I'll fix that and repost.

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 888

Quoted 5 lines

drivers/gpu/drm/i915/intel_dp.c +3 -0

+                       if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
+                               usleep_range(400, 500);
                                continue;
+                       }

Where did the DP_AUX_CH_CTL_TIME_OUT_ERROR handling go?

As I recall, previous review feedback indicated that the timeout condition there was already accounted for.

on 12/15, Paulo commented:

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

When I checked the BSpec, that seemed to be the case so I removed the TIME_OUT_ERROR. Without this
code in place, we still pass the compliance tests for AUX transactions, one of which is for a no-reply transaction.
That case specifically should hit the TIME_OUT_ERROR if it was going to occur, I would think.

Quoted 2 lines

If you can give me a case where that becomes an issue, it's a simple fix to add it back in there.

Not doing the usleep for the timeout error does seem sane enough to me, but I didn't actually read through the specs to confirm that.

Quoted 6 lines

However my concern is that you no longer check the timeout error bit at all inside the loop and instead just check the done bit even when the timeout error may have happened. Now, I'm not sure both bits can actually be set at the same time, but if they can't the original error check was entirely redundant. So it would seem safer to keep checking both error bits before the done bit.

While there's no harm in checking the timeout bit here, does it really make sense to do so unless we're
going to take action on it?

As you said, it seems reasonable enough to not
wait an addition 400-500us,
so is there something else to do? It may be worth logging the error just to make sure there's some
record of when it happens, even if we're not going to do anything else.

As for exclusion between the two bits, the BSpec makes no indication that they're exclusive of one another. So
it's entirely possible to have both set.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

                        if (status & DP_AUX_CH_CTL_DONE)
                                break;
                }

--
1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

2015-04-01 16:45 GMT-03:00 Todd Previte <[email protected]>:

Quoted 72 lines · Ville Syrjälä

Quoted 70 lines · Todd Previte

Quoted 64 lines · Ville Syrjälä

Quoted 29 lines · Todd Previte

Quoted 28 lines

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

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

Signed-off-by Todd Previte <[email protected]>

---
    drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++--
    1 file changed, 8 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +0 -1

index ed2f60c..dc87276 100644
@@ -877,9 +877,15 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
                                    DP_AUX_CH_CTL_TIME_OUT_ERROR |
                                    DP_AUX_CH_CTL_RECEIVE_ERROR);
    -                    if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
-                                     DP_AUX_CH_CTL_RECEIVE_ERROR))

Quoted 8 lines

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 884

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp.c +2 -0

+                       /* 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

drivers/gpu/drm/i915/intel_dp.c +2 -0

+                            requirement so skip to next iteration
+                       */

Weird format for multi line comment.

Yeah I had to squish it in there to keep it under 80 columns. Needs the '*' on the left side too though. I'll fix that and repost.

Quoted 5 lines

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 888

Quoted 4 lines

drivers/gpu/drm/i915/intel_dp.c +3 -0

+                       if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
+                               usleep_range(400, 500);
                                 continue;
+                       }

Where did the DP_AUX_CH_CTL_TIME_OUT_ERROR handling go?

As I recall, previous review feedback indicated that the timeout condition there was already accounted for.

on 12/15, Paulo commented:

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

When I checked the BSpec, that seemed to be the case so I removed the TIME_OUT_ERROR. Without this
code in place, we still pass the compliance tests for AUX transactions, one of which is for a no-reply transaction.
That case specifically should hit the TIME_OUT_ERROR if it was going to occur, I would think.

Quoted 2 lines

If you can give me a case where that becomes an issue, it's a simple fix to add it back in there.

Not doing the usleep for the timeout error does seem sane enough to me, but I didn't actually read through the specs to confirm that.

Quoted 6 lines

Quoted 6 lines

However my concern is that you no longer check the timeout error bit at all inside the loop and instead just check the done bit even when the timeout error may have happened. Now, I'm not sure both bits can actually be set at the same time, but if they can't the original error check was entirely redundant. So it would seem safer to keep checking both error bits before the done bit.

I agree with Ville here. See below.

Quoted 3 lines

While there's no harm in checking the timeout bit here, does it really make sense to do so unless we're
going to take action on it?

Before this patch, we would check the bit and then run "continue", regardless of the state of DP_AUX_CH_CTL_DONE. With your patch, we don't check for the _TIME_OUT bit, which means that if both _TIME_OUT and _CTL_DONE bits are set, we will "break" instead of the old behavior. I think Ville's point is that we should probably continue with the old behavior, especially since this patch is just about adding a new sleep call, and not the specific interaction of _TIME_OUT and _CTL_DONE.

Quoted 6 lines

As you said, it seems reasonable enough to not
wait an addition 400-500us,
so is there something else to do? It may be worth logging the error just to make sure there's some
record of when it happens, even if we're not going to do anything else.

I'm not sure adding a log message here is a good idea: we're probably going to flood dmesg on a case that is somewhat expected and recoverable. We already print an error message in case we finish the loop without _CTL_DONE set, so I think we're covered regarding error printing already: just run "continue" without logging anything since we're going to retry anyway.

Quoted 17 lines

As for exclusion between the two bits, the BSpec makes no indication that they're exclusive of one another. So
it's entirely possible to have both set.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

                         if (status & DP_AUX_CH_CTL_DONE)
                                 break;
                 }

--
1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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. As of now, the only action taken is to log the error, since the HW will have already waited the required amount of time for the transaction to complete.

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)

Signed-off-by Todd Previte <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +11 -3

index ed2f60c..8b59458 100644
@@ -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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

2015-04-06 23:09 GMT-03:00 Todd Previte <[email protected]>:

Quoted 9 lines

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. As of now, the only action taken is to log the error, since the HW will have already waited the required amount of time for the transaction to complete.

As of V6, this paragraph is no longer true. We could just remove it while applying.

With that: Reviewed-by: Paulo Zanoni <[email protected]>.

Quoted 48 lines

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)

Signed-off-by Todd Previte <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c | 13 +++++++++++--
  1 file changed, 11 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +11 -3

index ed2f60c..8b59458 100644
@@ -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

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Displayport compliance test 4.2.2.6 requires that a source device be capable of detecting a corrupt EDID. To do this, the test sets up an invalid EDID header to be read by the source device. Unfortunately, the DRM EDID reading and parsing functions are actually too good in this case and prevent the source from reading the corrupted EDID. The result is a failed compliance test.

In order to successfully pass the test, the raw EDID header must be checked on each read to see if has been "corrupted". If an invalid raw header is detected, a flag is set that allows the compliance testing code to acknowledge that fact and react appropriately. The flag is automatically cleared on read.

This code is designed to expressly work for compliance testing without disrupting normal operations for EDID reading and parsing.

Signed-off-by Todd Previte <[email protected]>

Cc [email protected]

---
 drivers/gpu/drm/drm_edid.c       | 33 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 include/drm/drm_edid.h           |  5 +++++
 4 files changed, 56 insertions(+)

drivers/gpu/drm/drm_edid.c +0 -0

index 53bc7a6..3d4f473 100644

drivers/gpu/drm/drm_edid.c +21 -0

+
+/**
+ * drm_raw_edid_header_valid - check to see if the raw header is
+ * corrupt or not. Used solely for Displayport compliance
+ * testing and required by Link CTS Core 1.2 rev1.1 4.2.2.6.
+ * @raw_edid: pointer to raw base EDID block
+ *
+ * Indicates whether the original EDID header as read from the
+ * device was corrupt or not. Clears on read.
+ *
+ * Return: true if the raw header was corrupt, otherwise false
+ */
+bool drm_raw_edid_header_corrupt(void)
+{
+       bool corrupted = raw_edid_header_corrupted;
+
+       raw_edid_header_corrupted = 0;
+       return corrupted;
+}
+EXPORT_SYMBOL(drm_raw_edid_header_corrupt);
+
 /**
  * drm_edid_header_is_valid - sanity check the header of the base EDID block
  * @raw_edid: pointer to raw base EDID block

drivers/gpu/drm/drm_edid.c +0 -0

        return score;
 }
 EXPORT_SYMBOL(drm_edid_header_is_valid);

drivers/gpu/drm/i915/intel_dp.c +7 -0

index dc87276..57f8e43 100644
@@ -3824,6 +3824,9 @@ update_status:
                                   &response, 1);
        if (status <= 0)
                DRM_DEBUG_KMS("Could not write test response to sink\n");
+
+       /* Clear flag here, after testing is complete*/
+       intel_dp->compliance_edid_invalid = 0;
 }
 
 static int

@@ -3896,6 +3899,10 @@ 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];
 

@@ -3912,6 +3919,16 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
                return;
        }

drivers/gpu/drm/i915/intel_dp.c +1 -0

+
        /* 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)) {

drivers/gpu/drm/i915/intel_drv.h +1 -0

index e7b62be..42e4251 100644
@@ -651,6 +651,7 @@ struct intel_dp {
        /* Displayport compliance testing */
        unsigned long compliance_test_type;
        bool compliance_testing_active;
+       bool compliance_edid_invalid;
 };
 
 struct intel_digital_port {

include/drm/drm_edid.h +5 -1

index 87d85e8..8a7eb22 100644
@@ -388,4 +388,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
                              size_t len),
        void *data);
 
+/* Check for corruption in raw EDID header - Displayport compliance
+  * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+ */
+bool drm_raw_edid_header_corrupt(void);
+
 #endif /* __DRM_EDID_H__ */
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

2015-03-31 14:15 GMT-03:00 Todd Previte <[email protected]>:

Quoted 31 lines

Displayport compliance test 4.2.2.6 requires that a source device be capable of detecting a corrupt EDID. To do this, the test sets up an invalid EDID header to be read by the source device. Unfortunately, the DRM EDID reading and parsing functions are actually too good in this case and prevent the source from reading the corrupted EDID. The result is a failed compliance test.

In order to successfully pass the test, the raw EDID header must be checked on each read to see if has been "corrupted". If an invalid raw header is detected, a flag is set that allows the compliance testing code to acknowledge that fact and react appropriately. The flag is automatically cleared on read.

This code is designed to expressly work for compliance testing without disrupting normal operations for EDID reading and parsing.

Signed-off-by Todd Previte <[email protected]>

Cc [email protected]

---
  drivers/gpu/drm/drm_edid.c       | 33 +++++++++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
  drivers/gpu/drm/i915/intel_drv.h |  1 +
  include/drm/drm_edid.h           |  5 +++++
  4 files changed, 56 insertions(+)

drivers/gpu/drm/drm_edid.c +0 -0

index 53bc7a6..3d4f473 100644
@@ -990,6 +990,32 @@ static const u8 edid_header[] = {
         0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
  };
 

drivers/gpu/drm/drm_edid.c static const u8 edid_header[] = { · line 997

Quoted 5 lines

drivers/gpu/drm/drm_edid.c +5 -0

+
+/* Flag for EDID corruption testing
+ * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+ */
+static bool raw_edid_header_corrupted;

A static variable like this is not a good design, especially for a module like drm.ko. If you really need this, please store it inside some struct. But see below first.

Quoted 29 lines

drivers/gpu/drm/drm_edid.c +21 -0

+
+/**
+ * drm_raw_edid_header_valid - check to see if the raw header is
+ * corrupt or not. Used solely for Displayport compliance
+ * testing and required by Link CTS Core 1.2 rev1.1 4.2.2.6.
+ * @raw_edid: pointer to raw base EDID block
+ *
+ * Indicates whether the original EDID header as read from the
+ * device was corrupt or not. Clears on read.
+ *
+ * Return: true if the raw header was corrupt, otherwise false
+ */
+bool drm_raw_edid_header_corrupt(void)
+{
+       bool corrupted = raw_edid_header_corrupted;
+
+       raw_edid_header_corrupted = 0;
+       return corrupted;
+}
+EXPORT_SYMBOL(drm_raw_edid_header_corrupt);
+
  /**
   * drm_edid_header_is_valid - sanity check the header of the base EDID block
   * @raw_edid: pointer to raw base EDID block

@@ -1006,6 +1032,13 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
                 if (raw_edid[i] == edid_header[i])
                         score++;
 

drivers/gpu/drm/drm_edid.c int drm_edid_header_is_valid(const u8 *raw_edid) · line 1041

Quoted 7 lines

drivers/gpu/drm/drm_edid.c +7 -0

+       if (score != 8) {
+               /* Log and set flag here for EDID corruption testing
+                * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+                */
+               DRM_DEBUG_DRIVER("Raw EDID header invalid\n");
+               raw_edid_header_corrupted = 1;
+       }

The problem is that here we're limiting ourselves to just a bad edid header, not a bad edid in general, so there are many things which we might not get - such as a simple wrong checksum edid value. I remember that on the previous patch you calculated the whole checksum manually, but I don't see that code anymore. What was the reason for the change?

Also, while reviewing the patch I just discovered connector->bad_edid_counter. Can't we just use it instead of this patch? I mean: grab the current counter, check edid, see if the counter moved.

Quoted 32 lines

drivers/gpu/drm/drm_edid.c +0 -0

         return score;
  }
  EXPORT_SYMBOL(drm_edid_header_is_valid);

drivers/gpu/drm/i915/intel_dp.c +7 -0

index dc87276..57f8e43 100644
@@ -3824,6 +3824,9 @@ update_status:
                                    &response, 1);
         if (status <= 0)
                 DRM_DEBUG_KMS("Could not write test response to sink\n");
+
+       /* Clear flag here, after testing is complete*/
+       intel_dp->compliance_edid_invalid = 0;
  }
 
  static int

@@ -3896,6 +3899,10 @@ 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];
 

@@ -3912,6 +3919,16 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
                 return;
         }
 

drivers/gpu/drm/i915/intel_dp.c intel_dp_check_link_status(struct intel_dp *intel_dp) · line 3930

Quoted 9 lines

drivers/gpu/drm/i915/intel_dp.c +9 -0

+       /* Compliance testing requires an EDID read for all HPD events
+        * Link CTS Core 1.2 rev 1.1: Test 4.2.2.1
+        * Flag set here will be handled in the EDID test function
+        */
+       edid_read = drm_get_edid(connector, adapter);
+       if (!edid_read || drm_raw_edid_header_corrupt() == 1) {
+               DRM_DEBUG_DRIVER("EDID invalid, setting flag\n");
+               intel_dp->compliance_edid_invalid = 1;
+       }

I see that on the next patch you also add a drm_get_edid() call, so we have apparently added 2 calls for the edid test. Do we really need both? Why is this one needed? Why is that one needed?

Also, some more ideas:

I also thought that we already automatically issued get_edid() calls on the normal hotplug code path, so it would be a "third" call on the codepath for the test. Can't we just rely on this one?

Another idea would be: instead of getting the edid from inside the Kernel, we could try to get it from the user-space, using the GetResources/GetConnector IOCTLs, and also maybe look at the EDID properties to possibly validate the EDID (in case that edid did not get "fixed" by the Kernel). The nice thing about this is that it would make the test be more like a real driver usage. Do you see any possible problems with this approach?

Quoted 33 lines

drivers/gpu/drm/i915/intel_dp.c +1 -0

+
         /* 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)) {

drivers/gpu/drm/i915/intel_drv.h +1 -0

index e7b62be..42e4251 100644
@@ -651,6 +651,7 @@ struct intel_dp {
         /* Displayport compliance testing */
         unsigned long compliance_test_type;
         bool compliance_testing_active;
+       bool compliance_edid_invalid;
  };
 
  struct intel_digital_port {

include/drm/drm_edid.h +5 -1

index 87d85e8..8a7eb22 100644
@@ -388,4 +388,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
                               size_t len),
         void *data);
 
+/* Check for corruption in raw EDID header - Displayport compliance
+  * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+ */
+bool drm_raw_edid_header_corrupt(void);
+
  #endif /* __DRM_EDID_H__ */
--

1.9.1

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 42 lines

2015-03-31 14:15 GMT-03:00 Todd Previte <[email protected]>:

Quoted 40 lines

Displayport compliance test 4.2.2.6 requires that a source device be capable of detecting
a corrupt EDID. To do this, the test sets up an invalid EDID header to be read by the source
device. Unfortunately, the DRM EDID reading and parsing functions are actually too good in
this case and prevent the source from reading the corrupted EDID. The result is a failed
compliance test.

In order to successfully pass the test, the raw EDID header must be checked on each read
to see if has been "corrupted". If an invalid raw header is detected, a flag is set that
allows the compliance testing code to acknowledge that fact and react appropriately. The
flag is automatically cleared on read.

This code is designed to expressly work for compliance testing without disrupting normal
operations for EDID reading and parsing.

Signed-off-by Todd Previte <[email protected]>

Cc [email protected]

---
  drivers/gpu/drm/drm_edid.c       | 33 +++++++++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
  drivers/gpu/drm/i915/intel_drv.h |  1 +
  include/drm/drm_edid.h           |  5 +++++
  4 files changed, 56 insertions(+)

drivers/gpu/drm/drm_edid.c +0 -0

index 53bc7a6..3d4f473 100644
@@ -990,6 +990,32 @@ static const u8 edid_header[] = {
         0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
  };
 

Quoted 9 lines

drivers/gpu/drm/drm_edid.c static const u8 edid_header[] = { · line 997

Quoted 5 lines

drivers/gpu/drm/drm_edid.c +5 -0

+
+/* Flag for EDID corruption testing
+ * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+ */
+static bool raw_edid_header_corrupted;

A static variable like this is not a good design, especially for a module like drm.ko. If you really need this, please store it inside some struct. But see below first.

Per our discussion this morning, I concur. This has been removed in favor of a different solution that uses a new boolean flag in the drm_connector struct.

Capturing more of the discussion here, the static boolean was a bad idea to begin with and needed to be removed. One solution was to make the flag non-static and non-clear-on-read, then add a separate clear() function. But it still had the problem of potential misuse other places in the code. The current solution (which will be posted with V5) modifies the is_valid() function and adds a flag in the drm_connector struct that can be used to detect this low-level header corruption.

Quoted 30 lines

Quoted 30 lines

drivers/gpu/drm/drm_edid.c +21 -0

+
+/**
+ * drm_raw_edid_header_valid - check to see if the raw header is
+ * corrupt or not. Used solely for Displayport compliance
+ * testing and required by Link CTS Core 1.2 rev1.1 4.2.2.6.
+ * @raw_edid: pointer to raw base EDID block
+ *
+ * Indicates whether the original EDID header as read from the
+ * device was corrupt or not. Clears on read.
+ *
+ * Return: true if the raw header was corrupt, otherwise false
+ */
+bool drm_raw_edid_header_corrupt(void)
+{
+       bool corrupted = raw_edid_header_corrupted;
+
+       raw_edid_header_corrupted = 0;
+       return corrupted;
+}
+EXPORT_SYMBOL(drm_raw_edid_header_corrupt);
+
  /**
   * drm_edid_header_is_valid - sanity check the header of the base EDID

block
* @raw_edid: pointer to raw base EDID block

drivers/gpu/drm/drm_edid.c +0 -0

@@ -1006,6 +1032,13 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
                 if (raw_edid[i] == edid_header[i])
                         score++;
 

Quoted 13 lines

drivers/gpu/drm/drm_edid.c int drm_edid_header_is_valid(const u8 *raw_edid) · line 1041

Quoted 7 lines

drivers/gpu/drm/drm_edid.c +7 -0

+       if (score != 8) {
+               /* Log and set flag here for EDID corruption testing
+                * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+                */
+               DRM_DEBUG_DRIVER("Raw EDID header invalid\n");
+               raw_edid_header_corrupted = 1;
+       }

The problem is that here we're limiting ourselves to just a bad edid header, not a bad edid in general, so there are many things which we might not get - such as a simple wrong checksum edid value. I remember that on the previous patch you calculated the whole checksum manually, but I don't see that code anymore. What was the reason for the change?

So this code is specifically for the 4.2.2.6 compliance test that is looking for nothing more than an invalid EDID header.

In fact, the test unit only
sets that header as invalid once, so if you miss it on the first read, you can't go back and check it again later - the test will now fail. So catching the general case isn't really what this is about - it's about being able to detect a corrupt EDID header even if it only happens once.

Honestly, the DRM EDID code is VERY good about catching corruption cases and in the case of corrupted headers, fixing them and moving on. I had to tie into it at a fairly low level in order to catch the invalid header before the code fixed it.

With respect to the checksum code, for quite a while the checksum computation was incorrect in the DRM code. Somewhere along in November of last year or 2013 (I remember the month, not the year, go figure) someone came along and added a checksum computation that actually worked. So that rendered that original code I wrote unnecessary.

Quoted 4 lines

Also, while reviewing the patch I just discovered connector->bad_edid_counter. Can't we just use it instead of this patch? I mean: grab the current counter, check edid, see if the counter moved.

I think the above description highlights why using this counter really isn't an option. Since the code only gets one shot at catching that invalid header, it's essential to make sure it's captured specifically. Comparing before and after values of this counter doesn't specifically say that the header was invalid, only that SOMEthing in the EDID was invalid.

Quoted 39 lines

Quoted 39 lines

drivers/gpu/drm/drm_edid.c +0 -0

         return score;
  }
  EXPORT_SYMBOL(drm_edid_header_is_valid);

drivers/gpu/drm/i915/intel_dp.c +3 -0

index dc87276..57f8e43 100644
@@ -3824,6 +3824,9 @@ update_status:
                                    &response, 1);
         if (status <= 0)
                 DRM_DEBUG_KMS("Could not write test response to
 sink\n");
+
+       /* Clear flag here, after testing is complete*/
+       intel_dp->compliance_edid_invalid = 0;
  }
 

static int

drivers/gpu/drm/i915/intel_dp.c +4 -0

@@ -3896,6 +3899,10 @@ 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];

drivers/gpu/drm/i915/intel_dp.c +0 -0

@@ -3912,6 +3919,16 @@ intel_dp_check_link_status(struct intel_dp
 *intel_dp)
                 return;
         }
 

Quoted 13 lines

drivers/gpu/drm/i915/intel_dp.c intel_dp_check_link_status(struct intel_dp *intel_dp) · line 3930

Quoted 9 lines

drivers/gpu/drm/i915/intel_dp.c +9 -0

+       /* Compliance testing requires an EDID read for all HPD events
+        * Link CTS Core 1.2 rev 1.1: Test 4.2.2.1
+        * Flag set here will be handled in the EDID test function
+        */
+       edid_read = drm_get_edid(connector, adapter);
+       if (!edid_read || drm_raw_edid_header_corrupt() == 1) {
+               DRM_DEBUG_DRIVER("EDID invalid, setting flag\n");
+               intel_dp->compliance_edid_invalid = 1;
+       }

I see that on the next patch you also add a drm_get_edid() call, so we have apparently added 2 calls for the edid test. Do we really need both? Why is this one needed? Why is that one needed?

So there's two issues here - first is the same one mentioned above, catching that single instance of a corrupted EDID header. The second is that the checksum from the test device differs between the two reads. If you remove either one of them, one test or the other will fail.

Quoted 6 lines

Also, some more ideas:

I also thought that we already automatically issued get_edid() calls on the normal hotplug code path, so it would be a "third" call on the codepath for the test. Can't we just rely on this one?

Same issue as above.

Quoted 9 lines

Another idea would be: instead of getting the edid from inside the Kernel, we could try to get it from the user-space, using the GetResources/GetConnector IOCTLs, and also maybe look at the EDID properties to possibly validate the EDID (in case that edid did not get "fixed" by the Kernel). The nice thing about this is that it would make the test be more like a real driver usage. Do you see any possible problems with this approach?

I don't really see this as a valid option in light of the descriptions I've given above. This has a good chance of introducing latency problems which may adversely affect the tests as well.

Quoted 44 lines

Quoted 41 lines

drivers/gpu/drm/i915/intel_dp.c +1 -0

+
         /* 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)) {

drivers/gpu/drm/i915/intel_drv.h +1 -0

index e7b62be..42e4251 100644
@@ -651,6 +651,7 @@ struct intel_dp {
         /* Displayport compliance testing */
         unsigned long compliance_test_type;
         bool compliance_testing_active;
+       bool compliance_edid_invalid;
  };
 
  struct intel_digital_port {

include/drm/drm_edid.h +5 -0

index 87d85e8..8a7eb22 100644
@@ -388,4 +388,9 @@ struct edid *drm_do_get_edid(struct drm_connector
 *connector,
                               size_t len),
         void *data);
 
+/* Check for corruption in raw EDID header - Displayport compliance
+  * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+ */
+bool drm_raw_edid_header_corrupt(void);
+

#endif /* __DRM_EDID_H__ */
--
1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

2015-04-08 18:43 GMT-03:00 Todd Previte<[email protected]>:

Quoted 103 lines · Paulo Zanoni

Quoted 40 lines

2015-03-31 14:15 GMT-03:00 Todd Previte<[email protected]>:

Quoted 39 lines

Displayport compliance test 4.2.2.6 requires that a source device be capable of detecting
a corrupt EDID. To do this, the test sets up an invalid EDID header to be read by the source
device. Unfortunately, the DRM EDID reading and parsing functions are actually too good in
this case and prevent the source from reading the corrupted EDID. The result is a failed
compliance test.

In order to successfully pass the test, the raw EDID header must be checked on each read
to see if has been "corrupted". If an invalid raw header is detected, a flag is set that
allows the compliance testing code to acknowledge that fact and react appropriately. The
flag is automatically cleared on read.

This code is designed to expressly work for compliance testing without disrupting normal
operations for EDID reading and parsing.

Signed-off-by Todd Previte<[email protected]>

Cc [email protected]

---
   drivers/gpu/drm/drm_edid.c       | 33 +++++++++++++++++++++++++++++++++
   drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
   drivers/gpu/drm/i915/intel_drv.h |  1 +
   include/drm/drm_edid.h           |  5 +++++
   4 files changed, 56 insertions(+)

drivers/gpu/drm/drm_edid.c +0 -0

index 53bc7a6..3d4f473 100644
@@ -990,6 +990,32 @@ static const u8 edid_header[] = {
          0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
   };
 

Quoted 8 lines

drivers/gpu/drm/drm_edid.c static const u8 edid_header[] = { · line 997

Quoted 5 lines

drivers/gpu/drm/drm_edid.c +5 -0

+
+/* Flag for EDID corruption testing
+ * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+ */
+static bool raw_edid_header_corrupted;

A static variable like this is not a good design, especially for a module like drm.ko. If you really need this, please store it inside some struct. But see below first.

Per our discussion this morning, I concur. This has been removed in favor of a different solution that uses a new boolean flag in the drm_connector struct.

Capturing more of the discussion here, the static boolean was a bad idea to begin with and needed to be removed. One solution was to make the flag non-static and non-clear-on-read, then add a separate clear() function. But it still had the problem of potential misuse other places in the code. The current solution (which will be posted with V5) modifies the is_valid() function and adds a flag in the drm_connector struct that can be used to detect this low-level header corruption.

Quoted 29 lines

Quoted 29 lines

drivers/gpu/drm/drm_edid.c +21 -0

+
+/**
+ * drm_raw_edid_header_valid - check to see if the raw header is
+ * corrupt or not. Used solely for Displayport compliance
+ * testing and required by Link CTS Core 1.2 rev1.1 4.2.2.6.
+ * @raw_edid: pointer to raw base EDID block
+ *
+ * Indicates whether the original EDID header as read from the
+ * device was corrupt or not. Clears on read.
+ *
+ * Return: true if the raw header was corrupt, otherwise false
+ */
+bool drm_raw_edid_header_corrupt(void)
+{
+       bool corrupted = raw_edid_header_corrupted;
+
+       raw_edid_header_corrupted = 0;
+       return corrupted;
+}
+EXPORT_SYMBOL(drm_raw_edid_header_corrupt);
+
   /**
    * drm_edid_header_is_valid - sanity check the header of the base EDID

block
* @raw_edid: pointer to raw base EDID block

drivers/gpu/drm/drm_edid.c +0 -0

@@ -1006,6 +1032,13 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
                  if (raw_edid[i] == edid_header[i])
                          score++;
 

Quoted 12 lines

drivers/gpu/drm/drm_edid.c int drm_edid_header_is_valid(const u8 *raw_edid) · line 1041

Quoted 7 lines

drivers/gpu/drm/drm_edid.c +7 -0

+       if (score != 8) {
+               /* Log and set flag here for EDID corruption testing
+                * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+                */
+               DRM_DEBUG_DRIVER("Raw EDID header invalid\n");
+               raw_edid_header_corrupted = 1;
+       }

The problem is that here we're limiting ourselves to just a bad edid header, not a bad edid in general, so there are many things which we might not get - such as a simple wrong checksum edid value. I remember that on the previous patch you calculated the whole checksum manually, but I don't see that code anymore. What was the reason for the change?

Quoted 2 lines

So this code is specifically for the 4.2.2.6 compliance test that is looking for nothing more than an invalid EDID header.

On the version of the spec I have (1.2 Core, Aug 22 2011), 4.2.2.6 is "EDID Corruption Detection", and it mentions "EDID corruption" without really getting into the details of header corruption. On the "Test procedure" description, it mentions "Reference Sink sets up EDID with incorrect checksum", which we don't check. Of course, changing the header may produce an incorrect checksum, but maybe the wrong header is just a particular detail of the compliance testing device you have, while others could potentially have other forms of corruption, such as just a bad checksum?

In the paragraphs below you elaborate even more on the assumption of a bad header instead of just a bad checksum, so maybe we have different versions of the spec? (I still remember when I used version 1.0 of a certain non-backwards-compatible spec to review a patch made against version 0.8 of the same spec)

Quoted 21 lines

In fact, the test unit only
sets that header as invalid once, so if you miss it on the first read, you can't go back and check it again later - the test will now fail. So catching the general case isn't really what this is about - it's about being able to detect a corrupt EDID header even if it only happens once.

Honestly, the DRM EDID code is VERY good about catching corruption cases and in the case of corrupted headers, fixing them and moving on. I had to tie into it at a fairly low level in order to catch the invalid header before the code fixed it.

With respect to the checksum code, for quite a while the checksum computation was incorrect in the DRM code. Somewhere along in November of last year or 2013 (I remember the month, not the year, go figure) someone came along and added a checksum computation that actually worked. So that rendered that original code I wrote unnecessary.

Quoted 4 lines

Also, while reviewing the patch I just discovered connector->bad_edid_counter. Can't we just use it instead of this patch? I mean: grab the current counter, check edid, see if the counter moved.

Quoted 5 lines

I think the above description highlights why using this counter really isn't an option. Since the code only gets one shot at catching that invalid header, it's essential to make sure it's captured specifically. Comparing before and after values of this counter doesn't specifically say that the header was invalid, only that SOMEthing in the EDID was invalid.

Which is, according to the way I read the spec, not a problem.

Quoted 51 lines

Quoted 51 lines

Quoted 38 lines

drivers/gpu/drm/drm_edid.c +0 -0

          return score;
   }
   EXPORT_SYMBOL(drm_edid_header_is_valid);

drivers/gpu/drm/i915/intel_dp.c +3 -0

index dc87276..57f8e43 100644
@@ -3824,6 +3824,9 @@ update_status:
                                     &response, 1);
          if (status <= 0)
                  DRM_DEBUG_KMS("Could not write test response to
 sink\n");
+
+       /* Clear flag here, after testing is complete*/
+       intel_dp->compliance_edid_invalid = 0;
   }
 

static int

drivers/gpu/drm/i915/intel_dp.c +4 -0

@@ -3896,6 +3899,10 @@ 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];

drivers/gpu/drm/i915/intel_dp.c +0 -0

@@ -3912,6 +3919,16 @@ intel_dp_check_link_status(struct intel_dp
 *intel_dp)
                  return;
          }

drivers/gpu/drm/i915/intel_dp.c intel_dp_check_link_status(struct intel_dp · line 3931

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp.c +9 -0

 
+       /* Compliance testing requires an EDID read for all HPD events
+        * Link CTS Core 1.2 rev 1.1: Test 4.2.2.1
+        * Flag set here will be handled in the EDID test function
+        */
+       edid_read = drm_get_edid(connector, adapter);
+       if (!edid_read || drm_raw_edid_header_corrupt() == 1) {
+               DRM_DEBUG_DRIVER("EDID invalid, setting flag\n");
+               intel_dp->compliance_edid_invalid = 1;
+       }

I see that on the next patch you also add a drm_get_edid() call, so we have apparently added 2 calls for the edid test. Do we really need both? Why is this one needed? Why is that one needed?

Quoted 4 lines

So there's two issues here - first is the same one mentioned above, catching that single instance of a corrupted EDID header. The second is that the checksum from the test device differs between the two reads. If you remove either one of them, one test or the other will fail.

But then why not keep both at the same place? The one here is going to affect a lot more than just compliance testing, while the other is contained to DP compliance code.

Quoted 13 lines

Quoted 5 lines

Also, some more ideas:

I also thought that we already automatically issued get_edid() calls on the normal hotplug code path, so it would be a "third" call on the codepath for the test. Can't we just rely on this one?

Same issue as above.

Quoted 7 lines

Another idea would be: instead of getting the edid from inside the Kernel, we could try to get it from the user-space, using the GetResources/GetConnector IOCTLs, and also maybe look at the EDID properties to possibly validate the EDID (in case that edid did not get "fixed" by the Kernel). The nice thing about this is that it would make the test be more like a real driver usage. Do you see any possible problems with this approach?

Quoted 3 lines

I don't really see this as a valid option in light of the descriptions I've given above. This has a good chance of introducing latency problems which may adversely affect the tests as well.

We have 5 seconds, that's way more than enough.

Quoted 35 lines

Quoted 35 lines

Quoted 35 lines

drivers/gpu/drm/i915/intel_dp.c +1 -0

+
          /* 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)) {

drivers/gpu/drm/i915/intel_drv.h +1 -0

index e7b62be..42e4251 100644
@@ -651,6 +651,7 @@ struct intel_dp {
          /* Displayport compliance testing */
          unsigned long compliance_test_type;
          bool compliance_testing_active;
+       bool compliance_edid_invalid;
   };
 
   struct intel_digital_port {

include/drm/drm_edid.h +5 -0

index 87d85e8..8a7eb22 100644
@@ -388,4 +388,9 @@ struct edid *drm_do_get_edid(struct drm_connector
 *connector,
                                size_t len),
          void *data);
 
+/* Check for corruption in raw EDID header - Displayport compliance
+  * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+ */
+bool drm_raw_edid_header_corrupt(void);
+

   #endif /* __DRM_EDID_H__ */

--
1.9.1

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Signed-off-by Todd Previte <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c  | 53 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  3 ++-
 2 files changed, 55 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_dp.c +10 -0

index 57f8e43..16d35903 100644
@@ -41,6 +41,16 @@
 
 #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)
 
+/* Compliance test status bits  */
+#define INTEL_DP_EDID_SHIFT_MASK       0
+#define INTEL_DP_EDID_OK               (0 << INTEL_DP_EDID_SHIFT_MASK)
+#define INTEL_DP_EDID_CORRUPT          (1 << INTEL_DP_EDID_SHIFT_MASK)
+
+#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;

drivers/gpu/drm/i915/intel_dp.c +13 -0

+       } else {
+               ret = drm_dp_dpcd_write(&intel_dp->aux,
+                                       DP_TEST_EDID_CHECKSUM,
+                                       &edid_read->checksum, 1);
+               test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
+               intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ |
+                                                INTEL_DP_EDID_OK       |
+                                                INTEL_DP_RESOLUTION_STANDARD;
+       }
+
+       /* Set test active flag here so userspace doesn't interrupt things */
+       intel_dp->compliance_testing_active = 1;
+
        return test_result;
 }
 

drivers/gpu/drm/i915/intel_drv.h +1 -1

+       unsigned long compliance_test_data;
        bool compliance_testing_active;
        bool compliance_edid_invalid;
 };
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

2015-03-31 14:15 GMT-03:00 Todd Previte <[email protected]>:

Quoted 60 lines

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

Signed-off-by Todd Previte <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c  | 53 ++++++++++++++++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_drv.h |  3 ++-
  2 files changed, 55 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_dp.c +10 -0

index 57f8e43..16d35903 100644
@@ -41,6 +41,16 @@
 
  #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)
 
+/* Compliance test status bits  */
+#define INTEL_DP_EDID_SHIFT_MASK       0
+#define INTEL_DP_EDID_OK               (0 << INTEL_DP_EDID_SHIFT_MASK)
+#define INTEL_DP_EDID_CORRUPT          (1 << INTEL_DP_EDID_SHIFT_MASK)
+
+#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;

@@ -3766,7 +3776,44 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 

drivers/gpu/drm/i915/intel_dp.c static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3783

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp.c +4 -0

  static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
  {
+       struct drm_connector *connector = &intel_dp->attached_connector->base;
+       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+       struct edid *edid_read = NULL;
         uint8_t test_result = DP_TEST_NAK;
+       uint32_t ret = 0;

Variable "ret" is set but not used.

drivers/gpu/drm/i915/intel_dp.c static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3785

Quoted 3 lines

drivers/gpu/drm/i915/intel_dp.c +2 -0

+
+       edid_read = drm_get_edid(connector, adapter);

This is the additional edid read mentioned in the review of the previous patch.

drivers/gpu/drm/i915/intel_dp.c static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3795

Quoted 11 lines

drivers/gpu/drm/i915/intel_dp.c +10 -0

+
+       if (drm_raw_edid_header_corrupt() == 1) {
+               DRM_DEBUG_DRIVER("EDID Header corrupted\n");
+               intel_dp->compliance_edid_invalid = 1;
+       }
+
+       if (edid_read == NULL || 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) */

Missing '*' char on the comment.

drivers/gpu/drm/i915/intel_dp.c static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3797

Quoted 4 lines

drivers/gpu/drm/i915/intel_dp.c +2 -0

+               if (intel_dp->aux.i2c_nack_count > 0 ||
+                       intel_dp->aux.i2c_defer_count > 0)

Since we're potentially reading edid more than once after the test starts - as explained in the review of p4 -, do those nack/defer counts make sense? Shouldn't we zero them before each edid read attempt?

drivers/gpu/drm/i915/intel_dp.c static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3803

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp.c +6 -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 = DP_TEST_LINK_EDID_READ |
+                                                INTEL_DP_EDID_CORRUPT  |
+                                                INTEL_DP_RESOLUTION_FAILSAFE;

The compliance_test_data variable certainly needs a very good documentation on what its bits means - possibly on top of its definition, which is on patch 1 right now. Maybe it should be split into more than one variable, since the bits that go inside it come from different places. At least in the definition we should notice "bits2-4 are from dp_helper.h". Another problem is that bits 0 and 1 from dp_helper.h can't be used there, which is even more confusing.

Quoted 17 lines

drivers/gpu/drm/i915/intel_dp.c +13 -0

+       } else {
+               ret = drm_dp_dpcd_write(&intel_dp->aux,
+                                       DP_TEST_EDID_CHECKSUM,
+                                       &edid_read->checksum, 1);
+               test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
+               intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ |
+                                                INTEL_DP_EDID_OK       |
+                                                INTEL_DP_RESOLUTION_STANDARD;
+       }
+
+       /* Set test active flag here so userspace doesn't interrupt things */
+       intel_dp->compliance_testing_active = 1;
+
         return test_result;
  }
 

drivers/gpu/drm/i915/intel_dp.c mst_fail: · line 4651

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp.c +6 -0

@@ -4596,6 +4643,12 @@ mst_fail:
                 DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
                 intel_dp->is_mst = false;
                 drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
+       } else {
+               /* SST mode - handle short/long pulses here */
+               drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+               intel_dp_check_link_status(intel_dp);
+               drm_modeset_unlock(&dev->mode_config.connection_mutex);
+               ret = IRQ_HANDLED;

I guess this chunk belongs to patch 6, especially since you rewrite part of this new code there.

Quoted 11 lines

drivers/gpu/drm/i915/intel_dp.c +0 -0

         }
  put_power:
         intel_display_power_put(dev_priv, power_domain);

drivers/gpu/drm/i915/intel_drv.h +0 -0

index 42e4251..fb6134f 100644
@@ -649,7 +649,8 @@ struct intel_dp {
                                      uint32_t aux_clock_divider);
 

drivers/gpu/drm/i915/intel_drv.h struct intel_dp { · line 652

Quoted 3 lines

drivers/gpu/drm/i915/intel_drv.h +1 -1

         /* Displayport compliance testing */
-       unsigned long compliance_test_type;
+       unsigned char compliance_test_type;

This is the chunk I was talking about in my review of p1.

Quoted 7 lines

drivers/gpu/drm/i915/intel_drv.h +1 -1

+       unsigned long compliance_test_data;
         bool compliance_testing_active;
         bool compliance_edid_invalid;
  };
--

1.9.1

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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.

Signed-off-by Todd Previte <[email protected]>

Cc [email protected]

---
 drivers/gpu/drm/drm_dp_helper.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/gpu/drm/drm_dp_helper.c +0 -0

index 79968e3..0539758 100644

drivers/gpu/drm/drm_dp_helper.c +0 -0

 			usleep_range(400, 500);
 			continue;
 

Signature · 6 lines

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Signed-off-by Todd Previte <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +0 -10

index 16d35903..0a77f5a 100644
@@ -4622,16 +4622,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
                        if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
                                goto mst_fail;
                }
-
-               if (!intel_dp->is_mst) {
-                       /*
-                        * we'll check the link status via the normal hot plug path later -
-                        * but for short hpds we should check it now
-                        */
-                       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-                       intel_dp_check_link_status(intel_dp);
-                       drm_modeset_unlock(&dev->mode_config.connection_mutex);
-               }
        }
 
        ret = IRQ_HANDLED;

drivers/gpu/drm/i915/intel_dp.c +10 -2

+       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) {
+               /* TO DO: Handle short/long pulses individually
+                       Can save on training times and overhead by not doing
+                       full link status updating/processing for short pulses
+                */
                drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
                intel_dp_check_link_status(intel_dp);
                drm_modeset_unlock(&dev->mode_config.connection_mutex);
                ret = IRQ_HANDLED;
        }
-put_power:
        intel_display_power_put(dev_priv, power_domain);
 
        return ret;
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

2015-04-01 15:00 GMT-03:00 Todd Previte <[email protected]>:

Quoted 8 lines

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.

It's not clear to me what exactly is the problem with the current code. Can you please clarify?

Quoted 51 lines

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

Signed-off-by Todd Previte <[email protected]>

---
  drivers/gpu/drm/i915/intel_dp.c | 28 +++++++++++-----------------
  1 file changed, 11 insertions(+), 17 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +0 -10

index 16d35903..0a77f5a 100644
@@ -4622,16 +4622,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
                         if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
                                 goto mst_fail;
                 }
-
-               if (!intel_dp->is_mst) {
-                       /*
-                        * we'll check the link status via the normal hot plug path later -
-                        * but for short hpds we should check it now
-                        */
-                       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-                       intel_dp_check_link_status(intel_dp);
-                       drm_modeset_unlock(&dev->mode_config.connection_mutex);
-               }
         }
 

drivers/gpu/drm/i915/intel_dp.c intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) · line 4632

Quoted 12 lines

drivers/gpu/drm/i915/intel_dp.c +1 -6

         ret = IRQ_HANDLED;

@@ -4639,18 +4629,22 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
         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) {
-               DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
-               intel_dp->is_mst = false;
-               drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
-       } else {
-               /* SST mode - handle short/long pulses here */
+       DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);

So now aren't we going to get MST log messages on non-MST cases, such as a disconnect with long_hpd? I mean, even non-MST jumps to the mst_fail label.

Quoted 19 lines

drivers/gpu/drm/i915/intel_dp.c +10 -1

+       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) {
+               /* TO DO: Handle short/long pulses individually
+                       Can save on training times and overhead by not doing
+                       full link status updating/processing for short pulses
+                */
                 drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
                 intel_dp_check_link_status(intel_dp);
                 drm_modeset_unlock(&dev->mode_config.connection_mutex);
                 ret = IRQ_HANDLED;
         }
-put_power:
         intel_display_power_put(dev_priv, power_domain);
 

Quoted 8 lines

drivers/gpu/drm/i915/intel_dp.c +0 -1

         return ret;
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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.

Signed-off-by Todd Previte <[email protected]>

Cc [email protected]

---
 drivers/gpu/drm/drm_dp_helper.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/gpu/drm/drm_dp_helper.c +0 -0

index 79968e3..0539758 100644

drivers/gpu/drm/drm_dp_helper.c +0 -1

                        usleep_range(400, 500);
                        continue;
 
--

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

2015-03-31 14:15 GMT-03:00 Todd Previte <[email protected]>:

Quoted 31 lines

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.

Signed-off-by Todd Previte <[email protected]>

Cc [email protected]

---
  drivers/gpu/drm/drm_dp_helper.c | 5 +++++
  1 file changed, 5 insertions(+)

drivers/gpu/drm/drm_dp_helper.c +2 -0

index 79968e3..0539758 100644
@@ -469,6 +469,11 @@ 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

drivers/gpu/drm/drm_dp_helper.c static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) · line 476

Quoted 3 lines

drivers/gpu/drm/drm_dp_helper.c +3 -0

+                        * transaction to pass this test
+                        */
+                       retry--;

I wouldn't be surprised if someone discovers a monitor or some sort of dongle that keeps sending I2C defer errors forever, keeping us in an infinite loop. Shouldn't we count each error in separate? Or maybe just loop up to 14 times, in case that doesn't violate any spec (I didn't check)?

Quoted 10 lines

drivers/gpu/drm/drm_dp_helper.c +0 -1

                         usleep_range(400, 500);
                         continue;
 
--

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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.

Signed-off-by Todd Previte <[email protected]>

Cc [email protected]

---
 drivers/gpu/drm/drm_dp_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/gpu/drm/drm_dp_helper.c +0 -0

index 79968e3..23025cf 100644

drivers/gpu/drm/drm_dp_helper.c +1 -1

+                               retry--;
                        usleep_range(400, 500);
                        continue;
 
--

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

2015-04-06 23:11 GMT-03:00 Todd Previte <[email protected]>:

Quoted 32 lines

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.

Signed-off-by Todd Previte <[email protected]>

Cc [email protected]

---
 drivers/gpu/drm/drm_dp_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/gpu/drm/drm_dp_helper.c +0 -0

index 79968e3..23025cf 100644

drivers/gpu/drm/drm_dp_helper.c static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) · line 476

Quoted 8 lines

drivers/gpu/drm/drm_dp_helper.c +5 -0

@@ -469,6 +469,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 < 8)

I don't think this is the way to go. During normal (non-compliance-testing) operation we never zero i2c_defer_count, so we can't expect this to work, since we may start drm_dp_i2c_do_msg with a i2c_defer_count value different than zero. Also, during i915.ko DP compliance we only zero i2c_defer_count at the very beginning of each test, not at every aux transaction, and I really think we need a solution that is not specific to compliance testing.

Quoted 13 lines

drivers/gpu/drm/drm_dp_helper.c +1 -1

+                               retry--;
                        usleep_range(400, 500);
                        continue;
 
--

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Signature · 5 lines

Paulo Zanoni
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

2015-04-06 23:11 GMT-03:00 Todd Previte <[email protected]>:

Quoted 39 lines

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.

Signed-off-by Todd Previte <[email protected]>

Cc [email protected]

---
  drivers/gpu/drm/drm_dp_helper.c | 6 ++++++
  1 file changed, 6 insertions(+)

drivers/gpu/drm/drm_dp_helper.c +4 -0

index 79968e3..23025cf 100644
@@ -469,6 +469,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
+                        */

drivers/gpu/drm/drm_dp_helper.c static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) · line 476

Quoted 1 line

drivers/gpu/drm/drm_dp_helper.c +1 -0

+                       if (aux->i2c_defer_count < 8)

I don't think this is the way to go. During normal (non-compliance-testing) operation we never zero i2c_defer_count, so we can't expect this to work, since we may start drm_dp_i2c_do_msg with a i2c_defer_count value different than zero. Also, during i915.ko DP compliance we only zero i2c_defer_count at the very beginning of each test, not at every aux transaction, and I really think we need a solution that is not specific to compliance testing.

Quoted 5 lines

drivers/gpu/drm/drm_dp_helper.c +1 -0

+                               retry--;
                         usleep_range(400, 500);
                         continue;
 

Quoted 7 lines

--
1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel