[PATCH 0/5] Add automation support for DP compliance Tests

[PATCH 0/5] Add automation support for DP compliance Tests

[Manasi Navare]

This patch series adds the automation support for DP Compliance Tests
for EDID and Video Pattern generation from CTS specification 1.2 Rev 1.1.

Jim Bride (1):
  Add support for forcing 6 bpc on DP pipes.

Manasi Navare (4):
  drm/i915: Invoke the DP Compliance test request handler in the short
    pulse path
  drm/i915: Disable the Link training automation support
  drm/i915: Fixes to support the DP Compliance EDID tests.
  drm/i915: Implement intel_dp_autotest_video_pattern function for
    DP Video pattern compliance tests

 drivers/gpu/drm/i915/i915_debugfs.c  | 14 +++++-
 drivers/gpu/drm/i915/intel_display.c | 32 +++++++++++++-
 drivers/gpu/drm/i915/intel_dp.c      | 86 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h     | 10 +++++
 include/drm/drm_dp_helper.h          | 14 +++++-
 5 files changed, 147 insertions(+), 9 deletions(-)

-- 
1.9.1

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

[PATCH 1/5] drm/i915: Invoke the DP Compliance test request handler in the short pulse path

[Manasi Navare]

HPD Short pulse test requests occur for DP Compliance Link Training
and Video Pattern tests.The DP Test request handler needs to be
invoked by these tests in the short pulse path in order to support
automated DP Compliance tests.

Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c12c414..19a95ed 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4257,7 +4257,7 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
 				   sink_irq_vector);
 
 		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
-			DRM_DEBUG_DRIVER("Test request in short pulse not handled\n");
+			intel_dp_handle_test_request(intel_dp);
 		if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
-- 
1.9.1

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

Re: [PATCH 1/5] drm/i915: Invoke the DP Compliance test request handler in the short pulse path

[Thulasimani, Sivakumar]

i am bit partial :) coz i was involved but isn't the same done in patch 
shared
earlier ?
https://patchwork.freedesktop.org/patch/82587/

why not integrate that here if something more is done in the following 
patches ?

regards,
Sivakumar

On 4/30/2016 6:58 AM, Manasi Navare wrote:
> HPD Short pulse test requests occur for DP Compliance Link Training
> and Video Pattern tests.The DP Test request handler needs to be
> invoked by these tests in the short pulse path in order to support
> automated DP Compliance tests.
>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c12c414..19a95ed 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4257,7 +4257,7 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
>   				   sink_irq_vector);
>   
>   		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
> -			DRM_DEBUG_DRIVER("Test request in short pulse not handled\n");
> +			intel_dp_handle_test_request(intel_dp);
>   		if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ))
>   			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>   	}

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

[PATCH 2/5] drm/i915: Disable the Link training automation support

[Manasi Navare]

Kernel does not have automation support for DP compliance Link
training tests. So the Link Training test handler should return
a TEST_NAK.

Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 19a95ed..0961f22 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4011,7 +4011,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 
 static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
 {
-	uint8_t test_result = DP_TEST_ACK;
+	uint8_t test_result = DP_TEST_NAK;
 	return test_result;
 }
 
-- 
1.9.1

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

Re: [PATCH 2/5] drm/i915: Disable the Link training automation support

[Ander Conselvan De Oliveira]

On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote:
> Kernel does not have automation support for DP compliance Link
> training tests. So the Link Training test handler should return
> a TEST_NAK.

Is this test activated by short or long pulse? The commit message of commit
09b1eb130e43 ("drm/i915: Move Displayport test request and sink IRQ logic to
intel_dp_detect()") suggests the latter. In that case, the order of this and
patch 1 should be inverted.

Otherwise, for patches 1 and 2:

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>


> 
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 19a95ed..0961f22 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4011,7 +4011,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8
> *sink_irq_vector)
>  
>  static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>  {
> -	uint8_t test_result = DP_TEST_ACK;
> +	uint8_t test_result = DP_TEST_NAK;
>  	return test_result;
>  }
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Disable the Link training automation support

[Manasi Navare]

On Mon, May 23, 2016 at 11:10:24AM +0300, Ander Conselvan De Oliveira wrote:
> On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote:
> > Kernel does not have automation support for DP compliance Link
> > training tests. So the Link Training test handler should return
> > a TEST_NAK.
> 
> Is this test activated by short or long pulse? The commit message of commit
> 09b1eb130e43 ("drm/i915: Move Displayport test request and sink IRQ logic to
> intel_dp_detect()") suggests the latter. In that case, the order of this and
> patch 1 should be inverted.
> 
> Otherwise, for patches 1 and 2:
> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> 
>

This test (Link Training test) is activated by a short pulse, however
the patch 09b1eb130e43 only supported EDID tests that get activated
by a long pulse. 
> > 
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 19a95ed..0961f22 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4011,7 +4011,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8
> > *sink_irq_vector)
> >  
> >  static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
> >  {
> > -	uint8_t test_result = DP_TEST_ACK;
> > +	uint8_t test_result = DP_TEST_NAK;
> >  	return test_result;
> >  }
> >  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Manasi Navare]

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(-)

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

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

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

[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<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

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

[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<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;

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

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

[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<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;

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

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

[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<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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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<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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] Add support for forcing 6 bpc on DP pipes.

[Manasi Navare]

From: Jim Bride <jim.bride@linux.intel.com>

For DP compliance we need to be able to control the output color
type for the pipe associated with the DP port. To do this we rely
on the intel_dp_test_force_bpc debugfs file and the associated value
stored in struct intel_dp. If the debugfs file has a non-zero value
and we're on a display port connector, then we use the value from
debugfs to calculate the bpp for the pipe.  For cases where we are
not on DP or there has not been an overridden value then we behave
as normal.

Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5ffccf6..1618d36 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12102,11 +12102,39 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
 
 	/* Clamp display bpp to EDID value */
 	for_each_connector_in_state(state, connector, connector_state, i) {
+		int type = 0;
+
+		if (connector_state->best_encoder) {
+			struct intel_encoder *ienc;
+
+			ienc = to_intel_encoder(connector_state->best_encoder);
+			type = ienc->type;
+		}
+
 		if (connector_state->crtc != &crtc->base)
 			continue;
 
-		connected_sink_compute_bpp(to_intel_connector(connector),
-					   pipe_config);
+		/* For DP compliance we need to ensure that we can override
+		 * the computed bpp for the pipe.
+		 */
+		if (type == INTEL_OUTPUT_DISPLAYPORT) {
+			struct intel_dp *intel_dp =
+				enc_to_intel_dp(connector_state->best_encoder);
+
+			if (intel_dp &&
+			    (intel_dp->compliance_force_bpc != 0)) {
+				pipe_config->pipe_bpp =
+					intel_dp->compliance_force_bpc*3;
+				DRM_DEBUG_KMS("JMB Setting pipe_bpp to %d\n",
+					      pipe_config->pipe_bpp);
+			} else {
+				connected_sink_compute_bpp(to_intel_connector(connector),
+						   pipe_config);
+			}
+		} else {
+			connected_sink_compute_bpp(to_intel_connector(connector),
+						   pipe_config);
+		}
 	}
 
 	return bpp;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e23eed7..10eaff8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -865,6 +865,7 @@ struct intel_dp {
 	unsigned long compliance_test_type;
 	unsigned long compliance_test_data;
 	bool compliance_test_active;
+	unsigned long compliance_force_bpc; /* 0 for default or bpc to use */
 };
 
 struct intel_digital_port {
-- 
1.9.1

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

Re: [PATCH 4/5] Add support for forcing 6 bpc on DP pipes.

[Jim Bride]

On Fri, Apr 29, 2016 at 06:28:14PM -0700, Manasi Navare wrote:
> From: Jim Bride <jim.bride@linux.intel.com>
> 
> For DP compliance we need to be able to control the output color
> type for the pipe associated with the DP port. To do this we rely
> on the intel_dp_test_force_bpc debugfs file and the associated value
> stored in struct intel_dp. If the debugfs file has a non-zero value
> and we're on a display port connector, then we use the value from
> debugfs to calculate the bpp for the pipe.  For cases where we are
> not on DP or there has not been an overridden value then we behave
> as normal.
> 
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  2 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5ffccf6..1618d36 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12102,11 +12102,39 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
>  
>  	/* Clamp display bpp to EDID value */
>  	for_each_connector_in_state(state, connector, connector_state, i) {
> +		int type = 0;
> +
> +		if (connector_state->best_encoder) {
> +			struct intel_encoder *ienc;
> +
> +			ienc = to_intel_encoder(connector_state->best_encoder);
> +			type = ienc->type;
> +		}
> +
>  		if (connector_state->crtc != &crtc->base)
>  			continue;
>  
> -		connected_sink_compute_bpp(to_intel_connector(connector),
> -					   pipe_config);
> +		/* For DP compliance we need to ensure that we can override
> +		 * the computed bpp for the pipe.
> +		 */
> +		if (type == INTEL_OUTPUT_DISPLAYPORT) {
> +			struct intel_dp *intel_dp =
> +				enc_to_intel_dp(connector_state->best_encoder);
> +
> +			if (intel_dp &&
> +			    (intel_dp->compliance_force_bpc != 0)) {
> +				pipe_config->pipe_bpp =
> +					intel_dp->compliance_force_bpc*3;
> +				DRM_DEBUG_KMS("JMB Setting pipe_bpp to %d\n",
> +					      pipe_config->pipe_bpp);

I missed this before, but you should probably take the 'JMB' out of the debug
message above.

Jim

> +			} else {
> +				connected_sink_compute_bpp(to_intel_connector(connector),
> +						   pipe_config);
> +			}
> +		} else {
> +			connected_sink_compute_bpp(to_intel_connector(connector),
> +						   pipe_config);
> +		}
>  	}
>  
>  	return bpp;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e23eed7..10eaff8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -865,6 +865,7 @@ struct intel_dp {
>  	unsigned long compliance_test_type;
>  	unsigned long compliance_test_data;
>  	bool compliance_test_active;
> +	unsigned long compliance_force_bpc; /* 0 for default or bpc to use */
>  };
>  
>  struct intel_digital_port {
> -- 
> 1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] Add support for forcing 6 bpc on DP pipes.

[Ander Conselvan De Oliveira]

On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote:
> From: Jim Bride <jim.bride@linux.intel.com>
> 
> For DP compliance we need to be able to control the output color
> type for the pipe associated with the DP port. To do this we rely
> on the intel_dp_test_force_bpc debugfs file and the associated value
> stored in struct intel_dp. If the debugfs file has a non-zero value
> and we're on a display port connector, then we use the value from
> debugfs to calculate the bpp for the pipe.  For cases where we are
> not on DP or there has not been an overridden value then we behave
> as normal.
> 
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  2 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 5ffccf6..1618d36 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12102,11 +12102,39 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
>  
>  	/* Clamp display bpp to EDID value */
>  	for_each_connector_in_state(state, connector, connector_state, i) {
> +		int type = 0;
> +
> +		if (connector_state->best_encoder) {
> +			struct intel_encoder *ienc;
> +
> +			ienc = to_intel_encoder(connector_state-
> >best_encoder);
> +			type = ienc->type;
> +		}
> +
>  		if (connector_state->crtc != &crtc->base)
>  			continue;
>  
> -		connected_sink_compute_bpp(to_intel_connector(connector),
> -					   pipe_config);
> +		/* For DP compliance we need to ensure that we can override
> +		 * the computed bpp for the pipe.
> +		 */
> +		if (type == INTEL_OUTPUT_DISPLAYPORT) {
> +			struct intel_dp *intel_dp =
> +				enc_to_intel_dp(connector_state-
> >best_encoder);
> +
> +			if (intel_dp &&
> +			    (intel_dp->compliance_force_bpc != 0)) {
> +				pipe_config->pipe_bpp =
> +					intel_dp->compliance_force_bpc*3;
> +				DRM_DEBUG_KMS("JMB Setting pipe_bpp to %d\n",
> +					      pipe_config->pipe_bpp);
> +			} else {
> +				connected_sink_compute_bpp(to_intel_connector
> (connector),
> +						   pipe_config);

This kind of thing should be done in the encoder compute_config hook.

> +			}
> +		} else {
> +			connected_sink_compute_bpp(to_intel_connector(connect
> or),
> +						   pipe_config);
> +		}
>  	}
>  
>  	return bpp;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index e23eed7..10eaff8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -865,6 +865,7 @@ struct intel_dp {
>  	unsigned long compliance_test_type;
>  	unsigned long compliance_test_data;
>  	bool compliance_test_active;
> +	unsigned long compliance_force_bpc; /* 0 for default or bpc to use */

There's no code for setting compliance_test_active anywhere. The commit message
mentions debugfs, but I didn't see anything related in the patch. 


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

Re: [PATCH 4/5] Add support for forcing 6 bpc on DP pipes.

[Jim Bride]

On Mon, May 23, 2016 at 11:22:17AM +0300, Ander Conselvan De Oliveira wrote:
> On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote:
> > From: Jim Bride <jim.bride@linux.intel.com>
> > 
> > For DP compliance we need to be able to control the output color
> > type for the pipe associated with the DP port. To do this we rely
> > on the intel_dp_test_force_bpc debugfs file and the associated value
> > stored in struct intel_dp. If the debugfs file has a non-zero value
> > and we're on a display port connector, then we use the value from
> > debugfs to calculate the bpp for the pipe.  For cases where we are
> > not on DP or there has not been an overridden value then we behave
> > as normal.
> > 
> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++++++++--
> >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >  2 files changed, 31 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 5ffccf6..1618d36 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12102,11 +12102,39 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
> >  
> >  	/* Clamp display bpp to EDID value */
> >  	for_each_connector_in_state(state, connector, connector_state, i) {
> > +		int type = 0;
> > +
> > +		if (connector_state->best_encoder) {
> > +			struct intel_encoder *ienc;
> > +
> > +			ienc = to_intel_encoder(connector_state-
> > >best_encoder);
> > +			type = ienc->type;
> > +		}
> > +
> >  		if (connector_state->crtc != &crtc->base)
> >  			continue;
> >  
> > -		connected_sink_compute_bpp(to_intel_connector(connector),
> > -					   pipe_config);
> > +		/* For DP compliance we need to ensure that we can override
> > +		 * the computed bpp for the pipe.
> > +		 */
> > +		if (type == INTEL_OUTPUT_DISPLAYPORT) {
> > +			struct intel_dp *intel_dp =
> > +				enc_to_intel_dp(connector_state-
> > >best_encoder);
> > +
> > +			if (intel_dp &&
> > +			    (intel_dp->compliance_force_bpc != 0)) {
> > +				pipe_config->pipe_bpp =
> > +					intel_dp->compliance_force_bpc*3;
> > +				DRM_DEBUG_KMS("JMB Setting pipe_bpp to %d\n",
> > +					      pipe_config->pipe_bpp);
> > +			} else {
> > +				connected_sink_compute_bpp(to_intel_connector
> > (connector),
> > +						   pipe_config);
> 
> This kind of thing should be done in the encoder compute_config hook.

Even though it's really not specific to an individual encoder configuration?
This seems to be the central place where we're ensuring that we have a sane
value for bpp relative to the display, and thus a good place to set this
override to make compliance happy (which needs a specific bpc value for some
test cases rather than one that is deemed sane relative to the sink's
capabilities.

> 
> > +			}
> > +		} else {
> > +			connected_sink_compute_bpp(to_intel_connector(connect
> > or),
> > +						   pipe_config);
> > +		}
> >  	}
> >  
> >  	return bpp;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index e23eed7..10eaff8 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -865,6 +865,7 @@ struct intel_dp {
> >  	unsigned long compliance_test_type;
> >  	unsigned long compliance_test_data;
> >  	bool compliance_test_active;
> > +	unsigned long compliance_force_bpc; /* 0 for default or bpc to use */
> 
> There's no code for setting compliance_test_active anywhere. The commit message
> mentions debugfs, but I didn't see anything related in the patch. 

It appears that Manasi forgot to include one of the patches I had sent her.
The debugfs support code was in a separate patch.

Jim

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

Re: [PATCH 4/5] Add support for forcing 6 bpc on DP pipes.

[Ander Conselvan De Oliveira]

On Mon, 2016-05-23 at 10:42 -0700, Jim Bride wrote:
> On Mon, May 23, 2016 at 11:22:17AM +0300, Ander Conselvan De Oliveira wrote:
> > 
> > On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote:
> > > 
> > > From: Jim Bride <jim.bride@linux.intel.com>
> > > 
> > > For DP compliance we need to be able to control the output color
> > > type for the pipe associated with the DP port. To do this we rely
> > > on the intel_dp_test_force_bpc debugfs file and the associated value
> > > stored in struct intel_dp. If the debugfs file has a non-zero value
> > > and we're on a display port connector, then we use the value from
> > > debugfs to calculate the bpp for the pipe.  For cases where we are
> > > not on DP or there has not been an overridden value then we behave
> > > as normal.
> > > 
> > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++++++++-
> > > -
> > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> > >  2 files changed, 31 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 5ffccf6..1618d36 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -12102,11 +12102,39 @@ compute_baseline_pipe_bpp(struct intel_crtc
> > > *crtc,
> > >  
> > >  	/* Clamp display bpp to EDID value */
> > >  	for_each_connector_in_state(state, connector, connector_state, i)
> > > {
> > > +		int type = 0;
> > > +
> > > +		if (connector_state->best_encoder) {
> > > +			struct intel_encoder *ienc;
> > > +
> > > +			ienc = to_intel_encoder(connector_state-
> > > > 
> > > > best_encoder);
> > > +			type = ienc->type;
> > > +		}
> > > +
> > >  		if (connector_state->crtc != &crtc->base)
> > >  			continue;
> > >  
> > > -		connected_sink_compute_bpp(to_intel_connector(connector),
> > > -					   pipe_config);
> > > +		/* For DP compliance we need to ensure that we can
> > > override
> > > +		 * the computed bpp for the pipe.
> > > +		 */
> > > +		if (type == INTEL_OUTPUT_DISPLAYPORT) {
> > > +			struct intel_dp *intel_dp =
> > > +				enc_to_intel_dp(connector_state-
> > > > 
> > > > best_encoder);
> > > +
> > > +			if (intel_dp &&
> > > +			    (intel_dp->compliance_force_bpc != 0)) {
> > > +				pipe_config->pipe_bpp =
> > > +					intel_dp->compliance_force_bpc*3;
> > > +				DRM_DEBUG_KMS("JMB Setting pipe_bpp to
> > > %d\n",
> > > +					      pipe_config->pipe_bpp);
> > > +			} else {
> > > +				connected_sink_compute_bpp(to_intel_conne
> > > ctor
> > > (connector),
> > > +						   pipe_config);
> > This kind of thing should be done in the encoder compute_config hook.
> Even though it's really not specific to an individual encoder configuration?
> This seems to be the central place where we're ensuring that we have a sane
> value for bpp relative to the display, and thus a good place to set this
> override to make compliance happy (which needs a specific bpc value for some
> test cases rather than one that is deemed sane relative to the sink's
> capabilities.

Well, this code path is only reached when the DisplayPort associated with a
given encoder is in the middle of compliance testing. I'd say that's very
encoder specific.

The bpp computation happens in two phases. Here a baseline is computed,
considering what is generally supported by the hardware. The encoders are
allowed to override that value. You can look at HDMI for an example: it may
require a bpp override since HDMI doesn't supports 10 bpc, only 8 or 12. You can
 find similar code also in LVDS and even DP.

Unfortunately, there is very little documentation of what the hooks are supposed
to do. But for the question at hand, yes, it should really be in
intel_dp_compute_config().

Ander

> 
> > 
> > 
> > > 
> > > +			}
> > > +		} else {
> > > +			connected_sink_compute_bpp(to_intel_connector(con
> > > nect
> > > or),
> > > +						   pipe_config);
> > > +		}
> > >  	}
> > >  
> > >  	return bpp;
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index e23eed7..10eaff8 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -865,6 +865,7 @@ struct intel_dp {
> > >  	unsigned long compliance_test_type;
> > >  	unsigned long compliance_test_data;
> > >  	bool compliance_test_active;
> > > +	unsigned long compliance_force_bpc; /* 0 for default or bpc to
> > > use */
> > There's no code for setting compliance_test_active anywhere. The commit
> > message
> > mentions debugfs, but I didn't see anything related in the patch. 
> It appears that Manasi forgot to include one of the patches I had sent her.
> The debugfs support code was in a separate patch.
> 
> Jim
> 
> > 
> > 
> > 
> > Ander
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] Add support for forcing 6 bpc on DP pipes.

[Manasi Navare]

On Tue, May 24, 2016 at 08:45:45AM +0300, Ander Conselvan De Oliveira wrote:
> On Mon, 2016-05-23 at 10:42 -0700, Jim Bride wrote:
> > On Mon, May 23, 2016 at 11:22:17AM +0300, Ander Conselvan De Oliveira wrote:
> > > 
> > > On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote:
> > > > 
> > > > From: Jim Bride <jim.bride@linux.intel.com>
> > > > 
> > > > For DP compliance we need to be able to control the output color
> > > > type for the pipe associated with the DP port. To do this we rely
> > > > on the intel_dp_test_force_bpc debugfs file and the associated value
> > > > stored in struct intel_dp. If the debugfs file has a non-zero value
> > > > and we're on a display port connector, then we use the value from
> > > > debugfs to calculate the bpp for the pipe.  For cases where we are
> > > > not on DP or there has not been an overridden value then we behave
> > > > as normal.
> > > > 
> > > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++++++++-
> > > > -
> > > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> > > >  2 files changed, 31 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index 5ffccf6..1618d36 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -12102,11 +12102,39 @@ compute_baseline_pipe_bpp(struct intel_crtc
> > > > *crtc,
> > > >  
> > > >  	/* Clamp display bpp to EDID value */
> > > >  	for_each_connector_in_state(state, connector, connector_state, i)
> > > > {
> > > > +		int type = 0;
> > > > +
> > > > +		if (connector_state->best_encoder) {
> > > > +			struct intel_encoder *ienc;
> > > > +
> > > > +			ienc = to_intel_encoder(connector_state-
> > > > > 
> > > > > best_encoder);
> > > > +			type = ienc->type;
> > > > +		}
> > > > +
> > > >  		if (connector_state->crtc != &crtc->base)
> > > >  			continue;
> > > >  
> > > > -		connected_sink_compute_bpp(to_intel_connector(connector),
> > > > -					   pipe_config);
> > > > +		/* For DP compliance we need to ensure that we can
> > > > override
> > > > +		 * the computed bpp for the pipe.
> > > > +		 */
> > > > +		if (type == INTEL_OUTPUT_DISPLAYPORT) {
> > > > +			struct intel_dp *intel_dp =
> > > > +				enc_to_intel_dp(connector_state-
> > > > > 
> > > > > best_encoder);
> > > > +
> > > > +			if (intel_dp &&
> > > > +			    (intel_dp->compliance_force_bpc != 0)) {
> > > > +				pipe_config->pipe_bpp =
> > > > +					intel_dp->compliance_force_bpc*3;
> > > > +				DRM_DEBUG_KMS("JMB Setting pipe_bpp to
> > > > %d\n",
> > > > +					      pipe_config->pipe_bpp);
> > > > +			} else {
> > > > +				connected_sink_compute_bpp(to_intel_conne
> > > > ctor
> > > > (connector),
> > > > +						   pipe_config);
> > > This kind of thing should be done in the encoder compute_config hook.
> > Even though it's really not specific to an individual encoder configuration?
> > This seems to be the central place where we're ensuring that we have a sane
> > value for bpp relative to the display, and thus a good place to set this
> > override to make compliance happy (which needs a specific bpc value for some
> > test cases rather than one that is deemed sane relative to the sink's
> > capabilities.
> 
> Well, this code path is only reached when the DisplayPort associated with a
> given encoder is in the middle of compliance testing. I'd say that's very
> encoder specific.
> 
> The bpp computation happens in two phases. Here a baseline is computed,
> considering what is generally supported by the hardware. The encoders are
> allowed to override that value. You can look at HDMI for an example: it may
> require a bpp override since HDMI doesn't supports 10 bpc, only 8 or 12. You can
>  find similar code also in LVDS and even DP.
> 
> Unfortunately, there is very little documentation of what the hooks are supposed
> to do. But for the question at hand, yes, it should really be in
> intel_dp_compute_config().
> 
> Ander

Inside of intel_dp_compute_config(), do we need to change the pipe_config->pipe_bpp
to use intel_dp->compliance_force_bpc in case of CTS or should we just modify the local
bpp value to use the force_bpc?

Manasi
> 
> > 
> > > 
> > > 
> > > > 
> > > > +			}
> > > > +		} else {
> > > > +			connected_sink_compute_bpp(to_intel_connector(con
> > > > nect
> > > > or),
> > > > +						   pipe_config);
> > > > +		}
> > > >  	}
> > > >  
> > > >  	return bpp;
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index e23eed7..10eaff8 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -865,6 +865,7 @@ struct intel_dp {
> > > >  	unsigned long compliance_test_type;
> > > >  	unsigned long compliance_test_data;
> > > >  	bool compliance_test_active;
> > > > +	unsigned long compliance_force_bpc; /* 0 for default or bpc to
> > > > use */
> > > There's no code for setting compliance_test_active anywhere. The commit
> > > message
> > > mentions debugfs, but I didn't see anything related in the patch. 
> > It appears that Manasi forgot to include one of the patches I had sent her.
> > The debugfs support code was in a separate patch.
> > 
> > Jim
> > 
> > > 
> > > 
> > > 
> > > Ander
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] Add support for forcing 6 bpc on DP pipes.

[Ander Conselvan De Oliveira]

On Wed, 2016-05-25 at 17:42 -0700, Manasi Navare wrote:
> On Tue, May 24, 2016 at 08:45:45AM +0300, Ander Conselvan De Oliveira wrote:
> > 
> > On Mon, 2016-05-23 at 10:42 -0700, Jim Bride wrote:
> > > 
> > > On Mon, May 23, 2016 at 11:22:17AM +0300, Ander Conselvan De Oliveira
> > > wrote:
> > > > 
> > > > 
> > > > On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote:
> > > > > 
> > > > > 
> > > > > From: Jim Bride <jim.bride@linux.intel.com>
> > > > > 
> > > > > For DP compliance we need to be able to control the output color
> > > > > type for the pipe associated with the DP port. To do this we rely
> > > > > on the intel_dp_test_force_bpc debugfs file and the associated value
> > > > > stored in struct intel_dp. If the debugfs file has a non-zero value
> > > > > and we're on a display port connector, then we use the value from
> > > > > debugfs to calculate the bpp for the pipe.  For cases where we are
> > > > > not on DP or there has not been an overridden value then we behave
> > > > > as normal.
> > > > > 
> > > > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.c | 32
> > > > > ++++++++++++++++++++++++++++++-
> > > > > -
> > > > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> > > > >  2 files changed, 31 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > index 5ffccf6..1618d36 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -12102,11 +12102,39 @@ compute_baseline_pipe_bpp(struct intel_crtc
> > > > > *crtc,
> > > > >  
> > > > >  	/* Clamp display bpp to EDID value */
> > > > >  	for_each_connector_in_state(state, connector,
> > > > > connector_state, i)
> > > > > {
> > > > > +		int type = 0;
> > > > > +
> > > > > +		if (connector_state->best_encoder) {
> > > > > +			struct intel_encoder *ienc;
> > > > > +
> > > > > +			ienc = to_intel_encoder(connector_state-
> > > > > > 
> > > > > > 
> > > > > > best_encoder);
> > > > > +			type = ienc->type;
> > > > > +		}
> > > > > +
> > > > >  		if (connector_state->crtc != &crtc->base)
> > > > >  			continue;
> > > > >  
> > > > > -		connected_sink_compute_bpp(to_intel_connector(connect
> > > > > or),
> > > > > -					   pipe_config);
> > > > > +		/* For DP compliance we need to ensure that we can
> > > > > override
> > > > > +		 * the computed bpp for the pipe.
> > > > > +		 */
> > > > > +		if (type == INTEL_OUTPUT_DISPLAYPORT) {
> > > > > +			struct intel_dp *intel_dp =
> > > > > +				enc_to_intel_dp(connector_state-
> > > > > > 
> > > > > > 
> > > > > > best_encoder);
> > > > > +
> > > > > +			if (intel_dp &&
> > > > > +			    (intel_dp->compliance_force_bpc != 0)) {
> > > > > +				pipe_config->pipe_bpp =
> > > > > +					intel_dp-
> > > > > >compliance_force_bpc*3;
> > > > > +				DRM_DEBUG_KMS("JMB Setting pipe_bpp
> > > > > to
> > > > > %d\n",
> > > > > +					      pipe_config->pipe_bpp);
> > > > > +			} else {
> > > > > +				connected_sink_compute_bpp(to_intel_c
> > > > > onne
> > > > > ctor
> > > > > (connector),
> > > > > +						   pipe_config);
> > > > This kind of thing should be done in the encoder compute_config hook.
> > > Even though it's really not specific to an individual encoder
> > > configuration?
> > > This seems to be the central place where we're ensuring that we have a
> > > sane
> > > value for bpp relative to the display, and thus a good place to set this
> > > override to make compliance happy (which needs a specific bpc value for
> > > some
> > > test cases rather than one that is deemed sane relative to the sink's
> > > capabilities.
> > Well, this code path is only reached when the DisplayPort associated with a
> > given encoder is in the middle of compliance testing. I'd say that's very
> > encoder specific.
> > 
> > The bpp computation happens in two phases. Here a baseline is computed,
> > considering what is generally supported by the hardware. The encoders are
> > allowed to override that value. You can look at HDMI for an example: it may
> > require a bpp override since HDMI doesn't supports 10 bpc, only 8 or 12. You
> > can
> >  find similar code also in LVDS and even DP.
> > 
> > Unfortunately, there is very little documentation of what the hooks are
> > supposed
> > to do. But for the question at hand, yes, it should really be in
> > intel_dp_compute_config().
> > 
> > Ander
> Inside of intel_dp_compute_config(), do we need to change the pipe_config-
> >pipe_bpp
> to use intel_dp->compliance_force_bpc in case of CTS or should we just modify
> the local
> bpp value to use the force_bpc?

I think is enough to change the bpp variable before the loop that decides the
link rate and lane count, similarly to what is done in the eDP case when a bpp
value is read form the VBT.

Ander

> 
> Manasi
> > 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > +			}
> > > > > +		} else {
> > > > > +			connected_sink_compute_bpp(to_intel_connector
> > > > > (con
> > > > > nect
> > > > > or),
> > > > > +						   pipe_config);
> > > > > +		}
> > > > >  	}
> > > > >  
> > > > >  	return bpp;
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index e23eed7..10eaff8 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -865,6 +865,7 @@ struct intel_dp {
> > > > >  	unsigned long compliance_test_type;
> > > > >  	unsigned long compliance_test_data;
> > > > >  	bool compliance_test_active;
> > > > > +	unsigned long compliance_force_bpc; /* 0 for default or bpc
> > > > > to
> > > > > use */
> > > > There's no code for setting compliance_test_active anywhere. The commit
> > > > message
> > > > mentions debugfs, but I didn't see anything related in the patch. 
> > > It appears that Manasi forgot to include one of the patches I had sent
> > > her.
> > > The debugfs support code was in a separate patch.
> > > 
> > > Jim
> > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > Ander
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] Add support for forcing 6 bpc on DP pipes.

[Manasi Navare]

On Mon, May 23, 2016 at 10:42:41AM -0700, Jim Bride wrote:
> On Mon, May 23, 2016 at 11:22:17AM +0300, Ander Conselvan De Oliveira wrote:
> > On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote:
> > > From: Jim Bride <jim.bride@linux.intel.com>
> > > 
> > > For DP compliance we need to be able to control the output color
> > > type for the pipe associated with the DP port. To do this we rely
> > > on the intel_dp_test_force_bpc debugfs file and the associated value
> > > stored in struct intel_dp. If the debugfs file has a non-zero value
> > > and we're on a display port connector, then we use the value from
> > > debugfs to calculate the bpp for the pipe.  For cases where we are
> > > not on DP or there has not been an overridden value then we behave
> > > as normal.
> > > 
> > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++++++++--
> > >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> > >  2 files changed, 31 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 5ffccf6..1618d36 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -12102,11 +12102,39 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
> > >  
> > >  	/* Clamp display bpp to EDID value */
> > >  	for_each_connector_in_state(state, connector, connector_state, i) {
> > > +		int type = 0;
> > > +
> > > +		if (connector_state->best_encoder) {
> > > +			struct intel_encoder *ienc;
> > > +
> > > +			ienc = to_intel_encoder(connector_state-
> > > >best_encoder);
> > > +			type = ienc->type;
> > > +		}
> > > +
> > >  		if (connector_state->crtc != &crtc->base)
> > >  			continue;
> > >  
> > > -		connected_sink_compute_bpp(to_intel_connector(connector),
> > > -					   pipe_config);
> > > +		/* For DP compliance we need to ensure that we can override
> > > +		 * the computed bpp for the pipe.
> > > +		 */
> > > +		if (type == INTEL_OUTPUT_DISPLAYPORT) {
> > > +			struct intel_dp *intel_dp =
> > > +				enc_to_intel_dp(connector_state-
> > > >best_encoder);
> > > +
> > > +			if (intel_dp &&
> > > +			    (intel_dp->compliance_force_bpc != 0)) {
> > > +				pipe_config->pipe_bpp =
> > > +					intel_dp->compliance_force_bpc*3;
> > > +				DRM_DEBUG_KMS("JMB Setting pipe_bpp to %d\n",
> > > +					      pipe_config->pipe_bpp);
> > > +			} else {
> > > +				connected_sink_compute_bpp(to_intel_connector
> > > (connector),
> > > +						   pipe_config);
> > 
> > This kind of thing should be done in the encoder compute_config hook.
> 
> Even though it's really not specific to an individual encoder configuration?
> This seems to be the central place where we're ensuring that we have a sane
> value for bpp relative to the display, and thus a good place to set this
> override to make compliance happy (which needs a specific bpc value for some
> test cases rather than one that is deemed sane relative to the sink's
> capabilities.
> 
> > 
> > > +			}
> > > +		} else {
> > > +			connected_sink_compute_bpp(to_intel_connector(connect
> > > or),
> > > +						   pipe_config);
> > > +		}
> > >  	}
> > >  
> > >  	return bpp;
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index e23eed7..10eaff8 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -865,6 +865,7 @@ struct intel_dp {
> > >  	unsigned long compliance_test_type;
> > >  	unsigned long compliance_test_data;
> > >  	bool compliance_test_active;
> > > +	unsigned long compliance_force_bpc; /* 0 for default or bpc to use */
> > 
> > There's no code for setting compliance_test_active anywhere. The commit message
> > mentions debugfs, but I didn't see anything related in the patch. 

The compliance_test_active is also being set as part of the compliance
test handlers for edid and video pattern generation.
> 
> It appears that Manasi forgot to include one of the patches I had sent her.
> The debugfs support code was in a separate patch.
> 
> Jim

We do not need the debugfs node to set the bpc value. Instead this bpc value in 
intel_dp structure is being set as a part of video_pattern_autotes.
Instead of userspace setting this value through debugfs node, the kernel is setting this directly.
I will update the commit message to remove the debugfs node setting.

Manasi
> 
> > 
> > 
> > Ander
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/i915: Implement intel_dp_autotest_video_pattern function for DP Video pattern compliance tests

[Manasi Navare]

This video pattern test function gets invoked through the
compliance test handler on a HPD short pulse if the test type is
set to DP_TEST_VIDEO_PATTERN. This performs the DPCD registers
reads to read the requested test pattern, video pattern resolution,
frame rate and bits per color value. The results of this analysis
are handed off to userspace so that the userspace app can set the
video pattern mode appropriately for the test result/response.

The compliance_test_active flag is set at the end of the individual
test handling functions. This is so that the kernel-side operations
can be completed without the risk of interruption from the userspace
app that is polling on that flag.

Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 14 ++++++-
 drivers/gpu/drm/i915/intel_dp.c     | 76 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h    |  9 +++++
 include/drm/drm_dp_helper.h         | 14 ++++++-
 4 files changed, 111 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 6ee69b1..c8d0805 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4458,7 +4458,19 @@ static int i915_displayport_test_data_show(struct seq_file *m, void *data)
 		if (connector->status == connector_status_connected &&
 		    connector->encoder != NULL) {
 			intel_dp = enc_to_intel_dp(connector->encoder);
-			seq_printf(m, "%lx", intel_dp->compliance_test_data);
+			if (intel_dp->compliance_test_type ==
+			    DP_TEST_LINK_EDID_READ)
+				seq_printf(m, "%lx",
+					   intel_dp->compliance_test_data);
+			else if (intel_dp->compliance_test_type ==
+				 DP_TEST_LINK_VIDEO_PATTERN) {
+				seq_printf(m, "hdisplay: %lu\n",
+					   intel_dp->test_data.hdisplay);
+				seq_printf(m, "vdisplay: %lu\n",
+					   intel_dp->test_data.vdisplay);
+				seq_printf(m, "bpc: %u\n",
+					   intel_dp->test_data.bpc);
+			}
 		} else
 			seq_puts(m, "0");
 	}
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 456fc17..134cff8 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4018,6 +4018,82 @@ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
 static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 {
 	uint8_t test_result = DP_TEST_NAK;
+	uint8_t test_pattern;
+	uint16_t  h_width, v_height, test_misc;
+	int status = 0;
+
+	/* Automation support for Video Pattern Tests has a dependency
+	 * on Link training Automation support (CTS Test 4.3.1.11)
+	 * Hence it returns a TEST NAK until the Link Training automation
+	 * support is added to the kernel
+	 */
+	return test_result;
+
+	/* Read the TEST_PATTERN (DP CTS 3.1.5) */
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_PATTERN,
+				  &test_pattern, 1);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Could not read test pattern from"
+			      "refernce sink\n");
+		return 0;
+	}
+	if (test_pattern != DP_COLOR_RAMP)
+		return test_result;
+	intel_dp->test_data.video_pattern = test_pattern;
+
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_H_WIDTH,
+				  &h_width, 2);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Could not read H Width from"
+			      "refernce sink\n");
+		return 0;
+	}
+	intel_dp->test_data.hdisplay = (h_width & DP_TEST_MSB_MASK) >> 8 |
+					(h_width << 8);
+
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_V_HEIGHT,
+				  &v_height, 2);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Could not read V Height from"
+			      "refernce sink\n");
+		return 0;
+	}
+	intel_dp->test_data.vdisplay = (v_height & DP_TEST_MSB_MASK) >> 8 |
+					(v_height << 8);
+
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_MISC,
+				  &test_misc, 1);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Could not read TEST MISC from"
+			      "refernce sink\n");
+		return 0;
+	}
+	if (((test_misc & DP_TEST_COLOR_FORMAT_MASK) >> 1) !=
+	    DP_VIDEO_PATTERN_RGB_FORMAT)
+		return test_result;
+	if (((test_misc & DP_TEST_DYNAMIC_RANGE_MASK) >> 3) !=
+	    DP_VIDEO_PATTERN_VESA)
+		return test_result;
+	switch ((test_misc & DP_TEST_BIT_DEPTH_MASK) >> 5) {
+	case 0:
+		intel_dp->compliance_force_bpc = 6;
+		intel_dp->test_data.bpc = 6;
+		break;
+	case 1:
+		intel_dp->compliance_force_bpc = 8;
+		intel_dp->test_data.bpc = 8;
+		break;
+	default:
+		return test_result;
+	}
+	/* Set test active flag here so userspace doesn't interrupt things */
+	intel_dp->compliance_test_active = 1;
+
+	test_result = DP_TEST_ACK;
+	DRM_DEBUG_KMS("Hdisplay = %lu\n Vdisplay = %lu\n BPC = %u\n ",
+		      intel_dp->test_data.hdisplay,
+		      intel_dp->test_data.vdisplay,
+		      intel_dp->test_data.bpc);
 	return test_result;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 10eaff8..6ba8a75 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -794,6 +794,12 @@ enum link_m_n_set {
 	M2_N2
 };
 
+struct compliance_test_data {
+	uint8_t video_pattern;
+	uint16_t hdisplay, vdisplay;
+	uint8_t bpc;
+};
+
 struct intel_dp {
 	i915_reg_t output_reg;
 	i915_reg_t aux_ch_ctl_reg;
@@ -866,6 +872,9 @@ struct intel_dp {
 	unsigned long compliance_test_data;
 	bool compliance_test_active;
 	unsigned long compliance_force_bpc; /* 0 for default or bpc to use */
+	struct compliance_test_data test_data;   /* a structure to hold all dp
+						  * compliance test data
+						  */
 };
 
 struct intel_digital_port {
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 92d9a52..7422dc5 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -412,7 +412,19 @@
 
 #define DP_TEST_LANE_COUNT		    0x220
 
-#define DP_TEST_PATTERN			    0x221
+#define DP_TEST_PATTERN				0x221
+#define DP_COLOR_RAMP				(1 << 0)
+#define DP_TEST_H_WIDTH				0x22E
+#define DP_TEST_V_HEIGHT			0x230
+#define DP_TEST_MISC				0x232
+#define DP_TEST_MSB_MASK			0xFF00
+#define DP_VIDEO_PATTERN_RGB_FORMAT		0
+#define DP_TEST_COLOR_FORMAT_MASK		0x6
+#define DP_TEST_DYNAMIC_RANGE_MASK		(1 << 3)
+#define DP_VIDEO_PATTERN_VESA			0
+#define DP_TEST_BIT_DEPTH_MASK			0x00E0
+#define DP_TEST_BIT_DEPTH_6			0
+#define DP_TEST_BIT_DEPTH_8			1
 
 #define DP_TEST_CRC_R_CR		    0x240
 #define DP_TEST_CRC_G_Y			    0x242
-- 
1.9.1

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

Re: [PATCH 5/5] drm/i915: Implement intel_dp_autotest_video_pattern function for DP Video pattern compliance tests

[Ander Conselvan De Oliveira]

On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote:
> This video pattern test function gets invoked through the
> compliance test handler on a HPD short pulse if the test type is
> set to DP_TEST_VIDEO_PATTERN. This performs the DPCD registers
> reads to read the requested test pattern, video pattern resolution,
> frame rate and bits per color value. The results of this analysis
> are handed off to userspace so that the userspace app can set the
> video pattern mode appropriately for the test result/response.
> 
> The compliance_test_active flag is set at the end of the individual
> test handling functions. This is so that the kernel-side operations
> can be completed without the risk of interruption from the userspace
> app that is polling on that flag.
> 
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 14 ++++++-
>  drivers/gpu/drm/i915/intel_dp.c     | 76
> +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h    |  9 +++++
>  include/drm/drm_dp_helper.h         | 14 ++++++-
>  4 files changed, 111 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 6ee69b1..c8d0805 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4458,7 +4458,19 @@ static int i915_displayport_test_data_show(struct
> seq_file *m, void *data)
>  		if (connector->status == connector_status_connected &&
>  		    connector->encoder != NULL) {
>  			intel_dp = enc_to_intel_dp(connector->encoder);
> -			seq_printf(m, "%lx", intel_dp->compliance_test_data);
> +			if (intel_dp->compliance_test_type ==
> +			    DP_TEST_LINK_EDID_READ)
> +				seq_printf(m, "%lx",
> +					   intel_dp->compliance_test_data);
> +			else if (intel_dp->compliance_test_type ==
> +				 DP_TEST_LINK_VIDEO_PATTERN) {
> +				seq_printf(m, "hdisplay: %lu\n",
> +					   intel_dp->test_data.hdisplay);
> +				seq_printf(m, "vdisplay: %lu\n",
> +					   intel_dp->test_data.vdisplay);
> +				seq_printf(m, "bpc: %u\n",
> +					   intel_dp->test_data.bpc);
> +			}
>  		} else
>  			seq_puts(m, "0");
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 456fc17..134cff8 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4018,6 +4018,82 @@ static uint8_t intel_dp_autotest_link_training(struct
> intel_dp *intel_dp)
>  static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>  {
>  	uint8_t test_result = DP_TEST_NAK;
> +	uint8_t test_pattern;
> +	uint16_t  h_width, v_height, test_misc;
> +	int status = 0;
> +
> +	/* Automation support for Video Pattern Tests has a dependency
> +	 * on Link training Automation support (CTS Test 4.3.1.11)
> +	 * Hence it returns a TEST NAK until the Link Training automation
> +	 * support is added to the kernel
> +	 */
> +	return test_result;

We shouldn't merge this patch until this is resolved. There's no point in adding
dead code.


> +
> +	/* Read the TEST_PATTERN (DP CTS 3.1.5) */
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_PATTERN,
> +				  &test_pattern, 1);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Could not read test pattern from"
> +			      "refernce sink\n");
> +		return 0;
> +	}
> +	if (test_pattern != DP_COLOR_RAMP)
> +		return test_result;
> +	intel_dp->test_data.video_pattern = test_pattern;
> +
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_H_WIDTH,
> +				  &h_width, 2);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Could not read H Width from"
> +			      "refernce sink\n");
> +		return 0;
> +	}
> +	intel_dp->test_data.hdisplay = (h_width & DP_TEST_MSB_MASK) >> 8 |
> +					(h_width << 8);

Just use the kernel endianness helpers, i.e.:

__le16 h_width;

drm_dp_dpcd_read(..., &h_width, 2)
hdisplay = le16_to_cpu(h_width);

> +
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_V_HEIGHT,
> +				  &v_height, 2);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Could not read V Height from"
> +			      "refernce sink\n");

reference

> +		return 0;
> +	}
> +	intel_dp->test_data.vdisplay = (v_height & DP_TEST_MSB_MASK) >> 8 |
> +					(v_height << 8);

Same here.

> +
> +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_MISC,
> +				  &test_misc, 1);
> +	if (status <= 0) {
> +		DRM_DEBUG_KMS("Could not read TEST MISC from"
> +			      "refernce sink\n");

reference

> +		return 0;
> +	}
> +	if (((test_misc & DP_TEST_COLOR_FORMAT_MASK) >> 1) !=
> +	    DP_VIDEO_PATTERN_RGB_FORMAT)
> +		return test_result;
> +	if (((test_misc & DP_TEST_DYNAMIC_RANGE_MASK) >> 3) !=
> +	    DP_VIDEO_PATTERN_VESA)
> +		return test_result;
> +	switch ((test_misc & DP_TEST_BIT_DEPTH_MASK) >> 5) {

Maybe define *_SHIFT to avoid the hardcoded values. A couple of helpers to make
the code more readable woudn't be bad either.

> +	case 0:
> +		intel_dp->compliance_force_bpc = 6;
> +		intel_dp->test_data.bpc = 6;
> +		break;
> +	case 1:
> +		intel_dp->compliance_force_bpc = 8;
> +		intel_dp->test_data.bpc = 8;

So here's where compliance_force_bpc from patch 3 is set. Would be better to
squash that patch with this one.

> +		break;
> +	default:
> +		return test_result;
> +	}
> +	/* Set test active flag here so userspace doesn't interrupt things */
> +	intel_dp->compliance_test_active = 1;
> +
> +	test_result = DP_TEST_ACK;
> +	DRM_DEBUG_KMS("Hdisplay = %lu\n Vdisplay = %lu\n BPC = %u\n ",
> +		      intel_dp->test_data.hdisplay,
> +		      intel_dp->test_data.vdisplay,
> +		      intel_dp->test_data.bpc);
>  	return test_result;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 10eaff8..6ba8a75 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -794,6 +794,12 @@ enum link_m_n_set {
>  	M2_N2
>  };
>  
> +struct compliance_test_data {
> +	uint8_t video_pattern;
> +	uint16_t hdisplay, vdisplay;
> +	uint8_t bpc;
> +};
> +
>  struct intel_dp {
>  	i915_reg_t output_reg;
>  	i915_reg_t aux_ch_ctl_reg;
> @@ -866,6 +872,9 @@ struct intel_dp {
>  	unsigned long compliance_test_data;
>  	bool compliance_test_active;
>  	unsigned long compliance_force_bpc; /* 0 for default or bpc to use */
> +	struct compliance_test_data test_data;   /* a structure to hold all
> dp
> +						  * compliance test data
> +						  */
>  };
>  
>  struct intel_digital_port {
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 92d9a52..7422dc5 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -412,7 +412,19 @@
>  
>  #define DP_TEST_LANE_COUNT		    0x220
>  
> -#define DP_TEST_PATTERN			    0x221
> +#define DP_TEST_PATTERN				0x221
> +#define DP_COLOR_RAMP				(1 << 0)
> +#define DP_TEST_H_WIDTH				0x22E
> +#define DP_TEST_V_HEIGHT			0x230
> +#define DP_TEST_MISC				0x232
> +#define DP_TEST_MSB_MASK			0xFF00
> +#define DP_VIDEO_PATTERN_RGB_FORMAT		0
> +#define DP_TEST_COLOR_FORMAT_MASK		0x6
> +#define DP_TEST_DYNAMIC_RANGE_MASK		(1 << 3)
> +#define DP_VIDEO_PATTERN_VESA			0
> +#define DP_TEST_BIT_DEPTH_MASK			0x00E0
> +#define DP_TEST_BIT_DEPTH_6			0
> +#define DP_TEST_BIT_DEPTH_8			1

This should be in a separate patch with

Cc: dri-devel@lists.freedesktop.org

>  
>  #define DP_TEST_CRC_R_CR		    0x240
>  #define DP_TEST_CRC_G_Y			    0x242
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Implement intel_dp_autotest_video_pattern function for DP Video pattern compliance tests

[Manasi Navare]

On Mon, May 23, 2016 at 03:00:20PM +0300, Ander Conselvan De Oliveira wrote:
> On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote:
> > This video pattern test function gets invoked through the
> > compliance test handler on a HPD short pulse if the test type is
> > set to DP_TEST_VIDEO_PATTERN. This performs the DPCD registers
> > reads to read the requested test pattern, video pattern resolution,
> > frame rate and bits per color value. The results of this analysis
> > are handed off to userspace so that the userspace app can set the
> > video pattern mode appropriately for the test result/response.
> > 
> > The compliance_test_active flag is set at the end of the individual
> > test handling functions. This is so that the kernel-side operations
> > can be completed without the risk of interruption from the userspace
> > app that is polling on that flag.
> > 
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 14 ++++++-
> >  drivers/gpu/drm/i915/intel_dp.c     | 76
> > +++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h    |  9 +++++
> >  include/drm/drm_dp_helper.h         | 14 ++++++-
> >  4 files changed, 111 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 6ee69b1..c8d0805 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -4458,7 +4458,19 @@ static int i915_displayport_test_data_show(struct
> > seq_file *m, void *data)
> >  		if (connector->status == connector_status_connected &&
> >  		    connector->encoder != NULL) {
> >  			intel_dp = enc_to_intel_dp(connector->encoder);
> > -			seq_printf(m, "%lx", intel_dp->compliance_test_data);
> > +			if (intel_dp->compliance_test_type ==
> > +			    DP_TEST_LINK_EDID_READ)
> > +				seq_printf(m, "%lx",
> > +					   intel_dp->compliance_test_data);
> > +			else if (intel_dp->compliance_test_type ==
> > +				 DP_TEST_LINK_VIDEO_PATTERN) {
> > +				seq_printf(m, "hdisplay: %lu\n",
> > +					   intel_dp->test_data.hdisplay);
> > +				seq_printf(m, "vdisplay: %lu\n",
> > +					   intel_dp->test_data.vdisplay);
> > +				seq_printf(m, "bpc: %u\n",
> > +					   intel_dp->test_data.bpc);
> > +			}
> >  		} else
> >  			seq_puts(m, "0");
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 456fc17..134cff8 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4018,6 +4018,82 @@ static uint8_t intel_dp_autotest_link_training(struct
> > intel_dp *intel_dp)
> >  static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
> >  {
> >  	uint8_t test_result = DP_TEST_NAK;
> > +	uint8_t test_pattern;
> > +	uint16_t  h_width, v_height, test_misc;
> > +	int status = 0;
> > +
> > +	/* Automation support for Video Pattern Tests has a dependency
> > +	 * on Link training Automation support (CTS Test 4.3.1.11)
> > +	 * Hence it returns a TEST NAK until the Link Training automation
> > +	 * support is added to the kernel
> > +	 */
> > +	return test_result;
> 
> We shouldn't merge this patch until this is resolved. There's no point in adding
> dead code.
> 
>

I agree. I would still respin it based on the review feedback and keep it ready.
So as soon as the link training rework is done, this patch can be pulled in and 
this test can b enabled.
 
> > +
> > +	/* Read the TEST_PATTERN (DP CTS 3.1.5) */
> > +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_PATTERN,
> > +				  &test_pattern, 1);
> > +	if (status <= 0) {
> > +		DRM_DEBUG_KMS("Could not read test pattern from"
> > +			      "refernce sink\n");
> > +		return 0;
> > +	}
> > +	if (test_pattern != DP_COLOR_RAMP)
> > +		return test_result;
> > +	intel_dp->test_data.video_pattern = test_pattern;
> > +
> > +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_H_WIDTH,
> > +				  &h_width, 2);
> > +	if (status <= 0) {
> > +		DRM_DEBUG_KMS("Could not read H Width from"
> > +			      "refernce sink\n");
> > +		return 0;
> > +	}
> > +	intel_dp->test_data.hdisplay = (h_width & DP_TEST_MSB_MASK) >> 8 |
> > +					(h_width << 8);
> 
> Just use the kernel endianness helpers, i.e.:
> 
> __le16 h_width;
> 
> drm_dp_dpcd_read(..., &h_width, 2)
> hdisplay = le16_to_cpu(h_width);
>

I am not changing the endiannness here. This logic is implemented to swap
the LSB and MSB because dpcd_read helper function reads two bytes from two consecutive
registers into a 16 bit variable. This is required because DPR writes 0:7 bits of Hwidth
to register 0x22Fh and 8:15 bits into register 0x22Eh. So when dpcd_read reads these two registers,
it stores LSB and MSB in the swapped manner in a 16 bit variable. 
Refer to Table 3-4 of VESA DP Link layer CTS spec version 1.2

 
> > +
> > +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_V_HEIGHT,
> > +				  &v_height, 2);
> > +	if (status <= 0) {
> > +		DRM_DEBUG_KMS("Could not read V Height from"
> > +			      "refernce sink\n");
> 
> reference
> 
> > +		return 0;
> > +	}
> > +	intel_dp->test_data.vdisplay = (v_height & DP_TEST_MSB_MASK) >> 8 |
> > +					(v_height << 8);
> 
> Same here.
> 
> > +
> > +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_MISC,
> > +				  &test_misc, 1);
> > +	if (status <= 0) {
> > +		DRM_DEBUG_KMS("Could not read TEST MISC from"
> > +			      "refernce sink\n");
> 
> reference
> 
> > +		return 0;
> > +	}
> > +	if (((test_misc & DP_TEST_COLOR_FORMAT_MASK) >> 1) !=
> > +	    DP_VIDEO_PATTERN_RGB_FORMAT)
> > +		return test_result;
> > +	if (((test_misc & DP_TEST_DYNAMIC_RANGE_MASK) >> 3) !=
> > +	    DP_VIDEO_PATTERN_VESA)
> > +		return test_result;
> > +	switch ((test_misc & DP_TEST_BIT_DEPTH_MASK) >> 5) {
> 
> Maybe define *_SHIFT to avoid the hardcoded values. A couple of helpers to make
> the code more readable woudn't be bad either.

Will do
> 
> > +	case 0:
> > +		intel_dp->compliance_force_bpc = 6;
> > +		intel_dp->test_data.bpc = 6;
> > +		break;
> > +	case 1:
> > +		intel_dp->compliance_force_bpc = 8;
> > +		intel_dp->test_data.bpc = 8;
> 
> So here's where compliance_force_bpc from patch 3 is set. Would be better to
> squash that patch with this one.

I feel this should be a standalone patch that adds the test handler for 
video pattern generation and the force_bpc can be in a separate patch.
However I will go ahead and change the commit message for the bpc patch
to indicate that it gets set by video pattern CTS tests.


> 
> > +		break;
> > +	default:
> > +		return test_result;
> > +	}
> > +	/* Set test active flag here so userspace doesn't interrupt things */
> > +	intel_dp->compliance_test_active = 1;
> > +
> > +	test_result = DP_TEST_ACK;
> > +	DRM_DEBUG_KMS("Hdisplay = %lu\n Vdisplay = %lu\n BPC = %u\n ",
> > +		      intel_dp->test_data.hdisplay,
> > +		      intel_dp->test_data.vdisplay,
> > +		      intel_dp->test_data.bpc);
> >  	return test_result;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 10eaff8..6ba8a75 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -794,6 +794,12 @@ enum link_m_n_set {
> >  	M2_N2
> >  };
> >  
> > +struct compliance_test_data {
> > +	uint8_t video_pattern;
> > +	uint16_t hdisplay, vdisplay;
> > +	uint8_t bpc;
> > +};
> > +
> >  struct intel_dp {
> >  	i915_reg_t output_reg;
> >  	i915_reg_t aux_ch_ctl_reg;
> > @@ -866,6 +872,9 @@ struct intel_dp {
> >  	unsigned long compliance_test_data;
> >  	bool compliance_test_active;
> >  	unsigned long compliance_force_bpc; /* 0 for default or bpc to use */
> > +	struct compliance_test_data test_data;   /* a structure to hold all
> > dp
> > +						  * compliance test data
> > +						  */
> >  };
> >  
> >  struct intel_digital_port {
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index 92d9a52..7422dc5 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -412,7 +412,19 @@
> >  
> >  #define DP_TEST_LANE_COUNT		    0x220
> >  
> > -#define DP_TEST_PATTERN			    0x221
> > +#define DP_TEST_PATTERN				0x221
> > +#define DP_COLOR_RAMP				(1 << 0)
> > +#define DP_TEST_H_WIDTH				0x22E
> > +#define DP_TEST_V_HEIGHT			0x230
> > +#define DP_TEST_MISC				0x232
> > +#define DP_TEST_MSB_MASK			0xFF00
> > +#define DP_VIDEO_PATTERN_RGB_FORMAT		0
> > +#define DP_TEST_COLOR_FORMAT_MASK		0x6
> > +#define DP_TEST_DYNAMIC_RANGE_MASK		(1 << 3)
> > +#define DP_VIDEO_PATTERN_VESA			0
> > +#define DP_TEST_BIT_DEPTH_MASK			0x00E0
> > +#define DP_TEST_BIT_DEPTH_6			0
> > +#define DP_TEST_BIT_DEPTH_8			1
> 
> This should be in a separate patch with

I agree, i will separate this out into a different patch.
> 
> Cc: dri-devel@lists.freedesktop.org
> 
> >  
> >  #define DP_TEST_CRC_R_CR		    0x240
> >  #define DP_TEST_CRC_G_Y			    0x242
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Implement intel_dp_autotest_video_pattern function for DP Video pattern compliance tests

[Ander Conselvan De Oliveira]

On Wed, 2016-05-25 at 15:46 -0700, Manasi Navare wrote:
> On Mon, May 23, 2016 at 03:00:20PM +0300, Ander Conselvan De Oliveira wrote:
> > 
> > On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote:
> > > 
> > > This video pattern test function gets invoked through the
> > > compliance test handler on a HPD short pulse if the test type is
> > > set to DP_TEST_VIDEO_PATTERN. This performs the DPCD registers
> > > reads to read the requested test pattern, video pattern resolution,
> > > frame rate and bits per color value. The results of this analysis
> > > are handed off to userspace so that the userspace app can set the
> > > video pattern mode appropriately for the test result/response.
> > > 
> > > The compliance_test_active flag is set at the end of the individual
> > > test handling functions. This is so that the kernel-side operations
> > > can be completed without the risk of interruption from the userspace
> > > app that is polling on that flag.
> > > 
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 14 ++++++-
> > >  drivers/gpu/drm/i915/intel_dp.c     | 76
> > > +++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h    |  9 +++++
> > >  include/drm/drm_dp_helper.h         | 14 ++++++-
> > >  4 files changed, 111 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 6ee69b1..c8d0805 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -4458,7 +4458,19 @@ static int i915_displayport_test_data_show(struct
> > > seq_file *m, void *data)
> > >  		if (connector->status == connector_status_connected &&
> > >  		    connector->encoder != NULL) {
> > >  			intel_dp = enc_to_intel_dp(connector->encoder);
> > > -			seq_printf(m, "%lx", intel_dp-
> > > >compliance_test_data);
> > > +			if (intel_dp->compliance_test_type ==
> > > +			    DP_TEST_LINK_EDID_READ)
> > > +				seq_printf(m, "%lx",
> > > +					   intel_dp-
> > > >compliance_test_data);
> > > +			else if (intel_dp->compliance_test_type ==
> > > +				 DP_TEST_LINK_VIDEO_PATTERN) {
> > > +				seq_printf(m, "hdisplay: %lu\n",
> > > +					   intel_dp->test_data.hdisplay);
> > > +				seq_printf(m, "vdisplay: %lu\n",
> > > +					   intel_dp->test_data.vdisplay);
> > > +				seq_printf(m, "bpc: %u\n",
> > > +					   intel_dp->test_data.bpc);
> > > +			}
> > >  		} else
> > >  			seq_puts(m, "0");
> > >  	}
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 456fc17..134cff8 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4018,6 +4018,82 @@ static uint8_t
> > > intel_dp_autotest_link_training(struct
> > > intel_dp *intel_dp)
> > >  static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
> > >  {
> > >  	uint8_t test_result = DP_TEST_NAK;
> > > +	uint8_t test_pattern;
> > > +	uint16_t  h_width, v_height, test_misc;
> > > +	int status = 0;
> > > +
> > > +	/* Automation support for Video Pattern Tests has a dependency
> > > +	 * on Link training Automation support (CTS Test 4.3.1.11)
> > > +	 * Hence it returns a TEST NAK until the Link Training automation
> > > +	 * support is added to the kernel
> > > +	 */
> > > +	return test_result;
> > We shouldn't merge this patch until this is resolved. There's no point in
> > adding
> > dead code.
> > 
> > 
> I agree. I would still respin it based on the review feedback and keep it
> ready.
> So as soon as the link training rework is done, this patch can be pulled in
> and 
> this test can b enabled.
>  
> > 
> > > 
> > > +
> > > +	/* Read the TEST_PATTERN (DP CTS 3.1.5) */
> > > +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_PATTERN,
> > > +				  &test_pattern, 1);
> > > +	if (status <= 0) {
> > > +		DRM_DEBUG_KMS("Could not read test pattern from"
> > > +			      "refernce sink\n");
> > > +		return 0;
> > > +	}
> > > +	if (test_pattern != DP_COLOR_RAMP)
> > > +		return test_result;
> > > +	intel_dp->test_data.video_pattern = test_pattern;
> > > +
> > > +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_H_WIDTH,
> > > +				  &h_width, 2);
> > > +	if (status <= 0) {
> > > +		DRM_DEBUG_KMS("Could not read H Width from"
> > > +			      "refernce sink\n");
> > > +		return 0;
> > > +	}
> > > +	intel_dp->test_data.hdisplay = (h_width & DP_TEST_MSB_MASK) >> 8
> > > |
> > > +					(h_width << 8);
> > Just use the kernel endianness helpers, i.e.:
> > 
> > __le16 h_width;
> > 
> > drm_dp_dpcd_read(..., &h_width, 2)
> > hdisplay = le16_to_cpu(h_width);
> > 
> I am not changing the endiannness here. This logic is implemented to swap
> the LSB and MSB because dpcd_read helper function reads two bytes from two
> consecutive
> registers into a 16 bit variable. This is required because DPR writes 0:7 bits
> of Hwidth
> to register 0x22Fh and 8:15 bits into register 0x22Eh. So when dpcd_read reads
> these two registers,
> it stores LSB and MSB in the swapped manner in a 16 bit variable. 
> Refer to Table 3-4 of VESA DP Link layer CTS spec version 1.2

Would the byte swap above still be necessary in a big-endian architecture?

While the snippet I wrote above is wrong, this is still an endianness issue. The
MSB is at the lowest address, which means the value is big-endian. And x86 is
little endian. So that should have been:

__be16 h_width;
drm_dp_dpcd_read(..., &h_width, sizeof h_width);
hdisplay = be16_to_cpu(h_width);


> 
>  
> > 
> > > 
> > > +
> > > +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_V_HEIGHT,
> > > +				  &v_height, 2);
> > > +	if (status <= 0) {
> > > +		DRM_DEBUG_KMS("Could not read V Height from"
> > > +			      "refernce sink\n");
> > reference
> > 
> > > 
> > > +		return 0;
> > > +	}
> > > +	intel_dp->test_data.vdisplay = (v_height & DP_TEST_MSB_MASK) >> 8
> > > |
> > > +					(v_height << 8);
> > Same here.
> > 
> > > 
> > > +
> > > +	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_MISC,
> > > +				  &test_misc, 1);
> > > +	if (status <= 0) {
> > > +		DRM_DEBUG_KMS("Could not read TEST MISC from"
> > > +			      "refernce sink\n");
> > reference
> > 
> > > 
> > > +		return 0;
> > > +	}
> > > +	if (((test_misc & DP_TEST_COLOR_FORMAT_MASK) >> 1) !=
> > > +	    DP_VIDEO_PATTERN_RGB_FORMAT)
> > > +		return test_result;
> > > +	if (((test_misc & DP_TEST_DYNAMIC_RANGE_MASK) >> 3) !=
> > > +	    DP_VIDEO_PATTERN_VESA)
> > > +		return test_result;
> > > +	switch ((test_misc & DP_TEST_BIT_DEPTH_MASK) >> 5) {
> > Maybe define *_SHIFT to avoid the hardcoded values. A couple of helpers to
> > make
> > the code more readable woudn't be bad either.
> Will do
> > 
> > 
> > > 
> > > +	case 0:
> > > +		intel_dp->compliance_force_bpc = 6;
> > > +		intel_dp->test_data.bpc = 6;
> > > +		break;
> > > +	case 1:
> > > +		intel_dp->compliance_force_bpc = 8;
> > > +		intel_dp->test_data.bpc = 8;
> > So here's where compliance_force_bpc from patch 3 is set. Would be better to
> > squash that patch with this one.
> I feel this should be a standalone patch that adds the test handler for 
> video pattern generation and the force_bpc can be in a separate patch.
> However I will go ahead and change the commit message for the bpc patch
> to indicate that it gets set by video pattern CTS tests.

It's a better practice to include code where its used. It makes review easier
since things are in context and it also helps with bisectability. If a bisect
result would point to this patch, but the error would be in the code added by
the previous one, that wouldn't be immediately obvious. Since this patch is
fairly small, I don't think the split makes sense.

Ander

> 
> 
> > 
> > 
> > > 
> > > +		break;
> > > +	default:
> > > +		return test_result;
> > > +	}
> > > +	/* Set test active flag here so userspace doesn't interrupt
> > > things */
> > > +	intel_dp->compliance_test_active = 1;
> > > +
> > > +	test_result = DP_TEST_ACK;
> > > +	DRM_DEBUG_KMS("Hdisplay = %lu\n Vdisplay = %lu\n BPC = %u\n ",
> > > +		      intel_dp->test_data.hdisplay,
> > > +		      intel_dp->test_data.vdisplay,
> > > +		      intel_dp->test_data.bpc);
> > >  	return test_result;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 10eaff8..6ba8a75 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -794,6 +794,12 @@ enum link_m_n_set {
> > >  	M2_N2
> > >  };
> > >  
> > > +struct compliance_test_data {
> > > +	uint8_t video_pattern;
> > > +	uint16_t hdisplay, vdisplay;
> > > +	uint8_t bpc;
> > > +};
> > > +
> > >  struct intel_dp {
> > >  	i915_reg_t output_reg;
> > >  	i915_reg_t aux_ch_ctl_reg;
> > > @@ -866,6 +872,9 @@ struct intel_dp {
> > >  	unsigned long compliance_test_data;
> > >  	bool compliance_test_active;
> > >  	unsigned long compliance_force_bpc; /* 0 for default or bpc to
> > > use */
> > > +	struct compliance_test_data test_data;   /* a structure to hold
> > > all
> > > dp
> > > +						  * compliance test data
> > > +						  */
> > >  };
> > >  
> > >  struct intel_digital_port {
> > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > > index 92d9a52..7422dc5 100644
> > > --- a/include/drm/drm_dp_helper.h
> > > +++ b/include/drm/drm_dp_helper.h
> > > @@ -412,7 +412,19 @@
> > >  
> > >  #define DP_TEST_LANE_COUNT		    0x220
> > >  
> > > -#define DP_TEST_PATTERN			    0x221
> > > +#define DP_TEST_PATTERN				0x221
> > > +#define DP_COLOR_RAMP				(1 << 0)
> > > +#define DP_TEST_H_WIDTH				0x22E
> > > +#define DP_TEST_V_HEIGHT			0x230
> > > +#define DP_TEST_MISC				0x232
> > > +#define DP_TEST_MSB_MASK			0xFF00
> > > +#define DP_VIDEO_PATTERN_RGB_FORMAT		0
> > > +#define DP_TEST_COLOR_FORMAT_MASK		0x6
> > > +#define DP_TEST_DYNAMIC_RANGE_MASK		(1 << 3)
> > > +#define DP_VIDEO_PATTERN_VESA			0
> > > +#define DP_TEST_BIT_DEPTH_MASK			0x00E0
> > > +#define DP_TEST_BIT_DEPTH_6			0
> > > +#define DP_TEST_BIT_DEPTH_8			1
> > This should be in a separate patch with
> I agree, i will separate this out into a different patch.
> > 
> > 
> > Cc: dri-devel@lists.freedesktop.org
> > 
> > > 
> > >  
> > >  #define DP_TEST_CRC_R_CR		    0x240
> > >  #define DP_TEST_CRC_G_Y			    0x242
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for Add automation support for DP compliance Tests

[Patchwork]

== Series Details ==

Series: Add automation support for DP compliance Tests
URL   : https://patchwork.freedesktop.org/series/6563/
State : success

== Summary ==

Series 6563v1 Add automation support for DP compliance Tests
http://patchwork.freedesktop.org/api/1.0/series/6563/revisions/1/mbox/


bdw-nuci7-2      total:203  pass:191  dwarn:0   dfail:0   fail:0   skip:12 
bsw-nuc-2        total:202  pass:161  dwarn:0   dfail:0   fail:0   skip:41 
byt-nuc          total:202  pass:161  dwarn:0   dfail:0   fail:0   skip:41 
ivb-t430s        total:203  pass:172  dwarn:0   dfail:0   fail:0   skip:31 
skl-i7k-2        total:203  pass:176  dwarn:0   dfail:0   fail:0   skip:27 
snb-dellxps      total:203  pass:161  dwarn:0   dfail:0   fail:0   skip:42 
snb-x220t        total:203  pass:161  dwarn:0   dfail:0   fail:1   skip:41 

Results at /archive/results/CI_IGT_test/Patchwork_2120/

4acfa93fec0ce38d4efdb0acf0081c39338833c8 drm-intel-nightly: 2016y-04m-29d-16h-48m-26s UTC integration manifest
de0fcd5 drm/i915: Implement intel_dp_autotest_video_pattern function for DP Video pattern compliance tests
0a1638ca Add support for forcing 6 bpc on DP pipes.
2b10ee0 drm/i915: Fixes to support the DP Compliance EDID tests.
d91a243 drm/i915: Disable the Link training automation support
cd9949c drm/i915: Invoke the DP Compliance test request handler in the short pulse path

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