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

Ander Conselvan De Oliveira
2016-05-23T08:18:20Z

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;