On 12/12/14 1:25 PM, Paulo Zanoni wrote:
2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@xxxxxxxxx>: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 Signed-off-by: Todd Previte<tprevite@xxxxxxxxx> --- drivers/gpu/drm/i915/intel_dp.c | 72 +++++++++++++++++++++++++++++++++++++--- drivers/gpu/drm/i915/intel_drv.h | 4 +++ 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 3fc3296..3dc92a3 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -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) {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...+ 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).
Good catches on these two - thanks Paulo. They've been fixed in V3.
+ 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)Same here...
Same as above. Fixed in V3.
+ DRM_DEBUG_KMS("Could not write test response to sink\n"); + + intel_dp->compliance_testing_active = 0; } static int diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 588b618..d1a807a 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -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.
Ok I'll keep this in mind moving forward. Thanks Paulo!
+ }; struct intel_digital_port { -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
