Displayport compliance testing V6

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

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

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

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

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

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

Reviewed-by Paulo Zanoni <[email protected]>

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

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

index 14cdd00..263eff3 100644
@@ -3970,11 +3970,78 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 	return true;
 }
 
-static void
-intel_dp_handle_test_request(struct intel_dp *intel_dp)
+static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_ACK;
+	return test_result;
+}
+
+static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_NAK;
+	return test_result;
+}
+
+static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_NAK;
+	return test_result;
+}
+
+static uint8_t intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_NAK;
+	return test_result;
+}
+
+static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 {
-	/* NAK by default */
-	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK);
+	uint8_t response = DP_TEST_NAK;
+	uint8_t rxdata = 0;
+	int status = 0;
+
+	intel_dp->compliance_test_type = 0;
+	intel_dp->aux.i2c_nack_count = 0;
+	intel_dp->aux.i2c_defer_count = 0;
+
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Could not read test request from sink\n");
+		goto update_status;
+	}
+
+	switch (rxdata) {
+	case DP_TEST_LINK_TRAINING:
+		DRM_DEBUG_KMS("LINK_TRAINING test requested\n");
+		intel_dp->compliance_test_type = DP_TEST_LINK_TRAINING;
+		response = intel_dp_autotest_link_training(intel_dp);
+		break;
+	case DP_TEST_LINK_VIDEO_PATTERN:
+		DRM_DEBUG_KMS("TEST_PATTERN test requested\n");
+		intel_dp->compliance_test_type = DP_TEST_LINK_VIDEO_PATTERN;
+		response = intel_dp_autotest_video_pattern(intel_dp);
+		break;
+	case DP_TEST_LINK_EDID_READ:
+		DRM_DEBUG_KMS("EDID test requested\n");
+		intel_dp->compliance_test_type = DP_TEST_LINK_EDID_READ;
+		response = intel_dp_autotest_edid(intel_dp);
+		break;
+	case DP_TEST_LINK_PHY_TEST_PATTERN:
+		DRM_DEBUG_KMS("PHY_PATTERN test requested\n");
+		intel_dp->compliance_test_type = DP_TEST_LINK_PHY_TEST_PATTERN;
+		response = intel_dp_autotest_phy_pattern(intel_dp);
+		break;
+	default:
+		DRM_DEBUG_KMS("Invalid test request '%02x'\n", rxdata);
+		break;
+	}
+
+update_status:
+	status = drm_dp_dpcd_write(&intel_dp->aux,
+				   DP_TEST_RESPONSE,
+				   &response, 1);
+	if (status <= 0)
+		DRM_DEBUG_KMS("Could not write test response to sink\n");
 }
 
 static int

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

index 6a2ee0c..a4675fa 100644
@@ -739,6 +739,9 @@ struct intel_dp {
 				     bool has_aux_irq,
 				     int send_bytes,
 				     uint32_t aux_clock_divider);
+
+	/* Displayport compliance testing */
+	unsigned long compliance_test_type;
 };
 
 struct intel_digital_port {

Signature · 6 lines

1.9.1

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

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

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

V1:
- This is the second part of the single-patch split previously mentioned.
V2:
- Remerge the two split patches into one and update the commit message accordingly.
- Replace the SW connected status check with a HW HPD pin status check
- Adds a new function that examines the status of the HPD pin to determine if a sink device is connected
V3:
- CLean up of the patch merge from previous split
- Updated the commit message

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

---
 drivers/gpu/drm/i915/intel_dp.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +15 -19

index 263eff3..9c38986 100644
@@ -4119,24 +4119,8 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
        WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-       if (!intel_encoder->connectors_active)
-               return;
-
-       if (WARN_ON(!intel_encoder->base.crtc))
-               return;
-
-       if (!to_intel_crtc(intel_encoder->base.crtc)->active)
-               return;
-
-       /* Try to read receiver status if the link appears to be up */
-       if (!intel_dp_get_link_status(intel_dp, link_status)) {
-               return;
-       }
-
-       /* Now read the DPCD to see if it's actually running */
-       if (!intel_dp_get_dpcd(intel_dp)) {
+       if (!intel_dp_get_dpcd(intel_dp))
                return;
-       }
 
        /* Try to read the source of the interrupt */
        if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&

@@ -4145,13 +4129,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
                drm_dp_dpcd_writeb(&intel_dp->aux,
                                   DP_DEVICE_SERVICE_IRQ_VECTOR,
                                   sink_irq_vector);
-
                if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
                        intel_dp_handle_test_request(intel_dp);
                if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
                        DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
        }
 
+       if (!intel_encoder->connectors_active)
+               return;
+
+       if (WARN_ON(!intel_encoder->base.crtc))
+               return;
+
+       if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+               return;
+
+       /* Try to read receiver status if the link appears to be up */
+       if (!intel_dp_get_link_status(intel_dp, link_status)) {
+               return;
+       }
+
        if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
                DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
                              intel_encoder->base.name);
--

1.9.1

_______________________________________________
Intel-gfx mailing list
[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. This case just continues with the next iteration of the loop as the HW has already waited the required amount of time.

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

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

Reviewed-by Paulo Zanoni <[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 -2

index c112359..dae5c9a 100644
@@ -874,9 +874,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;
 		}

Signature · 6 lines

1.9.1

_______________________________________________
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. The test specification states that the sink device sets up the EDID with an invalid checksum. To do this, the sink sets up an invalid EDID header, expecting the source device to generate the checksum and compare it to the value stored in the last byte of the block data.

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

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

V2:
- Removed the static bool global
- Added a bool to the drm_connector struct to reaplce the static one for holding the status of raw edid header corruption detection
- Modified the function signature of the is_valid function to take an additional parameter to store the corruption detected value
- Fixed the other callers of the above is_valid function
V3:
- Updated the commit message to be more clear about what and why this patch does what it does.
- Added comment in code to clarify the operations there
- Removed compliance variable and check_link_status update; those have been moved to a later patch
- Removed variable assignment from the bottom of the test handler V4:
- Removed i915 tag from subject line as the patch is not i915-specific V5:
- Moved code causing a compilation error to this patch where the variable is actually declared
- Maintained blank lines / spacing so as to not contaminate the patch V6:
- Removed extra debug messages
- Added documentation to for the added parameter on drm_edid_block_valid
- Fixed more whitespace issues in check_link_status
- Added a clear of the header_corrupt flag to the end of the test handler in intel_dp.c
- Changed the usage of the new function prototype in several places to use NULL where it is not needed by compliance testing
V7:
- Updated to account for long_pulse flag propagation

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

Cc [email protected]

---
 drivers/gpu/drm/drm_edid.c      | 30 ++++++++++++++++++++++++++----
 drivers/gpu/drm/drm_edid_load.c |  7 +++++--
 drivers/gpu/drm/i915/intel_dp.c |  6 +++++-
 include/drm/drm_crtc.h          |  8 +++++++-
 4 files changed, 43 insertions(+), 8 deletions(-)

drivers/gpu/drm/drm_edid.c +10 -1

index 53bc7a6..1ed18f5 100644
@@ -1041,13 +1041,15 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
  * @raw_edid: pointer to raw EDID block
  * @block: type of block to validate (0 for base, extension otherwise)
  * @print_bad_edid: if true, dump bad EDID blocks to the console
+ * @header_corrupt: if true, the header or checksum is invalid
  *
  * Validate a base or extension EDID block and optionally dump bad blocks to
  * the console.
  *
  * Return: True if the block is valid, false otherwise.
  */
-bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
+bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
+                         bool *header_corrupt)
 {
        u8 csum;
        struct edid *edid = (struct edid *)raw_edid;

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

drivers/gpu/drm/drm_edid.c +9 -3

+                               *header_corrupt = 1;
+                       }
                        goto bad;
                }
        }

@@ -1129,7 +1147,7 @@ bool drm_edid_is_valid(struct edid *edid)
                return false;
 
        for (i = 0; i <= edid->extensions; i++)
-               if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
+               if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true, NULL))
                        return false;
 
        return true;

@@ -1232,7 +1250,8 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
        for (i = 0; i < 4; i++) {
                if (get_edid_block(data, block, 0, EDID_LENGTH))
                        goto out;
-               if (drm_edid_block_valid(block, 0, print_bad_edid))
+               if (drm_edid_block_valid(block, 0, print_bad_edid,
+                                        &connector->edid_header_corrupt))
                        break;
                if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
                        connector->null_edid_counter++;

@@ -1257,7 +1276,10 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
                                  block + (valid_extensions + 1) * EDID_LENGTH,
                                  j, EDID_LENGTH))
                                goto out;
-                       if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
+                       if (drm_edid_block_valid(block + (valid_extensions + 1)
+                                                * EDID_LENGTH, j,
+                                                print_bad_edid,
+                                                NULL)) {
                                valid_extensions++;
                                break;
                        }

drivers/gpu/drm/drm_edid_load.c +5 -2

index 4c0aa97..48b48e8 100644
@@ -216,7 +216,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
                goto out;
        }
 
-       if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
+       if (!drm_edid_block_valid(edid, 0, print_bad_edid,
+                                 &connector->edid_header_corrupt)) {
                connector->bad_edid_counter++;
                DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
                    name);

@@ -229,7 +230,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
                if (i != valid_extensions + 1)
                        memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
                            edid + i * EDID_LENGTH, EDID_LENGTH);
-               if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, print_bad_edid))
+               if (drm_edid_block_valid(edid + i * EDID_LENGTH, i,
+                                        print_bad_edid,
+                                        NULL))
                        valid_extensions++;
        }
 

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

index b120d59..ee41e10 100644
@@ -4005,6 +4005,7 @@ static uint8_t intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
 
 static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 {
+       struct drm_connector *connector = &intel_dp->attached_connector->base;
        uint8_t response = DP_TEST_NAK;
        uint8_t rxdata = 0;
        int status = 0;

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

 }
 
 static int

@@ -4136,7 +4140,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp, bool long_hpd)
         */
        if (long_hpd) {
                edid_read = drm_get_edid(connector, adapter);
-               if (!edid_read) {
+               if (!edid_read || connector->edid_header_corrupt == 1) {
                        DRM_DEBUG_DRIVER("Invalid EDID detected\n");
                } else {
                        kfree(edid_read);

include/drm/drm_crtc.h +7 -2

index d4e4b82..f7a4a92 100644
@@ -719,6 +719,11 @@ struct drm_connector {
        int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */
        unsigned bad_edid_counter;
 
+       /* Flag for raw EDID header corruption - used in Displayport
+        * compliance testing - * Displayport Link CTS Core 1.2 rev1.1 4.2.2.6
+        */
+       bool edid_header_corrupt;
+
        struct dentry *debugfs_entry;
 
        struct drm_connector_state *state;

@@ -1443,7 +1448,8 @@ extern void drm_set_preferred_mode(struct drm_connector *connector,
                                   int hpref, int vpref);
 
 extern int drm_edid_header_is_valid(const u8 *raw_edid);
-extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid);
+extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
+                                bool *header_corrupt);
 extern bool drm_edid_is_valid(struct edid *edid);
 
 extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
--

1.9.1

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

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

Quoted 94 lines

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

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

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

V2:
- Removed the static bool global
- Added a bool to the drm_connector struct to reaplce the static one for holding the status of raw edid header corruption detection
- Modified the function signature of the is_valid function to take an additional parameter to store the corruption detected value
- Fixed the other callers of the above is_valid function
V3:
- Updated the commit message to be more clear about what and why this patch does what it does.
- Added comment in code to clarify the operations there
- Removed compliance variable and check_link_status update; those have been moved to a later patch
- Removed variable assignment from the bottom of the test handler V4:
- Removed i915 tag from subject line as the patch is not i915-specific V5:
- Moved code causing a compilation error to this patch where the variable is actually declared
- Maintained blank lines / spacing so as to not contaminate the patch V6:
- Removed extra debug messages
- Added documentation to for the added parameter on drm_edid_block_valid
- Fixed more whitespace issues in check_link_status
- Added a clear of the header_corrupt flag to the end of the test handler in intel_dp.c
- Changed the usage of the new function prototype in several places to use NULL where it is not needed by compliance testing
V7:
- Updated to account for long_pulse flag propagation

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

Cc [email protected]

---
  drivers/gpu/drm/drm_edid.c      | 30 ++++++++++++++++++++++++++----
  drivers/gpu/drm/drm_edid_load.c |  7 +++++--
  drivers/gpu/drm/i915/intel_dp.c |  6 +++++-
  include/drm/drm_crtc.h          |  8 +++++++-
  4 files changed, 43 insertions(+), 8 deletions(-)

drivers/gpu/drm/drm_edid.c +15 -1

index 53bc7a6..1ed18f5 100644
@@ -1041,13 +1041,15 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
   * @raw_edid: pointer to raw EDID block
   * @block: type of block to validate (0 for base, extension otherwise)
   * @print_bad_edid: if true, dump bad EDID blocks to the console
+ * @header_corrupt: if true, the header or checksum is invalid
   *
   * Validate a base or extension EDID block and optionally dump bad blocks to
   * the console.
   *
   * Return: True if the block is valid, false otherwise.
   */
-bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
+bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
+                         bool *header_corrupt)
  {
         u8 csum;
         struct edid *edid = (struct edid *)raw_edid;

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

drivers/gpu/drm/drm_edid.c bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) · line 1083

Quoted 5 lines

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

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

Still using DRM_DEBUG_DRIVER. See include/drm/drmP.h for a description of each macro. BTW, since we're just going to dump the whole EDID anyway, the message is also not very informative.

Quoted 68 lines

drivers/gpu/drm/drm_edid.c +9 -3

+                               *header_corrupt = 1;
+                       }
                         goto bad;
                 }
         }

@@ -1129,7 +1147,7 @@ bool drm_edid_is_valid(struct edid *edid)
                 return false;
 
         for (i = 0; i <= edid->extensions; i++)
-               if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
+               if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true, NULL))
                         return false;
 
         return true;

@@ -1232,7 +1250,8 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
         for (i = 0; i < 4; i++) {
                 if (get_edid_block(data, block, 0, EDID_LENGTH))
                         goto out;
-               if (drm_edid_block_valid(block, 0, print_bad_edid))
+               if (drm_edid_block_valid(block, 0, print_bad_edid,
+                                        &connector->edid_header_corrupt))
                         break;
                 if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
                         connector->null_edid_counter++;

@@ -1257,7 +1276,10 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
                                   block + (valid_extensions + 1) * EDID_LENGTH,
                                   j, EDID_LENGTH))
                                 goto out;
-                       if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
+                       if (drm_edid_block_valid(block + (valid_extensions + 1)
+                                                * EDID_LENGTH, j,
+                                                print_bad_edid,
+                                                NULL)) {
                                 valid_extensions++;
                                 break;
                         }

drivers/gpu/drm/drm_edid_load.c +5 -2

index 4c0aa97..48b48e8 100644
@@ -216,7 +216,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
                 goto out;
         }
 
-       if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
+       if (!drm_edid_block_valid(edid, 0, print_bad_edid,
+                                 &connector->edid_header_corrupt)) {
                 connector->bad_edid_counter++;
                 DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
                     name);

@@ -229,7 +230,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
                 if (i != valid_extensions + 1)
                         memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
                             edid + i * EDID_LENGTH, EDID_LENGTH);
-               if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, print_bad_edid))
+               if (drm_edid_block_valid(edid + i * EDID_LENGTH, i,
+                                        print_bad_edid,
+                                        NULL))
                         valid_extensions++;
         }
 

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

index b120d59..ee41e10 100644
@@ -4005,6 +4005,7 @@ static uint8_t intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
 

drivers/gpu/drm/i915/intel_dp.c update_status: · line 4057

Quoted 13 lines

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

  static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
  {
+       struct drm_connector *connector = &intel_dp->attached_connector->base;
         uint8_t response = DP_TEST_NAK;
         uint8_t rxdata = 0;
         int status = 0;

@@ -4051,6 +4052,9 @@ update_status:
                                    &response, 1);
         if (status <= 0)
                 DRM_DEBUG_KMS("Could not write test response to sink\n");
+
+       /* Clear EDID corruption flag now */
+       connector->edid_header_corrupt = 0;

I don't think it makes sense to clear this inside i915.ko. We're adding the feature to drm.ko, so It makes sense to clear it there.

Isn't it possible to just clear it inside drm_edid_block_valid()? Something like:

if (score == 8) {
     if (header_corrupt)
         *header_corrupt = 0;
} else if (score >= edid_fixup) {
         ... etc ...
     }
}

If not, why?

Quoted 38 lines

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

  }
 
  static int

@@ -4136,7 +4140,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp, bool long_hpd)
          */
         if (long_hpd) {
                 edid_read = drm_get_edid(connector, adapter);
-               if (!edid_read) {
+               if (!edid_read || connector->edid_header_corrupt == 1) {
                         DRM_DEBUG_DRIVER("Invalid EDID detected\n");
                 } else {
                         kfree(edid_read);

include/drm/drm_crtc.h +7 -1

index d4e4b82..f7a4a92 100644
@@ -719,6 +719,11 @@ struct drm_connector {
         int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */
         unsigned bad_edid_counter;
 
+       /* Flag for raw EDID header corruption - used in Displayport
+        * compliance testing - * Displayport Link CTS Core 1.2 rev1.1 4.2.2.6
+        */
+       bool edid_header_corrupt;
+
         struct dentry *debugfs_entry;
 
         struct drm_connector_state *state;

@@ -1443,7 +1448,8 @@ extern void drm_set_preferred_mode(struct drm_connector *connector,
                                    int hpref, int vpref);
 
  extern int drm_edid_header_is_valid(const u8 *raw_edid);
-extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid);
+extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
+                                bool *header_corrupt);
  extern bool drm_edid_is_valid(struct edid *edid);
 

Quoted 8 lines

include/drm/drm_crtc.h +0 -1

  extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
--

1.9.1

_______________________________________________
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. The test specification states that the sink device sets up the EDID with an invalid checksum. To do this, the sink sets up an invalid EDID header, expecting the source device to generate the checksum and compare it to the value stored in the last byte of the block data.

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

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

V2:
- Removed the static bool global
- Added a bool to the drm_connector struct to reaplce the static one for holding the status of raw edid header corruption detection
- Modified the function signature of the is_valid function to take an additional parameter to store the corruption detected value
- Fixed the other callers of the above is_valid function
V3:
- Updated the commit message to be more clear about what and why this patch does what it does.
- Added comment in code to clarify the operations there
- Removed compliance variable and check_link_status update; those have been moved to a later patch
- Removed variable assignment from the bottom of the test handler V4:
- Removed i915 tag from subject line as the patch is not i915-specific V5:
- Moved code causing a compilation error to this patch where the variable is actually declared
- Maintained blank lines / spacing so as to not contaminate the patch V6:
- Removed extra debug messages
- Added documentation to for the added parameter on drm_edid_block_valid
- Fixed more whitespace issues in check_link_status
- Added a clear of the header_corrupt flag to the end of the test handler in intel_dp.c
- Changed the usage of the new function prototype in several places to use NULL where it is not needed by compliance testing
V7:
- Updated to account for long_pulse flag propagation
V8:
- Removed clearing of header_corrupt flag from the test handler in intel_dp.c
- Added clearing of header_corrupt flag in the drm_edid_block_valid function

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

Cc [email protected]

---
 drivers/gpu/drm/drm_edid.c      | 35 +++++++++++++++++++++++++++++------
 drivers/gpu/drm/drm_edid_load.c |  7 +++++--
 drivers/gpu/drm/i915/intel_dp.c |  2 +-
 include/drm/drm_crtc.h          |  8 +++++++-
 4 files changed, 42 insertions(+), 10 deletions(-)

drivers/gpu/drm/drm_edid.c +9 -3

index 53bc7a6..6d037f9 100644
@@ -1041,13 +1041,15 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
  * @raw_edid: pointer to raw EDID block
  * @block: type of block to validate (0 for base, extension otherwise)
  * @print_bad_edid: if true, dump bad EDID blocks to the console
+ * @header_corrupt: if true, the header or checksum is invalid
  *
  * Validate a base or extension EDID block and optionally dump bad blocks to
  * the console.
  *
  * Return: True if the block is valid, false otherwise.
  */
-bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
+bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
+                         bool *header_corrupt)
 {
        u8 csum;
        struct edid *edid = (struct edid *)raw_edid;

@@ -1060,11 +1062,28 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
 
        if (block == 0) {
                int score = drm_edid_header_is_valid(raw_edid);
-               if (score == 8) ;
-               else if (score >= edid_fixup) {
+               if (score == 8) {
+                       if (header_corrupt)
+                               *header_corrupt = 0;
+               } else if (score >= edid_fixup) {
+                       /* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6
+                        * In order to properly generate the invalid checksum

drivers/gpu/drm/drm_edid.c +10 -3

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

@@ -1129,7 +1148,7 @@ bool drm_edid_is_valid(struct edid *edid)
                return false;
 
        for (i = 0; i <= edid->extensions; i++)
-               if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
+               if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true, NULL))
                        return false;
 
        return true;

@@ -1232,7 +1251,8 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
        for (i = 0; i < 4; i++) {
                if (get_edid_block(data, block, 0, EDID_LENGTH))
                        goto out;
-               if (drm_edid_block_valid(block, 0, print_bad_edid))
+               if (drm_edid_block_valid(block, 0, print_bad_edid,
+                                        &connector->edid_header_corrupt))
                        break;
                if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
                        connector->null_edid_counter++;

@@ -1257,7 +1277,10 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
                                  block + (valid_extensions + 1) * EDID_LENGTH,
                                  j, EDID_LENGTH))
                                goto out;
-                       if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
+                       if (drm_edid_block_valid(block + (valid_extensions + 1)
+                                                * EDID_LENGTH, j,
+                                                print_bad_edid,
+                                                NULL)) {
                                valid_extensions++;
                                break;
                        }

drivers/gpu/drm/drm_edid_load.c +5 -2

index 4c0aa97..48b48e8 100644
@@ -216,7 +216,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
                goto out;
        }
 
-       if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
+       if (!drm_edid_block_valid(edid, 0, print_bad_edid,
+                                 &connector->edid_header_corrupt)) {
                connector->bad_edid_counter++;
                DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
                    name);

@@ -229,7 +230,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
                if (i != valid_extensions + 1)
                        memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
                            edid + i * EDID_LENGTH, EDID_LENGTH);
-               if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, print_bad_edid))
+               if (drm_edid_block_valid(edid + i * EDID_LENGTH, i,
+                                        print_bad_edid,
+                                        NULL))
                        valid_extensions++;
        }
 

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

index 7ca0e18..97224ff 100644
@@ -4139,7 +4139,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp, bool long_hpd)
         */
        if (long_hpd) {
                edid_read = drm_get_edid(connector, adapter);
-               if (!edid_read) {
+               if (!edid_read || connector->edid_header_corrupt == 1) {
                        DRM_DEBUG_DRIVER("Invalid EDID detected\n");
                } else {
                        kfree(edid_read);

include/drm/drm_crtc.h +7 -2

index d4e4b82..f7a4a92 100644
@@ -719,6 +719,11 @@ struct drm_connector {
        int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */
        unsigned bad_edid_counter;
 
+       /* Flag for raw EDID header corruption - used in Displayport
+        * compliance testing - * Displayport Link CTS Core 1.2 rev1.1 4.2.2.6
+        */
+       bool edid_header_corrupt;
+
        struct dentry *debugfs_entry;
 
        struct drm_connector_state *state;

@@ -1443,7 +1448,8 @@ extern void drm_set_preferred_mode(struct drm_connector *connector,
                                   int hpref, int vpref);
 
 extern int drm_edid_header_is_valid(const u8 *raw_edid);
-extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid);
+extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
+                                bool *header_corrupt);
 extern bool drm_edid_is_valid(struct edid *edid);
 
 extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
--

1.9.1

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

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

Quoted 99 lines

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

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

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

V2:
- Removed the static bool global
- Added a bool to the drm_connector struct to reaplce the static one for holding the status of raw edid header corruption detection
- Modified the function signature of the is_valid function to take an additional parameter to store the corruption detected value
- Fixed the other callers of the above is_valid function
V3:
- Updated the commit message to be more clear about what and why this patch does what it does.
- Added comment in code to clarify the operations there
- Removed compliance variable and check_link_status update; those have been moved to a later patch
- Removed variable assignment from the bottom of the test handler V4:
- Removed i915 tag from subject line as the patch is not i915-specific V5:
- Moved code causing a compilation error to this patch where the variable is actually declared
- Maintained blank lines / spacing so as to not contaminate the patch V6:
- Removed extra debug messages
- Added documentation to for the added parameter on drm_edid_block_valid
- Fixed more whitespace issues in check_link_status
- Added a clear of the header_corrupt flag to the end of the test handler in intel_dp.c
- Changed the usage of the new function prototype in several places to use NULL where it is not needed by compliance testing
V7:
- Updated to account for long_pulse flag propagation
V8:
- Removed clearing of header_corrupt flag from the test handler in intel_dp.c
- Added clearing of header_corrupt flag in the drm_edid_block_valid function

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

Cc [email protected]

---
  drivers/gpu/drm/drm_edid.c      | 35 +++++++++++++++++++++++++++++------
  drivers/gpu/drm/drm_edid_load.c |  7 +++++--
  drivers/gpu/drm/i915/intel_dp.c |  2 +-
  include/drm/drm_crtc.h          |  8 +++++++-
  4 files changed, 42 insertions(+), 10 deletions(-)

drivers/gpu/drm/drm_edid.c +15 -3

index 53bc7a6..6d037f9 100644
@@ -1041,13 +1041,15 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
   * @raw_edid: pointer to raw EDID block
   * @block: type of block to validate (0 for base, extension otherwise)
   * @print_bad_edid: if true, dump bad EDID blocks to the console
+ * @header_corrupt: if true, the header or checksum is invalid
   *
   * Validate a base or extension EDID block and optionally dump bad blocks to
   * the console.
   *
   * Return: True if the block is valid, false otherwise.
   */
-bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
+bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
+                         bool *header_corrupt)
  {
         u8 csum;
         struct edid *edid = (struct edid *)raw_edid;

@@ -1060,11 +1062,28 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
 
         if (block == 0) {
                 int score = drm_edid_header_is_valid(raw_edid);
-               if (score == 8) ;
-               else if (score >= edid_fixup) {
+               if (score == 8) {
+                       if (header_corrupt)
+                               *header_corrupt = 0;
+               } else if (score >= edid_fixup) {
+                       /* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6
+                        * In order to properly generate the invalid checksum
+                        * required for this test, it must be generated using
+                        * the raw EDID data. Otherwise, the fix-up code here
+                        * will correct the problem, the checksum is correct
+                        * and the test fails
+                        */
+                       csum = drm_edid_block_checksum(raw_edid);

drivers/gpu/drm/drm_edid.c bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) · line 1080

Quoted 4 lines

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

+                       if (csum) {
+                               if (header_corrupt)
+                                       *header_corrupt = 1;
+                       }

I was ready to give the R-B tag, but then I realized this. Shouldn't we be setting header_corrupt here regardless of the checksum value? I mean, score is not 8, so the header is indeed corrupt.

Also, setting "header_corrupt" to 1 in case the checksum is invalid is somewhat a distortion of meaning in the variable. If we really need the checksum, we should probably have something like connector->raw_checksum, or maybe rename header_corrupt to edid_corrupt, where "edid_corrupt" means "either invalid header or invalid checksum"?

And for compliance testing specifically, as far as I understand, I see on patch 7 that you only care about the checksum if the header is valid, so we don't seem to need the csum value here, so it seems my suggestion to just assign header_corrupt=1 regardless of the checksum value seems to be enough to solve your problem.

Sorry for not realizing this earlier, the back-and-forth between patches royally confused me.

Quoted 104 lines

drivers/gpu/drm/drm_edid.c +10 -3

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

@@ -1129,7 +1148,7 @@ bool drm_edid_is_valid(struct edid *edid)
                 return false;
 
         for (i = 0; i <= edid->extensions; i++)
-               if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
+               if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true, NULL))
                         return false;
 
         return true;

@@ -1232,7 +1251,8 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
         for (i = 0; i < 4; i++) {
                 if (get_edid_block(data, block, 0, EDID_LENGTH))
                         goto out;
-               if (drm_edid_block_valid(block, 0, print_bad_edid))
+               if (drm_edid_block_valid(block, 0, print_bad_edid,
+                                        &connector->edid_header_corrupt))
                         break;
                 if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
                         connector->null_edid_counter++;

@@ -1257,7 +1277,10 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
                                   block + (valid_extensions + 1) * EDID_LENGTH,
                                   j, EDID_LENGTH))
                                 goto out;
-                       if (drm_edid_block_valid(block + (valid_extensions + 1) * EDID_LENGTH, j, print_bad_edid)) {
+                       if (drm_edid_block_valid(block + (valid_extensions + 1)
+                                                * EDID_LENGTH, j,
+                                                print_bad_edid,
+                                                NULL)) {
                                 valid_extensions++;
                                 break;
                         }

drivers/gpu/drm/drm_edid_load.c +5 -2

index 4c0aa97..48b48e8 100644
@@ -216,7 +216,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
                 goto out;
         }
 
-       if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
+       if (!drm_edid_block_valid(edid, 0, print_bad_edid,
+                                 &connector->edid_header_corrupt)) {
                 connector->bad_edid_counter++;
                 DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
                     name);

@@ -229,7 +230,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
                 if (i != valid_extensions + 1)
                         memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
                             edid + i * EDID_LENGTH, EDID_LENGTH);
-               if (drm_edid_block_valid(edid + i * EDID_LENGTH, i, print_bad_edid))
+               if (drm_edid_block_valid(edid + i * EDID_LENGTH, i,
+                                        print_bad_edid,
+                                        NULL))
                         valid_extensions++;
         }
 

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

index 7ca0e18..97224ff 100644
@@ -4139,7 +4139,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp, bool long_hpd)
          */
         if (long_hpd) {
                 edid_read = drm_get_edid(connector, adapter);
-               if (!edid_read) {
+               if (!edid_read || connector->edid_header_corrupt == 1) {
                         DRM_DEBUG_DRIVER("Invalid EDID detected\n");
                 } else {
                         kfree(edid_read);

include/drm/drm_crtc.h +7 -1

index d4e4b82..f7a4a92 100644
@@ -719,6 +719,11 @@ struct drm_connector {
         int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */
         unsigned bad_edid_counter;
 
+       /* Flag for raw EDID header corruption - used in Displayport
+        * compliance testing - * Displayport Link CTS Core 1.2 rev1.1 4.2.2.6
+        */
+       bool edid_header_corrupt;
+
         struct dentry *debugfs_entry;
 
         struct drm_connector_state *state;

@@ -1443,7 +1448,8 @@ extern void drm_set_preferred_mode(struct drm_connector *connector,
                                    int hpref, int vpref);
 
  extern int drm_edid_header_is_valid(const u8 *raw_edid);
-extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid);
+extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
+                                bool *header_corrupt);
  extern bool drm_edid_is_valid(struct edid *edid);
 

Quoted 8 lines

include/drm/drm_crtc.h +0 -1

  extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
--

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

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

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

Cc [email protected]

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

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

index 71dcbc6..7f0356e 100644
@@ -432,7 +432,7 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
  */
 static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 {
-       unsigned int retry;
+       unsigned int retry, defer_i2c;
        int ret;
 
        /*

@@ -440,7 +440,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
         * is required to retry at least seven times upon receiving AUX_DEFER
         * before giving up the AUX transaction.
         */
-       for (retry = 0; retry < 7; retry++) {
+       for (retry = 0, defer_i2c = 0; retry < (7 + defer_i2c); retry++) {
                mutex_lock(&aux->hw_mutex);
                ret = aux->transfer(aux, msg);
                mutex_unlock(&aux->hw_mutex);

@@ -499,7 +499,13 @@ 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");
+                       /* DP Compliance Test 4.2.2.5 Requirement:
+                        * Must have at least 7 retries for I2C defers on the
+                        * transaction to pass this test
+                        */
                        aux->i2c_defer_count++;
+                       if (defer_i2c < 7)
+                               defer_i2c++;
                        usleep_range(400, 500);
                        continue;
 
--

1.9.1

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

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

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

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

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

V2:
- Updated the test active variable name to match the change in the initial patch of the series
V3:
- Added a fix in the test_active_write function to prevent a NULL pointer dereference if the encoder on the connector is invalid

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

---
 drivers/gpu/drm/i915/i915_debugfs.c | 209 ++++++++++++++++++++++++++++++++++++
 1 file changed, 209 insertions(+)

drivers/gpu/drm/i915/i915_debugfs.c +22 -0

index 2394924..c33d390 100644
@@ -3937,6 +3937,212 @@ static const struct file_operations i915_display_crc_ctl_fops = {
        .write = display_crc_ctl_write
 };
 
+static ssize_t i915_displayport_test_active_write(struct file *file,
+                                           const char __user *ubuf,
+                                           size_t len, loff_t *offp)
+{
+       char *input_buffer;
+       int status = 0;
+       struct seq_file *m;
+       struct drm_device *dev;
+       struct drm_connector *connector;
+       struct list_head *connector_list;
+       struct intel_dp *intel_dp;
+       int val = 0;
+
+       m = file->private_data;
+       if (!m) {
+               status = -ENODEV;
+               return status;
+       }
+       dev = m->private;
+
+       if (!dev) {
+               status = -ENODEV;

drivers/gpu/drm/i915/i915_debugfs.c +174 -0

+
+       if (copy_from_user(input_buffer, ubuf, len)) {
+               status = -EFAULT;
+               goto out;
+       }
+
+       input_buffer[len] = '\0';
+       DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
+
+       list_for_each_entry(connector, connector_list, head) {
+
+               if (connector->connector_type !=
+                   DRM_MODE_CONNECTOR_DisplayPort)
+                       continue;
+
+               if (connector->connector_type ==
+                   DRM_MODE_CONNECTOR_DisplayPort &&
+                   connector->status == connector_status_connected &&
+                   connector->encoder != NULL) {
+                       intel_dp = enc_to_intel_dp(connector->encoder);
+                       status = kstrtoint(input_buffer, 10, &val);
+                       if (status < 0)
+                               goto out;
+                       DRM_DEBUG_DRIVER("Got %d for test active\n", val);
+                       /* To prevent erroneous activation of the compliance
+                        * testing code, only accept an actual value of 1 here
+                        */
+                       if (val == 1)
+                               intel_dp->compliance_test_active = 1;
+                       else
+                               intel_dp->compliance_test_active = 0;
+               }
+       }
+out:
+       kfree(input_buffer);
+       if (status < 0)
+               return status;
+
+       *offp += len;
+       return len;
+}
+
+static int i915_displayport_test_active_show(struct seq_file *m, void *data)
+{
+       struct drm_device *dev = m->private;
+       struct drm_connector *connector;
+       struct list_head *connector_list = &dev->mode_config.connector_list;
+       struct intel_dp *intel_dp;
+
+       if (!dev)
+               return -ENODEV;
+
+       list_for_each_entry(connector, connector_list, head) {
+
+               if (connector->connector_type !=
+                   DRM_MODE_CONNECTOR_DisplayPort)
+                       continue;
+
+               if (connector->status == connector_status_connected &&
+                   connector->encoder != NULL) {
+                       intel_dp = enc_to_intel_dp(connector->encoder);
+                       if (intel_dp->compliance_test_active)
+                               seq_puts(m, "1");
+                       else
+                               seq_puts(m, "0");
+               } else
+                       seq_puts(m, "0");
+       }
+
+       return 0;
+}
+
+static int i915_displayport_test_active_open(struct inode *inode,
+                                      struct file *file)
+{
+       struct drm_device *dev = inode->i_private;
+
+       return single_open(file, i915_displayport_test_active_show, dev);
+}
+
+static const struct file_operations i915_displayport_test_active_fops = {
+       .owner = THIS_MODULE,
+       .open = i915_displayport_test_active_open,
+       .read = seq_read,
+       .llseek = seq_lseek,
+       .release = single_release,
+       .write = i915_displayport_test_active_write
+};
+
+static int i915_displayport_test_data_show(struct seq_file *m, void *data)
+{
+       struct drm_device *dev = m->private;
+       struct drm_connector *connector;
+       struct list_head *connector_list = &dev->mode_config.connector_list;
+       struct intel_dp *intel_dp;
+
+       if (!dev)
+               return -ENODEV;
+
+       list_for_each_entry(connector, connector_list, head) {
+
+               if (connector->connector_type !=
+                   DRM_MODE_CONNECTOR_DisplayPort)
+                       continue;
+
+               if (connector->status == connector_status_connected &&
+                   connector->encoder != NULL) {
+                       intel_dp = enc_to_intel_dp(connector->encoder);
+                       seq_printf(m, "%lx", intel_dp->compliance_test_data);
+               } else
+                       seq_puts(m, "0");
+       }
+
+       return 0;
+}
+static int i915_displayport_test_data_open(struct inode *inode,
+                                      struct file *file)
+{
+       struct drm_device *dev = inode->i_private;
+
+       return single_open(file, i915_displayport_test_data_show, dev);
+}
+
+static const struct file_operations i915_displayport_test_data_fops = {
+       .owner = THIS_MODULE,
+       .open = i915_displayport_test_data_open,
+       .read = seq_read,
+       .llseek = seq_lseek,
+       .release = single_release
+};
+
+static int i915_displayport_test_type_show(struct seq_file *m, void *data)
+{
+       struct drm_device *dev = m->private;
+       struct drm_connector *connector;
+       struct list_head *connector_list = &dev->mode_config.connector_list;
+       struct intel_dp *intel_dp;
+
+       if (!dev)
+               return -ENODEV;
+
+       list_for_each_entry(connector, connector_list, head) {
+
+               if (connector->connector_type !=
+                   DRM_MODE_CONNECTOR_DisplayPort)
+                       continue;
+
+               if (connector->status == connector_status_connected &&
+                   connector->encoder != NULL) {
+                       intel_dp = enc_to_intel_dp(connector->encoder);
+                       seq_printf(m, "%02lx", intel_dp->compliance_test_type);
+               } else
+                       seq_puts(m, "0");
+       }
+
+       return 0;
+}
+
+static int i915_displayport_test_type_open(struct inode *inode,
+                                      struct file *file)
+{
+       struct drm_device *dev = inode->i_private;
+
+       return single_open(file, i915_displayport_test_type_show, dev);
+}
+
+static const struct file_operations i915_displayport_test_type_fops = {
+       .owner = THIS_MODULE,
+       .open = i915_displayport_test_type_open,
+       .read = seq_read,
+       .llseek = seq_lseek,
+       .release = single_release
+};
+
 static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
 {
        struct drm_device *dev = m->private;