On Thu, Jan 11, 2018 at 05:27:52AM +0000, Ramalingam C wrote: > > > On Wednesday 10 January 2018 11:45 PM, Rodrigo Vivi wrote: > > On Wed, Jan 10, 2018 at 02:47:00PM +0000, Lohith BS wrote: > > Dynamic Refresh Rate Switch(DRRS) is used to switch the panel's > refresh rate to the lowest vrefresh supported by panel, when frame is > not flipped for more than a Sec. > > In kernel, DRRS uses the front buffer tracking infrastructure. > Hence DRRS test coverage is added along with other frontbuffer tracking > based features such as FBC and PSR tests. > > Here, we are testing DRRS with other features in all possible > combinations, in all required test cases, to capture any possible > regression. > > v2: Addressed the comments and suggestions from Vlad, Marius. > The signoff details from the earlier work are also included. > > v3: Modified vblank rate calculation by using reply-sequence, > provided by drmWaitVBlank, as suggested by Chris Wilson. > > v4: As suggested from Chris Wilson and Daniel Vetter > 1) Avoided using pthread for calculating vblank refresh rate, > instead used drmWaitVBlank reply sequence. > 2) Avoided using kernel-specific info like transitional delays, > instead polling mechanism with timeout is used. > 3) Included edp-DRRS as a subtest in kms_frontbuffer_tracking.c, > instead of having a separate test. > > v5: This patch adds DRRS as a new feature in the > kms_frontbuffer_tracking IGT. > DRRS switch to lower vrefresh rate is tested at slow-draw subtest. > > Note: > 1) Currently kernel doesn't have support to enable and disable > the DRRS feature dynamically(as in case of PSR). Hence if the > panel supports DRRS it will be enabled by default. > > This is in continuation of last patch > "https://patchwork.freedesktop.org/patch/162726/"; > > v6: This patch adds runtime enable and disable feature for testing DRRS > > v7: This patch adds runtime enable and disable feature for testing DRRS > through debugfs entry "i915_drrs_ctl". > > v8: Commit message is updated to reflect current implementation. > > v9: Addressed Paulo Zanoni comments. > Check for DRRS_LOW at tests with OFFSCREEN + FBS_INDIVIDUAL. > > v10: Corrected DRRS state expectation in suspend related subtests. > > v11: Removing the global flag is_psr_drrs_combo [Rodrigo]. > > v12: Rewriting the DRRS inactive deduction [Rodrigo]. > > v13: En/Dis-able DRRS only when DRRS is capable on the setup. > > Signed-off-by: Lohith BS <lohith.bs@xxxxxxxxx> > Signed-off-by: aknautiy <ankit.k.nautiyal@xxxxxxxxx> > --- > tests/kms_frontbuffer_tracking.c | 172 +++++++++++++++++++++++++++++++++++++-- > 1 file changed, 164 insertions(+), 8 deletions(-) > > diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c > index 1601cab..6b2299b 100644 > --- a/tests/kms_frontbuffer_tracking.c > +++ b/tests/kms_frontbuffer_tracking.c > @@ -34,7 +34,7 @@ > > > IGT_TEST_DESCRIPTION("Test the Kernel's frontbuffer tracking mechanism and " > - "its related features: FBC and PSR"); > + "its related features: FBC, PSR and DRRS"); > > /* > * One of the aspects of this test is that, for every subtest, we try different > @@ -105,8 +105,9 @@ struct test_mode { > FEATURE_NONE = 0, > FEATURE_FBC = 1, > FEATURE_PSR = 2, > - FEATURE_COUNT = 4, > - FEATURE_DEFAULT = 4, > + FEATURE_DRRS = 4, > + FEATURE_COUNT = 8, > + FEATURE_DEFAULT = 8, > } feature; > > /* Possible pixel formats. We just use FORMAT_DEFAULT for most tests and > @@ -182,6 +183,13 @@ struct { > .can_test = false, > }; > > +#define MAX_DRRS_STATUS_BUF_LEN 256 > + > +struct { > + bool can_test; > +} drrs = { > + .can_test = false, > +}; > > #define SINK_CRC_SIZE 12 > typedef struct { > @@ -790,7 +798,14 @@ static void __debugfs_read(const char *param, char *buf, int len) > buf[len] = '\0'; > } > > +static void __debugfs_write(const char *param, char *buf, int len) > +{ > + igt_assert_eq(igt_sysfs_write(drm.debugfs, param, buf, len - 1), > + len - 1); > +} > + > #define debugfs_read(p, arr) __debugfs_read(p, arr, sizeof(arr)) > +#define debugfs_write(p, arr) __debugfs_write(p, arr, sizeof(arr)) > > static bool fbc_is_enabled(void) > { > @@ -825,6 +840,62 @@ static void psr_print_status(void) > igt_info("PSR status:\n%s\n", buf); > } > > +void drrs_set(unsigned int val) > +{ > + char buf[2]; > + > + if (!drrs.can_test) > + return; > > Can you please elaborate more about this solution here? > > I couldn't find the logs anymore. What was the issue that CI caught? > > > CI failure were caused due to two reasons: > > • Error from kernel on debugfs write when the platform doesn't support DRRS (<gen7). > > This can be addressed either from kernel by returning success from > i915_drrs_ctl_set instead of -ENODEV for <gen7, -ENODEV is better, but then igt test skip when handling this -ENODEV explicitly instead of a random return on !can_test. > or by not En/Disable the drrs feature when you already knew that feature is > not supported. > > • Non Availability of debugfs_fd for debugfs write for DRRS when eDP panel was not > there. > > This is handled by adding debugfs_write. Which will open and close the file > as and when it is required using igt_sysfs_write. > Similar to already existing debugfs_read. > > > What happens if a regular user on HSW with no DRRS monitor (same configuration as CI) > write to this debugfs enabling DRRS? > > > Basically this debugfs write in kernel > on >=gen7 > for drrs enable req: will enable drrs if there is any DRRS capable eDP panel. > for drrs disable req: will disable drrs if there is any DRRS capable eDP > panel in enabled state. > on <gen7 > returns -ENODEV. Not sure whether we should just skip all operations and > return success on <gen7 also. > > This patch skips any write into the drrs ctrl debugfs. > > > What I wonder is if this is the right place for this solution or on the > kernel side. > > Also if it is a testcase I wonder if it shouldn't have a skip handling > somewhere instead of a blind return.... > > In this IGT before every subtest, all the features are disabled and only the features > required for that subtest is enabled. > So we though its good to validate the en/Disable call at the implementation of drrs_set > than at calling place. Please suggest if you think otherway. my suggestion is to keep -ENODEV and igt check explicitly for this return value and skip testcase if -ENODEV is returned. > > -Ram > > > Thanks, > Rodrigo. > > > + > + igt_debug("Manually %sabling DRRS. %llu\n", val ? "en" : "dis", val); > + snprintf(buf, sizeof(buf), "%d", val); > + debugfs_write("i915_drrs_ctl", buf); > +} > + > +static bool is_drrs_high(void) > +{ > + char buf[MAX_DRRS_STATUS_BUF_LEN]; > + > + debugfs_read("i915_drrs_status", buf); > + return strstr(buf, "DRRS_HIGH_RR"); > +} > + > +static bool is_drrs_low(void) > +{ > + char buf[MAX_DRRS_STATUS_BUF_LEN]; > + > + debugfs_read("i915_drrs_status", buf); > + return strstr(buf, "DRRS_LOW_RR"); > +} > + > +static bool is_drrs_supported(void) > +{ > + char buf[MAX_DRRS_STATUS_BUF_LEN]; > + > + debugfs_read("i915_drrs_status", buf); > + return strstr(buf, "DRRS Supported: Yes"); > +} > + > +static bool is_drrs_inactive(void) > +{ > + char buf[MAX_DRRS_STATUS_BUF_LEN]; > + > + debugfs_read("i915_drrs_status", buf); > + > + if (strstr(buf, "DRRS_State: ")) > + return false; > + > + return true; > +} > + > +static void drrs_print_status(void) > +{ > + char buf[MAX_DRRS_STATUS_BUF_LEN]; > + > + debugfs_read("i915_drrs_status", buf); > + igt_info("DRRS STATUS :\n%s\n", buf); > +} > + > static struct timespec fbc_get_last_action(void) > { > struct timespec ret = { 0, 0 }; > @@ -935,10 +1006,17 @@ static bool psr_wait_until_enabled(void) > return igt_wait(psr_is_enabled(), 5000, 1); > } > > +static bool drrs_wait_until_rr_switch_to_low(void) > +{ > + return igt_wait(is_drrs_low(), 5000, 1); > +} > + > #define fbc_enable() igt_set_module_param_int("enable_fbc", 1) > #define fbc_disable() igt_set_module_param_int("enable_fbc", 0) > #define psr_enable() igt_set_module_param_int("enable_psr", 1) > #define psr_disable() igt_set_module_param_int("enable_psr", 0) > +#define drrs_enable() drrs_set(1) > +#define drrs_disable() drrs_set(0) > > static void get_sink_crc(sink_crc_t *crc, bool mandatory) > { > @@ -1184,6 +1262,7 @@ static void disable_features(const struct test_mode *t) > > fbc_disable(); > psr_disable(); > + drrs_disable(); > } > > static void *busy_thread_func(void *data) > @@ -1577,6 +1656,22 @@ static void teardown_psr(void) > { > } > > +static void setup_drrs(void) > +{ > + if (get_connector(prim_mode_params.connector_id)->connector_type != > + DRM_MODE_CONNECTOR_eDP) { > + igt_info("Can't test DRRS: no usable eDP screen.\n"); > + return; > + } > + > + if (!is_drrs_supported()) { > + igt_info("Can't test DRRS: Not supported.\n"); > + return; > + } > + > + drrs.can_test = true; > +} > + > static void setup_environment(void) > { > setup_drm(); > @@ -1584,6 +1679,7 @@ static void setup_environment(void) > > setup_fbc(); > setup_psr(); > + setup_drrs(); > > setup_crcs(); > } > @@ -1662,6 +1758,11 @@ static void do_flush(const struct test_mode *t) > #define ASSERT_PSR_ENABLED (1 << 6) > #define ASSERT_PSR_DISABLED (1 << 7) > > +#define DRRS_ASSERT_FLAGS (7 << 8) > +#define ASSERT_DRRS_HIGH (1 << 8) > +#define ASSERT_DRRS_LOW (1 << 9) > +#define ASSERT_DRRS_INACTIVE (1 << 10) > + > static int adjust_assertion_flags(const struct test_mode *t, int flags) > { > if (!(flags & DONT_ASSERT_FEATURE_STATUS)) { > @@ -1669,12 +1770,17 @@ static int adjust_assertion_flags(const struct test_mode *t, int flags) > flags |= ASSERT_FBC_ENABLED; > if (!(flags & ASSERT_PSR_DISABLED)) > flags |= ASSERT_PSR_ENABLED; > + if (!((flags & ASSERT_DRRS_LOW) || > + (flags & ASSERT_DRRS_INACTIVE))) > + flags |= ASSERT_DRRS_HIGH; > } > > if ((t->feature & FEATURE_FBC) == 0) > flags &= ~FBC_ASSERT_FLAGS; > if ((t->feature & FEATURE_PSR) == 0) > flags &= ~PSR_ASSERT_FLAGS; > + if ((t->feature & FEATURE_DRRS) == 0) > + flags &= ~DRRS_ASSERT_FLAGS; > > return flags; > } > @@ -1706,6 +1812,23 @@ static void do_status_assertions(int flags) > return; > } > > + if (flags & ASSERT_DRRS_HIGH) { > + if (!is_drrs_high()) { > + drrs_print_status(); > + igt_assert_f(false, "DRRS HIGH\n"); > + } > + } else if (flags & ASSERT_DRRS_LOW) { > + if (!drrs_wait_until_rr_switch_to_low()) { > + drrs_print_status(); > + igt_assert_f(false, "DRRS LOW\n"); > + } > + } else if (flags & ASSERT_DRRS_INACTIVE) { > + if (!is_drrs_inactive()) { > + drrs_print_status(); > + igt_assert_f(false, "DRRS INACTIVE\n"); > + } > + } > + > if (flags & ASSERT_FBC_ENABLED) { > igt_require(!fbc_not_enough_stolen()); > igt_require(!fbc_stride_not_supported()); > @@ -1833,6 +1956,8 @@ static void enable_features_for_test(const struct test_mode *t) > fbc_enable(); > if (t->feature & FEATURE_PSR) > psr_enable(); > + if (t->feature & FEATURE_DRRS) > + drrs_enable(); > } > > static void check_test_requirements(const struct test_mode *t) > @@ -1852,6 +1977,18 @@ static void check_test_requirements(const struct test_mode *t) > "Can't test PSR without sink CRCs\n"); > } > > + if (t->feature & FEATURE_DRRS) > + igt_require_f(drrs.can_test, > + "Can't test DRRS with the current outputs\n"); > + > + /* > + * In kernel, When PSR is enabled, DRRS will be disabled. So If a test > + * case needs DRRS + PSR enabled, that will be skipped. > + */ > + igt_require_f(!((t->feature & FEATURE_PSR) && > + (t->feature & FEATURE_DRRS)), > + "Can't test PSR and DRRS together\n"); > + > if (opt.only_pipes != PIPE_COUNT) > igt_require(t->pipes == opt.only_pipes); > } > @@ -1973,7 +2110,7 @@ static void rte_subtest(const struct test_mode *t) > > unset_all_crtcs(); > do_assertions(ASSERT_FBC_DISABLED | ASSERT_PSR_DISABLED | > - DONT_ASSERT_CRC); > + DONT_ASSERT_CRC | ASSERT_DRRS_INACTIVE); > > enable_prim_screen_and_wait(t); > set_cursor_for_test(t, &prim_mode_params); > @@ -2065,6 +2202,13 @@ static void draw_subtest(const struct test_mode *t) > if (op_disables_psr(t, t->method)) > assertions |= ASSERT_PSR_DISABLED; > > + /* > + * On FBS_INDIVIDUAL, write to offscreen plane will not touch the > + * current frambuffer. Hence assert for DRRS_LOW. > + */ > + if ((t->fbs == FBS_INDIVIDUAL) && (t->screen == SCREEN_OFFSCREEN)) > + assertions |= ASSERT_DRRS_LOW; > + > prepare_subtest(t, pattern); > target = pick_target(t, params); > > @@ -2273,7 +2417,11 @@ static void slow_draw_subtest(const struct test_mode *t) > sleep(2); > > update_wanted_crc(t, &pattern->crcs[t->format][r]); > - do_assertions(0); > + > + if (t->feature & FEATURE_DRRS) > + do_assertions(ASSERT_DRRS_LOW); > + else > + do_assertions(0); > } > } > > @@ -2882,14 +3030,14 @@ static void suspend_subtest(const struct test_mode *t) > sleep(5); > igt_system_suspend_autoresume(SUSPEND_STATE_MEM, SUSPEND_TEST_NONE); > sleep(5); > - do_assertions(0); > + do_assertions(ASSERT_DRRS_LOW); > > unset_all_crtcs(); > sleep(5); > igt_system_suspend_autoresume(SUSPEND_STATE_MEM, SUSPEND_TEST_NONE); > sleep(5); > do_assertions(ASSERT_FBC_DISABLED | ASSERT_PSR_DISABLED | > - DONT_ASSERT_CRC); > + DONT_ASSERT_CRC | ASSERT_DRRS_INACTIVE); > > set_mode_for_params(params); > do_assertions(0); > @@ -3371,6 +3519,14 @@ static const char *feature_str(int feature) > return "psr"; > case FEATURE_FBC | FEATURE_PSR: > return "fbcpsr"; > + case FEATURE_DRRS: > + return "drrs"; > + case FEATURE_FBC | FEATURE_DRRS: > + return "fbcdrrs"; > + case FEATURE_PSR | FEATURE_DRRS: > + return "psrdrrs"; > + case FEATURE_FBC | FEATURE_PSR | FEATURE_DRRS: > + return "fbcpsrdrrs"; > default: > igt_assert(false); > } > @@ -3635,7 +3791,7 @@ int main(int argc, char *argv[]) > tilingchange_subtest(&t); > } > > - if (t.feature & FEATURE_PSR) > + if ((t.feature & FEATURE_PSR) || (t.feature & FEATURE_DRRS)) > igt_subtest_f("%s-slowdraw", feature_str(t.feature)) > slow_draw_subtest(&t); > > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
