| Message ID | 1418255597-4716-12-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>: > Updates the EDID compliance test function to perform the EDID read as > required by the tests. This read needs to take place in the kernel for > reasons of speed and efficiency. The results of the EDID read are handed > off to userspace so that the remainder of the test can be conducted there. > > 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 > > Signed-off-by: Todd Previte <tprevite@gmail.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 71 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index b6f5a72..2a13124 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -40,6 +40,13 @@ > > #define DP_LINK_CHECK_TIMEOUT (10 * 1000) > > +/* Compliance test status bits */ > +#define INTEL_DP_EDID_OK (0<<0) > +#define INTEL_DP_EDID_CORRUPT (1<<0) > +#define INTEL_DP_RESOLUTION_PREFERRED (1<<2) > +#define INTEL_DP_RESOLUTION_STANDARD (1<<3) > +#define INTEL_DP_RESOLUTION_FAILSAFE (1<<4) > + > struct dp_link_dpll { > int link_bw; > struct dpll dpll; > @@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) > return test_result; > } > > +static bool intel_dp_compute_edid_checksum(uint8_t *edid_data, > + uint8_t *edid_checksum) > +{ > + uint32_t byte_total = 0; > + uint8_t i = 0; > + bool edid_ok = true; > + > + /* Don't include last byte (the checksum) in the computation */ > + for (i = 0; i < EDID_LENGTH - 2; i++) Shouldn't this be "i < EDID_LENGHT - 1"? > + byte_total += edid_data[i]; > + > + *edid_checksum = 256 - (byte_total % 256); > + > + if (*edid_checksum != edid_data[EDID_LENGTH - 1]) { > + DRM_DEBUG_KMS("Invalid EDID checksum %02x, should be %02x\n", > + edid_data[EDID_LENGTH - 40 - 1], *edid_checksum); > + edid_ok = false; > + } > + > + return edid_ok; > +} > + > static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp) > { > - uint8_t test_result = DP_TEST_NAK; > + struct drm_connector *connector = &intel_dp->attached_connector->base; > + struct i2c_adapter *adapter = &intel_dp->aux.ddc; > + struct edid *edid_read = NULL; > + uint8_t *edid_data = NULL; > + uint8_t test_result = DP_TEST_NAK, checksum = 0; > + uint32_t ret = 0; > + > + intel_dp->aux.i2c_nack_count = 0; > + intel_dp->aux.i2c_defer_count = 0; > + > + edid_read = drm_get_edid(connector, adapter); > + > + if (edid_read == NULL) { > + /* 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, > + intel_dp->aux.i2c_defer_count); Don't we need to use these _count values somehow, instead of just printing them in the logs? Everything else looks fine. > + intel_dp->compliance_test_data = INTEL_DP_EDID_CORRUPT | > + INTEL_DP_RESOLUTION_FAILSAFE; > + } else { > + edid_data = (uint8_t *) edid_read; > + > + if (intel_dp_compute_edid_checksum(edid_data, &checksum)) { > + ret = drm_dp_dpcd_write(&intel_dp->aux, > + DP_TEST_EDID_CHECKSUM, > + &edid_read->checksum, 1); > + test_result = DP_TEST_ACK | > + DP_TEST_EDID_CHECKSUM_WRITE; > + intel_dp->compliance_test_data = > + INTEL_DP_EDID_OK | > + INTEL_DP_RESOLUTION_PREFERRED; > + } else { > + /* Invalid checksum - EDID corruption detection */ > + intel_dp->compliance_test_data = > + INTEL_DP_EDID_CORRUPT | > + INTEL_DP_RESOLUTION_FAILSAFE; > + } > + } > + > return test_result; > } > > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Dec 17, 2014 at 02:25:42PM -0200, Paulo Zanoni wrote: > 2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>: > > Updates the EDID compliance test function to perform the EDID read as > > required by the tests. This read needs to take place in the kernel for > > reasons of speed and efficiency. The results of the EDID read are handed > > off to userspace so that the remainder of the test can be conducted there. > > > > 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 > > > > Signed-off-by: Todd Previte <tprevite@gmail.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 71 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index b6f5a72..2a13124 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -40,6 +40,13 @@ > > > > #define DP_LINK_CHECK_TIMEOUT (10 * 1000) > > > > +/* Compliance test status bits */ > > +#define INTEL_DP_EDID_OK (0<<0) > > +#define INTEL_DP_EDID_CORRUPT (1<<0) > > +#define INTEL_DP_RESOLUTION_PREFERRED (1<<2) > > +#define INTEL_DP_RESOLUTION_STANDARD (1<<3) > > +#define INTEL_DP_RESOLUTION_FAILSAFE (1<<4) > > + > > struct dp_link_dpll { > > int link_bw; > > struct dpll dpll; > > @@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) > > return test_result; > > } > > > > +static bool intel_dp_compute_edid_checksum(uint8_t *edid_data, > > + uint8_t *edid_checksum) > > +{ > > + uint32_t byte_total = 0; > > + uint8_t i = 0; > > + bool edid_ok = true; > > + > > + /* Don't include last byte (the checksum) in the computation */ > > + for (i = 0; i < EDID_LENGTH - 2; i++) > > Shouldn't this be "i < EDID_LENGHT - 1"? > > > > + byte_total += edid_data[i]; > > + > > + *edid_checksum = 256 - (byte_total % 256); > > + > > + if (*edid_checksum != edid_data[EDID_LENGTH - 1]) { > > + DRM_DEBUG_KMS("Invalid EDID checksum %02x, should be %02x\n", > > + edid_data[EDID_LENGTH - 40 - 1], *edid_checksum); > > + edid_ok = false; > > + } > > + > > + return edid_ok; > > +} > > + > > static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp) > > { > > - uint8_t test_result = DP_TEST_NAK; > > + struct drm_connector *connector = &intel_dp->attached_connector->base; > > + struct i2c_adapter *adapter = &intel_dp->aux.ddc; > > + struct edid *edid_read = NULL; > > + uint8_t *edid_data = NULL; > > + uint8_t test_result = DP_TEST_NAK, checksum = 0; > > + uint32_t ret = 0; > > + > > + intel_dp->aux.i2c_nack_count = 0; > > + intel_dp->aux.i2c_defer_count = 0; > > + > > + edid_read = drm_get_edid(connector, adapter); > > + > > + if (edid_read == NULL) { > > + /* 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, > > + intel_dp->aux.i2c_defer_count); > > Don't we need to use these _count values somehow, instead of just > printing them in the logs? > > Everything else looks fine. > > > + intel_dp->compliance_test_data = INTEL_DP_EDID_CORRUPT | > > + INTEL_DP_RESOLUTION_FAILSAFE; > > + } else { > > + edid_data = (uint8_t *) edid_read; > > + > > + if (intel_dp_compute_edid_checksum(edid_data, &checksum)) { > > + ret = drm_dp_dpcd_write(&intel_dp->aux, > > + DP_TEST_EDID_CHECKSUM, > > + &edid_read->checksum, 1); > > + test_result = DP_TEST_ACK | > > + DP_TEST_EDID_CHECKSUM_WRITE; > > + intel_dp->compliance_test_data = > > + INTEL_DP_EDID_OK | > > + INTEL_DP_RESOLUTION_PREFERRED; > > + } else { > > + /* Invalid checksum - EDID corruption detection */ > > + intel_dp->compliance_test_data = > > + INTEL_DP_EDID_CORRUPT | > > + INTEL_DP_RESOLUTION_FAILSAFE; Just something random I've spotted while driving by: drm_get_edid does all the checksum stuff for you already (it retries up to 4 times if the checkusm is off and also checks a few other things). We should never reach this case and the checksum function is essentially dead code. Or do I miss something? -Daniel > > + } > > + } > > + > > return test_result; > > } > > > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 12/17/14 9:25 AM, Paulo Zanoni wrote: > 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>: >> Updates the EDID compliance test function to perform the EDID read as >> required by the tests. This read needs to take place in the kernel for >> reasons of speed and efficiency. The results of the EDID read are handed >> off to userspace so that the remainder of the test can be conducted there. >> >> 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 >> >> Signed-off-by: Todd Previte<tprevite@gmail.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 71 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index b6f5a72..2a13124 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -40,6 +40,13 @@ >> >> #define DP_LINK_CHECK_TIMEOUT (10 * 1000) >> >> +/* Compliance test status bits */ >> +#define INTEL_DP_EDID_OK (0<<0) >> +#define INTEL_DP_EDID_CORRUPT (1<<0) >> +#define INTEL_DP_RESOLUTION_PREFERRED (1<<2) >> +#define INTEL_DP_RESOLUTION_STANDARD (1<<3) >> +#define INTEL_DP_RESOLUTION_FAILSAFE (1<<4) >> + >> struct dp_link_dpll { >> int link_bw; >> struct dpll dpll; >> @@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) >> return test_result; >> } >> >> +static bool intel_dp_compute_edid_checksum(uint8_t *edid_data, >> + uint8_t *edid_checksum) >> +{ >> + uint32_t byte_total = 0; >> + uint8_t i = 0; >> + bool edid_ok = true; >> + >> + /* Don't include last byte (the checksum) in the computation */ >> + for (i = 0; i < EDID_LENGTH - 2; i++) > Shouldn't this be "i < EDID_LENGHT - 1"? Nope. :) EDID_LENGTH is defined as 128, so 128 would be off the end of the array. EDID_LENGTH - 1 would be the checksum itself, which I don't want to include when computing the checksum. So the last valid byte is actually byte 126, which is EDID_LENGTH - 2. >> + byte_total += edid_data[i]; >> + >> + *edid_checksum = 256 - (byte_total % 256); >> + >> + if (*edid_checksum != edid_data[EDID_LENGTH - 1]) { >> + DRM_DEBUG_KMS("Invalid EDID checksum %02x, should be %02x\n", >> + edid_data[EDID_LENGTH - 40 - 1], *edid_checksum); >> + edid_ok = false; >> + } >> + >> + return edid_ok; >> +} >> + >> static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp) >> { >> - uint8_t test_result = DP_TEST_NAK; >> + struct drm_connector *connector = &intel_dp->attached_connector->base; >> + struct i2c_adapter *adapter = &intel_dp->aux.ddc; >> + struct edid *edid_read = NULL; >> + uint8_t *edid_data = NULL; >> + uint8_t test_result = DP_TEST_NAK, checksum = 0; >> + uint32_t ret = 0; >> + >> + intel_dp->aux.i2c_nack_count = 0; >> + intel_dp->aux.i2c_defer_count = 0; >> + >> + edid_read = drm_get_edid(connector, adapter); >> + >> + if (edid_read == NULL) { >> + /* 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, >> + intel_dp->aux.i2c_defer_count); > Don't we need to use these _count values somehow, instead of just > printing them in the logs? > > Everything else looks fine. In this instance, no. All that is required is for the source device to appropriately respond to receiving an I2C DEFER or NACK from the sink device. If those counters are above 0 for purposes of compliance testing, that triggers the corrupt/invalid EDID response from the test mechanism. In reality, those counters could be tracked for a variety of reasons; one example is that they could used to slow down I2C transactions for sink devices with high response latencies, so that the DEFERs or NACKs could be reduced or avoided entirely. >> + intel_dp->compliance_test_data = INTEL_DP_EDID_CORRUPT | >> + INTEL_DP_RESOLUTION_FAILSAFE; >> + } else { >> + edid_data = (uint8_t *) edid_read; >> + >> + if (intel_dp_compute_edid_checksum(edid_data, &checksum)) { >> + ret = drm_dp_dpcd_write(&intel_dp->aux, >> + DP_TEST_EDID_CHECKSUM, >> + &edid_read->checksum, 1); >> + test_result = DP_TEST_ACK | >> + DP_TEST_EDID_CHECKSUM_WRITE; >> + intel_dp->compliance_test_data = >> + INTEL_DP_EDID_OK | >> + INTEL_DP_RESOLUTION_PREFERRED; >> + } else { >> + /* Invalid checksum - EDID corruption detection */ >> + intel_dp->compliance_test_data = >> + INTEL_DP_EDID_CORRUPT | >> + INTEL_DP_RESOLUTION_FAILSAFE; >> + } >> + } >> + >> return test_result; >> } >> >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
Readding intel-gfx. On Wed, Feb 18, 2015 at 5:57 PM, Todd Previte <tprevite@gmail.com> wrote: > On 12/17/14 1:20 PM, Daniel Vetter wrote: >> Just something random I've spotted while driving by: drm_get_edid does all >> the checksum stuff for you already (it retries up to 4 times if the >> checkusm is off and also checks a few other things). We should never reach >> this case and the checksum function is essentially dead code. >> >> Or do I miss something? >> -Daniel >> > > The data I have is that when connected to the DPR-100, the logs show that > the EDID checksum is incorrect from the DRM routines, which is why I went > and wrote this one. This is most likely a problem with the DPR-100 (that > they won't fix since they're not updating it anymore) and possibly still a > problem with the DPR-120 (which I can't test because I don't have one). So > this code is necessary in order to pass the EDID tests when using the > DPR-100. > > That said, it's also unlikely that the DRM routines are wholly wrong, since > that would result in many, many errors in the logs for all the shipping DP > monitors out there. So this is one case where special code is necessary in > order to pass the tests with a particular test device. If that's the case then we need to add some quirks to the core edid code (we have plenty of those already, shipping hw tends to be broken in all sorts of interesting ways). At least I don't see a point in passing compliance in such an important aspect as validating sink behaviour by adding special-purpose code instead of the code we use for real hw. -Daniel
On Wed, Dec 10, 2014 at 04:53:11PM -0700, Todd Previte wrote: > Updates the EDID compliance test function to perform the EDID read as > required by the tests. This read needs to take place in the kernel for > reasons of speed and efficiency. The results of the EDID read are handed > off to userspace so that the remainder of the test can be conducted there. > > 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 > > Signed-off-by: Todd Previte <tprevite@gmail.com> Returning the abstract discussion about where to put the edid checksum checks back to the concrete patch at hand. > --- > drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 71 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index b6f5a72..2a13124 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -40,6 +40,13 @@ > > #define DP_LINK_CHECK_TIMEOUT (10 * 1000) > > +/* Compliance test status bits */ > +#define INTEL_DP_EDID_OK (0<<0) > +#define INTEL_DP_EDID_CORRUPT (1<<0) > +#define INTEL_DP_RESOLUTION_PREFERRED (1<<2) > +#define INTEL_DP_RESOLUTION_STANDARD (1<<3) > +#define INTEL_DP_RESOLUTION_FAILSAFE (1<<4) > + > struct dp_link_dpll { > int link_bw; > struct dpll dpll; > @@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) > return test_result; > } > > +static bool intel_dp_compute_edid_checksum(uint8_t *edid_data, > + uint8_t *edid_checksum) > +{ > + uint32_t byte_total = 0; > + uint8_t i = 0; > + bool edid_ok = true; > + > + /* Don't include last byte (the checksum) in the computation */ > + for (i = 0; i < EDID_LENGTH - 2; i++) > + byte_total += edid_data[i]; > + > + *edid_checksum = 256 - (byte_total % 256); > + > + if (*edid_checksum != edid_data[EDID_LENGTH - 1]) { > + DRM_DEBUG_KMS("Invalid EDID checksum %02x, should be %02x\n", > + edid_data[EDID_LENGTH - 40 - 1], *edid_checksum); > + edid_ok = false; > + } > + > + return edid_ok; > +} > + > static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp) > { > - uint8_t test_result = DP_TEST_NAK; > + struct drm_connector *connector = &intel_dp->attached_connector->base; > + struct i2c_adapter *adapter = &intel_dp->aux.ddc; > + struct edid *edid_read = NULL; > + uint8_t *edid_data = NULL; > + uint8_t test_result = DP_TEST_NAK, checksum = 0; > + uint32_t ret = 0; > + > + intel_dp->aux.i2c_nack_count = 0; > + intel_dp->aux.i2c_defer_count = 0; > + > + edid_read = drm_get_edid(connector, adapter); > + > + if (edid_read == NULL) { So if the edid core thinks your edid is foul (e.g. checksum mismatch) you wont ever see the edid and land in this case. > + /* 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, > + intel_dp->aux.i2c_defer_count); > + intel_dp->compliance_test_data = INTEL_DP_EDID_CORRUPT | > + INTEL_DP_RESOLUTION_FAILSAFE; And return the above error condition of "corrupt edid" to the tester. > + } else { > + edid_data = (uint8_t *) edid_read; > + > + if (intel_dp_compute_edid_checksum(edid_data, &checksum)) { > + ret = drm_dp_dpcd_write(&intel_dp->aux, > + DP_TEST_EDID_CHECKSUM, > + &edid_read->checksum, 1); > + test_result = DP_TEST_ACK | > + DP_TEST_EDID_CHECKSUM_WRITE; > + intel_dp->compliance_test_data = > + INTEL_DP_EDID_OK | > + INTEL_DP_RESOLUTION_PREFERRED; > + } else { Which means except when there is some difference between your checksum code and the drm_edid.c checksum code this case here is dead code. And if there _is_ a difference then they'll never agree, so the "edid ok" case above can't happen. Either way one of the two sides of this if is dead code (at least if I haven't missed something). That's essentially the reason why I think we should fixup the checksum code in drm_edid.c and drop this. -Daniel > + /* Invalid checksum - EDID corruption detection */ > + intel_dp->compliance_test_data = > + INTEL_DP_EDID_CORRUPT | > + INTEL_DP_RESOLUTION_FAILSAFE; > + } > + } > + > return test_result; > } > > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index b6f5a72..2a13124 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -40,6 +40,13 @@ #define DP_LINK_CHECK_TIMEOUT (10 * 1000) +/* Compliance test status bits */ +#define INTEL_DP_EDID_OK (0<<0) +#define INTEL_DP_EDID_CORRUPT (1<<0) +#define INTEL_DP_RESOLUTION_PREFERRED (1<<2) +#define INTEL_DP_RESOLUTION_STANDARD (1<<3) +#define INTEL_DP_RESOLUTION_FAILSAFE (1<<4) + struct dp_link_dpll { int link_bw; struct dpll dpll; @@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) return test_result; } +static bool intel_dp_compute_edid_checksum(uint8_t *edid_data, + uint8_t *edid_checksum) +{ + uint32_t byte_total = 0; + uint8_t i = 0; + bool edid_ok = true; + + /* Don't include last byte (the checksum) in the computation */ + for (i = 0; i < EDID_LENGTH - 2; i++) + byte_total += edid_data[i]; + + *edid_checksum = 256 - (byte_total % 256); + + if (*edid_checksum != edid_data[EDID_LENGTH - 1]) { + DRM_DEBUG_KMS("Invalid EDID checksum %02x, should be %02x\n", + edid_data[EDID_LENGTH - 40 - 1], *edid_checksum); + edid_ok = false; + } + + return edid_ok; +} + static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp) { - uint8_t test_result = DP_TEST_NAK; + struct drm_connector *connector = &intel_dp->attached_connector->base; + struct i2c_adapter *adapter = &intel_dp->aux.ddc; + struct edid *edid_read = NULL; + uint8_t *edid_data = NULL; + uint8_t test_result = DP_TEST_NAK, checksum = 0; + uint32_t ret = 0; + + intel_dp->aux.i2c_nack_count = 0; + intel_dp->aux.i2c_defer_count = 0; + + edid_read = drm_get_edid(connector, adapter); + + if (edid_read == NULL) { + /* 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, + intel_dp->aux.i2c_defer_count); + intel_dp->compliance_test_data = INTEL_DP_EDID_CORRUPT | + INTEL_DP_RESOLUTION_FAILSAFE; + } else { + edid_data = (uint8_t *) edid_read; + + if (intel_dp_compute_edid_checksum(edid_data, &checksum)) { + ret = drm_dp_dpcd_write(&intel_dp->aux, + DP_TEST_EDID_CHECKSUM, + &edid_read->checksum, 1); + test_result = DP_TEST_ACK | + DP_TEST_EDID_CHECKSUM_WRITE; + intel_dp->compliance_test_data = + INTEL_DP_EDID_OK | + INTEL_DP_RESOLUTION_PREFERRED; + } else { + /* Invalid checksum - EDID corruption detection */ + intel_dp->compliance_test_data = + INTEL_DP_EDID_CORRUPT | + INTEL_DP_RESOLUTION_FAILSAFE; + } + } + return test_result; }
Updates the EDID compliance test function to perform the EDID read as required by the tests. This read needs to take place in the kernel for reasons of speed and efficiency. The results of the EDID read are handed off to userspace so that the remainder of the test can be conducted there. 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 Signed-off-by: Todd Previte <tprevite@gmail.com> --- drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-)