[igt-dev] [PATCH i-g-t] tests/i915/kms_psr2_su: Convert tests to dynamic

[Hogander, Jouni]

On Wed, 2022-08-17 at 11:35 +0530, Jeevan B wrote:
> Converting the existing subtests to dynamic subtests.

Can you please add a bit more on why you are doing this. Also you are
now adding own subtest for each pipe? Maybe couple of words about that
also.
 
> 
> Signed-off-by: Jeevan B <jeevan.b at intel.com>
> ---
>  tests/i915/kms_psr2_su.c | 64 +++++++++++++++++++++++++-------------
> --
>  1 file changed, 41 insertions(+), 23 deletions(-)
> 
> diff --git a/tests/i915/kms_psr2_su.c b/tests/i915/kms_psr2_su.c
> index caccf713..fcc3105b 100644
> --- a/tests/i915/kms_psr2_su.c
> +++ b/tests/i915/kms_psr2_su.c
> @@ -123,9 +123,15 @@ static void display_fini(data_t *data)
>  	igt_display_fini(&data->display);
>  }
>  
> -static void prepare(data_t *data)
> +static void prepare(data_t *data, enum pipe pipe)
>  {
>  	igt_plane_t *primary;
> +	igt_output_t *output;
> +	igt_display_t *display = &data->display;
> +
> +	igt_display_reset(display);
> +	output = data->output;
> +	igt_output_set_pipe(output, pipe);
>  
>  	/* all green frame */
>  	igt_create_color_fb(data->drm_fd,
> @@ -263,6 +269,7 @@ static void cleanup(data_t *data)
>  igt_main
>  {
>  	data_t data = {};
> +	enum pipe pipe;
>  
>  	igt_fixture {
>  		struct itimerspec interval;
> @@ -287,12 +294,17 @@ igt_main
>  			      "Error enabling PSR2\n");
>  		data.op = FRONTBUFFER;
>  		data.format = DRM_FORMAT_XRGB8888;
> -		prepare(&data);
> -		r = psr_wait_entry(data.debugfs_fd, PSR_MODE_2);
> -		cleanup(&data);
> -		if (!r)
> -			psr_print_debugfs(data.debugfs_fd);
> -		igt_require_f(r, "PSR2 can not be enabled\n");
> +
> +		for_each_pipe_with_valid_output(&data.display, pipe,
> data.output) {
> +			if (pipe != PIPE_A)
> +				break;

Isn't "continue" more correct here? Why can't you just do
prepare(&data, PIPE_A) as anyways your loop is doing that? Maybe adding
more descriptive commit message would help me to understand what you
are doing here and why.

> +			prepare(&data, pipe);
> +			r = psr_wait_entry(data.debugfs_fd,
> PSR_MODE_2);
> +			cleanup(&data);
> +			if (!r)
> +				psr_print_debugfs(data.debugfs_fd);
> +			igt_require_f(r, "PSR2 can not be enabled\n");
> +		}
>  
>  		/* blocking timerfd */
>  		data.change_screen_timerfd =
> timerfd_create(CLOCK_MONOTONIC, 0);
> @@ -312,23 +324,29 @@ igt_main
>  		while (*format != DRM_FORMAT_INVALID) {
>  			data.format = *format++;
>  			igt_describe("Test that selective update works
> when screen changes");
> -			igt_subtest_f("%s-%s", op_str(data.op),
> igt_format_str(data.format)) {
> -				if (data.op == FRONTBUFFER &&
> -				    intel_display_ver(intel_get_drm_dev
> id(data.drm_fd)) >= 12) {
> -					/*
> -					 * FIXME: Display 12+ platforms
> now have PSR2
> -					 * selective fetch enabled by
> default but we
> -					 * still can't properly handle
> frontbuffer
> -					 * rendering, so right it does
> full frame
> -					 * fetches at every frontbuffer
> rendering.
> -					 * So it is expected that this
> test will fail
> -					 * in display 12+ platform for
> now.
> -					 */
> -					igt_skip("PSR2 selective fetch
> is doing full frame fetches for frontbuffer rendering\n");
> +			igt_subtest_with_dynamic_f("%s-%s",
> op_str(data.op), igt_format_str(data.format)) {
> +				for_each_pipe_with_valid_output(&data.d
> isplay, pipe, data.output) {
> +					if (pipe != PIPE_A)
> +		                                break;

Same comment here as above.

> +					igt_dynamic_f("pipe-%s",
> kmstest_pipe_name(pipe)) {
> +						if (data.op ==
> FRONTBUFFER &&
> +						    intel_display_ver(i
> ntel_get_drm_devid(data.drm_fd)) >= 12) {
> +						/*
> +						 * FIXME: Display 12+
> platforms now have PSR2
> +						 * selective fetch
> enabled by default but we
> +						 * still can't properly
> handle frontbuffer
> +						 * rendering, so right
> it does full frame
> +						 * fetches at every
> frontbuffer rendering.
> +						 * So it is expected
> that this test will fail
> +						 * in display 12+
> platform for now.
> +						 */
> +						igt_skip("PSR2
> selective fetch is doing full frame fetches for frontbuffer
> rendering\n");
> +						}
> +					prepare(&data, pipe);
> +					run(&data);
> +					cleanup(&data);
> +					}
>  				}
> -				prepare(&data);
> -				run(&data);
> -				cleanup(&data);
>  			}
>  		}
>  	}

BR,

Jouni Högander