| Message ID | 1383345848-6872-2-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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); } /*
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(-)