Displayport Compliance Testing V2
Subsystems (2)
This is version 2 of the patch set for Displayport compliance testing support in the i915 driver. This implementation of compliance testing conforms to the VESA specification Displayport Link Compliance Testing Specification (Link CTS) 1.2 Core Rev1.1. The code has been partitioned into two segments - the kernel side comprised of this patch set and a userspace application that handles the heavy lifting for necessary mode sets. It can also handle link configuration changes if necessary.
Overview Changes for V2:
- Removed the kernel<->userpsace signal mechanism for communication during testing. The original code used signals for communication between the kernel and userspace which met with significant resistance during review. That mechanism has been removed in favor of a file polling mechanism that contains the necessary data for the user app to properly communicate with the kernel during compliance testing.
- Review feedback from as far back as July has been rolled up and integrated into this patch set where necessary/applicable. For those cases, the feedback has been directly noted in the commit message
- Some patches have been either moved or deleted from the set as they have gone through the review cycle. Where this has occurred, it has been noted in the commit message.
drm/i915: Add debugfs write and test param parsing function for DP test control drm/i915: Add new debugfs file for Displaypor compliance test control drm/i915: Update debugfs functions for Displayport compliance testing drm/i915: Add debugfs function to check connector status for compliance testing drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation drm/i915: Update intel_dp_compute_config() to handle compliance test requests drm/i915: Update the EDID automated compliance test function
drm/i915: Implement the 'open' and 'write' debugfs functions for Displayport compliance drm/i915: Add config parsing utilities in debugfs for Displayport compliance drm/i915: Add Displayport link configuration structure
drm/i915: Add and implement the debugfs 'show' functions for Displayport compliance drm/i915: Add support functions in debugfs for handling Displayport compliance configuration drm/i915: Add file ops for Displayport configuration in debugfs
drm/i915: Add debugfs information for Displayport compliance testing drm/i915: Add a delay in Displayport AUX transactions for compliance testing drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing drm/i915: Add automated testing support for Displayport compliance testing
drivers/gpu/drm/i915/i915_debugfs.c | 569 +++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_dp.c | 219 ++++++++++-- drivers/gpu/drm/i915/intel_drv.h | 19 ++ 3 files changed, 779 insertions(+), 28 deletions(-)
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 1/17 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 respond to test automation requests from a sink device.
V2:
- Addressed previous mailing list feedback
- Fixed compilation issue (struct members declared in a later patch)
- Updated debug messages to be more accurate
- Added status checks for the DPCD read/write calls
- Removed excess comments and debug messages
- Fixed debug message compilation warnings
- Fixed compilation issue with missing variables
- Updated link training autotest to ACK
--- drivers/gpu/drm/i915/intel_dp.c | 72 +++++++++++++++++++++++++++++++++++++--- drivers/gpu/drm/i915/intel_drv.h | 4 +++ 2 files changed, 72 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +68 -4
@@ -3744,11 +3744,75 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)return true; } -static void -intel_dp_handle_test_request (struct intel_dp *intel_dp) +static uint8_t intel_dp_autotest_link_training (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_ACK ; +return test_result; +} + +static uint8_t intel_dp_autotest_video_pattern (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +static uint8_t intel_dp_autotest_edid (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +static uint8_t intel_dp_autotest_phy_pattern (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +static void intel_dp_handle_test_request (struct intel_dp *intel_dp) { -/* NAK by default */ -drm_dp_dpcd_writeb (&intel_dp->aux,DP_TEST_RESPONSE ,DP_TEST_NAK ); +uint8_t response =DP_TEST_NAK ; +uint8_t rxdata =0 ; +int status =0 ; + + status =drm_dp_dpcd_read (&intel_dp->aux,DP_TEST_REQUEST , &rxdata,1 ); +if (status !=0 ) { + response =DP_TEST_NAK ; +DRM_DEBUG_KMS ("Could not read test request from sink\n" ); +return ; + } + +switch (rxdata) { +case DP_TEST_LINK_TRAINING : +DRM_DEBUG_KMS ("LINK_TRAINING test requested\n" ); + response =intel_dp_autotest_link_training (intel_dp); +break ; +case DP_TEST_LINK_VIDEO_PATTERN : +DRM_DEBUG_KMS ("TEST_PATTERN test requested\n" ); + response =intel_dp_autotest_video_pattern (intel_dp); +break ; +case DP_TEST_LINK_EDID_READ : +DRM_DEBUG_KMS ("EDID test requested\n" ); + response =intel_dp_autotest_edid (intel_dp); +break ; +case DP_TEST_LINK_PHY_TEST_PATTERN : +DRM_DEBUG_KMS ("PHY_PATTERN test requested\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 ("FAUX_PATTERN testing not supported\n" ); +break ; +default : +DRM_DEBUG_KMS ("Invalid test request '%02x'\n" , rxdata); +break ; + } + status =drm_dp_dpcd_write (&intel_dp->aux, +DP_TEST_RESPONSE , + &response,1 ); +if (status !=0 ) +DRM_DEBUG_KMS ("Could not write test response to sink\n" ); + + intel_dp->compliance_testing_active =0 ; } static int
drivers/gpu/drm/i915/intel_drv.h +4 -0
@@ -638,6 +638,10 @@ struct intel_dp {struct mutex mutex; } drrs_state; +/* Displayport compliance testing */ +unsigned long compliance_test_data; +bool compliance_testing_active; + };struct intel_digital_port {
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 01/17] drm/i915: Add automated testing support for Displayport compliance testing
2014-12-10 21:53 GMT-02:00 Todd Previte <[email protected]>:
Quoted 54 lines
Add the skeleton framework for supporting automation for Displayport compliance testing. This patch adds the necessary framework for the source device to appropriately respond to test automation requests from a sink device.
V2:
- Addressed previous mailing list feedback
- Fixed compilation issue (struct members declared in a later patch)
- Updated debug messages to be more accurate
- Added status checks for the DPCD read/write calls
- Removed excess comments and debug messages
- Fixed debug message compilation warnings
- Fixed compilation issue with missing variables
- Updated link training autotest to ACKSigned-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 72 +++++++++++++++++++++++++++++++++++++--- drivers/gpu/drm/i915/intel_drv.h | 4 +++ 2 files changed, 72 insertions(+), 4 deletions(-)drivers/gpu/drm/i915/intel_dp.c +24
-2@@ -3744,11 +3744,75 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)return true; } -static void -intel_dp_handle_test_request (struct intel_dp *intel_dp) +static uint8_t intel_dp_autotest_link_training (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_ACK ; +return test_result; +} + +static uint8_t intel_dp_autotest_video_pattern (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +static uint8_t intel_dp_autotest_edid (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +static uint8_t intel_dp_autotest_phy_pattern (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} +
drivers/gpu/drm/i915/intel_dp.c intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector) · line 3778
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +7
-2+static void intel_dp_handle_test_request (struct intel_dp *intel_dp) { -/* NAK by default */ -drm_dp_dpcd_writeb (&intel_dp->aux,DP_TEST_RESPONSE ,DP_TEST_NAK ); +uint8_t response =DP_TEST_NAK ; +uint8_t rxdata =0 ; +int status =0 ; + + status =drm_dp_dpcd_read (&intel_dp->aux,DP_TEST_REQUEST , &rxdata,1 ); +if (status !=0 ) {
Why are we checking for zero here? In the "happy case", shouldn't this function return 1? To my understanding, we would be ignoring all test requests from the users, which means you wouldn't be able to test anything in your series at all... I see that you don't change this line at all in the rest of your series, so maybe I'm just crazy and failing to notice some detail...
drivers/gpu/drm/i915/intel_dp.c intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector) · line 3780
Quoted 2 lines
drivers/gpu/drm/i915/intel_dp.c +2
-0+ response =DP_TEST_NAK ; +DRM_DEBUG_KMS ("Could not read test request from sink\n" );
You assign a value to "response" but don't do anything to it. Shouldn't we be trying to send the NAK in this case? If yes, then the code is missing, if no, then I guess we can remove the "response" assignment (well, we could remove it in both cases since it's already initialized to DP_TEST_NAK anyway).
Quoted 22 lines
drivers/gpu/drm/i915/intel_dp.c +22
-0+return ; + } + +switch (rxdata) { +case DP_TEST_LINK_TRAINING : +DRM_DEBUG_KMS ("LINK_TRAINING test requested\n" ); + response =intel_dp_autotest_link_training (intel_dp); +break ; +case DP_TEST_LINK_VIDEO_PATTERN : +DRM_DEBUG_KMS ("TEST_PATTERN test requested\n" ); + response =intel_dp_autotest_video_pattern (intel_dp); +break ; +case DP_TEST_LINK_EDID_READ : +DRM_DEBUG_KMS ("EDID test requested\n" ); + response =intel_dp_autotest_edid (intel_dp); +break ; +case DP_TEST_LINK_PHY_TEST_PATTERN : +DRM_DEBUG_KMS ("PHY_PATTERN test requested\n" ); + response =intel_dp_autotest_phy_pattern (intel_dp); +break ; +/* FAUX is optional in DP 1.2*/ +case DP_TEST_LINK_FAUX_PATTERN :
drivers/gpu/drm/i915/intel_dp.c intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector) · line 3812
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10
-0+DRM_DEBUG_KMS ("FAUX_PATTERN testing not supported\n" ); +break ; +default : +DRM_DEBUG_KMS ("Invalid test request '%02x'\n" , rxdata); +break ; + } + status =drm_dp_dpcd_write (&intel_dp->aux, +DP_TEST_RESPONSE , + &response,1 ); +if (status !=0 )
Same here...
Quoted 11 lines
drivers/gpu/drm/i915/intel_dp.c +3
-0+DRM_DEBUG_KMS ("Could not write test response to sink\n" ); + + intel_dp->compliance_testing_active =0 ; } static intdrivers/gpu/drm/i915/intel_drv.h +0
-0
drivers/gpu/drm/i915/intel_drv.h struct intel_dp { · line 643
Quoted 6 lines
drivers/gpu/drm/i915/intel_drv.h +3
-0@@ -638,6 +638,10 @@ struct intel_dp {struct mutex mutex; } drrs_state; +/* Displayport compliance testing */ +unsigned long compliance_test_data; +bool compliance_testing_active;
Not a change request, but just a note: usually it's better to just add new field/members in the patches that actually start using them. Because sometimes we merge the first patches before the others, and we may decide to change the later patches so they stop using those fields, so we risk ending with unused space. Also, adding a field just in the patch that uses it allows the reviewer to check if the chosen type, name and location are appropriate, etc.
Quoted 11 lines
drivers/gpu/drm/i915/intel_drv.h +1
-1+ };struct intel_digital_port { --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/17] drm/i915: Add automated testing support for Displayport compliance testing
Quoted 73 lines · Paulo Zanoni
2014-12-10 21:53 GMT-02:00 Todd Previte<[email protected]>:
Quoted 54 lines
Add the skeleton framework for supporting automation for Displayport compliance testing. This patch adds the necessary framework for the source device to appropriately respond to test automation requests from a sink device.
V2:
- Addressed previous mailing list feedback
- Fixed compilation issue (struct members declared in a later patch)
- Updated debug messages to be more accurate
- Added status checks for the DPCD read/write calls
- Removed excess comments and debug messages
- Fixed debug message compilation warnings
- Fixed compilation issue with missing variables
- Updated link training autotest to ACKSigned-off-by Todd Previte<[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 72 +++++++++++++++++++++++++++++++++++++--- drivers/gpu/drm/i915/intel_drv.h | 4 +++ 2 files changed, 72 insertions(+), 4 deletions(-)drivers/gpu/drm/i915/intel_dp.c +24
-2@@ -3744,11 +3744,75 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)return true; } -static void -intel_dp_handle_test_request (struct intel_dp *intel_dp) +static uint8_t intel_dp_autotest_link_training (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_ACK ; +return test_result; +} + +static uint8_t intel_dp_autotest_video_pattern (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +static uint8_t intel_dp_autotest_edid (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} + +static uint8_t intel_dp_autotest_phy_pattern (struct intel_dp *intel_dp) +{ +uint8_t test_result =DP_TEST_NAK ; +return test_result; +} +drivers/gpu/drm/i915/intel_dp.c intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector) · line 3778
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +7
-2+static void intel_dp_handle_test_request (struct intel_dp *intel_dp) { -/* NAK by default */ -drm_dp_dpcd_writeb (&intel_dp->aux,DP_TEST_RESPONSE ,DP_TEST_NAK ); +uint8_t response =DP_TEST_NAK ; +uint8_t rxdata =0 ; +int status =0 ; + + status =drm_dp_dpcd_read (&intel_dp->aux,DP_TEST_REQUEST , &rxdata,1 ); +if (status !=0 ) {Why are we checking for zero here? In the "happy case", shouldn't this function return 1? To my understanding, we would be ignoring all test requests from the users, which means you wouldn't be able to test anything in your series at all... I see that you don't change this line at all in the rest of your series, so maybe I'm just crazy and failing to notice some detail...
Quoted 7 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector) · line 3780
Quoted 2 lines
drivers/gpu/drm/i915/intel_dp.c +2
-0+ response =DP_TEST_NAK ; +DRM_DEBUG_KMS ("Could not read test request from sink\n" );You assign a value to "response" but don't do anything to it. Shouldn't we be trying to send the NAK in this case? If yes, then the code is missing, if no, then I guess we can remove the "response" assignment (well, we could remove it in both cases since it's already initialized to
DP_TEST_NAKanyway).
Good catches on these two - thanks Paulo. They've been fixed in V3.
Quoted 32 lines
Quoted 22 lines
drivers/gpu/drm/i915/intel_dp.c +22
-0+return ; + } + +switch (rxdata) { +case DP_TEST_LINK_TRAINING : +DRM_DEBUG_KMS ("LINK_TRAINING test requested\n" ); + response =intel_dp_autotest_link_training (intel_dp); +break ; +case DP_TEST_LINK_VIDEO_PATTERN : +DRM_DEBUG_KMS ("TEST_PATTERN test requested\n" ); + response =intel_dp_autotest_video_pattern (intel_dp); +break ; +case DP_TEST_LINK_EDID_READ : +DRM_DEBUG_KMS ("EDID test requested\n" ); + response =intel_dp_autotest_edid (intel_dp); +break ; +case DP_TEST_LINK_PHY_TEST_PATTERN : +DRM_DEBUG_KMS ("PHY_PATTERN test requested\n" ); + response =intel_dp_autotest_phy_pattern (intel_dp); +break ; +/* FAUX is optional in DP 1.2*/ +case DP_TEST_LINK_FAUX_PATTERN :drivers/gpu/drm/i915/intel_dp.c intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector) · line 3812
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10
-0+DRM_DEBUG_KMS ("FAUX_PATTERN testing not supported\n" ); +break ; +default : +DRM_DEBUG_KMS ("Invalid test request '%02x'\n" , rxdata); +break ; + } + status =drm_dp_dpcd_write (&intel_dp->aux, +DP_TEST_RESPONSE , + &response,1 ); +if (status !=0 )
Quoted 2 lines
Same here...
Same as above. Fixed in V3.
Quoted 14 lines
Quoted 14 lines
drivers/gpu/drm/i915/intel_dp.c +3
-0+DRM_DEBUG_KMS ("Could not write test response to sink\n" ); + + intel_dp->compliance_testing_active =0 ; } static intdrivers/gpu/drm/i915/intel_drv.h +0
-0@@ -638,6 +638,10 @@ struct intel_dp {struct mutex mutex; } drrs_state;
Quoted 11 lines
drivers/gpu/drm/i915/intel_drv.h struct intel_dp { · line 643
Quoted 3 lines
drivers/gpu/drm/i915/intel_drv.h +3
-0+/* Displayport compliance testing */ +unsigned long compliance_test_data; +bool compliance_testing_active;Not a change request, but just a note: usually it's better to just add new field/members in the patches that actually start using them. Because sometimes we merge the first patches before the others, and we may decide to change the later patches so they stop using those fields, so we risk ending with unused space. Also, adding a field just in the patch that uses it allows the reviewer to check if the chosen type, name and location are appropriate, etc.
Ok I'll keep this in mind moving forward. Thanks Paulo!
Quoted 12 lines
Quoted 11 lines
drivers/gpu/drm/i915/intel_drv.h +1
-1+ };struct intel_digital_port { --1.9.1
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 2/17 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, 19 insertions(+), 13 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +19 -13
@@ -3890,21 +3890,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)WARN_ON (!drm_modeset_is_locked (&dev->mode_config.connection_mutex)); -if (!intel_encoder->connectors_active) -return ; - -if (WARN_ON (!intel_encoder->base.crtc)) -return ; - -if (!to_intel_crtc (intel_encoder->base.crtc)->active) -return ; - -/* Try to read receiver status if the link appears to be up */ -if (!intel_dp_get_link_status (intel_dp, link_status)) { +/* Bail if not connected */ +if (intel_dp->attached_connector->base.status != + connector_status_connected) { +DRM_DEBUG_KMS ("Not connected\n" );return ; } -/* Now read the DPCD to see if it's actually running */ +/* Attempt to read the DPCD */ if (!intel_dp_get_dpcd (intel_dp)) {return ; }
@@ -3916,13 +3909,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)drm_dp_dpcd_writeb (&intel_dp->aux,DP_DEVICE_SERVICE_IRQ_VECTOR , sink_irq_vector); -if (sink_irq_vector &DP_AUTOMATED_TEST_REQUEST )intel_dp_handle_test_request (intel_dp);if (sink_irq_vector & (DP_CP_IRQ |DP_SINK_SPECIFIC_IRQ ))DRM_DEBUG_DRIVER ("CP or sink specific irq unhandled\n" ); } +if (!intel_encoder->connectors_active) +return ; + +if (WARN_ON (!intel_encoder->base.crtc)) +return ; + +if (!to_intel_crtc (intel_encoder->base.crtc)->active) +return ; + +/* Try to read receiver status if the link appears to be up */ +if (!intel_dp_get_link_status (intel_dp, link_status)) { +return ; + } +if (!drm_dp_channel_eq_ok (link_status, intel_dp->lane_count)) {DRM_DEBUG_KMS ("%s: channel EQ not ok, retraining\n" , intel_encoder->base.name);
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 02/17] drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing
2014-12-10 21:53 GMT-02:00 Todd Previte <[email protected]>:
Quoted 4 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.
Why exactly is this needed?
Quoted 3 lines
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.
Why exactly? Can you please describe in terms of how the code is executed in each case and what is missing?
Since there appears to be 2 different changes, shouldn't this patch be split into 2 different patches?
Quoted 19 lines
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.Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)drivers/gpu/drm/i915/intel_dp.c +0
-2@@ -3890,21 +3890,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)WARN_ON (!drm_modeset_is_locked (&dev->mode_config.connection_mutex)); -if (!intel_encoder->connectors_active) -return ;
drivers/gpu/drm/i915/intel_dp.c intel_dp_check_link_status(struct intel_dp *intel_dp) · line 3893
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +1
-9- -if (WARN_ON (!intel_encoder->base.crtc)) -return ; - -if (!to_intel_crtc (intel_encoder->base.crtc)->active) -return ; - -/* Try to read receiver status if the link appears to be up */ -if (!intel_dp_get_link_status (intel_dp, link_status)) { +/* Bail if not connected */
Bikeshed: I don't see the reason for the comment above :)
Other than that, the patch looks fine. Let's see what PRTS will say about it.
Quoted 46 lines
drivers/gpu/drm/i915/intel_dp.c +18
-3+if (intel_dp->attached_connector->base.status != + connector_status_connected) { +DRM_DEBUG_KMS ("Not connected\n" );return ; } -/* Now read the DPCD to see if it's actually running */ +/* Attempt to read the DPCD */ if (!intel_dp_get_dpcd (intel_dp)) {return ; }@@ -3916,13 +3909,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)drm_dp_dpcd_writeb (&intel_dp->aux,DP_DEVICE_SERVICE_IRQ_VECTOR , sink_irq_vector); -if (sink_irq_vector &DP_AUTOMATED_TEST_REQUEST )intel_dp_handle_test_request (intel_dp);if (sink_irq_vector & (DP_CP_IRQ |DP_SINK_SPECIFIC_IRQ ))DRM_DEBUG_DRIVER ("CP or sink specific irq unhandled\n" ); } +if (!intel_encoder->connectors_active) +return ; + +if (WARN_ON (!intel_encoder->base.crtc)) +return ; + +if (!to_intel_crtc (intel_encoder->base.crtc)->active) +return ; + +/* Try to read receiver status if the link appears to be up */ +if (!intel_dp_get_link_status (intel_dp, link_status)) { +return ; + } +if (!drm_dp_channel_eq_ok (link_status, intel_dp->lane_count)) {DRM_DEBUG_KMS ("%s: channel EQ not ok, retraining\n" , intel_encoder->base.name); --1.9.1
_______________________________________________
Intel-gfx mailing list
[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 02/17] drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing
Quoted 6 lines · Paulo Zanoni
2014-12-10 21:53 GMT-02:00 Todd Previte<[email protected]>:
Quoted 4 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.
Why exactly is this needed?
The main reason for doing this is to make sure that a test request isn't missed. Checking for the status of the encoder/crtc isn't necessary for some test cases (AUX channel tests are one example) and without moving the check for the interrupt, these tests may not execute if one of those checks fails. Additionally, if reading the DPCD fails, regardless of whether or not testing is happening, there's no way to train the link since configurations and status can't be read, nor can link training parameters be written.
Quoted 4 lines
Quoted 2 lines
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.
Why exactly? Can you please describe in terms of how the code is executed in each case and what is missing?
This patch is actually both a bug fix and a component of compliance testing. Because HPD events are received both on connect and disconnect actions, it's vital that we don't try and train the link when we're transitioning from connected->disconnected. That results in errors and warning in the logs from failed AUX transactions and can trigger the WARN for the check of !base.crtc. By making the check at the beginning to see if the connection is truly active, those problems are avoided and testing / link training will only be attempted when there is a valid Displayport connection.
Quoted 2 lines
Since there appears to be 2 different changes, shouldn't this patch be split into 2 different patches?
It can be split into two if that will make it easier to upstream. The changes are separate but related, which is why I grouped them into a single patch.
Quoted 31 lines
Quoted 19 lines
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.Signed-off-by Todd Previte<[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)drivers/gpu/drm/i915/intel_dp.c +0
-2@@ -3890,21 +3890,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)WARN_ON (!drm_modeset_is_locked (&dev->mode_config.connection_mutex)); -if (!intel_encoder->connectors_active) -return ;drivers/gpu/drm/i915/intel_dp.c intel_dp_check_link_status(struct intel_dp *intel_dp) · line 3893
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +1
-9- -if (WARN_ON (!intel_encoder->base.crtc)) -return ; - -if (!to_intel_crtc (intel_encoder->base.crtc)->active) -return ; - -/* Try to read receiver status if the link appears to be up */ -if (!intel_dp_get_link_status (intel_dp, link_status)) { +/* Bail if not connected */ Bikeshed: I don't see the reason for the comment above :)
Quoted 1 line
Other than that, the patch looks fine. Let's see what PRTS will say about it.
Fair enough. Comment deleted. :) Fix will be in patch V3. The above responses to your questions were also included in the patch notes for further explanation of this patch.
Quoted 42 lines
Quoted 42 lines
drivers/gpu/drm/i915/intel_dp.c +18
-3+if (intel_dp->attached_connector->base.status != + connector_status_connected) { +DRM_DEBUG_KMS ("Not connected\n" );return ; } -/* Now read the DPCD to see if it's actually running */ +/* Attempt to read the DPCD */ if (!intel_dp_get_dpcd (intel_dp)) {return ; }@@ -3916,13 +3909,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)drm_dp_dpcd_writeb (&intel_dp->aux,DP_DEVICE_SERVICE_IRQ_VECTOR , sink_irq_vector); -if (sink_irq_vector &DP_AUTOMATED_TEST_REQUEST )intel_dp_handle_test_request (intel_dp);if (sink_irq_vector & (DP_CP_IRQ |DP_SINK_SPECIFIC_IRQ ))DRM_DEBUG_DRIVER ("CP or sink specific irq unhandled\n" ); } +if (!intel_encoder->connectors_active) +return ; + +if (WARN_ON (!intel_encoder->base.crtc)) +return ; + +if (!to_intel_crtc (intel_encoder->base.crtc)->active) +return ; + +/* Try to read receiver status if the link appears to be up */ +if (!intel_dp_get_link_status (intel_dp, link_status)) { +return ; + } +if (!drm_dp_channel_eq_ok (link_status, intel_dp->lane_count)) {DRM_DEBUG_KMS ("%s: channel EQ not ok, retraining\n" , intel_encoder->base.name); --1.9.1
Quoted 4 lines
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 02/17] drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing
2015-02-18 14:36 GMT-02:00 Todd Previte <[email protected]>:
Quoted 41 lines · Paulo Zanoni
Quoted 11 lines
2014-12-10 21:53 GMT-02:00 Todd Previte<[email protected]>:
Quoted 8 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.Quoted 1 line
Why exactly is this needed?
The main reason for doing this is to make sure that a test request isn't missed. Checking for the status of the encoder/crtc isn't necessary for some test cases (AUX channel tests are one example) and without moving the check for the interrupt, these tests may not execute if one of those checks fails. Additionally, if reading the DPCD fails, regardless of whether or not testing is happening, there's no way to train the link since configurations and status can't be read, nor can link training parameters be written.
Quoted 7 lines
Quoted 4 lines
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.Why exactly? Can you please describe in terms of how the code is executed in each case and what is missing?
This patch is actually both a bug fix and a component of compliance testing. Because HPD events are received both on connect and disconnect actions, it's vital that we don't try and train the link when we're transitioning from
connected->disconnected. That results in errors and warning in the logs from failed AUX transactions and can trigger the WARN for the check of !base.crtc. By making the check at the beginning to see if the connection is truly active, those problems are avoided and testing / link training will only be attempted when there is a valid Displayport connection.
Quoted 6 lines
Quoted 2 lines
Since there appears to be 2 different changes, shouldn't this patch be split into 2 different patches?
It can be split into two if that will make it easier to upstream. The changes are separate but related, which is why I grouped them into a single patch.
The big reason for splitting is the bisectability. In case we ever bisect a bug to this patch, we'll probably have to ask the user to test additional patches so we can check which of the logical changes actually introduced the problem. Anyway, since it's still a small patch, I don't think it's a big problem, so splitting is not a hard requirement IMHO.
Quoted 98 lines
Quoted 38 lines
Quoted 25 lines
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.Signed-off-by Todd Previte<[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)drivers/gpu/drm/i915/intel_dp.c +0
-2@@ -3890,21 +3890,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)WARN_ON (!drm_modeset_is_locked (&dev->mode_config.connection_mutex)); -if (!intel_encoder->connectors_active) -return ;drivers/gpu/drm/i915/intel_dp.c intel_dp_check_link_status(struct intel_dp · line 3895
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +1
-9- -if (WARN_ON (!intel_encoder->base.crtc)) -return ; - -if (!to_intel_crtc (intel_encoder->base.crtc)->active) -return ; - -/* Try to read receiver status if the link appears to be up */ -if (!intel_dp_get_link_status (intel_dp, link_status)) { +/* Bail if not connected */ Bikeshed: I don't see the reason for the comment above :)
Quoted 2 lines
Other than that, the patch looks fine. Let's see what PRTS will say about it.
Fair enough. Comment deleted. :) Fix will be in patch V3. The above responses to your questions were also included in the patch notes for further explanation of this patch.
Quoted 49 lines
Quoted 49 lines
drivers/gpu/drm/i915/intel_dp.c +4
-1+if (intel_dp->attached_connector->base.status != + connector_status_connected) { +DRM_DEBUG_KMS ("Not connected\n" );return ; } -/* Now read the DPCD to see if it's actually running */ +/* Attempt to read the DPCD */ if (!intel_dp_get_dpcd (intel_dp)) {return; }drivers/gpu/drm/i915/intel_dp.c +14
-1@@ -3916,13 +3909,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)drm_dp_dpcd_writeb (&intel_dp->aux,DP_DEVICE_SERVICE_IRQ_VECTOR , sink_irq_vector); -if (sink_irq_vector &DP_AUTOMATED_TEST_REQUEST )intel_dp_handle_test_request (intel_dp);if (sink_irq_vector & (DP_CP_IRQ |DP_SINK_SPECIFIC_IRQ ))DRM_DEBUG_DRIVER ("CP or sink specific irq unhandled\n"); } +if (!intel_encoder->connectors_active) +return ; + +if (WARN_ON (!intel_encoder->base.crtc)) +return ; + +if (!to_intel_crtc (intel_encoder->base.crtc)->active) +return ; + +/* Try to read receiver status if the link appears to be up */ +if (!intel_dp_get_link_status (intel_dp, link_status)) { +return ; + } +if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", intel_encoder->base.name);--
1.9.1_______________________________________________
Intel-gfx mailing list
[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/17 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 (no response / invalid response) must have a minimum delay of 400us before the resend can occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.
V2:
- Changed udelay() to usleep_range()
--- drivers/gpu/drm/i915/intel_dp.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_dp.c +6 -1
@@ -876,9 +876,14 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,DP_AUX_CH_CTL_TIME_OUT_ERROR | DP_AUX_CH_CTL_RECEIVE_ERROR); +/* DP compliance requires 400us delay for errors +and timeouts (DP CTS 1.2 Core Rev 1.1, 4.2.1.1 +& 4.2.1.2) */ if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR | -DP_AUX_CH_CTL_RECEIVE_ERROR )) +DP_AUX_CH_CTL_RECEIVE_ERROR )) { +usleep_range (400 ,500 );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
Re: [PATCH 03/17] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
2014-12-10 21:53 GMT-02:00 Todd Previte <[email protected]>:
Quoted 18 lines
The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1 specifies that repeated AUX transactions after a failure (no response / invalid response) must have a minimum delay of 400us before the resend can occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.
V2:
- Changedudelay()tousleep_range()Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_dp.c +0
-0
drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 884
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +5
-1@@ -876,9 +876,14 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,DP_AUX_CH_CTL_TIME_OUT_ERROR | DP_AUX_CH_CTL_RECEIVE_ERROR); +/* DP compliance requires 400us delay for errors +and timeouts (DP CTS 1.2 Core Rev 1.1, 4.2.1.1 +& 4.2.1.2) */ if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR | -DP_AUX_CH_CTL_RECEIVE_ERROR )) +DP_AUX_CH_CTL_RECEIVE_ERROR )) { +usleep_range (400 ,500 );
One thing to notice is that if we get a TIME_OUT_ERROR I guess it means we already waited our standard timeout (which is either 400, 600 or 1600, depending on the platform), so shouldn't we just do the usleep() after the RECEIVE_ERROR error?
Quoted 12 lines
drivers/gpu/drm/i915/intel_dp.c +1
-1continue ; + }if (status &DP_AUX_CH_CTL_DONE )break ; } --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 03/17] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
Quoted 22 lines · Paulo Zanoni
2014-12-10 21:53 GMT-02:00 Todd Previte<[email protected]>:
Quoted 21 lines
The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1 specifies that repeated AUX transactions after a failure (no response / invalid response) must have a minimum delay of 400us before the resend can occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.
V2:
- Changedudelay()tousleep_range()Signed-off-by Todd Previte<[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_dp.c +0
-0@@ -876,9 +876,14 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,DP_AUX_CH_CTL_TIME_OUT_ERROR | DP_AUX_CH_CTL_RECEIVE_ERROR);
Quoted 11 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 884
Quoted 7 lines
drivers/gpu/drm/i915/intel_dp.c +5
-1+/* DP compliance requires 400us delay for errors +and timeouts (DP CTS 1.2 Core Rev 1.1, 4.2.1.1 +& 4.2.1.2) */ if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR | -DP_AUX_CH_CTL_RECEIVE_ERROR )) +DP_AUX_CH_CTL_RECEIVE_ERROR )) { +usleep_range (400 ,500 );One thing to notice is that if we get a
TIME_OUT_ERRORI guess it means we already waited our standard timeout (which is either 400, 600 or 1600, depending on the platform), so shouldn't we just do theusleep()after theRECEIVE_ERRORerror?
Capture of IRC discussion:
It seems reasonable to just continue to the next iteration if the HW has already hit a timeout condition. Testing has determined that it is not necessary to handle the timeout case, since the HW has already waited the required amount of time. That case has been removed and the comment has been updated for V3.
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/i915/intel_dp.c +1
-1continue ; + }if (status &DP_AUX_CH_CTL_DONE )break ; } --1.9.1
Quoted 4 lines
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 4/17 drm/i915: Add debugfs information for Displayport compliance testing
In reply to Todd PreviteThis patch was part of "[PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing". That patch has been split into smaller patches for ease of review and integration.
This patch contains the definitions/declarations for some of the constants and data structures added to support debugfs output for Displayport compliance testing.
--- drivers/gpu/drm/i915/i915_debugfs.c | 41 +++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
drivers/gpu/drm/i915/i915_debugfs.c +41 -0
@@ -33,6 +33,7 @@#include <linux/slab.h> #include <linux/export.h> #include <linux/list_sort.h> +#include <linux/string.h> #include <asm/msr-index.h> #include <drm/drmP.h> #include "intel_drv.h"
@@ -51,6 +52,46 @@ static const char *yesno(int v)return v ?"yes" :"no" ; } +#define DP_PARAMETER_COUNT 8 +#define MAX_DP_CONFIG_LINE_COUNT 64 + +enum dp_config_param { +DP_CONFIG_PARAM_INVALID =-1 , +DP_CONFIG_PARAM_CONNECTOR =0 , +DP_CONFIG_PARAM_LINK_RATE , +DP_CONFIG_PARAM_LANE_COUNT , +DP_CONFIG_PARAM_VOLTAGE_SWING , +DP_CONFIG_PARAM_PREEMPHASIS , +DP_CONFIG_PARAM_HRES , +DP_CONFIG_PARAM_VRES , +DP_CONFIG_PARAM_BPP +}; + +struct dp_config { +enum dp_config_param type; +unsigned long value; +}; + +const char *dp_conf_tokens[DP_PARAMETER_COUNT ] = { +"Connector" , +"Link Rate" , +"Lane Count" , +"Voltage Swing Level" , +"Preemphasis Level" , +"Horizontal Resolution" , +"Vertical Resolution" , +"Bits Per Pixel" +}; +/* Strings for writing out dp_configs */ +#define DP_CONF_STR_CONNECTOR "Connector: %s\n" +#define DP_CONF_STR_LINKRATE "Link Rate: %02x\n" +#define DP_CONF_STR_LANES "Lane Count: %u\n" +#define DP_CONF_STR_VSWING "Voltage Swing Level: %u\n" +#define DP_CONF_STR_PREEMP "Preemphasis Level: %u\n" +#define DP_CONF_STR_HRES "Horizontal Resolution: %d\n" +#define DP_CONF_STR_VRES "Vertical Resolution: %d\n" +#define DP_CONF_STR_BPP "Bits Per Pixel: %u\n" +/* As the drm_debugfs_init() routines are called before dev->dev_private is * allocated we need to hook into the minor for release. */ static int
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jani Nikula
maintainerRe: [PATCH 04/17] drm/i915: Add debugfs information for Displayport compliance testing
Quoted 32 lines · Todd Previte
This patch was part of "[PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing". That patch has been split into smaller patches for ease of review and integration.
This patch contains the definitions/declarations for some of the constants and data structures added to support debugfs output for Displayport compliance testing.
Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/i915_debugfs.c | 41 +++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)drivers/gpu/drm/i915/i915_debugfs.c +4
-0@@ -33,6 +33,7 @@#include <linux/slab.h> #include <linux/export.h> #include <linux/list_sort.h> +#include <linux/string.h> #include <asm/msr-index.h> #include <drm/drmP.h> #include "intel_drv.h" @@ -51,6 +52,46 @@ static const char *yesno(int v)return v ?"yes" :"no" ; } +#define DP_PARAMETER_COUNT 8 +#define MAX_DP_CONFIG_LINE_COUNT 64 +
drivers/gpu/drm/i915/i915_debugfs.c static const char *yesno(int v) · line 67
Quoted 10 lines
drivers/gpu/drm/i915/i915_debugfs.c +10
-0+enum dp_config_param { +DP_CONFIG_PARAM_INVALID =-1 , +DP_CONFIG_PARAM_CONNECTOR =0 , +DP_CONFIG_PARAM_LINK_RATE , +DP_CONFIG_PARAM_LANE_COUNT , +DP_CONFIG_PARAM_VOLTAGE_SWING , +DP_CONFIG_PARAM_PREEMPHASIS , +DP_CONFIG_PARAM_HRES , +DP_CONFIG_PARAM_VRES , +DP_CONFIG_PARAM_BPP
If you start the enum with DP_CONFIG_PARAM_CONNECTOR, you can add DP_PARAMETER_COUNT here which will automatically be correct, and then add DP_CONFIG_PARAM_INVALID = -1 after that.
Quoted 6 lines
drivers/gpu/drm/i915/i915_debugfs.c +6
-0+}; + +struct dp_config { +enum dp_config_param type; +unsigned long value; +};
drivers/gpu/drm/i915/i915_debugfs.c static const char *yesno(int v) · line 83
Quoted 10 lines
drivers/gpu/drm/i915/i915_debugfs.c +10
-0+ +const char *dp_conf_tokens[DP_PARAMETER_COUNT ] = { +"Connector" , +"Link Rate" , +"Lane Count" , +"Voltage Swing Level" , +"Preemphasis Level" , +"Horizontal Resolution" , +"Vertical Resolution" , +"Bits Per Pixel"
It's customary to initialize these with designated initializers so that the order here is not tied to the enum. I.e.
[DP_CONFIG_PARAM_CONNECTOR] = "Connector",
drivers/gpu/drm/i915/i915_debugfs.c static const char *yesno(int v) · line 93
Quoted 10 lines
drivers/gpu/drm/i915/i915_debugfs.c +10
-0+}; +/* Strings for writing out dp_configs */ +#define DP_CONF_STR_CONNECTOR "Connector: %s\n" +#define DP_CONF_STR_LINKRATE "Link Rate: %02x\n" +#define DP_CONF_STR_LANES "Lane Count: %u\n" +#define DP_CONF_STR_VSWING "Voltage Swing Level: %u\n" +#define DP_CONF_STR_PREEMP "Preemphasis Level: %u\n" +#define DP_CONF_STR_HRES "Horizontal Resolution: %d\n" +#define DP_CONF_STR_VRES "Vertical Resolution: %d\n" +#define DP_CONF_STR_BPP "Bits Per Pixel: %u\n"
I really don't like format strings as defines, because it moves the format string away from the parameters that need to match the format.
A better option might be to have helper wrappers for seq_printf that take the required parameters.
BR,
Jani.
Quoted 11 lines
drivers/gpu/drm/i915/i915_debugfs.c +1
-0+/* As the drm_debugfs_init() routines are called before dev->dev_private is * allocated we need to hook into the minor for release. */ static intSignature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 04/17] drm/i915: Add debugfs information for Displayport compliance testing
Quoted 30 lines · Jani Nikula
Quoted 29 lines · Todd Previte
This patch was part of "[PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing". That patch has been split into smaller patches for ease of review and integration.
This patch contains the definitions/declarations for some of the constants and data structures added to support debugfs output for Displayport compliance testing.
Signed-off-by Todd Previte<[email protected]>--- drivers/gpu/drm/i915/i915_debugfs.c | 41 +++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)drivers/gpu/drm/i915/i915_debugfs.c +1
-0@@ -33,6 +33,7 @@#include <linux/slab.h> #include <linux/export.h> #include <linux/list_sort.h> +#include <linux/string.h> #include <asm/msr-index.h> #include <drm/drmP.h> #include "intel_drv.h" @@ -51,6 +52,46 @@ static const char *yesno(int v)return v ?"yes" :"no" ; }
Quoted 16 lines
drivers/gpu/drm/i915/i915_debugfs.c static const char *yesno(int v) · line 67
Quoted 13 lines
drivers/gpu/drm/i915/i915_debugfs.c +13
-0+#define DP_PARAMETER_COUNT 8 +#define MAX_DP_CONFIG_LINE_COUNT 64 + +enum dp_config_param { +DP_CONFIG_PARAM_INVALID =-1 , +DP_CONFIG_PARAM_CONNECTOR =0 , +DP_CONFIG_PARAM_LINK_RATE , +DP_CONFIG_PARAM_LANE_COUNT , +DP_CONFIG_PARAM_VOLTAGE_SWING , +DP_CONFIG_PARAM_PREEMPHASIS , +DP_CONFIG_PARAM_HRES , +DP_CONFIG_PARAM_VRES , +DP_CONFIG_PARAM_BPP If you start the enum with
DP_CONFIG_PARAM_CONNECTOR, you can addDP_PARAMETER_COUNThere which will automatically be correct, and then addDP_CONFIG_PARAM_INVALID= -1 after that.
Good point. That change has been integrated into V3.
Quoted 19 lines
Quoted 6 lines
drivers/gpu/drm/i915/i915_debugfs.c +6
-0+}; + +struct dp_config { +enum dp_config_param type; +unsigned long value; +};drivers/gpu/drm/i915/i915_debugfs.c static const char *yesno(int v) · line 83
Quoted 10 lines
drivers/gpu/drm/i915/i915_debugfs.c +10
-0+ +const char *dp_conf_tokens[DP_PARAMETER_COUNT ] = { +"Connector" , +"Link Rate" , +"Lane Count" , +"Voltage Swing Level" , +"Preemphasis Level" , +"Horizontal Resolution" , +"Vertical Resolution" , +"Bits Per Pixel" It's customary to initialize these with designated initializers so that the order here is not tied to the enum. I.e.
Quoted 1 line
[
DP_CONFIG_PARAM_CONNECTOR] = "Connector",
Makes sense. Fixed for V3.
Quoted 13 lines
drivers/gpu/drm/i915/i915_debugfs.c static const char *yesno(int v) · line 93
Quoted 10 lines
drivers/gpu/drm/i915/i915_debugfs.c +10
-0+}; +/* Strings for writing out dp_configs */ +#define DP_CONF_STR_CONNECTOR "Connector: %s\n" +#define DP_CONF_STR_LINKRATE "Link Rate: %02x\n" +#define DP_CONF_STR_LANES "Lane Count: %u\n" +#define DP_CONF_STR_VSWING "Voltage Swing Level: %u\n" +#define DP_CONF_STR_PREEMP "Preemphasis Level: %u\n" +#define DP_CONF_STR_HRES "Horizontal Resolution: %d\n" +#define DP_CONF_STR_VRES "Vertical Resolution: %d\n" +#define DP_CONF_STR_BPP "Bits Per Pixel: %u\n"I really don't like format strings as defines, because it moves the format string away from the parameters that need to match the format.
Quoted 5 lines
A better option might be to have helper wrappers for
seq_printfthat take the required parameters.BR,
Jani.
Fair enough. I added a function to wrap the seq_printf function and one to initialize the tokens instead of having them are straight #defines. Other patches further on in the sequence will need to be adjusted to accommodate these changes.
Quoted 11 lines
Quoted 11 lines
drivers/gpu/drm/i915/i915_debugfs.c +1
-0+/* As the drm_debugfs_init() routines are called before dev->dev_private is * allocated we need to hook into the minor for release. */ static intSignature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 5/17 drm/i915: Add file ops for Displayport configuration in debugfs
In reply to Todd PreviteThis patch was also part of "[PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing". Adds file operations structures for Displayport configuration in debugfs for compliance testing. Also adds the declarations for the open() and write() functions listed in the file ops structure.
--- drivers/gpu/drm/i915/i915_debugfs.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
drivers/gpu/drm/i915/i915_debugfs.c +23 -0
@@ -3694,6 +3694,28 @@ static const struct file_operations i915_display_crc_ctl_fops = { .write = display_crc_ctl_write }; +static int displayport_config_ctl_open (struct inode *inode, +struct file *file) +{ +return 0 ; +} + +static ssize_t displayport_config_ctl_write (struct file *file, +const char __user *ubuf, +size_t len,loff_t *offp) +{ +return 0 ; +} + +static const struct file_operations i915_displayport_config_ctl_fops = { + .owner =THIS_MODULE , + .open = displayport_config_ctl_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, + .write = displayport_config_ctl_write +}; +static void wm_latency_show (struct seq_file *m,const uint16_t wm[8 ]) {struct drm_device *dev = m->private;
@@ -4446,6 +4468,7 @@ static const struct i915_debugfs_files { {"i915_spr_wm_latency", &i915_spr_wm_latency_fops}, {"i915_cur_wm_latency", &i915_cur_wm_latency_fops}, {"i915_fbc_false_color", &i915_fbc_fc_fops}, + {"i915_displayport_config_ctl", &i915_displayport_config_ctl_fops} };void intel_display_crc_init (struct drm_device *dev)
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 6/17 drm/i915: Add support functions in debugfs for handling Displayport compliance configuration
In reply to Todd PreviteThis patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing". Adds two support functions for handling Displayport configuration parameters that are used for compliance testing.
--- drivers/gpu/drm/i915/i915_debugfs.c | 48 +++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
drivers/gpu/drm/i915/i915_debugfs.c +48 -0
@@ -3694,6 +3694,54 @@ static const struct file_operations i915_display_crc_ctl_fops = { .write = display_crc_ctl_write }; +enum dp_config_param displayport_get_config_param_type (char *input_string) +{ +enum dp_config_param param_type =DP_CONFIG_PARAM_INVALID ; +int i =0 ; + +for (i =0 ; i <DP_PARAMETER_COUNT ; i++) { +if (strncmp (input_string, dp_conf_tokens[i], +strlen (dp_conf_tokens[i])) ==0 ) { + param_type = (enum dp_config_param ) i; +break ; + } + } +return param_type; +} + +int value_for_config_param (enum dp_config_param param, +char *input_string, +void *output_value) +{ +int status =0 ; +unsigned int *out_ptr = (unsigned int *)output_value; +char *index = input_string; + +switch (param) { +case DP_CONFIG_PARAM_CONNECTOR : + output_value = (char *)index; +break ; +case DP_CONFIG_PARAM_LINK_RATE : +case DP_CONFIG_PARAM_LANE_COUNT : +case DP_CONFIG_PARAM_VOLTAGE_SWING : +case DP_CONFIG_PARAM_PREEMPHASIS : + status =kstrtol (index,16 , (long *)out_ptr); +break ; +case DP_CONFIG_PARAM_HRES : +case DP_CONFIG_PARAM_VRES : +case DP_CONFIG_PARAM_BPP : + status =kstrtol (index,10 , (long *)out_ptr); +break ; +default : +/* Unhandled case */ + status = -EINVAL ; + *out_ptr =0 ; +break ; + } + +return status; +} +static int displayport_config_ctl_open (struct inode *inode,struct file *file) {
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 06/17] drm/i915: Add support functions in debugfs for handling Displayport compliance configuration
2014-12-10 21:53 GMT-02:00 Todd Previte <[email protected]>:
Quoted 5 lines
This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing". Adds two support functions for handling Displayport configuration parameters that are used for compliance testing.
First of all, a little comment about not only this patch but others
too: you had the big 05/10 patch in the previous series, and even though it was big, I could review it since the new functions included their callers and everything. Now, I see you have patch 4 where you add just a few definitions without users. Then in patch 5 you add a debugfs file that does nothing; here in patch 6 you add 2 new functions without callers. It's kinda hard to review these things without knowing how they are used. Also, if at some later point of the review we decide to change, for example, patch 08/17, maybe some of the stuff added by patch 05 or 06 will have to be changed. I usually try to split patches into "minimal reviewable bisectable self-contained" pieces, where each patch moves the overall code a single step and add all the required structures/definitions. This way, every added function, definition and struct field has a user.
For example, the 2 functions added by this patch should probably be static - judging by the fact that you don't export them in a header file -, but if you go and declare them as static, then you'll get compiler warnings complaining that they are declared static but are not used.
Quoted 11 lines
Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/i915_debugfs.c | 48 +++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)drivers/gpu/drm/i915/i915_debugfs.c +0
-0@@ -3694,6 +3694,54 @@ static const struct file_operations i915_display_crc_ctl_fops = { .write = display_crc_ctl_write
drivers/gpu/drm/i915/i915_debugfs.c static const struct file_operations i915_display_crc_ctl_fops = { · line 3704
Quoted 10 lines
drivers/gpu/drm/i915/i915_debugfs.c +8
-0}; +enum dp_config_param displayport_get_config_param_type (char *input_string) +{ +enum dp_config_param param_type =DP_CONFIG_PARAM_INVALID ; +int i =0 ; + +for (i =0 ; i <DP_PARAMETER_COUNT ; i++) { +if (strncmp (input_string, dp_conf_tokens[i], +strlen (dp_conf_tokens[i])) ==0 ) {
I guess we never really did a high-level review of your debugfs protocol, and I can't really see everything since there's no user-space side here. From this function, I can imagine you're using a string protocol. Is there any major advantage of using a string protocol compared to using a binary one? By using binary, we would be able to avoid these string operations which add a lot of complexity and danger, both on the user and kernel side: if both shared the same .h containing the right enums, they would just be able to pass the enums around. Well, that's just a suggestion in case you think it's better, not a requirement for you to rewrite everything.
drivers/gpu/drm/i915/i915_debugfs.c static const struct file_operations i915_display_crc_ctl_fops = { · line 3715
Quoted 11 lines
drivers/gpu/drm/i915/i915_debugfs.c +11
-0+ param_type = (enum dp_config_param ) i; +break ; + } + } +return param_type; +} + +int value_for_config_param (enum dp_config_param param, +char *input_string, +void *output_value) +{
This function is a little weird since it can return pointers to different types of things. I would prefer if we kept simple and avoided the " void *" here., even if that meant a little more code. But, well, let me see in the later patches how this is used first.
Besides the bikeshed, the code looks correct.
Quoted 39 lines
drivers/gpu/drm/i915/i915_debugfs.c +29
-1+int status =0 ; +unsigned int *out_ptr = (unsigned int *)output_value; +char *index = input_string; + +switch (param) { +case DP_CONFIG_PARAM_CONNECTOR : + output_value = (char *)index; +break ; +case DP_CONFIG_PARAM_LINK_RATE : +case DP_CONFIG_PARAM_LANE_COUNT : +case DP_CONFIG_PARAM_VOLTAGE_SWING : +case DP_CONFIG_PARAM_PREEMPHASIS : + status =kstrtol (index,16 , (long *)out_ptr); +break ; +case DP_CONFIG_PARAM_HRES : +case DP_CONFIG_PARAM_VRES : +case DP_CONFIG_PARAM_BPP : + status =kstrtol (index,10 , (long *)out_ptr); +break ; +default : +/* Unhandled case */ + status = -EINVAL ; + *out_ptr =0 ; +break ; + } + +return status; +} +static int displayport_config_ctl_open (struct inode *inode,struct file *file) { --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 7/17 drm/i915: Add and implement the debugfs 'show' functions for Displayport compliance
In reply to Todd PreviteThis patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing". This patch implements the 'show' functions for the debugfs interface for Displayport compliance testing.
--- drivers/gpu/drm/i915/i915_debugfs.c | 85 +++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+)
drivers/gpu/drm/i915/i915_debugfs.c +85 -0
@@ -3694,6 +3694,56 @@ static const struct file_operations i915_display_crc_ctl_fops = { .write = display_crc_ctl_write }; +static void displayport_show_config_info (struct seq_file *m, +struct intel_connector *intel_connector) +{ +struct intel_encoder *intel_encoder = intel_connector->encoder; +struct intel_dp *intel_dp; +struct intel_crtc *icrtc; +struct intel_crtc_config *crtc_config; +char *conn_name = intel_connector->base.name; +int pipe_bpp, hres, vres; +uint8_t vs[4 ], pe[4 ]; +struct drm_display_mode *mode; +int i =0 ; + +if (intel_encoder) { + intel_dp =enc_to_intel_dp (&intel_encoder->base); +for (i =0 ; i < intel_dp->lane_count; i++) { + vs[i] = intel_dp->train_set[i] + &DP_TRAIN_VOLTAGE_SWING_MASK ; + pe[i] = (intel_dp->train_set[i] + &DP_TRAIN_PRE_EMPHASIS_MASK ) >>3 ; + } +if (intel_encoder->type ==INTEL_OUTPUT_DISPLAYPORT ) { +if (intel_encoder->new_crtc) { + crtc_config = &intel_encoder->new_crtc->config; + pipe_bpp = crtc_config->pipe_bpp; + mode = &crtc_config->adjusted_mode; + hres = mode->hdisplay; + vres = mode->vdisplay; + }else if (intel_encoder->base.crtc) { + icrtc =to_intel_crtc (intel_encoder->base.crtc); + pipe_bpp = icrtc->config.pipe_bpp; + mode = &icrtc->config.adjusted_mode; + hres = mode->hdisplay; + vres = mode->vdisplay; + }else { + pipe_bpp =0 ; + hres = vres =0 ; + } +seq_printf (m,DP_CONF_STR_CONNECTOR , conn_name); +seq_printf (m,DP_CONF_STR_LINKRATE , intel_dp->link_bw); +seq_printf (m,DP_CONF_STR_LANES , intel_dp->lane_count); +seq_printf (m,DP_CONF_STR_VSWING , vs[0 ]); +seq_printf (m,DP_CONF_STR_PREEMP , pe[0 ]); +seq_printf (m,DP_CONF_STR_HRES , hres); +seq_printf (m,DP_CONF_STR_VRES , vres); +seq_printf (m,DP_CONF_STR_BPP , pipe_bpp); + } + } +} +enum dp_config_param displayport_get_config_param_type (char *input_string) {enum dp_config_param param_type =DP_CONFIG_PARAM_INVALID ;
@@ -3742,6 +3792,41 @@ int value_for_config_param(enum dp_config_param param,return status; } +static int displayport_config_ctl_show (struct seq_file *m,void *data) +{ +struct drm_device *dev = m->private; +struct drm_connector *drm_connector; +struct intel_encoder *encoder; +struct intel_connector *connector; + +if (!dev) +return -ENODEV ; + +list_for_each_entry (drm_connector, + &dev->mode_config.connector_list, + head) { + connector =to_intel_connector (drm_connector); + encoder = connector->encoder; +if (drm_connector->connector_type == + DRM_MODE_CONNECTOR_DisplayPort) { +if (encoder->type ==INTEL_OUTPUT_DISPLAYPORT || + encoder->type ==INTEL_OUTPUT_UNKNOWN ) { +if (drm_connector->status == + connector_status_connected) { +displayport_show_config_info (m, + connector); + }else { +seq_printf (m,DP_CONF_STR_CONNECTOR , + encoder->base.name); +seq_puts (m,"disconnected\n" ); + } + } + } + } + +return 0 ; +} +static int displayport_config_ctl_open (struct inode *inode,struct file *file) {
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 07/17] drm/i915: Add and implement the debugfs 'show' functions for Displayport compliance
2014-12-10 21:53 GMT-02:00 Todd Previte <[email protected]>:
Quoted 15 lines
This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing". This patch implements the 'show' functions for the debugfs interface for Displayport compliance testing.
Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/i915_debugfs.c | 85 +++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+)drivers/gpu/drm/i915/i915_debugfs.c +0
-0
drivers/gpu/drm/i915/i915_debugfs.c static const struct file_operations i915_display_crc_ctl_fops = { · line 3702
Quoted 9 lines
drivers/gpu/drm/i915/i915_debugfs.c +6
-0@@ -3694,6 +3694,56 @@ static const struct file_operations i915_display_crc_ctl_fops = { .write = display_crc_ctl_write }; +static void displayport_show_config_info (struct seq_file *m, +struct intel_connector *intel_connector) +{ +struct intel_encoder *intel_encoder = intel_connector->encoder; +struct intel_dp *intel_dp; +struct intel_crtc *icrtc;
Please don't use "icrtc": the current trend is to call intel_crtc as just "crtc". In the past, "struct drm_crtc" would be "crtc" and "struct intel_crtc" would be "intel_crtc", but I don't remember seeing icrtc.
drivers/gpu/drm/i915/i915_debugfs.c static const struct file_operations i915_display_crc_ctl_fops = { · line 3710
Quoted 8 lines
drivers/gpu/drm/i915/i915_debugfs.c +8
-0+struct intel_crtc_config *crtc_config; +char *conn_name = intel_connector->base.name; +int pipe_bpp, hres, vres; +uint8_t vs[4 ], pe[4 ]; +struct drm_display_mode *mode; +int i =0 ; + +if (intel_encoder) {
Bikeshedding: since the whole function implementation is inside the "if" statement, we usually prefer to just "if (!intel_encoder) return;", because that saves one indentation level.
drivers/gpu/drm/i915/i915_debugfs.c static const struct file_operations i915_display_crc_ctl_fops = { · line 3716
Quoted 6 lines
drivers/gpu/drm/i915/i915_debugfs.c +6
-0+ intel_dp =enc_to_intel_dp (&intel_encoder->base); +for (i =0 ; i < intel_dp->lane_count; i++) { + vs[i] = intel_dp->train_set[i] + &DP_TRAIN_VOLTAGE_SWING_MASK ; + pe[i] = (intel_dp->train_set[i] + &DP_TRAIN_PRE_EMPHASIS_MASK ) >>3 ;
Please use DP_TRAIN_PRE_EMPHASIS_SHIFT instead of the hardcoded "3".
Quoted 8 lines
drivers/gpu/drm/i915/i915_debugfs.c +8
-0+ } +if (intel_encoder->type ==INTEL_OUTPUT_DISPLAYPORT ) { +if (intel_encoder->new_crtc) { + crtc_config = &intel_encoder->new_crtc->config; + pipe_bpp = crtc_config->pipe_bpp; + mode = &crtc_config->adjusted_mode; + hres = mode->hdisplay; + vres = mode->vdisplay;
drivers/gpu/drm/i915/i915_debugfs.c static const struct file_operations i915_display_crc_ctl_fops = { · line 3734
Quoted 10 lines
drivers/gpu/drm/i915/i915_debugfs.c +10
-0+ }else if (intel_encoder->base.crtc) { + icrtc =to_intel_crtc (intel_encoder->base.crtc); + pipe_bpp = icrtc->config.pipe_bpp; + mode = &icrtc->config.adjusted_mode; + hres = mode->hdisplay; + vres = mode->vdisplay; + }else { + pipe_bpp =0 ; + hres = vres =0 ; + }
Why do you have this new_crtc vs current_crtc vs no crtc distinction here? Is it really necessary? Shouldn't the "DP testing debugfs protocol" establish when exactly the information should be queried, and then give some errors in case information is being requested at the wrong time?
Quoted 16 lines
drivers/gpu/drm/i915/i915_debugfs.c +12
-0+seq_printf (m,DP_CONF_STR_CONNECTOR , conn_name); +seq_printf (m,DP_CONF_STR_LINKRATE , intel_dp->link_bw); +seq_printf (m,DP_CONF_STR_LANES , intel_dp->lane_count); +seq_printf (m,DP_CONF_STR_VSWING , vs[0 ]); +seq_printf (m,DP_CONF_STR_PREEMP , pe[0 ]); +seq_printf (m,DP_CONF_STR_HRES , hres); +seq_printf (m,DP_CONF_STR_VRES , vres); +seq_printf (m,DP_CONF_STR_BPP , pipe_bpp); + } + } +} +enum dp_config_param displayport_get_config_param_type (char *input_string) {enum dp_config_param param_type =DP_CONFIG_PARAM_INVALID ;
drivers/gpu/drm/i915/i915_debugfs.c int value_for_config_param(enum dp_config_param param, · line 3800
Quoted 9 lines
drivers/gpu/drm/i915/i915_debugfs.c +6
-0@@ -3742,6 +3792,41 @@ int value_for_config_param(enum dp_config_param param,return status; } +static int displayport_config_ctl_show (struct seq_file *m,void *data) +{ +struct drm_device *dev = m->private; +struct drm_connector *drm_connector; +struct intel_encoder *encoder; +struct intel_connector *connector;
The standard is to make "struct drm_connector *connector" and "struct intel_connector *intel_connector", or "struct intel_connector *connector" and then always use &connector->base for when drm_connector is needed. Like the encoders and crtcs.
drivers/gpu/drm/i915/i915_debugfs.c int value_for_config_param(enum dp_config_param param, · line 3813
Quoted 13 lines
drivers/gpu/drm/i915/i915_debugfs.c +13
-0+ +if (!dev) +return -ENODEV ; + +list_for_each_entry (drm_connector, + &dev->mode_config.connector_list, + head) { + connector =to_intel_connector (drm_connector); + encoder = connector->encoder; +if (drm_connector->connector_type == + DRM_MODE_CONNECTOR_DisplayPort) { +if (encoder->type ==INTEL_OUTPUT_DISPLAYPORT || + encoder->type ==INTEL_OUTPUT_UNKNOWN ) {
If the connector is DP, I think you don't need to check the encoder type (because you check for "connected" below). Also, you could have used "&&" instead of nesting if statements, which would save one indentation level.
Quoted 26 lines
drivers/gpu/drm/i915/i915_debugfs.c +16
-1+if (drm_connector->status == + connector_status_connected) { +displayport_show_config_info (m, + connector); + }else { +seq_printf (m,DP_CONF_STR_CONNECTOR , + encoder->base.name); +seq_puts (m,"disconnected\n" ); + } + } + } + } + +return 0 ; +} +static int displayport_config_ctl_open (struct inode *inode,struct file *file) { --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 07/17] drm/i915: Add and implement the debugfs 'show' functions for Displayport compliance
Quoted 19 lines · Paulo Zanoni
2014-12-10 21:53 GMT-02:00 Todd Previte <[email protected]>:
Quoted 7 lines
drivers/gpu/drm/i915/i915_debugfs.c +7
-0+if (intel_encoder->type ==INTEL_OUTPUT_DISPLAYPORT ) { +if (intel_encoder->new_crtc) { + crtc_config = &intel_encoder->new_crtc->config; + pipe_bpp = crtc_config->pipe_bpp; + mode = &crtc_config->adjusted_mode; + hres = mode->hdisplay; + vres = mode->vdisplay;drivers/gpu/drm/i915/i915_debugfs.c static const struct file_operations i915_display_crc_ctl_fops = { · line 3734
Quoted 10 lines
drivers/gpu/drm/i915/i915_debugfs.c +10
-0+ }else if (intel_encoder->base.crtc) { + icrtc =to_intel_crtc (intel_encoder->base.crtc); + pipe_bpp = icrtc->config.pipe_bpp; + mode = &icrtc->config.adjusted_mode; + hres = mode->hdisplay; + vres = mode->vdisplay; + }else { + pipe_bpp =0 ; + hres = vres =0 ; + }
Quoted 5 lines
Why do you have this
new_crtcvscurrent_crtcvs no crtc distinction here? Is it really necessary? Shouldn't the "DP testing debugfs protocol" establish when exactly the information should be queried, and then give some errors in case information is being requested at the wrong time?
Presuming adequate locking exists (haven't checked tbh) new_* pointers always match the equivalent base pointers. new_ pointers/structures are exclusively for modeset code (and specifically state computation). I guess the new_crtc case needs to be dropped here.
-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
Re: [PATCH 07/17] drm/i915: Add and implement the debugfs 'show' functions for Displayport compliance
Quoted 24 lines · Daniel Vetter
Quoted 18 lines · Paulo Zanoni
2014-12-10 21:53 GMT-02:00 Todd Previte<[email protected]>:
Quoted 7 lines
drivers/gpu/drm/i915/i915_debugfs.c +7
-0+if (intel_encoder->type ==INTEL_OUTPUT_DISPLAYPORT ) { +if (intel_encoder->new_crtc) { + crtc_config = &intel_encoder->new_crtc->config; + pipe_bpp = crtc_config->pipe_bpp; + mode = &crtc_config->adjusted_mode; + hres = mode->hdisplay; + vres = mode->vdisplay;drivers/gpu/drm/i915/i915_debugfs.c static const struct file_operations i915_display_crc_ctl_fops = { · line 3734
Quoted 10 lines
drivers/gpu/drm/i915/i915_debugfs.c +10
-0+ }else if (intel_encoder->base.crtc) { + icrtc =to_intel_crtc (intel_encoder->base.crtc); + pipe_bpp = icrtc->config.pipe_bpp; + mode = &icrtc->config.adjusted_mode; + hres = mode->hdisplay; + vres = mode->vdisplay; + }else { + pipe_bpp =0 ; + hres = vres =0 ; + }Quoted 5 lines
Why do you have this
new_crtcvscurrent_crtcvs no crtc distinction here? Is it really necessary? Shouldn't the "DP testing debugfs protocol" establish when exactly the information should be queried, and then give some errors in case information is being requested at the wrong time?
Quoted 5 lines
Presuming adequate locking exists (haven't checked tbh) new_* pointers always match the equivalent base pointers.
new_pointers/structures are exclusively for modeset code (and specifically state computation). I guess thenew_crtccase needs to be dropped here.
-Daniel
I tested this again with the new_crtc case removed and it appears to work correctly. So the new_crtc case has been removed. The changes have been integrated into the monster patch.
-T
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 07/17] drm/i915: Add and implement the debugfs 'show' functions for Displayport compliance
In reply to Paulo ZanoniResponses inline below. Any changes have been rolled into the monster patch.
-T
Quoted 19 lines · Paulo Zanoni
2014-12-10 21:53 GMT-02:00 Todd Previte<[email protected]>:
Quoted 18 lines
This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing". This patch implements the 'show' functions for the debugfs interface for Displayport compliance testing.
Signed-off-by Todd Previte<[email protected]>--- drivers/gpu/drm/i915/i915_debugfs.c | 85 +++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+)drivers/gpu/drm/i915/i915_debugfs.c +0
-0@@ -3694,6 +3694,56 @@ static const struct file_operations i915_display_crc_ctl_fops = { .write = display_crc_ctl_write };
Quoted 10 lines
drivers/gpu/drm/i915/i915_debugfs.c static const struct file_operations i915_display_crc_ctl_fops = { · line 3702
Quoted 6 lines
drivers/gpu/drm/i915/i915_debugfs.c +6
-0+static void displayport_show_config_info (struct seq_file *m, +struct intel_connector *intel_connector) +{ +struct intel_encoder *intel_encoder = intel_connector->encoder; +struct intel_dp *intel_dp; +struct intel_crtc *icrtc;Please don't use "icrtc": the current trend is to call
intel_crtcas just "crtc". In the past, "structdrm_crtc" would be "crtc" and "structintel_crtc" would be "intel_crtc", but I don't remember seeing icrtc.
Just an attempt to get the code under the 80 char line limit. With the other changes below it's unnecessary anyways. It's fixed in V3.
Quoted 11 lines
drivers/gpu/drm/i915/i915_debugfs.c static const struct file_operations i915_display_crc_ctl_fops = { · line 3710
Quoted 8 lines
drivers/gpu/drm/i915/i915_debugfs.c +8
-0+struct intel_crtc_config *crtc_config; +char *conn_name = intel_connector->base.name; +int pipe_bpp, hres, vres; +uint8_t vs[4 ], pe[4 ]; +struct drm_display_mode *mode; +int i =0 ; + +if (intel_encoder) {Bikeshedding: since the whole function implementation is inside the "if" statement, we usually prefer to just "if (!intel_encoder) return;", because that saves one indentation level.
Changed for V3. Also changed the intel_encode->type check to the same thing, again saving a level of indent.
Quoted 8 lines
drivers/gpu/drm/i915/i915_debugfs.c static const struct file_operations i915_display_crc_ctl_fops = { · line 3716
Quoted 7 lines
drivers/gpu/drm/i915/i915_debugfs.c +6
-0+ intel_dp =enc_to_intel_dp (&intel_encoder->base); +for (i =0 ; i < intel_dp->lane_count; i++) { + vs[i] = intel_dp->train_set[i] + &DP_TRAIN_VOLTAGE_SWING_MASK ; + pe[i] = (intel_dp->train_set[i] + &DP_TRAIN_PRE_EMPHASIS_MASK ) >>3 ;Please use
DP_TRAIN_PRE_EMPHASIS_SHIFTinstead of the hardcoded "3".
Fixed.
Quoted 18 lines
Quoted 8 lines
drivers/gpu/drm/i915/i915_debugfs.c +8
-0+ } +if (intel_encoder->type ==INTEL_OUTPUT_DISPLAYPORT ) { +if (intel_encoder->new_crtc) { + crtc_config = &intel_encoder->new_crtc->config; + pipe_bpp = crtc_config->pipe_bpp; + mode = &crtc_config->adjusted_mode; + hres = mode->hdisplay; + vres = mode->vdisplay;drivers/gpu/drm/i915/i915_debugfs.c static const struct file_operations i915_display_crc_ctl_fops = { · line 3734
Quoted 10 lines
drivers/gpu/drm/i915/i915_debugfs.c +10
-0+ }else if (intel_encoder->base.crtc) { + icrtc =to_intel_crtc (intel_encoder->base.crtc); + pipe_bpp = icrtc->config.pipe_bpp; + mode = &icrtc->config.adjusted_mode; + hres = mode->hdisplay; + vres = mode->vdisplay; + }else { + pipe_bpp =0 ; + hres = vres =0 ; + }
Quoted 5 lines
Why do you have this
new_crtcvscurrent_crtcvs no crtc distinction here? Is it really necessary? Shouldn't the "DP testing debugfs protocol" establish when exactly the information should be queried, and then give some errors in case information is being requested at the wrong time?
That code is a hold-over from when I was making sure I wasn't missing test events because of invalid data structures. Part of the problem is that link training is tied to mode sets, which is where this code originally came from as I recall. In any case, I've removed the new_crtc case (in light of Daniel's reply) so it shouldn't be an issue.
Quoted 19 lines
Quoted 19 lines
drivers/gpu/drm/i915/i915_debugfs.c +12
-0+seq_printf (m,DP_CONF_STR_CONNECTOR , conn_name); +seq_printf (m,DP_CONF_STR_LINKRATE , intel_dp->link_bw); +seq_printf (m,DP_CONF_STR_LANES , intel_dp->lane_count); +seq_printf (m,DP_CONF_STR_VSWING , vs[0 ]); +seq_printf (m,DP_CONF_STR_PREEMP , pe[0 ]); +seq_printf (m,DP_CONF_STR_HRES , hres); +seq_printf (m,DP_CONF_STR_VRES , vres); +seq_printf (m,DP_CONF_STR_BPP , pipe_bpp); + } + } +} +enum dp_config_param displayport_get_config_param_type (char *input_string) {enum dp_config_param param_type =DP_CONFIG_PARAM_INVALID ;@@ -3742,6 +3792,41 @@ int value_for_config_param(enum dp_config_param param,return status; }
Quoted 10 lines
drivers/gpu/drm/i915/i915_debugfs.c int value_for_config_param(enum dp_config_param param, · line 3800
Quoted 6 lines
drivers/gpu/drm/i915/i915_debugfs.c +6
-0+static int displayport_config_ctl_show (struct seq_file *m,void *data) +{ +struct drm_device *dev = m->private; +struct drm_connector *drm_connector; +struct intel_encoder *encoder; +struct intel_connector *connector;The standard is to make "struct
drm_connector*connector" and "structintel_connector*intel_connector", or "structintel_connector*connector" and then always use&connector->basefor whendrm_connectoris needed. Like the encoders and crtcs.
Fair enough. Fixed in V3.
Quoted 9 lines
Quoted 9 lines
drivers/gpu/drm/i915/i915_debugfs.c +9
-0+ +if (!dev) +return -ENODEV ; + +list_for_each_entry (drm_connector, + &dev->mode_config.connector_list, + head) { + connector =to_intel_connector (drm_connector); + encoder = connector->encoder;
Quoted 8 lines
drivers/gpu/drm/i915/i915_debugfs.c int value_for_config_param(enum dp_config_param param, · line 3813
Quoted 4 lines
drivers/gpu/drm/i915/i915_debugfs.c +4
-0+if (drm_connector->connector_type == + DRM_MODE_CONNECTOR_DisplayPort) { +if (encoder->type ==INTEL_OUTPUT_DISPLAYPORT || + encoder->type ==INTEL_OUTPUT_UNKNOWN ) {If the connector is DP, I think you don't need to check the encoder type (because you check for "connected" below). Also, you could have used "&&" instead of nesting if statements, which would save one indentation level.
I changed the code around to save some indentation levels and address this particular issue. There was an issue I ran across where just using the DRM connector wasn't enough, but I haven't been able to reproduce that problem with the current code. So this has been removed.
Quoted 22 lines
Quoted 22 lines
drivers/gpu/drm/i915/i915_debugfs.c +16
-1+if (drm_connector->status == + connector_status_connected) { +displayport_show_config_info (m, + connector); + }else { +seq_printf (m,DP_CONF_STR_CONNECTOR , + encoder->base.name); +seq_puts (m,"disconnected\n" ); + } + } + } + } + +return 0 ; +} +static int displayport_config_ctl_open (struct inode *inode,struct file *file) { --1.9.1
Quoted 5 lines
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 8/17 drm/i915: Add Displayport link configuration structure
In reply to Todd PreviteThis patch was previously part of "[PATCH 07/10] drm/i915: Add structures for Displayport compliance testing parameters". Adds a struct to maintain link configuration data for Displayport compliance testing. The members added to the intel_dp struct are for compliance testing purposes only and should not be used during normal operations.
--- drivers/gpu/drm/i915/intel_drv.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
drivers/gpu/drm/i915/intel_drv.h +14 -0
@@ -577,6 +577,18 @@ enum edp_drrs_refresh_rate_type { DRRS_MAX_RR, /* RR count */ }; +struct intel_dp_link_config { +uint8_t lane_count; +uint8_t link_rate; +uint8_t vswing_level; +uint8_t preemp_level; +uint32_t hres; +uint32_t vres; +uint8_t bits_per_pixel; +struct drm_display_mode *compliance_mode; +char connector_name[12 ]; +}; +struct intel_dp {uint32_t output_reg;uint32_t aux_ch_ctl_reg;
@@ -641,6 +653,8 @@ struct intel_dp {/* Displayport compliance testing */ unsigned long compliance_test_data;bool compliance_testing_active; +struct intel_dp_link_config compliance_config; +struct intel_dp_link_config saved_config; };
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 9/17 drm/i915: Add config parsing utilities in debugfs for Displayport compliance
In reply to Todd PreviteThis patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing". This patch adds two functions to handle parsing of Displayport configuration information as it formatted in the debugfs file. It is used to process incoming configuration changes from the userspace compliance application during testing.
--- drivers/gpu/drm/i915/i915_debugfs.c | 128 ++++++++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+)
drivers/gpu/drm/i915/i915_debugfs.c +128 -0
@@ -3792,6 +3792,134 @@ int value_for_config_param(enum dp_config_param param,return status; } +int tokenize_dp_config (char *input_buffer,char *output[]) +{ +char *base = input_buffer, *index, *end; +int line_count =0 ; +int i =0 , len =0 ; +int done =0 ; + +if (!input_buffer) +return 0 ; + +while (!done) { + index =strpbrk (base,":" ); +if (index) { + len = index - base; + *index ='\0' ; + index++; +/* Save the type string */ + output[i] = base; + i++; + line_count++; + end =strpbrk (index,"\n\0" ); +if (end) { + *end ='\0' ; +/* Eat up whitespace */ +while (*index <=0x20 ) + index++; + output[i] = index; + i++; + line_count++; + }else + done =1 ; +/* Move to the next section of the string */ + base = end +1 ; + }else + done =1 ; + } +return line_count; +} + +static int displayport_parse_config (char *input_buffer, +ssize_t buffer_size, +struct intel_dp *intel_dp) +{ +int status =0 ; +char *lines[MAX_DP_CONFIG_LINE_COUNT ]; +int i =0 ; +struct dp_config parms[DP_PARAMETER_COUNT ]; +int line_count =0 ; +char *buffer = input_buffer; +enum dp_config_param parm_type; +unsigned long parm_value; + + line_count =tokenize_dp_config (buffer, lines); + +if (line_count ==0 ) { +DRM_DEBUG_DRIVER ("No lines to process\n" ); +return 0 ; + } + +for (i =0 ; i < line_count; i +=2 ) { + parm_type =displayport_get_config_param_type (lines[i]); +if (parm_type !=DP_CONFIG_PARAM_INVALID ) { + status =value_for_config_param (parm_type, + lines[i+1 ], + &parm_value); +if (status ==0 ) { + parms[parm_type].type = parm_type; + parms[parm_type].value = parm_value; + } + } + } + +if (parms[DP_CONFIG_PARAM_LINK_RATE ].value ==0x06 || + parms[DP_CONFIG_PARAM_LINK_RATE ].value ==0x0a || + parms[DP_CONFIG_PARAM_LINK_RATE ].value ==0x14 ) { + intel_dp->compliance_config.link_rate = + parms[DP_CONFIG_PARAM_LINK_RATE ].value; + }else +return -EINVAL ; + +if (parms[DP_CONFIG_PARAM_LANE_COUNT ].value ==0x01 || + parms[DP_CONFIG_PARAM_LANE_COUNT ].value ==0x02 || + parms[DP_CONFIG_PARAM_LANE_COUNT ].value ==0x04 ) { + intel_dp->compliance_config.lane_count = + parms[DP_CONFIG_PARAM_LANE_COUNT ].value; + }else +return -EINVAL ; + +if (parms[DP_CONFIG_PARAM_VOLTAGE_SWING ].value <=0x03 && + parms[DP_CONFIG_PARAM_VOLTAGE_SWING ].value >=0x00 ) { + intel_dp->compliance_config.vswing_level = + parms[DP_CONFIG_PARAM_VOLTAGE_SWING ].value; + }else +return -EINVAL ; + +if (parms[DP_CONFIG_PARAM_PREEMPHASIS ].value <=0x03 && + parms[DP_CONFIG_PARAM_PREEMPHASIS ].value >=0x00 ) { + intel_dp->compliance_config.preemp_level = + parms[DP_CONFIG_PARAM_PREEMPHASIS ].value; + }else +return -EINVAL ; + +if (parms[DP_CONFIG_PARAM_BPP ].value ==18 || + parms[DP_CONFIG_PARAM_BPP ].value ==24 || + parms[DP_CONFIG_PARAM_BPP ].value ==30 || + parms[DP_CONFIG_PARAM_BPP ].value ==36 ) { + intel_dp->compliance_config.bits_per_pixel = + parms[DP_CONFIG_PARAM_PREEMPHASIS ].value; + }else +return -EINVAL ; + +if (parms[DP_CONFIG_PARAM_HRES ].value >0 && + parms[DP_CONFIG_PARAM_HRES ].value <=8192 ) { + intel_dp->compliance_config.hres = + parms[DP_CONFIG_PARAM_HRES ].value; + }else +return -EINVAL ; + +if (parms[DP_CONFIG_PARAM_VRES ].value >0 && + parms[DP_CONFIG_PARAM_VRES ].value <=8192 ) { + intel_dp->compliance_config.vres = + parms[DP_CONFIG_PARAM_VRES ].value; + }else +return -EINVAL ; + +return status; +} +static int displayport_config_ctl_show (struct seq_file *m,void *data) {struct drm_device *dev = m->private;
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 10/17 drm/i915: Implement the 'open' and 'write' debugfs functions for Displayport compliance
In reply to Todd PreviteThis patch is a combination of sections out of the following two previous patches:
[PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing
[PATCH 07/10] drm/i915: Add structures for Displayport compliance testing parameters
This patch implements the debugfs functions for 'open' and 'write' as they are specified in the file ops structure. The 'open' function outputs Displayport data to the appropriate debugfs file while the 'write' function handles parsing of user data written to that same file.
--- drivers/gpu/drm/i915/i915_debugfs.c | 66 +++++++++++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/i915_debugfs.c +64 -2
@@ -3958,14 +3958,76 @@ static int displayport_config_ctl_show(struct seq_file *m, void *data)static int displayport_config_ctl_open (struct inode *inode,struct file *file) { -return 0 ; +struct drm_device *dev = inode->i_private; + +return single_open (file, displayport_config_ctl_show, dev); }static ssize_t displayport_config_ctl_write (struct file *file,const char __user *ubuf,size_t len,loff_t *offp) { -return 0 ; +char *input_buffer; +int status =0 ; +struct seq_file *m; +struct drm_device *dev; +struct drm_connector *connector; +struct drm_encoder *drm_enc; +struct intel_encoder *intel_encoder; +struct intel_connector *intel_connector; +struct intel_dp *intel_dp; + + m = file->private_data; +if (!m) { + status = -ENODEV ; +return status; + } + dev = m->private; + +if (!dev) { + status = -ENODEV ; +return status; + } + +if (len ==0 ) +return 0 ; + + input_buffer =kmalloc (len +1 ,GFP_KERNEL ); +if (!input_buffer) +return -ENOMEM ; + +if (copy_from_user (input_buffer, ubuf, len)) { + status = -EFAULT ; + goto out; + } + + input_buffer[len] ='\0' ; +DRM_DEBUG_DRIVER ("Copied %d bytes from user\n" , (unsigned int )len); + +list_for_each_entry (connector, &dev->mode_config.connector_list, head) { + intel_connector =to_intel_connector (connector); + intel_encoder = intel_connector->encoder; + drm_enc = &intel_encoder->base; +if (connector->connector_type == + DRM_MODE_CONNECTOR_DisplayPort) { +if (intel_encoder->type ==INTEL_OUTPUT_DISPLAYPORT || + intel_encoder->type ==INTEL_OUTPUT_UNKNOWN ) { + intel_dp =enc_to_intel_dp (drm_enc); + status =displayport_parse_config (input_buffer, + len, + intel_dp); +if (status <0 ) + goto out; + } + } + } +out: +kfree (input_buffer); +if (status <0 ) +return status; + + *offp += len; +return len; }static const struct file_operations i915_displayport_config_ctl_fops = {
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 11/17 drm/i915: Update the EDID automated compliance test function
In reply to Todd PreviteUpdates the EDID compliance test function to perform the EDID read as required by the tests. This read needs to take place in the kernel for reasons of speed and efficiency. The results of the EDID read are handed off to userspace so that the remainder of the test can be conducted there.
V2:
- Addressed mailing list feedback
- Removed excess debug messages
- Removed extraneous comments
- Fixed formatting issues (line length > 80)
- Updated the debug message in compute_edid_checksum to output hex values instead of decimal
--- drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_dp.c +71 -1
@@ -40,6 +40,13 @@#define DP_LINK_CHECK_TIMEOUT (10 * 1000) +/* Compliance test status bits */ +#define INTEL_DP_EDID_OK (0<<0) +#define INTEL_DP_EDID_CORRUPT (1<<0) +#define INTEL_DP_RESOLUTION_PREFERRED (1<<2) +#define INTEL_DP_RESOLUTION_STANDARD (1<<3) +#define INTEL_DP_RESOLUTION_FAILSAFE (1<<4) +struct dp_link_dpll {int link_bw;struct dpll dpll;
@@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)return test_result; } +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; + +/* Don't include last byte (the checksum) in the computation */ +for (i =0 ; i <EDID_LENGTH -2 ; i++) + byte_total += edid_data[i]; + + *edid_checksum =256 - (byte_total %256 ); + +if (*edid_checksum != edid_data[EDID_LENGTH -1 ]) { +DRM_DEBUG_KMS ("Invalid EDID checksum %02x, should be %02x\n" , + edid_data[EDID_LENGTH -40 -1 ], *edid_checksum); + edid_ok = false; + } + +return edid_ok; +} +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 ret =0 ; + + intel_dp->aux.i2c_nack_count =0 ; + intel_dp->aux.i2c_defer_count =0 ; + + edid_read =drm_get_edid (connector, adapter); + +if (edid_read ==NULL ) { +/* Check for NACKs/DEFERs, use failsafe if detected +(DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */ +if (intel_dp->aux.i2c_nack_count >0 || + intel_dp->aux.i2c_defer_count >0 ) +DRM_DEBUG_KMS ("EDID read had %d NACKs, %d DEFERs\n" , + intel_dp->aux.i2c_nack_count, + intel_dp->aux.i2c_defer_count); + intel_dp->compliance_test_data =INTEL_DP_EDID_CORRUPT | +INTEL_DP_RESOLUTION_FAILSAFE ; + }else { + edid_data = (uint8_t *) edid_read; + +if (intel_dp_compute_edid_checksum (edid_data, &checksum)) { + ret =drm_dp_dpcd_write (&intel_dp->aux, +DP_TEST_EDID_CHECKSUM , + &edid_read->checksum,1 ); + test_result =DP_TEST_ACK | +DP_TEST_EDID_CHECKSUM_WRITE ; + intel_dp->compliance_test_data = +INTEL_DP_EDID_OK | +INTEL_DP_RESOLUTION_PREFERRED ; + }else { +/* Invalid checksum - EDID corruption detection */ + intel_dp->compliance_test_data = +INTEL_DP_EDID_CORRUPT | +INTEL_DP_RESOLUTION_FAILSAFE ; + } + } +return test_result; }
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 11/17] drm/i915: Update the EDID automated compliance test function
2014-12-10 21:53 GMT-02:00 Todd Previte <[email protected]>:
Quoted 39 lines
Updates the EDID compliance test function to perform the EDID read as required by the tests. This read needs to take place in the kernel for reasons of speed and efficiency. The results of the EDID read are handed off to userspace so that the remainder of the test can be conducted there.
V2:
- Addressed mailing list feedback
- Removed excess debug messages
- Removed extraneous comments
- Fixed formatting issues (line length > 80)
- Updated the debug message incompute_edid_checksumto output hex values instead of decimalSigned-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_dp.c +7
-0@@ -40,6 +40,13 @@#define DP_LINK_CHECK_TIMEOUT (10 * 1000) +/* Compliance test status bits */ +#define INTEL_DP_EDID_OK (0<<0) +#define INTEL_DP_EDID_CORRUPT (1<<0) +#define INTEL_DP_RESOLUTION_PREFERRED (1<<2) +#define INTEL_DP_RESOLUTION_STANDARD (1<<3) +#define INTEL_DP_RESOLUTION_FAILSAFE (1<<4) +struct dp_link_dpll {int link_bw;struct dpll dpll;@@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)return test_result; }
drivers/gpu/drm/i915/intel_dp.c static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3779
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +9
-0+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; + +/* Don't include last byte (the checksum) in the computation */ +for (i =0 ; i <EDID_LENGTH -2 ; i++)
Shouldn't this be "i < EDID_LENGHT - 1"?
Quoted 26 lines
drivers/gpu/drm/i915/intel_dp.c +23
-1+ byte_total += edid_data[i]; + + *edid_checksum =256 - (byte_total %256 ); + +if (*edid_checksum != edid_data[EDID_LENGTH -1 ]) { +DRM_DEBUG_KMS ("Invalid EDID checksum %02x, should be %02x\n" , + edid_data[EDID_LENGTH -40 -1 ], *edid_checksum); + edid_ok = false; + } + +return edid_ok; +} +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 ret =0 ; + + intel_dp->aux.i2c_nack_count =0 ; + intel_dp->aux.i2c_defer_count =0 ; +
drivers/gpu/drm/i915/intel_dp.c static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3814
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10
-0+ edid_read =drm_get_edid (connector, adapter); + +if (edid_read ==NULL ) { +/* Check for NACKs/DEFERs, use failsafe if detected +(DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */ +if (intel_dp->aux.i2c_nack_count >0 || + intel_dp->aux.i2c_defer_count >0 ) +DRM_DEBUG_KMS ("EDID read had %d NACKs, %d DEFERs\n" , + intel_dp->aux.i2c_nack_count, + intel_dp->aux.i2c_defer_count);
Don't we need to use these _count values somehow, instead of just printing them in the logs?
Everything else looks fine.
Quoted 32 lines
drivers/gpu/drm/i915/intel_dp.c +22
-1+ intel_dp->compliance_test_data =INTEL_DP_EDID_CORRUPT | +INTEL_DP_RESOLUTION_FAILSAFE ; + }else { + edid_data = (uint8_t *) edid_read; + +if (intel_dp_compute_edid_checksum (edid_data, &checksum)) { + ret =drm_dp_dpcd_write (&intel_dp->aux, +DP_TEST_EDID_CHECKSUM , + &edid_read->checksum,1 ); + test_result =DP_TEST_ACK | +DP_TEST_EDID_CHECKSUM_WRITE ; + intel_dp->compliance_test_data = +INTEL_DP_EDID_OK | +INTEL_DP_RESOLUTION_PREFERRED ; + }else { +/* Invalid checksum - EDID corruption detection */ + intel_dp->compliance_test_data = +INTEL_DP_EDID_CORRUPT | +INTEL_DP_RESOLUTION_FAILSAFE ; + } + } +return test_result; } --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 11/17] drm/i915: Update the EDID automated compliance test function
Quoted 94 lines · Paulo Zanoni
2014-12-10 21:53 GMT-02:00 Todd Previte <[email protected]>:
Quoted 39 lines
Updates the EDID compliance test function to perform the EDID read as required by the tests. This read needs to take place in the kernel for reasons of speed and efficiency. The results of the EDID read are handed off to userspace so that the remainder of the test can be conducted there.
V2:
- Addressed mailing list feedback
- Removed excess debug messages
- Removed extraneous comments
- Fixed formatting issues (line length > 80)
- Updated the debug message incompute_edid_checksumto output hex values instead of decimalSigned-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_dp.c +7
-0@@ -40,6 +40,13 @@#define DP_LINK_CHECK_TIMEOUT (10 * 1000) +/* Compliance test status bits */ +#define INTEL_DP_EDID_OK (0<<0) +#define INTEL_DP_EDID_CORRUPT (1<<0) +#define INTEL_DP_RESOLUTION_PREFERRED (1<<2) +#define INTEL_DP_RESOLUTION_STANDARD (1<<3) +#define INTEL_DP_RESOLUTION_FAILSAFE (1<<4) +struct dp_link_dpll {int link_bw;struct dpll dpll;@@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)return test_result; }drivers/gpu/drm/i915/intel_dp.c static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3779
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +9
-0+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; + +/* Don't include last byte (the checksum) in the computation */ +for (i =0 ; i <EDID_LENGTH -2 ; i++)Shouldn't this be "i <
EDID_LENGHT- 1"?Quoted 26 lines
drivers/gpu/drm/i915/intel_dp.c +23
-1+ byte_total += edid_data[i]; + + *edid_checksum =256 - (byte_total %256 ); + +if (*edid_checksum != edid_data[EDID_LENGTH -1 ]) { +DRM_DEBUG_KMS ("Invalid EDID checksum %02x, should be %02x\n" , + edid_data[EDID_LENGTH -40 -1 ], *edid_checksum); + edid_ok = false; + } + +return edid_ok; +} +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 ret =0 ; + + intel_dp->aux.i2c_nack_count =0 ; + intel_dp->aux.i2c_defer_count =0 ; +drivers/gpu/drm/i915/intel_dp.c static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3814
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10
-0+ edid_read =drm_get_edid (connector, adapter); + +if (edid_read ==NULL ) { +/* Check for NACKs/DEFERs, use failsafe if detected +(DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */ +if (intel_dp->aux.i2c_nack_count >0 || + intel_dp->aux.i2c_defer_count >0 ) +DRM_DEBUG_KMS ("EDID read had %d NACKs, %d DEFERs\n" , + intel_dp->aux.i2c_nack_count, + intel_dp->aux.i2c_defer_count);Don't we need to use these
_countvalues somehow, instead of just printing them in the logs?
Quoted 21 lines
Everything else looks fine.
Quoted 19 lines
drivers/gpu/drm/i915/intel_dp.c +19
-0+ intel_dp->compliance_test_data =INTEL_DP_EDID_CORRUPT | +INTEL_DP_RESOLUTION_FAILSAFE ; + }else { + edid_data = (uint8_t *) edid_read; + +if (intel_dp_compute_edid_checksum (edid_data, &checksum)) { + ret =drm_dp_dpcd_write (&intel_dp->aux, +DP_TEST_EDID_CHECKSUM , + &edid_read->checksum,1 ); + test_result =DP_TEST_ACK | +DP_TEST_EDID_CHECKSUM_WRITE ; + intel_dp->compliance_test_data = +INTEL_DP_EDID_OK | +INTEL_DP_RESOLUTION_PREFERRED ; + }else { +/* Invalid checksum - EDID corruption detection */ + intel_dp->compliance_test_data = +INTEL_DP_EDID_CORRUPT | +INTEL_DP_RESOLUTION_FAILSAFE ;
Just something random I've spotted while driving by: drm_get_edid does all the checksum stuff for you already (it retries up to 4 times if the checkusm is off and also checks a few other things). We should never reach this case and the checksum function is essentially dead code.
Or do I miss something?
-Daniel
Quoted 22 lines
Quoted 13 lines
drivers/gpu/drm/i915/intel_dp.c +3
-1+ } + } +return test_result; } --1.9.1
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfxSignature · 5 lines
Paulo Zanoni _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 11/17] drm/i915: Update the EDID automated compliance test function
Readding intel-gfx.
Quoted 18 lines · Todd Previte
Quoted 8 lines · Daniel Vetter
Just something random I've spotted while driving by:
drm_get_ediddoes all the checksum stuff for you already (it retries up to 4 times if the checkusm is off and also checks a few other things). We should never reach this case and the checksum function is essentially dead code.Or do I miss something?
-DanielThe data I have is that when connected to the DPR-100, the logs show that the EDID checksum is incorrect from the DRM routines, which is why I went and wrote this one. This is most likely a problem with the DPR-100 (that they won't fix since they're not updating it anymore) and possibly still a problem with the DPR-120 (which I can't test because I don't have one). So this code is necessary in order to pass the EDID tests when using the DPR-100.
Quoted 4 lines
That said, it's also unlikely that the DRM routines are wholly wrong, since that would result in many, many errors in the logs for all the shipping DP monitors out there. So this is one case where special code is necessary in order to pass the tests with a particular test device.
If that's the case then we need to add some quirks to the core edid code (we have plenty of those already, shipping hw tends to be broken in all sorts of interesting ways). At least I don't see a point in passing compliance in such an important aspect as validating sink behaviour by adding special-purpose code instead of the code we use for real hw.
-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
Re: [PATCH 11/17] drm/i915: Update the EDID automated compliance test function
In reply to Paulo ZanoniQuoted 41 lines · Paulo Zanoni
2014-12-10 21:53 GMT-02:00 Todd Previte<[email protected]>:
Quoted 40 lines
Updates the EDID compliance test function to perform the EDID read as required by the tests. This read needs to take place in the kernel for reasons of speed and efficiency. The results of the EDID read are handed off to userspace so that the remainder of the test can be conducted there.
V2:
- Addressed mailing list feedback
- Removed excess debug messages
- Removed extraneous comments
- Fixed formatting issues (line length > 80)
- Updated the debug message incompute_edid_checksumto output hex values instead of decimalSigned-off-by Todd Previte<[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_dp.c +7
-0@@ -40,6 +40,13 @@#define DP_LINK_CHECK_TIMEOUT (10 * 1000) +/* Compliance test status bits */ +#define INTEL_DP_EDID_OK (0<<0) +#define INTEL_DP_EDID_CORRUPT (1<<0) +#define INTEL_DP_RESOLUTION_PREFERRED (1<<2) +#define INTEL_DP_RESOLUTION_STANDARD (1<<3) +#define INTEL_DP_RESOLUTION_FAILSAFE (1<<4) +struct dp_link_dpll {int link_bw;struct dpll dpll;@@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)return test_result; }
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3779
Quoted 9 lines
drivers/gpu/drm/i915/intel_dp.c +9
-0+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; + +/* Don't include last byte (the checksum) in the computation */ +for (i =0 ; i <EDID_LENGTH -2 ; i++)Shouldn't this be "i <
EDID_LENGHT- 1"?
Nope. :) EDID_LENGTH is defined as 128, so 128 would be off the end of the array. EDID_LENGTH - 1 would be the checksum itself, which I don't want to include when computing the checksum. So the last valid byte is actually byte 126, which is EDID_LENGTH - 2.
Quoted 39 lines
Quoted 26 lines
drivers/gpu/drm/i915/intel_dp.c +23
-1+ byte_total += edid_data[i]; + + *edid_checksum =256 - (byte_total %256 ); + +if (*edid_checksum != edid_data[EDID_LENGTH -1 ]) { +DRM_DEBUG_KMS ("Invalid EDID checksum %02x, should be %02x\n" , + edid_data[EDID_LENGTH -40 -1 ], *edid_checksum); + edid_ok = false; + } + +return edid_ok; +} +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 ret =0 ; + + intel_dp->aux.i2c_nack_count =0 ; + intel_dp->aux.i2c_defer_count =0 ; +drivers/gpu/drm/i915/intel_dp.c static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3814
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10
-0+ edid_read =drm_get_edid (connector, adapter); + +if (edid_read ==NULL ) { +/* Check for NACKs/DEFERs, use failsafe if detected +(DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */ +if (intel_dp->aux.i2c_nack_count >0 || + intel_dp->aux.i2c_defer_count >0 ) +DRM_DEBUG_KMS ("EDID read had %d NACKs, %d DEFERs\n" , + intel_dp->aux.i2c_nack_count, + intel_dp->aux.i2c_defer_count);Don't we need to use these
_countvalues somehow, instead of just printing them in the logs?
Quoted 1 line
Everything else looks fine.
In this instance, no. All that is required is for the source device to appropriately respond to receiving an I2C DEFER or NACK from the sink device. If those counters are above 0 for purposes of compliance testing, that triggers the corrupt/invalid EDID response from the test mechanism.
In reality, those counters could be tracked for a variety of reasons; one example is that they could used to slow down I2C transactions for sink devices with high response latencies, so that the DEFERs or NACKs could be reduced or avoided entirely.
Quoted 25 lines
Quoted 25 lines
drivers/gpu/drm/i915/intel_dp.c +22
-0+ intel_dp->compliance_test_data =INTEL_DP_EDID_CORRUPT | +INTEL_DP_RESOLUTION_FAILSAFE ; + }else { + edid_data = (uint8_t *) edid_read; + +if (intel_dp_compute_edid_checksum (edid_data, &checksum)) { + ret =drm_dp_dpcd_write (&intel_dp->aux, +DP_TEST_EDID_CHECKSUM , + &edid_read->checksum,1 ); + test_result =DP_TEST_ACK | +DP_TEST_EDID_CHECKSUM_WRITE ; + intel_dp->compliance_test_data = +INTEL_DP_EDID_OK | +INTEL_DP_RESOLUTION_PREFERRED ; + }else { +/* Invalid checksum - EDID corruption detection */ + intel_dp->compliance_test_data = +INTEL_DP_EDID_CORRUPT | +INTEL_DP_RESOLUTION_FAILSAFE ; + } + } +return test_result; }
Quoted 8 lines
Quoted 7 lines
--
1.9.1_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 11/17] drm/i915: Update the EDID automated compliance test function
In reply to Todd PreviteQuoted 13 lines · Todd Previte
Updates the EDID compliance test function to perform the EDID read as required by the tests. This read needs to take place in the kernel for reasons of speed and efficiency. The results of the EDID read are handed off to userspace so that the remainder of the test can be conducted there.
V2:
- Addressed mailing list feedback
- Removed excess debug messages
- Removed extraneous comments
- Fixed formatting issues (line length > 80)
- Updated the debug message incompute_edid_checksumto output hex values instead of decimal
Quoted 1 line
Signed-off-by Todd Previte <[email protected]>
Returning the abstract discussion about where to put the edid checksum checks back to the concrete patch at hand.
Quoted 54 lines
--- drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-)drivers/gpu/drm/i915/intel_dp.c +32
-1@@ -40,6 +40,13 @@#define DP_LINK_CHECK_TIMEOUT (10 * 1000) +/* Compliance test status bits */ +#define INTEL_DP_EDID_OK (0<<0) +#define INTEL_DP_EDID_CORRUPT (1<<0) +#define INTEL_DP_RESOLUTION_PREFERRED (1<<2) +#define INTEL_DP_RESOLUTION_STANDARD (1<<3) +#define INTEL_DP_RESOLUTION_FAILSAFE (1<<4) +struct dp_link_dpll {int link_bw;struct dpll dpll;@@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)return test_result; } +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; + +/* Don't include last byte (the checksum) in the computation */ +for (i =0 ; i <EDID_LENGTH -2 ; i++) + byte_total += edid_data[i]; + + *edid_checksum =256 - (byte_total %256 ); + +if (*edid_checksum != edid_data[EDID_LENGTH -1 ]) { +DRM_DEBUG_KMS ("Invalid EDID checksum %02x, should be %02x\n" , + edid_data[EDID_LENGTH -40 -1 ], *edid_checksum); + edid_ok = false; + } + +return edid_ok; +} +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 ;
drivers/gpu/drm/i915/intel_dp.c static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3807
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10
-0+uint8_t *edid_data =NULL ; +uint8_t test_result =DP_TEST_NAK , checksum =0 ; +uint32_t ret =0 ; + + intel_dp->aux.i2c_nack_count =0 ; + intel_dp->aux.i2c_defer_count =0 ; + + edid_read =drm_get_edid (connector, adapter); + +if (edid_read ==NULL ) {
So if the edid core thinks your edid is foul (e.g. checksum mismatch) you wont ever see the edid and land in this case.
drivers/gpu/drm/i915/intel_dp.c static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3816
Quoted 9 lines
drivers/gpu/drm/i915/intel_dp.c +9
-0+/* Check for NACKs/DEFERs, use failsafe if detected +(DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */ +if (intel_dp->aux.i2c_nack_count >0 || + intel_dp->aux.i2c_defer_count >0 ) +DRM_DEBUG_KMS ("EDID read had %d NACKs, %d DEFERs\n" , + intel_dp->aux.i2c_nack_count, + intel_dp->aux.i2c_defer_count); + intel_dp->compliance_test_data =INTEL_DP_EDID_CORRUPT | +INTEL_DP_RESOLUTION_FAILSAFE ;
And return the above error condition of "corrupt edid" to the tester.
drivers/gpu/drm/i915/intel_dp.c static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) · line 3829
Quoted 13 lines
drivers/gpu/drm/i915/intel_dp.c +13
-0+ }else { + edid_data = (uint8_t *) edid_read; + +if (intel_dp_compute_edid_checksum (edid_data, &checksum)) { + ret =drm_dp_dpcd_write (&intel_dp->aux, +DP_TEST_EDID_CHECKSUM , + &edid_read->checksum,1 ); + test_result =DP_TEST_ACK | +DP_TEST_EDID_CHECKSUM_WRITE ; + intel_dp->compliance_test_data = +INTEL_DP_EDID_OK | +INTEL_DP_RESOLUTION_PREFERRED ; + }else {
Which means except when there is some difference between your checksum code and the drm_edid.c checksum code this case here is dead code. And if there _is_ a difference then they'll never agree, so the "edid ok" case above can't happen. Either way one of the two sides of this if is dead code (at least if I haven't missed something).
That's essentially the reason why I think we should fixup the checksum code in drm_edid.c and drop this.
-Daniel
Quoted 17 lines
drivers/gpu/drm/i915/intel_dp.c +7
-0+/* Invalid checksum - EDID corruption detection */ + intel_dp->compliance_test_data = +INTEL_DP_EDID_CORRUPT | +INTEL_DP_RESOLUTION_FAILSAFE ; + } + } +return test_result; }Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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 12/17 drm/i915: Update intel_dp_compute_config() to handle compliance test requests
In reply to Todd PreviteAdds provisions in intel_dp_compute_config() to accommodate compliance testing. Mostly this invovles circumventing the automatic link configuration parameters and allowing the compliance code to set those parameters as required by the tests.
--- drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
drivers/gpu/drm/i915/intel_dp.c +16 -0
@@ -1189,6 +1189,21 @@ intel_dp_compute_config(struct intel_encoder *encoder, pipe_config->has_drrs = false; pipe_config->has_audio = intel_dp->has_audio; +/* Compliance testing should skip most of this function */ +if (!is_edp (intel_dp) && intel_dp->compliance_testing_active) { + bpp = intel_dp->compliance_config.bits_per_pixel; + lane_count = intel_dp->compliance_config.lane_count; + clock = intel_dp->compliance_config.link_rate >>3 ; +/* Assign here and skip at the end - ensures correct values */ + intel_dp->link_bw = bws[clock]; + intel_dp->lane_count = lane_count; + pipe_config->pipe_bpp = bpp; + pipe_config->port_clock = +drm_dp_bw_code_to_link_rate (intel_dp->link_bw); + + goto compliance_exit; + } +if (is_edp (intel_dp) && intel_connector->panel.fixed_mode) {intel_fixed_panel_mode (intel_connector->panel.fixed_mode, adjusted_mode);
@@ -1275,6 +1290,7 @@ found:DRM_DEBUG_KMS ("DP link bw required %i available %i\n" , mode_rate, link_avail); +compliance_exit: intel_link_compute_m_n(bpp, lane_count, adjusted_mode->crtc_clock, pipe_config->port_clock,
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 12/17] drm/i915: Update intel_dp_compute_config() to handle compliance test requests
2014-12-10 21:53 GMT-02:00 Todd Previte <[email protected]>:
Quoted 15 lines
Adds provisions in
intel_dp_compute_config()to accommodate compliance testing. Mostly this invovles circumventing the automatic link configuration parameters and allowing the compliance code to set those parameters as required by the tests.Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)drivers/gpu/drm/i915/intel_dp.c +0
-0
drivers/gpu/drm/i915/intel_dp.c intel_dp_compute_config(struct intel_encoder *encoder, · line 1193
Quoted 5 lines
drivers/gpu/drm/i915/intel_dp.c +2
-0@@ -1189,6 +1189,21 @@ intel_dp_compute_config(struct intel_encoder *encoder, pipe_config->has_drrs = false; pipe_config->has_audio = intel_dp->has_audio; +/* Compliance testing should skip most of this function */ +if (!is_edp (intel_dp) && intel_dp->compliance_testing_active) {
I couldn't find any patch on your series that flips intel_dp->compliance_testing_active to true, which is weird since it would prevent us from testing the code.
Also, if we can make sure that we never set compliance_testing_active to true on eDP, we can remove the is_edp() check.
Quoted 17 lines
drivers/gpu/drm/i915/intel_dp.c +13
-0+ bpp = intel_dp->compliance_config.bits_per_pixel; + lane_count = intel_dp->compliance_config.lane_count; + clock = intel_dp->compliance_config.link_rate >>3 ; +/* Assign here and skip at the end - ensures correct values */ + intel_dp->link_bw = bws[clock]; + intel_dp->lane_count = lane_count; + pipe_config->pipe_bpp = bpp; + pipe_config->port_clock = +drm_dp_bw_code_to_link_rate (intel_dp->link_bw); + + goto compliance_exit; + } +if (is_edp (intel_dp) && intel_connector->panel.fixed_mode) {intel_fixed_panel_mode (intel_connector->panel.fixed_mode, adjusted_mode);
drivers/gpu/drm/i915/intel_dp.c found: · line 1293
Quoted 4 lines
drivers/gpu/drm/i915/intel_dp.c +1
-0@@ -1275,6 +1290,7 @@ found:DRM_DEBUG_KMS ("DP link bw required %i available %i\n" , mode_rate, link_avail); +compliance_exit:
Don't we need to move the color range adjustments to this point?
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +0
-1intel_link_compute_m_n(bpp, lane_count, adjusted_mode->crtc_clock, pipe_config->port_clock, --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 12/17] drm/i915: Update intel_dp_compute_config() to handle compliance test requests
Quoted 26 lines · Paulo Zanoni
2014-12-10 21:53 GMT-02:00 Todd Previte <[email protected]>:
Quoted 15 lines
Adds provisions in
intel_dp_compute_config()to accommodate compliance testing. Mostly this invovles circumventing the automatic link configuration parameters and allowing the compliance code to set those parameters as required by the tests.Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)drivers/gpu/drm/i915/intel_dp.c +0
-0drivers/gpu/drm/i915/intel_dp.c intel_dp_compute_config(struct intel_encoder *encoder, · line 1193
Quoted 5 lines
drivers/gpu/drm/i915/intel_dp.c +2
-0@@ -1189,6 +1189,21 @@ intel_dp_compute_config(struct intel_encoder *encoder, pipe_config->has_drrs = false; pipe_config->has_audio = intel_dp->has_audio; +/* Compliance testing should skip most of this function */ +if (!is_edp (intel_dp) && intel_dp->compliance_testing_active) {I couldn't find any patch on your series that flips
intel_dp->compliance_testing_activeto true, which is weird since it would prevent us from testing the code.
Quoted 2 lines
Also, if we can make sure that we never set
compliance_testing_activeto true on eDP, we can remove theis_edp()check.
Why would we not allow automation compliance testing on eDP? There are automation tests and fixtures from Unigraf and Agilent for eDP.
-Clint
Quoted 21 lines
Quoted 21 lines
drivers/gpu/drm/i915/intel_dp.c +13
-0+ bpp = intel_dp->compliance_config.bits_per_pixel; + lane_count = intel_dp->compliance_config.lane_count; + clock = intel_dp->compliance_config.link_rate >>3 ; +/* Assign here and skip at the end - ensures correct values */ + intel_dp->link_bw = bws[clock]; + intel_dp->lane_count = lane_count; + pipe_config->pipe_bpp = bpp; + pipe_config->port_clock = +drm_dp_bw_code_to_link_rate (intel_dp->link_bw); + + goto compliance_exit; + } +if (is_edp (intel_dp) && intel_connector->panel.fixed_mode) {intel_fixed_panel_mode (intel_connector->panel.fixed_mode, adjusted_mode);@@ -1275,6 +1290,7 @@ found:DRM_DEBUG_KMS ("DP link bw required %i available %i\n" , mode_rate, link_avail);
Quoted 17 lines
drivers/gpu/drm/i915/intel_dp.c found: · line 1293
Quoted 1 line
drivers/gpu/drm/i915/intel_dp.c +1
-0+compliance_exit:Don't we need to move the color range adjustments to this point?
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +0
-1intel_link_compute_m_n(bpp, lane_count, adjusted_mode->crtc_clock, pipe_config->port_clock, --1.9.1
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 12/17] drm/i915: Update intel_dp_compute_config() to handle compliance test requests
Quoted 28 lines · Clint Taylor
Quoted 27 lines · Paulo Zanoni
2014-12-10 21:53 GMT-02:00 Todd Previte <[email protected]>:
Quoted 15 lines
Adds provisions in
intel_dp_compute_config()to accommodate compliance testing. Mostly this invovles circumventing the automatic link configuration parameters and allowing the compliance code to set those parameters as required by the tests.Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)drivers/gpu/drm/i915/intel_dp.c +0
-0drivers/gpu/drm/i915/intel_dp.c intel_dp_compute_config(struct intel_encoder · line 1194
Quoted 6 lines
drivers/gpu/drm/i915/intel_dp.c +2
-0@@ -1189,6 +1189,21 @@ intel_dp_compute_config(struct intel_encoder *encoder, pipe_config->has_drrs = false; pipe_config->has_audio = intel_dp->has_audio; +/* Compliance testing should skip most of this function */ +if (!is_edp (intel_dp) && intel_dp->compliance_testing_active) {I couldn't find any patch on your series that flips
intel_dp->compliance_testing_activeto true, which is weird since it would prevent us from testing the code.
Quoted 6 lines
Quoted 2 lines
Also, if we can make sure that we never set
compliance_testing_activeto true on eDP, we can remove theis_edp()check.Why would we not allow automation compliance testing on eDP? There are automation tests and fixtures from Unigraf and Agilent for eDP.
-Clint
eDP has different testing requirements and is a completely different specification than the one for regular Displayport. This patch set is for external Displayport connections only in accordance with the Displayport Link CTS 1.2 Core rev 1.1a document.
Quoted 33 lines
Quoted 33 lines
Quoted 20 lines
drivers/gpu/drm/i915/intel_dp.c +4
-0+ bpp = intel_dp->compliance_config.bits_per_pixel; + lane_count = intel_dp->compliance_config.lane_count; + clock = intel_dp->compliance_config.link_rate >>3 ; +/* Assign here and skip at the end - ensures correct values */
drivers/gpu/drm/i915/intel_dp.c +9
-0+ intel_dp->link_bw = bws[clock]; + intel_dp->lane_count = lane_count; + pipe_config->pipe_bpp = bpp; + pipe_config->port_clock = +drm_dp_bw_code_to_link_rate (intel_dp->link_bw); + + goto compliance_exit; + } +if (is_edp (intel_dp) && intel_connector->panel.fixed_mode) {intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
adjusted_mode);drivers/gpu/drm/i915/intel_dp.c found: · line 1293
Quoted 4 lines
drivers/gpu/drm/i915/intel_dp.c +1
-0@@ -1275,6 +1290,7 @@ found:DRM_DEBUG_KMS ("DP link bw required %i available %i\n" , mode_rate, link_avail); +compliance_exit:Don't we need to move the color range adjustments to this point?
Quoted 6 lines
drivers/gpu/drm/i915/intel_dp.c +0
-0intel_link_compute_m_n(bpp, lane_count, adjusted_mode->crtc_clock, pipe_config->port_clock,Signature · 1 line
1.9.1
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 13/17 drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation
In reply to Todd PreviteMoves the non-MST case out of the if-statement and places it at the beginning of the function to handle HPD events for SST mode. The reasoning behind this is to accommodate link status checks for compliance testing. Some test devices use long pulses to perform test requests so link status must be checked regardless of the pulse width for the SST operational mode.
This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the previous compliance testing patch sequence. Review feedback on that patch indicated that updating intel_dp_hot_plug() was not the correct place for the test handler.
--- drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +12 -10
@@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) power_domain =intel_display_port_power_domain (intel_encoder);intel_display_power_get (dev_priv, power_domain); +if (!intel_dp->is_mst) { +/* +* Pulse width doesn't matter for SST mode +* Handle the HPD event now +*/ +drm_modeset_lock (&dev->mode_config.connection_mutex,NULL ); +intel_dp_check_link_status (intel_dp); +drm_modeset_unlock (&dev->mode_config.connection_mutex); + ret = false; + goto put_power; + } +if (long_hpd) {if (HAS_PCH_SPLIT (dev)) {
@@ -4637,16 +4649,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)if (intel_dp_check_mst_status (intel_dp) == -EINVAL ) goto mst_fail; } - -if (!intel_dp->is_mst) { -/* -* we'll check the link status via the normal hot plug path later - -* but for short hpds we should check it now -*/ -drm_modeset_lock (&dev->mode_config.connection_mutex,NULL ); -intel_dp_check_link_status (intel_dp); -drm_modeset_unlock (&dev->mode_config.connection_mutex); - } } ret = false; goto put_power;
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 13/17] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation
2014-12-10 21:53 GMT-02:00 Todd Previte <[email protected]>:
Quoted 5 lines
Moves the non-MST case out of the if-statement and places it at the beginning of the function to handle HPD events for SST mode. The reasoning behind this is to accommodate link status checks for compliance testing. Some test devices use long pulses to perform test requests so link status must be checked regardless of the pulse width for the SST operational mode.
Can you please elaborate a little more on what do you see on these devices? The test spec is very clear about short vs long HPD pulses, so it's hard to believe a test device would get this wrong. We have some registers on the PCH that allow us to redefine short vs long durations. Have you tried to play with them?
More below:
Quoted 16 lines
This patch replaces [PATCH 10/10] drm/i915: Fix
intel_dp_hot_plug()in the previous compliance testing patch sequence. Review feedback on that patch indicated that updatingintel_dp_hot_plug()was not the correct place for the test handler.Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)drivers/gpu/drm/i915/intel_dp.c +0
-0
drivers/gpu/drm/i915/intel_dp.c intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) · line 4622
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +7
-0@@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) power_domain =intel_display_port_power_domain (intel_encoder);intel_display_power_get (dev_priv, power_domain); +if (!intel_dp->is_mst) { +/* +* Pulse width doesn't matter for SST mode +* Handle the HPD event now +*/ +drm_modeset_lock (&dev->mode_config.connection_mutex,NULL ); +intel_dp_check_link_status (intel_dp);
The very first thing intel_dp_check_link_status() does is to return in case "connector->base.status != connected". If we're getting a long HPD, it doesn't seem make sense to check this field because the status might be changing due to the long HPD.
Quoted 32 lines
drivers/gpu/drm/i915/intel_dp.c +5
-11+drm_modeset_unlock (&dev->mode_config.connection_mutex); + ret = false; + goto put_power; + } +if (long_hpd) {if (HAS_PCH_SPLIT (dev)) {@@ -4637,16 +4649,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)if (intel_dp_check_mst_status (intel_dp) == -EINVAL ) goto mst_fail; } - -if (!intel_dp->is_mst) { -/* -* we'll check the link status via the normal hot plug path later - -* but for short hpds we should check it now -*/ -drm_modeset_lock (&dev->mode_config.connection_mutex,NULL ); -intel_dp_check_link_status (intel_dp); -drm_modeset_unlock (&dev->mode_config.connection_mutex); - } } ret = false; goto put_power; --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 13/17] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation
Quoted 34 lines · Paulo Zanoni
2014-12-10 21:53 GMT-02:00 Todd Previte <[email protected]>:
Quoted 5 lines
Moves the non-MST case out of the if-statement and places it at the beginning of the function to handle HPD events for SST mode. The reasoning behind this is to accommodate link status checks for compliance testing. Some test devices use long pulses to perform test requests so link status must be checked regardless of the pulse width for the SST operational mode.
Can you please elaborate a little more on what do you see on these devices? The test spec is very clear about short vs long HPD pulses, so it's hard to believe a test device would get this wrong. We have some registers on the PCH that allow us to redefine short vs long durations. Have you tried to play with them?
More below:
Quoted 19 lines
This patch replaces [PATCH 10/10] drm/i915: Fix
intel_dp_hot_plug()in the previous compliance testing patch sequence. Review feedback on that patch indicated that updatingintel_dp_hot_plug()was not the correct place for the test handler.Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)drivers/gpu/drm/i915/intel_dp.c +0
-0@@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) power_domain =intel_display_port_power_domain (intel_encoder);intel_display_power_get (dev_priv, power_domain);
Quoted 12 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) · line 4622
Quoted 7 lines
drivers/gpu/drm/i915/intel_dp.c +7
-0+if (!intel_dp->is_mst) { +/* +* Pulse width doesn't matter for SST mode +* Handle the HPD event now +*/ +drm_modeset_lock (&dev->mode_config.connection_mutex,NULL ); +intel_dp_check_link_status (intel_dp);The very first thing
intel_dp_check_link_status()does is to return in case "connector->base.status!= connected". If we're getting a long HPD, it doesn't seem make sense to check this field because the status might be changing due to the long HPD.
I don't think we can unconditionally run SST hpd logic before we've correctly handled mst mode. It likely screws up the accounting.
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/i915/intel_dp.c +5
-0+drm_modeset_unlock (&dev->mode_config.connection_mutex); + ret = false; + goto put_power; + } +if (long_hpd) {
Quoted 11 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) · line 4645
Quoted 11 lines
drivers/gpu/drm/i915/intel_dp.c +0
-6if (HAS_PCH_SPLIT (dev)) {@@ -4637,16 +4649,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)if (intel_dp_check_mst_status (intel_dp) == -EINVAL ) goto mst_fail; } - -if (!intel_dp->is_mst) { -/* -* we'll check the link status via the normal hot plug path later - -* but for short hpds we should check it now -*/
Just aside: The above comment is outdated and can be remove. This is now the only place where we handle link retraining. The function could be made static and dropped from headers, too.
-Daniel
Quoted 23 lines
Quoted 14 lines
drivers/gpu/drm/i915/intel_dp.c +0
-5-drm_modeset_lock (&dev->mode_config.connection_mutex,NULL ); -intel_dp_check_link_status (intel_dp); -drm_modeset_unlock (&dev->mode_config.connection_mutex); - } } ret = false; goto put_power; --1.9.1
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfxSignature · 5 lines
Paulo Zanoni _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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
Re: [PATCH 13/17] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation
Quoted 44 lines · Daniel Vetter
Quoted 32 lines · Paulo Zanoni
2014-12-10 21:53 GMT-02:00 Todd Previte<[email protected]>:
Quoted 5 lines
Moves the non-MST case out of the if-statement and places it at the beginning of the function to handle HPD events for SST mode. The reasoning behind this is to accommodate link status checks for compliance testing. Some test devices use long pulses to perform test requests so link status must be checked regardless of the pulse width for the SST operational mode.
Can you please elaborate a little more on what do you see on these devices? The test spec is very clear about short vs long HPD pulses, so it's hard to believe a test device would get this wrong. We have some registers on the PCH that allow us to redefine short vs long durations. Have you tried to play with them?
More below:
Quoted 18 lines
This patch replaces [PATCH 10/10] drm/i915: Fix
intel_dp_hot_plug()in the previous compliance testing patch sequence. Review feedback on that patch indicated that updatingintel_dp_hot_plug()was not the correct place for the test handler.Signed-off-by Todd Previte<[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)drivers/gpu/drm/i915/intel_dp.c +0
-0@@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) power_domain =intel_display_port_power_domain (intel_encoder);intel_display_power_get (dev_priv, power_domain);Quoted 11 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) · line 4622
Quoted 7 lines
drivers/gpu/drm/i915/intel_dp.c +7
-0+if (!intel_dp->is_mst) { +/* +* Pulse width doesn't matter for SST mode +* Handle the HPD event now +*/ +drm_modeset_lock (&dev->mode_config.connection_mutex,NULL ); +intel_dp_check_link_status (intel_dp);The very first thing
intel_dp_check_link_status()does is to return in case "connector->base.status!= connected". If we're getting a long HPD, it doesn't seem make sense to check this field because the status might be changing due to the long HPD.
Quoted 2 lines
I don't think we can unconditionally run SST hpd logic before we've correctly handled mst mode. It likely screws up the accounting.
Upon further review, it looks like the best solution here is to place the SST code in the mst_fail case as the 'else' clause. That way SST mode stuff gets handled regardless and if a need arises in the future where differentiating between short and long becomes necessary, there's a place to handle it. This will be in V3.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/gpu/drm/i915/intel_dp.c +5
-0+drm_modeset_unlock (&dev->mode_config.connection_mutex); + ret = false; + goto put_power; + } +if (long_hpd) {
Quoted 15 lines
Quoted 11 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) · line 4645
Quoted 11 lines
drivers/gpu/drm/i915/intel_dp.c +0
-6if (HAS_PCH_SPLIT (dev)) {@@ -4637,16 +4649,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)if (intel_dp_check_mst_status (intel_dp) == -EINVAL ) goto mst_fail; } - -if (!intel_dp->is_mst) { -/* -* we'll check the link status via the normal hot plug path later - -* but for short hpds we should check it now -*/ Just aside: The above comment is outdated and can be remove. This is now the only place where we handle link retraining. The function could be made static and dropped from headers, too.
-Daniel
Can't be static and pulled from the header. It's called in intel_ddi.c as well.
But the comment and the code is gone, in light of the above changes.
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
drivers/gpu/drm/i915/intel_dp.c +0
-5-drm_modeset_lock (&dev->mode_config.connection_mutex,NULL ); -intel_dp_check_link_status (intel_dp); -drm_modeset_unlock (&dev->mode_config.connection_mutex); - } } ret = false; goto put_power; --1.9.1
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 6 lines
Quoted 6 lines
Signature · 5 lines
Paulo Zanoni _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 13/17] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation
In reply to Paulo ZanoniQuoted 6 lines · Paulo Zanoni
2014-12-10 21:53 GMT-02:00 Todd Previte<[email protected]>:
Quoted 5 lines
Moves the non-MST case out of the if-statement and places it at the beginning of the function to handle HPD events for SST mode. The reasoning behind this is to accommodate link status checks for compliance testing. Some test devices use long pulses to perform test requests so link status must be checked regardless of the pulse width for the SST operational mode.
Quoted 7 lines
Can you please elaborate a little more on what do you see on these devices? The test spec is very clear about short vs long HPD pulses, so it's hard to believe a test device would get this wrong. We have some registers on the PCH that allow us to redefine short vs long durations. Have you tried to play with them?
More below:
The issue is not in differentiating between the two pulse widths. The problem is that compliance testing mixes the two together, i.e. some tests are IRQ events where the source has to "service" the sink while other tests are examining the hot plug detection and response functionality. So when it comes to compliance testing, they both have to be handled and checked to see if there's a test request coming in or if it's a real HPD event of some kind. The current implementation only checks SST mode for the short pulse case.
In light of Daniel's comment, though, that makes this code even more broken. The best place to handle this (as I mention in my response to Daniel) is to place the SST case after the mst_fail tag. That way, once it is determined that MST mode is not in use, the SST handler can be invoked and events responded to appropriately.
Quoted 18 lines
Quoted 18 lines
This patch replaces [PATCH 10/10] drm/i915: Fix
intel_dp_hot_plug()in the previous compliance testing patch sequence. Review feedback on that patch indicated that updatingintel_dp_hot_plug()was not the correct place for the test handler.Signed-off-by Todd Previte<[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)drivers/gpu/drm/i915/intel_dp.c +0
-0@@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) power_domain =intel_display_port_power_domain (intel_encoder);intel_display_power_get (dev_priv, power_domain);
Quoted 11 lines
drivers/gpu/drm/i915/intel_dp.c intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) · line 4622
Quoted 7 lines
drivers/gpu/drm/i915/intel_dp.c +7
-0+if (!intel_dp->is_mst) { +/* +* Pulse width doesn't matter for SST mode +* Handle the HPD event now +*/ +drm_modeset_lock (&dev->mode_config.connection_mutex,NULL ); +intel_dp_check_link_status (intel_dp);The very first thing
intel_dp_check_link_status()does is to return in case "connector->base.status!= connected". If we're getting a long HPD, it doesn't seem make sense to check this field because the status might be changing due to the long HPD.
Long pulses are either connect OR disconnect events. In the case of the test device, what happens is that it's already connected, so HPD is asserted. It pulses the HPD line low for > 2ms and then reasserts it sometime later once the source device has disabled the main link. So it becomes a 2-fold event for us - the initial connected->disconnected transition and the following disconnected->connected transition. Both of those invoke our IRQ handler and cause neat and exciting things to happen. For compliance testing, the disconnect event needs to be ignored (by the test code, anyways) and the connect event needs to be checked for TEST_REQUEST=1. Thus, it absolutely makes sense to check this here.
Quoted 28 lines
Quoted 28 lines
drivers/gpu/drm/i915/intel_dp.c +5
-11+drm_modeset_unlock (&dev->mode_config.connection_mutex); + ret = false; + goto put_power; + } +if (long_hpd) {if (HAS_PCH_SPLIT (dev)) {@@ -4637,16 +4649,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)if (intel_dp_check_mst_status (intel_dp) == -EINVAL ) goto mst_fail; } - -if (!intel_dp->is_mst) { -/* -* we'll check the link status via the normal hot plug path later - -* but for short hpds we should check it now -*/ -drm_modeset_lock (&dev->mode_config.connection_mutex,NULL ); -intel_dp_check_link_status (intel_dp); -drm_modeset_unlock (&dev->mode_config.connection_mutex); - } } ret = false; goto put_power; --1.9.1
Quoted 5 lines
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 14/17 drm/i915: Add debugfs function to check connector status for compliance testing
In reply to Todd PreviteAdds a function to check the status of a Displayport connector. This function simplifies the Displayport compliance testing interface in debugfs by providing a single method for determining the status of a connector during Displayport compliance testing. This function checks whether or not the connector is a Displayport connector and whether or not that connector is active.
--- drivers/gpu/drm/i915/i915_debugfs.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
drivers/gpu/drm/i915/i915_debugfs.c +29 -0
@@ -3920,6 +3920,35 @@ static int displayport_parse_config(char *input_buffer,return status; } +static bool dp_connector_is_valid (struct drm_connector *connector, +bool check_active) +{ +struct intel_encoder *intel_encoder; +struct intel_connector *intel_connector; + + intel_connector =to_intel_connector (connector); + intel_encoder = intel_connector->encoder; + +if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort) + goto no; + +if (intel_encoder->type !=INTEL_OUTPUT_DISPLAYPORT && + intel_encoder->type !=INTEL_OUTPUT_UNKNOWN ) + goto no; + +if (check_active) { +if (connector->status == connector_status_connected) + goto yes; +else + goto no; + } + +yes: +return true; +no: +return false; +} +static int displayport_config_ctl_show (struct seq_file *m,void *data) {struct drm_device *dev = m->private;
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 14/17] drm/i915: Add debugfs function to check connector status for compliance testing
2014-12-10 21:53 GMT-02:00 Todd Previte <[email protected]>:
Quoted 6 lines
Adds a function to check the status of a Displayport connector. This function simplifies the Displayport compliance testing interface in debugfs by providing a single method for determining the status of a connector during Displayport compliance testing. This function checks whether or not the connector is a Displayport connector and whether or not that connector is active.
I see patches 14 and 15 rewrite some code that was added by patches 7 and 10. Instead of doing this, please make sure patch 7 already contains your dp_connector_is_valid() function, then patch 10 just calls it.
Also, please remove those "goto" and just use plain "return".
Quoted 52 lines
Signed-off-by Todd Previte <[email protected]>--- drivers/gpu/drm/i915/i915_debugfs.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)drivers/gpu/drm/i915/i915_debugfs.c +29
-1@@ -3920,6 +3920,35 @@ static int displayport_parse_config(char *input_buffer,return status; } +static bool dp_connector_is_valid (struct drm_connector *connector, +bool check_active) +{ +struct intel_encoder *intel_encoder; +struct intel_connector *intel_connector; + + intel_connector =to_intel_connector (connector); + intel_encoder = intel_connector->encoder; + +if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort) + goto no; + +if (intel_encoder->type !=INTEL_OUTPUT_DISPLAYPORT && + intel_encoder->type !=INTEL_OUTPUT_UNKNOWN ) + goto no; + +if (check_active) { +if (connector->status == connector_status_connected) + goto yes; +else + goto no; + } + +yes: +return true; +no: +return false; +} +static int displayport_config_ctl_show (struct seq_file *m,void *data) {struct drm_device *dev = m->private; --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 14/17] drm/i915: Add debugfs function to check connector status for compliance testing
Quoted 11 lines · Paulo Zanoni
2014-12-10 21:53 GMT-02:00 Todd Previte<[email protected]>:
Quoted 6 lines
Adds a function to check the status of a Displayport connector. This function simplifies the Displayport compliance testing interface in debugfs by providing a single method for determining the status of a connector during Displayport compliance testing. This function checks whether or not the connector is a Displayport connector and whether or not that connector is active.
I see patches 14 and 15 rewrite some code that was added by patches 7 and 10. Instead of doing this, please make sure patch 7 already contains your
dp_connector_is_valid()function, then patch 10 just calls it.
More code for the monster patch... ;) These have been added in for V3.
Quoted 2 lines
Also, please remove those "goto" and just use plain "return".
Done. It was an attempt to make the code cleaner and have a single point of exit, but in reality, it's not necessary and doesn't achieve its intended goal. They've been replaced with returns for V3.
Quoted 48 lines
Quoted 48 lines
Signed-off-by Todd Previte<[email protected]>--- drivers/gpu/drm/i915/i915_debugfs.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)drivers/gpu/drm/i915/i915_debugfs.c +29
-1@@ -3920,6 +3920,35 @@ static int displayport_parse_config(char *input_buffer,return status; } +static bool dp_connector_is_valid (struct drm_connector *connector, +bool check_active) +{ +struct intel_encoder *intel_encoder; +struct intel_connector *intel_connector; + + intel_connector =to_intel_connector (connector); + intel_encoder = intel_connector->encoder; + +if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort) + goto no; + +if (intel_encoder->type !=INTEL_OUTPUT_DISPLAYPORT && + intel_encoder->type !=INTEL_OUTPUT_UNKNOWN ) + goto no; + +if (check_active) { +if (connector->status == connector_status_connected) + goto yes; +else + goto no; + } + +yes: +return true; +no: +return false; +} +static int displayport_config_ctl_show (struct seq_file *m,void *data) {struct drm_device *dev = m->private; --1.9.1
Quoted 5 lines
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 14/17] drm/i915: Add debugfs function to check connector status for compliance testing
After reviewing the code again, with the changes from the previous comments, it looks like the dp_connector_is_valid() function is superfluous. So this patch and patch 15 can be disregarded. I've added an additional line to the config_ctl_write function to also check to make sure the connector is active before performing the write operations. The commit message for the monster patch where these are implemented has been updated to reflect this change as well.
-T
Quoted 16 lines · Todd Previte
Quoted 11 lines · Paulo Zanoni
2014-12-10 21:53 GMT-02:00 Todd Previte<[email protected]>:
Quoted 10 lines
Adds a function to check the status of a Displayport connector. This function
simplifies the Displayport compliance testing interface in debugfs by providing
a single method for determining the status of a connector during Displayport
compliance testing. This function checks whether or not the connector is a
Displayport connector and whether or not that connector is active.Quoted 4 lines
I see patches 14 and 15 rewrite some code that was added by patches 7 and 10. Instead of doing this, please make sure patch 7 already contains your
dp_connector_is_valid()function, then patch 10 just calls it.
Quoted 66 lines
More code for the monster patch... ;) These have been added in for V3.
Quoted 2 lines
Also, please remove those "goto" and just use plain "return".
Done. It was an attempt to make the code cleaner and have a single point of exit, but in reality, it's not necessary and doesn't achieve its intended goal. They've been replaced with returns for V3.
Quoted 58 lines
Quoted 57 lines
Signed-off-by Todd Previte<[email protected]>--- drivers/gpu/drm/i915/i915_debugfs.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)drivers/gpu/drm/i915/i915_debugfs.c +29
-0@@ -3920,6 +3920,35 @@ static int displayport_parse_config(char *input_buffer, return status; } +static bool dp_connector_is_valid (struct drm_connector *connector, +bool check_active) +{ +struct intel_encoder *intel_encoder; +struct intel_connector *intel_connector; + + intel_connector =to_intel_connector (connector); + intel_encoder = intel_connector->encoder; + +if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort) + goto no; + +if (intel_encoder->type !=INTEL_OUTPUT_DISPLAYPORT && + intel_encoder->type !=INTEL_OUTPUT_UNKNOWN ) + goto no; + +if (check_active) { +if (connector->status == connector_status_connected) + goto yes; +else + goto no; + } + +yes: +return true; +no: +return false; +} + static int displayport_config_ctl_show(struct seq_file *m, void*data)
{ struct drm_device *dev = m->private;--
1.9.1_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 15/17 drm/i915: Update debugfs functions for Displayport compliance testing
In reply to Todd PreviteUpdates displayport_config_ctl_write and displayport_config_ctl_show to use the dp_connector_is_valid() function. This saves on code and improves maintainability by unifying the code in a single, common path.
--- drivers/gpu/drm/i915/i915_debugfs.c | 42 +++++++++++++------------------------ 1 file changed, 14 insertions(+), 28 deletions(-)
drivers/gpu/drm/i915/i915_debugfs.c +14 -28
@@ -3955,6 +3955,7 @@ static int displayport_config_ctl_show(struct seq_file *m, void *data)struct drm_connector *drm_connector;struct intel_encoder *encoder;struct intel_connector *connector; +char *conn_name;if (!dev)return -ENODEV ;
@@ -3964,23 +3965,14 @@ static int displayport_config_ctl_show(struct seq_file *m, void *data) head) { connector =to_intel_connector (drm_connector); encoder = connector->encoder; -if (drm_connector->connector_type == - DRM_MODE_CONNECTOR_DisplayPort) { -if (encoder->type ==INTEL_OUTPUT_DISPLAYPORT || - encoder->type ==INTEL_OUTPUT_UNKNOWN ) { -if (drm_connector->status == - connector_status_connected) { -displayport_show_config_info (m, - connector); - }else { -seq_printf (m,DP_CONF_STR_CONNECTOR , - encoder->base.name); -seq_puts (m,"disconnected\n" ); - } - } + conn_name = connector->base.name; +if (dp_connector_is_valid (drm_connector,1 )) { +displayport_show_config_info (m, connector); + }else { +seq_printf (m,DP_CONF_STR_CONNECTOR , conn_name); +seq_puts (m,"disconnected\n" ); } } -return 0 ; }
@@ -4001,7 +3993,6 @@ static ssize_t displayport_config_ctl_write(struct file *file,struct seq_file *m;struct drm_device *dev;struct drm_connector *connector; -struct drm_encoder *drm_enc;struct intel_encoder *intel_encoder;struct intel_connector *intel_connector;struct intel_dp *intel_dp;
@@ -4036,18 +4027,13 @@ static ssize_t displayport_config_ctl_write(struct file *file,list_for_each_entry (connector, &dev->mode_config.connector_list, head) { intel_connector =to_intel_connector (connector); intel_encoder = intel_connector->encoder; - drm_enc = &intel_encoder->base; -if (connector->connector_type == - DRM_MODE_CONNECTOR_DisplayPort) { -if (intel_encoder->type ==INTEL_OUTPUT_DISPLAYPORT || - intel_encoder->type ==INTEL_OUTPUT_UNKNOWN ) { - intel_dp =enc_to_intel_dp (drm_enc); - status =displayport_parse_config (input_buffer, - len, - intel_dp); -if (status <0 ) - goto out; - } +if (dp_connector_is_valid (connector,0 )) { + intel_dp =enc_to_intel_dp (&intel_encoder->base); + status =displayport_parse_config (input_buffer, + len, + intel_dp); +if (status <0 ) + goto out; } } out:
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 16/17 drm/i915: Add new debugfs file for Displaypor compliance test control
In reply to Todd PreviteAdds a new file for controlling Displayport compliance testing via the debugfs interface. Adds two functions, 'open' and 'show', as well as the file operations structure to support reading the file from userspace. The new file is called 'i915_dp_test_ctl' and contains 4 fields:
- Connector name
- Test data
- Test active flag
- Test response
These four fields are used to control communication and operation between the userspace application and the kernel during compliance testing. A new variable is also added to the intel_dp struct to hold the response code from the user application. This response code is then sent to the sink device upon completion of the test.
--- drivers/gpu/drm/i915/i915_debugfs.c | 66 ++++++++++++++++++++++++++++++++++++- drivers/gpu/drm/i915/intel_drv.h | 1 + 2 files changed, 66 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/i915_debugfs.c +65 -1
@@ -54,6 +54,7 @@ static const char *yesno(int v)#define DP_PARAMETER_COUNT 8#define MAX_DP_CONFIG_LINE_COUNT 64 +#define DP_TEST_CTRL_LINE_COUNT 4enum dp_config_param {DP_CONFIG_PARAM_INVALID =-1 ,
@@ -67,6 +68,14 @@ enum dp_config_param {DP_CONFIG_PARAM_BPP }; +enum dp_ctrl_param { +DP_CTRL_PARAM_INVALID =-1 , +DP_CTRL_PARAM_CONNECTOR =0 , +DP_CTRL_PARAM_TEST_DATA =1 , +DP_CTRL_PARAM_TEST_ACTIVE =2 , +DP_CTRL_PARAM_TEST_RESPONSE =3 +}; +struct dp_config {enum dp_config_param type;unsigned long value;
@@ -4054,6 +4063,60 @@ static const struct file_operations i915_displayport_config_ctl_fops = { .write = displayport_config_ctl_write }; +static int displayport_test_ctl_show (struct seq_file *m,void *data) +{ +struct drm_device *dev = m->private; +struct drm_connector *connector; +struct intel_encoder *intel_encoder; +struct intel_connector *intel_connector; +struct intel_dp *intel_dp; + +if (!dev) +return -ENODEV ; + +list_for_each_entry (connector, &dev->mode_config.connector_list, head) { + intel_connector =to_intel_connector (connector); + intel_encoder = intel_connector->encoder; + +seq_printf (m,DP_CONF_STR_CONNECTOR , + intel_connector->base.name); + +if (dp_connector_is_valid (connector,1 )) { + intel_encoder = intel_connector->encoder; + intel_dp =enc_to_intel_dp (&intel_encoder->base); +/* Compliance test data informs userspace about current +request +*/ +seq_printf (m,"Test Data : %08lx\n" , + intel_dp->compliance_test_data); +seq_printf (m,"Test Active: %02x\n" , + intel_dp->compliance_testing_active); +seq_printf (m,"Test Response: %02lx\n" , + intel_dp->compliance_test_response); + }else { +seq_puts (m,"disconnected\n" ); + } + } + +return 0 ; +} + +static int displayport_test_ctl_open (struct inode *inode, +struct file *file) +{ +struct drm_device *dev = inode->i_private; + +return single_open (file, displayport_test_ctl_show, dev); +} + +static const struct file_operations i915_dp_test_ctl_fops = { + .owner =THIS_MODULE , + .open = displayport_test_ctl_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, +}; +static void wm_latency_show (struct seq_file *m,const uint16_t wm[8 ]) {struct drm_device *dev = m->private;
@@ -4806,7 +4869,8 @@ static const struct i915_debugfs_files { {"i915_spr_wm_latency", &i915_spr_wm_latency_fops}, {"i915_cur_wm_latency", &i915_cur_wm_latency_fops}, {"i915_fbc_false_color", &i915_fbc_fc_fops}, - {"i915_displayport_config_ctl", &i915_displayport_config_ctl_fops} + {"i915_displayport_config_ctl", &i915_displayport_config_ctl_fops}, + {"i915_dp_test_ctl", &i915_dp_test_ctl_fops} };void intel_display_crc_init (struct drm_device *dev)
drivers/gpu/drm/i915/intel_drv.h +1 -0
@@ -653,6 +653,7 @@ struct intel_dp {/* Displayport compliance testing */ unsigned long compliance_test_data;bool compliance_testing_active; +unsigned long compliance_test_response;struct intel_dp_link_config compliance_config;struct intel_dp_link_config saved_config;
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 17/17 drm/i915: Add debugfs write and test param parsing function for DP test control
In reply to Todd PreviteAdds and implements the 'write' function for the debugfs i915_dp_test_ctrl file. Also adds in the required parsing function to read in the data from the file once the user app has written its data to it.
--- drivers/gpu/drm/i915/i915_debugfs.c | 103 ++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+)
drivers/gpu/drm/i915/i915_debugfs.c +103 -0
@@ -4063,6 +4063,46 @@ static const struct file_operations i915_displayport_config_ctl_fops = { .write = displayport_config_ctl_write }; +static int displayport_parse_test_ctl (char *input_buffer, +ssize_t buffer_size, +struct intel_dp *intel_dp) +{ +char *ctrl_lines[DP_TEST_CTRL_LINE_COUNT ]; +int line_count; +int test_data; +int active; +int response; +char *conn_name;/* Connector name in the file */ +char *dp_name;/* Connector name in the intel_dp struct */ + +if (!input_buffer) +return -EIO ; + + line_count =tokenize_dp_config (input_buffer, ctrl_lines); +if (line_count !=DP_TEST_CTRL_LINE_COUNT ) +return -EIO ; + + conn_name = ctrl_lines[DP_CTRL_PARAM_CONNECTOR ]; + dp_name = intel_dp->attached_connector->base.name; + +if (strncmp (conn_name, dp_name,strlen (dp_name)) ==0 ) { +kstrtol (ctrl_lines[DP_CTRL_PARAM_TEST_DATA ], +16 , + (long *)&test_data); +kstrtol (ctrl_lines[DP_CTRL_PARAM_TEST_ACTIVE ], +16 , + (long *)&active); +kstrtol (ctrl_lines[DP_CTRL_PARAM_TEST_RESPONSE ], +16 , + (long *)&response); + }else { +DRM_DEBUG_DRIVER ("Connector names don't match\n" ); +return -EIO ; + } + +return 0 ; +} +static int displayport_test_ctl_show (struct seq_file *m,void *data) {struct drm_device *dev = m->private;
@@ -4109,9 +4149,72 @@ static int displayport_test_ctl_open(struct inode *inode,return single_open (file, displayport_test_ctl_show, dev); } +static ssize_t displayport_test_ctl_write (struct file *file, +const char __user *ubuf, +size_t len,loff_t *offp) +{ +char *input_buffer; +int status =0 ; +struct seq_file *m; +struct drm_device *dev; +struct drm_connector *connector; +struct intel_encoder *intel_encoder; +struct intel_connector *intel_connector; +struct intel_dp *intel_dp; + + m = file->private_data; +if (!m) { + status = -ENODEV ; +return status; + } + dev = m->private; + +if (!dev) { + status = -ENODEV ; +return status; + } + +if (len ==0 ) +return 0 ; + + input_buffer =kmalloc (len +1 ,GFP_KERNEL ); +if (!input_buffer) +return -ENOMEM ; + +if (copy_from_user (input_buffer, ubuf, len)) { + status = -EFAULT ; + goto out; + } + + input_buffer[len] ='\0' ; +DRM_DEBUG_DRIVER ("Copied %d bytes from user\n" , (unsigned int )len); + +list_for_each_entry (connector, &dev->mode_config.connector_list, head) { + intel_connector =to_intel_connector (connector); + intel_encoder = intel_connector->encoder; +if (dp_connector_is_valid (connector,0 )) { + intel_dp =enc_to_intel_dp (&intel_encoder->base); + status =displayport_parse_test_ctl (input_buffer, + len, + intel_dp); +if (status <0 ) + goto out; + } + } +out: +kfree (input_buffer); +if (status <0 ) +return status; + + *offp += len; +return len; + +} +static const struct file_operations i915_dp_test_ctl_fops = { .owner =THIS_MODULE , .open = displayport_test_ctl_open, + .write = displayport_test_ctl_write, .read = seq_read, .llseek = seq_lseek, .release = single_release,
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 17/17] drm/i915: Add debugfs write and test param parsing function for DP test control
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 364/364 364/364
ILK +5 360/366 365/366
SNB 448/450 448/450
IVB -1 497/498 496/498
BYT 289/289 289/289 HSW 563/564 563/564 BDW 417/417 417/417 -------------------------------------Detailed------------------------------------- Platform Test drm-intel-nightly Series Applied ILK igt_drv_suspend_fence-restore-untiled DMESG_WARN(1, M26)PASS(7, M37M26) PASS(1, M37) ILK igt_kms_flip_bcs-flip-vs-modeset-interruptible DMESG_WARN(1, M26)PASS(7, M37M26) PASS(1, M37) ILK igt_kms_flip_busy-flip-interruptible DMESG_WARN(2, M26)PASS(6, M37M26) PASS(1, M37) ILK igt_kms_flip_flip-vs-rmfb-interruptible DMESG_WARN(1, M26)PASS(7, M37M26) PASS(1, M37) ILK igt_kms_flip_rcs-flip-vs-dpms DMESG_WARN(1, M26)PASS(6, M37M26) PASS(1, M37) *IVB igt_kms_cursor_crc_cursor-64x64-sliding FAIL(1, M21)PASS(1, M34) DMESG_WARN(1, M21)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list [email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx