[Intel-gfx] [PATCH] drm/i915: Implement Displayport automated testing

[Todd Previte]

On 11/5/13 2:21 AM, Jani Nikula wrote:
> On Sat, 02 Nov 2013, Todd Previte <tprevite at gmail.com> wrote:
>> This initial patch adds support for automated testing of the source device
>> to the i915 driver. Most of this patch is infrastructure for the tests;
>> follow up patches will add support for the individual tests with updates
>> to ACK the tests that are supported (or NAK if the test
>> fails/is unsupported).
>>
>> Signed-off-by: Todd Previte <tprevite at gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 87 +++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 84 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index c8515bb..5f2a720 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -2813,11 +2813,92 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>>   	return true;
>>   }
>>
>> +/* Automated test function hook - description forthcoming */
>> +static bool
>> +intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>> +{
>> +	bool test_result = false;
>> +	return test_result;
>> +}
>> +
>> +/* Automated test function hook - description forthcoming */
>> +static bool
>> +intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>> +{
>> +	bool test_result = false;
>> +	return test_result;
>> +}
>> +
>> +/* Automated test function hook - description forthcoming */
>> +static bool
>> +intel_dp_autotest_edid(struct intel_dp *intel_dp)
>> +{
>> +	bool test_result = false;
>> +	return test_result;
>> +}
>> +
>> +/* Automated test function hook - description forthcoming */
>> +static bool
>> +intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
>> +{
>> +	bool test_result = false;
>> +	return test_result;
>> +}
>> +
>> +/* Automated test function hook - description forthcoming */
>> +static bool
>> +intel_dp_autotest_faux_pattern(struct intel_dp *intel_dp)
>> +{
>> +        bool test_result = false;
>> +        return test_result;
>> +}
>> +
>>   static void
>>   intel_dp_handle_test_request(struct intel_dp *intel_dp)
>> -{
>> -	/* NAK by default */
>> -	intel_dp_aux_native_write_1(intel_dp, DP_TEST_RESPONSE, DP_TEST_NAK);
>> +{	
>> +	uint8_t response = DP_TEST_NAK;
>> +	bool result = false;
>> +	uint8_t rxdata = 0;
>> +
>> +	DRM_DEBUG_KMS("Displayport: Recvd automated test request\n");
>> +	/* Read DP_TEST_REQUEST register to identify the requested test */
>> +	intel_dp_aux_native_read_retry(intel_dp, DP_TEST_REQUEST, &rxdata, 1);
> I don't think you need _retry here. It's only needed for the case when
> this might wake up the sink, but the call is cargo culted all over the
> place...

Sounds good. Paulo and I just discussed this at length and looked over 
the code. There's no reason for the extra retries here since the sink 
device is has to be awake to request the test in the first place. I'll 
get this changed for V4.

>> +	/* Determine which test has been requested */
> I think we get that without the comment. ;)
Perhaps I'm a bit overzealous with comments these days... ;)

I'll pull the extraneous comments from the patch when I'm revising it 
for V4.

>
> Is it possible multiple test request bits are set at the same time, or
> will there always be just one? The spec is a bit unclear on this.

Unfortunately, this is definitely one area where the spec is woefully 
lacking. The only indicator is this sentence in the CTS spec:

"...the Source DUT shall read the DPCD TEST_REQUEST field to see which 
test mode is being requested."

I went with a single test per request because it says "which test mode" 
instead of "which test modes". Not the best basis for making a decision, 
but there's no other indication in the text. I'm certainly open to 
arguments for the multiple tests scenario though.

>> +	switch (rxdata) {
>> +		/* ACK/NAK response based on the success or failure of the specified
>> +		   automated test function. Unimplemented tests will NAK as will those
>> +		   that are unsupported. */
>> +		case DP_TEST_LINK_TRAINING:
>> +			DRM_DEBUG_KMS("Displayport: Executing LINK_TRAINING request\n");
>> +			result = intel_dp_autotest_link_training(intel_dp);
>> +			break;
>> +		case DP_TEST_LINK_VIDEO_PATTERN:
>> +			DRM_DEBUG_KMS("Displayport: Executing TEST_PATTERN request\n");
>> +			result = intel_dp_autotest_video_pattern(intel_dp);
>> +			break;
>> +		case DP_TEST_LINK_EDID_READ:
>> +			DRM_DEBUG_KMS("Displayport: Executing EDID request\n");
>> +			result = intel_dp_autotest_edid(intel_dp);
>> +			break;
>> +		case DP_TEST_LINK_PHY_TEST_PATTERN:
>> +			DRM_DEBUG_KMS("Displayport: Executing PHY_PATTERN request\n");
>> +			result = intel_dp_autotest_phy_pattern(intel_dp);
>> +			break;
>> +                case DP_TEST_LINK_FAUX_PATTERN:
>> +                        printk(KERN_DEBUG "Displayport: Executing FAUX_PATTERN request \n");
>> +                        result = intel_dp_autotest_faux_pattern(intel_dp);
>> +                        break;
> Use tabs to indent.
Stupid editor preferences. ;) I'll fix these in V4.

>> +		/* Unsupported test case or something went wrong */
>> +		default:
>> +			/* Log error here for unhandled test request */
>> +			DRM_DEBUG_KMS("Displayport: Error - unhandled automated test type\n");
>> +			break;
>> +	}
>> +	/* Check for a valid test execution */
>> +	if (result == true)
> if (result) is customary.
Sounds good. I'll change it in V4. Consistency is good.
>
> An alternative would be to have the test calls return the response to be
> written to DP_TEST_RESPONSE, but I'm fine either way.
>
> BR,
> Jani.
I like that better than just a generic success/fail return. I'll change 
that for V4 as well.

>
>> +		response = DP_TEST_ACK;
>> +	/* Send ACK/NAK based on action taken above */
>> +	intel_dp_aux_native_write_1(intel_dp, DP_TEST_RESPONSE, response);
>>   }
>>
>>   /*
>> --
>> 1.8.1.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx