Displayport compliance testing
Subsystems (3)
PATCH v2 1/1 Displayport compliance testing
Quoted 6 lines
This patch set adds the foundational support for Displayport compliance testing in the i915 driver. It implements the framework for automated test support that preclude the need (most) for operator input during testing. Tests for AUX transactions, EDID reads and basic link training have also been included, along with any support and utility functions required. Some changes have also been made to existing code to accommodate compliance testing.
V2:
- Addressed review feedback from the mailing list
- Broke up patches into smaller, easily managed chunks
- Reordered the patches such that they can be applied in order
- Fixed checkpatch.pl errors across the patchset
- Updated and enhanced functionality for the EDID test function
- Completely revamped the mode set operations for compliance testing
PATCH 1/12 drm/i915: Add automated testing support for Displayport compliance testing
Add the skeleton framework for supporting automation for Displayport compliance testing. This patch adds the necessary framework for the source device to appropriately responded to test automation requests from a sink device.
--- drivers/gpu/drm/i915/intel_dp.c | 84 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +82 -2
@@ -3328,11 +3328,91 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) sink_irq_vector, 1) == 1; } +/* Displayport compliance testing - Link training */ +static uint8_t +intel_dp_autotest_link_training (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +/* Displayport compliance testing - Video pattern testing */ +static uint8_t +intel_dp_autotest_video_pattern (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +/* Displayport compliance testing - EDID operations */ +static uint8_t +intel_dp_autotest_edid (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +/* Displayport compliance testing - PHY pattern testing */ +static uint8_t +intel_dp_autotest_phy_pattern (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +/* Displayport compliance testing - Fast AUX transactions (optional) */ +static uint8_t +intel_dp_autotest_faux_pattern (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +DRM_DEBUG_KMS ("Displayport: Fast AUX (FAUX) not supported\n" ); +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 ret =0 ; + +DRM_DEBUG_KMS ("Displayport: Received automated test request\n" ); +/* Read DP_TEST_REQUEST register to identify the requested test */ + ret =drm_dp_dpcd_read (&intel_dp->aux,DP_TEST_REQUEST , &rxdata,1 ); + +/* Determine which test has been requested */ +switch (rxdata) { +/* ACK/NAK response based on test function response +Unimplemented/unsupported tests will NAK */ +case DP_TEST_LINK_TRAINING : +DRM_DEBUG_KMS ("Displayport: Executing LINK_TRAINING request\n" ); + response =intel_dp_autotest_link_training (intel_dp); +break ; +case DP_TEST_LINK_VIDEO_PATTERN : +DRM_DEBUG_KMS ("Displayport: Executing TEST_PATTERN request\n" ); + response =intel_dp_autotest_video_pattern (intel_dp); +break ; +case DP_TEST_LINK_EDID_READ : +DRM_DEBUG_KMS ("Displayport: Executing EDID request\n" ); + response =intel_dp_autotest_edid (intel_dp); +break ; +case DP_TEST_LINK_PHY_TEST_PATTERN : +DRM_DEBUG_KMS ("Displayport: Executing PHY_PATTERN request\n" ); + response =intel_dp_autotest_phy_pattern (intel_dp); +break ; +/* FAUX is optional in DP 1.2*/ +case DP_TEST_LINK_FAUX_PATTERN : +DRM_DEBUG_KMS ("Displayport: Executing FAUX_PATTERN request\n" ); + response =intel_dp_autotest_faux_pattern (intel_dp); +break ; +/* Unsupported test case or something went wrong */ +default : +/* Log error here for unhandled test request */ +DRM_DEBUG_KMS ("Displayport: Unhandled test request\n" ); +break ; + } +/* Send the response from the test result */ + ret =drm_dp_dpcd_write (&intel_dp->aux,DP_TEST_RESPONSE , &response,1 ); }/*
Signature · 1 line
1.9.1
Re: [PATCH 01/12] drm/i915: Add automated testing support for Displayport compliance testing
Hi
2014-07-14 16:10 GMT-03:00 Todd Previte <[email protected]>:
Quoted 3 lines
Add the skeleton framework for supporting automation for Displayport compliance testing. This patch adds the necessary framework for the source device to appropriately responded to test automation requests from a sink device.
These commit messages contain some long lines. Most of the patches I see go up to 70 or at most 80 columns on each line on the commit message, but your patches seem to have a 100-column limit.
Quoted 11 lines
Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 84 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/intel_dp.c +0
-0
drivers/gpu/drm/i915/intel_dp.c intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) · line 3331
Quoted 4 lines
drivers/gpu/drm/i915/intel_dp.c +1
-0@@ -3328,11 +3328,91 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) sink_irq_vector, 1) == 1; } +/* Displayport compliance testing - Link training */
Does the line above contain the formal name of the test from a given specification, or is it just a description? If it's the exact name of the test, then please state that it is the name, and also name the specification it comes from. But if it's just a description, I don't really see the value of a comment containing "DP compliance testing - link training", when the function is already called intel_dp_autotest_link_training(): that would be just duplicate information.
In your patch series I see many other examples of comments that don't add any new information. For example, on patch 7 we have:
/* Reset the NACK/DEFER counters */
intel_dp->aux.i2c_nack_count = 0;
intel_dp->aux.i2c_defer_count = 0;You don't really need to say you're about to reset a counter if the code is already saying that. Also, if someone ever changes the code but forgets to fix the comment, you will have an inconsistency that will lead code readers confused.
But think about the good side: if the comments are useless, it's because the code is readable :). And that should be the goal: code that is clear enough to not really need comments. Please read Documentation/CodingStyle, chapter 8 inside the Kernel source tree.
For this and the next patches of the series, I won't go and list every single comment I think should be removed, but keep in mind that IMHO 95% of them could be removed.
Quoted 27 lines
drivers/gpu/drm/i915/intel_dp.c +27
-0+static uint8_t +intel_dp_autotest_link_training (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +/* Displayport compliance testing - Video pattern testing */ +static uint8_t +intel_dp_autotest_video_pattern (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +/* Displayport compliance testing - EDID operations */ +static uint8_t +intel_dp_autotest_edid (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +/* Displayport compliance testing - PHY pattern testing */ +static uint8_t +intel_dp_autotest_phy_pattern (struct intel_dp *intel_dp) +{
drivers/gpu/drm/i915/intel_dp.c intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) · line 3368
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10
-0+uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +/* Displayport compliance testing - Fast AUX transactions (optional) */ +static uint8_t +intel_dp_autotest_faux_pattern (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +DRM_DEBUG_KMS ("Displayport: Fast AUX (FAUX) not supported\n" );
Why do we print this here and not for the other tests?
drivers/gpu/drm/i915/intel_dp.c intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) · line 3376
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +5
-2+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 ;
I would probably have named this "requested_test" or something else.
drivers/gpu/drm/i915/intel_dp.c intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) · line 3377
Quoted 1 line
drivers/gpu/drm/i915/intel_dp.c +1
-0+int ret =0 ;
Vairable "ret" is set but never read anywhere. If you edit drivers/gpu/drm/i915/Makefile, you can get a compiler warning:
drivers/gpu/drm/i915/intel_dp.c: In function ‘intel_dp_handle_test_request’:drivers/gpu/drm/i915/intel_dp.c:3467:6: warning: variable ‘ret’ set but not used [-Wunused-but-set-variable]
You will also see that there are a few more points of our driver where we make similar mistakes :)
drivers/gpu/drm/i915/intel_dp.c intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) · line 3388
Quoted 11 lines
drivers/gpu/drm/i915/intel_dp.c +11
-0+ +DRM_DEBUG_KMS ("Displayport: Received automated test request\n" ); +/* Read DP_TEST_REQUEST register to identify the requested test */ + ret =drm_dp_dpcd_read (&intel_dp->aux,DP_TEST_REQUEST , &rxdata,1 ); + +/* Determine which test has been requested */ +switch (rxdata) { +/* ACK/NAK response based on test function response +Unimplemented/unsupported tests will NAK */ +case DP_TEST_LINK_TRAINING : +DRM_DEBUG_KMS ("Displayport: Executing LINK_TRAINING request\n" );
We're kinda lying here, since we're still NAKing stuff, aren't we?
Thanks,
Paulo
Quoted 37 lines
drivers/gpu/drm/i915/intel_dp.c +27
-1+ response =intel_dp_autotest_link_training (intel_dp); +break ; +case DP_TEST_LINK_VIDEO_PATTERN : +DRM_DEBUG_KMS ("Displayport: Executing TEST_PATTERN request\n" ); + response =intel_dp_autotest_video_pattern (intel_dp); +break ; +case DP_TEST_LINK_EDID_READ : +DRM_DEBUG_KMS ("Displayport: Executing EDID request\n" ); + response =intel_dp_autotest_edid (intel_dp); +break ; +case DP_TEST_LINK_PHY_TEST_PATTERN : +DRM_DEBUG_KMS ("Displayport: Executing PHY_PATTERN request\n" ); + response =intel_dp_autotest_phy_pattern (intel_dp); +break ; +/* FAUX is optional in DP 1.2*/ +case DP_TEST_LINK_FAUX_PATTERN : +DRM_DEBUG_KMS ("Displayport: Executing FAUX_PATTERN request\n" ); + response =intel_dp_autotest_faux_pattern (intel_dp); +break ; +/* Unsupported test case or something went wrong */ +default : +/* Log error here for unhandled test request */ +DRM_DEBUG_KMS ("Displayport: Unhandled test request\n" ); +break ; + } +/* Send the response from the test result */ + ret =drm_dp_dpcd_write (&intel_dp->aux,DP_TEST_RESPONSE , &response,1 ); }/* -- 1.9.1
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 5 lines
Paulo Zanoni _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 01/12] drm/i915: Add automated testing support for Displayport compliance testing
In reply to Todd Previte2014-07-14 16:10 GMT-03:00 Todd Previte <[email protected]>:
Quoted 94 lines
Add the skeleton framework for supporting automation for Displayport compliance testing. This patch adds the necessary framework for the source device to appropriately responded to test automation requests from a sink device.
Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 84 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/intel_dp.c +72
-2@@ -3328,11 +3328,91 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) sink_irq_vector, 1) == 1; } +/* Displayport compliance testing - Link training */ +static uint8_t +intel_dp_autotest_link_training (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +/* Displayport compliance testing - Video pattern testing */ +static uint8_t +intel_dp_autotest_video_pattern (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +/* Displayport compliance testing - EDID operations */ +static uint8_t +intel_dp_autotest_edid (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +/* Displayport compliance testing - PHY pattern testing */ +static uint8_t +intel_dp_autotest_phy_pattern (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +/* Displayport compliance testing - Fast AUX transactions (optional) */ +static uint8_t +intel_dp_autotest_faux_pattern (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +DRM_DEBUG_KMS ("Displayport: Fast AUX (FAUX) not supported\n" ); +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 ret =0 ; + +DRM_DEBUG_KMS ("Displayport: Received automated test request\n" ); +/* Read DP_TEST_REQUEST register to identify the requested test */ + ret =drm_dp_dpcd_read (&intel_dp->aux,DP_TEST_REQUEST , &rxdata,1 ); + +/* Determine which test has been requested */ +switch (rxdata) { +/* ACK/NAK response based on test function response +Unimplemented/unsupported tests will NAK */ +case DP_TEST_LINK_TRAINING : +DRM_DEBUG_KMS ("Displayport: Executing LINK_TRAINING request\n" ); + response =intel_dp_autotest_link_training (intel_dp); +break ; +case DP_TEST_LINK_VIDEO_PATTERN : +DRM_DEBUG_KMS ("Displayport: Executing TEST_PATTERN request\n" ); + response =intel_dp_autotest_video_pattern (intel_dp); +break ; +case DP_TEST_LINK_EDID_READ : +DRM_DEBUG_KMS ("Displayport: Executing EDID request\n" ); + response =intel_dp_autotest_edid (intel_dp); +break ; +case DP_TEST_LINK_PHY_TEST_PATTERN : +DRM_DEBUG_KMS ("Displayport: Executing PHY_PATTERN request\n" ); + response =intel_dp_autotest_phy_pattern (intel_dp); +break ; +/* FAUX is optional in DP 1.2*/ +case DP_TEST_LINK_FAUX_PATTERN : +DRM_DEBUG_KMS ("Displayport: Executing FAUX_PATTERN request\n" );
drivers/gpu/drm/i915/intel_dp.c intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) · line 3415
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10
-0+ response =intel_dp_autotest_faux_pattern (intel_dp); +break ; +/* Unsupported test case or something went wrong */ +default : +/* Log error here for unhandled test request */ +DRM_DEBUG_KMS ("Displayport: Unhandled test request\n" ); +break ; + } +/* Send the response from the test result */ + ret =drm_dp_dpcd_write (&intel_dp->aux,DP_TEST_RESPONSE , &response,1 );
Just one more idea: we could count amount of time passed during the test execution, since the spec defines some timeouts.
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +0
-1}/* -- 1.9.1
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Paulo Zanoni
PATCH 2/12 drm/i915: Add a function to compute the EDID checksum for Displayport compliance
In reply to Todd PreviteThis function computes the EDID checksum for a block of EDID data. This function is necessary for Displayport compliance testing as it does not not require the complete EDID checking functionality provided by the DRM layer functions.
--- drivers/gpu/drm/i915/intel_dp.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
drivers/gpu/drm/i915/intel_dp.c +23 -0
@@ -3328,6 +3328,29 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) sink_irq_vector, 1) == 1; } +static bool intel_dp_compute_edid_checksum (uint8_t *edid_data,uint8_t *edid_checksum) +{ +uint32_t byte_total =0 ; +uint8_t i =0 ; +bool edid_ok = true; + +/* Compute byte total w/o the checksum value*/ +for (i =0 ; i <EDID_LENGTH -2 ; i++) + byte_total += edid_data[i]; + +DRM_DEBUG_KMS ("Displayport: EDID total = %d, EDID checksum = %d\n" , byte_total, edid_data[EDID_LENGTH -1 ]); + +/* Compute the checksum */ + *edid_checksum =256 - (byte_total %256 ); + +if (*edid_checksum != edid_data[EDID_LENGTH -1 ]) { +DRM_DEBUG_KMS ("Displayport: Invalid EDID checksum %d, should be %d\n" , edid_data[EDID_LENGTH -1 ], *edid_checksum); + edid_ok = false; + } + +return edid_ok; +} +/* Displayport compliance testing - Link training */ static uint8_t intel_dp_autotest_link_training (struct intel_dp *intel_dp)
Signature · 1 line
1.9.1
Re: [PATCH 02/12] drm/i915: Add a function to compute the EDID checksum for Displayport compliance
2014-07-14 16:10 GMT-03:00 Todd Previte <[email protected]>:
Quoted 3 lines
This function computes the EDID checksum for a block of EDID data. This function is necessary for Displayport compliance testing as it does not not require the complete EDID checking functionality provided by the DRM layer functions.
Can you give specific reasons why you can't reuse drm_edid_block_valid(), or extract a piece of it to a new function that will be called by both the i915 code and drm_edid_block_valid()?
Quoted 11 lines
Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)drivers/gpu/drm/i915/intel_dp.c +0
-0
drivers/gpu/drm/i915/intel_dp.c intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) · line 3331
Quoted 4 lines
drivers/gpu/drm/i915/intel_dp.c +1
-0@@ -3328,6 +3328,29 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) sink_irq_vector, 1) == 1; } +static bool intel_dp_compute_edid_checksum (uint8_t *edid_data,uint8_t *edid_checksum)
A little note about patch-splitting: here you are adding a function that is not called anywhere. This is not a common thing, we usually only add a function on the patch where it gets called for the first time. It is much easier to review a function when it has callers, since you can know the context of why it is really needed, and see if it is being called correctly more easily. You can also judge if it needs to be static or exported. Also, adding functions without callers can produce compiler warnings, like this:
drivers/gpu/drm/i915/intel_dp.c:3421:13: warning:
‘intel_dp_compute_edid_checksum’ defined but not used [-Wunused-function]
You do the same thing with patch 4, 5 and 6. IMHO, you should squash all these patches on patch 7. No problem that patch 7 is going to get
big: it is still one logical change.
Another example of why I think this should be squashed: when I see patch 7, I don't see the need for the "edid_checksum" argument of this function. Even if you're going to use the argument later, it's better if you only patch the function to add the argument at the point you really need it.
drivers/gpu/drm/i915/intel_dp.c intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) · line 3339
Quoted 8 lines
drivers/gpu/drm/i915/intel_dp.c +8
-0+{ +uint32_t byte_total =0 ; +uint8_t i =0 ; +bool edid_ok = true; + +/* Compute byte total w/o the checksum value*/ +for (i =0 ; i <EDID_LENGTH -2 ; i++) + byte_total += edid_data[i];
Too many tabs here.
drivers/gpu/drm/i915/intel_dp.c intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) · line 3341
Quoted 2 lines
drivers/gpu/drm/i915/intel_dp.c +2
-0+ +DRM_DEBUG_KMS ("Displayport: EDID total = %d, EDID checksum = %d\n" , byte_total, edid_data[EDID_LENGTH -1 ]);
We allow a line to have more than 80 chars when the message being print is too big and you would need to cut it in the middle, but on this case you could have added a line break after the end of the string, leaving "bytes_total, edid_data[...]);" on the line below. This would have kept both lines smaller than the 80-column restriction, without breaking a greppable debug message. Also, keep in mind that DRM_DEBUG_KMS also prints the name of the function, so you probably don't need to print, for example, the "Displayport" characters. The same comment applies for the DRM_DEBUG_KMS call below.
Thanks,
Paulo
Quoted 22 lines
drivers/gpu/drm/i915/intel_dp.c +12
-1+ +/* Compute the checksum */ + *edid_checksum =256 - (byte_total %256 ); + +if (*edid_checksum != edid_data[EDID_LENGTH -1 ]) { +DRM_DEBUG_KMS ("Displayport: Invalid EDID checksum %d, should be %d\n" , edid_data[EDID_LENGTH -1 ], *edid_checksum); + edid_ok = false; + } + +return edid_ok; +} +/* Displayport compliance testing - Link training */ static uint8_t intel_dp_autotest_link_training (struct intel_dp *intel_dp) --1.9.1
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 5 lines
Paulo Zanoni _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 3/12 drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs
In reply to Todd PreviteThese counters are used for Displayort complinace testing to detect error conditions when executing certain compliance tests. Currently these are used in the EDID tests to determine if the video mode needs to be set to the preferred mode or the failsafe mode.
--- drivers/gpu/drm/drm_dp_helper.c | 2 ++ include/drm/drm_dp_helper.h | 1 + 2 files changed, 3 insertions(+)
drivers/gpu/drm/drm_dp_helper.c +2 -0
@@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)case DP_AUX_I2C_REPLY_NACK :DRM_DEBUG_KMS ("I2C nack\n" ); + aux->i2c_nack_count++;return -EREMOTEIO ;case DP_AUX_I2C_REPLY_DEFER :DRM_DEBUG_KMS ("I2C defer\n" ); + aux->i2c_defer_count++;usleep_range (400 ,500 );continue ;
include/drm/drm_dp_helper.h +1 -0
@@ -550,6 +550,7 @@ struct drm_dp_aux {struct mutex hw_mutex;ssize_t (*transfer)(struct drm_dp_aux *aux,struct drm_dp_aux_msg *msg); +uint8_t i2c_nack_count, i2c_defer_count; }; ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
Signature · 1 line
1.9.1
Re: [PATCH 03/12] drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs
2014-07-14 16:10 GMT-03:00 Todd Previte <[email protected]>:
Quoted 5 lines
These counters are used for Displayort complinace testing to detect error conditions when executing certain compliance tests. Currently these are used in the EDID tests to determine if the video mode needs to be set to the preferred mode or the failsafe mode.
It would be nice if you could cite on the commit message the name of the specification and the name of the test(s) that use it.
Usually when I have patches that touch things outside drivers/gpu/drm/i915, I add a Cc tag so I don't forget to email the appropriate list:
Quoted 28 lines
Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/drm_dp_helper.c | 2 ++ include/drm/drm_dp_helper.h | 1 + 2 files changed, 3 insertions(+)drivers/gpu/drm/drm_dp_helper.c +2
-0@@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)case DP_AUX_I2C_REPLY_NACK :DRM_DEBUG_KMS ("I2C nack\n" ); + aux->i2c_nack_count++;return -EREMOTEIO ;case DP_AUX_I2C_REPLY_DEFER :DRM_DEBUG_KMS ("I2C defer\n" ); + aux->i2c_defer_count++;usleep_range (400 ,500 );continue ;include/drm/drm_dp_helper.h +0
-0
include/drm/drm_dp_helper.h struct drm_dp_aux { · line 553
Quoted 4 lines
include/drm/drm_dp_helper.h +1
-0@@ -550,6 +550,7 @@ struct drm_dp_aux {struct mutex hw_mutex;ssize_t (*transfer)(struct drm_dp_aux *aux,struct drm_dp_aux_msg *msg); +uint8_t i2c_nack_count, i2c_defer_count;
Does it really need to be uint8_t? I see on patch 7 that you don't really write this value to a place that only accepts uint8_t-sized arguments, so I fear that if we get 256 NACKs or DEFERs we may end up doing the wrong thing.
Also, why don't we need to count the native NACKs and DEFERs?
Quoted 10 lines
include/drm/drm_dp_helper.h +0
-1}; ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset, --1.9.1
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Paulo Zanoni
PATCH 2/10 drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs
These counters are used for Displayort compliance testing to detect error conditions when executing tests 4.2.2.4 and 4.2.2.5 in the Displayport Link CTS specificaiton. They determine whether to use the preferred/requested mode or the failsafe mode during these tests.
V2:
- Addressed previous review feedback
- Updated commit message
- Changed from uint8_t to uint32_t
--- drivers/gpu/drm/drm_dp_helper.c | 2 ++ include/drm/drm_dp_helper.h | 1 + 2 files changed, 3 insertions(+)
drivers/gpu/drm/drm_dp_helper.c +2 -0
@@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)case DP_AUX_I2C_REPLY_NACK :DRM_DEBUG_KMS ("I2C nack\n" ); + aux->i2c_nack_count++;return -EREMOTEIO ;case DP_AUX_I2C_REPLY_DEFER :DRM_DEBUG_KMS ("I2C defer\n" ); + aux->i2c_defer_count++;usleep_range (400 ,500 );continue ;
include/drm/drm_dp_helper.h +1 -0
@@ -551,6 +551,7 @@ struct drm_dp_aux {struct mutex hw_mutex;ssize_t (*transfer)(struct drm_dp_aux *aux,struct drm_dp_aux_msg *msg); +uint32_t i2c_nack_count, i2c_defer_count; }; ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 02/10] drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs
Quoted 39 lines · Todd Previte
These counters are used for Displayort compliance testing to detect error conditions when executing tests 4.2.2.4 and 4.2.2.5 in the Displayport Link CTS specificaiton. They determine whether to use the preferred/requested mode or the failsafe mode during these tests.
V2:
- Addressed previous review feedback
- Updated commit message
- Changed fromuint8_ttouint32_tSigned-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/drm_dp_helper.c | 2 ++ include/drm/drm_dp_helper.h | 1 + 2 files changed, 3 insertions(+)drivers/gpu/drm/drm_dp_helper.c +2
-0@@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)case DP_AUX_I2C_REPLY_NACK :DRM_DEBUG_KMS ("I2C nack\n" ); + aux->i2c_nack_count++;return -EREMOTEIO ;case DP_AUX_I2C_REPLY_DEFER :DRM_DEBUG_KMS ("I2C defer\n" ); + aux->i2c_defer_count++;usleep_range (400 ,500 );continue ;include/drm/drm_dp_helper.h +0
-0
include/drm/drm_dp_helper.h struct drm_dp_aux { · line 554
Quoted 4 lines
include/drm/drm_dp_helper.h +1
-0@@ -551,6 +551,7 @@ struct drm_dp_aux {struct mutex hw_mutex;ssize_t (*transfer)(struct drm_dp_aux *aux,struct drm_dp_aux_msg *msg); +uint32_t i2c_nack_count, i2c_defer_count;
My reply missed your resend. I think unsigned instead of uint32_t has clearer intent - at least nearyb driver code uint32_t usually means a 32bit hw register value. Which this isn't.
I've bikeshedded this and picked this up into my drm-misc tree. -Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 4/12 drm/i915: Add wrapper function for intel_crtc_set_config()
In reply to Todd PreviteAdd a wrapper around intel_crtc_set_config() to allow it to be called from the DP compliance test functions. This is necessary to perform the internal mode set operations required for compliance testing.
--- drivers/gpu/drm/i915/intel_display.c | 6 ++++++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ 2 files changed, 8 insertions(+)
drivers/gpu/drm/i915/intel_display.c +6 -0
@@ -11249,6 +11249,12 @@ out_config:return ret; } +/* Wrapper function for setting a configuration for DP compliance testing */ +int intel_dp_set_config (struct drm_mode_set *set) +{ +return intel_crtc_set_config (set); +} +static const struct drm_crtc_funcs intel_crtc_funcs = { .gamma_set = intel_crtc_gamma_set, .set_config = intel_crtc_set_config,
drivers/gpu/drm/i915/intel_drv.h +2 -0
@@ -836,6 +836,8 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, struct intel_crtc_config *pipe_config);int intel_format_to_fourcc (int format);void intel_crtc_wait_for_pending_flips (struct drm_crtc *crtc); +/* Wrapper function for setting a configuration for DP compliance testing */ +int intel_dp_set_config (struct drm_mode_set *set);/* intel_dp.c */
Signature · 1 line
1.9.1
Re: [PATCH 04/12] drm/i915: Add wrapper function for intel_crtc_set_config()
2014-07-14 16:10 GMT-03:00 Todd Previte <[email protected]>:
Quoted 15 lines
Add a wrapper around
intel_crtc_set_config()to allow it to be called from the DP compliance test functions. This is necessary to perform the internal mode set operations required for compliance testing.Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 6 ++++++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ 2 files changed, 8 insertions(+)drivers/gpu/drm/i915/intel_display.c +0
-0
drivers/gpu/drm/i915/intel_display.c out_config: · line 11255
Quoted 7 lines
drivers/gpu/drm/i915/intel_display.c +4
-0@@ -11249,6 +11249,12 @@ out_config:return ret; } +/* Wrapper function for setting a configuration for DP compliance testing */ +int intel_dp_set_config (struct drm_mode_set *set) +{ +return intel_crtc_set_config (set);
Why not just export intel_crtc_set_config(), since the wrapper does nothing but call the wrapped thing? I see you don't change this function on the other patches.
Quoted 25 lines
drivers/gpu/drm/i915/intel_display.c +2
-0+} +static const struct drm_crtc_funcs intel_crtc_funcs = { .gamma_set = intel_crtc_gamma_set, .set_config = intel_crtc_set_config,drivers/gpu/drm/i915/intel_drv.h +2
-1@@ -836,6 +836,8 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode, struct intel_crtc_config *pipe_config);int intel_format_to_fourcc (int format);void intel_crtc_wait_for_pending_flips (struct drm_crtc *crtc); +/* Wrapper function for setting a configuration for DP compliance testing */ +int intel_dp_set_config (struct drm_mode_set *set);/* intel_dp.c */ --1.9.1
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Paulo Zanoni
PATCH 5/12 drm/i915: Add a function to get the EDID preferred mode for Displayport compliance testing
In reply to Todd PreviteAdd a simple function to pull the preferred mode out of an EDID block. This function is designed for use during Displayport compliance testing.
--- drivers/gpu/drm/i915/intel_dp.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
drivers/gpu/drm/i915/intel_dp.c +25 -0
@@ -3351,6 +3351,31 @@ static bool intel_dp_compute_edid_checksum(uint8_t *edid_data, uint8_t *edid_chereturn edid_ok; } +static struct drm_display_mode * +intel_dp_get_edid_preferred_mode (struct intel_dp *intel_dp) +{ +struct drm_display_mode *found_mode =NULL ; +struct drm_connector *connector = &intel_dp->attached_connector->base; +int mode_count =0 ; + +list_for_each_entry (found_mode, &connector->probed_modes, head) { +/* Check for a preferred mode */ +if (found_mode->type &DRM_MODE_TYPE_PREFERRED ) { +/* Found the preferred mode, return it */ +DRM_DEBUG_KMS ("Displayport: Found preferred mode '%s'\n" , + found_mode->name); + goto exit_with_mode; + } + mode_count++; + } +/* No mode found, report the error */ +DRM_DEBUG_KMS ("Displayport: Preferred mode not found in %d probed modes\n" , + mode_count); + +exit_with_mode: +return found_mode; +} +/* Displayport compliance testing - Link training */ static uint8_t intel_dp_autotest_link_training (struct intel_dp *intel_dp)
Signature · 1 line
1.9.1
Re: [PATCH 05/12] drm/i915: Add a function to get the EDID preferred mode for Displayport compliance testing
2014-07-14 16:10 GMT-03:00 Todd Previte <[email protected]>:
Quoted 3 lines
Add a simple function to pull the preferred mode out of an EDID block. This function is designed for use during Displayport compliance testing.
The preferred mode can be changed by the quirks. Do you think that could be a problem?
Also, as said earlier, this should be squashed in patch 7.
Quoted 10 lines
Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)drivers/gpu/drm/i915/intel_dp.c +0
-0
drivers/gpu/drm/i915/intel_dp.c static bool intel_dp_compute_edid_checksum(uint8_t *edid_data, uint8_t *edid_che · line 3354
Quoted 4 lines
drivers/gpu/drm/i915/intel_dp.c +1
-0@@ -3351,6 +3351,31 @@ static bool intel_dp_compute_edid_checksum(uint8_t *edid_data, uint8_t *edid_chereturn edid_ok; } +static struct drm_display_mode*
One of the inconsistencies with our driver is that we have both "return_type \n function_name" and "return_type function_name" (on the same line). I wish we were consistent. Anyway, I'd add a space between "_mode" and '*'
Quoted 34 lines
drivers/gpu/drm/i915/intel_dp.c +24
-1+intel_dp_get_edid_preferred_mode(struct intel_dp *intel_dp) +{ +struct drm_display_mode *found_mode =NULL ; +struct drm_connector *connector = &intel_dp->attached_connector->base; +int mode_count =0 ; + +list_for_each_entry (found_mode, &connector->probed_modes, head) { +/* Check for a preferred mode */ +if (found_mode->type &DRM_MODE_TYPE_PREFERRED ) { +/* Found the preferred mode, return it */ +DRM_DEBUG_KMS ("Displayport: Found preferred mode '%s'\n" , + found_mode->name); + goto exit_with_mode; + } + mode_count++; + } +/* No mode found, report the error */ +DRM_DEBUG_KMS ("Displayport: Preferred mode not found in %d probed modes\n" , + mode_count); + +exit_with_mode: +return found_mode; +} +/* Displayport compliance testing - Link training */ static uint8_t intel_dp_autotest_link_training (struct intel_dp *intel_dp) --1.9.1
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Paulo Zanoni
PATCH 6/12 drm/i915: Add a constant and function for getting the Displayport compliance failsafe video mode
In reply to Todd PreviteAdds the failsafe mode (640x480@60hz) as a constant and a function that retrieves it. These are designed for use in Displayport compliance testing only and should not be used outside that context.
--- drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
drivers/gpu/drm/i915/intel_dp.c +12 -0
@@ -82,6 +82,18 @@ static const struct dp_link_dpll chv_dpll[] = { { .p1 = 2, .p2 = 1, .n = 1, .m1 = 2, .m2 = 0x6c00000 } } }; +/* VESA 640x480@60Hz - DP compliance failsafe mode */ +static struct drm_display_mode dp_failsafe_mode = { +DRM_MODE ("640x480" ,DRM_MODE_TYPE_DRIVER ,25200 ,640 ,664 ,752 , +800 ,0 ,480 ,490 ,492 ,525 ,0 , +DRM_MODE_FLAG_NHSYNC |DRM_MODE_FLAG_NVSYNC ) +}; + +struct drm_display_mode *intel_dp_get_failsafe_mode (void ) +{ +return &dp_failsafe_mode; +} +/** * is_edp - is the given port attached to an eDP panel (either CPU or PCH) * @intel_dp: DP struct
Signature · 1 line
1.9.1
Re: [PATCH 06/12] drm/i915: Add a constant and function for getting the Displayport compliance failsafe video mode
2014-07-14 16:10 GMT-03:00 Todd Previte <[email protected]>:
Quoted 15 lines
Adds the failsafe mode (640x480@60hz) as a constant and a function that retrieves it. These are designed for use in Displayport compliance testing only and should not be used outside that context.
Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)drivers/gpu/drm/i915/intel_dp.c +0
-0@@ -82,6 +82,18 @@ static const struct dp_link_dpll chv_dpll[] = { { .p1 = 2, .p2 = 1, .n = 1, .m1 = 2, .m2 = 0x6c00000 } }
drivers/gpu/drm/i915/intel_dp.c static const struct dp_link_dpll chv_dpll[] = { · line 92
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +8
-0}; +/* VESA 640x480@60Hz - DP compliance failsafe mode */ +static struct drm_display_mode dp_failsafe_mode = { +DRM_MODE ("640x480" ,DRM_MODE_TYPE_DRIVER ,25200 ,640 ,664 ,752 , +800 ,0 ,480 ,490 ,492 ,525 ,0 , +DRM_MODE_FLAG_NHSYNC |DRM_MODE_FLAG_NVSYNC ) +}; + +struct drm_display_mode *intel_dp_get_failsafe_mode (void )
Since on patch 7 you call this from intel_dp.c, and you never add a forward declaration to any .h file, I guess this function should be static, right? Also, I'd squash it into patch 7.
Quoted 14 lines
drivers/gpu/drm/i915/intel_dp.c +4
-1+{ +return &dp_failsafe_mode; +} +/** * is_edp - is the given port attached to an eDP panel (either CPU or PCH) * @intel_dp: DP struct -- 1.9.1
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Paulo Zanoni
PATCH 7/12 drm/i915: Update EDID automated test function for Displayport compliance
In reply to Todd PreviteImplements an updated version of the automated testing function that handles Displayport compliance for EDID operations.
--- drivers/gpu/drm/i915/intel_dp.c | 77 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 76 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_dp.c +76 -1
@@ -3408,7 +3408,82 @@ intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)static uint8_t intel_dp_autotest_edid (struct intel_dp *intel_dp) { -uint8_t test_result =DP_TEST_NAK ; +struct drm_connector *connector = &intel_dp->attached_connector->base; +struct i2c_adapter *adapter = &intel_dp->aux.ddc; +struct edid *edid_read =NULL ; +uint8_t *edid_data =NULL ; +uint8_t test_result =DP_TEST_NAK , checksum =0 ; +uint32_t i =0 , ret =0 ; +struct drm_display_mode *use_mode =NULL ; +int mode_count =0 ; +struct drm_mode_set modeset; + +DRM_DEBUG_KMS ("Displayport: EDID automated test\n" ); + +/* Reset the NACK/DEFER counters */ + intel_dp->aux.i2c_nack_count =0 ; + intel_dp->aux.i2c_defer_count =0 ; +/* Now read out the EDID */ + edid_read =drm_get_edid (connector, adapter); + +if (edid_read ==NULL ) { +/* Check for NACKs/DEFERs, goto 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 ("Displayport: EDID read generated %d I2C NACKs, %d DEFERs\n" , + intel_dp->aux.i2c_nack_count, + intel_dp->aux.i2c_defer_count); + goto failsafe; + } + +/* FIXME: Need to determine how to detect E-DDC here (4.2.2.9) */ + edid_data = (uint8_t *) edid_read; + +if (intel_dp_compute_edid_checksum (edid_data, &checksum)) { +/* Write the checksum to EDID checksum register */ + ret =drm_dp_dpcd_write (&intel_dp->aux, +DP_TEST_EDID_CHECKSUM , + &edid_read->checksum,1 ); +/* Reponse is ACK and and checksum written */ + test_result =DP_TEST_ACK |DP_TEST_EDID_CHECKSUM_WRITE ; + }else { +/* Invalid checksum - EDID corruption detection test */ + goto failsafe; + } + +/* Update EDID modes */ + mode_count =intel_connector_update_modes (connector, edid_read); +if (!mode_count) { +DRM_DEBUG_KMS ("Displayport: Mode update failed\n" ); + goto failsafe; + } + +/* Get the EDID preferred mode if available */ + use_mode =intel_dp_get_edid_preferred_mode (intel_dp); +if (use_mode ==NULL ) + goto failsafe; +else + goto set_mode; + +failsafe: +DRM_DEBUG_KMS ("Displayport: Setting failsafe display mode\n" ); + use_mode =intel_dp_get_failsafe_mode (); +/* FIXME: <TAP> Need to set DP to 6bpc here as well */ + intel_dp->attached_connector->encoder->new_crtc->config.pipe_bpp =18 ; + +set_mode: +/* Configure the display mode necessary */ + modeset.connectors = &connector; + modeset.num_connectors =1 ; + modeset.crtc = connector->encoder->crtc; + modeset.fb = modeset.crtc->primary->fb; + modeset.x =0 ; + modeset.y =0 ; + modeset.mode = use_mode; +/* Set the config */ +intel_dp_set_config (&modeset); +return test_result; }
Signature · 1 line
1.9.1
Re: [PATCH 07/12] drm/i915: Update EDID automated test function for Displayport compliance
2014-07-14 16:10 GMT-03:00 Todd Previte <[email protected]>:
Quoted 2 lines
Implements an updated version of the automated testing function that handles Displayport compliance for EDID operations.
Both the commit message and the code should mention the name of the specification that defines these tests, and also mention which specific tests are implemented by this patch/function. I see that there are multiple tests being implemented here, but reading the 232 pages of the spec will require too much time, so knowing which ones are implemented really helps the reviewers :)
Also, you should tell us what happens before and after this patch when you run your own tests. How many tests were we previously passing? How many tests are we passing now? I see there are some FIXME lines below, which leads to the question: is the code you provided enough and complete, or do we still need more adjustments to pass everything we can with what you built? In other words: is this patch, alone, already an improvement to the situation?
Quoted 14 lines
Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 77 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 76 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_dp.c +0
-0@@ -3408,7 +3408,82 @@ intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)static uint8_t intel_dp_autotest_edid (struct intel_dp *intel_dp) {
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3419
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +9
-1-uint8_t test_result =DP_TEST_NAK ; +struct drm_connector *connector = &intel_dp->attached_connector->base; +struct i2c_adapter *adapter = &intel_dp->aux.ddc; +struct edid *edid_read =NULL ; +uint8_t *edid_data =NULL ; +uint8_t test_result =DP_TEST_NAK , checksum =0 ; +uint32_t i =0 , ret =0 ; +struct drm_display_mode *use_mode =NULL ; +int mode_count =0 ; +struct drm_mode_set modeset;
You have initialized every single variable you defined. This creates the problem that unused variables are not pointed by the compiler unless you enable the flags to warn set-but-not-used variables. For example, "i" is unused, and "ret" is set but never used. I also don't really see the point in, for example, NULL-initializing stuff like edid_data and edid_read.
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3423
Quoted 4 lines
drivers/gpu/drm/i915/intel_dp.c +4
-0+ +DRM_DEBUG_KMS ("Displayport: EDID automated test\n" ); + +/* Reset the NACK/DEFER counters */
As I said before, this is a great example of the "comment says what the code already says" problem.
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3431
Quoted 8 lines
drivers/gpu/drm/i915/intel_dp.c +8
-0+ intel_dp->aux.i2c_nack_count =0 ; + intel_dp->aux.i2c_defer_count =0 ; +/* Now read out the EDID */ + edid_read =drm_get_edid (connector, adapter); + +if (edid_read ==NULL ) { +/* Check for NACKs/DEFERs, goto failsafe if detected +(DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
Our coding standard is to put aligned '*'s on each line of a multi-line comment. So this should be "\t\t * (DP CTS 1.2 etc..." instead of what is above. In theory, the "/*" and "*/" strings should be on their own lines, alone, but we are inconsistent regarding this (even though Daniel randomly complained about this to me a few times in the past). As usual, check Documentation/CodingStyle for better explanations.
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3433
Quoted 2 lines
drivers/gpu/drm/i915/intel_dp.c +2
-0+if (intel_dp->aux.i2c_nack_count >0 || + intel_dp->aux.i2c_defer_count >0 )
We tend to align the contents of the extra line with the '(' char on the line above.
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3436
Quoted 3 lines
drivers/gpu/drm/i915/intel_dp.c +3
-0+DRM_DEBUG_KMS ("Displayport: EDID read generated %d I2C NACKs, %d DEFERs\n" , + intel_dp->aux.i2c_nack_count, + intel_dp->aux.i2c_defer_count);
There are way too many tabs on the 2 lines above.
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3440
Quoted 4 lines
drivers/gpu/drm/i915/intel_dp.c +4
-0+ goto failsafe; + } + +/* FIXME: Need to determine how to detect E-DDC here (4.2.2.9) */
But what is the problem with the current code? What are the consequences of not implementing the FIXME?
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3447
Quoted 7 lines
drivers/gpu/drm/i915/intel_dp.c +7
-0+ edid_data = (uint8_t *) edid_read; + +if (intel_dp_compute_edid_checksum (edid_data, &checksum)) { +/* Write the checksum to EDID checksum register */ + ret =drm_dp_dpcd_write (&intel_dp->aux, +DP_TEST_EDID_CHECKSUM , + &edid_read->checksum,1 );
Way too many tabs above too.
Quoted 15 lines
drivers/gpu/drm/i915/intel_dp.c +15
-0+/* Reponse is ACK and and checksum written */ + test_result =DP_TEST_ACK |DP_TEST_EDID_CHECKSUM_WRITE ; + }else { +/* Invalid checksum - EDID corruption detection test */ + goto failsafe; + } + +/* Update EDID modes */ + mode_count =intel_connector_update_modes (connector, edid_read); +if (!mode_count) { +DRM_DEBUG_KMS ("Displayport: Mode update failed\n" ); + goto failsafe; + } + +/* Get the EDID preferred mode if available */
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3472
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10
-0+ use_mode =intel_dp_get_edid_preferred_mode (intel_dp); +if (use_mode ==NULL ) + goto failsafe; +else + goto set_mode; + +failsafe: +DRM_DEBUG_KMS ("Displayport: Setting failsafe display mode\n" ); + use_mode =intel_dp_get_failsafe_mode (); +/* FIXME: <TAP> Need to set DP to 6bpc here as well */
What does <TAP> mean? What are the consequences of not setting DP to 6bpc here as well?
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3479
Quoted 7 lines
drivers/gpu/drm/i915/intel_dp.c +7
-0+ intel_dp->attached_connector->encoder->new_crtc->config.pipe_bpp =18 ; + +set_mode: +/* Configure the display mode necessary */ + modeset.connectors = &connector; + modeset.num_connectors =1 ; + modeset.crtc = connector->encoder->crtc;
I guess this could result in NULL pointer dereferences. I don't see a guarantee that the connector will have a CRTC at this point.
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3480
Quoted 1 line
drivers/gpu/drm/i915/intel_dp.c +1
-0+ modeset.fb = modeset.crtc->primary->fb;
And I also don't see a guarantee that we'll have an FB, and that the FB will be big enough for the current mode we're setting.
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3486
Quoted 6 lines
drivers/gpu/drm/i915/intel_dp.c +6
-0+ modeset.x =0 ; + modeset.y =0 ; + modeset.mode = use_mode; +/* Set the config */ +intel_dp_set_config (&modeset); +
I guess Daniel said this won't really work, right? This is why it will probably be simpler to involve user space on the compliance testing (in addition to the fact that we will test real-world production-used code). At this point we could just write to the debugfs file something like "set preferred mode" or "set failsafe mode", then wait for the modeset to happen. We could either try to detect the modeset (by setting some variable to zero before we send the message to user-space, and then verifying that intel_set_mode changed it to true) or we could make the user-space write a string to the debugfs file, like "done" and assume user-space is correct and finish the function (it's debugfs anyway, and root, and DRM master, right?).
Also, if we use the failsafe mode we'll return DP_TEST_NAK. Is that intentional? Also, it is a little strange that the ACK is the last thing we should do here, and the spec doesn't seem to be clear reagarding when we should send it (or I just didn't find the piece of text that says it...). I guess that by running the compliance tests we will discover that by looking at the test result.
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +0
-1return test_result; } --1.9.1
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Paulo Zanoni
Re: [PATCH 07/12] drm/i915: Update EDID automated test function for Displayport compliance
Quoted 8 lines
Paulo Zanoni <mailto:[email protected]>
Tuesday, July 29, 2014 3:37 PM
2014-07-14 16:10 GMT-03:00 Todd Previte<[email protected]>:Quoted 2 lines
Implements an updated version of the automated testing function that handles Displayport compliance for EDID operations.
Quoted 6 lines
Both the commit message and the code should mention the name of the specification that defines these tests, and also mention which specific tests are implemented by this patch/function. I see that there are multiple tests being implemented here, but reading the 232 pages of the spec will require too much time, so knowing which ones are implemented really helps the reviewers :)
Heh fair enough. I can put all the test information in the commit message. I think it's already in the code, but I'll double-check that as well.
Quoted 8 lines
Also, you should tell us what happens before and after this patch when you run your own tests. How many tests were we previously passing? How many tests are we passing now? I see there are some FIXME lines below, which leads to the question: is the code you provided enough and complete, or do we still need more adjustments to pass everything we can with what you built? In other words: is this patch, alone, already an improvement to the situation?
Makes sense. I'll add the additional info in the commit messages.
Quoted 23 lines
Quoted 23 lines
Signed-off-by Todd Previte<[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 77 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 76 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_dp.c +8
-1@@ -3408,7 +3408,82 @@ intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)static uint8_t intel_dp_autotest_edid (struct intel_dp *intel_dp) { -uint8_t test_result =DP_TEST_NAK ; +struct drm_connector *connector =&intel_dp->attached_connector->base; +struct i2c_adapter *adapter =&intel_dp->aux.ddc; +struct edid *edid_read =NULL ; +uint8_t *edid_data =NULL ; +uint8_t test_result =DP_TEST_NAK , checksum =0 ; +uint32_t i =0 , ret =0 ; +struct drm_display_mode *use_mode =NULL ; +int mode_count =0 ;
Quoted 8 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3419
Quoted 1 line
drivers/gpu/drm/i915/intel_dp.c +1
-0+struct drm_mode_set modeset;You have initialized every single variable you defined. This creates the problem that unused variables are not pointed by the compiler unless you enable the flags to warn set-but-not-used variables. For example, "i" is unused, and "ret" is set but never used. I also don't really see the point in, for example, NULL-initializing stuff like
edid_dataandedid_read.
I always initialize all the variables I declare as a matter of course. I've always been of the opinion that I'd rather use up a little extra stack space than to have to track down the weird problems that can result from using uninitialized variables. Compilers complain about using uninitialized variables as well, so this is more of a difference of opinion than anything else.
However, since there are unused variables, I'll remove those in the next iteration of this patch. :)
Quoted 7 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3423
Quoted 4 lines
drivers/gpu/drm/i915/intel_dp.c +4
-0+ +DRM_DEBUG_KMS ("Displayport: EDID automated test\n" ); + +/* Reset the NACK/DEFER counters */ As I said before, this is a great example of the "comment says what the code already says" problem.
Fair enough. It will be removed.
Quoted 11 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3431
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +8
-0+ intel_dp->aux.i2c_nack_count =0 ; + intel_dp->aux.i2c_defer_count =0 ; +/* Now read out the EDID */ + edid_read =drm_get_edid (connector, adapter); + +if (edid_read ==NULL ) { +/* Check for NACKs/DEFERs, goto failsafe if detected +(DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
Quoted 7 lines
Our coding standard is to put aligned '*'s on each line of a multi-line comment. So this should be "\t\t * (DP CTS 1.2 etc..." instead of what is above. In theory, the "/*" and "*/" strings should be on their own lines, alone, but we are inconsistent regarding this (even though Daniel randomly complained about this to me a few times in the past). As usual, check
Documentation/CodingStylefor better explanations.
I'll adjust the code to conform to the coding standard, whatever it happens to be.
Quoted 6 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3433
Quoted 3 lines
drivers/gpu/drm/i915/intel_dp.c +2
-0+if (intel_dp->aux.i2c_nack_count>0 || + intel_dp->aux.i2c_defer_count>0 )We tend to align the contents of the extra line with the '(' char on the line above.
I think this is a problem with tab sizes (as evidenced below). My editor is set for 4-space tabs whereas the kernel standard is 8-space tabs. I've resisted changing it because it spaces the code out way too much for my liking while I'm editing. But apparently this is going to be an issue moving forward, so I'm going to have to change it.
Quoted 6 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3436
Quoted 4 lines
drivers/gpu/drm/i915/intel_dp.c +3
-0+DRM_DEBUG_KMS ("Displayport: EDID read generated %d I2C NACKs, %d DEFERs\n" , + intel_dp->aux.i2c_nack_count, + intel_dp->aux.i2c_defer_count);There are way too many tabs on the 2 lines above.
See above. This is the tab size problem.
Quoted 9 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3440
Quoted 6 lines
drivers/gpu/drm/i915/intel_dp.c +4
-0+ goto failsafe; + } + +/* FIXME: Need to determine how to detect E-DDC here (4.2.2.9) */ But what is the problem with the current code? What are the consequences of not implementing the FIXME?
4.2.2.9 is a separate test. Until we address it in the code, the test will fail. If additional explanation is necessary, that can be added.
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3447
Quoted 8 lines
drivers/gpu/drm/i915/intel_dp.c +7
-0+ edid_data = (uint8_t *) edid_read; + +if (intel_dp_compute_edid_checksum (edid_data,&checksum)) { +/* Write the checksum to EDID checksum register */ + ret =drm_dp_dpcd_write (&intel_dp->aux, +DP_TEST_EDID_CHECKSUM , +&edid_read->checksum,1 );Way too many tabs above too.
Same tab problem.
Quoted 22 lines
Quoted 22 lines
drivers/gpu/drm/i915/intel_dp.c +20
-0+/* Reponse is ACK and and checksum written */ + test_result =DP_TEST_ACK |DP_TEST_EDID_CHECKSUM_WRITE ; + }else { +/* Invalid checksum - EDID corruption detection test */ + goto failsafe; + } + +/* Update EDID modes */ + mode_count =intel_connector_update_modes (connector, edid_read); +if (!mode_count) { +DRM_DEBUG_KMS ("Displayport: Mode update failed\n" ); + goto failsafe; + } + +/* Get the EDID preferred mode if available */ + use_mode =intel_dp_get_edid_preferred_mode (intel_dp); +if (use_mode ==NULL ) + goto failsafe; +else + goto set_mode;
Quoted 8 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
Quoted 5 lines
drivers/gpu/drm/i915/intel_dp.c +5
-0+ +failsafe: +DRM_DEBUG_KMS ("Displayport: Setting failsafe display mode\n" ); + use_mode =intel_dp_get_failsafe_mode (); +/* FIXME:<TAP> Need to set DP to 6bpc here as well */ What does<TAP> mean? What are the consequences of not setting DP to 6bpc here as well?
My initials? :) That's an oversight on my part. Those are just notes that I'm writing to myself while working in the code. It's a tag I can easily search to make sure I haven't missed anything. That shouldn't have been in there and instead should have been a simple "FIXME" with the added explanation. To answer your question though, without 18bpp the test fails stating that the video format is not correct.
Quoted 25 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3479
Quoted 7 lines
drivers/gpu/drm/i915/intel_dp.c +7
-0+ intel_dp->attached_connector->encoder->new_crtc->config.pipe_bpp =18 ; + +set_mode: +/* Configure the display mode necessary */ + modeset.connectors =&connector; + modeset.num_connectors =1 ; + modeset.crtc = connector->encoder->crtc;I guess this could result in NULL pointer dereferences. I don't see a guarantee that the connector will have a CRTC at this point.
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3480
Quoted 1 line
drivers/gpu/drm/i915/intel_dp.c +1
-0+ modeset.fb = modeset.crtc->primary->fb;And I also don't see a guarantee that we'll have an FB, and that the FB will be big enough for the current mode we're setting.
drivers/gpu/drm/i915/intel_dp.c intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3486
Quoted 6 lines
drivers/gpu/drm/i915/intel_dp.c +6
-0+ modeset.x =0 ; + modeset.y =0 ; + modeset.mode = use_mode; +/* Set the config */ +intel_dp_set_config (&modeset); +
Quoted 11 lines
I guess Daniel said this won't really work, right? This is why it will probably be simpler to involve user space on the compliance testing (in addition to the fact that we will test real-world production-used code). At this point we could just write to the debugfs file something like "set preferred mode" or "set failsafe mode", then wait for the modeset to happen. We could either try to detect the modeset (by setting some variable to zero before we send the message to user-space, and then verifying that
intel_set_modechanged it to true) or we could make the user-space write a string to the debugfs file, like "done" and assume user-space is correct and finish the function (it's debugfs anyway, and root, and DRM master, right?).
Right. I think this whole block is what Daniel was referring to. The two reasons to move it to userspace are that a) it's a more reliable solution to do this in userspace and b) it allows for testing the real code paths that are actively in use rather than just testing dedicated paths for a compliance badge.
Quoted 7 lines
Also, if we use the failsafe mode we'll return
DP_TEST_NAK. Is that intentional? Also, it is a little strange that the ACK is the last thing we should do here, and the spec doesn't seem to be clear reagarding when we should send it (or I just didn't find the piece of text that says it...). I guess that by running the compliance tests we will discover that by looking at the test result.
No it shouldn't NAK the test since it's been implemented. That's an error in the code that needs to be corrected. Page 24 of the Link CTS spec has some info on the ACK/NAK. Here's what it says:
"If the test mode requested is supported, the Source DUT shall
start transmitting the
TEST_VIDEO_PATTERN in the test mode requested, and set the TEST_ACK
bit in the TEST_RESPONSE
register."
"If the test mode requested is not supported, the Source DUT shall
signal a negative acknowledgement by
setting the TEST_NAK bit in the TEST_RESPONSE register."
"The Source DUT must acknowledge the interrupt by writing to theTEST_RESPONSE register within 5
seconds of IRQ HPD pulse detect."
In some cases, the ACK can only be set after the operation is complete (i.e. active video being transmitted or correct test pattern being transmitted). So for uniformity, the ACK with the appropriate other test bits set is done at the end. Also, with the 5 second timeout, there's no need to respond with an ACK immediately in some cases and wait til the end in others.
Quoted 125 lines
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +0
-1return test_result; } --1.9.1
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfxTodd Previte <mailto:[email protected]>
Monday, July 14, 2014 12:10 PM
Implements an updated version of the automated testing function that handles Displayport compliance for EDID operations.Signed-off-by Todd Previte <[email protected]>---
drivers/gpu/drm/i915/intel_dp.c| 77
++++++++++++++++++++++++++++++++++++++++-1 file changed, 76 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_dp.c +0
-0@@ -3408,7 +3408,82 @@ intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)static
uint8_tintel_dp_autotest_edid(struct intel_dp *intel_dp)
{
-uint8_ttest_result=DP_TEST_NAK;
+ structdrm_connector*connector =&intel_dp->attached_connector->base;
+ structi2c_adapter*adapter =&intel_dp->aux.ddc;
+ struct edid*edid_read= NULL;
+uint8_t*edid_data= NULL;
+uint8_ttest_result=DP_TEST_NAK, checksum = 0;
+uint32_ti = 0, ret = 0;
+ structdrm_display_mode*use_mode= NULL;
+ intmode_count= 0;
+ structdrm_mode_setmodeset;drivers/gpu/drm/i915/intel_dp.c +15
-0+ +DRM_DEBUG_KMS ("Displayport: EDID automated test\n" ); + +/* Reset the NACK/DEFER counters */ + intel_dp->aux.i2c_nack_count =0 ; + intel_dp->aux.i2c_defer_count =0 ; +/* Now read out the EDID */ + edid_read =drm_get_edid (connector, adapter); + +if (edid_read ==NULL ) { +/* Check for NACKs/DEFERs, goto 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("Displayport: EDID read generated %d I2C NACKs, %dDEFERs\n",
+intel_dp->aux.i2c_nack_count,
+intel_dp->aux.i2c_defer_count);
+ goto failsafe;
+ }drivers/gpu/drm/i915/intel_dp.c +48
-0+ +/* FIXME: Need to determine how to detect E-DDC here (4.2.2.9) */ + edid_data = (uint8_t *) edid_read; + +if (intel_dp_compute_edid_checksum (edid_data, &checksum)) { +/* Write the checksum to EDID checksum register */ + ret =drm_dp_dpcd_write (&intel_dp->aux, +DP_TEST_EDID_CHECKSUM , + &edid_read->checksum,1 ); +/* Reponse is ACK and and checksum written */ + test_result =DP_TEST_ACK |DP_TEST_EDID_CHECKSUM_WRITE ; + }else { +/* Invalid checksum - EDID corruption detection test */ + goto failsafe; + } + +/* Update EDID modes */ + mode_count =intel_connector_update_modes (connector, edid_read); +if (!mode_count) { +DRM_DEBUG_KMS ("Displayport: Mode update failed\n" ); + goto failsafe; + } + +/* Get the EDID preferred mode if available */ + use_mode =intel_dp_get_edid_preferred_mode (intel_dp); +if (use_mode ==NULL ) + goto failsafe; +else + goto set_mode; + +failsafe: +DRM_DEBUG_KMS ("Displayport: Setting failsafe display mode\n" ); + use_mode =intel_dp_get_failsafe_mode (); +/* FIXME: <TAP> Need to set DP to 6bpc here as well */ + intel_dp->attached_connector->encoder->new_crtc->config.pipe_bpp =18 ; + +set_mode: +/* Configure the display mode necessary */ + modeset.connectors = &connector; + modeset.num_connectors =1 ; + modeset.crtc = connector->encoder->crtc; + modeset.fb = modeset.crtc->primary->fb; + modeset.x =0 ; + modeset.y =0 ; + modeset.mode = use_mode; +/* Set the config */ +intel_dp_set_config (&modeset); +return
test_result;
}Todd Previte <mailto:[email protected]>
Monday, July 14, 2014 12:10 PMV2:
- Addressed review feedback from the mailing list
- Broke up patches into smaller, easily managed chunks
- Reordered the patches such that they can be applied in order
- Fixed checkpatch.pl errors across the patchset
- Updated and enhanced functionality for the EDID test function
- Completely revamped the mode set operations for compliance testing
Signature · 1 line
Sent with Postbox <http://www.getpostbox.com>
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 8/12 drm/i915: Improve reliability for Displayport link training
In reply to Todd PreviteLink training for Displayport can fail in many ways and at multiple different points during the training process. Previously, errors were logged but no additional action was taken based on them. Consequently, training attempts could continue even after errors have occured that would prevent successful link training. This patch updates the link training functions and where/how they're used to be more intelligent about failures and to stop trying to train the link when it's a lost cause.
--- drivers/gpu/drm/i915/intel_ddi.c | 23 +++++++++-- drivers/gpu/drm/i915/intel_dp.c | 89 +++++++++++++++++++++++++++++----------- drivers/gpu/drm/i915/intel_drv.h | 7 ++-- 3 files changed, 90 insertions(+), 29 deletions(-)
drivers/gpu/drm/i915/intel_ddi.c +20 -3
@@ -1246,6 +1246,7 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)struct intel_crtc *crtc =to_intel_crtc (encoder->crtc);enum port port =intel_ddi_get_encoder_port (intel_encoder);int type = intel_encoder->type; +uint8_t fail_code =0 ;if (crtc->config.has_audio) { DRM_DEBUG_DRIVER("Audio on pipe %c on DDI\n",
@@ -1274,10 +1275,19 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder) intel_dp->DP |=DDI_PORT_WIDTH (intel_dp->lane_count);intel_dp_sink_dpms (intel_dp,DRM_MODE_DPMS_ON ); -intel_dp_start_link_train (intel_dp); -intel_dp_complete_link_train (intel_dp); +if (!intel_dp_start_link_train (intel_dp)) { + fail_code =1 ; + goto failed; + } +if (!intel_dp_complete_link_train (intel_dp)) { + fail_code =2 ; + goto failed; + }if (port !=PORT_A ) -intel_dp_stop_link_train (intel_dp); +if (!intel_dp_stop_link_train (intel_dp)) { + fail_code =3 ; + goto failed; + } }else if (type ==INTEL_OUTPUT_HDMI ) {struct intel_hdmi *intel_hdmi =enc_to_intel_hdmi (encoder);
@@ -1285,6 +1295,13 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder) crtc->config.has_hdmi_sink, &crtc->config.adjusted_mode); } + +return ; + +failed: +/* Clear link training here */ +intel_dp_set_idle_link_train (enc_to_intel_dp (encoder)); +DRM_DEBUG_KMS ("Failed to pre-enable DP, fail code %d\n" , fail_code); }static void intel_ddi_post_disable (struct intel_encoder *intel_encoder)
drivers/gpu/drm/i915/intel_dp.c +66 -23
191 lines not on this page · raw message
drivers/gpu/drm/i915/intel_drv.h +4 -3
@@ -844,9 +844,10 @@ int intel_dp_set_config(struct drm_mode_set *set);void intel_dp_init (struct drm_device *dev,int output_reg,enum port port);bool intel_dp_init_connector (struct intel_digital_port *intel_dig_port,struct intel_connector *intel_connector); -void intel_dp_start_link_train (struct intel_dp *intel_dp); -void intel_dp_complete_link_train (struct intel_dp *intel_dp); -void intel_dp_stop_link_train (struct intel_dp *intel_dp); +bool intel_dp_start_link_train (struct intel_dp *intel_dp); +bool intel_dp_complete_link_train (struct intel_dp *intel_dp); +bool intel_dp_stop_link_train (struct intel_dp *intel_dp); +void intel_dp_set_idle_link_train (struct intel_dp *intel_dp);void intel_dp_sink_dpms (struct intel_dp *intel_dp,int mode);void intel_dp_encoder_destroy (struct drm_encoder *encoder);void intel_dp_check_link_status (struct intel_dp *intel_dp);
Signature · 1 line
1.9.1
Re: [PATCH 08/12] drm/i915: Improve reliability for Displayport link training
2014-07-14 16:10 GMT-03:00 Todd Previte <[email protected]>:
Quoted 6 lines
Link training for Displayport can fail in many ways and at multiple different points during the training process. Previously, errors were logged but no additional action was taken based on them. Consequently, training attempts could continue even after errors have occured that would prevent successful link training. This patch updates the link training functions and where/how they're used to be more intelligent about failures and to stop trying to train the link when it's a lost cause.
I agree we need to do something about this problem, but I'm not sure how this patch improves the situation. Can you please describe more how exactly the changes you did are getting us towards the solution of the problem? Of course, the points where you start signaling previously-unsignaled errors are obviously an improvement.
Anyway, this patch should probably be split in 3:
- A patch to add the boolean return values and change intel_dp_check_link_status() + intel_enable_dp() + intel_ddi_pre_enable().
- A patch to signal dpcd error cases we were previously ignoring.
- A patch to that changes how intel_dp_start_link_train() and intel_dp_stop_link_train() currently behave (the "goto"s replacing "break" statements).
Se below for better explanations.
The big problem here is that these encoder callbacks never fail, so there's not really much to do after we detect a sink failure.
In the current code (without your patch), we already clearly signal the link training failures with debug+error messages, so the new debug messages at places linke intel_enable_dp() are not much of an improvement. Also, we already run intel_dp_set_idle_link_train() at the end of intel_dp_complete_link_train(), and we do additional things such as calling intel_dp_stop_link_train(). And I guess we do the non-DDI equivalent steps at some point too, so I'm not sure how jumping straight to intel_dp_set_idle_link_train() helps, since we do it anyway as part of the normal sequence. Also, our mode set sequence is currently completely followed - even though the sink fails to understand what we throw at it - and I'm always afraid of not following the sequence exactly as described in the spec, since it could lead to unpredicted bugs (we had this problem dozens of times in the past).
I think the real cool solution would be to retry link training with different parameters (different clock and number of lanes), but I imagine this would require a lot of code refactoring since we probably need to go back to the compute_config stages of the modeset sequence. Or maybe just finding a way to tell the user-space modesetting app that it has a black screen would already be helpful.
Other people may think that the real-real long-term solution would be to fix our code so it never fails link training or gives black screens :)
Some more below:
Quoted 65 lines
Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_ddi.c | 23 +++++++++-- drivers/gpu/drm/i915/intel_dp.c | 89 +++++++++++++++++++++++++++++----------- drivers/gpu/drm/i915/intel_drv.h | 7 ++-- 3 files changed, 90 insertions(+), 29 deletions(-)drivers/gpu/drm/i915/intel_ddi.c +20
-3@@ -1246,6 +1246,7 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)struct intel_crtc *crtc =to_intel_crtc (encoder->crtc);enum port port =intel_ddi_get_encoder_port (intel_encoder);int type = intel_encoder->type; +uint8_t fail_code =0 ;if (crtc->config.has_audio) { DRM_DEBUG_DRIVER("Audio on pipe %c on DDI\n",@@ -1274,10 +1275,19 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder) intel_dp->DP |=DDI_PORT_WIDTH (intel_dp->lane_count);intel_dp_sink_dpms (intel_dp,DRM_MODE_DPMS_ON ); -intel_dp_start_link_train (intel_dp); -intel_dp_complete_link_train (intel_dp); +if (!intel_dp_start_link_train (intel_dp)) { + fail_code =1 ; + goto failed; + } +if (!intel_dp_complete_link_train (intel_dp)) { + fail_code =2 ; + goto failed; + }if (port !=PORT_A ) -intel_dp_stop_link_train (intel_dp); +if (!intel_dp_stop_link_train (intel_dp)) { + fail_code =3 ; + goto failed; + } }else if (type ==INTEL_OUTPUT_HDMI ) {struct intel_hdmi *intel_hdmi =enc_to_intel_hdmi (encoder);@@ -1285,6 +1295,13 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder) crtc->config.has_hdmi_sink, &crtc->config.adjusted_mode); } + +return ; + +failed: +/* Clear link training here */ +intel_dp_set_idle_link_train (enc_to_intel_dp (encoder)); +DRM_DEBUG_KMS ("Failed to pre-enable DP, fail code %d\n" , fail_code); }static void intel_ddi_post_disable (struct intel_encoder *intel_encoder)drivers/gpu/drm/i915/intel_dp.c +0
-0@@ -2018,23 +2018,42 @@ static void chv_post_disable_dp(struct intel_encoder *encoder)mutex_unlock (&dev_priv->dpio_lock); }
drivers/gpu/drm/i915/intel_dp.c static void chv_post_disable_dp(struct intel_encoder *encoder) · line 2029
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +3
-1-static void intel_enable_dp (struct intel_encoder *encoder) +static bool intel_enable_dp (struct intel_encoder *encoder) {struct intel_dp *intel_dp =enc_to_intel_dp (&encoder->base);struct drm_device *dev = encoder->base.dev;struct drm_i915_private *dev_priv = dev->dev_private;uint32_t dp_reg =I915_READ (intel_dp->output_reg); +uint8_t fail_code =0 ; +/* FIXME: Not sure this needs to be a WARN() */
You can git-blame that line and see its history :)
Anyway, the point is that if you reach this point of the code, DP _must_ be disabled, so "dp_reg & DP_POR_EN" tells us there's a bug somewhere else.
Quoted 19 lines
drivers/gpu/drm/i915/intel_dp.c +9
-4if (WARN_ON (dp_reg &DP_PORT_EN )) -return ; +return false;intel_edp_panel_vdd_on (intel_dp);intel_dp_sink_dpms (intel_dp,DRM_MODE_DPMS_ON ); -intel_dp_start_link_train (intel_dp); +if (!intel_dp_start_link_train (intel_dp)) { + fail_code =1 ; + goto failed; + }intel_edp_panel_on (intel_dp);edp_panel_vdd_off (intel_dp, true); -intel_dp_complete_link_train (intel_dp); -intel_dp_stop_link_train (intel_dp); +if (!intel_dp_complete_link_train (intel_dp)) { + fail_code =2 ; + goto failed; + }
drivers/gpu/drm/i915/intel_dp.c static void chv_post_disable_dp(struct intel_encoder *encoder) · line 2054
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10
-0+if (!intel_dp_stop_link_train (intel_dp)) { + fail_code =3 ; + goto failed; + } + +return true; + +failed: +/* Clear link training here */ +intel_dp_set_idle_link_train (intel_dp);
Function intel_enable_dp() is called by all non-DDI gens, but intel_dp_set_idle_link_train() has an early return for !HAS_DDI. So this function call is basically doing nothing here. IMHO this is the biggest problem with this patch.
Quoted 30 lines
drivers/gpu/drm/i915/intel_dp.c +6
-3+DRM_DEBUG_KMS ("Failed to enable DP with code %d\n" , fail_code); +return false; }static void g4x_enable_dp (struct intel_encoder *encoder)@@ -2956,7 +2975,7 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,return ret == intel_dp->lane_count; } -static void intel_dp_set_idle_link_train (struct intel_dp *intel_dp) +void intel_dp_set_idle_link_train (struct intel_dp *intel_dp) {struct intel_digital_port *intel_dig_port =dp_to_dig_port (intel_dp);struct drm_device *dev = intel_dig_port->base.base.dev;@@ -2988,7 +3007,7 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp) }/* Enable corresponding port and start training pattern 1 */ -void +bool intel_dp_start_link_train (struct intel_dp *intel_dp) {struct drm_encoder *encoder = &dp_to_dig_port (intel_dp)->base.base;@@ -3007,11 +3026,17 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) link_config[1 ] = intel_dp->lane_count;if (drm_dp_enhanced_frame_cap (intel_dp->dpcd)) link_config[1 ] |=DP_LANE_COUNT_ENHANCED_FRAME_EN ; -drm_dp_dpcd_write (&intel_dp->aux,DP_LINK_BW_SET , link_config,2 ); +if (drm_dp_dpcd_write (&intel_dp->aux,DP_LINK_BW_SET , link_config,2 ) !=2 ) { +DRM_DEBUG_KMS ("Failed to write sink DPCD for link rate and lane count\n" );
drivers/gpu/drm/i915/intel_dp.c intel_dp_start_link_train(struct intel_dp *intel_dp) · line 3039
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +6
-1+ goto failed; + } link_config[0 ] =0 ; link_config[1 ] =DP_SET_ANSI_8B10B ; -drm_dp_dpcd_write (&intel_dp->aux,DP_DOWNSPREAD_CTRL , link_config,2 ); +if (drm_dp_dpcd_write (&intel_dp->aux,DP_DOWNSPREAD_CTRL , link_config,2 ) !=2 ) { +DRM_DEBUG_KMS ("Failed to write sink DPCD for downspread control\n" ); + goto failed; + }
These two above are useful additions and could probably go into a separate patch. But maybe I'd make them be DRM_ERRORs since we probably want to easily notice them - and get the bug reports.
Maybe we could also create a new dp_dpcd_write_safe() function/macro that would be responsible for printing error messages in case we don't transfer all the bits we want, then we could make the whole intel_dp.c file use it. The nice thing of being a macro would be that the DRM_ERROR would print the name of the caller function.
Quoted 51 lines
drivers/gpu/drm/i915/intel_dp.c +6
-6DP |=DP_PORT_EN ;@@ -3020,7 +3045,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) DP_TRAINING_PATTERN_1 | DP_LINK_SCRAMBLING_DISABLE)) {DRM_ERROR ("failed to enable link training\n" ); -return ; + goto failed; } voltage =0xff ;@@ -3032,12 +3057,12 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)drm_dp_link_train_clock_recovery_delay (intel_dp->dpcd);if (!intel_dp_get_link_status (intel_dp, link_status)) {DRM_ERROR ("failed to get link status\n" ); -break ; + goto failed; }if (drm_dp_clock_recovery_ok (link_status, intel_dp->lane_count)) {DRM_DEBUG_KMS ("clock recovery OK\n" ); -break ; + goto cr_done; }/* Check to see if we've tried the max voltage */ @@ -3048,7 +3073,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) ++loop_tries;if (loop_tries ==5 ) {DRM_ERROR ("too many full retries, give up\n" ); -break ; + goto failed; } intel_dp_reset_link_train(intel_dp, &DP, DP_TRAINING_PATTERN_1 |@@ -3062,7 +3087,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) ++voltage_tries;if (voltage_tries ==5 ) {DRM_ERROR ("too many voltage retries, give up\n" ); -break ; + goto failed; } }else voltage_tries =0 ;@@ -3071,14 +3096,20 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)/* Update training set as requested by target */ if (!intel_dp_update_link_train (intel_dp, &DP , link_status)) {DRM_ERROR ("failed to update link training\n" ); -break ; + goto failed;
drivers/gpu/drm/i915/intel_dp.c intel_dp_start_link_train(struct intel_dp *intel_dp) · line 3109
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +6
-0} } +cr_done: intel_dp->DP =DP ; +return true; + +failed: +DRM_DEBUG_KMS ("Failed to initiate link training\n" ); +return false;
This set of changes where you replace "break" with "goto"s should be on a separate patch, with a nice explanation of what are the consequences of not doing "intel_dp->DP = DP" on the cases were we just "goto failed". If the link training failed, we should probably disable DP_PORT_EN.
Quoted 18 lines
drivers/gpu/drm/i915/intel_dp.c +2
-2} -void +bool intel_dp_complete_link_train (struct intel_dp *intel_dp) {bool channel_eq = false;@@ -3095,7 +3126,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) training_pattern | DP_LINK_SCRAMBLING_DISABLE)) {DRM_ERROR ("failed to start channel equalization\n" ); -return ; +return false; } tries =0 ;@@ -3154,14 +3185,17 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
drivers/gpu/drm/i915/intel_dp.c intel_dp_complete_link_train(struct intel_dp *intel_dp) · line 3194
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +4
-1intel_dp->DP =DP ; -if (channel_eq) +if (channel_eq) {DRM_DEBUG_KMS ("Channel EQ done. DP Training successful\n" ); +return true; + } +return false; }
Same here.
Quoted 56 lines
drivers/gpu/drm/i915/intel_dp.c +14
-5-void intel_dp_stop_link_train (struct intel_dp *intel_dp) +bool intel_dp_stop_link_train (struct intel_dp *intel_dp) { -intel_dp_set_link_train (intel_dp, &intel_dp->DP, +return intel_dp_set_link_train (intel_dp, &intel_dp->DP,DP_TRAINING_PATTERN_DISABLE ); }@@ -3600,9 +3634,18 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)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); -intel_dp_start_link_train (intel_dp); -intel_dp_complete_link_train (intel_dp); -intel_dp_stop_link_train (intel_dp); +if (!intel_dp_start_link_train (intel_dp)) { +DRM_DEBUG_KMS ("Start link training failed\n" ); +return ; + } +if (!intel_dp_complete_link_train (intel_dp)) { +DRM_DEBUG_KMS ("Complete link training failed\n" ); +return ; + } +if (!intel_dp_stop_link_train (intel_dp)) { +DRM_DEBUG_KMS ("Stop link training failed\n" ); +return ; + } } }drivers/gpu/drm/i915/intel_drv.h +4
-4@@ -844,9 +844,10 @@ int intel_dp_set_config(struct drm_mode_set *set);void intel_dp_init (struct drm_device *dev,int output_reg,enum port port);bool intel_dp_init_connector (struct intel_digital_port *intel_dig_port,struct intel_connector *intel_connector); -void intel_dp_start_link_train (struct intel_dp *intel_dp); -void intel_dp_complete_link_train (struct intel_dp *intel_dp); -void intel_dp_stop_link_train (struct intel_dp *intel_dp); +bool intel_dp_start_link_train (struct intel_dp *intel_dp); +bool intel_dp_complete_link_train (struct intel_dp *intel_dp); +bool intel_dp_stop_link_train (struct intel_dp *intel_dp); +void intel_dp_set_idle_link_train (struct intel_dp *intel_dp);void intel_dp_sink_dpms (struct intel_dp *intel_dp,int mode);void intel_dp_encoder_destroy (struct drm_encoder *encoder);void intel_dp_check_link_status (struct intel_dp *intel_dp); --1.9.1
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Paulo Zanoni
Re: [PATCH 08/12] drm/i915: Improve reliability for Displayport link training
Quoted 50 lines · Paulo Zanoni
2014-07-14 16:10 GMT-03:00 Todd Previte <[email protected]>:
Quoted 6 lines
Link training for Displayport can fail in many ways and at multiple different points during the training process. Previously, errors were logged but no additional action was taken based on them. Consequently, training attempts could continue even after errors have occured that would prevent successful link training. This patch updates the link training functions and where/how they're used to be more intelligent about failures and to stop trying to train the link when it's a lost cause.
I agree we need to do something about this problem, but I'm not sure how this patch improves the situation. Can you please describe more how exactly the changes you did are getting us towards the solution of the problem? Of course, the points where you start signaling previously-unsignaled errors are obviously an improvement.
Anyway, this patch should probably be split in 3:
- A patch to add the boolean return values and changeintel_dp_check_link_status()+intel_enable_dp()+intel_ddi_pre_enable().
- A patch to signal dpcd error cases we were previously ignoring.
- A patch to that changes howintel_dp_start_link_train()andintel_dp_stop_link_train()currently behave (the "goto"s replacing "break" statements).
Se below for better explanations.The big problem here is that these encoder callbacks never fail, so there's not really much to do after we detect a sink failure.
In the current code (without your patch), we already clearly signal the link training failures with debug+error messages, so the new debug messages at places linke
intel_enable_dp()are not much of an improvement. Also, we already runintel_dp_set_idle_link_train()at the end ofintel_dp_complete_link_train(), and we do additional things such as callingintel_dp_stop_link_train(). And I guess we do the non-DDI equivalent steps at some point too, so I'm not sure how jumping straight tointel_dp_set_idle_link_train()helps, since we do it anyway as part of the normal sequence. Also, our mode set sequence is currently completely followed - even though the sink fails to understand what we throw at it - and I'm always afraid of not following the sequence exactly as described in the spec, since it could lead to unpredicted bugs (we had this problem dozens of times in the past).I think the real cool solution would be to retry link training with different parameters (different clock and number of lanes), but I imagine this would require a lot of code refactoring since we probably need to go back to the
compute_configstages of the modeset sequence. Or maybe just finding a way to tell the user-space modesetting app that it has a black screen would already be helpful.
Quoted 3 lines
Other people may think that the real-real long-term solution would be to fix our code so it never fails link training or gives black screens :)
DP spec says that we need to be able to retrain. My rough idea was that we set some flags in pipe config (or wire up return values) which signals that we need to rerun the modeset (including compute_config and all that).
To keep the link bw/count limits around we could add some flags to the pipe config or something (and not completely clear it ofc). -Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
PATCH 9/12 drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing
In reply to Todd PreviteMove 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.
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.
--- drivers/gpu/drm/i915/intel_dp.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +18 -14
@@ -3600,22 +3600,13 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)u8 sink_irq_vector;u8 link_status[DP_LINK_STATUS_SIZE ]; -/* FIXME: This access isn't protected by any locks. */ -if (!intel_encoder->connectors_active) -return ; - -if (WARN_ON (!intel_encoder->base.crtc)) +/* Bail if not connected */ +if (intel_dp->attached_connector->base.status != connector_status_connected)return ; -/* Try to read receiver status if the link appears to be up */ -if (!intel_dp_get_link_status (intel_dp, link_status)) { -return ; - } - -/* Now read the DPCD to see if it's actually running */ -if (!intel_dp_get_dpcd (intel_dp)) { +/* Attempt to read the DPCD */ +if (!intel_dp_get_dpcd (intel_dp))return ; - }/* Try to read the source of the interrupt */ if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
@@ -3625,12 +3616,25 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)DP_DEVICE_SERVICE_IRQ_VECTOR , sink_irq_vector); -if (sink_irq_vector &DP_AUTOMATED_TEST_REQUEST ) +if (sink_irq_vector &DP_AUTOMATED_TEST_REQUEST ) { +DRM_DEBUG_KMS ("Displayport: Received automated test request\n" );intel_dp_handle_test_request (intel_dp); + }if (sink_irq_vector & (DP_CP_IRQ |DP_SINK_SPECIFIC_IRQ ))DRM_DEBUG_DRIVER ("CP or sink specific irq unhandled\n" ); } +/* FIXME: This access isn't protected by any locks. */ +if (!intel_encoder->connectors_active) +return ; + +if (WARN_ON (!intel_encoder->base.crtc)) +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);
Signature · 1 line
1.9.1
Re: [PATCH 09/12] drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing
2014-07-14 16:10 GMT-03:00 Todd Previte <[email protected]>:
Quoted 11 lines
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.
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->disconnectedtransition, the test code will attempt to execute even though the connection has been disabled, resulting in a faied test.
The problem here we that we just got an interrupt, so I am not sure if we should believe what's in intel_dp->attached_connector->base.status:
doesn't it reflect the old state that we had before we got the interrupt? I'm not sure if this information could be wrong. Can you please elaborate on that?
The rest of the patch seems correct.
Quoted 70 lines
Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)drivers/gpu/drm/i915/intel_dp.c +18
-15@@ -3600,22 +3600,13 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)u8 sink_irq_vector;u8 link_status[DP_LINK_STATUS_SIZE ]; -/* FIXME: This access isn't protected by any locks. */ -if (!intel_encoder->connectors_active) -return ; - -if (WARN_ON (!intel_encoder->base.crtc)) +/* Bail if not connected */ +if (intel_dp->attached_connector->base.status != connector_status_connected)return ; -/* Try to read receiver status if the link appears to be up */ -if (!intel_dp_get_link_status (intel_dp, link_status)) { -return ; - } - -/* Now read the DPCD to see if it's actually running */ -if (!intel_dp_get_dpcd (intel_dp)) { +/* Attempt to read the DPCD */ +if (!intel_dp_get_dpcd (intel_dp))return ; - }/* Try to read the source of the interrupt */ if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&@@ -3625,12 +3616,25 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)DP_DEVICE_SERVICE_IRQ_VECTOR , sink_irq_vector); -if (sink_irq_vector &DP_AUTOMATED_TEST_REQUEST ) +if (sink_irq_vector &DP_AUTOMATED_TEST_REQUEST ) { +DRM_DEBUG_KMS ("Displayport: Received automated test request\n" );intel_dp_handle_test_request (intel_dp); + }if (sink_irq_vector & (DP_CP_IRQ |DP_SINK_SPECIFIC_IRQ ))DRM_DEBUG_DRIVER ("CP or sink specific irq unhandled\n" ); } +/* FIXME: This access isn't protected by any locks. */ +if (!intel_encoder->connectors_active) +return ; + +if (WARN_ON (!intel_encoder->base.crtc)) +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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Paulo Zanoni
PATCH 10/12 drm/i915: Update link training automated test function for Displayport compliance
In reply to Todd PreviteImplements basic link training functionality for Displayport automated compliance testing.
--- drivers/gpu/drm/i915/intel_dp.c | 47 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
drivers/gpu/drm/i915/intel_dp.c +47 -0
@@ -3427,6 +3427,53 @@ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp) {uint8_t test_result =DP_TEST_NAK ; +uint8_t rxdata[2 ]; +uint8_t link_status[DP_LINK_STATUS_SIZE ]; +int bytes_ret =0 ; +struct drm_connector *connector = &intel_dp->attached_connector->base; +struct intel_digital_port *intel_dig_port = +enc_to_dig_port (&intel_dp->attached_connector->encoder->base); + +/* Read test parameters */ + bytes_ret =drm_dp_dpcd_read (&intel_dp->aux,DP_TEST_LINK_RATE , rxdata,2 ); + +/* Set link rate directly */ + intel_dp->link_bw = rxdata[0 ]; +/* Preserve 7:5 when setting lane count */ + intel_dp->lane_count &=0xE0 ; + intel_dp->lane_count |= rxdata[1 ]; + +DRM_DEBUG_KMS ("Displayport: Link training testing - %d lanes @ %02x link rate\n" , + intel_dp->lane_count, intel_dp->link_bw); + +/* Train the link */ + intel_dp->DP = intel_dig_port->saved_port_bits | +DDI_BUF_CTL_ENABLE | +DDI_BUF_EMP_400MV_0DB_HSW ; + intel_dp->DP |=DDI_PORT_WIDTH (intel_dp->lane_count); + +if (!intel_dp_start_link_train (intel_dp)) { +DRM_DEBUG_KMS ("Displayport: intel_dp_start_link_train() failed\n" ); + test_result = false; + } +if (!intel_dp_complete_link_train (intel_dp)) { +DRM_DEBUG_KMS ("Displayport: intel_dp_complete_link_train() failed\n" ); + test_result = false; + } +if (!intel_dp_stop_link_train (intel_dp)) { +DRM_DEBUG_KMS ("Displayport: intel_dp_stop_link_train() failed\n" ); + test_result = false; + } + +/* Check link status for successful completion */ +if (drm_dp_channel_eq_ok (link_status, intel_dp->lane_count)) + test_result = true; + +if (test_result == false) { +/* Clear link training here */ +intel_dp_set_idle_link_train (intel_dp); + } +return test_result; }
Signature · 1 line
1.9.1
Re: [PATCH 10/12] drm/i915: Update link training automated test function for Displayport compliance
Quoted 21 lines · Todd Previte
Implements basic link training functionality for Displayport automated compliance testing.
Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 47 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)drivers/gpu/drm/i915/intel_dp.c +5
-0@@ -3427,6 +3427,53 @@ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp) {uint8_t test_result =DP_TEST_NAK ; +uint8_t rxdata[2 ]; +uint8_t link_status[DP_LINK_STATUS_SIZE ]; +int bytes_ret =0 ; +struct drm_connector *connector = &intel_dp->attached_connector->base; + struct intel_digital_port *intel_dig_port =
drivers/gpu/drm/i915/intel_dp.c static uint8_t · line 3444
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10
-0+enc_to_dig_port (&intel_dp->attached_connector->encoder->base); + +/* Read test parameters */ + bytes_ret =drm_dp_dpcd_read (&intel_dp->aux,DP_TEST_LINK_RATE , rxdata,2 ); + +/* Set link rate directly */ + intel_dp->link_bw = rxdata[0 ]; +/* Preserve 7:5 when setting lane count */ + intel_dp->lane_count &=0xE0 ; + intel_dp->lane_count |= rxdata[1 ];
This won't work - if you change the link config you need to do a full recomputation of the modeset config and a full modeset. No, we dont (yet) have the infrastructure for this, which is why dp is such a pain since we can change the link config once we've decided on something :( -Daniel
Quoted 42 lines
drivers/gpu/drm/i915/intel_dp.c +32
-0+ +DRM_DEBUG_KMS ("Displayport: Link training testing - %d lanes @ %02x link rate\n" , + intel_dp->lane_count, intel_dp->link_bw); + +/* Train the link */ + intel_dp->DP = intel_dig_port->saved_port_bits | +DDI_BUF_CTL_ENABLE | +DDI_BUF_EMP_400MV_0DB_HSW ; + intel_dp->DP |=DDI_PORT_WIDTH (intel_dp->lane_count); + +if (!intel_dp_start_link_train (intel_dp)) { +DRM_DEBUG_KMS ("Displayport: intel_dp_start_link_train() failed\n" ); + test_result = false; + } +if (!intel_dp_complete_link_train (intel_dp)) { +DRM_DEBUG_KMS ("Displayport: intel_dp_complete_link_train() failed\n" ); + test_result = false; + } +if (!intel_dp_stop_link_train (intel_dp)) { +DRM_DEBUG_KMS ("Displayport: intel_dp_stop_link_train() failed\n" ); + test_result = false; + } + +/* Check link status for successful completion */ +if (drm_dp_channel_eq_ok (link_status, intel_dp->lane_count)) + test_result = true; + +if (test_result == false) { +/* Clear link training here */ +intel_dp_set_idle_link_train (intel_dp); + } +return test_result; }Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [PATCH 10/12] drm/i915: Update link training automated test function for Displayport compliance
Quoted 29 lines · On Tue, 22 Jul 2014 08:15:30 +0200 Daniel Vetter <[email protected]> wrote:
Quoted 28 lines · Todd Previte
Implements basic link training functionality for Displayport automated compliance testing.
Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 47 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)drivers/gpu/drm/i915/intel_dp.c +12
-0@@ -3427,6 +3427,53 @@ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp) {uint8_t test_result =DP_TEST_NAK ; +uint8_t rxdata[2 ]; +uint8_t link_status[DP_LINK_STATUS_SIZE ]; +int bytes_ret =0 ; +struct drm_connector *connector = &intel_dp->attached_connector->base; +struct intel_digital_port *intel_dig_port = +enc_to_dig_port (&intel_dp->attached_connector->encoder->base); + +/* Read test parameters */ + bytes_ret =drm_dp_dpcd_read (&intel_dp->aux,DP_TEST_LINK_RATE , rxdata,2 ); + +/* Set link rate directly */ + intel_dp->link_bw = rxdata[0 ];
Quoted 8 lines
drivers/gpu/drm/i915/intel_dp.c static uint8_t · line 3444
Quoted 3 lines
drivers/gpu/drm/i915/intel_dp.c +3
-0+/* Preserve 7:5 when setting lane count */ + intel_dp->lane_count &=0xE0 ; + intel_dp->lane_count |= rxdata[1 ];This won't work - if you change the link config you need to do a full recomputation of the modeset config and a full modeset. No, we dont (yet) have the infrastructure for this, which is why dp is such a pain since we can change the link config once we've decided on something :(
I think that depends on how we're going to structure the code. For simply calling the link train functions, it looks like messing with the intel_dp->foo fields will roughly do what Todd wants, which is to simply try a re-train with a different set of params, ignoring the actual fb and pipe configuration.
Or is that what you had in mind? You'd like to see valid data and mode timings to drive a given lane count and bw instead? I'm not sure the testing spec cares about that; Todd?
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
Re: [PATCH 10/12] drm/i915: Update link training automated test function for Displayport compliance
Quoted 17 lines · Jesse Barnes
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c static uint8_t · line 3444
Quoted 5 lines
drivers/gpu/drm/i915/intel_dp.c +5
-0+/* Set link rate directly */ + intel_dp->link_bw = rxdata[0 ]; +/* Preserve 7:5 when setting lane count */ + intel_dp->lane_count &=0xE0 ; + intel_dp->lane_count |= rxdata[1 ];This won't work - if you change the link config you need to do a full recomputation of the modeset config and a full modeset. No, we dont (yet) have the infrastructure for this, which is why dp is such a pain since we can change the link config once we've decided on something :(
I think that depends on how we're going to structure the code. For simply calling the link train functions, it looks like messing with the
intel_dp->foofields will roughly do what Todd wants, which is to simply try a re-train with a different set of params, ignoring the actual fb and pipe configuration.
Quoted 3 lines
Or is that what you had in mind? You'd like to see valid data and mode timings to drive a given lane count and bw instead? I'm not sure the testing spec cares about that; Todd?
If you change the link_bw then you need to do a full modeset sequence since that's the only way to convince our hw to switch to the new clock. The current code in this patch falls short and only retrains the link.
But even if we'd do the full modeset the compute_config stage would overwrite the link parameters again. So the correct way to do this is to pimp the compute config code to have limits (we might as well expose them as connector properties while at it for debugging) and then do a full modeset sequence here. Then we could finally implement (with a bit more code) the transparent fallback to a different config if the first one doesn't work.
-Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [PATCH 10/12] drm/i915: Update link training automated test function for Displayport compliance
Quoted 27 lines · On Tue, 22 Jul 2014 22:44:54 +0200 Daniel Vetter <[email protected]> wrote:
Quoted 17 lines · Jesse Barnes
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c static uint8_t · line 3444
Quoted 5 lines
drivers/gpu/drm/i915/intel_dp.c +5
-0+/* Set link rate directly */ + intel_dp->link_bw = rxdata[0 ]; +/* Preserve 7:5 when setting lane count */ + intel_dp->lane_count &=0xE0 ; + intel_dp->lane_count |= rxdata[1 ];This won't work - if you change the link config you need to do a full recomputation of the modeset config and a full modeset. No, we dont (yet) have the infrastructure for this, which is why dp is such a pain since we can change the link config once we've decided on something :(
I think that depends on how we're going to structure the code. For simply calling the link train functions, it looks like messing with the
intel_dp->foofields will roughly do what Todd wants, which is to simply try a re-train with a different set of params, ignoring the actual fb and pipe configuration.Quoted 3 lines
Or is that what you had in mind? You'd like to see valid data and mode timings to drive a given lane count and bw instead? I'm not sure the testing spec cares about that; Todd?
If you change the
link_bwthen you need to do a full modeset sequence since that's the only way to convince our hw to switch to the new clock. The current code in this patch falls short and only retrains the link.
Quoted 7 lines
But even if we'd do the full modeset the
compute_configstage would overwrite the link parameters again. So the correct way to do this is to pimp the compute config code to have limits (we might as well expose them as connector properties while at it for debugging) and then do a full modeset sequence here. Then we could finally implement (with a bit more code) the transparent fallback to a different config if the first one doesn't work.
Right, yeah going through the full sequence sounds like it's required for most of the tests based on what Todd tells me, since the test sink will look at the active mode transmitted etc.
So the simple re-train and send misc isn't sufficient, even if the training succeeds.
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
PATCH 11/12 drm/i915: Minor code clean up in intel_dp.c
In reply to Todd PreviteCleans up a couple of unused variables and an extraneous debug log message that was unintentionally left behind.
--- drivers/gpu/drm/i915/intel_dp.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +1 -3
@@ -3430,7 +3430,6 @@ intel_dp_autotest_link_training(struct intel_dp *intel_dp)uint8_t rxdata[2 ];uint8_t link_status[DP_LINK_STATUS_SIZE ];int bytes_ret =0 ; -struct drm_connector *connector = &intel_dp->attached_connector->base;struct intel_digital_port *intel_dig_port =enc_to_dig_port (&intel_dp->attached_connector->encoder->base);
@@ -3494,7 +3493,7 @@ intel_dp_autotest_edid(struct intel_dp *intel_dp)struct edid *edid_read =NULL ;uint8_t *edid_data =NULL ;uint8_t test_result =DP_TEST_NAK , checksum =0 ; -uint32_t i =0 , ret =0 ; +uint32_t ret =0 ;struct drm_display_mode *use_mode =NULL ;int mode_count =0 ;struct drm_mode_set modeset;
@@ -3592,7 +3591,6 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)uint8_t rxdata =0 ;int ret =0 ; -DRM_DEBUG_KMS ("Displayport: Received automated test request\n" );/* Read DP_TEST_REQUEST register to identify the requested test */ ret =drm_dp_dpcd_read (&intel_dp->aux,DP_TEST_REQUEST , &rxdata,1 );
Signature · 1 line
1.9.1
Re: [PATCH 11/12] drm/i915: Minor code clean up in intel_dp.c
Quoted 4 lines · Todd Previte
Cleans up a couple of unused variables and an extraneous debug log message that was unintentionally left behind.
Signed-off-by Todd Previte <[email protected]>
This should be squashed into the relevant earlier patches. -Daniel
Quoted 40 lines
--- drivers/gpu/drm/i915/intel_dp.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)drivers/gpu/drm/i915/intel_dp.c +1
-3@@ -3430,7 +3430,6 @@ intel_dp_autotest_link_training(struct intel_dp *intel_dp)uint8_t rxdata[2 ];uint8_t link_status[DP_LINK_STATUS_SIZE ];int bytes_ret =0 ; -struct drm_connector *connector = &intel_dp->attached_connector->base;struct intel_digital_port *intel_dig_port =enc_to_dig_port (&intel_dp->attached_connector->encoder->base);@@ -3494,7 +3493,7 @@ intel_dp_autotest_edid(struct intel_dp *intel_dp)struct edid *edid_read =NULL ;uint8_t *edid_data =NULL ;uint8_t test_result =DP_TEST_NAK , checksum =0 ; -uint32_t i =0 , ret =0 ; +uint32_t ret =0 ;struct drm_display_mode *use_mode =NULL ;int mode_count =0 ;struct drm_mode_set modeset;@@ -3592,7 +3591,6 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)uint8_t rxdata =0 ;int ret =0 ; -DRM_DEBUG_KMS ("Displayport: Received automated test request\n" );/* Read DP_TEST_REQUEST register to identify the requested test */ ret =drm_dp_dpcd_read (&intel_dp->aux,DP_TEST_REQUEST , &rxdata,1 );Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [PATCH 11/12] drm/i915: Minor code clean up in intel_dp.c
Quoted 11 lines
Daniel Vetter <mailto:[email protected]>
Tuesday, July 15, 2014 12:47 AMQuoted 4 lines · Todd Previte
Cleans up a couple of unused variables and an extraneous debug log message that was unintentionally left behind.
Signed-off-by Todd Previte<[email protected]>This should be squashed into the relevant earlier patches. -Daniel
Sounds good. I'll squash them in and resend the patches as v3. Thanks Daniel.
-T
Quoted 93 lines
Quoted 40 lines
--- drivers/gpu/drm/i915/intel_dp.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)drivers/gpu/drm/i915/intel_dp.c +1
-3@@ -3430,7 +3430,6 @@ intel_dp_autotest_link_training(struct intel_dp *intel_dp)uint8_t rxdata[2 ];uint8_t link_status[DP_LINK_STATUS_SIZE ];int bytes_ret =0 ; -struct drm_connector *connector =&intel_dp->attached_connector->base;struct intel_digital_port *intel_dig_port =enc_to_dig_port (&intel_dp->attached_connector->encoder->base);@@ -3494,7 +3493,7 @@ intel_dp_autotest_edid(struct intel_dp *intel_dp)struct edid *edid_read =NULL ;uint8_t *edid_data =NULL ;uint8_t test_result =DP_TEST_NAK , checksum =0 ; -uint32_t i =0 , ret =0 ; +uint32_t ret =0 ;struct drm_display_mode *use_mode =NULL ;int mode_count =0 ;struct drm_mode_set modeset;@@ -3592,7 +3591,6 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)uint8_t rxdata =0 ;int ret =0 ; -DRM_DEBUG_KMS ("Displayport: Received automated test request\n" );/* Read DP_TEST_REQUEST register to identify the requested test */ ret =drm_dp_dpcd_read (&intel_dp->aux,DP_TEST_REQUEST ,&rxdata,1 );Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfxTodd Previte <mailto:[email protected]>
Monday, July 14, 2014 12:10 PM
Cleans up a couple of unused variables and an extraneous debug log message that was unintentionally left behind.Signed-off-by Todd Previte <[email protected]>---
drivers/gpu/drm/i915/intel_dp.c| 4 +---1 file changed, 1 insertion(+), 3 deletions(-)drivers/gpu/drm/i915/intel_dp.c +0
-1@@ -3430,7 +3430,6 @@ intel_dp_autotest_link_training(struct intel_dp *intel_dp)uint8_t rxdata[2 ];uint8_t link_status[DP_LINK_STATUS_SIZE ];int bytes_ret =0 ; -struct drm_connector *connector = &intel_dp->attached_connector->base;struct
intel_digital_port*intel_dig_port=enc_to_dig_port(&intel_dp->attached_connector->encoder->base);drivers/gpu/drm/i915/intel_dp.c +1
-2@@ -3494,7 +3493,7 @@ intel_dp_autotest_edid(struct intel_dp *intel_dp)struct edid *edid_read =NULL ;uint8_t *edid_data =NULL ;uint8_t test_result =DP_TEST_NAK , checksum =0 ; -uint32_t i =0 , ret =0 ; +uint32_t ret =0 ;struct drm_display_mode *use_mode =NULL ;int mode_count =0 ;struct drm_mode_set modeset;@@ -3592,7 +3591,6 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)uint8_t rxdata =0 ;int ret =0 ; -DRM_DEBUG_KMS ("Displayport: Received automated test request\n" );/* Read DP_TEST_REQUEST register to identify the requested test */ ret =drm_dp_dpcd_read (&intel_dp->aux,DP_TEST_REQUEST , &rxdata,1 );Todd Previte <mailto:[email protected]>
Monday, July 14, 2014 12:10 PMV2:
- Addressed review feedback from the mailing list
- Broke up patches into smaller, easily managed chunks
- Reordered the patches such that they can be applied in order
- Fixed checkpatch.pl errors across the patchset
- Updated and enhanced functionality for the EDID test function
- Completely revamped the mode set operations for compliance testing
Signature · 1 line
Sent with Postbox <http://www.getpostbox.com>
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 12/12 drm/i915: Add a delay in Displayport AUX transactions for compliance testing
In reply to Todd PreviteThe Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1 specifies that repeated AUX transactions after a failure (NACK, DEFER or no 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.
--- drivers/gpu/drm/i915/intel_dp.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_dp.c +5 -1
@@ -573,8 +573,12 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, DP_AUX_CH_CTL_RECEIVE_ERROR);if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR | -DP_AUX_CH_CTL_RECEIVE_ERROR )) +DP_AUX_CH_CTL_RECEIVE_ERROR )) { +/* DP compliance requires 400us delay for errors/timeouts +(DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2) */ +udelay (400 );continue ; + }if (status &DP_AUX_CH_CTL_DONE )break ; }
Signature · 1 line
1.9.1
Re: [PATCH 12/12] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
Quoted 6 lines · Todd Previte
The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1 specifies that repeated AUX transactions after a failure (NACK, DEFER or no 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.
Signed-off-by Todd Previte <[email protected]>
Since this is a minimal timeout ... shouldn't we put it into the dp helpers instead?
-Daniel
Quoted 29 lines
--- drivers/gpu/drm/i915/intel_dp.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_dp.c +5
-1@@ -573,8 +573,12 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, DP_AUX_CH_CTL_RECEIVE_ERROR);if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR | -DP_AUX_CH_CTL_RECEIVE_ERROR )) +DP_AUX_CH_CTL_RECEIVE_ERROR )) { +/* DP compliance requires 400us delay for errors/timeouts +(DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2) */ +udelay (400 );continue ; + }if (status &DP_AUX_CH_CTL_DONE )break ; }Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [PATCH 12/12] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
Quoted 9 lines
Daniel Vetter <mailto:[email protected]>
Tuesday, July 15, 2014 12:46 AMQuoted 5 lines · Todd Previte
The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1 specifies that repeated AUX transactions after a failure (NACK, DEFER or no 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.
Quoted 5 lines
Quoted 1 line
Signed-off-by Todd Previte<[email protected]>Since this is a minimal timeout ... shouldn't we put it into the dp helpers instead?
-Daniel
This delay catches the case where the sink device either does not respond at all or responds with an invalid AUX transaction. These are lower level errors than the DRM helper functions are supposed to handle. I mistakenly mentioned NACK/DEFER in the commit message - that's not an accurate description of what this patch does. I will fix the commit message for v3 of this patch.
-T
Quoted 71 lines
Quoted 29 lines
--- drivers/gpu/drm/i915/intel_dp.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_dp.c +5
-1@@ -573,8 +573,12 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, DP_AUX_CH_CTL_RECEIVE_ERROR);if (status& (DP_AUX_CH_CTL_TIME_OUT_ERROR | -DP_AUX_CH_CTL_RECEIVE_ERROR )) +DP_AUX_CH_CTL_RECEIVE_ERROR )) { +/* DP compliance requires 400us delay for errors/timeouts +(DP CTS 1.2 Core Rev 1.1, 4.2.1.1& 4.2.1.2) */ +udelay (400 );continue ; + }if (status&DP_AUX_CH_CTL_DONE )break ; }Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfxTodd Previte <mailto:[email protected]>
Monday, July 14, 2014 12:10 PM
The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1 specifies that repeated AUX transactions after a failure (NACK, DEFER or no 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.Signed-off-by Todd Previte <[email protected]>---
drivers/gpu/drm/i915/intel_dp.c| 6 +++++-1 file changed, 5 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_dp.c +0
-0@@ -573,8 +573,12 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, DP_AUX_CH_CTL_RECEIVE_ERROR);if (status & (
DP_AUX_CH_CTL_TIME_OUT_ERROR|
-DP_AUX_CH_CTL_RECEIVE_ERROR))
+DP_AUX_CH_CTL_RECEIVE_ERROR)) {
+ /* DP compliance requires 400us delay for errors/timeouts
+ (DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2) */
+ udelay(400);
continue;
+ }
if (status &DP_AUX_CH_CTL_DONE)
break;
}
Todd Previte <mailto:[email protected]>
Monday, July 14, 2014 12:10 PMV2:
- Addressed review feedback from the mailing list
- Broke up patches into smaller, easily managed chunks
- Reordered the patches such that they can be applied in order
- Fixed checkpatch.pl errors across the patchset
- Updated and enhanced functionality for the EDID test function
- Completely revamped the mode set operations for compliance testing
Signature · 1 line
Sent with Postbox <http://www.getpostbox.com>
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 12/12] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
2014-07-15 12:34 GMT-03:00 Todd Previte <[email protected]>:
Quoted 17 lines
Daniel Vetter <[email protected]>
Tuesday, July 15, 2014 12:46 AMOn Mon, Jul 14, 2014 at 12:10:47PM -0700, Todd Previte wrote:
The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1 specifies that repeated AUX transactions after a failure (NACK, DEFER or no 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.
Signed-off-by Todd Previte <[email protected]> <[email protected]>Since this is a minimal timeout ... shouldn't we put it into the dp helpers instead?
-Daniel
Quoted 8 lines
This delay catches the case where the sink device either does not respond
at all or responds with an invalid AUX transaction. These are lower level errors than the DRM helper functions are supposed to handle. I mistakenly mentioned NACK/DEFER in the commit message - that's not an accurate description of what this patch does. I will fix the commit message for v3 of this patch.
I agree your patch idea seems to be correct, but you should probably also do a little review of drm_dp_dpcd_access() since it also retries everything 7 times and already has 400ms sleep.
Just one minor comment: why not use usleep_range() instead of udelay()?
Quoted 86 lines
(quoted diff) +0
-1-T --- drivers/gpu/drm/i915/intel_dp.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_dp.c +5
-2@@ -573,8 +573,12 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, DP_AUX_CH_CTL_RECEIVE_ERROR);if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR | -DP_AUX_CH_CTL_RECEIVE_ERROR )) +DP_AUX_CH_CTL_RECEIVE_ERROR )) { +/* DP compliance requires 400us delay for errors/timeouts +(DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2) */ +udelay (400 );continue ; + }if (status &DP_AUX_CH_CTL_DONE )break ; } --1.9.1
_______________________________________________
Intel-gfx mailing [email protected]://lists.freedesktop.org/mailman/listinfo/intel-gfxTodd Previte <[email protected]>
Monday, July 14, 2014 12:10 PM
The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1 specifies that
repeated AUX transactions after a failure (NACK, DEFER or no 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.Signed-off-by Todd Previte <[email protected]> <[email protected]>---
drivers/gpu/drm/i915/intel_dp.c| 6 +++++-1 file changed, 5 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_dp.c +0
-0@@ -573,8 +573,12 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, DP_AUX_CH_CTL_RECEIVE_ERROR);if (status & (
DP_AUX_CH_CTL_TIME_OUT_ERROR|
-DP_AUX_CH_CTL_RECEIVE_ERROR))
+DP_AUX_CH_CTL_RECEIVE_ERROR)) {
+ /* DP compliance requires 400us delay for errors/timeouts
+ (DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2) */
+ udelay(400);
continue;
+ }
if (status &DP_AUX_CH_CTL_DONE)
break;
}
Todd Previte <[email protected]>
Monday, July 14, 2014 12:10 PMV2:
- Addressed review feedback from the mailing list
- Broke up patches into smaller, easily managed chunks
- Reordered the patches such that they can be applied in order
- Fixed checkpatch.pl errors across the patchset
- Updated and enhanced functionality for the EDID test function
- Completely revamped the mode set operations for compliance testing--
Sent with Postbox <http://www.getpostbox.com>_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 7 lines · Todd Previte
Quoted 6 lines
This patch set adds the foundational support for Displayport compliance testing in the i915 driver. It implements the framework for automated test support that preclude the need (most) for operator input during testing. Tests for AUX transactions, EDID reads and basic link training have also been included, along with any support and utility functions required. Some changes have also been made to existing code to accommodate compliance testing.
Quoted 7 lines
V2:
- Addressed review feedback from the mailing list
- Broke up patches into smaller, easily managed chunks
- Reordered the patches such that they can be applied in order
- Fixed checkpatch.pl errors across the patchset
- Updated and enhanced functionality for the EDID test function
- Completely revamped the mode set operations for compliance testing
Ok, high level review of the overall approach.
So your design is to implement special functions for each test procedure. This has two issues:
- We have duplicated code we test, which has a really high chance to be different from what users actually run on their systems. And so dp validation won't actually validate the real code. Example would be the fallback edid reading tests using defer/nacks. The drm i2c helpers already have logic for this (including fallback modes), but very likely it doesn't quite match the dp requirements. So we need to adjust that (presuming the dp standard is sane).
- Even if we can fix this there's still lots of important code in the probe paths we can't test with this. Userspace updates edids through the get_connector ioctl, and there's lots of additional logic in-between until we reach the dp connector implementation in intel_dp.c.
So what I'd like to see is that the test procedures are implemented in userspace, using nothing more than the standard kms interfaces.
Afaics we need three steps overall:
- Get a shot hpd pulse and notice that we should do a validation test sequence. We need to get this information to userspace, and the best approach would be a uevent on the connector in sysfs. We can supply any additional metadata (like the test mode) userspace needs with uevent attributes.
- Do the test from userspace. Depending upon what exactly needs to be done we might need to extend the properties exposed to userspace, e.g. dp link parameters.
- Give the result (edid checksum, ...) back to the dp sink/validator. A generic dp aux transaction interface for userspace in the dp helpers, maybe even as a real /dev node should fit the bill.
dp validation has the potential to automatically test a pile of code for which we currently have 0 automated test coverage at all. I want to fully seize this opportunity instead of doing the bare minimum to get a (rather useless) certification.
-Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 46 lines · On Tue, 22 Jul 2014 08:41:11 +0200 Daniel Vetter <[email protected]> wrote:
Quoted 7 lines · Todd Previte
Quoted 6 lines
This patch set adds the foundational support for Displayport compliance testing in the i915 driver. It implements the framework for automated test support that preclude the need (most) for operator input during testing. Tests for AUX transactions, EDID reads and basic link training have also been included, along with any support and utility functions required. Some changes have also been made to existing code to accommodate compliance testing.
Quoted 7 lines
V2:
- Addressed review feedback from the mailing list
- Broke up patches into smaller, easily managed chunks
- Reordered the patches such that they can be applied in order
- Fixed checkpatch.pl errors across the patchset
- Updated and enhanced functionality for the EDID test function
- Completely revamped the mode set operations for compliance testingOk, high level review of the overall approach.
So your design is to implement special functions for each test procedure. This has two issues:
- We have duplicated code we test, which has a really high chance to be different from what users actually run on their systems. And so dp validation won't actually validate the real code. Example would be the fallback edid reading tests using defer/nacks. The drm i2c helpers already have logic for this (including fallback modes), but very likely it doesn't quite match the dp requirements. So we need to adjust that (presuming the dp standard is sane).
- Even if we can fix this there's still lots of important code in the probe paths we can't test with this. Userspace updates edids through theget_connectorioctl, and there's lots of additional logic in-between until we reach the dp connector implementation inintel_dp.c.So what I'd like to see is that the test procedures are implemented in userspace, using nothing more than the standard kms interfaces.
Afaics we need three steps overall:
- Get a shot hpd pulse and notice that we should do a validation test sequence. We need to get this information to userspace, and the best approach would be a uevent on the connector in sysfs. We can supply any additional metadata (like the test mode) userspace needs with uevent attributes.- Do the test from userspace. Depending upon what exactly needs to be done we might need to extend the properties exposed to userspace, e.g. dp link parameters.
Quoted 8 lines
- Give the result (edid checksum, ...) back to the dp sink/validator. A generic dp aux transaction interface for userspace in the dp helpers, maybe even as a real /dev node should fit the bill.
dp validation has the potential to automatically test a pile of code for which we currently have 0 automated test coverage at all. I want to fully seize this opportunity instead of doing the bare minimum to get a (rather useless) certification.
This amounts to a complete rewrite with a different goal. In this series, I only see two big new test functions added: one for EDID testing and one for link training. The EDID one could probably share some more code with the core, but for link training it seems pretty simple, unless we need a full mode & fb for some reason (I'm presuming Todd has tested this with the equipment). Paulo had some good feedback, but overall this is a small addition to get the compliance tests working and get some coverage on the link training paths which we desperately need.
Having some additional user level tests would be great, but that's a much bigger and different task than simply implementing what's required for the DP compliance test. Asking Todd to take on a huge new task just because he posted this series is a big request. Are you saying you'll reject this approach entirely?
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
Quoted 2 lines · Jesse Barnes
Are you saying
you'll reject this approach entirely?
I'm saying that I don't see terrible lot of value in adding a bunch of code for a sticker, and that we should look into making it actually useful by testing the paths that end-users end up using. And we have to keep this working once it's merged.
But if it doesn't make sense to make this sticker useful while still being able to get it then I'll reconsider.
-Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 11 lines · On Tue, 22 Jul 2014 22:53:44 +0200 Daniel Vetter <[email protected]> wrote:
Quoted 2 lines · Jesse Barnes
Are you saying
you'll reject this approach entirely?I'm saying that I don't see terrible lot of value in adding a bunch of code for a sticker, and that we should look into making it actually useful by testing the paths that end-users end up using. And we have to keep this working once it's merged.
But if it doesn't make sense to make this sticker useful while still being able to get it then I'll reconsider.
Yeah I think it depends on the test. We're supposed to go through existing paths for testing e.g. link training with different params (though with a fixed fb and mode), so getting coverage there is something we want regardless. But getting something like probing covered as part of the compliance testing may be something else entirely...
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
2014-07-22 18:11 GMT-03:00 Jesse Barnes <[email protected]>:
Quoted 15 lines
Quoted 11 lines · On Tue, 22 Jul 2014 22:53:44 +0200 Daniel Vetter <[email protected]> wrote:
Quoted 2 lines · Jesse Barnes
Are you saying
you'll reject this approach entirely?I'm saying that I don't see terrible lot of value in adding a bunch of code for a sticker, and that we should look into making it actually useful by testing the paths that end-users end up using. And we have to keep this working once it's merged.
But if it doesn't make sense to make this sticker useful while still being able to get it then I'll reconsider.
Quoted 6 lines
Yeah I think it depends on the test. We're supposed to go through existing paths for testing e.g. link training with different params (though with a fixed fb and mode), so getting coverage there is something we want regardless. But getting something like probing covered as part of the compliance testing may be something else entirely...
I was finally able to take some time to read the spec, and I agree that the hybrid approach looks like the way to go. Some tests require specifically-crafted FBs, while some other tests cause real hotplug events to be sent from the sink. If there's an unknown/unspecified user-space running when the tests are happening, who knows how it is going to react? Of course, for tests that can be implemented directly inside the Kernel still using the "standard" code paths, we should do it in the Kernel.
One possible approach that I thought would be the following:
- Each DP encoder provides its own debugfs file for DP test compiance (e.g., /sys/kernel/debug/dri/0/i915_ddi_b_dp_test_compliance).
- If the file is not open, any requests for tests that require special actions from our driver - outside of the normal behavior - will be NACKed.
- If the file is open, we ACK test requests and print special strings to the debugfs file telling the user-space app what it's supposed to do. We could use simple strings like "set the preferred mode", "set failsafe mode", "set mode using FB test pattern Y", etc. A stringly typed protocol :)
- The user-space app needs to be the DRM master, open the debugfs file, parse the operations it prints and act accordingly, and listen to the hotplug events sent by the Kernel.
- If some special corner quirky case needs to be done (e.g., train link with a specific number of lanes), the Kernel should store this information at struct intel_dp, and then when a modeset is done on this encoder, we check if the debugfs file is open (i.e., we're doing compliance testing) and then we use the specified configuration. With this, we can probably avoid special uevents or debug-only connector/encoder properties.
- The user-space app could be part of intel-gpu-tools.
Anyway, this is just an alternate idea to Daniel's suggestion, and many other possible implementation ideas would work for me. Todd, what is your opinion?
I will continue the review of the patches we currently have, since it seems we didn't decide what we're gonna merge.
Quoted 7 lines
--
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Paulo Zanoni
Quoted 32 lines · Paulo Zanoni
2014-07-22 18:11 GMT-03:00 Jesse Barnes <[email protected]>:
Quoted 15 lines
Quoted 11 lines · On Tue, 22 Jul 2014 22:53:44 +0200 Daniel Vetter <[email protected]> wrote:
Quoted 2 lines · Jesse Barnes
Are you saying
you'll reject this approach entirely?I'm saying that I don't see terrible lot of value in adding a bunch of code for a sticker, and that we should look into making it actually useful by testing the paths that end-users end up using. And we have to keep this working once it's merged.
But if it doesn't make sense to make this sticker useful while still being able to get it then I'll reconsider.
Quoted 6 lines
Yeah I think it depends on the test. We're supposed to go through existing paths for testing e.g. link training with different params (though with a fixed fb and mode), so getting coverage there is something we want regardless. But getting something like probing covered as part of the compliance testing may be something else entirely...
I was finally able to take some time to read the spec, and I agree that the hybrid approach looks like the way to go. Some tests require specifically-crafted FBs, while some other tests cause real hotplug events to be sent from the sink. If there's an unknown/unspecified user-space running when the tests are happening, who knows how it is going to react? Of course, for tests that can be implemented directly inside the Kernel still using the "standard" code paths, we should do it in the Kernel.
Quoted 6 lines
One possible approach that I thought would be the following:
- Each DP encoder provides its own debugfs file for DP test compiance (e.g.,/sys/kernel/debug/dri/0/i915_ddi_b_dp_test_compliance).
- If the file is not open, any requests for tests that require special actions from our driver - outside of the normal behavior - will be NACKed.
Yeah that sounds like a reasonable safety measure to make sure we don't do stupid things (e.g. malicious DP connector trying to break into the kernel).
Quoted 5 lines
- If the file is open, we ACK test requests and print special strings to the debugfs file telling the user-space app what it's supposed to do. We could use simple strings like "set the preferred mode", "set failsafe mode", "set mode using FB test pattern Y", etc. A stringly typed protocol :)
We need to check how much work this is, since we'll probably need to implement polling. Otoh we've just done that for the crc interfaces, so shouldn't be too much fuzz.
If userspace needs to do special dp aux transactions I still think we should simply expose a proper dp aux interface, similar to i2c.
Quoted 12 lines
- The user-space app needs to be the DRM master, open the debugfs file, parse the operations it prints and act accordingly, and listen to the hotplug events sent by the Kernel.
- If some special corner quirky case needs to be done (e.g., train link with a specific number of lanes), the Kernel should store this information at structintel_dp, and then when a modeset is done on this encoder, we check if the debugfs file is open (i.e., we're doing compliance testing) and then we use the specified configuration. With this, we can probably avoid special uevents or debug-only connector/encoder properties.
- The user-space app could be part of intel-gpu-tools.
Quoted 3 lines
Anyway, this is just an alternate idea to Daniel's suggestion, and many other possible implementation ideas would work for me. Todd, what is your opinion?
Well I've only tossed out a rough idea that we should try to use the same paths for validation as userspace is using in general. Sounds like you have some good ideas how to get there in practice.
-Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 25 lines
Paulo Zanoni <mailto:[email protected]>
Tuesday, July 29, 2014 2:53 PM
2014-07-22 18:11 GMT-03:00 Jesse Barnes<[email protected]>:Quoted 11 lines
Quoted 8 lines · On Tue, 22 Jul 2014 22:53:44 +0200 Daniel Vetter<[email protected]> wrote:
Quoted 2 lines · Jesse Barnes
Are you saying
you'll reject this approach entirely?I'm saying that I don't see terrible lot of value in adding a bunch of code for a sticker, and that we should look into making it actually useful by testing the paths that end-users end up using. And we have to keep this working once it's merged.
Quoted 8 lines
Quoted 2 lines
But if it doesn't make sense to make this sticker useful while still being able to get it then I'll reconsider.
Yeah I think it depends on the test. We're supposed to go through existing paths for testing e.g. link training with different params (though with a fixed fb and mode), so getting coverage there is something we want regardless. But getting something like probing covered as part of the compliance testing may be something else entirely...
Quoted 8 lines
I was finally able to take some time to read the spec, and I agree that the hybrid approach looks like the way to go. Some tests require specifically-crafted FBs, while some other tests cause real hotplug events to be sent from the sink. If there's an unknown/unspecified user-space running when the tests are happening, who knows how it is going to react? Of course, for tests that can be implemented directly inside the Kernel still using the "standard" code paths, we should do it in the Kernel.
There's no question that this is going to require considerable support within the kernel. The tests that can be placed inside the kernel are mostly going to be ones that simply can't be accomplished from userspace (the 400us AUX transaction retry, for instance) or are elements that are necessary to enable the userspace tests to execute properly. The tests themselves though should definitely be handled out in userspace where possible.
Quoted 6 lines
One possible approach that I thought would be the following:
- Each DP encoder provides its own debugfs file for DP test compiance (e.g.,/sys/kernel/debug/dri/0/i915_ddi_b_dp_test_compliance).
- If the file is not open, any requests for tests that require special actions from our driver - outside of the normal behavior - will be NACKed.
There's a couple options here that I've considered. This is a perfectly valid option but it's also possible to just use one debugfs file for the compliance work. It's unlikely that multiple Displayport connectors will ever be testing simultaneously, so the need to adjust the parameters on a per-connector basis is arguable. Only the connector for the sink that issued the test request would be required to read the debugfs file and adhere to its parameters.
For general debugging of Displayport though, this is a better option as it allows for considerably more flexibility and utility. Personally I prefer the individual file approach, even though it might end up being somewhat more complicated to implement. The single-file approach does have merit though, so if anyone has a solid argument in favor of that, it's worth the discussion.
Quoted 5 lines
- If the file is open, we ACK test requests and print special strings to the debugfs file telling the user-space app what it's supposed to do. We could use simple strings like "set the preferred mode", "set failsafe mode", "set mode using FB test pattern Y", etc. A stringly typed protocol :)
Probably better to use clearly-defined constants instead of strings, but yes, that's the idea. :)
Quoted 3 lines
- The user-space app needs to be the DRM master, open the debugfs file, parse the operations it prints and act accordingly, and listen to the hotplug events sent by the Kernel.
Agreed.
Quoted 7 lines
- If some special corner quirky case needs to be done (e.g., train link with a specific number of lanes), the Kernel should store this information at struct
intel_dp, and then when a modeset is done on this encoder, we check if the debugfs file is open (i.e., we're doing compliance testing) and then we use the specified configuration. With this, we can probably avoid special uevents or debug-only connector/encoder properties.
I would argue here that it's better to leave as much of the active configuration undisturbed as possible and only update the fields necessary to complete the test(s). Additionally, any fields that are changed should be saved and subsequently restored upon completion of the test(s)
Quoted 1 line
- The user-space app could be part of intel-gpu-tools.
Cool, this is what I was planning on doing. One thing I was looking into was potentially launching the test app from a uevent in the kernel. But none of the solutions I could find were all that good and some were downright scary. So having the app as a separate entity that needs to be launched by the tester isn't such a bad idea.
Anyway, this is just an alternate idea to Daniel's suggestion, and many other possible implementation ideas would work for me. Todd, what is your opinion?
I will continue the review of the patches we currently have, since it seems we didn't decide what we're gonna merge.
Quoted 94 lines
Quoted 7 lines
--
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfxJesse Barnes <mailto:[email protected]>
Tuesday, July 22, 2014 2:11 PM
On Tue, 22 Jul 2014 22:53:44 +0200Yeah I think it depends on the test. We're supposed to go through existing paths for testing e.g. link training with different params (though with a fixed fb and mode), so getting coverage there is something we want regardless. But getting something like probing covered as part of the compliance testing may be something else entirely...
Daniel Vetter <mailto:[email protected]>
Tuesday, July 22, 2014 1:53 PMI'm saying that I don't see terrible lot of value in adding a bunch of code for a sticker, and that we should look into making it actually useful by testing the paths that end-users end up using. And we have to keep this working once it's merged.
But if it doesn't make sense to make this sticker useful while still being able to get it then I'll reconsider.
-Daniel
Jesse Barnes <mailto:[email protected]>
Tuesday, July 22, 2014 1:48 PM
On Tue, 22 Jul 2014 08:41:11 +0200This amounts to a complete rewrite with a different goal. In this series, I only see two big new test functions added: one for EDID testing and one for link training. The EDID one could probably share some more code with the core, but for link training it seems pretty simple, unless we need a full mode & fb for some reason (I'm presuming Todd has tested this with the equipment). Paulo had some good feedback, but overall this is a small addition to get the compliance tests working and get some coverage on the link training paths which we desperately need.
Having some additional user level tests would be great, but that's a much bigger and different task than simply implementing what's required for the DP compliance test. Asking Todd to take on a huge new task just because he posted this series is a big request. Are you saying you'll reject this approach entirely?
Daniel Vetter <mailto:[email protected]>
Monday, July 21, 2014 11:41 PMOk, high level review of the overall approach.
So your design is to implement special functions for each test procedure. This has two issues:
- We have duplicated code we test, which has a really high chance to be different from what users actually run on their systems. And so dp validation won't actually validate the real code. Example would be the fallback edid reading tests using defer/nacks. The drm i2c helpers already have logic for this (including fallback modes), but very likely it doesn't quite match the dp requirements. So we need to adjust that (presuming the dp standard is sane).
- Even if we can fix this there's still lots of important code in the probe paths we can't test with this. Userspace updates edids through theget_connectorioctl, and there's lots of additional logic in-between until we reach the dp connector implementation inintel_dp.c.So what I'd like to see is that the test procedures are implemented in userspace, using nothing more than the standard kms interfaces.
Afaics we need three steps overall:
- Get a shot hpd pulse and notice that we should do a validation test sequence. We need to get this information to userspace, and the best approach would be a uevent on the connector in sysfs. We can supply any additional metadata (like the test mode) userspace needs with uevent attributes.- Do the test from userspace. Depending upon what exactly needs to be done we might need to extend the properties exposed to userspace, e.g. dp link parameters.
- Give the result (edid checksum, ...) back to the dp sink/validator. A generic dp aux transaction interface for userspace in the dp helpers, maybe even as a real /dev node should fit the bill.
dp validation has the potential to automatically test a pile of code for which we currently have 0 automated test coverage at all. I want to fully seize this opportunity instead of doing the bare minimum to get a (rather useless) certification.
-Daniel
Signature · 1 line
Sent with Postbox <http://www.getpostbox.com>
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 68 lines · On Tue, 22 Jul 2014 13:48:45 -0700 Jesse Barnes <[email protected]>…
Quoted 46 lines · On Tue, 22 Jul 2014 08:41:11 +0200 Daniel Vetter <[email protected]> wrote:
Quoted 7 lines · Todd Previte
Quoted 6 lines
This patch set adds the foundational support for Displayport compliance testing in the i915 driver. It implements the framework for automated test support that preclude the need (most) for operator input during testing. Tests for AUX transactions, EDID reads and basic link training have also been included, along with any support and utility functions required. Some changes have also been made to existing code to accommodate compliance testing.
Quoted 7 lines
V2:
- Addressed review feedback from the mailing list
- Broke up patches into smaller, easily managed chunks
- Reordered the patches such that they can be applied in order
- Fixed checkpatch.pl errors across the patchset
- Updated and enhanced functionality for the EDID test function
- Completely revamped the mode set operations for compliance testingOk, high level review of the overall approach.
So your design is to implement special functions for each test procedure. This has two issues:
- We have duplicated code we test, which has a really high chance to be different from what users actually run on their systems. And so dp validation won't actually validate the real code. Example would be the fallback edid reading tests using defer/nacks. The drm i2c helpers already have logic for this (including fallback modes), but very likely it doesn't quite match the dp requirements. So we need to adjust that (presuming the dp standard is sane).
- Even if we can fix this there's still lots of important code in the probe paths we can't test with this. Userspace updates edids through theget_connectorioctl, and there's lots of additional logic in-between until we reach the dp connector implementation inintel_dp.c.So what I'd like to see is that the test procedures are implemented in userspace, using nothing more than the standard kms interfaces.
Afaics we need three steps overall:
- Get a shot hpd pulse and notice that we should do a validation test sequence. We need to get this information to userspace, and the best approach would be a uevent on the connector in sysfs. We can supply any additional metadata (like the test mode) userspace needs with uevent attributes.- Do the test from userspace. Depending upon what exactly needs to be done we might need to extend the properties exposed to userspace, e.g. dp link parameters.
Quoted 8 lines
- Give the result (edid checksum, ...) back to the dp sink/validator. A generic dp aux transaction interface for userspace in the dp helpers, maybe even as a real /dev node should fit the bill.
dp validation has the potential to automatically test a pile of code for which we currently have 0 automated test coverage at all. I want to fully seize this opportunity instead of doing the bare minimum to get a (rather useless) certification.
This amounts to a complete rewrite with a different goal. In this series, I only see two big new test functions added: one for EDID testing and one for link training. The EDID one could probably share some more code with the core, but for link training it seems pretty simple, unless we need a full mode & fb for some reason (I'm presuming Todd has tested this with the equipment). Paulo had some good feedback, but overall this is a small addition to get the compliance tests working and get some coverage on the link training paths which we desperately need.
Quoted 5 lines
Having some additional user level tests would be great, but that's a much bigger and different task than simply implementing what's required for the DP compliance test. Asking Todd to take on a huge new task just because he posted this series is a big request. Are you saying you'll reject this approach entirely?
I'd have to check the spec, maybe the best approach is a hybrid; some tests handled directly in the kernel to easily re-use our existing bits, and some in userspace to push through a full mode set path more easily.
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center