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

Ramalingam C
2018-01-17T05:09:26Z

Summary from IRC discussions with Rodrigo:

drrs_disable will get called on all test setups(including the drrs
non-capable). So we cant fail/skip the sub_test based on the return
value from drrs_set(0)

drrs_enable will get called only when the test setup is capable of drrs.
And also after the request for enabling drrs, drrs state is verified
through i915_drrs_status.

So functionally it is not necessary to validate the debugfs_write return
value. But no harm in double checking the error state for drrs_set(1).
Rodrigo, as per above understanding i have mentioned the expected deltas
inline. If you confirm, we will submit the next version. Thanks for the
time.

On Wednesday 10 January 2018 08:17 PM, 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@intel.com>
> Signed-off-by: aknautiy <ankit.k.nautiyal@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);
> +}
> +
redefining the __debugfs_write() as below:

static int __debugfs_write(const char *param, char *buf, int len)
{
return igt_sysfs_write(drm.debugfs, param, buf, 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;
removing above two lines.
> +
> + igt_debug("Manually %sabling DRRS. %llu\n", val ? "en" : "dis", val);
> + snprintf(buf, sizeof(buf), "%d", val);
> + debugfs_write("i915_drrs_ctl", buf);
replacing above line with below code

ret = debugfs_write("i915_drrs_ctl", buf);

/* drrs_enable can't fail, as that is called on drrs capable setup only. */
if (val)
igt_assert_eq(ret, sizeof(buf) - 1);

--Ram

> +}
> +
> +static bool is_drrs_high(void)
> +{
> + char buf[MAX_DRRS_STATUS_BUF_LEN];
>