Re: [PATCH 3/5] drm/i915: Fixes to support the DP Compliance EDID tests.

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote:
> This patch addresses a few issues from the original patch for
> DP Compliance EDID test support submitted by
> Todd Previte<todd.previte@gmail.com>

Do you mean commit 559be30cb74d ("drm/i915: Implement the intel_dp_autotest_edid
function for DP EDID complaince tests")? Please see the link below on how to
refer to other commits in the commit message and how to add a Fixes: tag.

https://www.kernel.org/doc/Documentation/SubmittingPatches

> 
> Video Mode requested in the EDID test handler for the EDID Read
> test (CTS 4.2.2.3) should be set to PREFERRED as per the CTS spec.
> Intel connector status should be connected even if detect_edid is
> NULL when compliance_test flag is set. This is required to handle
> the corrupt EDID (CTS 4.2.2.6) or EDID Read Failure I2C NACK/I2C
> DEFER (CTS 4.2.2.4 and 4.2.2.5) tests from CTS spec.

What exactly do those tests test? It sounds like this patch adds a separate code
path to implement the right behavior only when running the CTS. Shouldn't the
driver handle those failures during normal operation in the same way?

> 
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 0961f22..456fc17 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4023,7 +4023,7 @@ 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;
> +	uint8_t test_result = DP_TEST_ACK;
>  	struct intel_connector *intel_connector = intel_dp-
> >attached_connector;
>  	struct drm_connector *connector = &intel_connector->base;
>  
> @@ -4058,7 +4058,7 @@ static uint8_t intel_dp_autotest_edid(struct intel_dp
> *intel_dp)
>  			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;
> +		intel_dp->compliance_test_data =
> INTEL_DP_RESOLUTION_PREFERRED;

Is this used for anything else than logging?

>  	}
>  
>  	/* Set test active flag here so userspace doesn't interrupt things */
> @@ -4650,7 +4650,7 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  
>  	intel_dp->detect_done = false;
>  
> -	if (intel_connector->detect_edid)
> +	if (intel_connector->detect_edid || intel_dp->compliance_test_active)

Should this check connector->edid_corrupt instead? I guess that would require
some logic to fallback to fail safe mode and bpc too.

I think Shubhangi had a patch for this same problem, but it also seems to create
a separate path for compliance.

Ander
>  		return connector_status_connected;
>  	else
>  		return connector_status_disconnected;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx