Re: [PATCH] Idleness DRRS:

Ramalingam C
2017-09-19T10:15:51Z

On Monday 18 September 2017 02:22 PM, Lohith BS wrote:
> From: Lohith BS <lohith.bs@intel.com>
>
> By default the DRRS state will be at DRRS_HIGH_RR. When a Display
> content is Idle for more than 1Sec Idleness will be declared and
> DRRS_LOW_RR will be invoked, changing the refresh rate to the
> lower most refresh rate supported by the panel. As soon as there
> is a display content change there will be a DRRS state transition
> as DRRS_LOW_RR--> DRRS_HIGH_RR, changing the refresh rate to the
> highest refresh rate supported by the panel.
>
> To test this, Idleness DRRS IGT will probe the DRRS state at below
> instances and compare with the expected state.
>
> Instance Expected State
> 1. Immediately after rendering the still image DRRS_HIGH_RR
> 2. After a delay of 1.2Sec DRRS_LOW_RR
> 3. After changing the frame buffer DRRS_HIGH_RR
> 4. After a delay of 1.2Sec DRRS_LOW_RR
> 5. After changing the frame buffer DRRS_HIGH_RR
> 6. After a delay of 1.2Sec DRRS_LOW_RR
>
> The test checks the driver DRRS state from the debugfs entry. To check the
> actual refresh-rate, the number of vblanks received per sec.
> The refresh-rate calculated is checked against the expected refresh-rate
> with a tolerance value of 2.
>
> This patch is a continuation of the earlier work
> https://patchwork.freedesktop.org/patch/45472/ towards igt for idleness
>
> DRRS. The code is tested on Broxton BXT_T platform.
>
> 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/"
>
> Signed-off-by: Lohith BS <lohith.bs@intel.com>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> Signed-off-by: aknautiy <ankit.k.nautiyal@intel.com>
> ---
> tests/kms_frontbuffer_tracking.c | 161 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 152 insertions(+), 9 deletions(-)
>
> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> index a068c8a..4f44109 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 DRRS and PSR");
As DRRS and PSR cant be tested together it will be more meaningful to
write as
"its related features: FBC and
PSR/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 = 6,
> + FEATURE_DEFAULT = 6,
> } feature;
>
> /* Possible pixel formats. We just use FORMAT_DEFAULT for most tests and
> @@ -180,6 +181,9 @@ struct {
> bool can_test;
> } psr = {
> .can_test = false,
> +},
> +drrs = {
> + .can_test = false,
> };
>
>
> @@ -822,6 +826,52 @@ static void psr_print_status(void)
> igt_info("PSR status:\n%s\n", buf);
> }
>
> +static bool is_drrs_high(void)
> +{
> + char buf[256];
> +
> + debugfs_read("i915_drrs_status", buf);
> + return strstr(buf, "DRRS_HIGH_RR");
> +}
> +
> +static bool is_drrs_low(void)
> +{
> + char buf[256];
> +
> + debugfs_read("i915_drrs_status", buf);
> + return strstr(buf, "DRRS_LOW_RR");
> +}
> +
> +static bool is_drrs_enabled(void)
> +{
> + char buf[256];
> +
> + debugfs_read("i915_drrs_status", buf);
> + return strstr(buf, "DRRS Supported: Yes");
> +}
> +
> +static bool is_drrs_inactive(void)
> +{
> + char buf[256];
> +
> + debugfs_read("i915_drrs_status", buf);
> + return strstr(buf, "No active crtc found");
> +}
> +
> +static void drrs_print_status(void)
> +{
> + char buf[256];
> +
> + if (is_drrs_high())
> + igt_info("DRRS STATUS : DRRS HIGH\n");
> +
> + if (is_drrs_low())
> + igt_info("DRRS_STATUS : DRRS LOW\n");
> +
> + if (is_drrs_inactive())
> + igt_info("DRRS_STATUS : DRRS DISABLED\n");
> +}
> +
> static struct timespec fbc_get_last_action(void)
> {
> struct timespec ret = { 0, 0 };
> @@ -1575,6 +1625,25 @@ 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_enabled()) {
> + igt_info("Can't test DRRS: not supported in the driver.\n");
> + return;
> + }
> + drrs.can_test = true;
As PSR is capable on a sink and src, DRRS wont be used. So drrs.can_test
is false if psr.can_test is true.
> +}
> +
> +static void teardown_drrs(void)
> +{
> +}
> +
We can add this if we have means of disabling the DRRS in run time. We
should remove this.
> static void setup_environment(void)
> {
> setup_drm();
> @@ -1582,7 +1651,7 @@ static void setup_environment(void)
>
> setup_fbc();
> setup_psr();
> -
> + setup_drrs();
> setup_crcs();
> }
>
> @@ -1592,6 +1661,7 @@ static void teardown_environment(void)
>
> teardown_crcs();
> teardown_psr();
> + teardown_drrs();
I dont think we need this function call.

--Ram
> teardown_fbc();
> teardown_modeset();
> teardown_drm();
> @@ -1660,6 +1730,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)) {
> @@ -1667,12 +1742,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;
> }
> @@ -1704,6 +1784,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 (!is_drrs_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 DISABLED\n");
> + }
> + }
> +
> if (flags & ASSERT_FBC_ENABLED) {
> igt_require(!fbc_not_enough_stolen());
> igt_require(!fbc_stride_not_supported());
> @@ -1850,6 +1947,10 @@ 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");
> +
> if (opt.only_pipes != PIPE_COUNT)
> igt_require(t->pipes == opt.only_pipes);
> }
> @@ -1971,7 +2072,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);
> @@ -2008,6 +2109,24 @@ static bool op_disables_psr(const struct test_mode *t,
> return false;
> }
>
> +static bool op_sets_drrs_high(const struct test_mode *t,
> + enum igt_draw_method method)
> +{
> + if (method != IGT_DRAW_MMAP_GTT)
> + return false;
> + if (t->screen == SCREEN_PRIM)
> + return true;
> + /* On FBS_SHARED, even if the target is not the DRRS screen
> + * (SCREEN_PRIM), all primary planes share the same frontbuffer, so a
> + * write to the second screen primary plane - or offscreen plane - will
> + * touch the framebuffer that's also used by the primary screen and making
> + * DRRS state as high
> + */
> + if (t->fbs == FBS_SHARED && t->plane == PLANE_PRI)
> + return true;
> + return false;
> +}
> +
> /*
> * draw - draw a set of rectangles on the screen using the provided method
> *
> @@ -2063,6 +2182,9 @@ static void draw_subtest(const struct test_mode *t)
> if (op_disables_psr(t, t->method))
> assertions |= ASSERT_PSR_DISABLED;
>
> + if (op_sets_drrs_high(t, t->method))
> + assertions |= ASSERT_DRRS_HIGH;
> +
> prepare_subtest(t, pattern);
> target = pick_target(t, params);
>
> @@ -2152,6 +2274,10 @@ static void multidraw_subtest(const struct test_mode *t)
> !wc_used)
> assertions |= ASSERT_PSR_DISABLED;
>
> + if (op_sets_drrs_high(t, used_method) &&
> + !wc_used)
> + assertions |= ASSERT_DRRS_HIGH;
> +
> do_assertions(assertions);
> }
>
> @@ -2206,6 +2332,7 @@ static void badformat_subtest(const struct test_mode *t)
> {
> bool fbc_valid = format_is_valid(FEATURE_FBC, t->format);
> bool psr_valid = format_is_valid(FEATURE_PSR, t->format);
> + bool drrs_valid = format_is_valid(FEATURE_DRRS, t->format);
> int assertions = ASSERT_NO_ACTION_CHANGE;
>
> prepare_subtest_data(t, NULL);
> @@ -2219,6 +2346,9 @@ static void badformat_subtest(const struct test_mode *t)
> assertions |= ASSERT_FBC_DISABLED;
> if (!psr_valid)
> assertions |= ASSERT_PSR_DISABLED;
> + if (!drrs_valid)
> + assertions |= ASSERT_DRRS_HIGH;
> +
> do_assertions(assertions);
> }
>
> @@ -2277,7 +2407,15 @@ 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_PSR) {
> + do_assertions(0);
> + }
> +
> + if (t->feature & FEATURE_DRRS) {
> + sleep(1);
> + do_assertions(ASSERT_DRRS_LOW);
> + }
> +
> }
> }
>
> @@ -2464,6 +2602,7 @@ static void fliptrack_subtest(const struct test_mode *t, enum flip_type type)
> update_wanted_crc(t, &pattern->crcs[t->format][r]);
>
> do_assertions(ASSERT_PSR_DISABLED);
> + do_assertions(ASSERT_DRRS_HIGH);
> }
>
> igt_remove_fb(drm.fd, &fb2);
> @@ -2892,7 +3031,7 @@ 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(ASSERT_FBC_DISABLED | ASSERT_PSR_DISABLED |
> + do_assertions(ASSERT_FBC_DISABLED | ASSERT_PSR_DISABLED | ASSERT_DRRS_HIGH |
> DONT_ASSERT_CRC);
>
> set_mode_for_params(params);
> @@ -2966,7 +3105,7 @@ static void farfromfence_subtest(const struct test_mode *t)
> update_wanted_crc(t, &pattern->crcs[t->format][r]);
>
> /* GTT draws disable PSR. */
> - do_assertions(assertions | ASSERT_PSR_DISABLED);
> + do_assertions(assertions | ASSERT_PSR_DISABLED | ASSERT_DRRS_HIGH);
> }
>
> igt_remove_fb(drm.fd, &tall_fb);
> @@ -3375,6 +3514,10 @@ 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";
> default:
> igt_assert(false);
> }
> @@ -3639,7 +3782,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);
>