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

2016-05-26 Thread Ander Conselvan De Oliveira 
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
> > 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 
> > > ---
> > >  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 = _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;
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2016-05-25 Thread Manasi Navare 
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
> 
> 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 
> > ---
> >  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 = _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;
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2016-05-24 Thread Shubhangi Shrivastava 



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

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
---
  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 = _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;


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2016-05-24 Thread Shubhangi Shrivastava 



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

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 
---
  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 = _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;


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2016-05-23 Thread Ander Conselvan De Oliveira 
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

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 
> ---
>  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 = _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

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

2016-04-29 Thread Manasi Navare 
This patch addresses a few issues from the original patch for
DP Compliance EDID test support submitted by
Todd Previte

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 
---
 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 = _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;
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx