Re: [PATCH] Idleness DRRS:

[Ramalingam C <ramalingam.c@intel.com>]

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);
>   

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx