[Intel-gfx] [PATCH i-g-t v13] tests/kms_frontbuffer_tracking: Including DRRS test coverage
Rodrigo Vivi
rodrigo.vivi at intel.com
Thu Jan 11 20:22:42 UTC 2018
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 at intel.com>
> Signed-off-by: aknautiy <ankit.k.nautiyal at intel.com>
> ---
> 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
More information about the Intel-gfx
mailing list