| Message ID | 1380882730-32207-1-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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
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.
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
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.
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)
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(-)