[PATCH 2/5] drm: Add edid_corrupt flag for Displayport Link CTS 4.2.2.6

[PATCH 2/5] drm: Add edid_corrupt flag for Displayport Link CTS 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
test never sees the invalid checksum. This results in a failure to pass the
compliance test.

To correct this issue, when the EDID code detects that the header is invalid,
a flag is set to indicate that the EDID is corrupted. In this case, it sets
edid_corrupt flag and continues with its fix-up code. 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 edid_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
V9:
- Renamed header_corrupt flag to edid_corrupt to more accurately reflect its
  value and purpose
- Updated commit message
V10:
- Updated for versioning and patch swizzle
- Revised the title to more accurately reflect the nature and contents of
  the patch

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

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 53bc7a6..69a5a09 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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
+ * @edid_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 *edid_corrupt)
 {
 	u8 csum;
 	struct edid *edid = (struct edid *)raw_edid;
@@ -1060,11 +1062,23 @@ 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 (edid_corrupt)
+				*edid_corrupt = 0;
+		} else if (score >= edid_fixup) {
+			/* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6
+			 * The corrupt flag needs to be set here otherwise, the 
+			 * fix-up code here will correct the problem, the 
+			 * checksum is correct and the test fails
+			 */
+			if (edid_corrupt)
+				*edid_corrupt = 1;
 			DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
 			memcpy(raw_edid, edid_header, sizeof(edid_header));
 		} else {
+			if (edid_corrupt) {
+				*edid_corrupt = 1;
+			}
 			goto bad;
 		}
 	}
@@ -1129,7 +1143,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 +1246,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_corrupt))
 			break;
 		if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
 			connector->null_edid_counter++;
@@ -1257,7 +1272,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;
 			}
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 4c0aa97..c5605fe 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_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++;
 	}
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index d4e4b82..8bc2724 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_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 *edid_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 2/5] drm: Add edid_corrupt flag for Displayport Link CTS 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
test never sees the invalid checksum. This results in a failure to pass the
compliance test.

To correct this issue, when the EDID code detects that the header is invalid,
a flag is set to indicate that the EDID is corrupted. In this case, it sets
edid_corrupt flag and continues with its fix-up code. This flag is also set in
the case of a more seriously damaged header (fixup score less than the
threshold). For consistency, the edid_corrupt flag is also set when the
checksum is invalid as well.

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
V9:
- Renamed header_corrupt flag to edid_corrupt to more accurately reflect its
  value and purpose
- Updated commit message
V10:
- Updated for versioning and patch swizzle
- Revised the title to more accurately reflect the nature and contents of
  the patch
- Fixed formatting/whitespace problems
- Added set flag when computed checksum is invalid

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

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 53bc7a6..be6713c 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -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
+ * @edid_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 *edid_corrupt)
 {
 	u8 csum;
 	struct edid *edid = (struct edid *)raw_edid;
@@ -1060,11 +1062,22 @@ 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 (edid_corrupt)
+				*edid_corrupt = 0;
+		} else if (score >= edid_fixup) {
+			/* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6
+			 * The corrupt flag needs to be set here otherwise, the
+			 * fix-up code here will correct the problem, the
+			 * checksum is correct and the test fails
+			 */
+			if (edid_corrupt)
+				*edid_corrupt = 1;
 			DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
 			memcpy(raw_edid, edid_header, sizeof(edid_header));
 		} else {
+			if (edid_corrupt)
+				*edid_corrupt = 1;
 			goto bad;
 		}
 	}
@@ -1075,6 +1088,9 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
 			DRM_ERROR("EDID checksum is invalid, remainder is %d\n", csum);
 		}
 
+		if (edid_corrupt)
+			*edid_corrupt = 1;
+
 		/* allow CEA to slide through, switches mangle this */
 		if (raw_edid[0] != 0x02)
 			goto bad;
@@ -1129,7 +1145,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 +1248,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_corrupt))
 			break;
 		if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
 			connector->null_edid_counter++;
@@ -1257,7 +1274,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;
 			}
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 4c0aa97..c5605fe 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_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++;
 	}
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index d4e4b82..8bc2724 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_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 *edid_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

Re: [PATCH 2/5] drm: Add edid_corrupt flag for Displayport Link CTS 4.2.2.6

[Alex Deucher]

On Tue, Apr 21, 2015 at 2:09 PM, Todd Previte <tprevite@gmail.com> 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
> test never sees the invalid checksum. This results in a failure to pass the
> compliance test.
>
> To correct this issue, when the EDID code detects that the header is invalid,
> a flag is set to indicate that the EDID is corrupted. In this case, it sets
> edid_corrupt flag and continues with its fix-up code. This flag is also set in
> the case of a more seriously damaged header (fixup score less than the
> threshold). For consistency, the edid_corrupt flag is also set when the
> checksum is invalid as well.
>
> 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
> V9:
> - Renamed header_corrupt flag to edid_corrupt to more accurately reflect its
>   value and purpose
> - Updated commit message
> V10:
> - Updated for versioning and patch swizzle
> - Revised the title to more accurately reflect the nature and contents of
>   the patch
> - Fixed formatting/whitespace problems
> - Added set flag when computed checksum is invalid
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> Cc: dri-devel@lists.freedesktop.org

Seems reasonable.

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

> ---
>  drivers/gpu/drm/drm_edid.c      | 32 ++++++++++++++++++++++++++------
>  drivers/gpu/drm/drm_edid_load.c |  7 +++++--
>  include/drm/drm_crtc.h          |  8 +++++++-
>  3 files changed, 38 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 53bc7a6..be6713c 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -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
> + * @edid_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 *edid_corrupt)
>  {
>         u8 csum;
>         struct edid *edid = (struct edid *)raw_edid;
> @@ -1060,11 +1062,22 @@ 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 (edid_corrupt)
> +                               *edid_corrupt = 0;
> +               } else if (score >= edid_fixup) {
> +                       /* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6
> +                        * The corrupt flag needs to be set here otherwise, the
> +                        * fix-up code here will correct the problem, the
> +                        * checksum is correct and the test fails
> +                        */
> +                       if (edid_corrupt)
> +                               *edid_corrupt = 1;
>                         DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
>                         memcpy(raw_edid, edid_header, sizeof(edid_header));
>                 } else {
> +                       if (edid_corrupt)
> +                               *edid_corrupt = 1;
>                         goto bad;
>                 }
>         }
> @@ -1075,6 +1088,9 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
>                         DRM_ERROR("EDID checksum is invalid, remainder is %d\n", csum);
>                 }
>
> +               if (edid_corrupt)
> +                       *edid_corrupt = 1;
> +
>                 /* allow CEA to slide through, switches mangle this */
>                 if (raw_edid[0] != 0x02)
>                         goto bad;
> @@ -1129,7 +1145,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 +1248,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_corrupt))
>                         break;
>                 if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
>                         connector->null_edid_counter++;
> @@ -1257,7 +1274,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;
>                         }
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 4c0aa97..c5605fe 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_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++;
>         }
>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index d4e4b82..8bc2724 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_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 *edid_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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm: Add edid_corrupt flag for Displayport Link CTS 4.2.2.6

[Paulo Zanoni]

2015-04-22 17:20 GMT-03:00 Alex Deucher <alexdeucher@gmail.com>:
> On Tue, Apr 21, 2015 at 2:09 PM, Todd Previte <tprevite@gmail.com> 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
>> test never sees the invalid checksum. This results in a failure to pass the
>> compliance test.
>>
>> To correct this issue, when the EDID code detects that the header is invalid,
>> a flag is set to indicate that the EDID is corrupted. In this case, it sets
>> edid_corrupt flag and continues with its fix-up code. This flag is also set in
>> the case of a more seriously damaged header (fixup score less than the
>> threshold). For consistency, the edid_corrupt flag is also set when the
>> checksum is invalid as well.
>>
>> 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
>> V9:
>> - Renamed header_corrupt flag to edid_corrupt to more accurately reflect its
>>   value and purpose
>> - Updated commit message
>> V10:
>> - Updated for versioning and patch swizzle
>> - Revised the title to more accurately reflect the nature and contents of
>>   the patch
>> - Fixed formatting/whitespace problems
>> - Added set flag when computed checksum is invalid
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> Cc: dri-devel@lists.freedesktop.org
>
> Seems reasonable.
>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
(since I made comments on the previous versions, this is my
documentation that the patch looks fine to me now)

>
>> ---
>>  drivers/gpu/drm/drm_edid.c      | 32 ++++++++++++++++++++++++++------
>>  drivers/gpu/drm/drm_edid_load.c |  7 +++++--
>>  include/drm/drm_crtc.h          |  8 +++++++-
>>  3 files changed, 38 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 53bc7a6..be6713c 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -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
>> + * @edid_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 *edid_corrupt)
>>  {
>>         u8 csum;
>>         struct edid *edid = (struct edid *)raw_edid;
>> @@ -1060,11 +1062,22 @@ 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 (edid_corrupt)
>> +                               *edid_corrupt = 0;
>> +               } else if (score >= edid_fixup) {
>> +                       /* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6
>> +                        * The corrupt flag needs to be set here otherwise, the
>> +                        * fix-up code here will correct the problem, the
>> +                        * checksum is correct and the test fails
>> +                        */
>> +                       if (edid_corrupt)
>> +                               *edid_corrupt = 1;
>>                         DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");
>>                         memcpy(raw_edid, edid_header, sizeof(edid_header));
>>                 } else {
>> +                       if (edid_corrupt)
>> +                               *edid_corrupt = 1;
>>                         goto bad;
>>                 }
>>         }
>> @@ -1075,6 +1088,9 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
>>                         DRM_ERROR("EDID checksum is invalid, remainder is %d\n", csum);
>>                 }
>>
>> +               if (edid_corrupt)
>> +                       *edid_corrupt = 1;
>> +
>>                 /* allow CEA to slide through, switches mangle this */
>>                 if (raw_edid[0] != 0x02)
>>                         goto bad;
>> @@ -1129,7 +1145,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 +1248,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_corrupt))
>>                         break;
>>                 if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
>>                         connector->null_edid_counter++;
>> @@ -1257,7 +1274,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;
>>                         }
>> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
>> index 4c0aa97..c5605fe 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_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++;
>>         }
>>
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index d4e4b82..8bc2724 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_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 *edid_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
> _______________________________________________
> 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

[PATCH 4/5] 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 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 <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 575172e..80a02a4 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -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
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Daniel Vetter]

On Sat, Apr 18, 2015 at 12:04:18AM -0700, Todd Previte wrote:
> 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 <tprevite@gmail.com>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Applied to topic/drm-misc.
-Daniel

> ---
>  drivers/gpu/drm/drm_dp_helper.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 575172e..80a02a4 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -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
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[PATCH 4/5] 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 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 <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 575172e..80a02a4 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -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
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel