Re: [PATCH i-g-t v12] tests/kms_frontbuffer_tracking: Including DRRS test coverage

Ramalingam C
2018-01-06T13:41:43Z

On Saturday 06 January 2018 04:18 PM, Ramalingam C wrote:
>
>
> On Friday 05 January 2018 11:25 PM, Rodrigo Vivi wrote:
>> On Fri, Jan 05, 2018 at 11:40:29AM +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].
>>>
>>> Signed-off-by: Lohith BS <lohith.bs@intel.com>
>>> Signed-off-by: aknautiy <ankit.k.nautiyal@intel.com>
>> I was almost adding the rv-b here, but I will step back while CI is not
>> fully happy with the patch:
>>
>> https://patchwork.freedesktop.org/series/32888/
>>
>> regressed many sub-tests on snb and hsw...
> Rodrigo, please share the logs for regressions. We will analyze it.
Please ignore this. found in the already shared link.
-Ram
>
> Thanks
> -Ram
>>
>>> ---
>>>   tests/kms_frontbuffer_tracking.c | 178
>>> +++++++++++++++++++++++++++++++++++++--
>>>   1 file changed, 170 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/tests/kms_frontbuffer_tracking.c
>>> b/tests/kms_frontbuffer_tracking.c
>>> index 1601cab..4b87273 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
>>> @@ -156,6 +157,7 @@ struct rect {
>>>   struct {
>>>       int fd;
>>>       int debugfs;
>>> +    int drrs_debugfs_fd;
>>>       drmModeResPtr res;
>>>       drmModeConnectorPtr connectors[MAX_CONNECTORS];
>>>       drmModeEncoderPtr encoders[MAX_ENCODERS];
>>> @@ -182,6 +184,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 {
>>> @@ -825,6 +834,60 @@ static void psr_print_status(void)
>>>       igt_info("PSR status:\n%s\n", buf);
>>>   }
>>>   +void drrs_set(unsigned int val)
>>> +{
>>> +    char buf[2];
>>> +
>>> +    igt_debug("Manually %sabling DRRS. %llu\n", val ? "en" : "dis",
>>> val);
>>> +    snprintf(buf, sizeof(buf), "%d", val);
>>> +    igt_assert_eq(write(drm.drrs_debugfs_fd,
>>> +              buf, strlen(buf)), strlen(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 +998,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 +1254,7 @@ static void disable_features(const struct
>>> test_mode *t)
>>>         fbc_disable();
>>>       psr_disable();
>>> +    drrs_disable();
>>>   }
>>>     static void *busy_thread_func(void *data)
>>> @@ -1508,6 +1579,12 @@ static void teardown_crcs(void)
>>>       igt_pipe_crc_free(pipe_crc);
>>>   }
>>>   +static void teardown_drrs(void)
>>> +{
>>> +    if (drm.drrs_debugfs_fd != -1)
>>> +        close(drm.drrs_debugfs_fd);
>>> +}
>>> +
>>>   static bool fbc_supported_on_chipset(void)
>>>   {
>>>       char buf[128];
>>> @@ -1577,6 +1654,29 @@ 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;
>>> +    }
>>> +
>>> +    drm.drrs_debugfs_fd = openat(drm.debugfs, "i915_drrs_ctl",
>>> O_WRONLY);
>>> +
>>> +    if (drm.drrs_debugfs_fd > 0) {
>>> +        drrs.can_test = true;
>>> +    } else {
>>> +        igt_info("Can't test DRRS: Debugfs entry i915_drrs_ctl not
>>> found.\n");
>>> +        drrs.can_test = false;
>>> +    }
>>> +}
>>> +
>>>   static void setup_environment(void)
>>>   {
>>>       setup_drm();
>>> @@ -1584,6 +1684,7 @@ static void setup_environment(void)
>>>         setup_fbc();
>>>       setup_psr();
>>> +    setup_drrs();
>>>         setup_crcs();
>>>   }
>>> @@ -1596,6 +1697,7 @@ static void teardown_environment(void)
>>>       teardown_psr();
>>>       teardown_fbc();
>>>       teardown_modeset();
>>> +    teardown_drrs();
>>>       teardown_drm();
>>>   }
>>>   @@ -1662,6 +1764,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 +1776,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 +1818,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 +1962,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 +1983,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 +2116,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 +2208,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 +2423,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 +3036,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 +3525,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 +3797,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
>>>
>