* [PATCH 1/9] drm/i915: Add automated testing support for Displayport compliance testing
2015-03-31 17:14 [intel-gfx][PATCH V4] Displayport compliance testing V4 Todd Previte
@ 2015-03-31 17:14 ` Todd Previte
2015-04-07 0:10 ` Paulo Zanoni
2015-03-31 17:14 ` [PATCH 2/9] drm/i915: Update intel_dp_check_link_status() " Todd Previte
` (7 subsequent siblings)
8 siblings, 1 reply; 44+ messages in thread
From: Todd Previte @ 2015-03-31 17:14 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH 1/9] drm/i915: Add automated testing support for Displayport compliance testing
2015-03-31 17:14 ` [PATCH 1/9] drm/i915: Add automated testing support for Displayport compliance testing Todd Previte
@ 2015-04-07 0:10 ` Paulo Zanoni
2015-04-07 2:15 ` Todd Previte
2015-04-08 15:35 ` Todd Previte
0 siblings, 2 replies; 44+ messages in thread
From: Paulo Zanoni @ 2015-04-07 0:10 UTC (permalink / raw)
To: Todd Previte; +Cc: Intel Graphics Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/9] drm/i915: Add automated testing support for Displayport compliance testing
2015-04-07 0:10 ` Paulo Zanoni
@ 2015-04-07 2:15 ` Todd Previte
2015-04-08 15:35 ` Todd Previte
1 sibling, 0 replies; 44+ messages in thread
From: Todd Previte @ 2015-04-07 2:15 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/9] drm/i915: Add automated testing support for Displayport compliance testing
2015-04-07 0:10 ` Paulo Zanoni
2015-04-07 2:15 ` Todd Previte
@ 2015-04-08 15:35 ` Todd Previte
1 sibling, 0 replies; 44+ messages in thread
From: Todd Previte @ 2015-04-08 15:35 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 2/9] drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing
2015-03-31 17:14 [intel-gfx][PATCH V4] Displayport compliance testing V4 Todd Previte
2015-03-31 17:14 ` [PATCH 1/9] drm/i915: Add automated testing support for Displayport compliance testing Todd Previte
@ 2015-03-31 17:14 ` Todd Previte
2015-03-31 17:15 ` [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for " Todd Previte
` (6 subsequent siblings)
8 siblings, 0 replies; 44+ messages in thread
From: Todd Previte @ 2015-03-31 17:14 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
2015-03-31 17:14 [intel-gfx][PATCH V4] Displayport compliance testing V4 Todd Previte
2015-03-31 17:14 ` [PATCH 1/9] drm/i915: Add automated testing support for Displayport compliance testing Todd Previte
2015-03-31 17:14 ` [PATCH 2/9] drm/i915: Update intel_dp_check_link_status() " Todd Previte
@ 2015-03-31 17:15 ` Todd Previte
2015-04-01 18:23 ` Ville Syrjälä
` (2 more replies)
2015-03-31 17:15 ` [PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport " Todd Previte
` (5 subsequent siblings)
8 siblings, 3 replies; 44+ messages in thread
From: Todd Previte @ 2015-03-31 17:15 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
2015-03-31 17:15 ` [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for " Todd Previte
@ 2015-04-01 18:23 ` Ville Syrjälä
2015-04-01 18:55 ` Todd Previte
2015-04-03 16:08 ` [PATCH 03/11] " Todd Previte
2015-04-07 2:09 ` Todd Previte
2 siblings, 1 reply; 44+ messages in thread
From: Ville Syrjälä @ 2015-04-01 18:23 UTC (permalink / raw)
To: Todd Previte; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
2015-04-01 18:23 ` Ville Syrjälä
@ 2015-04-01 18:55 ` Todd Previte
2015-04-01 19:22 ` Ville Syrjälä
0 siblings, 1 reply; 44+ messages in thread
From: Todd Previte @ 2015-04-01 18:55 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
2015-04-01 18:55 ` Todd Previte
@ 2015-04-01 19:22 ` Ville Syrjälä
2015-04-01 19:45 ` Todd Previte
0 siblings, 1 reply; 44+ messages in thread
From: Ville Syrjälä @ 2015-04-01 19:22 UTC (permalink / raw)
To: Todd Previte; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
2015-04-01 19:22 ` Ville Syrjälä
@ 2015-04-01 19:45 ` Todd Previte
2015-04-06 23:28 ` Paulo Zanoni
0 siblings, 1 reply; 44+ messages in thread
From: Todd Previte @ 2015-04-01 19:45 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
2015-04-01 19:45 ` Todd Previte
@ 2015-04-06 23:28 ` Paulo Zanoni
2015-04-07 2:07 ` Todd Previte
0 siblings, 1 reply; 44+ messages in thread
From: Paulo Zanoni @ 2015-04-06 23:28 UTC (permalink / raw)
To: Todd Previte; +Cc: Intel Graphics Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
2015-04-06 23:28 ` Paulo Zanoni
@ 2015-04-07 2:07 ` Todd Previte
0 siblings, 0 replies; 44+ messages in thread
From: Todd Previte @ 2015-04-07 2:07 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 03/11] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
2015-03-31 17:15 ` [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for " Todd Previte
2015-04-01 18:23 ` Ville Syrjälä
@ 2015-04-03 16:08 ` Todd Previte
2015-04-07 2:09 ` Todd Previte
2 siblings, 0 replies; 44+ messages in thread
From: Todd Previte @ 2015-04-03 16:08 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 03/11] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
2015-03-31 17:15 ` [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for " Todd Previte
2015-04-01 18:23 ` Ville Syrjälä
2015-04-03 16:08 ` [PATCH 03/11] " Todd Previte
@ 2015-04-07 2:09 ` Todd Previte
2015-04-07 13:57 ` Paulo Zanoni
2 siblings, 1 reply; 44+ messages in thread
From: Todd Previte @ 2015-04-07 2:09 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH 03/11] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
2015-04-07 2:09 ` Todd Previte
@ 2015-04-07 13:57 ` Paulo Zanoni
2015-04-09 18:49 ` Todd Previte
0 siblings, 1 reply; 44+ messages in thread
From: Paulo Zanoni @ 2015-04-07 13:57 UTC (permalink / raw)
To: Todd Previte; +Cc: Intel Graphics Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 03/11] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
2015-04-07 13:57 ` Paulo Zanoni
@ 2015-04-09 18:49 ` Todd Previte
0 siblings, 0 replies; 44+ messages in thread
From: Todd Previte @ 2015-04-09 18:49 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing
2015-03-31 17:14 [intel-gfx][PATCH V4] Displayport compliance testing V4 Todd Previte
` (2 preceding siblings ...)
2015-03-31 17:15 ` [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for " Todd Previte
@ 2015-03-31 17:15 ` Todd Previte
2015-04-08 16:51 ` [Intel-gfx] " Paulo Zanoni
2015-03-31 17:15 ` [PATCH 5/9] drm/i915: Update the EDID automated compliance test function Todd Previte
` (4 subsequent siblings)
8 siblings, 1 reply; 44+ messages in thread
From: Todd Previte @ 2015-03-31 17:15 UTC (permalink / raw)
To: intel-gfx; +Cc: dri-devel
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [Intel-gfx] [PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing
2015-03-31 17:15 ` [PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport " Todd Previte
@ 2015-04-08 16:51 ` Paulo Zanoni
2015-04-08 21:43 ` Todd Previte
0 siblings, 1 reply; 44+ messages in thread
From: Paulo Zanoni @ 2015-04-08 16:51 UTC (permalink / raw)
To: Todd Previte; +Cc: Intel Graphics Development, DRI Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing
2015-04-08 16:51 ` [Intel-gfx] " Paulo Zanoni
@ 2015-04-08 21:43 ` Todd Previte
2015-04-08 22:37 ` Paulo Zanoni
0 siblings, 1 reply; 44+ messages in thread
From: Todd Previte @ 2015-04-08 21:43 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development, DRI Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing
2015-04-08 21:43 ` Todd Previte
@ 2015-04-08 22:37 ` Paulo Zanoni
2015-04-10 14:44 ` Todd Previte
0 siblings, 1 reply; 44+ messages in thread
From: Paulo Zanoni @ 2015-04-08 22:37 UTC (permalink / raw)
To: Todd Previte; +Cc: Intel Graphics Development, DRI Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport compliance testing
2015-04-08 22:37 ` Paulo Zanoni
@ 2015-04-10 14:44 ` Todd Previte
0 siblings, 0 replies; 44+ messages in thread
From: Todd Previte @ 2015-04-10 14:44 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development, DRI Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 5/9] drm/i915: Update the EDID automated compliance test function
2015-03-31 17:14 [intel-gfx][PATCH V4] Displayport compliance testing V4 Todd Previte
` (3 preceding siblings ...)
2015-03-31 17:15 ` [PATCH 4/9] drm/i915: Add check for corrupt raw EDID header for Displayport " Todd Previte
@ 2015-03-31 17:15 ` Todd Previte
2015-04-08 17:02 ` Paulo Zanoni
2015-03-31 17:15 ` [PATCH 6/9] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation Todd Previte
` (3 subsequent siblings)
8 siblings, 1 reply; 44+ messages in thread
From: Todd Previte @ 2015-03-31 17:15 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH 5/9] drm/i915: Update the EDID automated compliance test function
2015-03-31 17:15 ` [PATCH 5/9] drm/i915: Update the EDID automated compliance test function Todd Previte
@ 2015-04-08 17:02 ` Paulo Zanoni
2015-04-09 21:36 ` Todd Previte
0 siblings, 1 reply; 44+ messages in thread
From: Paulo Zanoni @ 2015-04-08 17:02 UTC (permalink / raw)
To: Todd Previte; +Cc: Intel Graphics Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/9] drm/i915: Update the EDID automated compliance test function
2015-04-08 17:02 ` Paulo Zanoni
@ 2015-04-09 21:36 ` Todd Previte
0 siblings, 0 replies; 44+ messages in thread
From: Todd Previte @ 2015-04-09 21:36 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 6/9] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation
2015-03-31 17:14 [intel-gfx][PATCH V4] Displayport compliance testing V4 Todd Previte
` (4 preceding siblings ...)
2015-03-31 17:15 ` [PATCH 5/9] drm/i915: Update the EDID automated compliance test function Todd Previte
@ 2015-03-31 17:15 ` Todd Previte
2015-04-01 17:52 ` [PATCH 1/5] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling Todd Previte
2015-04-01 18:00 ` [PATCH 06/11] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation Todd Previte
2015-03-31 17:15 ` [PATCH 7/9] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling Todd Previte
` (2 subsequent siblings)
8 siblings, 2 replies; 44+ messages in thread
From: Todd Previte @ 2015-03-31 17:15 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 1/5] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling
2015-03-31 17:15 ` [PATCH 6/9] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation Todd Previte
@ 2015-04-01 17:52 ` Todd Previte
2015-04-01 18:15 ` Ville Syrjälä
2015-04-01 18:00 ` [PATCH 06/11] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation Todd Previte
1 sibling, 1 reply; 44+ messages in thread
From: Todd Previte @ 2015-04-01 17:52 UTC (permalink / raw)
To: tprevite; +Cc: dri-devel
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH 1/5] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling
2015-04-01 17:52 ` [PATCH 1/5] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling Todd Previte
@ 2015-04-01 18:15 ` Ville Syrjälä
0 siblings, 0 replies; 44+ messages in thread
From: Ville Syrjälä @ 2015-04-01 18:15 UTC (permalink / raw)
To: Todd Previte; +Cc: dri-devel
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 06/11] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation
2015-03-31 17:15 ` [PATCH 6/9] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation Todd Previte
2015-04-01 17:52 ` [PATCH 1/5] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling Todd Previte
@ 2015-04-01 18:00 ` Todd Previte
2015-04-08 18:53 ` Paulo Zanoni
1 sibling, 1 reply; 44+ messages in thread
From: Todd Previte @ 2015-04-01 18:00 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH 06/11] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation
2015-04-01 18:00 ` [PATCH 06/11] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation Todd Previte
@ 2015-04-08 18:53 ` Paulo Zanoni
2015-04-09 21:35 ` Todd Previte
0 siblings, 1 reply; 44+ messages in thread
From: Paulo Zanoni @ 2015-04-08 18:53 UTC (permalink / raw)
To: Todd Previte; +Cc: Intel Graphics Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 06/11] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation
2015-04-08 18:53 ` Paulo Zanoni
@ 2015-04-09 21:35 ` Todd Previte
0 siblings, 0 replies; 44+ messages in thread
From: Todd Previte @ 2015-04-09 21:35 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 7/9] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling
2015-03-31 17:14 [intel-gfx][PATCH V4] Displayport compliance testing V4 Todd Previte
` (5 preceding siblings ...)
2015-03-31 17:15 ` [PATCH 6/9] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation Todd Previte
@ 2015-03-31 17:15 ` Todd Previte
2015-04-07 0:05 ` Paulo Zanoni
2015-04-07 2:11 ` [PATCH 07/11] " Todd Previte
2015-03-31 17:15 ` [PATCH 8/9] drm/i915: Add debugfs test control files for Displayport compliance testing Todd Previte
2015-03-31 17:15 ` [PATCH 9/9] drm: Fix the 'native defer' message in drm_dp_i2c_do_msg() Todd Previte
8 siblings, 2 replies; 44+ messages in thread
From: Todd Previte @ 2015-03-31 17:15 UTC (permalink / raw)
To: intel-gfx; +Cc: dri-devel
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH 7/9] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling
2015-03-31 17:15 ` [PATCH 7/9] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling Todd Previte
@ 2015-04-07 0:05 ` Paulo Zanoni
2015-04-07 1:21 ` Todd Previte
2015-04-07 2:11 ` [PATCH 07/11] " Todd Previte
1 sibling, 1 reply; 44+ messages in thread
From: Paulo Zanoni @ 2015-04-07 0:05 UTC (permalink / raw)
To: Todd Previte; +Cc: Intel Graphics Development, DRI Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 7/9] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling
2015-04-07 0:05 ` Paulo Zanoni
@ 2015-04-07 1:21 ` Todd Previte
0 siblings, 0 replies; 44+ messages in thread
From: Todd Previte @ 2015-04-07 1:21 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development, DRI Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 07/11] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling
2015-03-31 17:15 ` [PATCH 7/9] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling Todd Previte
2015-04-07 0:05 ` Paulo Zanoni
@ 2015-04-07 2:11 ` Todd Previte
2015-04-07 14:29 ` Paulo Zanoni
1 sibling, 1 reply; 44+ messages in thread
From: Todd Previte @ 2015-04-07 2:11 UTC (permalink / raw)
To: intel-gfx; +Cc: dri-devel
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH 07/11] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling
2015-04-07 2:11 ` [PATCH 07/11] " Todd Previte
@ 2015-04-07 14:29 ` Paulo Zanoni
2015-04-07 14:47 ` Ville Syrjälä
2015-04-07 18:47 ` Todd Previte
0 siblings, 2 replies; 44+ messages in thread
From: Paulo Zanoni @ 2015-04-07 14:29 UTC (permalink / raw)
To: Todd Previte; +Cc: Intel Graphics Development, DRI Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 07/11] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling
2015-04-07 14:29 ` Paulo Zanoni
@ 2015-04-07 14:47 ` Ville Syrjälä
2015-04-07 18:47 ` Todd Previte
1 sibling, 0 replies; 44+ messages in thread
From: Ville Syrjälä @ 2015-04-07 14:47 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development, DRI Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 07/11] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling
2015-04-07 14:29 ` Paulo Zanoni
2015-04-07 14:47 ` Ville Syrjälä
@ 2015-04-07 18:47 ` Todd Previte
1 sibling, 0 replies; 44+ messages in thread
From: Todd Previte @ 2015-04-07 18:47 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development, DRI Development
[-- 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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 8/9] drm/i915: Add debugfs test control files for Displayport compliance testing
2015-03-31 17:14 [intel-gfx][PATCH V4] Displayport compliance testing V4 Todd Previte
` (6 preceding siblings ...)
2015-03-31 17:15 ` [PATCH 7/9] drm/i915: Fix for DP CTS test 4.2.2.5 - I2C DEFER handling Todd Previte
@ 2015-03-31 17:15 ` Todd Previte
2015-04-01 18:12 ` [PATCH 08/11] " Todd Previte
2015-03-31 17:15 ` [PATCH 9/9] drm: Fix the 'native defer' message in drm_dp_i2c_do_msg() Todd Previte
8 siblings, 1 reply; 44+ messages in thread
From: Todd Previte @ 2015-03-31 17:15 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 08/11] drm/i915: Add debugfs test control files for Displayport compliance testing
2015-03-31 17:15 ` [PATCH 8/9] drm/i915: Add debugfs test control files for Displayport compliance testing Todd Previte
@ 2015-04-01 18:12 ` Todd Previte
0 siblings, 0 replies; 44+ messages in thread
From: Todd Previte @ 2015-04-01 18:12 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH 9/9] drm: Fix the 'native defer' message in drm_dp_i2c_do_msg()
2015-03-31 17:14 [intel-gfx][PATCH V4] Displayport compliance testing V4 Todd Previte
` (7 preceding siblings ...)
2015-03-31 17:15 ` [PATCH 8/9] drm/i915: Add debugfs test control files for Displayport compliance testing Todd Previte
@ 2015-03-31 17:15 ` Todd Previte
2015-04-01 4:45 ` shuang.he
2015-04-06 21:16 ` [Intel-gfx] " Paulo Zanoni
8 siblings, 2 replies; 44+ messages in thread
From: Todd Previte @ 2015-03-31 17:15 UTC (permalink / raw)
To: intel-gfx; +Cc: dri-devel
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH 9/9] drm: Fix the 'native defer' message in drm_dp_i2c_do_msg()
2015-03-31 17:15 ` [PATCH 9/9] drm: Fix the 'native defer' message in drm_dp_i2c_do_msg() Todd Previte
@ 2015-04-01 4:45 ` shuang.he
2015-04-06 21:16 ` [Intel-gfx] " Paulo Zanoni
1 sibling, 0 replies; 44+ messages in thread
From: shuang.he @ 2015-04-01 4:45 UTC (permalink / raw)
To: shuang.he, ethan.gao, intel-gfx, tprevite
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Intel-gfx] [PATCH 9/9] drm: Fix the 'native defer' message in drm_dp_i2c_do_msg()
2015-03-31 17:15 ` [PATCH 9/9] drm: Fix the 'native defer' message in drm_dp_i2c_do_msg() Todd Previte
2015-04-01 4:45 ` shuang.he
@ 2015-04-06 21:16 ` Paulo Zanoni
1 sibling, 0 replies; 44+ messages in thread
From: Paulo Zanoni @ 2015-04-06 21:16 UTC (permalink / raw)
To: Todd Previte; +Cc: Intel Graphics Development, DRI Development
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
^ permalink raw reply [flat|nested] 44+ messages in thread