Re: [PATCH] Idleness DRRS:

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

On Tue, Sep 19, 2017 at 10:46:49AM +0000, Ramalingam C wrote:
> 
> 
> On Tuesday 19 September 2017 01:24 AM, Rodrigo Vivi wrote:
> > On Mon, Sep 18, 2017 at 08:52:12AM +0000,  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");
> > >   /*
> > >    * 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;
> > > +}
> > > +
> > > +static void teardown_drrs(void)
> > > +{
> > > +}
> > > +
> > >   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();
> > >   	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);
> > I just merged the patch that blocks DRRS if PSR is enabled.
> > 
> > I'm not sure if we now need some changes on this behavirou here forcing PSR flag to be disabled
> > when testing DRRS.
> Hi,
> 
> When PSR is capable on sink and src, this IGT shouldn't test the DRRS.

I believe the test case needs to be smarter than that.
It should actually always test DRRS, not only when PSR is not capable on sink and src.
But disabling i915 param psr_enable and forcing a modeset PSR src will not be ok anymore
so DRRS can be tested. So, whenever we enable PSR by default we don't break DRRS tests on CI.

> This will be made sure using the drrs.cat_test flag (set to False if
> PSR.can_test is True)
> 
> And had a offline discussion with Paulo on adding the DRRS in the
> frontbuffer tracking test.
> At present on a setup, If PSR is not capable and DRRS is capable, DRRS is
> alway enabled.
> Idleness detection and corresponding Vrefresh change will be ongoing
> parallel to FBC testing. It is always FBC||DRRS.
> 
> in Paulo's opinion having a means of dynamically disabling the DRRS is
> compulsory to find out the regression caused by DRRS.
> i.e we can test the combination of features(FBC, FBC||(DRRS/PSR) and
> DRRS/PSR) based on the front buffer tracking,
> against the combination of features(FBC, FBC||PSR, PSR) excluding the DRRS.
> Difference in the results will call out whether a regression is caused due
> to DRRS or not.
> 
> Untill we add a run time disable option for DRRS in kernel, could we have
> the functional test for DRRS merged in the front buffer tracking IGT?
> This wont change the current environment for testing if other features like
> FBC, that is done in this IGT.
> 
> And this can't be final shape of the IGT but a intermediate one.
> Once the disable interface for DRRS is added corresponding tear_down_drrs
> option can be added to this IGT.
> 
> 
> --Ram
> > 
> > > -- 
> > > 1.9.1
> > > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx