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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Jan 01, 2018 at 01:45:32PM +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.
> 
> Signed-off-by: Lohith BS <lohith.bs@intel.com>
> Signed-off-by: aknautiy <ankit.k.nautiyal@intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 188 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 179 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> index 1601cab..1039c9e 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,15 @@ struct {
>  	.can_test = false,
>  };
>  
> +#define MAX_DRRS_STATUS_BUF_LEN 256
> +
> +struct {
> +	bool can_test;
> +	bool is_psr_drrs_combo;

Why do you need this is_psr_drrs_combo at all?

> +} drrs = {
> +	.can_test = false,
> +	.is_psr_drrs_combo = false,
> +};
>  
>  #define SINK_CRC_SIZE 12
>  typedef struct {
> @@ -825,6 +836,64 @@ 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, "No active crtc found"))
> +		return true;
> +	if (strstr(buf, "Idleness DRRS: Disabled"))
> +		return true;
> +	if (strstr(buf, "DRRS Supported : No"))
> +		return true;
> +
> +	return false;
> +}
> +
> +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 +1004,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 +1260,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 +1585,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 +1660,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 +1690,7 @@ static void setup_environment(void)
>  
>  	setup_fbc();
>  	setup_psr();
> +	setup_drrs();
>  
>  	setup_crcs();
>  }
> @@ -1596,6 +1703,7 @@ static void teardown_environment(void)
>  	teardown_psr();
>  	teardown_fbc();
>  	teardown_modeset();
> +	teardown_drrs();
>  	teardown_drm();
>  }
>  
> @@ -1662,6 +1770,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 +1782,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 +1824,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 +1968,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 +1989,20 @@ 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.
> +	 */
> +	drrs.is_psr_drrs_combo = ((t->feature & FEATURE_PSR) &&
> +				  (t->feature & FEATURE_DRRS));
> +
> +	igt_require_f(!drrs.is_psr_drrs_combo,
> +		      "Can't test PSR and DRRS together\n");

it seems you don't have to save that on global scruct for this local
usage only.

but also why not just

	/*
	 * 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");


everything else lgtm...

> +
>  	if (opt.only_pipes != PIPE_COUNT)
>  		igt_require(t->pipes == opt.only_pipes);
>  }
> @@ -1973,7 +2124,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 +2216,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 +2431,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,17 +3044,17 @@ 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);
> +	do_assertions(ASSERT_DRRS_HIGH);
>  }
>  
>  /**
> @@ -3371,6 +3533,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 +3805,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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx