| Message ID | 1461979695-27113-4-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
On Monday 23 May 2016 01:48 PM, 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? > >> 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 For tests: * 4.2.2.4 : Failed EDID read, I2C_NAK * 4.2.2.5 : Failed EDID read, I2C_DEFER * 4.2.2.6 : EDID corruption detected when edid is detected to be corrupt, we need to fall back to failsafe mode. This mode requires change for bpc and connected status which is being performed in the patch: https://patchwork.freedesktop.org/patch/82566/ "drm/i915: Use fail safe mode when edid is corrupt" Shubhangi >> return connector_status_connected; >> else >> return connector_status_disconnected;
On Monday 23 May 2016 01:48 PM, 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? > >> 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 For tests: * 4.2.2.4 : Failed EDID read, I2C_NAK * 4.2.2.5 : Failed EDID read, I2C_DEFER * 4.2.2.6 : EDID corruption detected when edid is detected to be corrupt, we need to fall back to failsafe mode. This requires change for bpc and status to be connected which is being performed in the patch: https://patchwork.freedesktop.org/patch/82566/ "drm/i915: Use fail safe mode when edid is corrupt" Shubhangi >> return connector_status_connected; >> else >> return connector_status_disconnected;
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. > > > > 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;
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;
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; } /* 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) return connector_status_connected; else return connector_status_disconnected;
This patch addresses a few issues from the original patch for DP Compliance EDID test support submitted by Todd Previte<todd.previte@gmail.com> 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. 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(-)