[PATCH] drm/i915: Implement Displayport automated testing

[PATCH] drm/i915: Implement Displayport automated testing

[Todd Previte]

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@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 108 +++++++++++++++++++++++++++++++++++++++-
 include/drm/drm_dp_helper.h     |   3 +-
 2 files changed, 108 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9770160..a042d59 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -64,6 +64,26 @@ static const struct dp_link_dpll vlv_dpll[] = {
 		{ .p1 = 2, .p2 = 2, .n = 1, .m1 = 2, .m2 = 27 } }
 };
 
+/******************************************************************************
+********            Displayport automated testing                      ********
+******************************************************************************/
+/* Automated testing function - link training */
+static bool
+intel_dp_autotest_link_training(struct intel_dp *intel_dp);
+/* Automated testing function - video test pattern */
+static bool
+intel_dp_autotest_video_pattern(struct intel_dp *intel_dp);
+/* Automated testing function - EDID read */
+static bool
+intel_dp_autotest_edid(struct intel_dp *intel_dp);
+/* Automated testing function - PHY pattern */
+static bool
+intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp);
+/* Automated testing function - faux pattern */
+static bool
+intel_dp_autotest_faux_pattern(struct intel_dp *intel_dp);
+/*****************************************************************************/
+
 /**
  * is_edp - is the given port attached to an eDP panel (either CPU or PCH)
  * @intel_dp: DP struct
@@ -2732,9 +2752,93 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 
 static void
 intel_dp_handle_test_request(struct intel_dp *intel_dp)
+{	
+	uint8_t response = DP_TEST_NAK;
+	bool result = false;
+	uint8_t rxdata = 0;
+
+	printk(KERN_DEBUG "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);
+	/* Determine which test has been requested */
+	switch (rxdata) {
+		/* Supported tests handled below */
+		case DP_TEST_LINK_TRAINING:
+			printk(KERN_DEBUG "Displayport: Executing LINK_TRAINING request\n");
+			result = intel_dp_autotest_link_training(intel_dp);
+			break;
+		case DP_TEST_LINK_VIDEO_PATTERN:
+			printk(KERN_DEBUG "Displayport: Executing TEST_PATTERN request\n");
+			result = intel_dp_autotest_video_pattern(intel_dp);
+			break;
+		case DP_TEST_LINK_EDID_READ:
+			printk(KERN_DEBUG "Displayport: Executing EDID request\n");
+			result = intel_dp_autotest_edid(intel_dp);
+			break;
+		case DP_TEST_LINK_PHY_TEST_PATTERN:
+			printk(KERN_DEBUG "Displayport: Executing PHY_PATTERN request\n");
+			result = intel_dp_autotest_phy_pattern(intel_dp);
+			break;
+		case DP_TEST_LINK_FAUX_TEST_PATTERN:
+			printk(KERN_DEBUG "Displayport: Executing FAUX_PATTERN request \n");
+			result = intel_dp_autotest_faux_pattern(intel_dp);
+			break;
+		/* Unsupported test case or something went wrong */
+		default:
+			/* Log error here for unhandled test request */
+			printk(KERN_DEBUG "Displayport: Error - unhandled automated test type\n");
+			break;
+	}
+	/* Check for a valid test execution */
+	if (result == true)
+		response = DP_TEST_ACK;
+	/* Send ACK/NAK based on action taken above */
+	intel_dp_aux_native_write_1(intel_dp, DP_TEST_RESPONSE, response);
+}
+
+/* Automated testing function - link training */
+static bool
+intel_dp_autotest_link_training(struct intel_dp *intel_dp)
+{
+	/* Automated test function hook */
+	bool test_result = false;
+	return test_result;
+}
+
+/* Automated testing function - video test pattern */
+static bool
+intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
+{
+	/* Automated test function hook */
+	bool test_result = false;
+	return test_result;
+}
+
+/* Automated testing function - EDID read */
+static bool
+intel_dp_autotest_edid(struct intel_dp *intel_dp)
+{
+	/* Automated test function hook */
+	bool test_result = false;
+	return test_result;
+}
+
+/* Automated testing function - PHY pattern */
+static bool
+intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
+{
+	/* Automated test function hook */
+	bool test_result = false;
+	return test_result;
+}
+
+/* Automated testing function - faux pattern */
+static bool
+intel_dp_autotest_faux_pattern(struct intel_dp *intel_dp)
 {
-	/* NAK by default */
-	intel_dp_aux_native_write_1(intel_dp, DP_TEST_RESPONSE, DP_TEST_NAK);
+	/* Automated test function hook */
+	bool test_result = false;
+	return test_result;
 }
 
 /*
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index ae8dbfb..9fa544b 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -266,9 +266,10 @@
 
 #define DP_TEST_REQUEST			    0x218
 # define DP_TEST_LINK_TRAINING		    (1 << 0)
-# define DP_TEST_LINK_PATTERN		    (1 << 1)
+# define DP_TEST_LINK_VIDEO_PATTERN	    (1 << 1)
 # define DP_TEST_LINK_EDID_READ		    (1 << 2)
 # define DP_TEST_LINK_PHY_TEST_PATTERN	    (1 << 3) /* DPCD >= 1.1 */
+# define DP_TEST_LINK_FAUX_TEST_PATTERN	    (1 << 4) /* DPCD >= 1.2 */
 
 #define DP_TEST_LINK_RATE		    0x219
 # define DP_LINK_RATE_162		    (0x6)
-- 
1.8.1.2

Re: [PATCH] drm/i915: Implement Displayport automated testing

[Chris Wilson]

On Fri, Oct 04, 2013 at 03:32:10AM -0700, Todd Previte 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@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 108 +++++++++++++++++++++++++++++++++++++++-
>  include/drm/drm_dp_helper.h     |   3 +-
>  2 files changed, 108 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 9770160..a042d59 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -64,6 +64,26 @@ static const struct dp_link_dpll vlv_dpll[] = {
>  		{ .p1 = 2, .p2 = 2, .n = 1, .m1 = 2, .m2 = 27 } }
>  };
>  
> +/******************************************************************************
> +********            Displayport automated testing                      ********
> +******************************************************************************/
> +/* Automated testing function - link training */
> +static bool
> +intel_dp_autotest_link_training(struct intel_dp *intel_dp);

The function comment does little more than spell out the function name.
What I would prefer to see is a theory-of-operation in the block
comment. And all these forward declarations can disappear with clear
ordering in the source code.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Implement Displayport automated testing

[Jani Nikula]

On Fri, 04 Oct 2013, Todd Previte <tprevite@gmail.com> wrote:
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index ae8dbfb..9fa544b 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -266,9 +266,10 @@
>  
>  #define DP_TEST_REQUEST			    0x218
>  # define DP_TEST_LINK_TRAINING		    (1 << 0)
> -# define DP_TEST_LINK_PATTERN		    (1 << 1)
> +# define DP_TEST_LINK_VIDEO_PATTERN	    (1 << 1)
>  # define DP_TEST_LINK_EDID_READ		    (1 << 2)
>  # define DP_TEST_LINK_PHY_TEST_PATTERN	    (1 << 3) /* DPCD >= 1.1 */
> +# define DP_TEST_LINK_FAUX_TEST_PATTERN	    (1 << 4) /* DPCD >= 1.2 */
>  
>  #define DP_TEST_LINK_RATE		    0x219
>  # define DP_LINK_RATE_162		    (0x6)

This hunk needs to be a separate patch, and sent to dri-devel.

Jani.



-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Implement Displayport automated testing

[Todd Previte]

On 10/4/13 3:45 AM, Chris Wilson wrote:
> On Fri, Oct 04, 2013 at 03:32:10AM -0700, Todd Previte 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@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 108 +++++++++++++++++++++++++++++++++++++++-
>>   include/drm/drm_dp_helper.h     |   3 +-
>>   2 files changed, 108 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 9770160..a042d59 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -64,6 +64,26 @@ static const struct dp_link_dpll vlv_dpll[] = {
>>   		{ .p1 = 2, .p2 = 2, .n = 1, .m1 = 2, .m2 = 27 } }
>>   };
>>
>> +/******************************************************************************
>> +********            Displayport automated testing                      ********
>> +******************************************************************************/
>> +/* Automated testing function - link training */
>> +static bool
>> +intel_dp_autotest_link_training(struct intel_dp *intel_dp);
> The function comment does little more than spell out the function name.
> What I would prefer to see is a theory-of-operation in the block
> comment. And all these forward declarations can disappear with clear
> ordering in the source code.
> -Chris
>
I wasn't sure which way to go with this and opted for the declarations, 
but it's just as easy to reorder the definitions to be prior to their 
use in the handler. I'll get that fixed up and add more useful comments 
above the functions for V2. Thanks Chris.


-T

Re: [PATCH] drm/i915: Implement Displayport automated testing

[Todd Previte]

On 10/4/13 4:49 AM, Jani Nikula wrote:
> On Fri, 04 Oct 2013, Todd Previte<tprevite@gmail.com>  wrote:
>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> index ae8dbfb..9fa544b 100644
>> --- a/include/drm/drm_dp_helper.h
>> +++ b/include/drm/drm_dp_helper.h
>> @@ -266,9 +266,10 @@
>>   
>>   #define DP_TEST_REQUEST			    0x218
>>   # define DP_TEST_LINK_TRAINING		    (1 << 0)
>> -# define DP_TEST_LINK_PATTERN		    (1 << 1)
>> +# define DP_TEST_LINK_VIDEO_PATTERN	    (1 << 1)
>>   # define DP_TEST_LINK_EDID_READ		    (1 << 2)
>>   # define DP_TEST_LINK_PHY_TEST_PATTERN	    (1 << 3) /* DPCD >= 1.1 */
>> +# define DP_TEST_LINK_FAUX_TEST_PATTERN	    (1 << 4) /* DPCD >= 1.2 */
>>   
>>   #define DP_TEST_LINK_RATE		    0x219
>>   # define DP_LINK_RATE_162		    (0x6)
> This hunk needs to be a separate patch, and sent to dri-devel.
>
> Jani.
>
>
>
Oh right. Originally I was using local definitions until I found those 
in the header and used them instead. I'll break those out into a 
separate patch for V2 as well. Thanks Jani.

-T

[PATCH V2] drm/i915: Implement Displayport automated testing

[Todd Previte]

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@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 75 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 72 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9770160..1f97a1c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2730,11 +2730,80 @@ 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;
+}
+
 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;
+
+	printk(KERN_DEBUG "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);
+	/* Determine which test has been requested */
+	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:
+			printk(KERN_DEBUG "Displayport: Executing LINK_TRAINING request\n");
+			result = intel_dp_autotest_link_training(intel_dp);
+			break;
+		case DP_TEST_LINK_PATTERN:
+			printk(KERN_DEBUG "Displayport: Executing TEST_PATTERN request\n");
+			result = intel_dp_autotest_video_pattern(intel_dp);
+			break;
+		case DP_TEST_LINK_EDID_READ:
+			printk(KERN_DEBUG "Displayport: Executing EDID request\n");
+			result = intel_dp_autotest_edid(intel_dp);
+			break;
+		case DP_TEST_LINK_PHY_TEST_PATTERN:
+			printk(KERN_DEBUG "Displayport: Executing PHY_PATTERN request\n");
+			result = intel_dp_autotest_phy_pattern(intel_dp);
+			break;
+		/* Unsupported test case or something went wrong */
+		default:
+			/* Log error here for unhandled test request */
+			printk(KERN_DEBUG "Displayport: Error - unhandled automated test type\n");
+			break;
+	}
+	/* Check for a valid test execution */
+	if (result == true)
+		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

Re: [PATCH] drm/i915: Implement Displayport automated testing

[Ben Widawsky]

On Fri, Oct 04, 2013 at 11:11:32AM -0700, Todd Previte wrote:
> On 10/4/13 3:45 AM, Chris Wilson wrote:
> >On Fri, Oct 04, 2013 at 03:32:10AM -0700, Todd Previte 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@gmail.com>
> >>---
> >>  drivers/gpu/drm/i915/intel_dp.c | 108 +++++++++++++++++++++++++++++++++++++++-
> >>  include/drm/drm_dp_helper.h     |   3 +-
> >>  2 files changed, 108 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>index 9770160..a042d59 100644
> >>--- a/drivers/gpu/drm/i915/intel_dp.c
> >>+++ b/drivers/gpu/drm/i915/intel_dp.c
> >>@@ -64,6 +64,26 @@ static const struct dp_link_dpll vlv_dpll[] = {
> >>  		{ .p1 = 2, .p2 = 2, .n = 1, .m1 = 2, .m2 = 27 } }
> >>  };
> >>
> >>+/******************************************************************************
> >>+********            Displayport automated testing                      ********
> >>+******************************************************************************/
> >>+/* Automated testing function - link training */
> >>+static bool
> >>+intel_dp_autotest_link_training(struct intel_dp *intel_dp);
> >The function comment does little more than spell out the function name.
> >What I would prefer to see is a theory-of-operation in the block
> >comment. And all these forward declarations can disappear with clear
> >ordering in the source code.
> >-Chris
> >
> I wasn't sure which way to go with this and opted for the
> declarations, but it's just as easy to reorder the definitions to be
> prior to their use in the handler. I'll get that fixed up and add
> more useful comments above the functions for V2. Thanks Chris.
> 
> 
> -T

Since it sounds like you have a v2 in the works anyway, can you please
change printk to DRM_DEBUG_KMS.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Implement Displayport automated testing

[Todd Previte]

On 10/4/13 1:39 PM, Ben Widawsky wrote:
> On Fri, Oct 04, 2013 at 11:11:32AM -0700, Todd Previte wrote:
>> On 10/4/13 3:45 AM, Chris Wilson wrote:
>>> On Fri, Oct 04, 2013 at 03:32:10AM -0700, Todd Previte 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@gmail.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_dp.c | 108 +++++++++++++++++++++++++++++++++++++++-
>>>>   include/drm/drm_dp_helper.h     |   3 +-
>>>>   2 files changed, 108 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 9770160..a042d59 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -64,6 +64,26 @@ static const struct dp_link_dpll vlv_dpll[] = {
>>>>   		{ .p1 = 2, .p2 = 2, .n = 1, .m1 = 2, .m2 = 27 } }
>>>>   };
>>>>
>>>> +/******************************************************************************
>>>> +********            Displayport automated testing                      ********
>>>> +******************************************************************************/
>>>> +/* Automated testing function - link training */
>>>> +static bool
>>>> +intel_dp_autotest_link_training(struct intel_dp *intel_dp);
>>> The function comment does little more than spell out the function name.
>>> What I would prefer to see is a theory-of-operation in the block
>>> comment. And all these forward declarations can disappear with clear
>>> ordering in the source code.
>>> -Chris
>>>
>> I wasn't sure which way to go with this and opted for the
>> declarations, but it's just as easy to reorder the definitions to be
>> prior to their use in the handler. I'll get that fixed up and add
>> more useful comments above the functions for V2. Thanks Chris.
>>
>>
>> -T
> Since it sounds like you have a v2 in the works anyway, can you please
> change printk to DRM_DEBUG_KMS.
>
I have another patch that's following onto this one once the drm header 
changes get through, so I can either put this in that patch or change 
them in a separate patch if that's preferred.

-T

[PATCH V3] drm/i915: Implement Displayport automated testing

[Todd Previte]

Revised patch incorporating feedback from the various reviews.
   - Changed printk() to DRM_DEBUG_KMS()
   - Removed extraneous comments
   - Added test hook for Fast AUX automated testing        

Note this patch relies on the constants defined in drm/drm_dp_helper.h that were updated
in a separate patch.

[PATCH] drm/i915: Implement Displayport automated testing

[Todd Previte]

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@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);
+	/* Determine which test has been requested */
+	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;
+		/* 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)
+		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

Re: [PATCH] drm/i915: Implement Displayport automated testing

[Jani Nikula]

On Sat, 02 Nov 2013, Todd Previte <tprevite@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@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...

> +	/* Determine which test has been requested */

I think we get that without the comment. ;)

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.

> +	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.

> +		/* 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.

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.


> +		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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [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@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@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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Implement Displayport automated testing

[Daniel Vetter]

On Tue, Nov 5, 2013 at 10:01 PM, Todd Previte <tprevite@gmail.com> wrote:
> 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.

I'd say until we fail compliance somewhere we should opt to not care.
And once this becomes an issue maybe we should file a spec bug - if
that's possible for DP.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch