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

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Qua, 2017-12-06 às 20:43 +0530, Lohith BS escreveu:
> 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.


Not a full review, but: coding style and indentation needs to be fixed.

There are a few chunks where the only thing the patch does is make
indentation wrong.

There are chunks that only add blank lines.

There are chunks that leave functions without empty lines between them.

There are chunks that add code that do not follow the coding style used
by the rest of the file (which is the same style used by the Kernel).

There are chunks that use white spaces in places where tabs should be
used.

Please review your own patch before we can review it.

FEATURE_COUNT needs to be 8. Any undesired combinations need explicit
code to avoid them with a nice comment explaining why.

> 
> Signed-off-by: Lohith BS <lohith.bs@intel.com>
> Signed-off-by: aknautiy <ankit.k.nautiyal@intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 166
> +++++++++++++++++++++++++++++++++++----
>  1 file changed, 151 insertions(+), 15 deletions(-)
> 
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> index a068c8a..b06d304 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");
>  
>  /*
>   * 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
> @@ -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];
> @@ -178,10 +180,8 @@ struct {
>  
>  struct {
>  	bool can_test;
> -} psr = {
> -	.can_test = false,
> -};
> -
> +} psr = { .can_test = false,},
> +drrs = { .can_test = false,};
>  
>  #define SINK_CRC_SIZE 12
>  typedef struct {
> @@ -775,8 +775,8 @@ static bool set_mode_for_params(struct
> modeset_params *params)
>  	int rc;
>  
>  	rc = drmModeSetCrtc(drm.fd, params->crtc_id, params->fb.fb-
> >fb_id,
> -			    params->fb.x, params->fb.y,
> -			    &params->connector_id, 1, params->mode);
> +				params->fb.x, params->fb.y,
> +				&params->connector_id, 1, params-
> >mode);
>  	return (rc == 0);
>  }
>  
> @@ -822,6 +822,63 @@ static void psr_print_status(void)
>  	igt_info("PSR status:\n%s\n", buf);
>  }
>  
> +void drrs_set(unsigned int val)
> +{
> +	char buf[16];
> +
> +	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[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_supported(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);
> +
> +	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[256];
> +
> +	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 };
> @@ -932,10 +989,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)
>  {
> @@ -1182,6 +1246,7 @@ static void disable_features(const struct
> test_mode *t)
>  
>  	fbc_disable();
>  	psr_disable();
> +	drrs_disable();
>  }
>  
>  static void *busy_thread_func(void *data)
> @@ -1506,6 +1571,11 @@ 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];
> @@ -1575,6 +1645,30 @@ static void teardown_psr(void)
>  {
>  }
>  
> +static void setup_drrs(void)
> +{
> +	int is_drrs_debug_ctl;
> +	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();
> @@ -1582,7 +1676,7 @@ static void setup_environment(void)
>  
>  	setup_fbc();
>  	setup_psr();
> -
> +	setup_drrs();
>  	setup_crcs();
>  }
>  
> @@ -1594,6 +1688,7 @@ static void teardown_environment(void)
>  	teardown_psr();
>  	teardown_fbc();
>  	teardown_modeset();
> +	teardown_drrs();
>  	teardown_drm();
>  }
>  
> @@ -1660,6 +1755,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 +1767,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 +1809,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());
> @@ -1831,6 +1953,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)
> @@ -1850,6 +1974,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 +2099,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);
> @@ -2219,6 +2347,7 @@ static void badformat_subtest(const struct
> test_mode *t)
>  		assertions |= ASSERT_FBC_DISABLED;
>  	if (!psr_valid)
>  		assertions |= ASSERT_PSR_DISABLED;
> +
>  	do_assertions(assertions);
>  }
>  
> @@ -2277,7 +2406,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);
>  	}
>  }
>  
> @@ -2892,8 +3025,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 |
> -		      DONT_ASSERT_CRC);
> +	do_assertions(ASSERT_FBC_DISABLED | ASSERT_PSR_DISABLED |
> DONT_ASSERT_CRC);
>  
>  	set_mode_for_params(params);
>  	do_assertions(0);
> @@ -3375,6 +3507,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 +3775,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);
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx