[igt-dev] [PATCH i-g-t 2/2] tests/kms_frontbuffer_tracking: Remove redundant modesets during subtest start, v4.

[Dhinakaran Pandiyan]

On Tue, 2018-08-14 at 14:22 +0200, Maarten Lankhorst wrote:
> CRC capturing enables the display, then disables it again. With
> igt_display we can use igt_display_reset to restore the original
> state,
> without committing it to the hw.
> 
> All subtests first set their own state anyway, so we can save up on
> the number of commits.
> 
> Changes since v1:
> - Try to avoid modesets for PSR if the kernel supports it, but
> otherwise force
>   a modeset for the changes to take effect.
> Changes since v2:
> - Rebase on top of previous PSR changes.
> Changes since v3:
> - Rebase on move to lib/igt_psr.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 57 ++++++++++++++++++++++++----
> ----
>  1 file changed, 44 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> index 7ea2f697184d..2cec30da2750 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -1129,14 +1129,14 @@ static void unset_all_crtcs(void)
>  	igt_display_commit(&drm.display);
>  }
>  
> -static void disable_features(const struct test_mode *t)
> +static bool disable_features(const struct test_mode *t)
>  {
>  	if (t->feature == FEATURE_DEFAULT)
> -		return;
> +		return false;
>  
>  	fbc_disable();
> -	psr_disable(drm.debugfs);
>  	drrs_disable();
> +	return psr_disable(drm.debugfs);
>  }
>  
>  static void *busy_thread_func(void *data)
> @@ -1220,7 +1220,7 @@ static void init_blue_crc(enum pixel_format
> format)
>  
>  	print_crc("Blue CRC:  ", &blue_crcs[format].crc);
>  
> -	unset_all_crtcs();
> +	igt_display_reset(&drm.display);
>  
>  	igt_remove_fb(drm.fd, &blue);
>  
> @@ -1272,7 +1272,7 @@ static void init_crcs(enum pixel_format format,
>  		print_crc("", &pattern->crcs[format][r]);
>  	}
>  
> -	unset_all_crtcs();
> +	igt_display_reset(&drm.display);
>  
>  	for (r = 0; r < pattern->n_rects; r++)
>  		igt_remove_fb(drm.fd, &tmp_fbs[r]);
> @@ -1695,6 +1695,22 @@ static void enable_scnd_screen_and_wait(const
> struct test_mode *t)
>  	do_assertions(ASSERT_NO_ACTION_CHANGE);
>  }
>  
> +static void enable_both_screens_and_wait(const struct test_mode *t)
> +{
> +	fill_fb_region(&prim_mode_params.primary, COLOR_PRIM_BG);
> +	fill_fb_region(&scnd_mode_params.primary, COLOR_SCND_BG);
> +
> +	__set_mode_for_params(&prim_mode_params);
> +	__set_mode_for_params(&scnd_mode_params);
> +
> +	igt_display_commit2(&drm.display, drm.display.is_atomic ?
> COMMIT_ATOMIC : COMMIT_LEGACY);
> +
> +	wanted_crc = &blue_crcs[t->format].crc;
> +	fbc_update_last_action();
> +
> +	do_assertions(ASSERT_NO_ACTION_CHANGE);
> +}
> +
>  static void set_region_for_test(const struct test_mode *t,
>  				struct fb_region *reg)
>  {
> @@ -1709,17 +1725,21 @@ static void set_region_for_test(const struct
> test_mode *t,
>  	do_assertions(ASSERT_NO_ACTION_CHANGE);
>  }
>  
> -static void enable_features_for_test(const struct test_mode *t)
> +static bool enable_features_for_test(const struct test_mode *t)
>  {
> +	bool ret = false;
> +
>  	if (t->feature == FEATURE_DEFAULT)
> -		return;
> +		return false;
>  
>  	if (t->feature & FEATURE_FBC)
>  		fbc_enable();
>  	if (t->feature & FEATURE_PSR)
> -		psr_enable(drm.debugfs);
> +		ret = psr_enable(drm.debugfs);
>  	if (t->feature & FEATURE_DRRS)
>  		drrs_enable();
> +
> +	return ret;
>  }
>  
>  static void check_test_requirements(const struct test_mode *t)
> @@ -1801,28 +1821,40 @@ static void set_crtc_fbs(const struct
> test_mode *t)
>  static void prepare_subtest_data(const struct test_mode *t,
>  				 struct draw_pattern_info *pattern)
>  {
> +	bool need_modeset;
> +
>  	check_test_requirements(t);
>  
>  	stop_busy_thread();
>  
> -	disable_features(t);
> +	need_modeset = disable_features(t);
>  	set_crtc_fbs(t);
>  
>  	if (t->screen == SCREEN_OFFSCREEN)
>  		fill_fb_region(&offscreen_fb, COLOR_OFFSCREEN_BG);
>  
> -	unset_all_crtcs();
> +	igt_display_reset(&drm.display);
> +	if (need_modeset)
> +		igt_display_commit(&drm.display);
>  
>  	init_blue_crc(t->format);
>  	if (pattern)
>  		init_crcs(t->format, pattern);
>  
> -	enable_features_for_test(t);
> +	igt_display_reset(&drm.display);

Both init_blue_crc() and init_crc() call igt_display_reset(). Why is a
another reset required here?

Feel like there's too much going on in this one patch. Can you please
split the modeset eliminations you are implementing? I think changes
corresponding to PSR enabling, CRC initialization and the refactoring
of enable_both_screens_and_wait() all warrant to be individual patches.


-DK

> +
> +	need_modeset = enable_features_for_test(t);
> +	if (need_modeset)
> +		igt_display_commit(&drm.display);
>  }
>  
>  static void prepare_subtest_screens(const struct test_mode *t)
>  {
> -	enable_prim_screen_and_wait(t);
> +	if (t->pipes == PIPE_DUAL)
> +		enable_both_screens_and_wait(t);
> +	else
> +		enable_prim_screen_and_wait(t);
> +
>  	if (t->screen == SCREEN_PRIM) {
>  		if (t->plane == PLANE_CUR)
>  			set_region_for_test(t,
> &prim_mode_params.cursor);
> @@ -1833,7 +1865,6 @@ static void prepare_subtest_screens(const
> struct test_mode *t)
>  	if (t->pipes == PIPE_SINGLE)
>  		return;
>  
> -	enable_scnd_screen_and_wait(t);
>  	if (t->screen == SCREEN_SCND) {
>  		if (t->plane == PLANE_CUR)
>  			set_region_for_test(t,
> &scnd_mode_params.cursor);
-- 
-DK