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

Ander Conselvan De Oliveira
2016-05-26T08:57:07Z

On Wed, 2016-05-25 at 17:22 -0700, Manasi Navare wrote:
> On Mon, May 23, 2016 at 11:18:20AM +0300, Ander Conselvan De Oliveira wrote:
> >
> > 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?
> >
> These tests see if the system behaves as expected in case of currupt EDID
> or I2C NACK or I2C DEFER and validates if in all these cases it displays
> the failsafe mode. This test gets triggered on a long pulse sent by DPR 120.

So my question is, in those failure scenarios when not in the middle of
compliance testing, does the system behaves as expected?

Ander

>
>
> >
> > >
> > >
> > > 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?
> >
> This is used to tell the userspace app to display the Preferred mode
> or failsafe mode for that test scenario.
> This compliance test data gets read in userspace IGT App.
>
>
> >
> > >
> > >   }
> > >
> > >   /* 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
> This check only makes sure that if the compliance test is in progress then
> that means
> it is testing for cases like corrupt edid and NACK/I2C defer and hence its a
> fake or
> purposely created corrupt EDID or I2C failure scenario so report the connector
> as connected. Otherwise, it reports it out as disconnected and treats this
> test scenario
> as a real failure and the test does not complete.
>
> Manasi
>
> >
> > >
> > >   return connector_status_connected;
> > >   else
> > >   return connector_status_disconnected;