Displayport compliance testing V7

Due to changes in the driver and to support Displayport compliance testing, the test request and sink IRQ logic has been relocated from intel_dp_check_link_status to intel_dp_detect. This is because the bulk of the compliance tests that set the TEST_REQUEST bit in the DEVICE_IRQ field of the DPCD issue a long pulse / hot plug event to signify the start of the test. Currently, for a long pulse, intel_dp_check_link_status is not called for a long HPD pulse, so if test requests come in, they cannot be detected by the driver.

Once located in the intel_dp_detect, in the regular hot plug event path, proper detection of Displayport compliance test requests occurs which then invokes the test handler to support them. Additionally, this places compliance testing in the normal operational paths, eliminating as much special case code as possible.

The only change in intel_dp_check_link_status with this patch is that when the IRQ is the result of a test request from the sink, the test handler is not invoked during the short pulse path. Short pulse test requests are for a particular variety of tests (mainly link training) that will be implemented in the future. Once those tests are available, the test request handler will be called from here as well.

V1:
- Rewored the commit message to be more clear about the content and intent of this patch
- Restore IRQ detection logic to intel_dp_check_link_status(). Continue to detect and clear sink IRQs in the short pulse case. Ignore test requests in the short pulses for now since they are for future test implementations.

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

---
 drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

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

index 2ef05d7..b249ee8 100644
@@ -4156,7 +4156,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
                                   sink_irq_vector);
 
                if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
-                       intel_dp_handle_test_request(intel_dp);
+                       DRM_DEBUG_DRIVER("Test request in short pulse not handled\n");
                if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
                        DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
        }

@@ -4386,6 +4386,7 @@ intel_dp_detect(struct drm_connector *connector, bool force)
        enum drm_connector_status status;
        enum intel_display_power_domain power_domain;
        bool ret;
+       u8 sink_irq_vector;
 
        DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
                      connector->base.id, connector->name);

@@ -4428,6 +4429,20 @@ intel_dp_detect(struct drm_connector *connector, bool force)
                intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
        status = connector_status_connected;
 
+       /* Try to read the source of the interrupt */
+       if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
+           intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
+               /* Clear interrupt source */
+               drm_dp_dpcd_writeb(&intel_dp->aux,
+                                  DP_DEVICE_SERVICE_IRQ_VECTOR,
+                                  sink_irq_vector);
+
+               if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
+                       intel_dp_handle_test_request(intel_dp);
+               if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
+                       DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
+       }
+
 out:
        intel_dp_power_put(intel_dp, power_domain);
        return status;
--

1.9.1

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

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

Unfortunately, the DRM EDID reading and parsing functions are actually too good in this case; the header is fixed before the checksum is computed and thus the 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 <[email protected]>

Cc [email protected]

---
 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(-)

drivers/gpu/drm/drm_edid.c +26 -6

index 53bc7a6..be6713c 100644
@@ -1041,13 +1041,15 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
  * @raw_edid: pointer to raw EDID block
  * @block: type of block to validate (0 for base, extension otherwise)
  * @print_bad_edid: if true, dump bad EDID blocks to the console
+ * @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;
                        }

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

index 4c0aa97..c5605fe 100644
@@ -216,7 +216,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
                goto out;
        }
 
-       if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
+       if (!drm_edid_block_valid(edid, 0, print_bad_edid,
+                                 &connector->edid_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++;
        }
 

include/drm/drm_crtc.h +7 -2

index d4e4b82..8bc2724 100644
@@ -719,6 +719,11 @@ struct drm_connector {
        int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */
        unsigned bad_edid_counter;
 
+       /* Flag for raw EDID header corruption - used in Displayport
+        * compliance testing - * Displayport Link CTS Core 1.2 rev1.1 4.2.2.6
+        */
+       bool edid_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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 62 lines

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

Unfortunately, the DRM EDID reading and parsing functions are actually too good in this case; the header is fixed before the checksum is computed and thus the 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

Quoted 2 lines

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

Cc [email protected]

Seems reasonable.

Reviewed-by Alex Deucher <[email protected]>

Quoted 148 lines

---
 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(-)

drivers/gpu/drm/drm_edid.c +26 -6

index 53bc7a6..be6713c 100644
@@ -1041,13 +1041,15 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
  * @raw_edid: pointer to raw EDID block
  * @block: type of block to validate (0 for base, extension otherwise)
  * @print_bad_edid: if true, dump bad EDID blocks to the console
+ * @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;
                        }

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

index 4c0aa97..c5605fe 100644
@@ -216,7 +216,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
                goto out;
        }
 
-       if (!drm_edid_block_valid(edid, 0, print_bad_edid)) {
+       if (!drm_edid_block_valid(edid, 0, print_bad_edid,
+                                 &connector->edid_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++;
        }
 

include/drm/drm_crtc.h +7 -2

index d4e4b82..8bc2724 100644
@@ -719,6 +719,11 @@ struct drm_connector {
        int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */
        unsigned bad_edid_counter;
 
+       /* Flag for raw EDID header corruption - used in Displayport
+        * compliance testing - * Displayport Link CTS Core 1.2 rev1.1 4.2.2.6
+        */
+       bool edid_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

Quoted 4 lines

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

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

The kerneldoc for this newly added parameter was missing from the original patch. This patch adds the appropriate kerneldoc entry.

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

---
 include/drm/drm_crtc.h | 1 +
 1 file changed, 1 insertion(+)

include/drm/drm_crtc.h +1 -0

index 8bc2724..9cd290c 100644
@@ -647,6 +647,7 @@ struct drm_encoder {
  * @audio_latency: audio latency info from ELD, if found
  * @null_edid_counter: track sinks that give us all zeros for the EDID
  * @bad_edid_counter: track sinks that give us an EDID with invalid checksum
+ * @edid_corrupt: flag for EDID header corruption detection
  * @debugfs_entry: debugfs directory for this connector
  * @state: current atomic state for this connector
  * @has_tile: is this connector connected to a tiled monitor

Signature · 6 lines

1.9.1

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

Updates the EDID compliance test function to perform the analyze and react to the EDID data read as a result of a hot plug event. The results of this analysis are handed off to userspace so that the userspace app can set the display mode appropriately for the test result/response.

The compliance_test_active flag now appears at the end of the individual test handling functions. This is so that the kernel-side operations can be completed without the risk of interruption from the userspace app that is polling on that flag.

V2:
- Addressed mailing list feedback
- Removed excess debug messages
- Removed extraneous comments
- Fixed formatting issues (line length > 80)
- Updated the debug message in compute_edid_checksum to output hex values instead of decimal
V3:
- Addressed more list feedback
- Added the test_active flag to the autotest function
- Removed test_active flag from handler
- Added failsafe check on the compliance test active flag
at the end of the test handler
- Fixed checkpatch.pl issues
V4:
- Removed the checksum computation function and its use as it has been rendered superfluous by changes to the core DRM EDID functions
- Updated to use the raw header corruption detection mechanism
- Moved the declaration of the test_data variable here
V5:
- Update test active flag variable name to match the change in the first patch of the series.
- Relocated the test active flag declaration and initialization to this patch
V6:
- Updated to use the new flag for raw EDID header corruption
- Removed the extra EDID read from the autotest function
- Added the edid_checksum variable to struct intel_dp so that the autotest function can write it to the sink device
- Moved the update to the hpd_pulse function to another patch
- Removed extraneous constants
V7:
- Fixed erroneous placement of the checksum assignment. In some cases such as when the EDID read fails and is NULL, this causes a NULL ptr dereference in the kernel. Bad news. Fixed now.
V8:
- Updated to support the kfree() on the EDID data added previously V9:
- Updated for the long_hpd flag propagation
V10:
- Updated to use actual checksum from the EDID read that occurs during normal hot plug path execution
- Removed variables from intel_dp struct that are no longer needed
- Updated the patch subject to more closely match the nature and contents of the patch

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

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

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

index 55d1f5f..4c1f6a9 100644

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

+#define INTEL_DP_RESOLUTION_PREFERRED  (1 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define INTEL_DP_RESOLUTION_STANDARD   (2 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define INTEL_DP_RESOLUTION_FAILSAFE   (3 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+
 struct dp_link_dpll {
        int link_bw;
        struct dpll dpll;

@@ -3994,6 +4000,38 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
 {
        uint8_t test_result = DP_TEST_NAK;
+       uint32_t ret = 0;
+       struct intel_connector *intel_connector = intel_dp->attached_connector;
+       struct drm_connector *connector = &intel_connector->base;
+
+       if (intel_connector->detect_edid == NULL ||
+           connector->edid_corrupt == 1 ||
+           intel_dp->aux.i2c_defer_count > 6) {
+               /* Check for NACKs/DEFERs, use failsafe if detected
+                *  (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5)
+                */
+               if (intel_dp->aux.i2c_nack_count > 0 ||
+                       intel_dp->aux.i2c_defer_count > 0)
+                       DRM_DEBUG_KMS("EDID read had %d NACKs, %d DEFERs\n",
+                                     intel_dp->aux.i2c_nack_count,

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

+
+               test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
+               intel_dp->compliance_test_data = INTEL_DP_RESOLUTION_STANDARD;
+       }
+
+       /* Set test active flag here so userspace doesn't interrupt things */
+       intel_dp->compliance_test_active = 1;
+
        return test_result;
 }
 

@@ -4009,7 +4047,10 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
        uint8_t rxdata = 0;
        int status = 0;
 
+       intel_dp->compliance_test_active = 0;
        intel_dp->compliance_test_type = 0;
+       intel_dp->compliance_test_data = 0;
+
        intel_dp->aux.i2c_nack_count = 0;
        intel_dp->aux.i2c_defer_count = 0;
 

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

index a4675fa..6c71be9 100644
@@ -742,6 +742,8 @@ struct intel_dp {
 
        /* Displayport compliance testing */
        unsigned long compliance_test_type;
+       unsigned long compliance_test_data;
+       bool compliance_test_active;
 };
 
 struct intel_digital_port {
--

1.9.1

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

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

Quoted 67 lines

Updates the EDID compliance test function to perform the analyze and react to the EDID data read as a result of a hot plug event. The results of this analysis are handed off to userspace so that the userspace app can set the display mode appropriately for the test result/response.

The compliance_test_active flag now appears at the end of the individual test handling functions. This is so that the kernel-side operations can be completed without the risk of interruption from the userspace app that is polling on that flag.

V2:
- Addressed mailing list feedback
- Removed excess debug messages
- Removed extraneous comments
- Fixed formatting issues (line length > 80)
- Updated the debug message in compute_edid_checksum to output hex values instead of decimal
V3:
- Addressed more list feedback
- Added the test_active flag to the autotest function
- Removed test_active flag from handler
- Added failsafe check on the compliance test active flag
at the end of the test handler
- Fixed checkpatch.pl issues
V4:
- Removed the checksum computation function and its use as it has been rendered superfluous by changes to the core DRM EDID functions
- Updated to use the raw header corruption detection mechanism
- Moved the declaration of the test_data variable here
V5:
- Update test active flag variable name to match the change in the first patch of the series.
- Relocated the test active flag declaration and initialization to this patch
V6:
- Updated to use the new flag for raw EDID header corruption
- Removed the extra EDID read from the autotest function
- Added the edid_checksum variable to struct intel_dp so that the autotest function can write it to the sink device
- Moved the update to the hpd_pulse function to another patch
- Removed extraneous constants
V7:
- Fixed erroneous placement of the checksum assignment. In some cases such as when the EDID read fails and is NULL, this causes a NULL ptr dereference in the kernel. Bad news. Fixed now.
V8:
- Updated to support the kfree() on the EDID data added previously V9:
- Updated for the long_hpd flag propagation
V10:
- Updated to use actual checksum from the EDID read that occurs during normal hot plug path execution
- Removed variables from intel_dp struct that are no longer needed
- Updated the patch subject to more closely match the nature and contents of the patch

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

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

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

index 55d1f5f..4c1f6a9 100644

drivers/gpu/drm/i915/intel_dp.c line 45

Quoted 5 lines

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

@@ -41,6 +41,12 @@
 
 #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)
 
+/* Compliance test status bits  */
+#define INTEL_DP_RESOLUTION_SHIFT_MASK 4

Now that you cleaned all the other stuff, can't we start at 0 instead of 4? (of course, this would require a change to the user space tool)

Quoted 25 lines

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

+#define INTEL_DP_RESOLUTION_PREFERRED  (1 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define INTEL_DP_RESOLUTION_STANDARD   (2 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define INTEL_DP_RESOLUTION_FAILSAFE   (3 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+
 struct dp_link_dpll {
        int link_bw;
        struct dpll dpll;

@@ -3994,6 +4000,38 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
 {
        uint8_t test_result = DP_TEST_NAK;
+       uint32_t ret = 0;
+       struct intel_connector *intel_connector = intel_dp->attached_connector;
+       struct drm_connector *connector = &intel_connector->base;
+
+       if (intel_connector->detect_edid == NULL ||
+           connector->edid_corrupt == 1 ||
+           intel_dp->aux.i2c_defer_count > 6) {
+               /* Check for NACKs/DEFERs, use failsafe if detected
+                *  (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5)
+                */
+               if (intel_dp->aux.i2c_nack_count > 0 ||
+                       intel_dp->aux.i2c_defer_count > 0)
+                       DRM_DEBUG_KMS("EDID read had %d NACKs, %d DEFERs\n",
+                                     intel_dp->aux.i2c_nack_count,

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

Quoted 10 lines

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

+                                     intel_dp->aux.i2c_defer_count);
+               intel_dp->compliance_test_data = INTEL_DP_RESOLUTION_FAILSAFE;
+       } else {
+               ret = drm_dp_dpcd_write(&intel_dp->aux,
+                                       DP_TEST_EDID_CHECKSUM,
+                                       &intel_connector->detect_edid->checksum, 1);
+               if (ret <= 0)
+                       DRM_DEBUG_DRIVER("Failed to write EDID checksum\n");
+               else
+                       DRM_DEBUG_DRIVER("EDID checksum written to sink\n");

You're mixing DRM_DEBUG_DRIVER and DRM_DEBUG_KMS. I'd stick with only one, and I'd probably pick _KMS.

With or without the above bikesheds: Reviewed-by: Paulo Zanoni <[email protected]>

Quoted 43 lines

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

+
+               test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
+               intel_dp->compliance_test_data = INTEL_DP_RESOLUTION_STANDARD;
+       }
+
+       /* Set test active flag here so userspace doesn't interrupt things */
+       intel_dp->compliance_test_active = 1;
+
        return test_result;
 }
 

@@ -4009,7 +4047,10 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
        uint8_t rxdata = 0;
        int status = 0;
 
+       intel_dp->compliance_test_active = 0;
        intel_dp->compliance_test_type = 0;
+       intel_dp->compliance_test_data = 0;
+
        intel_dp->aux.i2c_nack_count = 0;
        intel_dp->aux.i2c_defer_count = 0;
 

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

index a4675fa..6c71be9 100644
@@ -742,6 +742,8 @@ struct intel_dp {
 
        /* Displayport compliance testing */
        unsigned long compliance_test_type;
+       unsigned long compliance_test_data;
+       bool compliance_test_active;
 };
 
 struct intel_digital_port {
--

1.9.1

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

Signature · 5 lines

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

Quoted 117 lines · Paulo Zanoni

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

Quoted 67 lines

Updates the EDID compliance test function to perform the analyze and react to the EDID data read as a result of a hot plug event. The results of this analysis are handed off to userspace so that the userspace app can set the display mode appropriately for the test result/response.

The compliance_test_active flag now appears at the end of the individual test handling functions. This is so that the kernel-side operations can be completed without the risk of interruption from the userspace app that is polling on that flag.

V2:
- Addressed mailing list feedback
- Removed excess debug messages
- Removed extraneous comments
- Fixed formatting issues (line length > 80)
- Updated the debug message in compute_edid_checksum to output hex values instead of decimal
V3:
- Addressed more list feedback
- Added the test_active flag to the autotest function
- Removed test_active flag from handler
- Added failsafe check on the compliance test active flag
at the end of the test handler
- Fixed checkpatch.pl issues
V4:
- Removed the checksum computation function and its use as it has been rendered superfluous by changes to the core DRM EDID functions
- Updated to use the raw header corruption detection mechanism
- Moved the declaration of the test_data variable here
V5:
- Update test active flag variable name to match the change in the first patch of the series.
- Relocated the test active flag declaration and initialization to this patch
V6:
- Updated to use the new flag for raw EDID header corruption
- Removed the extra EDID read from the autotest function
- Added the edid_checksum variable to struct intel_dp so that the autotest function can write it to the sink device
- Moved the update to the hpd_pulse function to another patch
- Removed extraneous constants
V7:
- Fixed erroneous placement of the checksum assignment. In some cases such as when the EDID read fails and is NULL, this causes a NULL ptr dereference in the kernel. Bad news. Fixed now.
V8:
- Updated to support the kfree() on the EDID data added previously V9:
- Updated for the long_hpd flag propagation
V10:
- Updated to use actual checksum from the EDID read that occurs during normal hot plug path execution
- Removed variables from intel_dp struct that are no longer needed
- Updated the patch subject to more closely match the nature and contents of the patch

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

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

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

index 55d1f5f..4c1f6a9 100644

drivers/gpu/drm/i915/intel_dp.c line 45

Quoted 5 lines

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

@@ -41,6 +41,12 @@
 
 #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)
 
+/* Compliance test status bits  */
+#define INTEL_DP_RESOLUTION_SHIFT_MASK 4

Now that you cleaned all the other stuff, can't we start at 0 instead of 4? (of course, this would require a change to the user space tool)

Quoted 25 lines

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

+#define INTEL_DP_RESOLUTION_PREFERRED  (1 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define INTEL_DP_RESOLUTION_STANDARD   (2 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define INTEL_DP_RESOLUTION_FAILSAFE   (3 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+
 struct dp_link_dpll {
        int link_bw;
        struct dpll dpll;

@@ -3994,6 +4000,38 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
 {
        uint8_t test_result = DP_TEST_NAK;
+       uint32_t ret = 0;
+       struct intel_connector *intel_connector = intel_dp->attached_connector;
+       struct drm_connector *connector = &intel_connector->base;
+
+       if (intel_connector->detect_edid == NULL ||
+           connector->edid_corrupt == 1 ||
+           intel_dp->aux.i2c_defer_count > 6) {
+               /* Check for NACKs/DEFERs, use failsafe if detected
+                *  (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5)
+                */
+               if (intel_dp->aux.i2c_nack_count > 0 ||
+                       intel_dp->aux.i2c_defer_count > 0)
+                       DRM_DEBUG_KMS("EDID read had %d NACKs, %d DEFERs\n",
+                                     intel_dp->aux.i2c_nack_count,

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

Quoted 10 lines

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

+                                     intel_dp->aux.i2c_defer_count);
+               intel_dp->compliance_test_data = INTEL_DP_RESOLUTION_FAILSAFE;
+       } else {
+               ret = drm_dp_dpcd_write(&intel_dp->aux,
+                                       DP_TEST_EDID_CHECKSUM,
+                                       &intel_connector->detect_edid->checksum, 1);
+               if (ret <= 0)
+                       DRM_DEBUG_DRIVER("Failed to write EDID checksum\n");
+               else
+                       DRM_DEBUG_DRIVER("EDID checksum written to sink\n");

You're mixing DRM_DEBUG_DRIVER and DRM_DEBUG_KMS. I'd stick with only one, and I'd probably pick _KMS.

Quoted 2 lines

With or without the above bikesheds: Reviewed-by: Paulo Zanoni <[email protected]>

Yeah I second the two polish requests from Paulo. Merged the previous patch meanwhile.

-Daniel

Quoted 52 lines

Quoted 43 lines

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

+
+               test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
+               intel_dp->compliance_test_data = INTEL_DP_RESOLUTION_STANDARD;
+       }
+
+       /* Set test active flag here so userspace doesn't interrupt things */
+       intel_dp->compliance_test_active = 1;
+
        return test_result;
 }
 

@@ -4009,7 +4047,10 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
        uint8_t rxdata = 0;
        int status = 0;
 
+       intel_dp->compliance_test_active = 0;
        intel_dp->compliance_test_type = 0;
+       intel_dp->compliance_test_data = 0;
+
        intel_dp->aux.i2c_nack_count = 0;
        intel_dp->aux.i2c_defer_count = 0;
 

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

index a4675fa..6c71be9 100644
@@ -742,6 +742,8 @@ struct intel_dp {
 
        /* Displayport compliance testing */
        unsigned long compliance_test_type;
+       unsigned long compliance_test_data;
+       bool compliance_test_active;
 };
 
 struct intel_digital_port {
--

1.9.1

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

Signature · 5 lines

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

Signature · 7 lines

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

Updates the EDID compliance test function to perform the analyze and react to the EDID data read as a result of a hot plug event. The results of this analysis are handed off to userspace so that the userspace app can set the display mode appropriately for the test result/response.

The compliance_test_active flag now appears at the end of the individual test handling functions. This is so that the kernel-side operations can be completed without the risk of interruption from the userspace app that is polling on that flag.

V2:
- Addressed mailing list feedback
- Removed excess debug messages
- Removed extraneous comments
- Fixed formatting issues (line length > 80)
- Updated the debug message in compute_edid_checksum to output hex values instead of decimal
V3:
- Addressed more list feedback
- Added the test_active flag to the autotest function
- Removed test_active flag from handler
- Added failsafe check on the compliance test active flag
at the end of the test handler
- Fixed checkpatch.pl issues
V4:
- Removed the checksum computation function and its use as it has been rendered superfluous by changes to the core DRM EDID functions
- Updated to use the raw header corruption detection mechanism
- Moved the declaration of the test_data variable here
V5:
- Update test active flag variable name to match the change in the first patch of the series.
- Relocated the test active flag declaration and initialization to this patch
V6:
- Updated to use the new flag for raw EDID header corruption
- Removed the extra EDID read from the autotest function
- Added the edid_checksum variable to struct intel_dp so that the autotest function can write it to the sink device
- Moved the update to the hpd_pulse function to another patch
- Removed extraneous constants
V7:
- Fixed erroneous placement of the checksum assignment. In some cases such as when the EDID read fails and is NULL, this causes a NULL ptr dereference in the kernel. Bad news. Fixed now.
V8:
- Updated to support the kfree() on the EDID data added previously V9:
- Updated for the long_hpd flag propagation
V10:
- Updated to use actual checksum from the EDID read that occurs during normal hot plug path execution
- Removed variables from intel_dp struct that are no longer needed
- Updated the patch subject to more closely match the nature and contents of the patch
- Fixed formatting problem (long line)
V11:
- Removed extra debug messages
- Updated comments to be more informative
- Removed extra variable
V12:
- Removed the 4 bit offset of the resolution setting in compliance data
- Changed to DRM_DEBUG_KMS instead of DRM_DEBUG_DRIVER

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

Reviewed-by Paulo Zanoni <[email protected]>

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

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

index b249ee8..a59bd75 100644
@@ -41,6 +41,12 @@
 
 #define DP_LINK_CHECK_TIMEOUT	(10 * 1000)
 
+/* Compliance test status bits  */
+#define INTEL_DP_RESOLUTION_SHIFT_MASK	0
+#define INTEL_DP_RESOLUTION_PREFERRED	(1 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define INTEL_DP_RESOLUTION_STANDARD	(2 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define INTEL_DP_RESOLUTION_FAILSAFE	(3 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+
 struct dp_link_dpll {
 	int link_bw;
 	struct dpll dpll;

@@ -3994,6 +4000,39 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
 {
 	uint8_t test_result = DP_TEST_NAK;
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct drm_connector *connector = &intel_connector->base;
+
+	if (intel_connector->detect_edid == NULL ||
+	    connector->edid_corrupt == 1 ||
+	    intel_dp->aux.i2c_defer_count > 6) {
+		/* Check EDID read for NACKs, DEFERs and corruption
+		 * (DP CTS 1.2 Core r1.1)
+		 *    4.2.2.4 : Failed EDID read, I2C_NAK
+		 *    4.2.2.5 : Failed EDID read, I2C_DEFER
+		 *    4.2.2.6 : EDID corruption detected
+		 * Use failsafe mode for all cases
+		 */
+		if (intel_dp->aux.i2c_nack_count > 0 ||
+			intel_dp->aux.i2c_defer_count > 0)
+			DRM_DEBUG_KMS("EDID read had %d NACKs, %d DEFERs\n",
+				      intel_dp->aux.i2c_nack_count,
+				      intel_dp->aux.i2c_defer_count);
+		intel_dp->compliance_test_data = INTEL_DP_RESOLUTION_FAILSAFE;
+	} else {
+		if (!drm_dp_dpcd_write(&intel_dp->aux,
+					DP_TEST_EDID_CHECKSUM,
+					&intel_connector->detect_edid->checksum,
+					1));
+			DRM_DEBUG_KMS("Failed to write EDID checksum\n");
+
+		test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
+		intel_dp->compliance_test_data = INTEL_DP_RESOLUTION_STANDARD;
+	}
+
+	/* Set test active flag here so userspace doesn't interrupt things */
+	intel_dp->compliance_test_active = 1;
+
 	return test_result;
 }
 

@@ -4009,7 +4048,10 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 	uint8_t rxdata = 0;
 	int status = 0;
 
+	intel_dp->compliance_test_active = 0;
 	intel_dp->compliance_test_type = 0;
+	intel_dp->compliance_test_data = 0;
+
 	intel_dp->aux.i2c_nack_count = 0;
 	intel_dp->aux.i2c_defer_count = 0;
 

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

index a4675fa..6c71be9 100644
@@ -742,6 +742,8 @@ struct intel_dp {
 
 	/* Displayport compliance testing */
 	unsigned long compliance_test_type;
+	unsigned long compliance_test_data;
+	bool compliance_test_active;
 };
 
 struct intel_digital_port {

Signature · 6 lines

1.9.1

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

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

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

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

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

Cc [email protected]

Reviewed-by Paulo Zanoni <[email protected]>

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

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

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

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

@@ -499,7 +499,13 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 
 		case DP_AUX_I2C_REPLY_DEFER:
 			DRM_DEBUG_KMS("I2C defer\n");
+			/* DP Compliance Test 4.2.2.5 Requirement:
+			 * Must have at least 7 retries for I2C defers on the
+			 * transaction to pass this test
+			 */
 			aux->i2c_defer_count++;
+			if (defer_i2c < 7)
+				defer_i2c++;
 			usleep_range(400, 500);
 			continue;
 

Signature · 6 lines

1.9.1

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