[igt-dev] [PATCH i-g-t 0/3] Validate max source size

[igt-dev] [PATCH i-g-t 0/3] Validate max source size

[Swati Sharma]

i915 specific test is added to validate max source size.

Swati Sharma (3):
  tests/kms_plane_scaling: Prep work
  tests/kms_plane_scaling: Remove extra newline
  tests/kms_plane_scaling: Add test to validate max source size

 tests/kms_plane_scaling.c | 113 +++++++++++++++++++++++++++++++++++---
 1 file changed, 104 insertions(+), 9 deletions(-)

-- 
2.25.1

[igt-dev] [PATCH i-g-t 1/3] tests/kms_plane_scaling: Prep work

[Swati Sharma]

struct can be reused to add more test cases.

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 tests/kms_plane_scaling.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
index 4a04f42a0..382d199e0 100644
--- a/tests/kms_plane_scaling.c
+++ b/tests/kms_plane_scaling.c
@@ -42,6 +42,15 @@ typedef struct {
 	bool extended;
 } data_t;
 
+struct invalid_paramtests {
+	const char *testname;
+	uint32_t planesize[2];
+	struct {
+		enum igt_atomic_plane_properties prop;
+		uint32_t value;
+	} params[8];
+};
+
 const struct {
 	const char * const describe;
 	const char * const name;
@@ -827,14 +836,7 @@ static void invalid_parameter_tests(data_t *d)
 	igt_plane_t *plane;
 	int rval;
 
-	const struct {
-		const char *testname;
-		uint32_t planesize[2];
-		struct {
-			enum igt_atomic_plane_properties prop;
-			uint32_t value;
-		} params[8];
-	} paramtests[] = {
+	static const struct invalid_paramtests paramtests[2] = {
 		{
 			.testname = "less-than-1-height-src",
 			.planesize = {256, 8},
-- 
2.25.1

[igt-dev] [PATCH i-g-t 2/3] tests/kms_plane_scaling: Remove extra newline

[Swati Sharma]

Remove extra newline.

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 tests/kms_plane_scaling.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
index 382d199e0..9737d8645 100644
--- a/tests/kms_plane_scaling.c
+++ b/tests/kms_plane_scaling.c
@@ -872,7 +872,6 @@ static void invalid_parameter_tests(data_t *d)
 							paramtests[i].planesize[0],
 							paramtests[i].planesize[1]);
 
-
 				for (uint32_t j = 0; paramtests[i].params[j].prop != ~0; j++)
 					igt_plane_set_prop_value(plane,
 									paramtests[i].params[j].prop,
-- 
2.25.1

[igt-dev] [PATCH i-g-t 3/3] tests/kms_plane_scaling: Add test to validate max source size

[Swati Sharma]

i915 specific test is added to validate max source size. This
test is expected to return -EINVAL for platforms where we have
max source width 4096 whereas it will pass on platforms having
max source width 5120. We have created fb of size 5120x4320
(=size of source) and downscaled to 3840x2400(=size of dest).

On MTL(disp_ver=14), in dmesg we can see

"[drm:skl_update_scaler [i915]] scaler_user index 0.0: src
5120x4320 dst 3840x2400 size is out of scaler range."

since max source width supported is 4096 whereas same test will
pass on ADLP(display_ver=13) since max source width supported
is 5120.

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 tests/kms_plane_scaling.c | 94 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
index 9737d8645..b43d8405f 100644
--- a/tests/kms_plane_scaling.c
+++ b/tests/kms_plane_scaling.c
@@ -890,6 +890,95 @@ static void invalid_parameter_tests(data_t *d)
 	}
 }
 
+/*
+ *	Max source/destination width/height
+ *	for i915 driver.
+ *
+ *	DISPLAY_VER < 11
+ *		max_src_w = 4096
+ *		max_src_h = 4096
+ *		max_dst_w = 4096
+ *		max_dst_h = 4096
+ *
+ *	DISPLAY_VER = 11
+ *		max_src_w = 5120
+ *		max_src_h = 4096
+ *		max_dst_w = 5120
+ *		max_dst_h = 4096
+ *
+ *	DISPLAY_VER = 12-13
+ *		max_src_w = 5120
+ *		max_src_h = 8192
+ *		max_dst_w = 8192
+ *		max_dst_h = 8192
+ *
+ *	DISPLAY_VER = 14
+ *		max_src_w = 4096
+ *		max_src_h = 8192
+ *		max_dst_w = 8192
+ *		max_dst_h = 8192
+ */
+
+static void i915_max_source_size_test(data_t *d)
+{
+	enum pipe pipe = PIPE_A;
+	igt_output_t *output;
+	igt_fb_t fb;
+	igt_plane_t *plane;
+	int rval;
+
+	static const struct invalid_paramtests paramtests[6] = {
+		{
+			.testname = "max-src-size",
+			.planesize = {3840, 2400},
+			.params = {{~0}}
+		},
+	};
+
+	igt_fixture {
+		output = igt_get_single_output_for_pipe(&d->display, pipe);
+		igt_require(output);
+
+		igt_output_set_pipe(output, pipe);
+		plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
+
+		igt_create_fb(d->drm_fd, 5120, 4320,
+			DRM_FORMAT_XRGB8888,
+			DRM_FORMAT_MOD_NONE,
+			&fb);
+	}
+
+	igt_describe("test for validating max source size.");
+	igt_subtest_with_dynamic("i915-max-source-size") {
+		for (uint32_t i = 0; i < ARRAY_SIZE(paramtests); i++) {
+			igt_dynamic(paramtests[i].testname) {
+				igt_plane_set_position(plane, 0, 0);
+				igt_plane_set_fb(plane, &fb);
+				igt_plane_set_size(plane,
+							paramtests[i].planesize[0],
+							paramtests[i].planesize[1]);
+
+				for (uint32_t j = 0; paramtests[i].params[j].prop != ~0; j++)
+					igt_plane_set_prop_value(plane,
+								 paramtests[i].params[j].prop,
+								 paramtests[i].params[j].value);
+
+				rval = igt_display_try_commit2(&d->display, COMMIT_ATOMIC);
+
+				if (intel_display_ver(d->devid) < 11 || intel_display_ver(d->devid) >= 14)
+					igt_assert(rval == -EINVAL || rval == -ERANGE);
+				else
+					igt_assert_eq(rval, 0);
+			}
+		}
+	}
+
+	igt_fixture {
+		igt_remove_fb(d->drm_fd, &fb);
+		igt_output_set_pipe(output, PIPE_NONE);
+	}
+}
+
 static int opt_handler(int opt, int opt_index, void *_data)
 {
 	data_t *data = _data;
@@ -1055,6 +1144,11 @@ igt_main_args("", long_opts, help_str, opt_handler, &data)
 
 	invalid_parameter_tests(&data);
 
+	igt_fixture
+		igt_require_f(data.devid, "This test is valid only for i915 devices.\n");
+
+	i915_max_source_size_test(&data);
+
 	igt_fixture {
 		igt_display_fini(&data.display);
 		close(data.drm_fd);
-- 
2.25.1

[igt-dev] [PATCH i-g-t 3/3] tests/kms_plane_scaling: Add test to validate max source size

[Swati Sharma]

i915 specific test is added to validate max source size. This
test is expected to return -EINVAL for platforms where we have
max source width 4096 whereas it will pass on platforms having
max source width 5120. We have created fb of size 5120x4320
(=size of source) and downscaled to 3840x2400(=size of dest).

On MTL(disp_ver=14), in dmesg we can see
"[drm:skl_update_scaler [i915]] scaler_user index 0.0: src
5120x4320 dst 3840x2400 size is out of scaler range."
since max source width supported is 4096.
whereas same test will pass on ADLP(display_ver=13) since max
source width  supported is 5120.

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 tests/kms_plane_scaling.c | 94 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
index 9737d8645..ae917b202 100644
--- a/tests/kms_plane_scaling.c
+++ b/tests/kms_plane_scaling.c
@@ -890,6 +890,95 @@ static void invalid_parameter_tests(data_t *d)
 	}
 }
 
+/*
+ *	Max source/destination width/height
+ *	for i915 driver.
+ *
+ *	DISPLAY_VER < 11
+ *		max_src_w = 4096
+ *		max_src_h = 4096
+ *		max_dst_w = 4096
+ *		max_dst_h = 4096
+ *
+ *	DISPLAY_VER = 11
+ *		max_src_w = 5120
+ *		max_src_h = 4096
+ *		max_dst_w = 5120
+ *		max_dst_h = 4096
+ *
+ *	DISPLAY_VER = 12-13
+ *		max_src_w = 5120
+ *		max_src_h = 8192
+ *		max_dst_w = 8192
+ *		max_dst_h = 8192
+ *
+ *	DISPLAY_VER = 14
+ *		max_src_w = 4096
+ *		max_src_h = 8192
+ *		max_dst_w = 8192
+ *		max_dst_h = 8192
+ */
+
+static void i915_max_source_size_test(data_t *d)
+{
+	enum pipe pipe = PIPE_A;
+	igt_output_t *output;
+	igt_fb_t fb;
+	igt_plane_t *plane;
+	int rval;
+
+	static const struct invalid_paramtests paramtests[1] = {
+		{
+			.testname = "max-src-size",
+			.planesize = {3840, 2400},
+			.params = {{~0}}
+		},
+	};
+
+	igt_fixture {
+		output = igt_get_single_output_for_pipe(&d->display, pipe);
+		igt_require(output);
+
+		igt_output_set_pipe(output, pipe);
+		plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
+
+		igt_create_fb(d->drm_fd, 5120, 4320,
+			DRM_FORMAT_XRGB8888,
+			DRM_FORMAT_MOD_NONE,
+			&fb);
+	}
+
+	igt_describe("test for validating max source size.");
+	igt_subtest_with_dynamic("i915-max-source-size") {
+		for (uint32_t i = 0; i < ARRAY_SIZE(paramtests); i++) {
+			igt_dynamic(paramtests[i].testname) {
+				igt_plane_set_position(plane, 0, 0);
+				igt_plane_set_fb(plane, &fb);
+				igt_plane_set_size(plane,
+							paramtests[i].planesize[0],
+							paramtests[i].planesize[1]);
+
+				for (uint32_t j = 0; paramtests[i].params[j].prop != ~0; j++)
+					igt_plane_set_prop_value(plane,
+								 paramtests[i].params[j].prop,
+								 paramtests[i].params[j].value);
+
+				rval = igt_display_try_commit2(&d->display, COMMIT_ATOMIC);
+
+				if (intel_display_ver(d->devid) < 11 || intel_display_ver(d->devid) >= 14)
+					igt_assert(rval == -EINVAL || rval == -ERANGE);
+				else
+					igt_assert_eq(rval, 0);
+			}
+		}
+	}
+
+	igt_fixture {
+		igt_remove_fb(d->drm_fd, &fb);
+		igt_output_set_pipe(output, PIPE_NONE);
+	}
+}
+
 static int opt_handler(int opt, int opt_index, void *_data)
 {
 	data_t *data = _data;
@@ -1055,6 +1144,11 @@ igt_main_args("", long_opts, help_str, opt_handler, &data)
 
 	invalid_parameter_tests(&data);
 
+	igt_fixture
+		igt_require_f(data.devid, "This test is valid only for i915 devices.\n");
+
+	i915_max_source_size_test(&data);
+
 	igt_fixture {
 		igt_display_fini(&data.display);
 		close(data.drm_fd);
-- 
2.25.1

[igt-dev] [PATCH i-g-t 3/3] tests/kms_plane_scaling: Add test to validate max source size

[Swati Sharma]

i915 specific test is added to validate max source size. This
test is expected to return -EINVAL for platforms where we have
max source width 4096 whereas it will pass on platforms having
max source width 5120. We have created fb of size 5120x4320
(=size of source) and downscaled to 3840x2400(=size of dest).

On MTL(disp_ver=14), in dmesg we can see
"[drm:skl_update_scaler [i915]] scaler_user index 0.0: src
5120x4320 dst 3840x2400 size is out of scaler range."
since max source width supported is 4096.
whereas same test will pass on ADLP(display_ver=13) since max
source width  supported is 5120.

Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 tests/kms_plane_scaling.c | 94 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
index 9737d8645..ae917b202 100644
--- a/tests/kms_plane_scaling.c
+++ b/tests/kms_plane_scaling.c
@@ -890,6 +890,95 @@ static void invalid_parameter_tests(data_t *d)
 	}
 }
 
+/*
+ *	Max source/destination width/height
+ *	for i915 driver.
+ *
+ *	DISPLAY_VER < 11
+ *		max_src_w = 4096
+ *		max_src_h = 4096
+ *		max_dst_w = 4096
+ *		max_dst_h = 4096
+ *
+ *	DISPLAY_VER = 11
+ *		max_src_w = 5120
+ *		max_src_h = 4096
+ *		max_dst_w = 5120
+ *		max_dst_h = 4096
+ *
+ *	DISPLAY_VER = 12-13
+ *		max_src_w = 5120
+ *		max_src_h = 8192
+ *		max_dst_w = 8192
+ *		max_dst_h = 8192
+ *
+ *	DISPLAY_VER = 14
+ *		max_src_w = 4096
+ *		max_src_h = 8192
+ *		max_dst_w = 8192
+ *		max_dst_h = 8192
+ */
+
+static void i915_max_source_size_test(data_t *d)
+{
+	enum pipe pipe = PIPE_A;
+	igt_output_t *output;
+	igt_fb_t fb;
+	igt_plane_t *plane;
+	int rval;
+
+	static const struct invalid_paramtests paramtests[1] = {
+		{
+			.testname = "max-src-size",
+			.planesize = {3840, 2400},
+			.params = {{~0}}
+		},
+	};
+
+	igt_fixture {
+		output = igt_get_single_output_for_pipe(&d->display, pipe);
+		igt_require(output);
+
+		igt_output_set_pipe(output, pipe);
+		plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
+
+		igt_create_fb(d->drm_fd, 5120, 4320,
+			DRM_FORMAT_XRGB8888,
+			DRM_FORMAT_MOD_NONE,
+			&fb);
+	}
+
+	igt_describe("test for validating max source size.");
+	igt_subtest_with_dynamic("i915-max-source-size") {
+		for (uint32_t i = 0; i < ARRAY_SIZE(paramtests); i++) {
+			igt_dynamic(paramtests[i].testname) {
+				igt_plane_set_position(plane, 0, 0);
+				igt_plane_set_fb(plane, &fb);
+				igt_plane_set_size(plane,
+							paramtests[i].planesize[0],
+							paramtests[i].planesize[1]);
+
+				for (uint32_t j = 0; paramtests[i].params[j].prop != ~0; j++)
+					igt_plane_set_prop_value(plane,
+								 paramtests[i].params[j].prop,
+								 paramtests[i].params[j].value);
+
+				rval = igt_display_try_commit2(&d->display, COMMIT_ATOMIC);
+
+				if (intel_display_ver(d->devid) < 11 || intel_display_ver(d->devid) >= 14)
+					igt_assert(rval == -EINVAL || rval == -ERANGE);
+				else
+					igt_assert_eq(rval, 0);
+			}
+		}
+	}
+
+	igt_fixture {
+		igt_remove_fb(d->drm_fd, &fb);
+		igt_output_set_pipe(output, PIPE_NONE);
+	}
+}
+
 static int opt_handler(int opt, int opt_index, void *_data)
 {
 	data_t *data = _data;
@@ -1055,6 +1144,11 @@ igt_main_args("", long_opts, help_str, opt_handler, &data)
 
 	invalid_parameter_tests(&data);
 
+	igt_fixture
+		igt_require_f(data.devid, "This test is valid only for i915 devices.\n");
+
+	i915_max_source_size_test(&data);
+
 	igt_fixture {
 		igt_display_fini(&data.display);
 		close(data.drm_fd);
-- 
2.25.1

Re: [igt-dev] [PATCH i-g-t 3/3] tests/kms_plane_scaling: Add test to validate max source size

[Juha-Pekka Heikkila]

On 15.2.2023 18.59, Swati Sharma wrote:
> i915 specific test is added to validate max source size. This
> test is expected to return -EINVAL for platforms where we have
> max source width 4096 whereas it will pass on platforms having
> max source width 5120. We have created fb of size 5120x4320
> (=size of source) and downscaled to 3840x2400(=size of dest).
> 
> On MTL(disp_ver=14), in dmesg we can see
> "[drm:skl_update_scaler [i915]] scaler_user index 0.0: src
> 5120x4320 dst 3840x2400 size is out of scaler range."
> since max source width supported is 4096.
> whereas same test will pass on ADLP(display_ver=13) since max
> source width  supported is 5120.
> 
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>   tests/kms_plane_scaling.c | 94 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 94 insertions(+)
> 
> diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
> index 9737d8645..ae917b202 100644
> --- a/tests/kms_plane_scaling.c
> +++ b/tests/kms_plane_scaling.c
> @@ -890,6 +890,95 @@ static void invalid_parameter_tests(data_t *d)
>   	}
>   }
>   
> +/*
> + *	Max source/destination width/height
> + *	for i915 driver.
> + *
> + *	DISPLAY_VER < 11
> + *		max_src_w = 4096
> + *		max_src_h = 4096
> + *		max_dst_w = 4096
> + *		max_dst_h = 4096
> + *
> + *	DISPLAY_VER = 11
> + *		max_src_w = 5120
> + *		max_src_h = 4096
> + *		max_dst_w = 5120
> + *		max_dst_h = 4096
> + *
> + *	DISPLAY_VER = 12-13
> + *		max_src_w = 5120
> + *		max_src_h = 8192
> + *		max_dst_w = 8192
> + *		max_dst_h = 8192
> + *
> + *	DISPLAY_VER = 14
> + *		max_src_w = 4096
> + *		max_src_h = 8192
> + *		max_dst_w = 8192
> + *		max_dst_h = 8192

I think here could be added comment these numbers are coming from 
drivers/gpu/drm/i915/display/skl_scaler.c in kernel sources

> + */
> +
> +static void i915_max_source_size_test(data_t *d)
> +{
> +	enum pipe pipe = PIPE_A;
> +	igt_output_t *output;
> +	igt_fb_t fb;
> +	igt_plane_t *plane;
> +	int rval;
> +
> +	static const struct invalid_paramtests paramtests[1] = {

"static const struct invalid_paramtests paramtests[]" would be better 
here, ie. not declaring here how long is the list of tests

> +		{
> +			.testname = "max-src-size",
> +			.planesize = {3840, 2400},
> +			.params = {{~0}}
> +		},
> +	};
> +
> +	igt_fixture {
> +		output = igt_get_single_output_for_pipe(&d->display, pipe);
> +		igt_require(output);
> +
> +		igt_output_set_pipe(output, pipe);
> +		plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> +
> +		igt_create_fb(d->drm_fd, 5120, 4320,
> +			DRM_FORMAT_XRGB8888,
> +			DRM_FORMAT_MOD_NONE,
> +			&fb);

above indentation is off on those lines with parameters.

> +	}
> +
> +	igt_describe("test for validating max source size.");
> +	igt_subtest_with_dynamic("i915-max-source-size") {
> +		for (uint32_t i = 0; i < ARRAY_SIZE(paramtests); i++) {
> +			igt_dynamic(paramtests[i].testname) {
> +				igt_plane_set_position(plane, 0, 0);
> +				igt_plane_set_fb(plane, &fb);
> +				igt_plane_set_size(plane,
> +							paramtests[i].planesize[0],
> +							paramtests[i].planesize[1]);
> +
> +				for (uint32_t j = 0; paramtests[i].params[j].prop != ~0; j++)
> +					igt_plane_set_prop_value(plane,
> +								 paramtests[i].params[j].prop,
> +								 paramtests[i].params[j].value);
> +
> +				rval = igt_display_try_commit2(&d->display, COMMIT_ATOMIC);
> +
> +				if (intel_display_ver(d->devid) < 11 || intel_display_ver(d->devid) >= 14)
> +					igt_assert(rval == -EINVAL || rval == -ERANGE);
> +				else
> +					igt_assert_eq(rval, 0);
> +			}
> +		}
> +	}
> +
> +	igt_fixture {
> +		igt_remove_fb(d->drm_fd, &fb);
> +		igt_output_set_pipe(output, PIPE_NONE);
> +	}
> +}
> +
>   static int opt_handler(int opt, int opt_index, void *_data)
>   {
>   	data_t *data = _data;
> @@ -1055,6 +1144,11 @@ igt_main_args("", long_opts, help_str, opt_handler, &data)
>   
>   	invalid_parameter_tests(&data);
>   
> +	igt_fixture
> +		igt_require_f(data.devid, "This test is valid only for i915 devices.\n");
> +
> +	i915_max_source_size_test(&data);
> +

These above changes I think could be wrapped inside igt_subtest_group so 
that i915 requirement doesn't become inherited if someone come add 
something here after this test.

>   	igt_fixture {
>   		igt_display_fini(&data.display);
>   		close(data.drm_fd);

Re: [igt-dev] [PATCH i-g-t 3/3] tests/kms_plane_scaling: Add test to validate max source size

[Kamil Konieczny]

Hi Swati,

On 2023-02-15 at 22:29:47 +0530, Swati Sharma wrote:
> i915 specific test is added to validate max source size. This
> test is expected to return -EINVAL for platforms where we have
> max source width 4096 whereas it will pass on platforms having
> max source width 5120. We have created fb of size 5120x4320
> (=size of source) and downscaled to 3840x2400(=size of dest).
> 
> On MTL(disp_ver=14), in dmesg we can see
> "[drm:skl_update_scaler [i915]] scaler_user index 0.0: src
> 5120x4320 dst 3840x2400 size is out of scaler range."
> since max source width supported is 4096.
> whereas same test will pass on ADLP(display_ver=13) since max
> source width  supported is 5120.
> 
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>

+cc Juha-Pekka

> ---
>  tests/kms_plane_scaling.c | 94 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 94 insertions(+)
> 
> diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
> index 9737d8645..ae917b202 100644
> --- a/tests/kms_plane_scaling.c
> +++ b/tests/kms_plane_scaling.c
> @@ -890,6 +890,95 @@ static void invalid_parameter_tests(data_t *d)
>  	}
>  }
>  
> +/*
> + *	Max source/destination width/height
> + *	for i915 driver.
> + *
> + *	DISPLAY_VER < 11
> + *		max_src_w = 4096
> + *		max_src_h = 4096
> + *		max_dst_w = 4096
> + *		max_dst_h = 4096
> + *
> + *	DISPLAY_VER = 11
> + *		max_src_w = 5120
> + *		max_src_h = 4096
> + *		max_dst_w = 5120
> + *		max_dst_h = 4096
> + *
> + *	DISPLAY_VER = 12-13
> + *		max_src_w = 5120
> + *		max_src_h = 8192
> + *		max_dst_w = 8192
> + *		max_dst_h = 8192
> + *
> + *	DISPLAY_VER = 14
> + *		max_src_w = 4096
> + *		max_src_h = 8192
> + *		max_dst_w = 8192
> + *		max_dst_h = 8192
> + */
> +
> +static void i915_max_source_size_test(data_t *d)
> +{
> +	enum pipe pipe = PIPE_A;
> +	igt_output_t *output;
> +	igt_fb_t fb;
> +	igt_plane_t *plane;
> +	int rval;
> +
> +	static const struct invalid_paramtests paramtests[1] = {
> +		{
> +			.testname = "max-src-size",
> +			.planesize = {3840, 2400},
> +			.params = {{~0}}
> +		},
> +	};
> +
> +	igt_fixture {
> +		output = igt_get_single_output_for_pipe(&d->display, pipe);
> +		igt_require(output);
--------------- ^
You are not in subtest.

> +
> +		igt_output_set_pipe(output, pipe);
> +		plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> +
> +		igt_create_fb(d->drm_fd, 5120, 4320,
> +			DRM_FORMAT_XRGB8888,
> +			DRM_FORMAT_MOD_NONE,
> +			&fb);
> +	}

Is this fixture only for i915 ? It looks generic, even if
igt_create_fb will fail (but you need to remember fb_id here).

> +
> +	igt_describe("test for validating max source size.");
> +	igt_subtest_with_dynamic("i915-max-source-size") {

Here you can put igt_require for i915 driver and also
require for fb_id != 0.

> +		for (uint32_t i = 0; i < ARRAY_SIZE(paramtests); i++) {
> +			igt_dynamic(paramtests[i].testname) {
> +				igt_plane_set_position(plane, 0, 0);
> +				igt_plane_set_fb(plane, &fb);
> +				igt_plane_set_size(plane,
> +							paramtests[i].planesize[0],
> +							paramtests[i].planesize[1]);
> +
> +				for (uint32_t j = 0; paramtests[i].params[j].prop != ~0; j++)
> +					igt_plane_set_prop_value(plane,
> +								 paramtests[i].params[j].prop,
> +								 paramtests[i].params[j].value);
> +
> +				rval = igt_display_try_commit2(&d->display, COMMIT_ATOMIC);
> +
> +				if (intel_display_ver(d->devid) < 11 || intel_display_ver(d->devid) >= 14)
> +					igt_assert(rval == -EINVAL || rval == -ERANGE);
> +				else
> +					igt_assert_eq(rval, 0);
> +			}
> +		}
> +	}
> +
> +	igt_fixture {
> +		igt_remove_fb(d->drm_fd, &fb);
> +		igt_output_set_pipe(output, PIPE_NONE);
> +	}
> +}
> +
>  static int opt_handler(int opt, int opt_index, void *_data)
>  {
>  	data_t *data = _data;
> @@ -1055,6 +1144,11 @@ igt_main_args("", long_opts, help_str, opt_handler, &data)
>  
>  	invalid_parameter_tests(&data);
>  
> +	igt_fixture
> +		igt_require_f(data.devid, "This test is valid only for i915 devices.\n");

Do not do this, use igt_require_f inside (dynamic_)subtest.

Regards,
Kamil

> +
> +	i915_max_source_size_test(&data);
> +
>  	igt_fixture {
>  		igt_display_fini(&data.display);
>  		close(data.drm_fd);
> -- 
> 2.25.1
> 

Re: [igt-dev] [PATCH i-g-t 3/3] tests/kms_plane_scaling: Add test to validate max source size

[Juha-Pekka Heikkila]

On 16.2.2023 21.29, Kamil Konieczny wrote:
> Hi Swati,
> 
> On 2023-02-15 at 22:29:47 +0530, Swati Sharma wrote:
>> i915 specific test is added to validate max source size. This
>> test is expected to return -EINVAL for platforms where we have
>> max source width 4096 whereas it will pass on platforms having
>> max source width 5120. We have created fb of size 5120x4320
>> (=size of source) and downscaled to 3840x2400(=size of dest).
>>
>> On MTL(disp_ver=14), in dmesg we can see
>> "[drm:skl_update_scaler [i915]] scaler_user index 0.0: src
>> 5120x4320 dst 3840x2400 size is out of scaler range."
>> since max source width supported is 4096.
>> whereas same test will pass on ADLP(display_ver=13) since max
>> source width  supported is 5120.
>>
>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> 
> +cc Juha-Pekka
> 
>> ---
>>   tests/kms_plane_scaling.c | 94 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 94 insertions(+)
>>
>> diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
>> index 9737d8645..ae917b202 100644
>> --- a/tests/kms_plane_scaling.c
>> +++ b/tests/kms_plane_scaling.c
>> @@ -890,6 +890,95 @@ static void invalid_parameter_tests(data_t *d)
>>   	}
>>   }
>>   
>> +/*
>> + *	Max source/destination width/height
>> + *	for i915 driver.
>> + *
>> + *	DISPLAY_VER < 11
>> + *		max_src_w = 4096
>> + *		max_src_h = 4096
>> + *		max_dst_w = 4096
>> + *		max_dst_h = 4096
>> + *
>> + *	DISPLAY_VER = 11
>> + *		max_src_w = 5120
>> + *		max_src_h = 4096
>> + *		max_dst_w = 5120
>> + *		max_dst_h = 4096
>> + *
>> + *	DISPLAY_VER = 12-13
>> + *		max_src_w = 5120
>> + *		max_src_h = 8192
>> + *		max_dst_w = 8192
>> + *		max_dst_h = 8192
>> + *
>> + *	DISPLAY_VER = 14
>> + *		max_src_w = 4096
>> + *		max_src_h = 8192
>> + *		max_dst_w = 8192
>> + *		max_dst_h = 8192
>> + */
>> +
>> +static void i915_max_source_size_test(data_t *d)
>> +{
>> +	enum pipe pipe = PIPE_A;
>> +	igt_output_t *output;
>> +	igt_fb_t fb;
>> +	igt_plane_t *plane;
>> +	int rval;
>> +
>> +	static const struct invalid_paramtests paramtests[1] = {
>> +		{
>> +			.testname = "max-src-size",
>> +			.planesize = {3840, 2400},
>> +			.params = {{~0}}
>> +		},
>> +	};
>> +
>> +	igt_fixture {
>> +		output = igt_get_single_output_for_pipe(&d->display, pipe);
>> +		igt_require(output);
> --------------- ^
> You are not in subtest.
> 
>> +
>> +		igt_output_set_pipe(output, pipe);
>> +		plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
>> +
>> +		igt_create_fb(d->drm_fd, 5120, 4320,
>> +			DRM_FORMAT_XRGB8888,
>> +			DRM_FORMAT_MOD_NONE,
>> +			&fb);
>> +	}
> 
> Is this fixture only for i915 ? It looks generic, even if
> igt_create_fb will fail (but you need to remember fb_id here).

just quickly drive by commenting.. these tests on this patch are for 
i915, hence above fixture is for tests below.

> 
>> +
>> +	igt_describe("test for validating max source size.");
>> +	igt_subtest_with_dynamic("i915-max-source-size") {
> 
> Here you can put igt_require for i915 driver and also
> require for fb_id != 0.
> 

If igt_create_fb will fail it will assert out at 
igt_create_fb_with_bo_size and there's no point in checking for fb_id 
being zero.

>> +		for (uint32_t i = 0; i < ARRAY_SIZE(paramtests); i++) {
>> +			igt_dynamic(paramtests[i].testname) {
>> +				igt_plane_set_position(plane, 0, 0);
>> +				igt_plane_set_fb(plane, &fb);
>> +				igt_plane_set_size(plane,
>> +							paramtests[i].planesize[0],
>> +							paramtests[i].planesize[1]);
>> +
>> +				for (uint32_t j = 0; paramtests[i].params[j].prop != ~0; j++)
>> +					igt_plane_set_prop_value(plane,
>> +								 paramtests[i].params[j].prop,
>> +								 paramtests[i].params[j].value);
>> +
>> +				rval = igt_display_try_commit2(&d->display, COMMIT_ATOMIC);
>> +
>> +				if (intel_display_ver(d->devid) < 11 || intel_display_ver(d->devid) >= 14)
>> +					igt_assert(rval == -EINVAL || rval == -ERANGE);
>> +				else
>> +					igt_assert_eq(rval, 0);
>> +			}
>> +		}
>> +	}
>> +
>> +	igt_fixture {
>> +		igt_remove_fb(d->drm_fd, &fb);
>> +		igt_output_set_pipe(output, PIPE_NONE);
>> +	}
>> +}
>> +
>>   static int opt_handler(int opt, int opt_index, void *_data)
>>   {
>>   	data_t *data = _data;
>> @@ -1055,6 +1144,11 @@ igt_main_args("", long_opts, help_str, opt_handler, &data)
>>   
>>   	invalid_parameter_tests(&data);
>>   
>> +	igt_fixture
>> +		igt_require_f(data.devid, "This test is valid only for i915 devices.\n");
> 
> Do not do this, use igt_require_f inside (dynamic_)subtest.

Imo igt_subtest_group is what's needed to be put around these, fixture 
is meant for this checking when anything inside will depend on 
something. Putting igt_require_f inside subtest will just waste test 
execution time when on platform where required conditions are not met. 
You can see same pattern used on other tests, simplest example coming to 
my mind at kms_addfb_basic.

/Juha-Pekka

> 
>> +
>> +	i915_max_source_size_test(&data);
>> +
>>   	igt_fixture {
>>   		igt_display_fini(&data.display);
>>   		close(data.drm_fd);
>> -- 
>> 2.25.1
>>

Re: [igt-dev] [PATCH i-g-t 3/3] tests/kms_plane_scaling: Add test to validate max source size

[Swati Sharma]

On 16-Feb-23 9:01 PM, Juha-Pekka Heikkila wrote:
> On 15.2.2023 18.59, Swati Sharma wrote:
>> i915 specific test is added to validate max source size. This
>> test is expected to return -EINVAL for platforms where we have
>> max source width 4096 whereas it will pass on platforms having
>> max source width 5120. We have created fb of size 5120x4320
>> (=size of source) and downscaled to 3840x2400(=size of dest).
>>
>> On MTL(disp_ver=14), in dmesg we can see
>> "[drm:skl_update_scaler [i915]] scaler_user index 0.0: src
>> 5120x4320 dst 3840x2400 size is out of scaler range."
>> since max source width supported is 4096.
>> whereas same test will pass on ADLP(display_ver=13) since max
>> source width  supported is 5120.
>>
>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>> ---
>>   tests/kms_plane_scaling.c | 94 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 94 insertions(+)
>>
>> diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
>> index 9737d8645..ae917b202 100644
>> --- a/tests/kms_plane_scaling.c
>> +++ b/tests/kms_plane_scaling.c
>> @@ -890,6 +890,95 @@ static void invalid_parameter_tests(data_t *d)
>>       }
>>   }
>> +/*
>> + *    Max source/destination width/height
>> + *    for i915 driver.
>> + *
>> + *    DISPLAY_VER < 11
>> + *        max_src_w = 4096
>> + *        max_src_h = 4096
>> + *        max_dst_w = 4096
>> + *        max_dst_h = 4096
>> + *
>> + *    DISPLAY_VER = 11
>> + *        max_src_w = 5120
>> + *        max_src_h = 4096
>> + *        max_dst_w = 5120
>> + *        max_dst_h = 4096
>> + *
>> + *    DISPLAY_VER = 12-13
>> + *        max_src_w = 5120
>> + *        max_src_h = 8192
>> + *        max_dst_w = 8192
>> + *        max_dst_h = 8192
>> + *
>> + *    DISPLAY_VER = 14
>> + *        max_src_w = 4096
>> + *        max_src_h = 8192
>> + *        max_dst_w = 8192
>> + *        max_dst_h = 8192
> 
> I think here could be added comment these numbers are coming from 
> drivers/gpu/drm/i915/display/skl_scaler.c in kernel sources
> 
>> + */
>> +
>> +static void i915_max_source_size_test(data_t *d)
>> +{
>> +    enum pipe pipe = PIPE_A;
>> +    igt_output_t *output;
>> +    igt_fb_t fb;
>> +    igt_plane_t *plane;
>> +    int rval;
>> +
>> +    static const struct invalid_paramtests paramtests[1] = {
> 
> "static const struct invalid_paramtests paramtests[]" would be better 
> here, ie. not declaring here how long is the list of tests
> 
>> +        {
>> +            .testname = "max-src-size",
>> +            .planesize = {3840, 2400},
>> +            .params = {{~0}}
>> +        },
>> +    };
>> +
>> +    igt_fixture {
>> +        output = igt_get_single_output_for_pipe(&d->display, pipe);
>> +        igt_require(output);
>> +
>> +        igt_output_set_pipe(output, pipe);
>> +        plane = igt_output_get_plane_type(output, 
>> DRM_PLANE_TYPE_PRIMARY);
>> +
>> +        igt_create_fb(d->drm_fd, 5120, 4320,
>> +            DRM_FORMAT_XRGB8888,
>> +            DRM_FORMAT_MOD_NONE,
>> +            &fb);
> 
> above indentation is off on those lines with parameters.
> 
>> +    }
>> +
>> +    igt_describe("test for validating max source size.");
>> +    igt_subtest_with_dynamic("i915-max-source-size") {
>> +        for (uint32_t i = 0; i < ARRAY_SIZE(paramtests); i++) {
>> +            igt_dynamic(paramtests[i].testname) {
>> +                igt_plane_set_position(plane, 0, 0);
>> +                igt_plane_set_fb(plane, &fb);
>> +                igt_plane_set_size(plane,
>> +                            paramtests[i].planesize[0],
>> +                            paramtests[i].planesize[1]);
>> +
>> +                for (uint32_t j = 0; paramtests[i].params[j].prop != 
>> ~0; j++)
>> +                    igt_plane_set_prop_value(plane,
>> +                                 paramtests[i].params[j].prop,
>> +                                 paramtests[i].params[j].value);
>> +
>> +                rval = igt_display_try_commit2(&d->display, 
>> COMMIT_ATOMIC);
>> +
>> +                if (intel_display_ver(d->devid) < 11 || 
>> intel_display_ver(d->devid) >= 14)
>> +                    igt_assert(rval == -EINVAL || rval == -ERANGE);
>> +                else
>> +                    igt_assert_eq(rval, 0);
>> +            }
>> +        }
>> +    }
>> +
>> +    igt_fixture {
>> +        igt_remove_fb(d->drm_fd, &fb);
>> +        igt_output_set_pipe(output, PIPE_NONE);
>> +    }
>> +}
>> +
>>   static int opt_handler(int opt, int opt_index, void *_data)
>>   {
>>       data_t *data = _data;
>> @@ -1055,6 +1144,11 @@ igt_main_args("", long_opts, help_str, 
>> opt_handler, &data)
>>       invalid_parameter_tests(&data);
>> +    igt_fixture
>> +        igt_require_f(data.devid, "This test is valid only for i915 
>> devices.\n");
>> +
>> +    i915_max_source_size_test(&data);
>> +
> 
> These above changes I think could be wrapped inside igt_subtest_group so 
> that i915 requirement doesn't become inherited if someone come add 
> something here after this test.

Done. Sent new series with the fixes 
https://patchwork.freedesktop.org/series/114059/

> 
>>       igt_fixture {
>>           igt_display_fini(&data.display);
>>           close(data.drm_fd);
> 
-- 
~Swati Sharma

Re: [igt-dev] [PATCH i-g-t 3/3] tests/kms_plane_scaling: Add test to validate max source size

[Swati Sharma]

Hi Kamil,

I have floated next rev https://patchwork.freedesktop.org/series/114059/
addressing review comments from JP.

igt_require() inside dynamic_subtest() is not recommended.
All these conditions should be checked before executing the
test. In the past, we have seen CRASHES if such conditions
are put inside the subtest.

On 17-Feb-23 12:59 AM, Kamil Konieczny wrote:
> Hi Swati,
> 
> On 2023-02-15 at 22:29:47 +0530, Swati Sharma wrote:
>> i915 specific test is added to validate max source size. This
>> test is expected to return -EINVAL for platforms where we have
>> max source width 4096 whereas it will pass on platforms having
>> max source width 5120. We have created fb of size 5120x4320
>> (=size of source) and downscaled to 3840x2400(=size of dest).
>>
>> On MTL(disp_ver=14), in dmesg we can see
>> "[drm:skl_update_scaler [i915]] scaler_user index 0.0: src
>> 5120x4320 dst 3840x2400 size is out of scaler range."
>> since max source width supported is 4096.
>> whereas same test will pass on ADLP(display_ver=13) since max
>> source width  supported is 5120.
>>
>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> 
> +cc Juha-Pekka
> 
>> ---
>>   tests/kms_plane_scaling.c | 94 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 94 insertions(+)
>>
>> diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
>> index 9737d8645..ae917b202 100644
>> --- a/tests/kms_plane_scaling.c
>> +++ b/tests/kms_plane_scaling.c
>> @@ -890,6 +890,95 @@ static void invalid_parameter_tests(data_t *d)
>>   	}
>>   }
>>   
>> +/*
>> + *	Max source/destination width/height
>> + *	for i915 driver.
>> + *
>> + *	DISPLAY_VER < 11
>> + *		max_src_w = 4096
>> + *		max_src_h = 4096
>> + *		max_dst_w = 4096
>> + *		max_dst_h = 4096
>> + *
>> + *	DISPLAY_VER = 11
>> + *		max_src_w = 5120
>> + *		max_src_h = 4096
>> + *		max_dst_w = 5120
>> + *		max_dst_h = 4096
>> + *
>> + *	DISPLAY_VER = 12-13
>> + *		max_src_w = 5120
>> + *		max_src_h = 8192
>> + *		max_dst_w = 8192
>> + *		max_dst_h = 8192
>> + *
>> + *	DISPLAY_VER = 14
>> + *		max_src_w = 4096
>> + *		max_src_h = 8192
>> + *		max_dst_w = 8192
>> + *		max_dst_h = 8192
>> + */
>> +
>> +static void i915_max_source_size_test(data_t *d)
>> +{
>> +	enum pipe pipe = PIPE_A;
>> +	igt_output_t *output;
>> +	igt_fb_t fb;
>> +	igt_plane_t *plane;
>> +	int rval;
>> +
>> +	static const struct invalid_paramtests paramtests[1] = {
>> +		{
>> +			.testname = "max-src-size",
>> +			.planesize = {3840, 2400},
>> +			.params = {{~0}}
>> +		},
>> +	};
>> +
>> +	igt_fixture {
>> +		output = igt_get_single_output_for_pipe(&d->display, pipe);
>> +		igt_require(output);
> --------------- ^
> You are not in subtest.
> 
>> +
>> +		igt_output_set_pipe(output, pipe);
>> +		plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
>> +
>> +		igt_create_fb(d->drm_fd, 5120, 4320,
>> +			DRM_FORMAT_XRGB8888,
>> +			DRM_FORMAT_MOD_NONE,
>> +			&fb);
>> +	}
> 
> Is this fixture only for i915 ? It looks generic, even if
> igt_create_fb will fail (but you need to remember fb_id here).
> 
>> +
>> +	igt_describe("test for validating max source size.");
>> +	igt_subtest_with_dynamic("i915-max-source-size") {
> 
> Here you can put igt_require for i915 driver and also
> require for fb_id != 0.
> 
>> +		for (uint32_t i = 0; i < ARRAY_SIZE(paramtests); i++) {
>> +			igt_dynamic(paramtests[i].testname) {
>> +				igt_plane_set_position(plane, 0, 0);
>> +				igt_plane_set_fb(plane, &fb);
>> +				igt_plane_set_size(plane,
>> +							paramtests[i].planesize[0],
>> +							paramtests[i].planesize[1]);
>> +
>> +				for (uint32_t j = 0; paramtests[i].params[j].prop != ~0; j++)
>> +					igt_plane_set_prop_value(plane,
>> +								 paramtests[i].params[j].prop,
>> +								 paramtests[i].params[j].value);
>> +
>> +				rval = igt_display_try_commit2(&d->display, COMMIT_ATOMIC);
>> +
>> +				if (intel_display_ver(d->devid) < 11 || intel_display_ver(d->devid) >= 14)
>> +					igt_assert(rval == -EINVAL || rval == -ERANGE);
>> +				else
>> +					igt_assert_eq(rval, 0);
>> +			}
>> +		}
>> +	}
>> +
>> +	igt_fixture {
>> +		igt_remove_fb(d->drm_fd, &fb);
>> +		igt_output_set_pipe(output, PIPE_NONE);
>> +	}
>> +}
>> +
>>   static int opt_handler(int opt, int opt_index, void *_data)
>>   {
>>   	data_t *data = _data;
>> @@ -1055,6 +1144,11 @@ igt_main_args("", long_opts, help_str, opt_handler, &data)
>>   
>>   	invalid_parameter_tests(&data);
>>   
>> +	igt_fixture
>> +		igt_require_f(data.devid, "This test is valid only for i915 devices.\n");
> 
> Do not do this, use igt_require_f inside (dynamic_)subtest.
> 
> Regards,
> Kamil
> 
>> +
>> +	i915_max_source_size_test(&data);
>> +
>>   	igt_fixture {
>>   		igt_display_fini(&data.display);
>>   		close(data.drm_fd);
>> -- 
>> 2.25.1
>>

-- 
~Swati Sharma