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

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

[Todd Previte]

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

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

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

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

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

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

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

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

[Todd Previte]

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

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

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

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

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

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

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

[Todd Previte]

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

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

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

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

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

[Alex Deucher]

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

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

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

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

[Todd Previte]

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

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

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

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

[Todd Previte]

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

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

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

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

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

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

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

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

[Todd Previte]

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

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

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

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

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

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

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

[Emil Velikov]

Hi Todd

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

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

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

[Todd Previte]

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

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

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

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

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

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

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

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

[Paulo Zanoni]

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

Bad chunk...


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

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


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

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

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


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

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


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

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


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

Same here: not iterating over block 0.

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


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



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

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

[Todd Previte]

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

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