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