[intel-gfx][PATCH V4] Displayport compliance testing V4

[intel-gfx][PATCH V4] Displayport compliance testing V4

[Todd Previte]

This is the 4th iteration of the Displayport compliance testing patch set for 
performing compliance testing operations of the i915 driver. High level changes 
are listed below, with the specifics for each patch listed in the commit messages.

Kernel:

Changes for V4:
- Removed the code for link configuration in debugfs. It wasn’t used in this 
  patch set so there was no need to add 500+ lines of code to the kernel. It may  
  be reintroduced in a future patch if it becomes necessary to support link 
  configuration testing for Displayport compliance
- Merged working changes in to the kernel code to keep the patches smaller and
  as discrete, testable units. This included moving variables around between 
  patches such that their declaration and use appears in the same patch. One 
  variable was removed entirely as it was no longer necessary.
- Changed the debugfs interface for test control. Previously test control was 
  handled by a single file that contained tags and values that were parsed and 
  used by both the user app and the kernel. This required a lot of parsing code    
  on both sides of the equation. That has been eliminated in favor of 3 separate,
  single value files for test type, test data and testing active. This reduces 
  the overhead of polling on test_active in the user app as well as eliminating 
  the need to parse a single monolithic file every time it checks the flag 
  (currently 1ms intervals). The net result is a more responsible app and a lot 
  less code on both sides.

Userspace app:

The userspace app can be found here:

https://github.com/tprevite/intel-gpu-tools/tree/dp_compliance

The user app has the following requirements:
- Must be executed as root from the command line
- No other display managers can be running. Must be in console mode.
- Only the test device should be connected to one of the external Displayport 
  ports. No other displays should be connected via Displayport. eDP displays are 
  ignored by the compliance code so they should operate normally.

Previous versions of the user app disabled all displays on the DUT. That is no 
longer necessary in order to execute Displayport compliance testing. The app can 
be run remotely via an SSH login or directly on the DUT at the preference of the 
operator. Aside from starting and stopping the app itself, no direct interaction 
is necessary at this time on the part of the test operator to perform compliance 
testing.

Some of the tests will still pass if the user app isn't running. The primary   
purpose of the user space application is to handle the heavy lifting of the
mode sets required to set the specific display resolutions for the tests. 


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

[PATCH 1/9] drm/i915: Add automated testing support for Displayport compliance testing

[Todd Previte]

Add the skeleton framework for supporting automation for Displayport compliance
testing. This patch adds the necessary framework for the source device to
appropriately respond to test automation requests from a sink device.

V2:
- Addressed previous mailing list feedback
- Fixed compilation issue (struct members declared in a later patch)
- Updated debug messages to be more accurate
- Added status checks for the DPCD read/write calls
- Removed excess comments and debug messages
- Fixed debug message compilation warnings
- Fixed compilation issue with missing variables
- Updated link training autotest to ACK

V3:
- Fixed the checks on the DPCD return code to be <= 0
  rather than != 0
- Removed extraneous assignment of a NAK return code in the
  DPCD read failure case
- Changed the return in the DPCD read failure case to a goto
  to the exit point where the status code is written to the sink
- Removed FAUX test case since it's deprecated now
- Removed the compliance flag assignment in handle_test_request

V4:
- Moved declaration of type_type here
- Removed declaration of test_data (moved to a later patch)
- Added reset to 0 for compliance test variables

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 75 +++++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h |  4 +++
 2 files changed, 75 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index eea9e36..960cc68 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3746,11 +3746,78 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 	return true;
 }
 
-static void
-intel_dp_handle_test_request(struct intel_dp *intel_dp)
+static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_ACK;
+	return test_result;
+}
+
+static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_NAK;
+	return test_result;
+}
+
+static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_NAK;
+	return test_result;
+}
+
+static uint8_t intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_NAK;
+	return test_result;
+}
+
+static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
 {
-	/* NAK by default */
-	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK);
+	uint8_t response = DP_TEST_NAK;
+	uint8_t rxdata = 0;
+	int status = 0;
+
+	intel_dp->compliance_testing_active = 0;
+	intel_dp->aux.i2c_nack_count = 0;
+	intel_dp->aux.i2c_defer_count = 0;
+
+	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
+	if (status <= 0) {
+		DRM_DEBUG_KMS("Could not read test request from sink\n");
+		goto update_status;
+	}
+
+	switch (rxdata) {
+	case DP_TEST_LINK_TRAINING:
+		DRM_DEBUG_KMS("LINK_TRAINING test requested\n");
+		intel_dp->compliance_test_type = DP_TEST_LINK_TRAINING;
+		response = intel_dp_autotest_link_training(intel_dp);
+		break;
+	case DP_TEST_LINK_VIDEO_PATTERN:
+		DRM_DEBUG_KMS("TEST_PATTERN test requested\n");
+		intel_dp->compliance_test_type = DP_TEST_LINK_VIDEO_PATTERN;
+		response = intel_dp_autotest_video_pattern(intel_dp);
+		break;
+	case DP_TEST_LINK_EDID_READ:
+		DRM_DEBUG_KMS("EDID test requested\n");
+		intel_dp->compliance_test_type = DP_TEST_LINK_EDID_READ;
+		response = intel_dp_autotest_edid(intel_dp);
+		break;
+	case DP_TEST_LINK_PHY_TEST_PATTERN:
+		DRM_DEBUG_KMS("PHY_PATTERN test requested\n");
+		intel_dp->compliance_test_type = DP_TEST_LINK_PHY_TEST_PATTERN;
+		response = intel_dp_autotest_phy_pattern(intel_dp);
+		break;
+	default:
+		DRM_DEBUG_KMS("Invalid test request '%02x'\n", rxdata);
+		break;
+	}
+
+update_status:
+	status = drm_dp_dpcd_write(&intel_dp->aux,
+				   DP_TEST_RESPONSE,
+				   &response, 1);
+	if (status <= 0)
+		DRM_DEBUG_KMS("Could not write test response to sink\n");
 }
 
 static int
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index eef79cc..e7b62be 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -647,6 +647,10 @@ struct intel_dp {
 				     bool has_aux_irq,
 				     int send_bytes,
 				     uint32_t aux_clock_divider);
+
+	/* Displayport compliance testing */
+	unsigned long compliance_test_type;
+	bool compliance_testing_active;
 };
 
 struct intel_digital_port {
-- 
1.9.1

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

Re: [PATCH 1/9] drm/i915: Add automated testing support for Displayport compliance testing

[Paulo Zanoni]

2015-03-31 14:14 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> Add the skeleton framework for supporting automation for Displayport compliance
> testing. This patch adds the necessary framework for the source device to
> appropriately respond to test automation requests from a sink device.
>
> V2:
> - Addressed previous mailing list feedback
> - Fixed compilation issue (struct members declared in a later patch)
> - Updated debug messages to be more accurate
> - Added status checks for the DPCD read/write calls
> - Removed excess comments and debug messages
> - Fixed debug message compilation warnings
> - Fixed compilation issue with missing variables
> - Updated link training autotest to ACK
>
> V3:
> - Fixed the checks on the DPCD return code to be <= 0
>   rather than != 0
> - Removed extraneous assignment of a NAK return code in the
>   DPCD read failure case
> - Changed the return in the DPCD read failure case to a goto
>   to the exit point where the status code is written to the sink
> - Removed FAUX test case since it's deprecated now
> - Removed the compliance flag assignment in handle_test_request
>
> V4:
> - Moved declaration of type_type here
> - Removed declaration of test_data (moved to a later patch)
> - Added reset to 0 for compliance test variables
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 75 +++++++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h |  4 +++
>  2 files changed, 75 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index eea9e36..960cc68 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3746,11 +3746,78 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>         return true;
>  }
>
> -static void
> -intel_dp_handle_test_request(struct intel_dp *intel_dp)
> +static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
> +{
> +       uint8_t test_result = DP_TEST_ACK;
> +       return test_result;
> +}
> +
> +static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
> +{
> +       uint8_t test_result = DP_TEST_NAK;
> +       return test_result;
> +}
> +
> +static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
> +{
> +       uint8_t test_result = DP_TEST_NAK;
> +       return test_result;
> +}
> +
> +static uint8_t intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
> +{
> +       uint8_t test_result = DP_TEST_NAK;
> +       return test_result;
> +}
> +
> +static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  {
> -       /* NAK by default */
> -       drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK);
> +       uint8_t response = DP_TEST_NAK;
> +       uint8_t rxdata = 0;
> +       int status = 0;
> +
> +       intel_dp->compliance_testing_active = 0;
> +       intel_dp->aux.i2c_nack_count = 0;
> +       intel_dp->aux.i2c_defer_count = 0;
> +
> +       status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
> +       if (status <= 0) {
> +               DRM_DEBUG_KMS("Could not read test request from sink\n");
> +               goto update_status;
> +       }
> +
> +       switch (rxdata) {
> +       case DP_TEST_LINK_TRAINING:
> +               DRM_DEBUG_KMS("LINK_TRAINING test requested\n");
> +               intel_dp->compliance_test_type = DP_TEST_LINK_TRAINING;
> +               response = intel_dp_autotest_link_training(intel_dp);
> +               break;
> +       case DP_TEST_LINK_VIDEO_PATTERN:
> +               DRM_DEBUG_KMS("TEST_PATTERN test requested\n");
> +               intel_dp->compliance_test_type = DP_TEST_LINK_VIDEO_PATTERN;
> +               response = intel_dp_autotest_video_pattern(intel_dp);
> +               break;
> +       case DP_TEST_LINK_EDID_READ:
> +               DRM_DEBUG_KMS("EDID test requested\n");
> +               intel_dp->compliance_test_type = DP_TEST_LINK_EDID_READ;
> +               response = intel_dp_autotest_edid(intel_dp);
> +               break;
> +       case DP_TEST_LINK_PHY_TEST_PATTERN:
> +               DRM_DEBUG_KMS("PHY_PATTERN test requested\n");
> +               intel_dp->compliance_test_type = DP_TEST_LINK_PHY_TEST_PATTERN;
> +               response = intel_dp_autotest_phy_pattern(intel_dp);
> +               break;
> +       default:
> +               DRM_DEBUG_KMS("Invalid test request '%02x'\n", rxdata);
> +               break;
> +       }
> +
> +update_status:
> +       status = drm_dp_dpcd_write(&intel_dp->aux,
> +                                  DP_TEST_RESPONSE,
> +                                  &response, 1);
> +       if (status <= 0)
> +               DRM_DEBUG_KMS("Could not write test response to sink\n");
>  }
>
>  static int
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index eef79cc..e7b62be 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -647,6 +647,10 @@ struct intel_dp {
>                                      bool has_aux_irq,
>                                      int send_bytes,
>                                      uint32_t aux_clock_divider);
> +
> +       /* Displayport compliance testing */
> +       unsigned long compliance_test_type;

Shouldn't this have a default/initialized value? I see we assign
values to it, but then we never assign back to a value that means "not
testing anything". It's hard to see if this is a problem since this
variable is not really used on this patch. Ideally, the definition and
assignments should be placed on the patch that actually uses them
(patch 8).

I also see that on patch 5 you change this to char instead of long,
but you still don't use it for anything... This is a little confusing.

> +       bool compliance_testing_active;

Same thing for this: ideally it should be defined on the patch that
actually does something with the variable.

Also, one variable is compliance_test_ and the other is
compliance_testING_ . It would be nice to keep both in the same
"namespace".

Anyway, the comments above are probably bikesheds. I'll keep
reviewing, so when I reach patch 8 I'll have a clearer view of these
variables, then I can come back to this patch.

>  };
>
>  struct intel_digital_port {
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 1/9] drm/i915: Add automated testing support for Displayport compliance testing

[Todd Previte]

On 4/6/15 5:10 PM, Paulo Zanoni wrote:
> 2015-03-31 14:14 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> Add the skeleton framework for supporting automation for Displayport compliance
>> testing. This patch adds the necessary framework for the source device to
>> appropriately respond to test automation requests from a sink device.
>>
>> V2:
>> - Addressed previous mailing list feedback
>> - Fixed compilation issue (struct members declared in a later patch)
>> - Updated debug messages to be more accurate
>> - Added status checks for the DPCD read/write calls
>> - Removed excess comments and debug messages
>> - Fixed debug message compilation warnings
>> - Fixed compilation issue with missing variables
>> - Updated link training autotest to ACK
>>
>> V3:
>> - Fixed the checks on the DPCD return code to be <= 0
>>    rather than != 0
>> - Removed extraneous assignment of a NAK return code in the
>>    DPCD read failure case
>> - Changed the return in the DPCD read failure case to a goto
>>    to the exit point where the status code is written to the sink
>> - Removed FAUX test case since it's deprecated now
>> - Removed the compliance flag assignment in handle_test_request
>>
>> V4:
>> - Moved declaration of type_type here
>> - Removed declaration of test_data (moved to a later patch)
>> - Added reset to 0 for compliance test variables
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c  | 75 +++++++++++++++++++++++++++++++++++++---
>>   drivers/gpu/drm/i915/intel_drv.h |  4 +++
>>   2 files changed, 75 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index eea9e36..960cc68 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3746,11 +3746,78 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>>          return true;
>>   }
>>
>> -static void
>> -intel_dp_handle_test_request(struct intel_dp *intel_dp)
>> +static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_ACK;
>> +       return test_result;
>> +}
>> +
>> +static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +static uint8_t intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>>   {
>> -       /* NAK by default */
>> -       drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK);
>> +       uint8_t response = DP_TEST_NAK;
>> +       uint8_t rxdata = 0;
>> +       int status = 0;
>> +
>> +       intel_dp->compliance_testing_active = 0;
>> +       intel_dp->aux.i2c_nack_count = 0;
>> +       intel_dp->aux.i2c_defer_count = 0;
>> +
>> +       status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
>> +       if (status <= 0) {
>> +               DRM_DEBUG_KMS("Could not read test request from sink\n");
>> +               goto update_status;
>> +       }
>> +
>> +       switch (rxdata) {
>> +       case DP_TEST_LINK_TRAINING:
>> +               DRM_DEBUG_KMS("LINK_TRAINING test requested\n");
>> +               intel_dp->compliance_test_type = DP_TEST_LINK_TRAINING;
>> +               response = intel_dp_autotest_link_training(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_VIDEO_PATTERN:
>> +               DRM_DEBUG_KMS("TEST_PATTERN test requested\n");
>> +               intel_dp->compliance_test_type = DP_TEST_LINK_VIDEO_PATTERN;
>> +               response = intel_dp_autotest_video_pattern(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_EDID_READ:
>> +               DRM_DEBUG_KMS("EDID test requested\n");
>> +               intel_dp->compliance_test_type = DP_TEST_LINK_EDID_READ;
>> +               response = intel_dp_autotest_edid(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_PHY_TEST_PATTERN:
>> +               DRM_DEBUG_KMS("PHY_PATTERN test requested\n");
>> +               intel_dp->compliance_test_type = DP_TEST_LINK_PHY_TEST_PATTERN;
>> +               response = intel_dp_autotest_phy_pattern(intel_dp);
>> +               break;
>> +       default:
>> +               DRM_DEBUG_KMS("Invalid test request '%02x'\n", rxdata);
>> +               break;
>> +       }
>> +
>> +update_status:
>> +       status = drm_dp_dpcd_write(&intel_dp->aux,
>> +                                  DP_TEST_RESPONSE,
>> +                                  &response, 1);
>> +       if (status <= 0)
>> +               DRM_DEBUG_KMS("Could not write test response to sink\n");
>>   }
>>
>>   static int
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index eef79cc..e7b62be 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -647,6 +647,10 @@ struct intel_dp {
>>                                       bool has_aux_irq,
>>                                       int send_bytes,
>>                                       uint32_t aux_clock_divider);
>> +
>> +       /* Displayport compliance testing */
>> +       unsigned long compliance_test_type;
> Shouldn't this have a default/initialized value? I see we assign
> values to it, but then we never assign back to a value that means "not
> testing anything". It's hard to see if this is a problem since this
> variable is not really used on this patch. Ideally, the definition and
> assignments should be placed on the patch that actually uses them
> (patch 8).
>
> I also see that on patch 5 you change this to char instead of long,
> but you still don't use it for anything... This is a little confusing.
Yeah I was trying to get everything placed correctly but apparently I 
missed on some of the variables. They all should be initialized at the 
top of the handle_test_request function. Each time a request comes in, 
those variables need to be reset so that's why they need to be there. 
I'll work on it more tonight and tomorrow to see if I can get everything 
put in the right place/patch.

>> +       bool compliance_testing_active;
> Same thing for this: ideally it should be defined on the patch that
> actually does something with the variable.
>
> Also, one variable is compliance_test_ and the other is
> compliance_testING_ . It would be nice to keep both in the same
> "namespace".
>
> Anyway, the comments above are probably bikesheds. I'll keep
> reviewing, so when I reach patch 8 I'll have a clearer view of these
> variables, then I can come back to this patch.
Heh it's funny that you pointed that out. I was going to go back and 
change that, since it bugs me that it's inconsistent. I'll fix it for 
the next revision of this patch.
>>   };
>>
>>   struct intel_digital_port {
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

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

Re: [PATCH 1/9] drm/i915: Add automated testing support for Displayport compliance testing

[Todd Previte]

On 4/6/15 5:10 PM, Paulo Zanoni wrote:
> 2015-03-31 14:14 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> Add the skeleton framework for supporting automation for Displayport compliance
>> testing. This patch adds the necessary framework for the source device to
>> appropriately respond to test automation requests from a sink device.
>>
>> V2:
>> - Addressed previous mailing list feedback
>> - Fixed compilation issue (struct members declared in a later patch)
>> - Updated debug messages to be more accurate
>> - Added status checks for the DPCD read/write calls
>> - Removed excess comments and debug messages
>> - Fixed debug message compilation warnings
>> - Fixed compilation issue with missing variables
>> - Updated link training autotest to ACK
>>
>> V3:
>> - Fixed the checks on the DPCD return code to be <= 0
>>    rather than != 0
>> - Removed extraneous assignment of a NAK return code in the
>>    DPCD read failure case
>> - Changed the return in the DPCD read failure case to a goto
>>    to the exit point where the status code is written to the sink
>> - Removed FAUX test case since it's deprecated now
>> - Removed the compliance flag assignment in handle_test_request
>>
>> V4:
>> - Moved declaration of type_type here
>> - Removed declaration of test_data (moved to a later patch)
>> - Added reset to 0 for compliance test variables
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c  | 75 +++++++++++++++++++++++++++++++++++++---
>>   drivers/gpu/drm/i915/intel_drv.h |  4 +++
>>   2 files changed, 75 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index eea9e36..960cc68 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3746,11 +3746,78 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>>          return true;
>>   }
>>
>> -static void
>> -intel_dp_handle_test_request(struct intel_dp *intel_dp)
>> +static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_ACK;
>> +       return test_result;
>> +}
>> +
>> +static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +static uint8_t intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
>> +{
>> +       uint8_t test_result = DP_TEST_NAK;
>> +       return test_result;
>> +}
>> +
>> +static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>>   {
>> -       /* NAK by default */
>> -       drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK);
>> +       uint8_t response = DP_TEST_NAK;
>> +       uint8_t rxdata = 0;
>> +       int status = 0;
>> +
>> +       intel_dp->compliance_testing_active = 0;
>> +       intel_dp->aux.i2c_nack_count = 0;
>> +       intel_dp->aux.i2c_defer_count = 0;
>> +
>> +       status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
>> +       if (status <= 0) {
>> +               DRM_DEBUG_KMS("Could not read test request from sink\n");
>> +               goto update_status;
>> +       }
>> +
>> +       switch (rxdata) {
>> +       case DP_TEST_LINK_TRAINING:
>> +               DRM_DEBUG_KMS("LINK_TRAINING test requested\n");
>> +               intel_dp->compliance_test_type = DP_TEST_LINK_TRAINING;
>> +               response = intel_dp_autotest_link_training(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_VIDEO_PATTERN:
>> +               DRM_DEBUG_KMS("TEST_PATTERN test requested\n");
>> +               intel_dp->compliance_test_type = DP_TEST_LINK_VIDEO_PATTERN;
>> +               response = intel_dp_autotest_video_pattern(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_EDID_READ:
>> +               DRM_DEBUG_KMS("EDID test requested\n");
>> +               intel_dp->compliance_test_type = DP_TEST_LINK_EDID_READ;
>> +               response = intel_dp_autotest_edid(intel_dp);
>> +               break;
>> +       case DP_TEST_LINK_PHY_TEST_PATTERN:
>> +               DRM_DEBUG_KMS("PHY_PATTERN test requested\n");
>> +               intel_dp->compliance_test_type = DP_TEST_LINK_PHY_TEST_PATTERN;
>> +               response = intel_dp_autotest_phy_pattern(intel_dp);
>> +               break;
>> +       default:
>> +               DRM_DEBUG_KMS("Invalid test request '%02x'\n", rxdata);
>> +               break;
>> +       }
>> +
>> +update_status:
>> +       status = drm_dp_dpcd_write(&intel_dp->aux,
>> +                                  DP_TEST_RESPONSE,
>> +                                  &response, 1);
>> +       if (status <= 0)
>> +               DRM_DEBUG_KMS("Could not write test response to sink\n");
>>   }
>>
>>   static int
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index eef79cc..e7b62be 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -647,6 +647,10 @@ struct intel_dp {
>>                                       bool has_aux_irq,
>>                                       int send_bytes,
>>                                       uint32_t aux_clock_divider);
>> +
>> +       /* Displayport compliance testing */
>> +       unsigned long compliance_test_type;
> Shouldn't this have a default/initialized value? I see we assign
> values to it, but then we never assign back to a value that means "not
> testing anything". It's hard to see if this is a problem since this
> variable is not really used on this patch. Ideally, the definition and
> assignments should be placed on the patch that actually uses them
> (patch 8).
I added the initialization on the test_type variable to the top of the 
handler. I think the confusion on its use is because it's actually 
"used", not just set, in the user app for determining how to respond. 
I'll post the user app to the list shortly as well, as that's ready for 
the list now as well.
> I also see that on patch 5 you change this to char instead of long,
> but you still don't use it for anything... This is a little confusing.
>
I just went and looked at this. The patch from format-patch I just 
generated doesn't have the change to char in it. I'll repost that 
version momentarily. Not sure what happened here, but in any case it's 
fixed.
>> +       bool compliance_testing_active;
> Same thing for this: ideally it should be defined on the patch that
> actually does something with the variable.
>
> Also, one variable is compliance_test_ and the other is
> compliance_testING_ . It would be nice to keep both in the same
> "namespace".
I changed the variable name to bring it in line and moved it to the 
patch where it's used. Both patches will be posted shortly.
> Anyway, the comments above are probably bikesheds. I'll keep
> reviewing, so when I reach patch 8 I'll have a clearer view of these
> variables, then I can come back to this patch.
>
>>   };
>>
>>   struct intel_digital_port {
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

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

[PATCH 2/9] drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing

[Todd Previte]

Move the DPCD read to the top and check for an interrupt from the sink to catch
Displayport automated testing requests necessary to support Displayport
compliance testing. The checks for active connectors and link status are moved
below the check for the interrupt.

The main reason for doing this is to make sure that a test request isn't missed.
Checking for the status of the encoder/crtc isn't necessary for some test cases
(AUX channel tests are one example) and without moving the check for the
interrupt, these tests may not execute if one of those checks fails.
Additionally, if reading the DPCD fails, regardless of whether or not testing is
happening, there's no way to train the link since configurations and status
can't be read, nor can link training parameters be written.

Adds a check at the top to verify that the device is connected. This is
necessary for DP compliance testing to ensure that test requests are captured
and acknowledged. If a test request is present during a connected->disconnected
transition, the test code will attempt to execute even though the connection has
been disabled, resulting in a faied test.

This patch is actually both a bug fix and a component of compliance testing.
Because HPD events are received both on connect and disconnect actions, it's
vital that we don't try and train the link when we're transitioning from
connected->disconnected. That results in errors and warning in the logs from
failed AUX transactions and can trigger the WARN for the check of !base.crtc.
By making the check at the beginning to see if the connection is truly active,
those problems are avoided and testing / link training will only be attempted
when there is a valid Displayport connection.

V2:
- N/A
V3:
- Removed extraneous comment
- Added responses to review feedback into the patch notes

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 960cc68..ed2f60c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3895,21 +3895,13 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	if (!intel_encoder->connectors_active)
-		return;
-
-	if (WARN_ON(!intel_encoder->base.crtc))
-		return;
-
-	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
-		return;
-
-	/* Try to read receiver status if the link appears to be up */
-	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+	if (intel_dp->attached_connector->base.status !=
+	    connector_status_connected) {
+		DRM_DEBUG_KMS("Not connected\n");
 		return;
 	}
 
-	/* Now read the DPCD to see if it's actually running */
+	/* Attempt to read the DPCD */
 	if (!intel_dp_get_dpcd(intel_dp)) {
 		return;
 	}
@@ -3921,13 +3913,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 		drm_dp_dpcd_writeb(&intel_dp->aux,
 				   DP_DEVICE_SERVICE_IRQ_VECTOR,
 				   sink_irq_vector);
-
 		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
 			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");
 	}
 
+	if (!intel_encoder->connectors_active)
+		return;
+
+	if (WARN_ON(!intel_encoder->base.crtc))
+		return;
+
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+		return;
+
+	/* Try to read receiver status if the link appears to be up */
+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+		return;
+	}
+
 	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
 		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
 			      intel_encoder->base.name);
-- 
1.9.1

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

[PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Todd Previte]

The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
specifies that repeated AUX transactions after a failure (no response /
invalid response) must have a minimum delay of 400us before the resend can
occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.

V2:
- Changed udelay() to usleep_range()
V3:
- Removed extraneous check for timeout
- Updated comment to reflect this change

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ed2f60c..dc87276 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -877,9 +877,15 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
 				   DP_AUX_CH_CTL_RECEIVE_ERROR);
 
-			if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
-				      DP_AUX_CH_CTL_RECEIVE_ERROR))
+			/* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
+			     400us delay required for errors and timeouts
+			     Timeout errors from the HW already meet this
+			     requirement so skip to next iteration
+			*/
+			if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
+				usleep_range(400, 500);
 				continue;
+			}
 			if (status & DP_AUX_CH_CTL_DONE)
 				break;
 		}
-- 
1.9.1

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

Re: [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Ville Syrjälä]

On Tue, Mar 31, 2015 at 10:15:00AM -0700, Todd Previte wrote:
> The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
> specifies that repeated AUX transactions after a failure (no response /
> invalid response) must have a minimum delay of 400us before the resend can
> occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.
> 
> V2:
> - Changed udelay() to usleep_range()
> V3:
> - Removed extraneous check for timeout
> - Updated comment to reflect this change
> 
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index ed2f60c..dc87276 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -877,9 +877,15 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
>  				   DP_AUX_CH_CTL_RECEIVE_ERROR);
>  
> -			if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
> -				      DP_AUX_CH_CTL_RECEIVE_ERROR))
> +			/* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
> +			     400us delay required for errors and timeouts
> +			     Timeout errors from the HW already meet this
> +			     requirement so skip to next iteration
> +			*/

Weird format for multi line comment.

> +			if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
> +				usleep_range(400, 500);
>  				continue;
> +			}

Where did the DP_AUX_CH_CTL_TIME_OUT_ERROR handling go?

>  			if (status & DP_AUX_CH_CTL_DONE)
>  				break;
>  		}
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Todd Previte]

On 4/1/2015 11:23 AM, Ville Syrjälä wrote:
> On Tue, Mar 31, 2015 at 10:15:00AM -0700, Todd Previte wrote:
>> The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
>> specifies that repeated AUX transactions after a failure (no response /
>> invalid response) must have a minimum delay of 400us before the resend can
>> occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.
>>
>> V2:
>> - Changed udelay() to usleep_range()
>> V3:
>> - Removed extraneous check for timeout
>> - Updated comment to reflect this change
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index ed2f60c..dc87276 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -877,9 +877,15 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>>   				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
>>   				   DP_AUX_CH_CTL_RECEIVE_ERROR);
>>   
>> -			if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> -				      DP_AUX_CH_CTL_RECEIVE_ERROR))
>> +			/* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
>> +			     400us delay required for errors and timeouts
>> +			     Timeout errors from the HW already meet this
>> +			     requirement so skip to next iteration
>> +			*/
> Weird format for multi line comment.
Yeah I had to squish it in there to keep it under 80 columns. Needs the 
'*' on the left side too though. I'll fix that and repost.
>
>> +			if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
>> +				usleep_range(400, 500);
>>   				continue;
>> +			}
> Where did the DP_AUX_CH_CTL_TIME_OUT_ERROR handling go?
As I recall, previous review feedback indicated that the timeout 
condition there was already accounted for.

on 12/15, Paulo commented:

One thing to notice is that if we get a TIME_OUT_ERROR I guess it
means we already waited our standard timeout (which is either 400, 600
or 1600, depending on the platform), so shouldn't we just do the
usleep() after the RECEIVE_ERROR error?


When I checked the BSpec, that seemed to be the case so I removed the 
TIME_OUT_ERROR. Without this
code in place, we still pass the compliance tests for AUX transactions, 
one of which is for a no-reply transaction.
That case specifically should hit the TIME_OUT_ERROR if it was going to 
occur, I would think.

If you can give me a case where that becomes an issue, it's a simple fix 
to add it back in there.

>
>>   			if (status & DP_AUX_CH_CTL_DONE)
>>   				break;
>>   		}
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Ville Syrjälä]

On Wed, Apr 01, 2015 at 11:55:44AM -0700, Todd Previte wrote:
> 
> 
> On 4/1/2015 11:23 AM, Ville Syrjälä wrote:
> > On Tue, Mar 31, 2015 at 10:15:00AM -0700, Todd Previte wrote:
> >> The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
> >> specifies that repeated AUX transactions after a failure (no response /
> >> invalid response) must have a minimum delay of 400us before the resend can
> >> occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.
> >>
> >> V2:
> >> - Changed udelay() to usleep_range()
> >> V3:
> >> - Removed extraneous check for timeout
> >> - Updated comment to reflect this change
> >>
> >> Signed-off-by: Todd Previte <tprevite@gmail.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++--
> >>   1 file changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index ed2f60c..dc87276 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -877,9 +877,15 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >>   				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
> >>   				   DP_AUX_CH_CTL_RECEIVE_ERROR);
> >>   
> >> -			if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
> >> -				      DP_AUX_CH_CTL_RECEIVE_ERROR))
> >> +			/* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
> >> +			     400us delay required for errors and timeouts
> >> +			     Timeout errors from the HW already meet this
> >> +			     requirement so skip to next iteration
> >> +			*/
> > Weird format for multi line comment.
> Yeah I had to squish it in there to keep it under 80 columns. Needs the 
> '*' on the left side too though. I'll fix that and repost.
> >
> >> +			if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
> >> +				usleep_range(400, 500);
> >>   				continue;
> >> +			}
> > Where did the DP_AUX_CH_CTL_TIME_OUT_ERROR handling go?
> As I recall, previous review feedback indicated that the timeout 
> condition there was already accounted for.
> 
> on 12/15, Paulo commented:
> 
> One thing to notice is that if we get a TIME_OUT_ERROR I guess it
> means we already waited our standard timeout (which is either 400, 600
> or 1600, depending on the platform), so shouldn't we just do the
> usleep() after the RECEIVE_ERROR error?
> 
> 
> When I checked the BSpec, that seemed to be the case so I removed the 
> TIME_OUT_ERROR. Without this
> code in place, we still pass the compliance tests for AUX transactions, 
> one of which is for a no-reply transaction.
> That case specifically should hit the TIME_OUT_ERROR if it was going to 
> occur, I would think.
> 
> If you can give me a case where that becomes an issue, it's a simple fix 
> to add it back in there.

Not doing the usleep for the timeout error does seem sane enough to me,
but I didn't actually read through the specs to confirm that.

However my concern is that you no longer check the timeout error bit
at all inside the loop and instead just check the done bit even when the
timeout error may have happened. Now, I'm not sure both bits can actually
be set at the same time, but if they can't the original error check was
entirely redundant. So it would seem safer to keep checking both error
bits before the done bit.

> 
> >
> >>   			if (status & DP_AUX_CH_CTL_DONE)
> >>   				break;
> >>   		}
> >> -- 
> >> 1.9.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Todd Previte]

On 4/1/2015 12:22 PM, Ville Syrjälä wrote:
> On Wed, Apr 01, 2015 at 11:55:44AM -0700, Todd Previte wrote:
>>
>> On 4/1/2015 11:23 AM, Ville Syrjälä wrote:
>>> On Tue, Mar 31, 2015 at 10:15:00AM -0700, Todd Previte wrote:
>>>> The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
>>>> specifies that repeated AUX transactions after a failure (no response /
>>>> invalid response) must have a minimum delay of 400us before the resend can
>>>> occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.
>>>>
>>>> V2:
>>>> - Changed udelay() to usleep_range()
>>>> V3:
>>>> - Removed extraneous check for timeout
>>>> - Updated comment to reflect this change
>>>>
>>>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++--
>>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index ed2f60c..dc87276 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -877,9 +877,15 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>>>>    				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
>>>>    				   DP_AUX_CH_CTL_RECEIVE_ERROR);
>>>>    
>>>> -			if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
>>>> -				      DP_AUX_CH_CTL_RECEIVE_ERROR))
>>>> +			/* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
>>>> +			     400us delay required for errors and timeouts
>>>> +			     Timeout errors from the HW already meet this
>>>> +			     requirement so skip to next iteration
>>>> +			*/
>>> Weird format for multi line comment.
>> Yeah I had to squish it in there to keep it under 80 columns. Needs the
>> '*' on the left side too though. I'll fix that and repost.
>>>> +			if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
>>>> +				usleep_range(400, 500);
>>>>    				continue;
>>>> +			}
>>> Where did the DP_AUX_CH_CTL_TIME_OUT_ERROR handling go?
>> As I recall, previous review feedback indicated that the timeout
>> condition there was already accounted for.
>>
>> on 12/15, Paulo commented:
>>
>> One thing to notice is that if we get a TIME_OUT_ERROR I guess it
>> means we already waited our standard timeout (which is either 400, 600
>> or 1600, depending on the platform), so shouldn't we just do the
>> usleep() after the RECEIVE_ERROR error?
>>
>>
>> When I checked the BSpec, that seemed to be the case so I removed the
>> TIME_OUT_ERROR. Without this
>> code in place, we still pass the compliance tests for AUX transactions,
>> one of which is for a no-reply transaction.
>> That case specifically should hit the TIME_OUT_ERROR if it was going to
>> occur, I would think.
>>
>> If you can give me a case where that becomes an issue, it's a simple fix
>> to add it back in there.
> Not doing the usleep for the timeout error does seem sane enough to me,
> but I didn't actually read through the specs to confirm that.
>
> However my concern is that you no longer check the timeout error bit
> at all inside the loop and instead just check the done bit even when the
> timeout error may have happened. Now, I'm not sure both bits can actually
> be set at the same time, but if they can't the original error check was
> entirely redundant. So it would seem safer to keep checking both error
> bits before the done bit.

While there's no harm in checking the timeout bit here, does it really 
make sense to do so unless we're
going to take action on it? As you said, it seems reasonable enough to 
not wait an addition 400-500us,
so is there something else to do? It may be worth logging the error just 
to make sure there's some
record of when it happens, even if we're not going to do anything else.

As for exclusion between the two bits, the BSpec makes no indication 
that they're exclusive of one another. So
it's entirely possible to have both set.

>>>>    			if (status & DP_AUX_CH_CTL_DONE)
>>>>    				break;
>>>>    		}
>>>> -- 
>>>> 1.9.1
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Paulo Zanoni]

2015-04-01 16:45 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>
>
> On 4/1/2015 12:22 PM, Ville Syrjälä wrote:
>>
>> On Wed, Apr 01, 2015 at 11:55:44AM -0700, Todd Previte wrote:
>>>
>>>
>>> On 4/1/2015 11:23 AM, Ville Syrjälä wrote:
>>>>
>>>> On Tue, Mar 31, 2015 at 10:15:00AM -0700, Todd Previte wrote:
>>>>>
>>>>> The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
>>>>> specifies that repeated AUX transactions after a failure (no response /
>>>>> invalid response) must have a minimum delay of 400us before the resend
>>>>> can
>>>>> occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this
>>>>> specifically.
>>>>>
>>>>> V2:
>>>>> - Changed udelay() to usleep_range()
>>>>> V3:
>>>>> - Removed extraneous check for timeout
>>>>> - Updated comment to reflect this change
>>>>>
>>>>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++--
>>>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>>> index ed2f60c..dc87276 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>> @@ -877,9 +877,15 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>>>>>                                    DP_AUX_CH_CTL_TIME_OUT_ERROR |
>>>>>                                    DP_AUX_CH_CTL_RECEIVE_ERROR);
>>>>>    -                    if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
>>>>> -                                     DP_AUX_CH_CTL_RECEIVE_ERROR))
>>>>> +                       /* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
>>>>> +                            400us delay required for errors and
>>>>> timeouts
>>>>> +                            Timeout errors from the HW already meet
>>>>> this
>>>>> +                            requirement so skip to next iteration
>>>>> +                       */
>>>>
>>>> Weird format for multi line comment.
>>>
>>> Yeah I had to squish it in there to keep it under 80 columns. Needs the
>>> '*' on the left side too though. I'll fix that and repost.
>>>>>
>>>>> +                       if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
>>>>> +                               usleep_range(400, 500);
>>>>>                                 continue;
>>>>> +                       }
>>>>
>>>> Where did the DP_AUX_CH_CTL_TIME_OUT_ERROR handling go?
>>>
>>> As I recall, previous review feedback indicated that the timeout
>>> condition there was already accounted for.
>>>
>>> on 12/15, Paulo commented:
>>>
>>> One thing to notice is that if we get a TIME_OUT_ERROR I guess it
>>> means we already waited our standard timeout (which is either 400, 600
>>> or 1600, depending on the platform), so shouldn't we just do the
>>> usleep() after the RECEIVE_ERROR error?
>>>
>>>
>>> When I checked the BSpec, that seemed to be the case so I removed the
>>> TIME_OUT_ERROR. Without this
>>> code in place, we still pass the compliance tests for AUX transactions,
>>> one of which is for a no-reply transaction.
>>> That case specifically should hit the TIME_OUT_ERROR if it was going to
>>> occur, I would think.
>>>
>>> If you can give me a case where that becomes an issue, it's a simple fix
>>> to add it back in there.
>>
>> Not doing the usleep for the timeout error does seem sane enough to me,
>> but I didn't actually read through the specs to confirm that.
>>
>> However my concern is that you no longer check the timeout error bit
>> at all inside the loop and instead just check the done bit even when the
>> timeout error may have happened. Now, I'm not sure both bits can actually
>> be set at the same time, but if they can't the original error check was
>> entirely redundant. So it would seem safer to keep checking both error
>> bits before the done bit.
>

I agree with Ville here. See below.

>
> While there's no harm in checking the timeout bit here, does it really make
> sense to do so unless we're
> going to take action on it?

Before this patch, we would check the bit and then run "continue",
regardless of the state of DP_AUX_CH_CTL_DONE. With your patch, we
don't check for the _TIME_OUT bit, which means that if both _TIME_OUT
and _CTL_DONE bits are set, we will "break" instead of the old
behavior. I think Ville's point is that we should probably continue
with the old behavior, especially since this patch is just about
adding a new sleep call, and not the specific interaction of _TIME_OUT
and _CTL_DONE.


> As you said, it seems reasonable enough to not
> wait an addition 400-500us,
> so is there something else to do? It may be worth logging the error just to
> make sure there's some
> record of when it happens, even if we're not going to do anything else.

I'm not sure adding a log message here is a good idea: we're probably
going to flood dmesg on a case that is somewhat expected and
recoverable. We already print an error message in case we finish the
loop without _CTL_DONE set, so I think we're covered regarding error
printing already: just run "continue" without logging anything since
we're going to retry anyway.


>
> As for exclusion between the two bits, the BSpec makes no indication that
> they're exclusive of one another. So
> it's entirely possible to have both set.
>
>
>>>>>                         if (status & DP_AUX_CH_CTL_DONE)
>>>>>                                 break;
>>>>>                 }
>>>>> --
>>>>> 1.9.1
>>>>>
>>>>> _______________________________________________
>>>>> Intel-gfx mailing list
>>>>> Intel-gfx@lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Todd Previte]

On 4/6/15 4:28 PM, Paulo Zanoni wrote:
> 2015-04-01 16:45 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>>
>> On 4/1/2015 12:22 PM, Ville Syrjälä wrote:
>>> On Wed, Apr 01, 2015 at 11:55:44AM -0700, Todd Previte wrote:
>>>>
>>>> On 4/1/2015 11:23 AM, Ville Syrjälä wrote:
>>>>> On Tue, Mar 31, 2015 at 10:15:00AM -0700, Todd Previte wrote:
>>>>>> The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
>>>>>> specifies that repeated AUX transactions after a failure (no response /
>>>>>> invalid response) must have a minimum delay of 400us before the resend
>>>>>> can
>>>>>> occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this
>>>>>> specifically.
>>>>>>
>>>>>> V2:
>>>>>> - Changed udelay() to usleep_range()
>>>>>> V3:
>>>>>> - Removed extraneous check for timeout
>>>>>> - Updated comment to reflect this change
>>>>>>
>>>>>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++--
>>>>>>     1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>>>> index ed2f60c..dc87276 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>>> @@ -877,9 +877,15 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>>>>>>                                     DP_AUX_CH_CTL_TIME_OUT_ERROR |
>>>>>>                                     DP_AUX_CH_CTL_RECEIVE_ERROR);
>>>>>>     -                    if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
>>>>>> -                                     DP_AUX_CH_CTL_RECEIVE_ERROR))
>>>>>> +                       /* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
>>>>>> +                            400us delay required for errors and
>>>>>> timeouts
>>>>>> +                            Timeout errors from the HW already meet
>>>>>> this
>>>>>> +                            requirement so skip to next iteration
>>>>>> +                       */
>>>>> Weird format for multi line comment.
>>>> Yeah I had to squish it in there to keep it under 80 columns. Needs the
>>>> '*' on the left side too though. I'll fix that and repost.
>>>>>> +                       if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
>>>>>> +                               usleep_range(400, 500);
>>>>>>                                  continue;
>>>>>> +                       }
>>>>> Where did the DP_AUX_CH_CTL_TIME_OUT_ERROR handling go?
>>>> As I recall, previous review feedback indicated that the timeout
>>>> condition there was already accounted for.
>>>>
>>>> on 12/15, Paulo commented:
>>>>
>>>> One thing to notice is that if we get a TIME_OUT_ERROR I guess it
>>>> means we already waited our standard timeout (which is either 400, 600
>>>> or 1600, depending on the platform), so shouldn't we just do the
>>>> usleep() after the RECEIVE_ERROR error?
>>>>
>>>>
>>>> When I checked the BSpec, that seemed to be the case so I removed the
>>>> TIME_OUT_ERROR. Without this
>>>> code in place, we still pass the compliance tests for AUX transactions,
>>>> one of which is for a no-reply transaction.
>>>> That case specifically should hit the TIME_OUT_ERROR if it was going to
>>>> occur, I would think.
>>>>
>>>> If you can give me a case where that becomes an issue, it's a simple fix
>>>> to add it back in there.
>>> Not doing the usleep for the timeout error does seem sane enough to me,
>>> but I didn't actually read through the specs to confirm that.
>>>
>>> However my concern is that you no longer check the timeout error bit
>>> at all inside the loop and instead just check the done bit even when the
>>> timeout error may have happened. Now, I'm not sure both bits can actually
>>> be set at the same time, but if they can't the original error check was
>>> entirely redundant. So it would seem safer to keep checking both error
>>> bits before the done bit.
> I agree with Ville here. See below.
>
>> While there's no harm in checking the timeout bit here, does it really make
>> sense to do so unless we're
>> going to take action on it?
> Before this patch, we would check the bit and then run "continue",
> regardless of the state of DP_AUX_CH_CTL_DONE. With your patch, we
> don't check for the _TIME_OUT bit, which means that if both _TIME_OUT
> and _CTL_DONE bits are set, we will "break" instead of the old
> behavior. I think Ville's point is that we should probably continue
> with the old behavior, especially since this patch is just about
> adding a new sleep call, and not the specific interaction of _TIME_OUT
> and _CTL_DONE.
I see what you're saying. That was an oversight on my part. Thanks for 
catching it. Updated patch will be here shortly.
>
>> As you said, it seems reasonable enough to not
>> wait an addition 400-500us,
>> so is there something else to do? It may be worth logging the error just to
>> make sure there's some
>> record of when it happens, even if we're not going to do anything else.
> I'm not sure adding a log message here is a good idea: we're probably
> going to flood dmesg on a case that is somewhat expected and
> recoverable. We already print an error message in case we finish the
> loop without _CTL_DONE set, so I think we're covered regarding error
> printing already: just run "continue" without logging anything since
> we're going to retry anyway.
Fair enough, although I'm not sure I'd agree that it's a somewhat 
expected case. It's certainly recoverable though. I'll remove the 
message to avoid possible log spam.
>
>> As for exclusion between the two bits, the BSpec makes no indication that
>> they're exclusive of one another. So
>> it's entirely possible to have both set.
>>
>>
>>>>>>                          if (status & DP_AUX_CH_CTL_DONE)
>>>>>>                                  break;
>>>>>>                  }
>>>>>> --
>>>>>> 1.9.1
>>>>>>
>>>>>> _______________________________________________
>>>>>> Intel-gfx mailing list
>>>>>> Intel-gfx@lists.freedesktop.org
>>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

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

[PATCH 03/11] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Todd Previte]

The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
specifies that repeated AUX transactions after a failure (no response /
invalid response) must have a minimum delay of 400us before the resend can
occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.

Also, the check for DP_AUX_CH_CTL_TIME_OUT_ERROR has been moved out into a
separate case. As of now, the only action taken is to log the error, since
the HW will have already waited the required amount of time for the
transaction to complete.

V2:
- Changed udelay() to usleep_range()
V3:
- Removed extraneous check for timeout
- Updated comment to reflect this change
V4:
- Reformatted a comment
V5:
- Added separate check for HW timeout on AUX transactions. A message
  is logged upon detection of this case.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ed2f60c..7302ff3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -877,9 +877,19 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
 				   DP_AUX_CH_CTL_RECEIVE_ERROR);
 
-			if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
-				      DP_AUX_CH_CTL_RECEIVE_ERROR))
+			if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR) {
+				DRM_DEBUG_DRIVER("HW timeout detected on AUX transaction\n");
+			}
+
+			/* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
+			 *   400us delay required for errors and timeouts
+			 *   Timeout errors from the HW already meet this
+			 *   requirement so skip to next iteration
+			 */
+			if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
+				usleep_range(400, 500);
 				continue;
+			}
 			if (status & DP_AUX_CH_CTL_DONE)
 				break;
 		}
-- 
1.9.1

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

[PATCH 03/11] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Todd Previte]

The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
specifies that repeated AUX transactions after a failure (no response /
invalid response) must have a minimum delay of 400us before the resend can
occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.

Also, the check for DP_AUX_CH_CTL_TIME_OUT_ERROR has been moved out into a
separate case. As of now, the only action taken is to log the error, since
the HW will have already waited the required amount of time for the
transaction to complete.

V2:
- Changed udelay() to usleep_range()
V3:
- Removed extraneous check for timeout
- Updated comment to reflect this change
V4:
- Reformatted a comment
V5:
- Added separate check for HW timeout on AUX transactions. A message
  is logged upon detection of this case.
V6:
- Add continue statement to HW timeout detect case
- Remove the log message indicating a timeout has been
  detected (review feedback)

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ed2f60c..8b59458 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -877,9 +877,18 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
 				   DP_AUX_CH_CTL_RECEIVE_ERROR);
 
-			if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
-				      DP_AUX_CH_CTL_RECEIVE_ERROR))
+			if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR)
 				continue;
+
+			/* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
+			 *   400us delay required for errors and timeouts
+			 *   Timeout errors from the HW already meet this
+			 *   requirement so skip to next iteration
+			 */
+			if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
+				usleep_range(400, 500);
+				continue;
+			}
 			if (status & DP_AUX_CH_CTL_DONE)
 				break;
 		}
-- 
1.9.1

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

Re: [PATCH 03/11] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Paulo Zanoni]

2015-04-06 23:09 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
> specifies that repeated AUX transactions after a failure (no response /
> invalid response) must have a minimum delay of 400us before the resend can
> occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.
>
> Also, the check for DP_AUX_CH_CTL_TIME_OUT_ERROR has been moved out into a
> separate case. As of now, the only action taken is to log the error, since
> the HW will have already waited the required amount of time for the
> transaction to complete.

As of V6, this paragraph is no longer true. We could just remove it
while applying.

With that: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>.

>
> V2:
> - Changed udelay() to usleep_range()
> V3:
> - Removed extraneous check for timeout
> - Updated comment to reflect this change
> V4:
> - Reformatted a comment
> V5:
> - Added separate check for HW timeout on AUX transactions. A message
>   is logged upon detection of this case.
> V6:
> - Add continue statement to HW timeout detect case
> - Remove the log message indicating a timeout has been
>   detected (review feedback)
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index ed2f60c..8b59458 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -877,9 +877,18 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>                                    DP_AUX_CH_CTL_TIME_OUT_ERROR |
>                                    DP_AUX_CH_CTL_RECEIVE_ERROR);
>
> -                       if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
> -                                     DP_AUX_CH_CTL_RECEIVE_ERROR))
> +                       if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR)
>                                 continue;
> +
> +                       /* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
> +                        *   400us delay required for errors and timeouts
> +                        *   Timeout errors from the HW already meet this
> +                        *   requirement so skip to next iteration
> +                        */
> +                       if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
> +                               usleep_range(400, 500);
> +                               continue;
> +                       }
>                         if (status & DP_AUX_CH_CTL_DONE)
>                                 break;
>                 }
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 03/11] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Todd Previte]

Fixed the commit message. That will be in V5 of the patch set to be 
posted today.

On 4/7/2015 6:57 AM, Paulo Zanoni wrote:
> 2015-04-06 23:09 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
>> specifies that repeated AUX transactions after a failure (no response /
>> invalid response) must have a minimum delay of 400us before the resend can
>> occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.
>>
>> Also, the check for DP_AUX_CH_CTL_TIME_OUT_ERROR has been moved out into a
>> separate case. As of now, the only action taken is to log the error, since
>> the HW will have already waited the required amount of time for the
>> transaction to complete.
> As of V6, this paragraph is no longer true. We could just remove it
> while applying.
>
> With that: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>.
>
>> V2:
>> - Changed udelay() to usleep_range()
>> V3:
>> - Removed extraneous check for timeout
>> - Updated comment to reflect this change
>> V4:
>> - Reformatted a comment
>> V5:
>> - Added separate check for HW timeout on AUX transactions. A message
>>    is logged upon detection of this case.
>> V6:
>> - Add continue statement to HW timeout detect case
>> - Remove the log message indicating a timeout has been
>>    detected (review feedback)
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index ed2f60c..8b59458 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -877,9 +877,18 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>>                                     DP_AUX_CH_CTL_TIME_OUT_ERROR |
>>                                     DP_AUX_CH_CTL_RECEIVE_ERROR);
>>
>> -                       if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> -                                     DP_AUX_CH_CTL_RECEIVE_ERROR))
>> +                       if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR)
>>                                  continue;
>> +
>> +                       /* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
>> +                        *   400us delay required for errors and timeouts
>> +                        *   Timeout errors from the HW already meet this
>> +                        *   requirement so skip to next iteration
>> +                        */
>> +                       if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
>> +                               usleep_range(400, 500);
>> +                               continue;
>> +                       }
>>                          if (status & DP_AUX_CH_CTL_DONE)
>>                                  break;
>>                  }
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

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

[PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing

[Todd Previte]

Displayport compliance test 4.2.2.6 requires that a source device be capable of detecting
a corrupt EDID. To do this, the test sets up an invalid EDID header to be read by the source
device. Unfortunately, the DRM EDID reading and parsing functions are actually too good in
this case and prevent the source from reading the corrupted EDID. The result is a failed
compliance test.

In order to successfully pass the test, the raw EDID header must be checked on each read
to see if has been "corrupted". If an invalid raw header is detected, a flag is set that
allows the compliance testing code to acknowledge that fact and react appropriately. The
flag is automatically cleared on read.

This code is designed to expressly work for compliance testing without disrupting normal
operations for EDID reading and parsing.

Signed-off-by: Todd Previte <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_edid.c       | 33 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 include/drm/drm_edid.h           |  5 +++++
 4 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 53bc7a6..3d4f473 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -990,6 +990,32 @@ static const u8 edid_header[] = {
 	0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
 };
 
+
+/* Flag for EDID corruption testing
+ * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+ */
+static bool raw_edid_header_corrupted;
+
+/**
+ * drm_raw_edid_header_valid - check to see if the raw header is
+ * corrupt or not. Used solely for Displayport compliance
+ * testing and required by Link CTS Core 1.2 rev1.1 4.2.2.6.
+ * @raw_edid: pointer to raw base EDID block
+ *
+ * Indicates whether the original EDID header as read from the
+ * device was corrupt or not. Clears on read.
+ *
+ * Return: true if the raw header was corrupt, otherwise false
+ */
+bool drm_raw_edid_header_corrupt(void)
+{
+	bool corrupted = raw_edid_header_corrupted;
+
+	raw_edid_header_corrupted = 0;
+	return corrupted;
+}
+EXPORT_SYMBOL(drm_raw_edid_header_corrupt);
+
 /**
  * drm_edid_header_is_valid - sanity check the header of the base EDID block
  * @raw_edid: pointer to raw base EDID block
@@ -1006,6 +1032,13 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
 		if (raw_edid[i] == edid_header[i])
 			score++;
 
+	if (score != 8) {
+		/* Log and set flag here for EDID corruption testing
+		 * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+		 */
+		DRM_DEBUG_DRIVER("Raw EDID header invalid\n");
+		raw_edid_header_corrupted = 1;
+	}
 	return score;
 }
 EXPORT_SYMBOL(drm_edid_header_is_valid);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index dc87276..57f8e43 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3824,6 +3824,9 @@ update_status:
 				   &response, 1);
 	if (status <= 0)
 		DRM_DEBUG_KMS("Could not write test response to sink\n");
+
+	/* Clear flag here, after testing is complete*/
+	intel_dp->compliance_edid_invalid = 0;
 }
 
 static int
@@ -3896,6 +3899,10 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
+	struct drm_connector *connector = &intel_dp->attached_connector->base;
+	struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+	struct edid *edid_read = NULL;
+
 	u8 sink_irq_vector;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
@@ -3912,6 +3919,16 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 		return;
 	}
 
+	/* Compliance testing requires an EDID read for all HPD events
+	 * Link CTS Core 1.2 rev 1.1: Test 4.2.2.1
+	 * Flag set here will be handled in the EDID test function
+	 */
+	edid_read = drm_get_edid(connector, adapter);
+	if (!edid_read || drm_raw_edid_header_corrupt() == 1) {
+		DRM_DEBUG_DRIVER("EDID invalid, setting flag\n");
+		intel_dp->compliance_edid_invalid = 1;
+	}
+
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
 	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e7b62be..42e4251 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -651,6 +651,7 @@ struct intel_dp {
 	/* Displayport compliance testing */
 	unsigned long compliance_test_type;
 	bool compliance_testing_active;
+	bool compliance_edid_invalid;
 };
 
 struct intel_digital_port {
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 87d85e8..8a7eb22 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -388,4 +388,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 			      size_t len),
 	void *data);
 
+/* Check for corruption in raw EDID header - Displayport compliance
+  * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
+ */
+bool drm_raw_edid_header_corrupt(void);
+
 #endif /* __DRM_EDID_H__ */
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing

[Paulo Zanoni]

2015-03-31 14:15 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> Displayport compliance test 4.2.2.6 requires that a source device be capable of detecting
> a corrupt EDID. To do this, the test sets up an invalid EDID header to be read by the source
> device. Unfortunately, the DRM EDID reading and parsing functions are actually too good in
> this case and prevent the source from reading the corrupted EDID. The result is a failed
> compliance test.
>
> In order to successfully pass the test, the raw EDID header must be checked on each read
> to see if has been "corrupted". If an invalid raw header is detected, a flag is set that
> allows the compliance testing code to acknowledge that fact and react appropriately. The
> flag is automatically cleared on read.
>
> This code is designed to expressly work for compliance testing without disrupting normal
> operations for EDID reading and parsing.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_edid.c       | 33 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  include/drm/drm_edid.h           |  5 +++++
>  4 files changed, 56 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 53bc7a6..3d4f473 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -990,6 +990,32 @@ static const u8 edid_header[] = {
>         0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
>  };
>
> +
> +/* Flag for EDID corruption testing
> + * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
> + */
> +static bool raw_edid_header_corrupted;

A static variable like this is not a good design, especially for a
module like drm.ko. If you really need this, please store it inside
some struct. But see below first.


> +
> +/**
> + * drm_raw_edid_header_valid - check to see if the raw header is
> + * corrupt or not. Used solely for Displayport compliance
> + * testing and required by Link CTS Core 1.2 rev1.1 4.2.2.6.
> + * @raw_edid: pointer to raw base EDID block
> + *
> + * Indicates whether the original EDID header as read from the
> + * device was corrupt or not. Clears on read.
> + *
> + * Return: true if the raw header was corrupt, otherwise false
> + */
> +bool drm_raw_edid_header_corrupt(void)
> +{
> +       bool corrupted = raw_edid_header_corrupted;
> +
> +       raw_edid_header_corrupted = 0;
> +       return corrupted;
> +}
> +EXPORT_SYMBOL(drm_raw_edid_header_corrupt);
> +
>  /**
>   * drm_edid_header_is_valid - sanity check the header of the base EDID block
>   * @raw_edid: pointer to raw base EDID block
> @@ -1006,6 +1032,13 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
>                 if (raw_edid[i] == edid_header[i])
>                         score++;
>
> +       if (score != 8) {
> +               /* Log and set flag here for EDID corruption testing
> +                * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
> +                */
> +               DRM_DEBUG_DRIVER("Raw EDID header invalid\n");
> +               raw_edid_header_corrupted = 1;
> +       }

The problem is that here we're limiting ourselves to just a bad edid
header, not a bad edid in general, so there are many things which we
might not get - such as a simple wrong checksum edid value. I remember
that on the previous patch you calculated the whole checksum manually,
but I don't see that code anymore. What was the reason for the change?

Also, while reviewing the patch I just discovered
connector->bad_edid_counter. Can't we just use it instead of this
patch? I mean: grab the current counter, check edid, see if the
counter moved.

>         return score;
>  }
>  EXPORT_SYMBOL(drm_edid_header_is_valid);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index dc87276..57f8e43 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3824,6 +3824,9 @@ update_status:
>                                    &response, 1);
>         if (status <= 0)
>                 DRM_DEBUG_KMS("Could not write test response to sink\n");
> +
> +       /* Clear flag here, after testing is complete*/
> +       intel_dp->compliance_edid_invalid = 0;
>  }
>
>  static int
> @@ -3896,6 +3899,10 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  {
>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
>         struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> +       struct drm_connector *connector = &intel_dp->attached_connector->base;
> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
> +       struct edid *edid_read = NULL;
> +
>         u8 sink_irq_vector;
>         u8 link_status[DP_LINK_STATUS_SIZE];
>
> @@ -3912,6 +3919,16 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>                 return;
>         }
>
> +       /* Compliance testing requires an EDID read for all HPD events
> +        * Link CTS Core 1.2 rev 1.1: Test 4.2.2.1
> +        * Flag set here will be handled in the EDID test function
> +        */
> +       edid_read = drm_get_edid(connector, adapter);
> +       if (!edid_read || drm_raw_edid_header_corrupt() == 1) {
> +               DRM_DEBUG_DRIVER("EDID invalid, setting flag\n");
> +               intel_dp->compliance_edid_invalid = 1;
> +       }

I see that on the next patch you also add a drm_get_edid() call, so we
have apparently added 2 calls for the edid test. Do we really need
both? Why is this one needed? Why is that one needed?

Also, some more ideas:

I also thought that we already automatically issued get_edid() calls
on the normal hotplug code path, so it would be a "third" call on the
codepath for the test. Can't we just rely on this one?

Another idea would be: instead of getting the edid from inside the
Kernel, we could try to get it from the user-space, using the
GetResources/GetConnector IOCTLs, and also maybe look at the EDID
properties to possibly validate the EDID (in case that edid did not
get "fixed" by the Kernel). The nice thing about this is that it would
make the test be more like a real driver usage. Do you see any
possible problems with this approach?

> +
>         /* Try to read the source of the interrupt */
>         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>             intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e7b62be..42e4251 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -651,6 +651,7 @@ struct intel_dp {
>         /* Displayport compliance testing */
>         unsigned long compliance_test_type;
>         bool compliance_testing_active;
> +       bool compliance_edid_invalid;
>  };
>
>  struct intel_digital_port {
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 87d85e8..8a7eb22 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -388,4 +388,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>                               size_t len),
>         void *data);
>
> +/* Check for corruption in raw EDID header - Displayport compliance
> +  * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
> + */
> +bool drm_raw_edid_header_corrupt(void);
> +
>  #endif /* __DRM_EDID_H__ */
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing

[Todd Previte]

On 4/8/2015 9:51 AM, Paulo Zanoni wrote:
> 2015-03-31 14:15 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> Displayport compliance test 4.2.2.6 requires that a source device be capable of detecting
>> a corrupt EDID. To do this, the test sets up an invalid EDID header to be read by the source
>> device. Unfortunately, the DRM EDID reading and parsing functions are actually too good in
>> this case and prevent the source from reading the corrupted EDID. The result is a failed
>> compliance test.
>>
>> In order to successfully pass the test, the raw EDID header must be checked on each read
>> to see if has been "corrupted". If an invalid raw header is detected, a flag is set that
>> allows the compliance testing code to acknowledge that fact and react appropriately. The
>> flag is automatically cleared on read.
>>
>> This code is designed to expressly work for compliance testing without disrupting normal
>> operations for EDID reading and parsing.
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> Cc: dri-devel@lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/drm_edid.c       | 33 +++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h |  1 +
>>   include/drm/drm_edid.h           |  5 +++++
>>   4 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 53bc7a6..3d4f473 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -990,6 +990,32 @@ static const u8 edid_header[] = {
>>          0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
>>   };
>>
>> +
>> +/* Flag for EDID corruption testing
>> + * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>> + */
>> +static bool raw_edid_header_corrupted;
> A static variable like this is not a good design, especially for a
> module like drm.ko. If you really need this, please store it inside
> some struct. But see below first.
Per our discussion this morning, I concur. This has been removed in 
favor of a different solution that uses a new boolean flag in the 
drm_connector struct.

Capturing more of the discussion here, the static boolean was a bad idea 
to begin with and needed to be removed. One solution was to make the 
flag non-static and non-clear-on-read, then add a separate clear() 
function. But it still had the problem of potential misuse other places 
in the code. The current solution (which will be posted with V5) 
modifies the is_valid() function and adds a flag in the drm_connector 
struct that can be used to detect this low-level header corruption.

>
>> +
>> +/**
>> + * drm_raw_edid_header_valid - check to see if the raw header is
>> + * corrupt or not. Used solely for Displayport compliance
>> + * testing and required by Link CTS Core 1.2 rev1.1 4.2.2.6.
>> + * @raw_edid: pointer to raw base EDID block
>> + *
>> + * Indicates whether the original EDID header as read from the
>> + * device was corrupt or not. Clears on read.
>> + *
>> + * Return: true if the raw header was corrupt, otherwise false
>> + */
>> +bool drm_raw_edid_header_corrupt(void)
>> +{
>> +       bool corrupted = raw_edid_header_corrupted;
>> +
>> +       raw_edid_header_corrupted = 0;
>> +       return corrupted;
>> +}
>> +EXPORT_SYMBOL(drm_raw_edid_header_corrupt);
>> +
>>   /**
>>    * drm_edid_header_is_valid - sanity check the header of the base EDID block
>>    * @raw_edid: pointer to raw base EDID block
>> @@ -1006,6 +1032,13 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
>>                  if (raw_edid[i] == edid_header[i])
>>                          score++;
>>
>> +       if (score != 8) {
>> +               /* Log and set flag here for EDID corruption testing
>> +                * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>> +                */
>> +               DRM_DEBUG_DRIVER("Raw EDID header invalid\n");
>> +               raw_edid_header_corrupted = 1;
>> +       }
> The problem is that here we're limiting ourselves to just a bad edid
> header, not a bad edid in general, so there are many things which we
> might not get - such as a simple wrong checksum edid value. I remember
> that on the previous patch you calculated the whole checksum manually,
> but I don't see that code anymore. What was the reason for the change?
So this code is specifically for the 4.2.2.6 compliance test that is 
looking for nothing more than an invalid EDID header. In fact, the test 
unit only sets that header as invalid once, so if you miss it on the 
first read, you can't go back and check it again later - the test will 
now fail. So catching the general case isn't really what this is about - 
it's about being able to detect a corrupt EDID header even if it only 
happens once.

Honestly, the DRM EDID code is VERY good about catching corruption cases 
and in the case of corrupted headers, fixing them and moving on. I had 
to tie into it at a fairly low level in order to catch the invalid 
header before the code fixed it.

With respect to the checksum code, for quite a while the checksum 
computation was incorrect in the DRM code. Somewhere along in November 
of last year or 2013 (I remember the month, not the year, go figure) 
someone came along and added a checksum computation that actually 
worked. So that rendered that original code I wrote unnecessary.

> Also, while reviewing the patch I just discovered
> connector->bad_edid_counter. Can't we just use it instead of this
> patch? I mean: grab the current counter, check edid, see if the
> counter moved.
I think the above description highlights why using this counter really 
isn't an option. Since the code only gets one shot at catching that 
invalid header, it's essential to make sure it's captured specifically. 
Comparing before and after values of this counter doesn't specifically 
say that the header was invalid, only that SOMEthing in the EDID was 
invalid.
>>          return score;
>>   }
>>   EXPORT_SYMBOL(drm_edid_header_is_valid);
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index dc87276..57f8e43 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3824,6 +3824,9 @@ update_status:
>>                                     &response, 1);
>>          if (status <= 0)
>>                  DRM_DEBUG_KMS("Could not write test response to sink\n");
>> +
>> +       /* Clear flag here, after testing is complete*/
>> +       intel_dp->compliance_edid_invalid = 0;
>>   }
>>
>>   static int
>> @@ -3896,6 +3899,10 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   {
>>          struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>          struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>> +       struct drm_connector *connector = &intel_dp->attached_connector->base;
>> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
>> +       struct edid *edid_read = NULL;
>> +
>>          u8 sink_irq_vector;
>>          u8 link_status[DP_LINK_STATUS_SIZE];
>>
>> @@ -3912,6 +3919,16 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>                  return;
>>          }
>>
>> +       /* Compliance testing requires an EDID read for all HPD events
>> +        * Link CTS Core 1.2 rev 1.1: Test 4.2.2.1
>> +        * Flag set here will be handled in the EDID test function
>> +        */
>> +       edid_read = drm_get_edid(connector, adapter);
>> +       if (!edid_read || drm_raw_edid_header_corrupt() == 1) {
>> +               DRM_DEBUG_DRIVER("EDID invalid, setting flag\n");
>> +               intel_dp->compliance_edid_invalid = 1;
>> +       }
> I see that on the next patch you also add a drm_get_edid() call, so we
> have apparently added 2 calls for the edid test. Do we really need
> both? Why is this one needed? Why is that one needed?
So there's two issues here - first is the same one mentioned above, 
catching that single instance of a corrupted EDID header. The second is 
that the checksum from the test device differs between the two reads. If 
you remove either one of them, one test or the other will fail.

> Also, some more ideas:
>
> I also thought that we already automatically issued get_edid() calls
> on the normal hotplug code path, so it would be a "third" call on the
> codepath for the test. Can't we just rely on this one?
Same issue as above.
>
> Another idea would be: instead of getting the edid from inside the
> Kernel, we could try to get it from the user-space, using the
> GetResources/GetConnector IOCTLs, and also maybe look at the EDID
> properties to possibly validate the EDID (in case that edid did not
> get "fixed" by the Kernel). The nice thing about this is that it would
> make the test be more like a real driver usage. Do you see any
> possible problems with this approach?
I don't really see this as a valid option in light of the descriptions 
I've given above. This has a good chance of introducing latency problems 
which may adversely affect the tests as well.

>> +
>>          /* Try to read the source of the interrupt */
>>          if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>              intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index e7b62be..42e4251 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -651,6 +651,7 @@ struct intel_dp {
>>          /* Displayport compliance testing */
>>          unsigned long compliance_test_type;
>>          bool compliance_testing_active;
>> +       bool compliance_edid_invalid;
>>   };
>>
>>   struct intel_digital_port {
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 87d85e8..8a7eb22 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -388,4 +388,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>>                                size_t len),
>>          void *data);
>>
>> +/* Check for corruption in raw EDID header - Displayport compliance
>> +  * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>> + */
>> +bool drm_raw_edid_header_corrupt(void);
>> +
>>   #endif /* __DRM_EDID_H__ */
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

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

Re: [PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing

[Paulo Zanoni]

2015-04-08 18:43 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>
>
> On 4/8/2015 9:51 AM, Paulo Zanoni wrote:
>>
>> 2015-03-31 14:15 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>>>
>>> Displayport compliance test 4.2.2.6 requires that a source device be
>>> capable of detecting
>>> a corrupt EDID. To do this, the test sets up an invalid EDID header to be
>>> read by the source
>>> device. Unfortunately, the DRM EDID reading and parsing functions are
>>> actually too good in
>>> this case and prevent the source from reading the corrupted EDID. The
>>> result is a failed
>>> compliance test.
>>>
>>> In order to successfully pass the test, the raw EDID header must be
>>> checked on each read
>>> to see if has been "corrupted". If an invalid raw header is detected, a
>>> flag is set that
>>> allows the compliance testing code to acknowledge that fact and react
>>> appropriately. The
>>> flag is automatically cleared on read.
>>>
>>> This code is designed to expressly work for compliance testing without
>>> disrupting normal
>>> operations for EDID reading and parsing.
>>>
>>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>>> Cc: dri-devel@lists.freedesktop.org
>>> ---
>>>   drivers/gpu/drm/drm_edid.c       | 33 +++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
>>>   drivers/gpu/drm/i915/intel_drv.h |  1 +
>>>   include/drm/drm_edid.h           |  5 +++++
>>>   4 files changed, 56 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>> index 53bc7a6..3d4f473 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -990,6 +990,32 @@ static const u8 edid_header[] = {
>>>          0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
>>>   };
>>>
>>> +
>>> +/* Flag for EDID corruption testing
>>> + * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>>> + */
>>> +static bool raw_edid_header_corrupted;
>>
>> A static variable like this is not a good design, especially for a
>> module like drm.ko. If you really need this, please store it inside
>> some struct. But see below first.
>
> Per our discussion this morning, I concur. This has been removed in favor of
> a different solution that uses a new boolean flag in the drm_connector
> struct.
>
> Capturing more of the discussion here, the static boolean was a bad idea to
> begin with and needed to be removed. One solution was to make the flag
> non-static and non-clear-on-read, then add a separate clear() function. But
> it still had the problem of potential misuse other places in the code. The
> current solution (which will be posted with V5) modifies the is_valid()
> function and adds a flag in the drm_connector struct that can be used to
> detect this low-level header corruption.
>
>
>>
>>> +
>>> +/**
>>> + * drm_raw_edid_header_valid - check to see if the raw header is
>>> + * corrupt or not. Used solely for Displayport compliance
>>> + * testing and required by Link CTS Core 1.2 rev1.1 4.2.2.6.
>>> + * @raw_edid: pointer to raw base EDID block
>>> + *
>>> + * Indicates whether the original EDID header as read from the
>>> + * device was corrupt or not. Clears on read.
>>> + *
>>> + * Return: true if the raw header was corrupt, otherwise false
>>> + */
>>> +bool drm_raw_edid_header_corrupt(void)
>>> +{
>>> +       bool corrupted = raw_edid_header_corrupted;
>>> +
>>> +       raw_edid_header_corrupted = 0;
>>> +       return corrupted;
>>> +}
>>> +EXPORT_SYMBOL(drm_raw_edid_header_corrupt);
>>> +
>>>   /**
>>>    * drm_edid_header_is_valid - sanity check the header of the base EDID
>>> block
>>>    * @raw_edid: pointer to raw base EDID block
>>> @@ -1006,6 +1032,13 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
>>>                  if (raw_edid[i] == edid_header[i])
>>>                          score++;
>>>
>>> +       if (score != 8) {
>>> +               /* Log and set flag here for EDID corruption testing
>>> +                * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>>> +                */
>>> +               DRM_DEBUG_DRIVER("Raw EDID header invalid\n");
>>> +               raw_edid_header_corrupted = 1;
>>> +       }
>>
>> The problem is that here we're limiting ourselves to just a bad edid
>> header, not a bad edid in general, so there are many things which we
>> might not get - such as a simple wrong checksum edid value. I remember
>> that on the previous patch you calculated the whole checksum manually,
>> but I don't see that code anymore. What was the reason for the change?
>
> So this code is specifically for the 4.2.2.6 compliance test that is looking
> for nothing more than an invalid EDID header.

On the version of the spec I have (1.2 Core, Aug 22 2011), 4.2.2.6 is
"EDID Corruption Detection", and it mentions "EDID corruption" without
really getting into the details of header corruption. On the "Test
procedure" description, it mentions "Reference Sink sets up EDID with
incorrect checksum", which we don't check. Of course, changing the
header may produce an incorrect checksum, but maybe the wrong header
is just a particular detail of the compliance testing device you have,
while others could potentially have other forms of corruption, such as
just a bad checksum?

In the paragraphs below you elaborate even more on the assumption of a
bad header instead of just a bad checksum, so maybe we have different
versions of the spec? (I still remember when I used version 1.0 of a
certain non-backwards-compatible spec to review a patch made against
version 0.8 of the same spec)

> In fact, the test unit only
> sets that header as invalid once, so if you miss it on the first read, you
> can't go back and check it again later - the test will now fail. So catching
> the general case isn't really what this is about - it's about being able to
> detect a corrupt EDID header even if it only happens once.
>
> Honestly, the DRM EDID code is VERY good about catching corruption cases and
> in the case of corrupted headers, fixing them and moving on. I had to tie
> into it at a fairly low level in order to catch the invalid header before
> the code fixed it.
>
> With respect to the checksum code, for quite a while the checksum
> computation was incorrect in the DRM code. Somewhere along in November of
> last year or 2013 (I remember the month, not the year, go figure) someone
> came along and added a checksum computation that actually worked. So that
> rendered that original code I wrote unnecessary.
>
>> Also, while reviewing the patch I just discovered
>> connector->bad_edid_counter. Can't we just use it instead of this
>> patch? I mean: grab the current counter, check edid, see if the
>> counter moved.
>
> I think the above description highlights why using this counter really isn't
> an option. Since the code only gets one shot at catching that invalid
> header, it's essential to make sure it's captured specifically. Comparing
> before and after values of this counter doesn't specifically say that the
> header was invalid, only that SOMEthing in the EDID was invalid.

Which is, according to the way I read the spec, not a problem.


>
>>>          return score;
>>>   }
>>>   EXPORT_SYMBOL(drm_edid_header_is_valid);
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index dc87276..57f8e43 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -3824,6 +3824,9 @@ update_status:
>>>                                     &response, 1);
>>>          if (status <= 0)
>>>                  DRM_DEBUG_KMS("Could not write test response to
>>> sink\n");
>>> +
>>> +       /* Clear flag here, after testing is complete*/
>>> +       intel_dp->compliance_edid_invalid = 0;
>>>   }
>>>
>>>   static int
>>> @@ -3896,6 +3899,10 @@ intel_dp_check_link_status(struct intel_dp
>>> *intel_dp)
>>>   {
>>>          struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>          struct intel_encoder *intel_encoder =
>>> &dp_to_dig_port(intel_dp)->base;
>>> +       struct drm_connector *connector =
>>> &intel_dp->attached_connector->base;
>>> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
>>> +       struct edid *edid_read = NULL;
>>> +
>>>          u8 sink_irq_vector;
>>>          u8 link_status[DP_LINK_STATUS_SIZE];
>>>
>>> @@ -3912,6 +3919,16 @@ intel_dp_check_link_status(struct intel_dp
>>> *intel_dp)
>>>                  return;
>>>          }
>>>
>>> +       /* Compliance testing requires an EDID read for all HPD events
>>> +        * Link CTS Core 1.2 rev 1.1: Test 4.2.2.1
>>> +        * Flag set here will be handled in the EDID test function
>>> +        */
>>> +       edid_read = drm_get_edid(connector, adapter);
>>> +       if (!edid_read || drm_raw_edid_header_corrupt() == 1) {
>>> +               DRM_DEBUG_DRIVER("EDID invalid, setting flag\n");
>>> +               intel_dp->compliance_edid_invalid = 1;
>>> +       }
>>
>> I see that on the next patch you also add a drm_get_edid() call, so we
>> have apparently added 2 calls for the edid test. Do we really need
>> both? Why is this one needed? Why is that one needed?
>
> So there's two issues here - first is the same one mentioned above, catching
> that single instance of a corrupted EDID header. The second is that the
> checksum from the test device differs between the two reads. If you remove
> either one of them, one test or the other will fail.

But then why not keep both at the same place? The one here is going to
affect a lot more than just compliance testing, while the other is
contained to DP compliance code.


>
>> Also, some more ideas:
>>
>> I also thought that we already automatically issued get_edid() calls
>> on the normal hotplug code path, so it would be a "third" call on the
>> codepath for the test. Can't we just rely on this one?
>
> Same issue as above.
>>
>>
>> Another idea would be: instead of getting the edid from inside the
>> Kernel, we could try to get it from the user-space, using the
>> GetResources/GetConnector IOCTLs, and also maybe look at the EDID
>> properties to possibly validate the EDID (in case that edid did not
>> get "fixed" by the Kernel). The nice thing about this is that it would
>> make the test be more like a real driver usage. Do you see any
>> possible problems with this approach?
>
> I don't really see this as a valid option in light of the descriptions I've
> given above. This has a good chance of introducing latency problems which
> may adversely affect the tests as well.

We have 5 seconds, that's way more than enough.

>
>
>>> +
>>>          /* Try to read the source of the interrupt */
>>>          if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>>              intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>> b/drivers/gpu/drm/i915/intel_drv.h
>>> index e7b62be..42e4251 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -651,6 +651,7 @@ struct intel_dp {
>>>          /* Displayport compliance testing */
>>>          unsigned long compliance_test_type;
>>>          bool compliance_testing_active;
>>> +       bool compliance_edid_invalid;
>>>   };
>>>
>>>   struct intel_digital_port {
>>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>> index 87d85e8..8a7eb22 100644
>>> --- a/include/drm/drm_edid.h
>>> +++ b/include/drm/drm_edid.h
>>> @@ -388,4 +388,9 @@ struct edid *drm_do_get_edid(struct drm_connector
>>> *connector,
>>>                                size_t len),
>>>          void *data);
>>>
>>> +/* Check for corruption in raw EDID header - Displayport compliance
>>> +  * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>>> + */
>>> +bool drm_raw_edid_header_corrupt(void);
>>> +
>>>   #endif /* __DRM_EDID_H__ */
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>



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

Re: [PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing

[Todd Previte]

On 4/8/2015 3:37 PM, Paulo Zanoni wrote:
> 2015-04-08 18:43 GMT-03:00 Todd Previte<tprevite@gmail.com>:
>> On 4/8/2015 9:51 AM, Paulo Zanoni wrote:
>>> 2015-03-31 14:15 GMT-03:00 Todd Previte<tprevite@gmail.com>:
>>>> Displayport compliance test 4.2.2.6 requires that a source device be
>>>> capable of detecting
>>>> a corrupt EDID. To do this, the test sets up an invalid EDID header to be
>>>> read by the source
>>>> device. Unfortunately, the DRM EDID reading and parsing functions are
>>>> actually too good in
>>>> this case and prevent the source from reading the corrupted EDID. The
>>>> result is a failed
>>>> compliance test.
>>>>
>>>> In order to successfully pass the test, the raw EDID header must be
>>>> checked on each read
>>>> to see if has been "corrupted". If an invalid raw header is detected, a
>>>> flag is set that
>>>> allows the compliance testing code to acknowledge that fact and react
>>>> appropriately. The
>>>> flag is automatically cleared on read.
>>>>
>>>> This code is designed to expressly work for compliance testing without
>>>> disrupting normal
>>>> operations for EDID reading and parsing.
>>>>
>>>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>>>> Cc:dri-devel@lists.freedesktop.org
>>>> ---
>>>>    drivers/gpu/drm/drm_edid.c       | 33 +++++++++++++++++++++++++++++++++
>>>>    drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
>>>>    drivers/gpu/drm/i915/intel_drv.h |  1 +
>>>>    include/drm/drm_edid.h           |  5 +++++
>>>>    4 files changed, 56 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>> index 53bc7a6..3d4f473 100644
>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>> @@ -990,6 +990,32 @@ static const u8 edid_header[] = {
>>>>           0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
>>>>    };
>>>>
>>>> +
>>>> +/* Flag for EDID corruption testing
>>>> + * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>>>> + */
>>>> +static bool raw_edid_header_corrupted;
>>> A static variable like this is not a good design, especially for a
>>> module like drm.ko. If you really need this, please store it inside
>>> some struct. But see below first.
>> Per our discussion this morning, I concur. This has been removed in favor of
>> a different solution that uses a new boolean flag in the drm_connector
>> struct.
>>
>> Capturing more of the discussion here, the static boolean was a bad idea to
>> begin with and needed to be removed. One solution was to make the flag
>> non-static and non-clear-on-read, then add a separate clear() function. But
>> it still had the problem of potential misuse other places in the code. The
>> current solution (which will be posted with V5) modifies the is_valid()
>> function and adds a flag in the drm_connector struct that can be used to
>> detect this low-level header corruption.
>>
>>
>>>> +
>>>> +/**
>>>> + * drm_raw_edid_header_valid - check to see if the raw header is
>>>> + * corrupt or not. Used solely for Displayport compliance
>>>> + * testing and required by Link CTS Core 1.2 rev1.1 4.2.2.6.
>>>> + * @raw_edid: pointer to raw base EDID block
>>>> + *
>>>> + * Indicates whether the original EDID header as read from the
>>>> + * device was corrupt or not. Clears on read.
>>>> + *
>>>> + * Return: true if the raw header was corrupt, otherwise false
>>>> + */
>>>> +bool drm_raw_edid_header_corrupt(void)
>>>> +{
>>>> +       bool corrupted = raw_edid_header_corrupted;
>>>> +
>>>> +       raw_edid_header_corrupted = 0;
>>>> +       return corrupted;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_raw_edid_header_corrupt);
>>>> +
>>>>    /**
>>>>     * drm_edid_header_is_valid - sanity check the header of the base EDID
>>>> block
>>>>     * @raw_edid: pointer to raw base EDID block
>>>> @@ -1006,6 +1032,13 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
>>>>                   if (raw_edid[i] == edid_header[i])
>>>>                           score++;
>>>>
>>>> +       if (score != 8) {
>>>> +               /* Log and set flag here for EDID corruption testing
>>>> +                * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>>>> +                */
>>>> +               DRM_DEBUG_DRIVER("Raw EDID header invalid\n");
>>>> +               raw_edid_header_corrupted = 1;
>>>> +       }
>>> The problem is that here we're limiting ourselves to just a bad edid
>>> header, not a bad edid in general, so there are many things which we
>>> might not get - such as a simple wrong checksum edid value. I remember
>>> that on the previous patch you calculated the whole checksum manually,
>>> but I don't see that code anymore. What was the reason for the change?
>> So this code is specifically for the 4.2.2.6 compliance test that is looking
>> for nothing more than an invalid EDID header.
> On the version of the spec I have (1.2 Core, Aug 22 2011), 4.2.2.6 is
> "EDID Corruption Detection", and it mentions "EDID corruption" without
> really getting into the details of header corruption. On the "Test
> procedure" description, it mentions "Reference Sink sets up EDID with
> incorrect checksum", which we don't check. Of course, changing the
> header may produce an incorrect checksum, but maybe the wrong header
> is just a particular detail of the compliance testing device you have,
> while others could potentially have other forms of corruption, such as
> just a bad checksum?
It could very well be particular this unit. So with a different test 
device, we might be able to get away with just checking the checksum. 
For this one, however, we don't appear to have that option. I added the 
checksum computation into the header fixup code just to make sure.

> In the paragraphs below you elaborate even more on the assumption of a
> bad header instead of just a bad checksum, so maybe we have different
> versions of the spec? (I still remember when I used version 1.0 of a
> certain non-backwards-compatible spec to review a patch made against
> version 0.8 of the same spec)
I do have a later version of the spec, but description of this test 
seems to be the same between the two.
>> In fact, the test unit only
>> sets that header as invalid once, so if you miss it on the first read, you
>> can't go back and check it again later - the test will now fail. So catching
>> the general case isn't really what this is about - it's about being able to
>> detect a corrupt EDID header even if it only happens once.
>>
>> Honestly, the DRM EDID code is VERY good about catching corruption cases and
>> in the case of corrupted headers, fixing them and moving on. I had to tie
>> into it at a fairly low level in order to catch the invalid header before
>> the code fixed it.
>>
>> With respect to the checksum code, for quite a while the checksum
>> computation was incorrect in the DRM code. Somewhere along in November of
>> last year or 2013 (I remember the month, not the year, go figure) someone
>> came along and added a checksum computation that actually worked. So that
>> rendered that original code I wrote unnecessary.
>>
>>> Also, while reviewing the patch I just discovered
>>> connector->bad_edid_counter. Can't we just use it instead of this
>>> patch? I mean: grab the current counter, check edid, see if the
>>> counter moved.
>> I think the above description highlights why using this counter really isn't
>> an option. Since the code only gets one shot at catching that invalid
>> header, it's essential to make sure it's captured specifically. Comparing
>> before and after values of this counter doesn't specifically say that the
>> header was invalid, only that SOMEthing in the EDID was invalid.
> Which is, according to the way I read the spec, not a problem.
I completely agree with you. Unfortunately, coding directly to the spec 
isn't enough in this case.

>
>>>>           return score;
>>>>    }
>>>>    EXPORT_SYMBOL(drm_edid_header_is_valid);
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>> index dc87276..57f8e43 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -3824,6 +3824,9 @@ update_status:
>>>>                                      &response, 1);
>>>>           if (status <= 0)
>>>>                   DRM_DEBUG_KMS("Could not write test response to
>>>> sink\n");
>>>> +
>>>> +       /* Clear flag here, after testing is complete*/
>>>> +       intel_dp->compliance_edid_invalid = 0;
>>>>    }
>>>>
>>>>    static int
>>>> @@ -3896,6 +3899,10 @@ intel_dp_check_link_status(struct intel_dp
>>>> *intel_dp)
>>>>    {
>>>>           struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>           struct intel_encoder *intel_encoder =
>>>> &dp_to_dig_port(intel_dp)->base;
>>>> +       struct drm_connector *connector =
>>>> &intel_dp->attached_connector->base;
>>>> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
>>>> +       struct edid *edid_read = NULL;
>>>> +
>>>>           u8 sink_irq_vector;
>>>>           u8 link_status[DP_LINK_STATUS_SIZE];
>>>>
>>>> @@ -3912,6 +3919,16 @@ intel_dp_check_link_status(struct intel_dp
>>>> *intel_dp)
>>>>                   return;
>>>>           }
>>>>
>>>> +       /* Compliance testing requires an EDID read for all HPD events
>>>> +        * Link CTS Core 1.2 rev 1.1: Test 4.2.2.1
>>>> +        * Flag set here will be handled in the EDID test function
>>>> +        */
>>>> +       edid_read = drm_get_edid(connector, adapter);
>>>> +       if (!edid_read || drm_raw_edid_header_corrupt() == 1) {
>>>> +               DRM_DEBUG_DRIVER("EDID invalid, setting flag\n");
>>>> +               intel_dp->compliance_edid_invalid = 1;
>>>> +       }
>>> I see that on the next patch you also add a drm_get_edid() call, so we
>>> have apparently added 2 calls for the edid test. Do we really need
>>> both? Why is this one needed? Why is that one needed?
>> So there's two issues here - first is the same one mentioned above, catching
>> that single instance of a corrupted EDID header. The second is that the
>> checksum from the test device differs between the two reads. If you remove
>> either one of them, one test or the other will fail.
> But then why not keep both at the same place? The one here is going to
> affect a lot more than just compliance testing, while the other is
> contained to DP compliance code.
I was able to find a solution that removed the duplicate EDID read. I 
had to add a checksum storage variable in the intel_dp struct, but 
that's infinitely better than having another EDID read.

Unfortunately though, the one that has to say is in the 
check_link_status. There's just no way around it because of the test 
4.2.2.1 that requires it to happen for a hot plug event. There's no test 
request bit set for that, or any other indicator. It simply has to 
happen for every HPD plug event.

>>> Also, some more ideas:
>>>
>>> I also thought that we already automatically issued get_edid() calls
>>> on the normal hotplug code path, so it would be a "third" call on the
>>> codepath for the test. Can't we just rely on this one?
>> Same issue as above.
>>> Another idea would be: instead of getting the edid from inside the
>>> Kernel, we could try to get it from the user-space, using the
>>> GetResources/GetConnector IOCTLs, and also maybe look at the EDID
>>> properties to possibly validate the EDID (in case that edid did not
>>> get "fixed" by the Kernel). The nice thing about this is that it would
>>> make the test be more like a real driver usage. Do you see any
>>> possible problems with this approach?
>> I don't really see this as a valid option in light of the descriptions I've
>> given above. This has a good chance of introducing latency problems which
>> may adversely affect the tests as well.
> We have 5 seconds, that's way more than enough.
The test has a 5 second timeout for the entire operation. I'm less 
concerned with timing out and more concerned about not being able to 
catch things fast enough or react fast enough to parameter or value 
changes. It may or may not be an issue for processing the EDID (I'd lean 
more towards the not case) but it's something that has to be kept in 
mind here, as this has caused problems in the past when building out the 
test interfaces.

In any case, this sounds like this is a suggestion rather than a 
blocking issue. My main concern with moving all this stuff into 
userspace is that it's moving towards building a Displayport-compliant 
user app versus a Displayport-compliant driver. But this is something 
that I can look into sometime down the road.


>>>> +
>>>>           /* Try to read the source of the interrupt */
>>>>           if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>>>               intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>>> b/drivers/gpu/drm/i915/intel_drv.h
>>>> index e7b62be..42e4251 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -651,6 +651,7 @@ struct intel_dp {
>>>>           /* Displayport compliance testing */
>>>>           unsigned long compliance_test_type;
>>>>           bool compliance_testing_active;
>>>> +       bool compliance_edid_invalid;
>>>>    };
>>>>
>>>>    struct intel_digital_port {
>>>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>>> index 87d85e8..8a7eb22 100644
>>>> --- a/include/drm/drm_edid.h
>>>> +++ b/include/drm/drm_edid.h
>>>> @@ -388,4 +388,9 @@ struct edid *drm_do_get_edid(struct drm_connector
>>>> *connector,
>>>>                                 size_t len),
>>>>           void *data);
>>>>
>>>> +/* Check for corruption in raw EDID header - Displayport compliance
>>>> +  * Displayport Link CTS Core 1.2 rev1.1 - 4.2.2.6
>>>> + */
>>>> +bool drm_raw_edid_header_corrupt(void);
>>>> +
>>>>    #endif /* __DRM_EDID_H__ */
>>>> --
>>>> 1.9.1
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>

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

[PATCH 5/9] drm/i915: Update the EDID automated compliance test function

[Todd Previte]

Updates the EDID compliance test function to perform the EDID read as
required by the tests. This read needs to take place in the kernel for
reasons of speed and efficiency. The results of the EDID read operations
are handed off to userspace so that the userspace app can set the display
mode appropriately for the test response.

The compliance_test_active flag now appears 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.

V2:
- Addressed mailing list feedback
- Removed excess debug messages
- Removed extraneous comments
- Fixed formatting issues (line length > 80)
- Updated the debug message in compute_edid_checksum to output hex values
  instead of decimal
V3:
- Addressed more list feedback
- Added the test_active flag to the autotest function
- Removed test_active flag from handler
- Added failsafe check on the compliance test active flag
  at the end of the test handler
- Fixed checkpatch.pl issues
V4:
- Removed the checksum computation function and its use as it has been
  rendered superfluous by changes to the core DRM EDID functions
- Updated to use the raw header corruption detection mechanism
- Moved the declaration of the test_data variable here

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 53 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  3 ++-
 2 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 57f8e43..16d35903 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -41,6 +41,16 @@
 
 #define DP_LINK_CHECK_TIMEOUT	(10 * 1000)
 
+/* Compliance test status bits  */
+#define INTEL_DP_EDID_SHIFT_MASK	0
+#define INTEL_DP_EDID_OK		(0 << INTEL_DP_EDID_SHIFT_MASK)
+#define INTEL_DP_EDID_CORRUPT		(1 << INTEL_DP_EDID_SHIFT_MASK)
+
+#define INTEL_DP_RESOLUTION_SHIFT_MASK	4
+#define INTEL_DP_RESOLUTION_PREFERRED	(1 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define INTEL_DP_RESOLUTION_STANDARD	(2 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define INTEL_DP_RESOLUTION_FAILSAFE	(3 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+
 struct dp_link_dpll {
 	int link_bw;
 	struct dpll dpll;
@@ -3766,7 +3776,44 @@ 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)
 {
+	struct drm_connector *connector = &intel_dp->attached_connector->base;
+	struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+	struct edid *edid_read = NULL;
 	uint8_t test_result = DP_TEST_NAK;
+	uint32_t ret = 0;
+
+	edid_read = drm_get_edid(connector, adapter);
+
+	if (drm_raw_edid_header_corrupt() == 1) {
+		DRM_DEBUG_DRIVER("EDID Header corrupted\n");
+		intel_dp->compliance_edid_invalid = 1;
+	}
+
+	if (edid_read == NULL || intel_dp->compliance_edid_invalid ||
+	    intel_dp->aux.i2c_defer_count > 6) {
+		/* Check for NACKs/DEFERs, use failsafe if detected
+		   (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
+		if (intel_dp->aux.i2c_nack_count > 0 ||
+			intel_dp->aux.i2c_defer_count > 0)
+			DRM_DEBUG_KMS("EDID read had %d NACKs, %d DEFERs\n",
+				      intel_dp->aux.i2c_nack_count,
+				      intel_dp->aux.i2c_defer_count);
+		intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ |
+						 INTEL_DP_EDID_CORRUPT  |
+						 INTEL_DP_RESOLUTION_FAILSAFE;
+	} else {
+		ret = drm_dp_dpcd_write(&intel_dp->aux,
+					DP_TEST_EDID_CHECKSUM,
+					&edid_read->checksum, 1);
+		test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
+		intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ |
+						 INTEL_DP_EDID_OK       |
+						 INTEL_DP_RESOLUTION_STANDARD;
+	}
+
+	/* Set test active flag here so userspace doesn't interrupt things */
+	intel_dp->compliance_testing_active = 1;
+
 	return test_result;
 }
 
@@ -4596,6 +4643,12 @@ mst_fail:
 		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
 		intel_dp->is_mst = false;
 		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
+	} else {
+		/* SST mode - handle short/long pulses here */
+		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+		intel_dp_check_link_status(intel_dp);
+		drm_modeset_unlock(&dev->mode_config.connection_mutex);
+		ret = IRQ_HANDLED;
 	}
 put_power:
 	intel_display_power_put(dev_priv, power_domain);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 42e4251..fb6134f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -649,7 +649,8 @@ struct intel_dp {
 				     uint32_t aux_clock_divider);
 
 	/* Displayport compliance testing */
-	unsigned long compliance_test_type;
+	unsigned char compliance_test_type;
+	unsigned long compliance_test_data;
 	bool compliance_testing_active;
 	bool compliance_edid_invalid;
 };
-- 
1.9.1

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

Re: [PATCH 5/9] drm/i915: Update the EDID automated compliance test function

[Paulo Zanoni]

2015-03-31 14:15 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> Updates the EDID compliance test function to perform the EDID read as
> required by the tests. This read needs to take place in the kernel for
> reasons of speed and efficiency. The results of the EDID read operations
> are handed off to userspace so that the userspace app can set the display
> mode appropriately for the test response.
>
> The compliance_test_active flag now appears 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.
>
> V2:
> - Addressed mailing list feedback
> - Removed excess debug messages
> - Removed extraneous comments
> - Fixed formatting issues (line length > 80)
> - Updated the debug message in compute_edid_checksum to output hex values
>   instead of decimal
> V3:
> - Addressed more list feedback
> - Added the test_active flag to the autotest function
> - Removed test_active flag from handler
> - Added failsafe check on the compliance test active flag
>   at the end of the test handler
> - Fixed checkpatch.pl issues
> V4:
> - Removed the checksum computation function and its use as it has been
>   rendered superfluous by changes to the core DRM EDID functions
> - Updated to use the raw header corruption detection mechanism
> - Moved the declaration of the test_data variable here
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 53 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  3 ++-
>  2 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 57f8e43..16d35903 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -41,6 +41,16 @@
>
>  #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)
>
> +/* Compliance test status bits  */
> +#define INTEL_DP_EDID_SHIFT_MASK       0
> +#define INTEL_DP_EDID_OK               (0 << INTEL_DP_EDID_SHIFT_MASK)
> +#define INTEL_DP_EDID_CORRUPT          (1 << INTEL_DP_EDID_SHIFT_MASK)
> +
> +#define INTEL_DP_RESOLUTION_SHIFT_MASK 4
> +#define INTEL_DP_RESOLUTION_PREFERRED  (1 << INTEL_DP_RESOLUTION_SHIFT_MASK)
> +#define INTEL_DP_RESOLUTION_STANDARD   (2 << INTEL_DP_RESOLUTION_SHIFT_MASK)
> +#define INTEL_DP_RESOLUTION_FAILSAFE   (3 << INTEL_DP_RESOLUTION_SHIFT_MASK)
> +
>  struct dp_link_dpll {
>         int link_bw;
>         struct dpll dpll;
> @@ -3766,7 +3776,44 @@ 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)
>  {
> +       struct drm_connector *connector = &intel_dp->attached_connector->base;
> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
> +       struct edid *edid_read = NULL;
>         uint8_t test_result = DP_TEST_NAK;
> +       uint32_t ret = 0;

Variable "ret" is set but not used.

> +
> +       edid_read = drm_get_edid(connector, adapter);

This is the additional edid read mentioned in the review of the previous patch.


> +
> +       if (drm_raw_edid_header_corrupt() == 1) {
> +               DRM_DEBUG_DRIVER("EDID Header corrupted\n");
> +               intel_dp->compliance_edid_invalid = 1;
> +       }
> +
> +       if (edid_read == NULL || intel_dp->compliance_edid_invalid ||
> +           intel_dp->aux.i2c_defer_count > 6) {
> +               /* Check for NACKs/DEFERs, use failsafe if detected
> +                  (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */

Missing '*' char on the comment.


> +               if (intel_dp->aux.i2c_nack_count > 0 ||
> +                       intel_dp->aux.i2c_defer_count > 0)

Since we're potentially reading edid more than once after the test
starts - as explained in the review of p4 -, do those nack/defer
counts make sense? Shouldn't we zero them before each edid read
attempt?


> +                       DRM_DEBUG_KMS("EDID read had %d NACKs, %d DEFERs\n",
> +                                     intel_dp->aux.i2c_nack_count,
> +                                     intel_dp->aux.i2c_defer_count);
> +               intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ |
> +                                                INTEL_DP_EDID_CORRUPT  |
> +                                                INTEL_DP_RESOLUTION_FAILSAFE;

The compliance_test_data variable certainly needs a very good
documentation on what its bits means - possibly on top of its
definition, which is on patch 1 right now. Maybe it should be split
into more than one variable, since the bits that go inside it come
from different places. At least in the definition we should notice
"bits2-4 are from dp_helper.h". Another problem is that bits 0 and 1
from dp_helper.h can't be used there, which is even more confusing.


> +       } else {
> +               ret = drm_dp_dpcd_write(&intel_dp->aux,
> +                                       DP_TEST_EDID_CHECKSUM,
> +                                       &edid_read->checksum, 1);
> +               test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
> +               intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ |
> +                                                INTEL_DP_EDID_OK       |
> +                                                INTEL_DP_RESOLUTION_STANDARD;
> +       }
> +
> +       /* Set test active flag here so userspace doesn't interrupt things */
> +       intel_dp->compliance_testing_active = 1;
> +
>         return test_result;
>  }
>
> @@ -4596,6 +4643,12 @@ mst_fail:
>                 DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
>                 intel_dp->is_mst = false;
>                 drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> +       } else {
> +               /* SST mode - handle short/long pulses here */
> +               drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +               intel_dp_check_link_status(intel_dp);
> +               drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +               ret = IRQ_HANDLED;

I guess this chunk belongs to patch 6, especially since you rewrite
part of this new code there.


>         }
>  put_power:
>         intel_display_power_put(dev_priv, power_domain);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 42e4251..fb6134f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -649,7 +649,8 @@ struct intel_dp {
>                                      uint32_t aux_clock_divider);
>
>         /* Displayport compliance testing */
> -       unsigned long compliance_test_type;
> +       unsigned char compliance_test_type;

This is the chunk I was talking about in my review of p1.

> +       unsigned long compliance_test_data;
>         bool compliance_testing_active;
>         bool compliance_edid_invalid;
>  };
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 5/9] drm/i915: Update the EDID automated compliance test function

[Todd Previte]

On 4/8/2015 10:02 AM, Paulo Zanoni wrote:
> 2015-03-31 14:15 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> Updates the EDID compliance test function to perform the EDID read as
>> required by the tests. This read needs to take place in the kernel for
>> reasons of speed and efficiency. The results of the EDID read operations
>> are handed off to userspace so that the userspace app can set the display
>> mode appropriately for the test response.
>>
>> The compliance_test_active flag now appears 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.
>>
>> V2:
>> - Addressed mailing list feedback
>> - Removed excess debug messages
>> - Removed extraneous comments
>> - Fixed formatting issues (line length > 80)
>> - Updated the debug message in compute_edid_checksum to output hex values
>>    instead of decimal
>> V3:
>> - Addressed more list feedback
>> - Added the test_active flag to the autotest function
>> - Removed test_active flag from handler
>> - Added failsafe check on the compliance test active flag
>>    at the end of the test handler
>> - Fixed checkpatch.pl issues
>> V4:
>> - Removed the checksum computation function and its use as it has been
>>    rendered superfluous by changes to the core DRM EDID functions
>> - Updated to use the raw header corruption detection mechanism
>> - Moved the declaration of the test_data variable here
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c  | 53 ++++++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h |  3 ++-
>>   2 files changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 57f8e43..16d35903 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -41,6 +41,16 @@
>>
>>   #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)
>>
>> +/* Compliance test status bits  */
>> +#define INTEL_DP_EDID_SHIFT_MASK       0
>> +#define INTEL_DP_EDID_OK               (0 << INTEL_DP_EDID_SHIFT_MASK)
>> +#define INTEL_DP_EDID_CORRUPT          (1 << INTEL_DP_EDID_SHIFT_MASK)
>> +
>> +#define INTEL_DP_RESOLUTION_SHIFT_MASK 4
>> +#define INTEL_DP_RESOLUTION_PREFERRED  (1 << INTEL_DP_RESOLUTION_SHIFT_MASK)
>> +#define INTEL_DP_RESOLUTION_STANDARD   (2 << INTEL_DP_RESOLUTION_SHIFT_MASK)
>> +#define INTEL_DP_RESOLUTION_FAILSAFE   (3 << INTEL_DP_RESOLUTION_SHIFT_MASK)
>> +
>>   struct dp_link_dpll {
>>          int link_bw;
>>          struct dpll dpll;
>> @@ -3766,7 +3776,44 @@ 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)
>>   {
>> +       struct drm_connector *connector = &intel_dp->attached_connector->base;
>> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
>> +       struct edid *edid_read = NULL;
>>          uint8_t test_result = DP_TEST_NAK;
>> +       uint32_t ret = 0;
> Variable "ret" is set but not used.

Well it was "used" but the value wasn't really checked. The next version 
of this patch checks the value and reports status based on that.

>
>> +
>> +       edid_read = drm_get_edid(connector, adapter);
> This is the additional edid read mentioned in the review of the previous patch.

This one has been removed for the next patch revision.

>
>> +
>> +       if (drm_raw_edid_header_corrupt() == 1) {
>> +               DRM_DEBUG_DRIVER("EDID Header corrupted\n");
>> +               intel_dp->compliance_edid_invalid = 1;
>> +       }
>> +
>> +       if (edid_read == NULL || intel_dp->compliance_edid_invalid ||
>> +           intel_dp->aux.i2c_defer_count > 6) {
>> +               /* Check for NACKs/DEFERs, use failsafe if detected
>> +                  (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
> Missing '*' char on the comment.
Fixed.
>
>
>> +               if (intel_dp->aux.i2c_nack_count > 0 ||
>> +                       intel_dp->aux.i2c_defer_count > 0)
> Since we're potentially reading edid more than once after the test
> starts - as explained in the review of p4 -, do those nack/defer
> counts make sense? Shouldn't we zero them before each edid read
> attempt?

That's a good point. That had potential to adversely affect the test 
results for the NACKs and DEFERs. Removing the extra EDID read should 
eliminate that problem though, so I think the next version should be ok.

>
>> +                       DRM_DEBUG_KMS("EDID read had %d NACKs, %d DEFERs\n",
>> +                                     intel_dp->aux.i2c_nack_count,
>> +                                     intel_dp->aux.i2c_defer_count);
>> +               intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ |
>> +                                                INTEL_DP_EDID_CORRUPT  |
>> +                                                INTEL_DP_RESOLUTION_FAILSAFE;
> The compliance_test_data variable certainly needs a very good
> documentation on what its bits means - possibly on top of its
> definition, which is on patch 1 right now. Maybe it should be split
> into more than one variable, since the bits that go inside it come
> from different places. At least in the definition we should notice
> "bits2-4 are from dp_helper.h". Another problem is that bits 0 and 1
> from dp_helper.h can't be used there, which is even more confusing.

Actually this already *has* been split into different variables. :) The 
test type (the bits from the dp_helper header) are already in a variable 
of the same name (compliance_test_type). The EDID corrupt flags can be 
removed too, since those are no longer necessary. The test data simply 
indicates to user space what resolution to set. I'll put a quick 
description that effect in the header where those variables are declared 
and clean up the code above for the next patch version.

>
>> +       } else {
>> +               ret = drm_dp_dpcd_write(&intel_dp->aux,
>> +                                       DP_TEST_EDID_CHECKSUM,
>> +                                       &edid_read->checksum, 1);
>> +               test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
>> +               intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ |
>> +                                                INTEL_DP_EDID_OK       |
>> +                                                INTEL_DP_RESOLUTION_STANDARD;
>> +       }
>> +
>> +       /* Set test active flag here so userspace doesn't interrupt things */
>> +       intel_dp->compliance_testing_active = 1;
>> +
>>          return test_result;
>>   }
>>
>> @@ -4596,6 +4643,12 @@ mst_fail:
>>                  DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
>>                  intel_dp->is_mst = false;
>>                  drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
>> +       } else {
>> +               /* SST mode - handle short/long pulses here */
>> +               drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> +               intel_dp_check_link_status(intel_dp);
>> +               drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +               ret = IRQ_HANDLED;
> I guess this chunk belongs to patch 6, especially since you rewrite
> part of this new code there.
Yep. This has been moved already. As I mentioned earlier on IRC some of 
the patch chunks have been moved around and squished here and there. So 
I've regenerated the whole set for V5.
>
>>          }
>>   put_power:
>>          intel_display_power_put(dev_priv, power_domain);
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 42e4251..fb6134f 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -649,7 +649,8 @@ struct intel_dp {
>>                                       uint32_t aux_clock_divider);
>>
>>          /* Displayport compliance testing */
>> -       unsigned long compliance_test_type;
>> +       unsigned char compliance_test_type;
> This is the chunk I was talking about in my review of p1.
Yeah, this should all be fixed now.
>> +       unsigned long compliance_test_data;
>>          bool compliance_testing_active;
>>          bool compliance_edid_invalid;
>>   };
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

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

[PATCH 6/9] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation

[Todd Previte]

Update the hot plug function to handle the SST case. Instead of placing
the SST case within the long/short pulse block, it is now handled after
determining that MST mode is not in use. This way, the topology management
layer can handle any MST-related operations while SST operations are still
correctly handled afterwards.

This patch also corrects the problem of SST mode only being handled in the
case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes
both short and long pulses are used by the different tests, thus both cases
need to be addressed for SST.

This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
previous compliance testing patch sequence. Review feedback on that patch
indicated that updating intel_dp_hot_plug() was not the correct place for
the test handler.

For the SST case, the main stream is disabled for long HPD pulses as this
generally indicates either a connect/disconnect event or link failure. For
a number of case in compliance testing, the source is required to disable
the main link upon detection of a long HPD.

V2:
- N/A
V3:
- Place the SST mode link status check into the mst_fail case
- Remove obsolete comment regarding SST mode operation
- Removed an erroneous line of code that snuck in during rebasing
V4:
- Added a disable of the main stream (DP transport) for the long pulse case
  for SST to support compliance testing

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 16d35903..787b138 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4622,16 +4622,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
 				goto mst_fail;
 		}
-
-		if (!intel_dp->is_mst) {
-			/*
-			 * we'll check the link status via the normal hot plug path later -
-			 * but for short hpds we should check it now
-			 */
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-		}
 	}
 
 	ret = IRQ_HANDLED;
-- 
1.9.1

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

[PATCH 1/5] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Todd Previte]

For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
device must attempt at least 7 times to read the EDID when it receives an
I2C defer. The normal DRM code makes only 7 retries, regardless of whether
or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
since there are native defers interspersed with the I2C defers which
results in less than 7 EDID read attempts.

The solution is to decrement the retry counter when an I2C DEFER is returned
such that another read attempt will be made. This situation should normally
only occur in compliance testing, however, as a worse case real-world
scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
for a single transaction to complete. The net result is a slightly slower
response to an EDID read that shouldn't significantly impact overall
performance.

Signed-off-by: Todd Previte <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_dp_helper.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 79968e3..0539758 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -469,6 +469,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 		case DP_AUX_I2C_REPLY_DEFER:
 			DRM_DEBUG_KMS("I2C defer\n");
 			aux->i2c_defer_count++;
+			/* DP Compliance Test 4.2.2.5 Requirement:
+			 * Must have at least 7 retries for I2C defers on the
+			 * transaction to pass this test
+			 */
+			retry--;
 			usleep_range(400, 500);
 			continue;
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Ville Syrjälä]

On Wed, Apr 01, 2015 at 10:52:58AM -0700, Todd Previte wrote:
> For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
> device must attempt at least 7 times to read the EDID when it receives an
> I2C defer. The normal DRM code makes only 7 retries, regardless of whether
> or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
> since there are native defers interspersed with the I2C defers which
> results in less than 7 EDID read attempts.
> 
> The solution is to decrement the retry counter when an I2C DEFER is returned
> such that another read attempt will be made. This situation should normally
> only occur in compliance testing, however, as a worse case real-world
> scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
> for a single transaction to complete. The net result is a slightly slower
> response to an EDID read that shouldn't significantly impact overall
> performance.
> 
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 79968e3..0539758 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -469,6 +469,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  		case DP_AUX_I2C_REPLY_DEFER:
>  			DRM_DEBUG_KMS("I2C defer\n");
>  			aux->i2c_defer_count++;
> +			/* DP Compliance Test 4.2.2.5 Requirement:
> +			 * Must have at least 7 retries for I2C defers on the
> +			 * transaction to pass this test
> +			 */
> +			retry--;

That could lead to an infinite loop. I think what we need to do is
count the native and i2c defers separately, and abort if either
exceeds the limit.

>  			usleep_range(400, 500);
>  			continue;
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 06/11] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation

[Todd Previte]

Update the hot plug function to handle the SST case. Instead of placing
the SST case within the long/short pulse block, it is now handled after
determining that MST mode is not in use. This way, the topology management
layer can handle any MST-related operations while SST operations are still
correctly handled afterwards.

This patch also corrects the problem of SST mode only being handled in the
case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes
both short and long pulses are used by the different tests, thus both cases
need to be addressed for SST.

This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
previous compliance testing patch sequence. Review feedback on that patch
indicated that updating intel_dp_hot_plug() was not the correct place for
the test handler.

For the SST case, the main stream is disabled for long HPD pulses as this
generally indicates either a connect/disconnect event or link failure. For
a number of case in compliance testing, the source is required to disable
the main link upon detection of a long HPD.

V2:
- N/A
V3:
- Place the SST mode link status check into the mst_fail case
- Remove obsolete comment regarding SST mode operation
- Removed an erroneous line of code that snuck in during rebasing
V4:
- Added a disable of the main stream (DP transport) for the long pulse case
  for SST to support compliance testing
V5:
- Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 16d35903..0a77f5a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4622,16 +4622,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
 				goto mst_fail;
 		}
-
-		if (!intel_dp->is_mst) {
-			/*
-			 * we'll check the link status via the normal hot plug path later -
-			 * but for short hpds we should check it now
-			 */
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-		}
 	}
 
 	ret = IRQ_HANDLED;
@@ -4639,18 +4629,22 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 	goto put_power;
 mst_fail:
 	/* if we were in MST mode, and device is not there get out of MST mode */
-	if (intel_dp->is_mst) {
-		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
-		intel_dp->is_mst = false;
-		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
-	} else {
-		/* SST mode - handle short/long pulses here */
+	DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+	intel_dp->is_mst = false;
+	drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
+
+put_power:
+	/* SST mode - handle short/long pulses here */
+	if (!intel_dp->is_mst) {
+		/* TO DO: Handle short/long pulses individually
+		        Can save on training times and overhead by not doing
+		        full link status updating/processing for short pulses
+		 */
 		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 		intel_dp_check_link_status(intel_dp);
 		drm_modeset_unlock(&dev->mode_config.connection_mutex);
 		ret = IRQ_HANDLED;
 	}
-put_power:
 	intel_display_power_put(dev_priv, power_domain);
 
 	return ret;
-- 
1.9.1

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

Re: [PATCH 06/11] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation

[Paulo Zanoni]

2015-04-01 15:00 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> Update the hot plug function to handle the SST case. Instead of placing
> the SST case within the long/short pulse block, it is now handled after
> determining that MST mode is not in use. This way, the topology management
> layer can handle any MST-related operations while SST operations are still
> correctly handled afterwards.
>
> This patch also corrects the problem of SST mode only being handled in the
> case of a short (0.5ms - 1.0ms) HPD pulse.

It's not clear to me what exactly is the problem with the current
code. Can you please clarify?


> For compliance testing purposes
> both short and long pulses are used by the different tests, thus both cases
> need to be addressed for SST.
>
> This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
> previous compliance testing patch sequence. Review feedback on that patch
> indicated that updating intel_dp_hot_plug() was not the correct place for
> the test handler.
>
> For the SST case, the main stream is disabled for long HPD pulses as this
> generally indicates either a connect/disconnect event or link failure. For
> a number of case in compliance testing, the source is required to disable
> the main link upon detection of a long HPD.
>
> V2:
> - N/A
> V3:
> - Place the SST mode link status check into the mst_fail case
> - Remove obsolete comment regarding SST mode operation
> - Removed an erroneous line of code that snuck in during rebasing
> V4:
> - Added a disable of the main stream (DP transport) for the long pulse case
>   for SST to support compliance testing
> V5:
> - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 28 +++++++++++-----------------
>  1 file changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 16d35903..0a77f5a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4622,16 +4622,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>                         if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
>                                 goto mst_fail;
>                 }
> -
> -               if (!intel_dp->is_mst) {
> -                       /*
> -                        * we'll check the link status via the normal hot plug path later -
> -                        * but for short hpds we should check it now
> -                        */
> -                       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -                       intel_dp_check_link_status(intel_dp);
> -                       drm_modeset_unlock(&dev->mode_config.connection_mutex);
> -               }
>         }
>
>         ret = IRQ_HANDLED;
> @@ -4639,18 +4629,22 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>         goto put_power;
>  mst_fail:
>         /* if we were in MST mode, and device is not there get out of MST mode */
> -       if (intel_dp->is_mst) {
> -               DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> -               intel_dp->is_mst = false;
> -               drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> -       } else {
> -               /* SST mode - handle short/long pulses here */
> +       DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);

So now aren't we going to get MST log messages on non-MST cases, such
as a disconnect with long_hpd? I mean, even non-MST jumps to the
mst_fail label.


> +       intel_dp->is_mst = false;
> +       drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> +
> +put_power:
> +       /* SST mode - handle short/long pulses here */
> +       if (!intel_dp->is_mst) {
> +               /* TO DO: Handle short/long pulses individually
> +                       Can save on training times and overhead by not doing
> +                       full link status updating/processing for short pulses
> +                */
>                 drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>                 intel_dp_check_link_status(intel_dp);
>                 drm_modeset_unlock(&dev->mode_config.connection_mutex);
>                 ret = IRQ_HANDLED;
>         }
> -put_power:
>         intel_display_power_put(dev_priv, power_domain);
>
>         return ret;
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 06/11] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation

[Todd Previte]

On 4/8/2015 11:53 AM, Paulo Zanoni wrote:
> 2015-04-01 15:00 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> Update the hot plug function to handle the SST case. Instead of placing
>> the SST case within the long/short pulse block, it is now handled after
>> determining that MST mode is not in use. This way, the topology management
>> layer can handle any MST-related operations while SST operations are still
>> correctly handled afterwards.
>>
>> This patch also corrects the problem of SST mode only being handled in the
>> case of a short (0.5ms - 1.0ms) HPD pulse.
> It's not clear to me what exactly is the problem with the current
> code. Can you please clarify?
The main problem is that the comment in the code there is wrong. We 
*don't* check the link status later as it says it will. The legacy HPD 
handler (intel_dp_hot_plug) has been gutted but the function is still 
there in the code. It probably should have been removed when the new 
handler was implemented, but that's another matter entirely.

So as things stand, the only time we actually check the link status is 
when we get a short pulse on the HPD line. Long pulses (i.e. 
connect/disconnect events) don't get processed. This code corrects that 
problem and properly handles both long and short HPD pulses for the SST 
mode.

>> For compliance testing purposes
>> both short and long pulses are used by the different tests, thus both cases
>> need to be addressed for SST.
>>
>> This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
>> previous compliance testing patch sequence. Review feedback on that patch
>> indicated that updating intel_dp_hot_plug() was not the correct place for
>> the test handler.
>>
>> For the SST case, the main stream is disabled for long HPD pulses as this
>> generally indicates either a connect/disconnect event or link failure. For
>> a number of case in compliance testing, the source is required to disable
>> the main link upon detection of a long HPD.
>>
>> V2:
>> - N/A
>> V3:
>> - Place the SST mode link status check into the mst_fail case
>> - Remove obsolete comment regarding SST mode operation
>> - Removed an erroneous line of code that snuck in during rebasing
>> V4:
>> - Added a disable of the main stream (DP transport) for the long pulse case
>>    for SST to support compliance testing
>> V5:
>> - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 28 +++++++++++-----------------
>>   1 file changed, 11 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 16d35903..0a77f5a 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4622,16 +4622,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>                          if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
>>                                  goto mst_fail;
>>                  }
>> -
>> -               if (!intel_dp->is_mst) {
>> -                       /*
>> -                        * we'll check the link status via the normal hot plug path later -
>> -                        * but for short hpds we should check it now
>> -                        */
>> -                       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> -                       intel_dp_check_link_status(intel_dp);
>> -                       drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> -               }
>>          }
>>
>>          ret = IRQ_HANDLED;
>> @@ -4639,18 +4629,22 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>          goto put_power;
>>   mst_fail:
>>          /* if we were in MST mode, and device is not there get out of MST mode */
>> -       if (intel_dp->is_mst) {
>> -               DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
>> -               intel_dp->is_mst = false;
>> -               drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
>> -       } else {
>> -               /* SST mode - handle short/long pulses here */
>> +       DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> So now aren't we going to get MST log messages on non-MST cases, such
> as a disconnect with long_hpd? I mean, even non-MST jumps to the
> mst_fail label.

Yeah that got removed by mistake. I corrected this for the next version 
of the patch.

>
>> +       intel_dp->is_mst = false;
>> +       drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
>> +
>> +put_power:
>> +       /* SST mode - handle short/long pulses here */
>> +       if (!intel_dp->is_mst) {
>> +               /* TO DO: Handle short/long pulses individually
>> +                       Can save on training times and overhead by not doing
>> +                       full link status updating/processing for short pulses
>> +                */
>>                  drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>                  intel_dp_check_link_status(intel_dp);
>>                  drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>                  ret = IRQ_HANDLED;
>>          }
>> -put_power:
>>          intel_display_power_put(dev_priv, power_domain);
>>
>>          return ret;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

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

[PATCH 7/9] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Todd Previte]

For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
device must attempt at least 7 times to read the EDID when it receives an
I2C defer. The normal DRM code makes only 7 retries, regardless of whether
or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
since there are native defers interspersed with the I2C defers which
results in less than 7 EDID read attempts.

The solution is to decrement the retry counter when an I2C DEFER is returned
such that another read attempt will be made. This situation should normally
only occur in compliance testing, however, as a worse case real-world
scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
for a single transaction to complete. The net result is a slightly slower
response to an EDID read that shouldn't significantly impact overall
performance.

Signed-off-by: Todd Previte <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_dp_helper.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 79968e3..0539758 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -469,6 +469,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 		case DP_AUX_I2C_REPLY_DEFER:
 			DRM_DEBUG_KMS("I2C defer\n");
 			aux->i2c_defer_count++;
+			/* DP Compliance Test 4.2.2.5 Requirement:
+			 * Must have at least 7 retries for I2C defers on the
+			 * transaction to pass this test
+			 */
+			retry--;
 			usleep_range(400, 500);
 			continue;
 
-- 
1.9.1

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

Re: [PATCH 7/9] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Paulo Zanoni]

2015-03-31 14:15 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
> device must attempt at least 7 times to read the EDID when it receives an
> I2C defer. The normal DRM code makes only 7 retries, regardless of whether
> or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
> since there are native defers interspersed with the I2C defers which
> results in less than 7 EDID read attempts.
>
> The solution is to decrement the retry counter when an I2C DEFER is returned
> such that another read attempt will be made. This situation should normally
> only occur in compliance testing, however, as a worse case real-world
> scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
> for a single transaction to complete. The net result is a slightly slower
> response to an EDID read that shouldn't significantly impact overall
> performance.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 79968e3..0539758 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -469,6 +469,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>                 case DP_AUX_I2C_REPLY_DEFER:
>                         DRM_DEBUG_KMS("I2C defer\n");
>                         aux->i2c_defer_count++;
> +                       /* DP Compliance Test 4.2.2.5 Requirement:
> +                        * Must have at least 7 retries for I2C defers on the
> +                        * transaction to pass this test
> +                        */
> +                       retry--;

I wouldn't be surprised if someone discovers a monitor or some sort of
dongle that keeps sending I2C defer errors forever, keeping us in an
infinite loop. Shouldn't we count each error in separate? Or maybe
just loop up to 14 times, in case that doesn't violate any spec (I
didn't check)?


>                         usleep_range(400, 500);
>                         continue;
>
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Paulo Zanoni
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/9] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Todd Previte]

On 4/6/15 5:05 PM, Paulo Zanoni wrote:
> 2015-03-31 14:15 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
>> device must attempt at least 7 times to read the EDID when it receives an
>> I2C defer. The normal DRM code makes only 7 retries, regardless of whether
>> or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
>> since there are native defers interspersed with the I2C defers which
>> results in less than 7 EDID read attempts.
>>
>> The solution is to decrement the retry counter when an I2C DEFER is returned
>> such that another read attempt will be made. This situation should normally
>> only occur in compliance testing, however, as a worse case real-world
>> scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
>> for a single transaction to complete. The net result is a slightly slower
>> response to an EDID read that shouldn't significantly impact overall
>> performance.
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> Cc: dri-devel@lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/drm_dp_helper.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>> index 79968e3..0539758 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -469,6 +469,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>>                  case DP_AUX_I2C_REPLY_DEFER:
>>                          DRM_DEBUG_KMS("I2C defer\n");
>>                          aux->i2c_defer_count++;
>> +                       /* DP Compliance Test 4.2.2.5 Requirement:
>> +                        * Must have at least 7 retries for I2C defers on the
>> +                        * transaction to pass this test
>> +                        */
>> +                       retry--;
> I wouldn't be surprised if someone discovers a monitor or some sort of
> dongle that keeps sending I2C defer errors forever, keeping us in an
> infinite loop. Shouldn't we count each error in separate? Or maybe
> just loop up to 14 times, in case that doesn't violate any spec (I
> didn't check)?
I think the safest thing to do would be to put a failsafe on the 
i2c_defer_counter. That would ensure that the compliance test gets its 7 
retries and that if we do encounter a misbehaving device, the driver 
won't let the unending defers cause an infinite loop. Updated patch 
shortly.

>>                          usleep_range(400, 500);
>>                          continue;
>>
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

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

[PATCH 07/11] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Todd Previte]

For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
device must attempt at least 7 times to read the EDID when it receives an
I2C defer. The normal DRM code makes only 7 retries, regardless of whether
or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
since there are native defers interspersed with the I2C defers which
results in less than 7 EDID read attempts.

The solution is to decrement the retry counter when an I2C DEFER is returned
such that another read attempt will be made. This situation should normally
only occur in compliance testing, however, as a worse case real-world
scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
for a single transaction to complete. The net result is a slightly slower
response to an EDID read that shouldn't significantly impact overall
performance.

V2:
- Added a check on the number of I2C Defers to limit the number
  of times that the retries variable will be decremented. This
  is to address review feedback regarding possible infinite loops
  from misbehaving sink devices.

Signed-off-by: Todd Previte <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_dp_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 79968e3..23025cf 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -469,6 +469,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 		case DP_AUX_I2C_REPLY_DEFER:
 			DRM_DEBUG_KMS("I2C defer\n");
 			aux->i2c_defer_count++;
+			/* DP Compliance Test 4.2.2.5 Requirement:
+			 * Must have at least 7 retries for I2C defers on the
+			 * transaction to pass this test
+			 */
+			if (aux->i2c_defer_count < 8)
+				retry--;
 			usleep_range(400, 500);
 			continue;
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/11] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Paulo Zanoni]

2015-04-06 23:11 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
> device must attempt at least 7 times to read the EDID when it receives an
> I2C defer. The normal DRM code makes only 7 retries, regardless of whether
> or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
> since there are native defers interspersed with the I2C defers which
> results in less than 7 EDID read attempts.
>
> The solution is to decrement the retry counter when an I2C DEFER is returned
> such that another read attempt will be made. This situation should normally
> only occur in compliance testing, however, as a worse case real-world
> scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
> for a single transaction to complete. The net result is a slightly slower
> response to an EDID read that shouldn't significantly impact overall
> performance.
>
> V2:
> - Added a check on the number of I2C Defers to limit the number
>   of times that the retries variable will be decremented. This
>   is to address review feedback regarding possible infinite loops
>   from misbehaving sink devices.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 79968e3..23025cf 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -469,6 +469,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>                 case DP_AUX_I2C_REPLY_DEFER:
>                         DRM_DEBUG_KMS("I2C defer\n");
>                         aux->i2c_defer_count++;
> +                       /* DP Compliance Test 4.2.2.5 Requirement:
> +                        * Must have at least 7 retries for I2C defers on the
> +                        * transaction to pass this test
> +                        */
> +                       if (aux->i2c_defer_count < 8)

I don't think this is the way to go. During normal
(non-compliance-testing) operation we never zero i2c_defer_count, so
we can't expect this to work, since we may start drm_dp_i2c_do_msg
with a i2c_defer_count value different than zero. Also, during i915.ko
DP compliance we only zero i2c_defer_count at the very beginning of
each test, not at every aux transaction, and I really think we need a
solution that is not specific to compliance testing.


> +                               retry--;
>                         usleep_range(400, 500);
>                         continue;
>
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Paulo Zanoni
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/11] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Ville Syrjälä]

On Tue, Apr 07, 2015 at 11:29:43AM -0300, Paulo Zanoni wrote:
> 2015-04-06 23:11 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> > For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
> > device must attempt at least 7 times to read the EDID when it receives an
> > I2C defer. The normal DRM code makes only 7 retries, regardless of whether
> > or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
> > since there are native defers interspersed with the I2C defers which
> > results in less than 7 EDID read attempts.
> >
> > The solution is to decrement the retry counter when an I2C DEFER is returned
> > such that another read attempt will be made. This situation should normally
> > only occur in compliance testing, however, as a worse case real-world
> > scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
> > for a single transaction to complete. The net result is a slightly slower
> > response to an EDID read that shouldn't significantly impact overall
> > performance.
> >
> > V2:
> > - Added a check on the number of I2C Defers to limit the number
> >   of times that the retries variable will be decremented. This
> >   is to address review feedback regarding possible infinite loops
> >   from misbehaving sink devices.
> >
> > Signed-off-by: Todd Previte <tprevite@gmail.com>
> > Cc: dri-devel@lists.freedesktop.org
> > ---
> >  drivers/gpu/drm/drm_dp_helper.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 79968e3..23025cf 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -469,6 +469,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> >                 case DP_AUX_I2C_REPLY_DEFER:
> >                         DRM_DEBUG_KMS("I2C defer\n");
> >                         aux->i2c_defer_count++;
> > +                       /* DP Compliance Test 4.2.2.5 Requirement:
> > +                        * Must have at least 7 retries for I2C defers on the
> > +                        * transaction to pass this test
> > +                        */
> > +                       if (aux->i2c_defer_count < 8)
> 
> I don't think this is the way to go. During normal
> (non-compliance-testing) operation we never zero i2c_defer_count, so
> we can't expect this to work, since we may start drm_dp_i2c_do_msg
> with a i2c_defer_count value different than zero. Also, during i915.ko
> DP compliance we only zero i2c_defer_count at the very beginning of
> each test, not at every aux transaction, and I really think we need a
> solution that is not specific to compliance testing.

What I was suggesting earlier (or trying to at least) would be
simply something like this:

int defer_native = 0, defer_i2c = 0;
while (defer_native < 7 && defer_i2c < 7) {
	...
	case DP_AUX_NATIVE_REPLY_NACK:
		...
		defer_native++;
		continue;
	}
	...
	case DP_AUX_I2C_REPLY_DEFER:
		...
		defer_i2c++;
		continue;
	}
	...
}

> 
> 
> > +                               retry--;
> >                         usleep_range(400, 500);
> >                         continue;
> >
> > --
> > 1.9.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/11] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling

[Todd Previte]

[-- Attachment #1.1: Type: text/plain, Size: 5221 bytes --]



On 4/7/15 7:29 AM, Paulo Zanoni wrote:
> 2015-04-06 23:11 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> For test 4.2.2.5 to pass per the Link CTS Core 1.2 rev1.1 spec, the source
>> device must attempt at least 7 times to read the EDID when it receives an
>> I2C defer. The normal DRM code makes only 7 retries, regardless of whether
>> or not the response is a native defer or an I2C defer. Test 4.2.2.5 fails
>> since there are native defers interspersed with the I2C defers which
>> results in less than 7 EDID read attempts.
>>
>> The solution is to decrement the retry counter when an I2C DEFER is returned
>> such that another read attempt will be made. This situation should normally
>> only occur in compliance testing, however, as a worse case real-world
>> scenario, it would result in 13 attempts ( 6 native defers, 7 I2C defers)
>> for a single transaction to complete. The net result is a slightly slower
>> response to an EDID read that shouldn't significantly impact overall
>> performance.
>>
>> V2:
>> - Added a check on the number of I2C Defers to limit the number
>>    of times that the retries variable will be decremented. This
>>    is to address review feedback regarding possible infinite loops
>>    from misbehaving sink devices.
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> Cc: dri-devel@lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/drm_dp_helper.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>> index 79968e3..23025cf 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -469,6 +469,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>>                  case DP_AUX_I2C_REPLY_DEFER:
>>                          DRM_DEBUG_KMS("I2C defer\n");
>>                          aux->i2c_defer_count++;
>> +                       /* DP Compliance Test 4.2.2.5 Requirement:
>> +                        * Must have at least 7 retries for I2C defers on the
>> +                        * transaction to pass this test
>> +                        */
>> +                       if (aux->i2c_defer_count < 8)
> I don't think this is the way to go. During normal
> (non-compliance-testing) operation we never zero i2c_defer_count, so
> we can't expect this to work, since we may start drm_dp_i2c_do_msg
> with a i2c_defer_count value different than zero. Also, during i915.ko
> DP compliance we only zero i2c_defer_count at the very beginning of
> each test, not at every aux transaction, and I really think we need a
> solution that is not specific to compliance testing.
To capture the discussion from IRC:

The primary issue previously was the potential for an infinite loop when 
decrementing the retry count. That is clearly addressed by this code, by 
only decrementing the loop while the defer count is below 8. In 
actuality, that needs to be 7, so a followup patch with that change will 
be posted shortly. There are other solutions (Ville has one in his 
reply), but this one works correctly, is minimally invasive and doesn't 
require changes to the loop structure.

The defer counter isn't used anywhere outside of Displayport compliance 
testing. In fact, the counters in the aux struct didn't even exist until 
I put them in there in a previous patch to support this exact test case. 
So there really isn't a non-DP compliance test specific solution to be 
had here. It's also unlikely that this has any significant effect on 
normal operations. In the event that a device misbehaves and begins 
issuing loads of defers, this code would only delay it by at most 7 
retries before the counter exceeded its value.

Since this value is not used outside of compliance testing, a reset 
per-transaction is unnecessary. Additionally, that would break the EDID 
compliance testing code as it would have no way of detecting that the 
read had failed due to continuous defers. As long as the counter is 
reset for each test request (which it is), this code will function 
properly for both real world and compliance operations.

As indicated in the comments, this code exists to ensure compliance with 
test 4.2.2.5 from the Displayport Link CTS Core 1.2 rev1.1. This test 
verifies that the source device is capable of responding to a failure to 
read the EDID when the sink device continuously defers the transaction. 
It ensures that at least 7 I2C defers are received before calling it 
quits, versus simply retrying the transaction 7 times regardless of the 
failure mode.

Since I'm going to post an updated patch anyways, I will likely roll the 
defer count increment into the if-statement to remove one line of code. 
That also requires moving the increment to a pre- versus post- 
condition, as follows:

     ...
     if (++aux->i2c_defer_count < 7)
         ...
     ...

>
>> +                               retry--;
>>                          usleep_range(400, 500);
>>                          continue;
>>
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>


[-- Attachment #1.2: Type: text/html, Size: 6694 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 8/9] drm/i915: Add debugfs test control files for Displayport compliance testing

[Todd Previte]

This patch adds 3 debugfs files for handling Displayport compliance testing
and supercedes the previous patches that implemented debugfs support for
compliance testing. Those two patches were:

- 1 <edit - get the commit/patch titles>
- 2 <edit - get the commit/patch titles>

This new patch simplifies the debugfs implementation by places a single
test control value into an individual file. Each file is readable by
the usersapce application and the test_active file is writable to
indicate to the kernel when userspace has completed its portion of the
test sequence.

Replacing the previous files simplifies operation and speeds response
time for the user app, as it is required to poll on the test_active file
in order to determine when it needs to begin its operations.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 208 ++++++++++++++++++++++++++++++++++++
 1 file changed, 208 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b315f01..18775ef 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3701,6 +3701,211 @@ static const struct file_operations i915_display_crc_ctl_fops = {
 	.write = display_crc_ctl_write
 };
 
+static ssize_t i915_displayport_test_active_write(struct file *file,
+					    const char __user *ubuf,
+					    size_t len, loff_t *offp)
+{
+	char *input_buffer;
+	int status = 0;
+	struct seq_file *m;
+	struct drm_device *dev;
+	struct drm_connector *connector;
+	struct list_head *connector_list;
+	struct intel_dp *intel_dp;
+	int val = 0;
+
+	m = file->private_data;
+	if (!m) {
+		status = -ENODEV;
+		return status;
+	}
+	dev = m->private;
+
+	if (!dev) {
+		status = -ENODEV;
+		return status;
+	}
+	connector_list = &dev->mode_config.connector_list;
+
+	if (len == 0)
+		return 0;
+
+	input_buffer = kmalloc(len + 1, GFP_KERNEL);
+	if (!input_buffer)
+		return -ENOMEM;
+
+	if (copy_from_user(input_buffer, ubuf, len)) {
+		status = -EFAULT;
+		goto out;
+	}
+
+	input_buffer[len] = '\0';
+	DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
+
+	list_for_each_entry(connector, connector_list, head) {
+
+		if (connector->connector_type !=
+		    DRM_MODE_CONNECTOR_DisplayPort)
+			continue;
+
+		if (connector->connector_type ==
+		    DRM_MODE_CONNECTOR_DisplayPort &&
+		    connector->status == connector_status_connected) {
+			intel_dp = enc_to_intel_dp(connector->encoder);
+			status = kstrtoint(input_buffer, 10, &val);
+			if (status < 0)
+				goto out;
+			DRM_DEBUG_DRIVER("Got %d for test active\n", val);
+			/* To prevent erroneous activation of the compliance
+			 * testing code, only accept an actual value of 1 here
+			 */
+			if (val == 1)
+				intel_dp->compliance_testing_active = 1;
+			else
+				intel_dp->compliance_testing_active = 0;
+		}
+	}
+out:
+	kfree(input_buffer);
+	if (status < 0)
+		return status;
+
+	*offp += len;
+	return len;
+}
+
+static int i915_displayport_test_active_show(struct seq_file *m, void *data)
+{
+	struct drm_device *dev = m->private;
+	struct drm_connector *connector;
+	struct list_head *connector_list = &dev->mode_config.connector_list;
+	struct intel_dp *intel_dp;
+
+	if (!dev)
+		return -ENODEV;
+
+	list_for_each_entry(connector, connector_list, head) {
+
+		if (connector->connector_type !=
+		    DRM_MODE_CONNECTOR_DisplayPort)
+			continue;
+
+		if (connector->status == connector_status_connected &&
+		    connector->encoder != NULL) {
+			intel_dp = enc_to_intel_dp(connector->encoder);
+			if (intel_dp->compliance_testing_active)
+				seq_puts(m, "1");
+			else
+				seq_puts(m, "0");
+		} else
+			seq_puts(m, "0");
+	}
+
+	return 0;
+}
+
+static int i915_displayport_test_active_open(struct inode *inode,
+				       struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+
+	return single_open(file, i915_displayport_test_active_show, dev);
+}
+
+static const struct file_operations i915_displayport_test_active_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_displayport_test_active_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+	.write = i915_displayport_test_active_write
+};
+
+static int i915_displayport_test_data_show(struct seq_file *m, void *data)
+{
+	struct drm_device *dev = m->private;
+	struct drm_connector *connector;
+	struct list_head *connector_list = &dev->mode_config.connector_list;
+	struct intel_dp *intel_dp;
+
+	if (!dev)
+		return -ENODEV;
+
+	list_for_each_entry(connector, connector_list, head) {
+
+		if (connector->connector_type !=
+		    DRM_MODE_CONNECTOR_DisplayPort)
+			continue;
+
+		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);
+		} else
+			seq_puts(m, "0");
+	}
+
+	return 0;
+}
+static int i915_displayport_test_data_open(struct inode *inode,
+				       struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+
+	return single_open(file, i915_displayport_test_data_show, dev);
+}
+
+static const struct file_operations i915_displayport_test_data_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_displayport_test_data_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release
+};
+
+static int i915_displayport_test_type_show(struct seq_file *m, void *data)
+{
+	struct drm_device *dev = m->private;
+	struct drm_connector *connector;
+	struct list_head *connector_list = &dev->mode_config.connector_list;
+	struct intel_dp *intel_dp;
+
+	if (!dev)
+		return -ENODEV;
+
+	list_for_each_entry(connector, connector_list, head) {
+
+		if (connector->connector_type !=
+		    DRM_MODE_CONNECTOR_DisplayPort)
+			continue;
+
+		if (connector->status == connector_status_connected &&
+		    connector->encoder != NULL) {
+			intel_dp = enc_to_intel_dp(connector->encoder);
+			seq_printf(m, "%02x", intel_dp->compliance_test_type);
+		} else
+			seq_puts(m, "0");
+	}
+
+	return 0;
+}
+
+static int i915_displayport_test_type_open(struct inode *inode,
+				       struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+
+	return single_open(file, i915_displayport_test_type_show, dev);
+}
+
+static const struct file_operations i915_displayport_test_type_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_displayport_test_type_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release
+};
+
 static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
 {
 	struct drm_device *dev = m->private;
@@ -4450,6 +4655,9 @@ static const struct i915_debugfs_files {
 	{"i915_spr_wm_latency", &i915_spr_wm_latency_fops},
 	{"i915_cur_wm_latency", &i915_cur_wm_latency_fops},
 	{"i915_fbc_false_color", &i915_fbc_fc_fops},
+	{"i915_dp_test_data", &i915_displayport_test_data_fops},
+	{"i915_dp_test_type", &i915_displayport_test_type_fops},
+	{"i915_dp_test_active", &i915_displayport_test_active_fops}
 };
 
 void intel_display_crc_init(struct drm_device *dev)
-- 
1.9.1

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

[PATCH 08/11] drm/i915: Add debugfs test control files for Displayport compliance testing

[Todd Previte]

This patch adds 3 debugfs files for handling Displayport compliance testing
and supercedes the previous patches that implemented debugfs support for
compliance testing. Those patches were:

- [PATCH 04/17] drm/i915: Add debugfs functions for Displayport
			  compliance testing
- [PATCH 08/17] drm/i915: Add new debugfs file for Displayport
			  compliance test control
- [PATCH 09/17] drm/i915: Add debugfs write and test param parsing
			  functions for DP test control

This new patch simplifies the debugfs implementation by places a single
test control value into an individual file. Each file is readable by
the usersapce application and the test_active file is writable to
indicate to the kernel when userspace has completed its portion of the
test sequence.

Replacing the previous files simplifies operation and speeds response
time for the user app, as it is required to poll on the test_active file
in order to determine when it needs to begin its operations.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 208 ++++++++++++++++++++++++++++++++++++
 1 file changed, 208 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b315f01..18775ef 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3701,6 +3701,211 @@ static const struct file_operations i915_display_crc_ctl_fops = {
 	.write = display_crc_ctl_write
 };
 
+static ssize_t i915_displayport_test_active_write(struct file *file,
+					    const char __user *ubuf,
+					    size_t len, loff_t *offp)
+{
+	char *input_buffer;
+	int status = 0;
+	struct seq_file *m;
+	struct drm_device *dev;
+	struct drm_connector *connector;
+	struct list_head *connector_list;
+	struct intel_dp *intel_dp;
+	int val = 0;
+
+	m = file->private_data;
+	if (!m) {
+		status = -ENODEV;
+		return status;
+	}
+	dev = m->private;
+
+	if (!dev) {
+		status = -ENODEV;
+		return status;
+	}
+	connector_list = &dev->mode_config.connector_list;
+
+	if (len == 0)
+		return 0;
+
+	input_buffer = kmalloc(len + 1, GFP_KERNEL);
+	if (!input_buffer)
+		return -ENOMEM;
+
+	if (copy_from_user(input_buffer, ubuf, len)) {
+		status = -EFAULT;
+		goto out;
+	}
+
+	input_buffer[len] = '\0';
+	DRM_DEBUG_DRIVER("Copied %d bytes from user\n", (unsigned int)len);
+
+	list_for_each_entry(connector, connector_list, head) {
+
+		if (connector->connector_type !=
+		    DRM_MODE_CONNECTOR_DisplayPort)
+			continue;
+
+		if (connector->connector_type ==
+		    DRM_MODE_CONNECTOR_DisplayPort &&
+		    connector->status == connector_status_connected) {
+			intel_dp = enc_to_intel_dp(connector->encoder);
+			status = kstrtoint(input_buffer, 10, &val);
+			if (status < 0)
+				goto out;
+			DRM_DEBUG_DRIVER("Got %d for test active\n", val);
+			/* To prevent erroneous activation of the compliance
+			 * testing code, only accept an actual value of 1 here
+			 */
+			if (val == 1)
+				intel_dp->compliance_testing_active = 1;
+			else
+				intel_dp->compliance_testing_active = 0;
+		}
+	}
+out:
+	kfree(input_buffer);
+	if (status < 0)
+		return status;
+
+	*offp += len;
+	return len;
+}
+
+static int i915_displayport_test_active_show(struct seq_file *m, void *data)
+{
+	struct drm_device *dev = m->private;
+	struct drm_connector *connector;
+	struct list_head *connector_list = &dev->mode_config.connector_list;
+	struct intel_dp *intel_dp;
+
+	if (!dev)
+		return -ENODEV;
+
+	list_for_each_entry(connector, connector_list, head) {
+
+		if (connector->connector_type !=
+		    DRM_MODE_CONNECTOR_DisplayPort)
+			continue;
+
+		if (connector->status == connector_status_connected &&
+		    connector->encoder != NULL) {
+			intel_dp = enc_to_intel_dp(connector->encoder);
+			if (intel_dp->compliance_testing_active)
+				seq_puts(m, "1");
+			else
+				seq_puts(m, "0");
+		} else
+			seq_puts(m, "0");
+	}
+
+	return 0;
+}
+
+static int i915_displayport_test_active_open(struct inode *inode,
+				       struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+
+	return single_open(file, i915_displayport_test_active_show, dev);
+}
+
+static const struct file_operations i915_displayport_test_active_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_displayport_test_active_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+	.write = i915_displayport_test_active_write
+};
+
+static int i915_displayport_test_data_show(struct seq_file *m, void *data)
+{
+	struct drm_device *dev = m->private;
+	struct drm_connector *connector;
+	struct list_head *connector_list = &dev->mode_config.connector_list;
+	struct intel_dp *intel_dp;
+
+	if (!dev)
+		return -ENODEV;
+
+	list_for_each_entry(connector, connector_list, head) {
+
+		if (connector->connector_type !=
+		    DRM_MODE_CONNECTOR_DisplayPort)
+			continue;
+
+		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);
+		} else
+			seq_puts(m, "0");
+	}
+
+	return 0;
+}
+static int i915_displayport_test_data_open(struct inode *inode,
+				       struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+
+	return single_open(file, i915_displayport_test_data_show, dev);
+}
+
+static const struct file_operations i915_displayport_test_data_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_displayport_test_data_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release
+};
+
+static int i915_displayport_test_type_show(struct seq_file *m, void *data)
+{
+	struct drm_device *dev = m->private;
+	struct drm_connector *connector;
+	struct list_head *connector_list = &dev->mode_config.connector_list;
+	struct intel_dp *intel_dp;
+
+	if (!dev)
+		return -ENODEV;
+
+	list_for_each_entry(connector, connector_list, head) {
+
+		if (connector->connector_type !=
+		    DRM_MODE_CONNECTOR_DisplayPort)
+			continue;
+
+		if (connector->status == connector_status_connected &&
+		    connector->encoder != NULL) {
+			intel_dp = enc_to_intel_dp(connector->encoder);
+			seq_printf(m, "%02x", intel_dp->compliance_test_type);
+		} else
+			seq_puts(m, "0");
+	}
+
+	return 0;
+}
+
+static int i915_displayport_test_type_open(struct inode *inode,
+				       struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+
+	return single_open(file, i915_displayport_test_type_show, dev);
+}
+
+static const struct file_operations i915_displayport_test_type_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_displayport_test_type_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release
+};
+
 static void wm_latency_show(struct seq_file *m, const uint16_t wm[8])
 {
 	struct drm_device *dev = m->private;
@@ -4450,6 +4655,9 @@ static const struct i915_debugfs_files {
 	{"i915_spr_wm_latency", &i915_spr_wm_latency_fops},
 	{"i915_cur_wm_latency", &i915_cur_wm_latency_fops},
 	{"i915_fbc_false_color", &i915_fbc_fc_fops},
+	{"i915_dp_test_data", &i915_displayport_test_data_fops},
+	{"i915_dp_test_type", &i915_displayport_test_type_fops},
+	{"i915_dp_test_active", &i915_displayport_test_active_fops}
 };
 
 void intel_display_crc_init(struct drm_device *dev)
-- 
1.9.1

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

[PATCH 9/9] drm: Fix the 'native defer' message in drm_dp_i2c_do_msg()

[Todd Previte]

The debug message is missing a newline at the end and it makes the
logs hard to read when a device defers a lot. Simple 2-character fix
adds the newline at the end.

Signed-off-by: Todd Previte <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_dp_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 0539758..281bb67 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -433,7 +433,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 			return -EREMOTEIO;
 
 		case DP_AUX_NATIVE_REPLY_DEFER:
-			DRM_DEBUG_KMS("native defer");
+			DRM_DEBUG_KMS("native defer\n");
 			/*
 			 * We could check for I2C bit rate capabilities and if
 			 * available adjust this interval. We could also be
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 9/9] drm: Fix the 'native defer' message in drm_dp_i2c_do_msg()

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6106
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -5              272/272              267/272
ILK                                  302/302              302/302
SNB                                  303/303              303/303
IVB                                  338/338              338/338
BYT                                  287/287              287/287
HSW                                  361/361              361/361
BDW                                  308/308              308/308
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 PNV  igt@gem_userptr_blits@coherency-sync      CRASH(2)PASS(3)      CRASH(2)
*PNV  igt@gem_fence_thrash@bo-write-verify-threaded-none      PASS(3)      FAIL(1)PASS(1)
*PNV  igt@gem_fence_thrash@bo-write-verify-y      PASS(2)      FAIL(1)PASS(1)
 PNV  igt@gem_tiled_pread_pwrite      FAIL(3)PASS(2)      FAIL(2)
 PNV  igt@gen3_render_tiledx_blits      FAIL(3)PASS(1)      FAIL(2)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 9/9] drm: Fix the 'native defer' message in drm_dp_i2c_do_msg()

[Paulo Zanoni]

2015-03-31 14:15 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> The debug message is missing a newline at the end and it makes the
> logs hard to read when a device defers a lot. Simple 2-character fix
> adds the newline at the end.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> Cc: dri-devel@lists.freedesktop.org

Why in some logs there is in fact a newline, such as here:
https://bugs.freedesktop.org/attachment.cgi?id=110049 ?

Anyway, it looks correct, so:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/drm_dp_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 0539758..281bb67 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -433,7 +433,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>                         return -EREMOTEIO;
>
>                 case DP_AUX_NATIVE_REPLY_DEFER:
> -                       DRM_DEBUG_KMS("native defer");
> +                       DRM_DEBUG_KMS("native defer\n");
>                         /*
>                          * We could check for I2C bit rate capabilities and if
>                          * available adjust this interval. We could also be
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 9/9] drm: Fix the 'native defer' message in drm_dp_i2c_do_msg()

[Todd Previte]

The debug message is missing a newline at the end and it makes the
logs hard to read when a device defers a lot. Simple 2-character fix
adds the newline at the end.

Signed-off-by: Todd Previte <tprevite@gmail.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_dp_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 0539758..281bb67 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -433,7 +433,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 			return -EREMOTEIO;
 
 		case DP_AUX_NATIVE_REPLY_DEFER:
-			DRM_DEBUG_KMS("native defer");
+			DRM_DEBUG_KMS("native defer\n");
 			/*
 			 * We could check for I2C bit rate capabilities and if
 			 * available adjust this interval. We could also be
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel