[Intel-gfx] [PATCH 11/17] drm/i915: Update the EDID automated compliance test function

[Daniel Vetter]

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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch