[PATCH] tests/kms_universal_plane: Re-work expectations for non-intel hw

[Kamil Konieczny]

Hi Rob,
On 2024-01-10 at 12:04:34 -0800, Rob Clark wrote:
> From: Rob Clark <robdclark at chromium.org>
> 
> The expectation assumptions do not fit for non-intel hw.  So re-work the
> expectations and add msm support.
> 
> Signed-off-by: Rob Clark <robdclark at chromium.org>
> ---
>  tests/kms_universal_plane.c | 102 +++++++++++++++++++++++++-----------
>  1 file changed, 72 insertions(+), 30 deletions(-)
> 
> diff --git a/tests/kms_universal_plane.c b/tests/kms_universal_plane.c
> index 6a39f93cc5aa..3158ff816046 100644
> --- a/tests/kms_universal_plane.c
> +++ b/tests/kms_universal_plane.c
> @@ -402,8 +402,7 @@ sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
>  	sanity_test_t test = { .data = data };
>  	igt_plane_t *primary;
>  	drmModeModeInfo *mode;
> -	int i;
> -	int expect = 0;

Could you keep this var?

> +	int i, ret;
>  
>  	igt_output_set_pipe(output, pipe);
>  	mode = igt_output_get_mode(output);
> @@ -418,43 +417,86 @@ sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
>  
>  	/*
>  	 * Try to use universal plane API to set primary plane that
> -	 * doesn't cover CRTC (should fail on pre-gen9 and succeed on
> -	 * gen9+).
> +	 * doesn't cover CRTC.
>  	 */
>  	igt_plane_set_fb(primary, &test.undersized_fb);
> -	if (is_intel_device(data->drm_fd))
> -		expect = (data->display_ver < 9) ? -EINVAL : 0;
> -	igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> +	ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> +
> +	if (is_intel_device(data->drm_fd)) {
> +		/* should fail on pre-gen9 and succeed on gen9+: */
> +		if (data->display_ver < 9)
> +			igt_assert_eq(ret, -EINVAL);
> +		else
> +			igt_assert_eq(ret, 0);
> +	} else if (is_msm_device(data->drm_fd)) {
> +		/* Should be supported on all gens: */
> +		igt_assert_eq(ret, 0);
> +	} else {
> +		igt_assert_eq(ret, 0);
> +	}

The old code above should work on msm, why do you changed it?

>  
> -	/* Same as above, but different plane positioning. */
> +	/* Same as above, but different plane positioning: */

Drop this change.

>  	igt_plane_set_position(primary, 100, 100);
> -	igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> +	ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> +
> +	if (is_intel_device(data->drm_fd)) {
> +		/* should fail on pre-gen9 and succeed on gen9+: */
> +		if (data->display_ver < 9)
> +			igt_assert_eq(ret, -EINVAL);
> +		else
> +			igt_assert_eq(ret, 0);
> +	} else if (is_msm_device(data->drm_fd)) {
> +		/* Should be supported on all gens: */
> +		igt_assert_eq(ret, 0);
> +	} else {
> +		igt_assert_eq(ret, 0);
> +	}

Same here, old code should just work for msm.

>  
>  	igt_plane_set_position(primary, 0, 0);
>  
> -	/* Try to use universal plane API to scale down (should fail on pre-gen9) */
> -	if (is_intel_device(data->drm_fd))
> -		expect = (data->display_ver < 9) ? -ERANGE : 0;

Please keep this if(...) expect = ...
for a reason which follows.

> -	igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> -				   output->config.crtc->crtc_id,
> -				   test.oversized_fb.fb_id, 0,
> -				   0, 0,
> -				   mode->hdisplay + 100,
> -				   mode->vdisplay + 100,
> -				   IGT_FIXED(0,0), IGT_FIXED(0,0),
> -				   IGT_FIXED(mode->hdisplay,0),
> -				   IGT_FIXED(mode->vdisplay,0)) == expect);
> +	/* Try to use universal plane API to scale down */
> +	ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> +			      output->config.crtc->crtc_id,
> +			      test.oversized_fb.fb_id, 0,
> +			      0, 0,
> +			      mode->hdisplay + 100,
> +			      mode->vdisplay + 100,
> +			      IGT_FIXED(0,0), IGT_FIXED(0,0),
> +			      IGT_FIXED(mode->hdisplay,0),
> +			      IGT_FIXED(mode->vdisplay,0));
> +
> +	if (is_intel_device(data->drm_fd)) {
> +		/* should fail on pre-gen9 and succeed on gen9+: */
> +		if (data->display_ver < 9)
> +			igt_assert_eq(ret, -ERANGE);
> +		else
> +			igt_assert_eq(ret, 0);

Or just simple
			igt_assert_eq(ret, expect);

> +	} else {
> +		/* Could succeed or fail with -ERANGE depending on hw limits: */
> +		igt_assert((ret == 0) || (ret == -ERANGE));
> +	}

This is a real change, it relaxed checks for non-intel HW.
imho we should ask others, for example amd developers if that is ok
for them.

>  
>  	/* Try to use universal plane API to scale up (should fail on pre-gen9) */
> -	igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> -				   output->config.crtc->crtc_id,
> -				   test.oversized_fb.fb_id, 0,
> -				   0, 0,
> -				   mode->hdisplay,
> -				   mode->vdisplay,
> -				   IGT_FIXED(0,0), IGT_FIXED(0,0),
> -				   IGT_FIXED(mode->hdisplay - 100,0),
> -				   IGT_FIXED(mode->vdisplay - 100,0)) == expect);
> +	ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> +			      output->config.crtc->crtc_id,
> +			      test.oversized_fb.fb_id, 0,
> +			      0, 0,
> +			      mode->hdisplay,
> +			      mode->vdisplay,
> +			      IGT_FIXED(0,0), IGT_FIXED(0,0),
> +			      IGT_FIXED(mode->hdisplay - 100,0),
> +			      IGT_FIXED(mode->vdisplay - 100,0));
> +
> +	if (is_intel_device(data->drm_fd)) {
> +		/* should fail on pre-gen9 and succeed on gen9+: */
> +		if (data->display_ver < 9)
> +			igt_assert_eq(ret, -ERANGE);
> +		else
> +			igt_assert_eq(ret, 0);

Same here, just simple:
			igt_assert_eq(ret, expect);

Regards,
Kamil

> +	} else {
> +		/* Could succeed or fail with -ERANGE depending on hw limits: */
> +		igt_assert((ret == 0) || (ret == -ERANGE));
> +	}
>  
>  	/* Find other crtcs and try to program our primary plane on them */
>  	for (i = 0; i < test.moderes->count_crtcs; i++)
> -- 
> 2.43.0
>