* [PATCH i-g-t v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases
@ 2018-07-23 18:25 ` Radhakrishna Sripada
0 siblings, 0 replies; 14+ messages in thread
From: Radhakrishna Sripada @ 2018-07-23 18:25 UTC (permalink / raw)
To: igt-dev; +Cc: intel-gfx, Daniel Vetter
From: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cleanup the testcases by moving the platform checks to a single function.
The earlier version of the path is posted here [1]
v2: Make use of the property enums to get the supported rotations
v3: Move hardcodings to a single function(Ville)
v4: Include the cherryview exception for reflect subtest(Maarten)
v5: Rebase and move the check from CNL to ICL for reflect-x case
v6: Fix the CI regression
v7: rebase
[1]: https://patchwork.freedesktop.org/patch/209647/
Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Mika Kahola <mika.kahola@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
---
tests/kms_rotation_crc.c | 35 ++++++++++++++++++++---------------
1 file changed, 20 insertions(+), 15 deletions(-)
diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
index 6cb5858adb0f..f20b8a6d4ba1 100644
--- a/tests/kms_rotation_crc.c
+++ b/tests/kms_rotation_crc.c
@@ -43,6 +43,7 @@ typedef struct {
uint32_t override_fmt;
uint64_t override_tiling;
int devid;
+ int gen;
} data_t;
typedef struct {
@@ -284,6 +285,17 @@ static void prepare_fbs(data_t *data, igt_output_t *output,
igt_plane_set_position(plane, data->pos_x, data->pos_y);
}
+static void igt_check_rotation(data_t *data)
+{
+ if (data->rotation & (IGT_ROTATION_90 | IGT_ROTATION_270))
+ igt_require(data->gen >= 9);
+ if (data->rotation & IGT_REFLECT_X)
+ igt_require(data->gen >= 11 ||
+ (IS_CHERRYVIEW(data->devid) && (data->rotation & IGT_ROTATION_0)));
+ if (data->rotation & IGT_ROTATION_180)
+ igt_require(data->gen >= 4);
+}
+
static void test_single_case(data_t *data, enum pipe pipe,
igt_output_t *output, igt_plane_t *plane,
enum rectangle_type rect,
@@ -352,15 +364,18 @@ static void test_plane_rotation(data_t *data, int plane_type, bool test_bad_form
igt_display_require_output(display);
+ igt_check_rotation(data);
+
for_each_pipe_with_valid_output(display, pipe, output) {
igt_plane_t *plane;
int i, j;
- if (IS_CHERRYVIEW(data->devid) && pipe != PIPE_B)
- continue;
-
igt_output_set_pipe(output, pipe);
+ if (IS_CHERRYVIEW(data->devid) && (data->rotation & IGT_REFLECT_X) &&
+ pipe != kmstest_pipe_to_index('B'))
+ continue;
+
plane = igt_output_get_plane_type(output, plane_type);
igt_require(igt_plane_has_prop(plane, IGT_PLANE_ROTATION));
@@ -521,14 +536,13 @@ igt_main
};
data_t data = {};
- int gen = 0;
igt_skip_on_simulation();
igt_fixture {
data.gfx_fd = drm_open_driver_master(DRIVER_INTEL);
data.devid = intel_get_drm_devid(data.gfx_fd);
- gen = intel_gen(data.devid);
+ data.gen = intel_gen(data.devid);
kmstest_set_vt_graphics_mode();
@@ -541,16 +555,12 @@ igt_main
igt_subtest_f("%s-rotation-%s",
plane_test_str(subtest->plane),
rot_test_str(subtest->rot)) {
- igt_require(!(subtest->rot &
- (IGT_ROTATION_90 | IGT_ROTATION_270)) ||
- gen >= 9);
data.rotation = subtest->rot;
test_plane_rotation(&data, subtest->plane, false);
}
}
igt_subtest_f("sprite-rotation-90-pos-100-0") {
- igt_require(gen >= 9);
data.rotation = IGT_ROTATION_90;
data.pos_x = 100,
data.pos_y = 0;
@@ -560,7 +570,6 @@ igt_main
data.pos_y = 0;
igt_subtest_f("bad-pixel-format") {
- igt_require(gen >= 9);
data.rotation = IGT_ROTATION_90;
data.override_fmt = DRM_FORMAT_RGB565;
test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY, true);
@@ -568,7 +577,6 @@ igt_main
data.override_fmt = 0;
igt_subtest_f("bad-tiling") {
- igt_require(gen >= 9);
data.rotation = IGT_ROTATION_90;
data.override_tiling = LOCAL_I915_FORMAT_MOD_X_TILED;
test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY, true);
@@ -579,9 +587,6 @@ igt_main
igt_subtest_f("primary-%s-reflect-x-%s",
tiling_test_str(reflect_x->tiling),
rot_test_str(reflect_x->rot)) {
- igt_require(gen >= 10 ||
- (IS_CHERRYVIEW(data.devid) && reflect_x->rot == IGT_ROTATION_0
- && reflect_x->tiling == LOCAL_I915_FORMAT_MOD_X_TILED));
data.rotation = (IGT_REFLECT_X | reflect_x->rot);
data.override_tiling = reflect_x->tiling;
test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY, false);
@@ -596,7 +601,7 @@ igt_main
enum pipe pipe;
igt_output_t *output;
- igt_require(gen >= 9);
+ igt_require(data.gen >= 9);
igt_display_require_output(&data.display);
for_each_pipe_with_valid_output(&data.display, pipe, output) {
--
2.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [igt-dev] [PATCH i-g-t v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases
@ 2018-07-23 18:25 ` Radhakrishna Sripada
0 siblings, 0 replies; 14+ messages in thread
From: Radhakrishna Sripada @ 2018-07-23 18:25 UTC (permalink / raw)
To: igt-dev; +Cc: Anusha Srivatsa, intel-gfx, Daniel Vetter
From: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cleanup the testcases by moving the platform checks to a single function.
The earlier version of the path is posted here [1]
v2: Make use of the property enums to get the supported rotations
v3: Move hardcodings to a single function(Ville)
v4: Include the cherryview exception for reflect subtest(Maarten)
v5: Rebase and move the check from CNL to ICL for reflect-x case
v6: Fix the CI regression
v7: rebase
[1]: https://patchwork.freedesktop.org/patch/209647/
Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Mika Kahola <mika.kahola@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
---
tests/kms_rotation_crc.c | 35 ++++++++++++++++++++---------------
1 file changed, 20 insertions(+), 15 deletions(-)
diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
index 6cb5858adb0f..f20b8a6d4ba1 100644
--- a/tests/kms_rotation_crc.c
+++ b/tests/kms_rotation_crc.c
@@ -43,6 +43,7 @@ typedef struct {
uint32_t override_fmt;
uint64_t override_tiling;
int devid;
+ int gen;
} data_t;
typedef struct {
@@ -284,6 +285,17 @@ static void prepare_fbs(data_t *data, igt_output_t *output,
igt_plane_set_position(plane, data->pos_x, data->pos_y);
}
+static void igt_check_rotation(data_t *data)
+{
+ if (data->rotation & (IGT_ROTATION_90 | IGT_ROTATION_270))
+ igt_require(data->gen >= 9);
+ if (data->rotation & IGT_REFLECT_X)
+ igt_require(data->gen >= 11 ||
+ (IS_CHERRYVIEW(data->devid) && (data->rotation & IGT_ROTATION_0)));
+ if (data->rotation & IGT_ROTATION_180)
+ igt_require(data->gen >= 4);
+}
+
static void test_single_case(data_t *data, enum pipe pipe,
igt_output_t *output, igt_plane_t *plane,
enum rectangle_type rect,
@@ -352,15 +364,18 @@ static void test_plane_rotation(data_t *data, int plane_type, bool test_bad_form
igt_display_require_output(display);
+ igt_check_rotation(data);
+
for_each_pipe_with_valid_output(display, pipe, output) {
igt_plane_t *plane;
int i, j;
- if (IS_CHERRYVIEW(data->devid) && pipe != PIPE_B)
- continue;
-
igt_output_set_pipe(output, pipe);
+ if (IS_CHERRYVIEW(data->devid) && (data->rotation & IGT_REFLECT_X) &&
+ pipe != kmstest_pipe_to_index('B'))
+ continue;
+
plane = igt_output_get_plane_type(output, plane_type);
igt_require(igt_plane_has_prop(plane, IGT_PLANE_ROTATION));
@@ -521,14 +536,13 @@ igt_main
};
data_t data = {};
- int gen = 0;
igt_skip_on_simulation();
igt_fixture {
data.gfx_fd = drm_open_driver_master(DRIVER_INTEL);
data.devid = intel_get_drm_devid(data.gfx_fd);
- gen = intel_gen(data.devid);
+ data.gen = intel_gen(data.devid);
kmstest_set_vt_graphics_mode();
@@ -541,16 +555,12 @@ igt_main
igt_subtest_f("%s-rotation-%s",
plane_test_str(subtest->plane),
rot_test_str(subtest->rot)) {
- igt_require(!(subtest->rot &
- (IGT_ROTATION_90 | IGT_ROTATION_270)) ||
- gen >= 9);
data.rotation = subtest->rot;
test_plane_rotation(&data, subtest->plane, false);
}
}
igt_subtest_f("sprite-rotation-90-pos-100-0") {
- igt_require(gen >= 9);
data.rotation = IGT_ROTATION_90;
data.pos_x = 100,
data.pos_y = 0;
@@ -560,7 +570,6 @@ igt_main
data.pos_y = 0;
igt_subtest_f("bad-pixel-format") {
- igt_require(gen >= 9);
data.rotation = IGT_ROTATION_90;
data.override_fmt = DRM_FORMAT_RGB565;
test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY, true);
@@ -568,7 +577,6 @@ igt_main
data.override_fmt = 0;
igt_subtest_f("bad-tiling") {
- igt_require(gen >= 9);
data.rotation = IGT_ROTATION_90;
data.override_tiling = LOCAL_I915_FORMAT_MOD_X_TILED;
test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY, true);
@@ -579,9 +587,6 @@ igt_main
igt_subtest_f("primary-%s-reflect-x-%s",
tiling_test_str(reflect_x->tiling),
rot_test_str(reflect_x->rot)) {
- igt_require(gen >= 10 ||
- (IS_CHERRYVIEW(data.devid) && reflect_x->rot == IGT_ROTATION_0
- && reflect_x->tiling == LOCAL_I915_FORMAT_MOD_X_TILED));
data.rotation = (IGT_REFLECT_X | reflect_x->rot);
data.override_tiling = reflect_x->tiling;
test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY, false);
@@ -596,7 +601,7 @@ igt_main
enum pipe pipe;
igt_output_t *output;
- igt_require(gen >= 9);
+ igt_require(data.gen >= 9);
igt_display_require_output(&data.display);
for_each_pipe_with_valid_output(&data.display, pipe, output) {
--
2.9.3
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases (rev5)
2018-07-23 18:25 ` [igt-dev] " Radhakrishna Sripada
(?)
@ 2018-07-23 18:58 ` Patchwork
-1 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2018-07-23 18:58 UTC (permalink / raw)
To: Radhakrishna Sripada; +Cc: igt-dev
== Series Details ==
Series: tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases (rev5)
URL : https://patchwork.freedesktop.org/series/40553/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4521 -> IGTPW_1632 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/40553/revisions/5/mbox/
== Known issues ==
Here are the changes found in IGTPW_1632 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@drv_module_reload@basic-reload:
fi-glk-j4005: PASS -> DMESG-WARN (fdo#106248, fdo#106725)
igt@gem_pwrite@basic:
fi-glk-j4005: PASS -> DMESG-WARN (fdo#105719)
igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
fi-glk-j4005: PASS -> FAIL (fdo#106765)
igt@kms_flip@basic-flip-vs-modeset:
fi-glk-j4005: PASS -> DMESG-WARN (fdo#106000, fdo#106097)
igt@kms_flip@basic-flip-vs-wf_vblank:
fi-glk-j4005: PASS -> FAIL (fdo#100368)
igt@kms_frontbuffer_tracking@basic:
fi-hsw-peppy: PASS -> DMESG-FAIL (fdo#102614, fdo#106103)
==== Possible fixes ====
igt@debugfs_test@read_all_entries:
fi-snb-2520m: INCOMPLETE (fdo#103713) -> PASS
igt@prime_vgem@basic-fence-flip:
fi-ilk-650: FAIL (fdo#104008) -> PASS
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
fdo#105719 https://bugs.freedesktop.org/show_bug.cgi?id=105719
fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
fdo#106103 https://bugs.freedesktop.org/show_bug.cgi?id=106103
fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
fdo#106765 https://bugs.freedesktop.org/show_bug.cgi?id=106765
== Participating hosts (47 -> 43) ==
Additional (1): fi-bsw-kefka
Missing (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u
== Build changes ==
* IGT: IGT_4570 -> IGTPW_1632
CI_DRM_4521: a4ebbd84c682fd30edbde6ac0e48d150d4c5c066 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_1632: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1632/
IGT_4570: 65cdccdc7bcbb791d791aeeeecb784a382110a3c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1632/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 14+ messages in thread
* [igt-dev] ✓ Fi.CI.IGT: success for tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases (rev5)
2018-07-23 18:25 ` [igt-dev] " Radhakrishna Sripada
(?)
(?)
@ 2018-07-23 20:23 ` Patchwork
-1 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2018-07-23 20:23 UTC (permalink / raw)
To: Radhakrishna Sripada; +Cc: igt-dev
== Series Details ==
Series: tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases (rev5)
URL : https://patchwork.freedesktop.org/series/40553/
State : success
== Summary ==
= CI Bug Log - changes from IGT_4570_full -> IGTPW_1632_full =
== Summary - WARNING ==
Minor unknown changes coming with IGTPW_1632_full need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in IGTPW_1632_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://patchwork.freedesktop.org/api/1.0/series/40553/revisions/5/mbox/
== Possible new issues ==
Here are the unknown changes that may have been introduced in IGTPW_1632_full:
=== IGT changes ===
==== Warnings ====
igt@gem_exec_schedule@deep-blt:
shard-kbl: PASS -> SKIP
igt@kms_atomic_interruptible@legacy-pageflip:
shard-snb: SKIP -> PASS +1
igt@pm_rc6_residency@rc6-accuracy:
shard-kbl: SKIP -> PASS +1
== Known issues ==
Here are the changes found in IGTPW_1632_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@gem_exec_schedule@pi-ringfull-vebox:
shard-glk: NOTRUN -> FAIL (fdo#103158)
igt@kms_flip@2x-plain-flip-fb-recreate-interruptible:
shard-hsw: PASS -> FAIL (fdo#100368)
shard-glk: PASS -> FAIL (fdo#100368)
igt@kms_flip@flip-vs-expired-vblank:
shard-glk: PASS -> FAIL (fdo#105189)
igt@kms_setmode@basic:
shard-apl: PASS -> FAIL (fdo#99912)
igt@perf_pmu@busy-idle-check-all-vcs0:
shard-snb: PASS -> INCOMPLETE (fdo#105411)
==== Possible fixes ====
igt@gem_mmap_gtt@coherency:
shard-glk: FAIL (fdo#100587) -> SKIP +1
igt@gem_ppgtt@blt-vs-render-ctxn:
shard-kbl: INCOMPLETE (fdo#103665, fdo#106023) -> PASS
igt@kms_flip@plain-flip-ts-check-interruptible:
shard-glk: FAIL (fdo#100368) -> PASS
igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-pwrite:
shard-glk: FAIL (fdo#103167) -> PASS
igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
shard-snb: FAIL (fdo#103166) -> PASS
igt@kms_setmode@basic:
shard-kbl: FAIL (fdo#99912) -> PASS
igt@perf_pmu@multi-client-vcs1:
shard-snb: INCOMPLETE (fdo#105411) -> SKIP
igt@prime_vgem@coherency-gtt:
shard-apl: FAIL (fdo#100587) -> SKIP +1
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#100587 https://bugs.freedesktop.org/show_bug.cgi?id=100587
fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
fdo#105189 https://bugs.freedesktop.org/show_bug.cgi?id=105189
fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
== Participating hosts (5 -> 5) ==
No changes in participating hosts
== Build changes ==
* IGT: IGT_4570 -> IGTPW_1632
* Linux: CI_DRM_4519 -> CI_DRM_4521
CI_DRM_4519: f14c0ec8fe9acce6fd1be84766f854ab8874eb33 @ git://anongit.freedesktop.org/gfx-ci/linux
CI_DRM_4521: a4ebbd84c682fd30edbde6ac0e48d150d4c5c066 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_1632: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1632/
IGT_4570: 65cdccdc7bcbb791d791aeeeecb784a382110a3c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1632/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases
2018-07-23 18:25 ` [igt-dev] " Radhakrishna Sripada
@ 2018-07-24 2:52 ` Dhinakaran Pandiyan
-1 siblings, 0 replies; 14+ messages in thread
From: Dhinakaran Pandiyan @ 2018-07-24 2:52 UTC (permalink / raw)
To: Radhakrishna Sripada, igt-dev; +Cc: Daniel Vetter, intel-gfx
On Mon, 2018-07-23 at 11:25 -0700, Radhakrishna Sripada wrote:
> From: Anusha Srivatsa <anusha.srivatsa@intel.com>
>
> Cleanup the testcases by moving the platform checks to a single
> function.
>
> The earlier version of the path is posted here [1]
>
> v2: Make use of the property enums to get the supported rotations
> v3: Move hardcodings to a single function(Ville)
> v4: Include the cherryview exception for reflect subtest(Maarten)
> v5: Rebase and move the check from CNL to ICL for reflect-x case
> v6: Fix the CI regression
> v7: rebase
>
> [1]: https://patchwork.freedesktop.org/patch/209647/
>
Oh well, I wrote my comments below and then read this link. Please add
new test requirements in separate patches. Only have the code movement
here.
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Mika Kahola <mika.kahola@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> ---
> tests/kms_rotation_crc.c | 35 ++++++++++++++++++++---------------
> 1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
> index 6cb5858adb0f..f20b8a6d4ba1 100644
> --- a/tests/kms_rotation_crc.c
> +++ b/tests/kms_rotation_crc.c
> @@ -43,6 +43,7 @@ typedef struct {
> uint32_t override_fmt;
> uint64_t override_tiling;
> int devid;
> + int gen;
> } data_t;
>
> typedef struct {
> @@ -284,6 +285,17 @@ static void prepare_fbs(data_t *data,
> igt_output_t *output,
> igt_plane_set_position(plane, data->pos_x, data-
> >pos_y);
> }
>
> +static void igt_check_rotation(data_t *data)
> +{
> + if (data->rotation & (IGT_ROTATION_90 | IGT_ROTATION_270))
> + igt_require(data->gen >= 9);
> + if (data->rotation & IGT_REFLECT_X)
> + igt_require(data->gen >= 11 ||
This check used to be igt_require(gen >= 10
> + (IS_CHERRYVIEW(data->devid) && (data-
> >rotation & IGT_ROTATION_0)));
There was also a check for tiling format
- (IS_CHERRYVIEW(data.devid) &&
reflect_x->rot == IGT_ROTATION_0
- && reflect_x->tiling ==
LOCAL_I915_FORMAT_MOD_X_TILED));
> + if (data->rotation & IGT_ROTATION_180)
> + igt_require(data->gen >= 4);
Doesn't look like this requirement was is in the test earlier.
> +}
> +
> static void test_single_case(data_t *data, enum pipe pipe,
> igt_output_t *output, igt_plane_t
> *plane,
> enum rectangle_type rect,
> @@ -352,15 +364,18 @@ static void test_plane_rotation(data_t *data,
> int plane_type, bool test_bad_form
>
> igt_display_require_output(display);
>
> + igt_check_rotation(data);
> +
> for_each_pipe_with_valid_output(display, pipe, output) {
> igt_plane_t *plane;
> int i, j;
>
> - if (IS_CHERRYVIEW(data->devid) && pipe != PIPE_B)
> - continue;
> -
> igt_output_set_pipe(output, pipe);
>
> + if (IS_CHERRYVIEW(data->devid) && (data->rotation &
> IGT_REFLECT_X) &&
> + pipe != kmstest_pipe_to_index('B'))
> + continue;
> +
Why do this?
> plane = igt_output_get_plane_type(output,
> plane_type);
> igt_require(igt_plane_has_prop(plane,
> IGT_PLANE_ROTATION));
>
> @@ -521,14 +536,13 @@ igt_main
> };
>
> data_t data = {};
> - int gen = 0;
>
> igt_skip_on_simulation();
>
> igt_fixture {
> data.gfx_fd = drm_open_driver_master(DRIVER_INTEL);
> data.devid = intel_get_drm_devid(data.gfx_fd);
> - gen = intel_gen(data.devid);
> + data.gen = intel_gen(data.devid);
>
> kmstest_set_vt_graphics_mode();
>
> @@ -541,16 +555,12 @@ igt_main
> igt_subtest_f("%s-rotation-%s",
> plane_test_str(subtest->plane),
> rot_test_str(subtest->rot)) {
> - igt_require(!(subtest->rot &
> - (IGT_ROTATION_90 |
> IGT_ROTATION_270)) ||
> - gen >= 9);
> data.rotation = subtest->rot;
> test_plane_rotation(&data, subtest->plane,
> false);
> }
> }
>
> igt_subtest_f("sprite-rotation-90-pos-100-0") {
> - igt_require(gen >= 9);
> data.rotation = IGT_ROTATION_90;
> data.pos_x = 100,
> data.pos_y = 0;
> @@ -560,7 +570,6 @@ igt_main
> data.pos_y = 0;
>
> igt_subtest_f("bad-pixel-format") {
> - igt_require(gen >= 9);
> data.rotation = IGT_ROTATION_90;
> data.override_fmt = DRM_FORMAT_RGB565;
> test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY,
> true);
> @@ -568,7 +577,6 @@ igt_main
> data.override_fmt = 0;
>
> igt_subtest_f("bad-tiling") {
> - igt_require(gen >= 9);
> data.rotation = IGT_ROTATION_90;
> data.override_tiling =
> LOCAL_I915_FORMAT_MOD_X_TILED;
> test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY,
> true);
> @@ -579,9 +587,6 @@ igt_main
> igt_subtest_f("primary-%s-reflect-x-%s",
> tiling_test_str(reflect_x->tiling),
> rot_test_str(reflect_x->rot)) {
> - igt_require(gen >= 10 ||
> - (IS_CHERRYVIEW(data.devid) &&
> reflect_x->rot == IGT_ROTATION_0
> - && reflect_x->tiling ==
> LOCAL_I915_FORMAT_MOD_X_TILED));
> data.rotation = (IGT_REFLECT_X | reflect_x-
> >rot);
> data.override_tiling = reflect_x->tiling;
> test_plane_rotation(&data,
> DRM_PLANE_TYPE_PRIMARY, false);
> @@ -596,7 +601,7 @@ igt_main
> enum pipe pipe;
> igt_output_t *output;
>
> - igt_require(gen >= 9);
> + igt_require(data.gen >= 9);
> igt_display_require_output(&data.display);
>
> for_each_pipe_with_valid_output(&data.display, pipe,
> output) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases
@ 2018-07-24 2:52 ` Dhinakaran Pandiyan
0 siblings, 0 replies; 14+ messages in thread
From: Dhinakaran Pandiyan @ 2018-07-24 2:52 UTC (permalink / raw)
To: Radhakrishna Sripada, igt-dev; +Cc: Daniel Vetter, intel-gfx
On Mon, 2018-07-23 at 11:25 -0700, Radhakrishna Sripada wrote:
> From: Anusha Srivatsa <anusha.srivatsa@intel.com>
>
> Cleanup the testcases by moving the platform checks to a single
> function.
>
> The earlier version of the path is posted here [1]
>
> v2: Make use of the property enums to get the supported rotations
> v3: Move hardcodings to a single function(Ville)
> v4: Include the cherryview exception for reflect subtest(Maarten)
> v5: Rebase and move the check from CNL to ICL for reflect-x case
> v6: Fix the CI regression
> v7: rebase
>
> [1]: https://patchwork.freedesktop.org/patch/209647/
>
Oh well, I wrote my comments below and then read this link. Please add
new test requirements in separate patches. Only have the code movement
here.
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Mika Kahola <mika.kahola@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> ---
> tests/kms_rotation_crc.c | 35 ++++++++++++++++++++---------------
> 1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
> index 6cb5858adb0f..f20b8a6d4ba1 100644
> --- a/tests/kms_rotation_crc.c
> +++ b/tests/kms_rotation_crc.c
> @@ -43,6 +43,7 @@ typedef struct {
> uint32_t override_fmt;
> uint64_t override_tiling;
> int devid;
> + int gen;
> } data_t;
>
> typedef struct {
> @@ -284,6 +285,17 @@ static void prepare_fbs(data_t *data,
> igt_output_t *output,
> igt_plane_set_position(plane, data->pos_x, data-
> >pos_y);
> }
>
> +static void igt_check_rotation(data_t *data)
> +{
> + if (data->rotation & (IGT_ROTATION_90 | IGT_ROTATION_270))
> + igt_require(data->gen >= 9);
> + if (data->rotation & IGT_REFLECT_X)
> + igt_require(data->gen >= 11 ||
This check used to be igt_require(gen >= 10
> + (IS_CHERRYVIEW(data->devid) && (data-
> >rotation & IGT_ROTATION_0)));
There was also a check for tiling format
- (IS_CHERRYVIEW(data.devid) &&
reflect_x->rot == IGT_ROTATION_0
- && reflect_x->tiling ==
LOCAL_I915_FORMAT_MOD_X_TILED));
> + if (data->rotation & IGT_ROTATION_180)
> + igt_require(data->gen >= 4);
Doesn't look like this requirement was is in the test earlier.
> +}
> +
> static void test_single_case(data_t *data, enum pipe pipe,
> igt_output_t *output, igt_plane_t
> *plane,
> enum rectangle_type rect,
> @@ -352,15 +364,18 @@ static void test_plane_rotation(data_t *data,
> int plane_type, bool test_bad_form
>
> igt_display_require_output(display);
>
> + igt_check_rotation(data);
> +
> for_each_pipe_with_valid_output(display, pipe, output) {
> igt_plane_t *plane;
> int i, j;
>
> - if (IS_CHERRYVIEW(data->devid) && pipe != PIPE_B)
> - continue;
> -
> igt_output_set_pipe(output, pipe);
>
> + if (IS_CHERRYVIEW(data->devid) && (data->rotation &
> IGT_REFLECT_X) &&
> + pipe != kmstest_pipe_to_index('B'))
> + continue;
> +
Why do this?
> plane = igt_output_get_plane_type(output,
> plane_type);
> igt_require(igt_plane_has_prop(plane,
> IGT_PLANE_ROTATION));
>
> @@ -521,14 +536,13 @@ igt_main
> };
>
> data_t data = {};
> - int gen = 0;
>
> igt_skip_on_simulation();
>
> igt_fixture {
> data.gfx_fd = drm_open_driver_master(DRIVER_INTEL);
> data.devid = intel_get_drm_devid(data.gfx_fd);
> - gen = intel_gen(data.devid);
> + data.gen = intel_gen(data.devid);
>
> kmstest_set_vt_graphics_mode();
>
> @@ -541,16 +555,12 @@ igt_main
> igt_subtest_f("%s-rotation-%s",
> plane_test_str(subtest->plane),
> rot_test_str(subtest->rot)) {
> - igt_require(!(subtest->rot &
> - (IGT_ROTATION_90 |
> IGT_ROTATION_270)) ||
> - gen >= 9);
> data.rotation = subtest->rot;
> test_plane_rotation(&data, subtest->plane,
> false);
> }
> }
>
> igt_subtest_f("sprite-rotation-90-pos-100-0") {
> - igt_require(gen >= 9);
> data.rotation = IGT_ROTATION_90;
> data.pos_x = 100,
> data.pos_y = 0;
> @@ -560,7 +570,6 @@ igt_main
> data.pos_y = 0;
>
> igt_subtest_f("bad-pixel-format") {
> - igt_require(gen >= 9);
> data.rotation = IGT_ROTATION_90;
> data.override_fmt = DRM_FORMAT_RGB565;
> test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY,
> true);
> @@ -568,7 +577,6 @@ igt_main
> data.override_fmt = 0;
>
> igt_subtest_f("bad-tiling") {
> - igt_require(gen >= 9);
> data.rotation = IGT_ROTATION_90;
> data.override_tiling =
> LOCAL_I915_FORMAT_MOD_X_TILED;
> test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY,
> true);
> @@ -579,9 +587,6 @@ igt_main
> igt_subtest_f("primary-%s-reflect-x-%s",
> tiling_test_str(reflect_x->tiling),
> rot_test_str(reflect_x->rot)) {
> - igt_require(gen >= 10 ||
> - (IS_CHERRYVIEW(data.devid) &&
> reflect_x->rot == IGT_ROTATION_0
> - && reflect_x->tiling ==
> LOCAL_I915_FORMAT_MOD_X_TILED));
> data.rotation = (IGT_REFLECT_X | reflect_x-
> >rot);
> data.override_tiling = reflect_x->tiling;
> test_plane_rotation(&data,
> DRM_PLANE_TYPE_PRIMARY, false);
> @@ -596,7 +601,7 @@ igt_main
> enum pipe pipe;
> igt_output_t *output;
>
> - igt_require(gen >= 9);
> + igt_require(data.gen >= 9);
> igt_display_require_output(&data.display);
>
> for_each_pipe_with_valid_output(&data.display, pipe,
> output) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases
2018-07-24 2:52 ` [Intel-gfx] " Dhinakaran Pandiyan
@ 2018-07-24 3:26 ` Dhinakaran Pandiyan
-1 siblings, 0 replies; 14+ messages in thread
From: Dhinakaran Pandiyan @ 2018-07-24 3:26 UTC (permalink / raw)
To: Radhakrishna Sripada, igt-dev; +Cc: Daniel Vetter, intel-gfx
On Mon, 2018-07-23 at 19:52 -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-07-23 at 11:25 -0700, Radhakrishna Sripada wrote:
> >
> > From: Anusha Srivatsa <anusha.srivatsa@intel.com>
> >
> > Cleanup the testcases by moving the platform checks to a single
> > function.
> >
> > The earlier version of the path is posted here [1]
> >
> > v2: Make use of the property enums to get the supported rotations
> > v3: Move hardcodings to a single function(Ville)
> > v4: Include the cherryview exception for reflect subtest(Maarten)
> > v5: Rebase and move the check from CNL to ICL for reflect-x case
> > v6: Fix the CI regression
> > v7: rebase
> >
> > [1]: https://patchwork.freedesktop.org/patch/209647/
> >
> Oh well, I wrote my comments below and then read this link. Please
> add
> new test requirements in separate patches. Only have the code
> movement
> here.
>
Quoting Ville from [1] above
"Perhaps the best solution would be to make it as generic as possible
by checking the plane supported rotations, while still keeoing the
manual checks for the few exceptions I listed above. Might even be
nice to put the generic stuff into something like
igt_plane_has_rotation(). And maybe the exceptions should be there
as well?"
If I am reading this correctly, this patch should have retained the
plane property checks in addition to exceptions you have added.
-DK
> >
> > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Mika Kahola <mika.kahola@intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com
> > >
> > ---
> > tests/kms_rotation_crc.c | 35 ++++++++++++++++++++---------------
> > 1 file changed, 20 insertions(+), 15 deletions(-)
> >
> > diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
> > index 6cb5858adb0f..f20b8a6d4ba1 100644
> > --- a/tests/kms_rotation_crc.c
> > +++ b/tests/kms_rotation_crc.c
> > @@ -43,6 +43,7 @@ typedef struct {
> > uint32_t override_fmt;
> > uint64_t override_tiling;
> > int devid;
> > + int gen;
> > } data_t;
> >
> > typedef struct {
> > @@ -284,6 +285,17 @@ static void prepare_fbs(data_t *data,
> > igt_output_t *output,
> > igt_plane_set_position(plane, data->pos_x, data-
> > >
> > > pos_y);
> > }
> >
> > +static void igt_check_rotation(data_t *data)
> > +{
> > + if (data->rotation & (IGT_ROTATION_90 | IGT_ROTATION_270))
> > + igt_require(data->gen >= 9);
> > + if (data->rotation & IGT_REFLECT_X)
> > + igt_require(data->gen >= 11 ||
> This check used to be igt_require(gen >= 10
>
> >
> > + (IS_CHERRYVIEW(data->devid) && (data-
> > >
> > > rotation & IGT_ROTATION_0)));
> There was also a check for tiling format
> - (IS_CHERRYVIEW(data.devid) &&
> reflect_x->rot == IGT_ROTATION_0
> - && reflect_x->tiling ==
> LOCAL_I915_FORMAT_MOD_X_TILED));
>
>
> >
> > + if (data->rotation & IGT_ROTATION_180)
> > + igt_require(data->gen >= 4);
> Doesn't look like this requirement was is in the test earlier.
>
> >
> > +}
> > +
> > static void test_single_case(data_t *data, enum pipe pipe,
> > igt_output_t *output, igt_plane_t
> > *plane,
> > enum rectangle_type rect,
> > @@ -352,15 +364,18 @@ static void test_plane_rotation(data_t *data,
> > int plane_type, bool test_bad_form
> >
> > igt_display_require_output(display);
> >
> > + igt_check_rotation(data);
> > +
> > for_each_pipe_with_valid_output(display, pipe, output) {
> > igt_plane_t *plane;
> > int i, j;
> >
> > - if (IS_CHERRYVIEW(data->devid) && pipe != PIPE_B)
> > - continue;
> > -
> > igt_output_set_pipe(output, pipe);
> >
> > + if (IS_CHERRYVIEW(data->devid) && (data->rotation
> > &
> > IGT_REFLECT_X) &&
> > + pipe != kmstest_pipe_to_index('B'))
> > + continue;
> > +
> Why do this?
>
> >
> > plane = igt_output_get_plane_type(output,
> > plane_type);
> > igt_require(igt_plane_has_prop(plane,
> > IGT_PLANE_ROTATION));
> >
> > @@ -521,14 +536,13 @@ igt_main
> > };
> >
> > data_t data = {};
> > - int gen = 0;
> >
> > igt_skip_on_simulation();
> >
> > igt_fixture {
> > data.gfx_fd =
> > drm_open_driver_master(DRIVER_INTEL);
> > data.devid = intel_get_drm_devid(data.gfx_fd);
> > - gen = intel_gen(data.devid);
> > + data.gen = intel_gen(data.devid);
> >
> > kmstest_set_vt_graphics_mode();
> >
> > @@ -541,16 +555,12 @@ igt_main
> > igt_subtest_f("%s-rotation-%s",
> > plane_test_str(subtest->plane),
> > rot_test_str(subtest->rot)) {
> > - igt_require(!(subtest->rot &
> > - (IGT_ROTATION_90 |
> > IGT_ROTATION_270)) ||
> > - gen >= 9);
> > data.rotation = subtest->rot;
> > test_plane_rotation(&data, subtest->plane,
> > false);
> > }
> > }
> >
> > igt_subtest_f("sprite-rotation-90-pos-100-0") {
> > - igt_require(gen >= 9);
> > data.rotation = IGT_ROTATION_90;
> > data.pos_x = 100,
> > data.pos_y = 0;
> > @@ -560,7 +570,6 @@ igt_main
> > data.pos_y = 0;
> >
> > igt_subtest_f("bad-pixel-format") {
> > - igt_require(gen >= 9);
> > data.rotation = IGT_ROTATION_90;
> > data.override_fmt = DRM_FORMAT_RGB565;
> > test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY,
> > true);
> > @@ -568,7 +577,6 @@ igt_main
> > data.override_fmt = 0;
> >
> > igt_subtest_f("bad-tiling") {
> > - igt_require(gen >= 9);
> > data.rotation = IGT_ROTATION_90;
> > data.override_tiling =
> > LOCAL_I915_FORMAT_MOD_X_TILED;
> > test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY,
> > true);
> > @@ -579,9 +587,6 @@ igt_main
> > igt_subtest_f("primary-%s-reflect-x-%s",
> > tiling_test_str(reflect_x->tiling),
> > rot_test_str(reflect_x->rot)) {
> > - igt_require(gen >= 10 ||
> > - (IS_CHERRYVIEW(data.devid) &&
> > reflect_x->rot == IGT_ROTATION_0
> > - && reflect_x->tiling ==
> > LOCAL_I915_FORMAT_MOD_X_TILED));
> > data.rotation = (IGT_REFLECT_X |
> > reflect_x-
> > >
> > > rot);
> > data.override_tiling = reflect_x->tiling;
> > test_plane_rotation(&data,
> > DRM_PLANE_TYPE_PRIMARY, false);
> > @@ -596,7 +601,7 @@ igt_main
> > enum pipe pipe;
> > igt_output_t *output;
> >
> > - igt_require(gen >= 9);
> > + igt_require(data.gen >= 9);
> > igt_display_require_output(&data.display);
> >
> > for_each_pipe_with_valid_output(&data.display,
> > pipe,
> > output) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases
@ 2018-07-24 3:26 ` Dhinakaran Pandiyan
0 siblings, 0 replies; 14+ messages in thread
From: Dhinakaran Pandiyan @ 2018-07-24 3:26 UTC (permalink / raw)
To: Radhakrishna Sripada, igt-dev; +Cc: Daniel Vetter, intel-gfx
On Mon, 2018-07-23 at 19:52 -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-07-23 at 11:25 -0700, Radhakrishna Sripada wrote:
> >
> > From: Anusha Srivatsa <anusha.srivatsa@intel.com>
> >
> > Cleanup the testcases by moving the platform checks to a single
> > function.
> >
> > The earlier version of the path is posted here [1]
> >
> > v2: Make use of the property enums to get the supported rotations
> > v3: Move hardcodings to a single function(Ville)
> > v4: Include the cherryview exception for reflect subtest(Maarten)
> > v5: Rebase and move the check from CNL to ICL for reflect-x case
> > v6: Fix the CI regression
> > v7: rebase
> >
> > [1]: https://patchwork.freedesktop.org/patch/209647/
> >
> Oh well, I wrote my comments below and then read this link. Please
> add
> new test requirements in separate patches. Only have the code
> movement
> here.
>
Quoting Ville from [1] above
"Perhaps the best solution would be to make it as generic as possible
by checking the plane supported rotations, while still keeoing the
manual checks for the few exceptions I listed above. Might even be
nice to put the generic stuff into something like
igt_plane_has_rotation(). And maybe the exceptions should be there
as well?"
If I am reading this correctly, this patch should have retained the
plane property checks in addition to exceptions you have added.
-DK
> >
> > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Mika Kahola <mika.kahola@intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com
> > >
> > ---
> > tests/kms_rotation_crc.c | 35 ++++++++++++++++++++---------------
> > 1 file changed, 20 insertions(+), 15 deletions(-)
> >
> > diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
> > index 6cb5858adb0f..f20b8a6d4ba1 100644
> > --- a/tests/kms_rotation_crc.c
> > +++ b/tests/kms_rotation_crc.c
> > @@ -43,6 +43,7 @@ typedef struct {
> > uint32_t override_fmt;
> > uint64_t override_tiling;
> > int devid;
> > + int gen;
> > } data_t;
> >
> > typedef struct {
> > @@ -284,6 +285,17 @@ static void prepare_fbs(data_t *data,
> > igt_output_t *output,
> > igt_plane_set_position(plane, data->pos_x, data-
> > >
> > > pos_y);
> > }
> >
> > +static void igt_check_rotation(data_t *data)
> > +{
> > + if (data->rotation & (IGT_ROTATION_90 | IGT_ROTATION_270))
> > + igt_require(data->gen >= 9);
> > + if (data->rotation & IGT_REFLECT_X)
> > + igt_require(data->gen >= 11 ||
> This check used to be igt_require(gen >= 10
>
> >
> > + (IS_CHERRYVIEW(data->devid) && (data-
> > >
> > > rotation & IGT_ROTATION_0)));
> There was also a check for tiling format
> - (IS_CHERRYVIEW(data.devid) &&
> reflect_x->rot == IGT_ROTATION_0
> - && reflect_x->tiling ==
> LOCAL_I915_FORMAT_MOD_X_TILED));
>
>
> >
> > + if (data->rotation & IGT_ROTATION_180)
> > + igt_require(data->gen >= 4);
> Doesn't look like this requirement was is in the test earlier.
>
> >
> > +}
> > +
> > static void test_single_case(data_t *data, enum pipe pipe,
> > igt_output_t *output, igt_plane_t
> > *plane,
> > enum rectangle_type rect,
> > @@ -352,15 +364,18 @@ static void test_plane_rotation(data_t *data,
> > int plane_type, bool test_bad_form
> >
> > igt_display_require_output(display);
> >
> > + igt_check_rotation(data);
> > +
> > for_each_pipe_with_valid_output(display, pipe, output) {
> > igt_plane_t *plane;
> > int i, j;
> >
> > - if (IS_CHERRYVIEW(data->devid) && pipe != PIPE_B)
> > - continue;
> > -
> > igt_output_set_pipe(output, pipe);
> >
> > + if (IS_CHERRYVIEW(data->devid) && (data->rotation
> > &
> > IGT_REFLECT_X) &&
> > + pipe != kmstest_pipe_to_index('B'))
> > + continue;
> > +
> Why do this?
>
> >
> > plane = igt_output_get_plane_type(output,
> > plane_type);
> > igt_require(igt_plane_has_prop(plane,
> > IGT_PLANE_ROTATION));
> >
> > @@ -521,14 +536,13 @@ igt_main
> > };
> >
> > data_t data = {};
> > - int gen = 0;
> >
> > igt_skip_on_simulation();
> >
> > igt_fixture {
> > data.gfx_fd =
> > drm_open_driver_master(DRIVER_INTEL);
> > data.devid = intel_get_drm_devid(data.gfx_fd);
> > - gen = intel_gen(data.devid);
> > + data.gen = intel_gen(data.devid);
> >
> > kmstest_set_vt_graphics_mode();
> >
> > @@ -541,16 +555,12 @@ igt_main
> > igt_subtest_f("%s-rotation-%s",
> > plane_test_str(subtest->plane),
> > rot_test_str(subtest->rot)) {
> > - igt_require(!(subtest->rot &
> > - (IGT_ROTATION_90 |
> > IGT_ROTATION_270)) ||
> > - gen >= 9);
> > data.rotation = subtest->rot;
> > test_plane_rotation(&data, subtest->plane,
> > false);
> > }
> > }
> >
> > igt_subtest_f("sprite-rotation-90-pos-100-0") {
> > - igt_require(gen >= 9);
> > data.rotation = IGT_ROTATION_90;
> > data.pos_x = 100,
> > data.pos_y = 0;
> > @@ -560,7 +570,6 @@ igt_main
> > data.pos_y = 0;
> >
> > igt_subtest_f("bad-pixel-format") {
> > - igt_require(gen >= 9);
> > data.rotation = IGT_ROTATION_90;
> > data.override_fmt = DRM_FORMAT_RGB565;
> > test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY,
> > true);
> > @@ -568,7 +577,6 @@ igt_main
> > data.override_fmt = 0;
> >
> > igt_subtest_f("bad-tiling") {
> > - igt_require(gen >= 9);
> > data.rotation = IGT_ROTATION_90;
> > data.override_tiling =
> > LOCAL_I915_FORMAT_MOD_X_TILED;
> > test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY,
> > true);
> > @@ -579,9 +587,6 @@ igt_main
> > igt_subtest_f("primary-%s-reflect-x-%s",
> > tiling_test_str(reflect_x->tiling),
> > rot_test_str(reflect_x->rot)) {
> > - igt_require(gen >= 10 ||
> > - (IS_CHERRYVIEW(data.devid) &&
> > reflect_x->rot == IGT_ROTATION_0
> > - && reflect_x->tiling ==
> > LOCAL_I915_FORMAT_MOD_X_TILED));
> > data.rotation = (IGT_REFLECT_X |
> > reflect_x-
> > >
> > > rot);
> > data.override_tiling = reflect_x->tiling;
> > test_plane_rotation(&data,
> > DRM_PLANE_TYPE_PRIMARY, false);
> > @@ -596,7 +601,7 @@ igt_main
> > enum pipe pipe;
> > igt_output_t *output;
> >
> > - igt_require(gen >= 9);
> > + igt_require(data.gen >= 9);
> > igt_display_require_output(&data.display);
> >
> > for_each_pipe_with_valid_output(&data.display,
> > pipe,
> > output) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases
2018-07-24 3:26 ` [Intel-gfx] " Dhinakaran Pandiyan
@ 2018-07-27 20:43 ` Radhakrishna Sripada
-1 siblings, 0 replies; 14+ messages in thread
From: Radhakrishna Sripada @ 2018-07-27 20:43 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: intel-gfx, igt-dev, Daniel Vetter
On Mon, Jul 23, 2018 at 08:26:06PM -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-07-23 at 19:52 -0700, Dhinakaran Pandiyan wrote:
> > On Mon, 2018-07-23 at 11:25 -0700, Radhakrishna Sripada wrote:
> > >
> > > From: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > >
> > > Cleanup the testcases by moving the platform checks to a single
> > > function.
> > >
> > > The earlier version of the path is posted here [1]
> > >
> > > v2: Make use of the property enums to get the supported rotations
> > > v3: Move hardcodings to a single function(Ville)
> > > v4: Include the cherryview exception for reflect subtest(Maarten)
> > > v5: Rebase and move the check from CNL to ICL for reflect-x case
> > > v6: Fix the CI regression
> > > v7: rebase
> > >
> > > [1]: https://patchwork.freedesktop.org/patch/209647/
> > >
> > Oh well, I wrote my comments below and then read this link. Please
> > add
> > new test requirements in separate patches. Only have the code
> > movement
> > here.
> >
>
> Quoting Ville from [1] above
>
> "Perhaps the best solution would be to make it as generic as possible
> by checking the plane supported rotations, while still keeoing the
> manual checks for the few exceptions I listed above. Might even be
> nice to put the generic stuff into something like
> igt_plane_has_rotation(). And maybe the exceptions should be there
> as well?"
>
> If I am reading this correctly, this patch should have retained the
> plane property checks in addition to exceptions you have added.
Based on the feedback received from patch v2 [1], we moved back to using
the platform checks instead of using the connector property.
[1] https://patchwork.freedesktop.org/patch/209647/
>
> -DK
>
> > >
> > > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com
> > > >
> > > ---
> > > tests/kms_rotation_crc.c | 35 ++++++++++++++++++++---------------
> > > 1 file changed, 20 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
> > > index 6cb5858adb0f..f20b8a6d4ba1 100644
> > > --- a/tests/kms_rotation_crc.c
> > > +++ b/tests/kms_rotation_crc.c
> > > @@ -43,6 +43,7 @@ typedef struct {
> > > uint32_t override_fmt;
> > > uint64_t override_tiling;
> > > int devid;
> > > + int gen;
> > > } data_t;
> > >
> > > typedef struct {
> > > @@ -284,6 +285,17 @@ static void prepare_fbs(data_t *data,
> > > igt_output_t *output,
> > > igt_plane_set_position(plane, data->pos_x, data-
> > > >
> > > > pos_y);
> > > }
> > >
> > > +static void igt_check_rotation(data_t *data)
> > > +{
> > > + if (data->rotation & (IGT_ROTATION_90 | IGT_ROTATION_270))
> > > + igt_require(data->gen >= 9);
> > > + if (data->rotation & IGT_REFLECT_X)
> > > + igt_require(data->gen >= 11 ||
> > This check used to be igt_require(gen >= 10
Will move it back to gen10.
> >
> > >
> > > + (IS_CHERRYVIEW(data->devid) && (data-
> > > >
> > > > rotation & IGT_ROTATION_0)));
> > There was also a check for tiling format
> > - (IS_CHERRYVIEW(data.devid) &&
> > reflect_x->rot == IGT_ROTATION_0
> > - && reflect_x->tiling ==
> > LOCAL_I915_FORMAT_MOD_X_TILED));
> >
> >
> > >
> > > + if (data->rotation & IGT_ROTATION_180)
> > > + igt_require(data->gen >= 4);
> > Doesn't look like this requirement was is in the test earlier.
> >
This is the requirement present in the kernel and got included here
on suggestion from here[1].
> > >
> > > +}
> > > +
> > > static void test_single_case(data_t *data, enum pipe pipe,
> > > igt_output_t *output, igt_plane_t
> > > *plane,
> > > enum rectangle_type rect,
> > > @@ -352,15 +364,18 @@ static void test_plane_rotation(data_t *data,
> > > int plane_type, bool test_bad_form
> > >
> > > igt_display_require_output(display);
> > >
> > > + igt_check_rotation(data);
> > > +
> > > for_each_pipe_with_valid_output(display, pipe, output) {
> > > igt_plane_t *plane;
> > > int i, j;
> > >
> > > - if (IS_CHERRYVIEW(data->devid) && pipe != PIPE_B)
> > > - continue;
> > > -
> > > igt_output_set_pipe(output, pipe);
> > >
> > > + if (IS_CHERRYVIEW(data->devid) && (data->rotation
> > > &
> > > IGT_REFLECT_X) &&
> > > + pipe != kmstest_pipe_to_index('B'))
> > > + continue;
> > > +
> > Why do this?
> >
On braswell reflectx is supported only on Pipe B and the test case
needs to be skipped for pipes A,C.
Regards,
Radhhakrishna(RK) Sripada
> > >
> > > plane = igt_output_get_plane_type(output,
> > > plane_type);
> > > igt_require(igt_plane_has_prop(plane,
> > > IGT_PLANE_ROTATION));
> > >
> > > @@ -521,14 +536,13 @@ igt_main
> > > };
> > >
> > > data_t data = {};
> > > - int gen = 0;
> > >
> > > igt_skip_on_simulation();
> > >
> > > igt_fixture {
> > > data.gfx_fd =
> > > drm_open_driver_master(DRIVER_INTEL);
> > > data.devid = intel_get_drm_devid(data.gfx_fd);
> > > - gen = intel_gen(data.devid);
> > > + data.gen = intel_gen(data.devid);
> > >
> > > kmstest_set_vt_graphics_mode();
> > >
> > > @@ -541,16 +555,12 @@ igt_main
> > > igt_subtest_f("%s-rotation-%s",
> > > plane_test_str(subtest->plane),
> > > rot_test_str(subtest->rot)) {
> > > - igt_require(!(subtest->rot &
> > > - (IGT_ROTATION_90 |
> > > IGT_ROTATION_270)) ||
> > > - gen >= 9);
> > > data.rotation = subtest->rot;
> > > test_plane_rotation(&data, subtest->plane,
> > > false);
> > > }
> > > }
> > >
> > > igt_subtest_f("sprite-rotation-90-pos-100-0") {
> > > - igt_require(gen >= 9);
> > > data.rotation = IGT_ROTATION_90;
> > > data.pos_x = 100,
> > > data.pos_y = 0;
> > > @@ -560,7 +570,6 @@ igt_main
> > > data.pos_y = 0;
> > >
> > > igt_subtest_f("bad-pixel-format") {
> > > - igt_require(gen >= 9);
> > > data.rotation = IGT_ROTATION_90;
> > > data.override_fmt = DRM_FORMAT_RGB565;
> > > test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY,
> > > true);
> > > @@ -568,7 +577,6 @@ igt_main
> > > data.override_fmt = 0;
> > >
> > > igt_subtest_f("bad-tiling") {
> > > - igt_require(gen >= 9);
> > > data.rotation = IGT_ROTATION_90;
> > > data.override_tiling =
> > > LOCAL_I915_FORMAT_MOD_X_TILED;
> > > test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY,
> > > true);
> > > @@ -579,9 +587,6 @@ igt_main
> > > igt_subtest_f("primary-%s-reflect-x-%s",
> > > tiling_test_str(reflect_x->tiling),
> > > rot_test_str(reflect_x->rot)) {
> > > - igt_require(gen >= 10 ||
> > > - (IS_CHERRYVIEW(data.devid) &&
> > > reflect_x->rot == IGT_ROTATION_0
> > > - && reflect_x->tiling ==
> > > LOCAL_I915_FORMAT_MOD_X_TILED));
> > > data.rotation = (IGT_REFLECT_X |
> > > reflect_x-
> > > >
> > > > rot);
> > > data.override_tiling = reflect_x->tiling;
> > > test_plane_rotation(&data,
> > > DRM_PLANE_TYPE_PRIMARY, false);
> > > @@ -596,7 +601,7 @@ igt_main
> > > enum pipe pipe;
> > > igt_output_t *output;
> > >
> > > - igt_require(gen >= 9);
> > > + igt_require(data.gen >= 9);
> > > igt_display_require_output(&data.display);
> > >
> > > for_each_pipe_with_valid_output(&data.display,
> > > pipe,
> > > output) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases
@ 2018-07-27 20:43 ` Radhakrishna Sripada
0 siblings, 0 replies; 14+ messages in thread
From: Radhakrishna Sripada @ 2018-07-27 20:43 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: Anusha Srivatsa, intel-gfx, igt-dev, Daniel Vetter
On Mon, Jul 23, 2018 at 08:26:06PM -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-07-23 at 19:52 -0700, Dhinakaran Pandiyan wrote:
> > On Mon, 2018-07-23 at 11:25 -0700, Radhakrishna Sripada wrote:
> > >
> > > From: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > >
> > > Cleanup the testcases by moving the platform checks to a single
> > > function.
> > >
> > > The earlier version of the path is posted here [1]
> > >
> > > v2: Make use of the property enums to get the supported rotations
> > > v3: Move hardcodings to a single function(Ville)
> > > v4: Include the cherryview exception for reflect subtest(Maarten)
> > > v5: Rebase and move the check from CNL to ICL for reflect-x case
> > > v6: Fix the CI regression
> > > v7: rebase
> > >
> > > [1]: https://patchwork.freedesktop.org/patch/209647/
> > >
> > Oh well, I wrote my comments below and then read this link. Please
> > add
> > new test requirements in separate patches. Only have the code
> > movement
> > here.
> >
>
> Quoting Ville from [1] above
>
> "Perhaps the best solution would be to make it as generic as possible
> by checking the plane supported rotations, while still keeoing the
> manual checks for the few exceptions I listed above. Might even be
> nice to put the generic stuff into something like
> igt_plane_has_rotation(). And maybe the exceptions should be there
> as well?"
>
> If I am reading this correctly, this patch should have retained the
> plane property checks in addition to exceptions you have added.
Based on the feedback received from patch v2 [1], we moved back to using
the platform checks instead of using the connector property.
[1] https://patchwork.freedesktop.org/patch/209647/
>
> -DK
>
> > >
> > > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com
> > > >
> > > ---
> > > tests/kms_rotation_crc.c | 35 ++++++++++++++++++++---------------
> > > 1 file changed, 20 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
> > > index 6cb5858adb0f..f20b8a6d4ba1 100644
> > > --- a/tests/kms_rotation_crc.c
> > > +++ b/tests/kms_rotation_crc.c
> > > @@ -43,6 +43,7 @@ typedef struct {
> > > uint32_t override_fmt;
> > > uint64_t override_tiling;
> > > int devid;
> > > + int gen;
> > > } data_t;
> > >
> > > typedef struct {
> > > @@ -284,6 +285,17 @@ static void prepare_fbs(data_t *data,
> > > igt_output_t *output,
> > > igt_plane_set_position(plane, data->pos_x, data-
> > > >
> > > > pos_y);
> > > }
> > >
> > > +static void igt_check_rotation(data_t *data)
> > > +{
> > > + if (data->rotation & (IGT_ROTATION_90 | IGT_ROTATION_270))
> > > + igt_require(data->gen >= 9);
> > > + if (data->rotation & IGT_REFLECT_X)
> > > + igt_require(data->gen >= 11 ||
> > This check used to be igt_require(gen >= 10
Will move it back to gen10.
> >
> > >
> > > + (IS_CHERRYVIEW(data->devid) && (data-
> > > >
> > > > rotation & IGT_ROTATION_0)));
> > There was also a check for tiling format
> > - (IS_CHERRYVIEW(data.devid) &&
> > reflect_x->rot == IGT_ROTATION_0
> > - && reflect_x->tiling ==
> > LOCAL_I915_FORMAT_MOD_X_TILED));
> >
> >
> > >
> > > + if (data->rotation & IGT_ROTATION_180)
> > > + igt_require(data->gen >= 4);
> > Doesn't look like this requirement was is in the test earlier.
> >
This is the requirement present in the kernel and got included here
on suggestion from here[1].
> > >
> > > +}
> > > +
> > > static void test_single_case(data_t *data, enum pipe pipe,
> > > igt_output_t *output, igt_plane_t
> > > *plane,
> > > enum rectangle_type rect,
> > > @@ -352,15 +364,18 @@ static void test_plane_rotation(data_t *data,
> > > int plane_type, bool test_bad_form
> > >
> > > igt_display_require_output(display);
> > >
> > > + igt_check_rotation(data);
> > > +
> > > for_each_pipe_with_valid_output(display, pipe, output) {
> > > igt_plane_t *plane;
> > > int i, j;
> > >
> > > - if (IS_CHERRYVIEW(data->devid) && pipe != PIPE_B)
> > > - continue;
> > > -
> > > igt_output_set_pipe(output, pipe);
> > >
> > > + if (IS_CHERRYVIEW(data->devid) && (data->rotation
> > > &
> > > IGT_REFLECT_X) &&
> > > + pipe != kmstest_pipe_to_index('B'))
> > > + continue;
> > > +
> > Why do this?
> >
On braswell reflectx is supported only on Pipe B and the test case
needs to be skipped for pipes A,C.
Regards,
Radhhakrishna(RK) Sripada
> > >
> > > plane = igt_output_get_plane_type(output,
> > > plane_type);
> > > igt_require(igt_plane_has_prop(plane,
> > > IGT_PLANE_ROTATION));
> > >
> > > @@ -521,14 +536,13 @@ igt_main
> > > };
> > >
> > > data_t data = {};
> > > - int gen = 0;
> > >
> > > igt_skip_on_simulation();
> > >
> > > igt_fixture {
> > > data.gfx_fd =
> > > drm_open_driver_master(DRIVER_INTEL);
> > > data.devid = intel_get_drm_devid(data.gfx_fd);
> > > - gen = intel_gen(data.devid);
> > > + data.gen = intel_gen(data.devid);
> > >
> > > kmstest_set_vt_graphics_mode();
> > >
> > > @@ -541,16 +555,12 @@ igt_main
> > > igt_subtest_f("%s-rotation-%s",
> > > plane_test_str(subtest->plane),
> > > rot_test_str(subtest->rot)) {
> > > - igt_require(!(subtest->rot &
> > > - (IGT_ROTATION_90 |
> > > IGT_ROTATION_270)) ||
> > > - gen >= 9);
> > > data.rotation = subtest->rot;
> > > test_plane_rotation(&data, subtest->plane,
> > > false);
> > > }
> > > }
> > >
> > > igt_subtest_f("sprite-rotation-90-pos-100-0") {
> > > - igt_require(gen >= 9);
> > > data.rotation = IGT_ROTATION_90;
> > > data.pos_x = 100,
> > > data.pos_y = 0;
> > > @@ -560,7 +570,6 @@ igt_main
> > > data.pos_y = 0;
> > >
> > > igt_subtest_f("bad-pixel-format") {
> > > - igt_require(gen >= 9);
> > > data.rotation = IGT_ROTATION_90;
> > > data.override_fmt = DRM_FORMAT_RGB565;
> > > test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY,
> > > true);
> > > @@ -568,7 +577,6 @@ igt_main
> > > data.override_fmt = 0;
> > >
> > > igt_subtest_f("bad-tiling") {
> > > - igt_require(gen >= 9);
> > > data.rotation = IGT_ROTATION_90;
> > > data.override_tiling =
> > > LOCAL_I915_FORMAT_MOD_X_TILED;
> > > test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY,
> > > true);
> > > @@ -579,9 +587,6 @@ igt_main
> > > igt_subtest_f("primary-%s-reflect-x-%s",
> > > tiling_test_str(reflect_x->tiling),
> > > rot_test_str(reflect_x->rot)) {
> > > - igt_require(gen >= 10 ||
> > > - (IS_CHERRYVIEW(data.devid) &&
> > > reflect_x->rot == IGT_ROTATION_0
> > > - && reflect_x->tiling ==
> > > LOCAL_I915_FORMAT_MOD_X_TILED));
> > > data.rotation = (IGT_REFLECT_X |
> > > reflect_x-
> > > >
> > > > rot);
> > > data.override_tiling = reflect_x->tiling;
> > > test_plane_rotation(&data,
> > > DRM_PLANE_TYPE_PRIMARY, false);
> > > @@ -596,7 +601,7 @@ igt_main
> > > enum pipe pipe;
> > > igt_output_t *output;
> > >
> > > - igt_require(gen >= 9);
> > > + igt_require(data.gen >= 9);
> > > igt_display_require_output(&data.display);
> > >
> > > for_each_pipe_with_valid_output(&data.display,
> > > pipe,
> > > output) {
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases
2018-07-27 20:43 ` Radhakrishna Sripada
@ 2018-07-27 22:06 ` Dhinakaran Pandiyan
-1 siblings, 0 replies; 14+ messages in thread
From: Dhinakaran Pandiyan @ 2018-07-27 22:06 UTC (permalink / raw)
To: Radhakrishna Sripada; +Cc: intel-gfx, igt-dev, Daniel Vetter
On Fri, 2018-07-27 at 13:43 -0700, Radhakrishna Sripada wrote:
> On Mon, Jul 23, 2018 at 08:26:06PM -0700, Dhinakaran Pandiyan wrote:
> >
> > On Mon, 2018-07-23 at 19:52 -0700, Dhinakaran Pandiyan wrote:
> > >
> > > On Mon, 2018-07-23 at 11:25 -0700, Radhakrishna Sripada wrote:
> > > >
> > > >
> > > > From: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > >
> > > > Cleanup the testcases by moving the platform checks to a single
> > > > function.
> > > >
> > > > The earlier version of the path is posted here [1]
> > > >
> > > > v2: Make use of the property enums to get the supported
> > > > rotations
> > > > v3: Move hardcodings to a single function(Ville)
> > > > v4: Include the cherryview exception for reflect
> > > > subtest(Maarten)
> > > > v5: Rebase and move the check from CNL to ICL for reflect-x
> > > > case
> > > > v6: Fix the CI regression
> > > > v7: rebase
> > > >
> > > > [1]: https://patchwork.freedesktop.org/patch/209647/
> > > >
> > > Oh well, I wrote my comments below and then read this link.
> > > Please
> > > add
> > > new test requirements in separate patches. Only have the code
> > > movement
> > > here.
> > >
> > Quoting Ville from [1] above
> >
> > "Perhaps the best solution would be to make it as generic as
> > possible
> > by checking the plane supported rotations, while still keeoing the
> > manual checks for the few exceptions I listed above. Might even be
> > nice to put the generic stuff into something like
> > igt_plane_has_rotation(). And maybe the exceptions should be there
> > as well?"
> >
> > If I am reading this correctly, this patch should have retained the
> > plane property checks in addition to exceptions you have added.
> Based on the feedback received from patch v2 [1], we moved back to
> using
> the platform checks instead of using the connector property.
>
Okay, I am not reading the comment, specifically "make it as generic as
possible by checking the plane supported rotations", the way you are.
Check with Ville.
Anyway, if you want to add new platform checks, I do not recommend
adding all of them in a single patch.
>
> [1] https://patchwork.freedesktop.org/patch/209647/
> >
> >
> > -DK
> >
> > >
> > > >
> > > >
> > > > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel
> > > > .com
> > > > >
> > > > >
> > > > ---
> > > > tests/kms_rotation_crc.c | 35 ++++++++++++++++++++----------
> > > > -----
> > > > 1 file changed, 20 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/tests/kms_rotation_crc.c
> > > > b/tests/kms_rotation_crc.c
> > > > index 6cb5858adb0f..f20b8a6d4ba1 100644
> > > > --- a/tests/kms_rotation_crc.c
> > > > +++ b/tests/kms_rotation_crc.c
> > > > @@ -43,6 +43,7 @@ typedef struct {
> > > > uint32_t override_fmt;
> > > > uint64_t override_tiling;
> > > > int devid;
> > > > + int gen;
> > > > } data_t;
> > > >
> > > > typedef struct {
> > > > @@ -284,6 +285,17 @@ static void prepare_fbs(data_t *data,
> > > > igt_output_t *output,
> > > > igt_plane_set_position(plane, data->pos_x,
> > > > data-
> > > > >
> > > > >
> > > > > pos_y);
> > > > }
> > > >
> > > > +static void igt_check_rotation(data_t *data)
> > > > +{
> > > > + if (data->rotation & (IGT_ROTATION_90 |
> > > > IGT_ROTATION_270))
> > > > + igt_require(data->gen >= 9);
> > > > + if (data->rotation & IGT_REFLECT_X)
> > > > + igt_require(data->gen >= 11 ||
> > > This check used to be igt_require(gen >= 10
> Will move it back to gen10.
> >
> > >
> > >
> > > >
> > > >
> > > > + (IS_CHERRYVIEW(data->devid) &&
> > > > (data-
> > > > >
> > > > >
> > > > > rotation & IGT_ROTATION_0)));
> > > There was also a check for tiling format
> > > - (IS_CHERRYVIEW(data.devid) &&
> > > reflect_x->rot == IGT_ROTATION_0
> > > - && reflect_x->tiling ==
> > > LOCAL_I915_FORMAT_MOD_X_TILED));
> > >
> > >
> > > >
> > > >
> > > > + if (data->rotation & IGT_ROTATION_180)
> > > > + igt_require(data->gen >= 4);
> > > Doesn't look like this requirement was is in the test earlier.
> > >
> This is the requirement present in the kernel and got included here
> on suggestion from here[1].
>
> >
> > >
> > > >
> > > >
> > > > +}
> > > > +
> > > > static void test_single_case(data_t *data, enum pipe pipe,
> > > > igt_output_t *output, igt_plane_t
> > > > *plane,
> > > > enum rectangle_type rect,
> > > > @@ -352,15 +364,18 @@ static void test_plane_rotation(data_t
> > > > *data,
> > > > int plane_type, bool test_bad_form
> > > >
> > > > igt_display_require_output(display);
> > > >
> > > > + igt_check_rotation(data);
> > > > +
> > > > for_each_pipe_with_valid_output(display, pipe, output)
> > > > {
> > > > igt_plane_t *plane;
> > > > int i, j;
> > > >
> > > > - if (IS_CHERRYVIEW(data->devid) && pipe !=
> > > > PIPE_B)
> > > > - continue;
> > > > -
> > > > igt_output_set_pipe(output, pipe);
> > > >
> > > > + if (IS_CHERRYVIEW(data->devid) && (data-
> > > > >rotation
> > > > &
> > > > IGT_REFLECT_X) &&
> > > > + pipe != kmstest_pipe_to_index('B'))
> > > > + continue;
> > > > +
> > > Why do this?
> > >
> On braswell reflectx is supported only on Pipe B and the test case
> needs to be skipped for pipes A,C.
>
>
> Regards,
> Radhhakrishna(RK) Sripada
> >
> > >
> > > >
> > > >
> > > > plane = igt_output_get_plane_type(output,
> > > > plane_type);
> > > > igt_require(igt_plane_has_prop(plane,
> > > > IGT_PLANE_ROTATION));
> > > >
> > > > @@ -521,14 +536,13 @@ igt_main
> > > > };
> > > >
> > > > data_t data = {};
> > > > - int gen = 0;
> > > >
> > > > igt_skip_on_simulation();
> > > >
> > > > igt_fixture {
> > > > data.gfx_fd =
> > > > drm_open_driver_master(DRIVER_INTEL);
> > > > data.devid = intel_get_drm_devid(data.gfx_fd);
> > > > - gen = intel_gen(data.devid);
> > > > + data.gen = intel_gen(data.devid);
> > > >
> > > > kmstest_set_vt_graphics_mode();
> > > >
> > > > @@ -541,16 +555,12 @@ igt_main
> > > > igt_subtest_f("%s-rotation-%s",
> > > > plane_test_str(subtest->plane),
> > > > rot_test_str(subtest->rot)) {
> > > > - igt_require(!(subtest->rot &
> > > > - (IGT_ROTATION_90 |
> > > > IGT_ROTATION_270)) ||
> > > > - gen >= 9);
> > > > data.rotation = subtest->rot;
> > > > test_plane_rotation(&data, subtest-
> > > > >plane,
> > > > false);
> > > > }
> > > > }
> > > >
> > > > igt_subtest_f("sprite-rotation-90-pos-100-0") {
> > > > - igt_require(gen >= 9);
> > > > data.rotation = IGT_ROTATION_90;
> > > > data.pos_x = 100,
> > > > data.pos_y = 0;
> > > > @@ -560,7 +570,6 @@ igt_main
> > > > data.pos_y = 0;
> > > >
> > > > igt_subtest_f("bad-pixel-format") {
> > > > - igt_require(gen >= 9);
> > > > data.rotation = IGT_ROTATION_90;
> > > > data.override_fmt = DRM_FORMAT_RGB565;
> > > > test_plane_rotation(&data,
> > > > DRM_PLANE_TYPE_PRIMARY,
> > > > true);
> > > > @@ -568,7 +577,6 @@ igt_main
> > > > data.override_fmt = 0;
> > > >
> > > > igt_subtest_f("bad-tiling") {
> > > > - igt_require(gen >= 9);
> > > > data.rotation = IGT_ROTATION_90;
> > > > data.override_tiling =
> > > > LOCAL_I915_FORMAT_MOD_X_TILED;
> > > > test_plane_rotation(&data,
> > > > DRM_PLANE_TYPE_PRIMARY,
> > > > true);
> > > > @@ -579,9 +587,6 @@ igt_main
> > > > igt_subtest_f("primary-%s-reflect-x-%s",
> > > > tiling_test_str(reflect_x-
> > > > >tiling),
> > > > rot_test_str(reflect_x->rot)) {
> > > > - igt_require(gen >= 10 ||
> > > > - (IS_CHERRYVIEW(data.devid)
> > > > &&
> > > > reflect_x->rot == IGT_ROTATION_0
> > > > - && reflect_x->tiling ==
> > > > LOCAL_I915_FORMAT_MOD_X_TILED));
> > > > data.rotation = (IGT_REFLECT_X |
> > > > reflect_x-
> > > > >
> > > > >
> > > > > rot);
> > > > data.override_tiling = reflect_x-
> > > > >tiling;
> > > > test_plane_rotation(&data,
> > > > DRM_PLANE_TYPE_PRIMARY, false);
> > > > @@ -596,7 +601,7 @@ igt_main
> > > > enum pipe pipe;
> > > > igt_output_t *output;
> > > >
> > > > - igt_require(gen >= 9);
> > > > + igt_require(data.gen >= 9);
> > > > igt_display_require_output(&data.display);
> > > >
> > > > for_each_pipe_with_valid_output(&data.display,
> > > > pipe,
> > > > output) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases
@ 2018-07-27 22:06 ` Dhinakaran Pandiyan
0 siblings, 0 replies; 14+ messages in thread
From: Dhinakaran Pandiyan @ 2018-07-27 22:06 UTC (permalink / raw)
To: Radhakrishna Sripada; +Cc: Anusha Srivatsa, intel-gfx, igt-dev, Daniel Vetter
On Fri, 2018-07-27 at 13:43 -0700, Radhakrishna Sripada wrote:
> On Mon, Jul 23, 2018 at 08:26:06PM -0700, Dhinakaran Pandiyan wrote:
> >
> > On Mon, 2018-07-23 at 19:52 -0700, Dhinakaran Pandiyan wrote:
> > >
> > > On Mon, 2018-07-23 at 11:25 -0700, Radhakrishna Sripada wrote:
> > > >
> > > >
> > > > From: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > >
> > > > Cleanup the testcases by moving the platform checks to a single
> > > > function.
> > > >
> > > > The earlier version of the path is posted here [1]
> > > >
> > > > v2: Make use of the property enums to get the supported
> > > > rotations
> > > > v3: Move hardcodings to a single function(Ville)
> > > > v4: Include the cherryview exception for reflect
> > > > subtest(Maarten)
> > > > v5: Rebase and move the check from CNL to ICL for reflect-x
> > > > case
> > > > v6: Fix the CI regression
> > > > v7: rebase
> > > >
> > > > [1]: https://patchwork.freedesktop.org/patch/209647/
> > > >
> > > Oh well, I wrote my comments below and then read this link.
> > > Please
> > > add
> > > new test requirements in separate patches. Only have the code
> > > movement
> > > here.
> > >
> > Quoting Ville from [1] above
> >
> > "Perhaps the best solution would be to make it as generic as
> > possible
> > by checking the plane supported rotations, while still keeoing the
> > manual checks for the few exceptions I listed above. Might even be
> > nice to put the generic stuff into something like
> > igt_plane_has_rotation(). And maybe the exceptions should be there
> > as well?"
> >
> > If I am reading this correctly, this patch should have retained the
> > plane property checks in addition to exceptions you have added.
> Based on the feedback received from patch v2 [1], we moved back to
> using
> the platform checks instead of using the connector property.
>
Okay, I am not reading the comment, specifically "make it as generic as
possible by checking the plane supported rotations", the way you are.
Check with Ville.
Anyway, if you want to add new platform checks, I do not recommend
adding all of them in a single patch.
>
> [1] https://patchwork.freedesktop.org/patch/209647/
> >
> >
> > -DK
> >
> > >
> > > >
> > > >
> > > > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel
> > > > .com
> > > > >
> > > > >
> > > > ---
> > > > tests/kms_rotation_crc.c | 35 ++++++++++++++++++++----------
> > > > -----
> > > > 1 file changed, 20 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/tests/kms_rotation_crc.c
> > > > b/tests/kms_rotation_crc.c
> > > > index 6cb5858adb0f..f20b8a6d4ba1 100644
> > > > --- a/tests/kms_rotation_crc.c
> > > > +++ b/tests/kms_rotation_crc.c
> > > > @@ -43,6 +43,7 @@ typedef struct {
> > > > uint32_t override_fmt;
> > > > uint64_t override_tiling;
> > > > int devid;
> > > > + int gen;
> > > > } data_t;
> > > >
> > > > typedef struct {
> > > > @@ -284,6 +285,17 @@ static void prepare_fbs(data_t *data,
> > > > igt_output_t *output,
> > > > igt_plane_set_position(plane, data->pos_x,
> > > > data-
> > > > >
> > > > >
> > > > > pos_y);
> > > > }
> > > >
> > > > +static void igt_check_rotation(data_t *data)
> > > > +{
> > > > + if (data->rotation & (IGT_ROTATION_90 |
> > > > IGT_ROTATION_270))
> > > > + igt_require(data->gen >= 9);
> > > > + if (data->rotation & IGT_REFLECT_X)
> > > > + igt_require(data->gen >= 11 ||
> > > This check used to be igt_require(gen >= 10
> Will move it back to gen10.
> >
> > >
> > >
> > > >
> > > >
> > > > + (IS_CHERRYVIEW(data->devid) &&
> > > > (data-
> > > > >
> > > > >
> > > > > rotation & IGT_ROTATION_0)));
> > > There was also a check for tiling format
> > > - (IS_CHERRYVIEW(data.devid) &&
> > > reflect_x->rot == IGT_ROTATION_0
> > > - && reflect_x->tiling ==
> > > LOCAL_I915_FORMAT_MOD_X_TILED));
> > >
> > >
> > > >
> > > >
> > > > + if (data->rotation & IGT_ROTATION_180)
> > > > + igt_require(data->gen >= 4);
> > > Doesn't look like this requirement was is in the test earlier.
> > >
> This is the requirement present in the kernel and got included here
> on suggestion from here[1].
>
> >
> > >
> > > >
> > > >
> > > > +}
> > > > +
> > > > static void test_single_case(data_t *data, enum pipe pipe,
> > > > igt_output_t *output, igt_plane_t
> > > > *plane,
> > > > enum rectangle_type rect,
> > > > @@ -352,15 +364,18 @@ static void test_plane_rotation(data_t
> > > > *data,
> > > > int plane_type, bool test_bad_form
> > > >
> > > > igt_display_require_output(display);
> > > >
> > > > + igt_check_rotation(data);
> > > > +
> > > > for_each_pipe_with_valid_output(display, pipe, output)
> > > > {
> > > > igt_plane_t *plane;
> > > > int i, j;
> > > >
> > > > - if (IS_CHERRYVIEW(data->devid) && pipe !=
> > > > PIPE_B)
> > > > - continue;
> > > > -
> > > > igt_output_set_pipe(output, pipe);
> > > >
> > > > + if (IS_CHERRYVIEW(data->devid) && (data-
> > > > >rotation
> > > > &
> > > > IGT_REFLECT_X) &&
> > > > + pipe != kmstest_pipe_to_index('B'))
> > > > + continue;
> > > > +
> > > Why do this?
> > >
> On braswell reflectx is supported only on Pipe B and the test case
> needs to be skipped for pipes A,C.
>
>
> Regards,
> Radhhakrishna(RK) Sripada
> >
> > >
> > > >
> > > >
> > > > plane = igt_output_get_plane_type(output,
> > > > plane_type);
> > > > igt_require(igt_plane_has_prop(plane,
> > > > IGT_PLANE_ROTATION));
> > > >
> > > > @@ -521,14 +536,13 @@ igt_main
> > > > };
> > > >
> > > > data_t data = {};
> > > > - int gen = 0;
> > > >
> > > > igt_skip_on_simulation();
> > > >
> > > > igt_fixture {
> > > > data.gfx_fd =
> > > > drm_open_driver_master(DRIVER_INTEL);
> > > > data.devid = intel_get_drm_devid(data.gfx_fd);
> > > > - gen = intel_gen(data.devid);
> > > > + data.gen = intel_gen(data.devid);
> > > >
> > > > kmstest_set_vt_graphics_mode();
> > > >
> > > > @@ -541,16 +555,12 @@ igt_main
> > > > igt_subtest_f("%s-rotation-%s",
> > > > plane_test_str(subtest->plane),
> > > > rot_test_str(subtest->rot)) {
> > > > - igt_require(!(subtest->rot &
> > > > - (IGT_ROTATION_90 |
> > > > IGT_ROTATION_270)) ||
> > > > - gen >= 9);
> > > > data.rotation = subtest->rot;
> > > > test_plane_rotation(&data, subtest-
> > > > >plane,
> > > > false);
> > > > }
> > > > }
> > > >
> > > > igt_subtest_f("sprite-rotation-90-pos-100-0") {
> > > > - igt_require(gen >= 9);
> > > > data.rotation = IGT_ROTATION_90;
> > > > data.pos_x = 100,
> > > > data.pos_y = 0;
> > > > @@ -560,7 +570,6 @@ igt_main
> > > > data.pos_y = 0;
> > > >
> > > > igt_subtest_f("bad-pixel-format") {
> > > > - igt_require(gen >= 9);
> > > > data.rotation = IGT_ROTATION_90;
> > > > data.override_fmt = DRM_FORMAT_RGB565;
> > > > test_plane_rotation(&data,
> > > > DRM_PLANE_TYPE_PRIMARY,
> > > > true);
> > > > @@ -568,7 +577,6 @@ igt_main
> > > > data.override_fmt = 0;
> > > >
> > > > igt_subtest_f("bad-tiling") {
> > > > - igt_require(gen >= 9);
> > > > data.rotation = IGT_ROTATION_90;
> > > > data.override_tiling =
> > > > LOCAL_I915_FORMAT_MOD_X_TILED;
> > > > test_plane_rotation(&data,
> > > > DRM_PLANE_TYPE_PRIMARY,
> > > > true);
> > > > @@ -579,9 +587,6 @@ igt_main
> > > > igt_subtest_f("primary-%s-reflect-x-%s",
> > > > tiling_test_str(reflect_x-
> > > > >tiling),
> > > > rot_test_str(reflect_x->rot)) {
> > > > - igt_require(gen >= 10 ||
> > > > - (IS_CHERRYVIEW(data.devid)
> > > > &&
> > > > reflect_x->rot == IGT_ROTATION_0
> > > > - && reflect_x->tiling ==
> > > > LOCAL_I915_FORMAT_MOD_X_TILED));
> > > > data.rotation = (IGT_REFLECT_X |
> > > > reflect_x-
> > > > >
> > > > >
> > > > > rot);
> > > > data.override_tiling = reflect_x-
> > > > >tiling;
> > > > test_plane_rotation(&data,
> > > > DRM_PLANE_TYPE_PRIMARY, false);
> > > > @@ -596,7 +601,7 @@ igt_main
> > > > enum pipe pipe;
> > > > igt_output_t *output;
> > > >
> > > > - igt_require(gen >= 9);
> > > > + igt_require(data.gen >= 9);
> > > > igt_display_require_output(&data.display);
> > > >
> > > > for_each_pipe_with_valid_output(&data.display,
> > > > pipe,
> > > > output) {
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases
2018-07-27 22:06 ` Dhinakaran Pandiyan
@ 2018-07-27 22:26 ` Radhakrishna Sripada
-1 siblings, 0 replies; 14+ messages in thread
From: Radhakrishna Sripada @ 2018-07-27 22:26 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: intel-gfx, igt-dev, Daniel Vetter
On Fri, Jul 27, 2018 at 03:06:19PM -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-07-27 at 13:43 -0700, Radhakrishna Sripada wrote:
> > On Mon, Jul 23, 2018 at 08:26:06PM -0700, Dhinakaran Pandiyan wrote:
> > >
> > > On Mon, 2018-07-23 at 19:52 -0700, Dhinakaran Pandiyan wrote:
> > > >
> > > > On Mon, 2018-07-23 at 11:25 -0700, Radhakrishna Sripada wrote:
> > > > >
> > > > >
> > > > > From: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > >
> > > > > Cleanup the testcases by moving the platform checks to a single
> > > > > function.
> > > > >
> > > > > The earlier version of the path is posted here [1]
> > > > >
> > > > > v2: Make use of the property enums to get the supported
> > > > > rotations
> > > > > v3: Move hardcodings to a single function(Ville)
> > > > > v4: Include the cherryview exception for reflect
> > > > > subtest(Maarten)
> > > > > v5: Rebase and move the check from CNL to ICL for reflect-x
> > > > > case
> > > > > v6: Fix the CI regression
> > > > > v7: rebase
> > > > >
> > > > > [1]: https://patchwork.freedesktop.org/patch/209647/
> > > > >
> > > > Oh well, I wrote my comments below and then read this link.
> > > > Please
> > > > add
> > > > new test requirements in separate patches. Only have the code
> > > > movement
> > > > here.
> > > >
> > > Quoting Ville from [1] above
> > >
> > > "Perhaps the best solution would be to make it as generic as
> > > possible
> > > by checking the plane supported rotations, while still keeoing the
> > > manual checks for the few exceptions I listed above. Might even be
> > > nice to put the generic stuff into something like
> > > igt_plane_has_rotation(). And maybe the exceptions should be there
> > > as well?"
> > >
> > > If I am reading this correctly, this patch should have retained the
> > > plane property checks in addition to exceptions you have added.
> > Based on the feedback received from patch v2 [1], we moved back to
> > using
> > the platform checks instead of using the connector property.
> >
>
> Okay, I am not reading the comment, specifically "make it as generic as
> possible by checking the plane supported rotations", the way you are.
> Check with Ville.
Based on that feedback incorporated the changes in v3[2].
>
> Anyway, if you want to add new platform checks, I do not recommend
> adding all of them in a single patch.
No new checks were added apart from the ones already supported by the driver.
>
>
> >
> > [1] https://patchwork.freedesktop.org/patch/209647/
[2] https://patchwork.freedesktop.org/patch/212409/
-RK
> > >
> > >
> > > -DK
> > >
> > > >
> > > > >
> > > > >
> > > > > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel
> > > > > .com
> > > > > >
> > > > > >
> > > > > ---
> > > > > tests/kms_rotation_crc.c | 35 ++++++++++++++++++++----------
> > > > > -----
> > > > > 1 file changed, 20 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/tests/kms_rotation_crc.c
> > > > > b/tests/kms_rotation_crc.c
> > > > > index 6cb5858adb0f..f20b8a6d4ba1 100644
> > > > > --- a/tests/kms_rotation_crc.c
> > > > > +++ b/tests/kms_rotation_crc.c
> > > > > @@ -43,6 +43,7 @@ typedef struct {
> > > > > uint32_t override_fmt;
> > > > > uint64_t override_tiling;
> > > > > int devid;
> > > > > + int gen;
> > > > > } data_t;
> > > > >
> > > > > typedef struct {
> > > > > @@ -284,6 +285,17 @@ static void prepare_fbs(data_t *data,
> > > > > igt_output_t *output,
> > > > > igt_plane_set_position(plane, data->pos_x,
> > > > > data-
> > > > > >
> > > > > >
> > > > > > pos_y);
> > > > > }
> > > > >
> > > > > +static void igt_check_rotation(data_t *data)
> > > > > +{
> > > > > + if (data->rotation & (IGT_ROTATION_90 |
> > > > > IGT_ROTATION_270))
> > > > > + igt_require(data->gen >= 9);
> > > > > + if (data->rotation & IGT_REFLECT_X)
> > > > > + igt_require(data->gen >= 11 ||
> > > > This check used to be igt_require(gen >= 10
> > Will move it back to gen10.
> > >
> > > >
> > > >
> > > > >
> > > > >
> > > > > + (IS_CHERRYVIEW(data->devid) &&
> > > > > (data-
> > > > > >
> > > > > >
> > > > > > rotation & IGT_ROTATION_0)));
> > > > There was also a check for tiling format
> > > > - (IS_CHERRYVIEW(data.devid) &&
> > > > reflect_x->rot == IGT_ROTATION_0
> > > > - && reflect_x->tiling ==
> > > > LOCAL_I915_FORMAT_MOD_X_TILED));
> > > >
> > > >
> > > > >
> > > > >
> > > > > + if (data->rotation & IGT_ROTATION_180)
> > > > > + igt_require(data->gen >= 4);
> > > > Doesn't look like this requirement was is in the test earlier.
> > > >
> > This is the requirement present in the kernel and got included here
> > on suggestion from here[1].
> >
> > >
> > > >
> > > > >
> > > > >
> > > > > +}
> > > > > +
> > > > > static void test_single_case(data_t *data, enum pipe pipe,
> > > > > igt_output_t *output, igt_plane_t
> > > > > *plane,
> > > > > enum rectangle_type rect,
> > > > > @@ -352,15 +364,18 @@ static void test_plane_rotation(data_t
> > > > > *data,
> > > > > int plane_type, bool test_bad_form
> > > > >
> > > > > igt_display_require_output(display);
> > > > >
> > > > > + igt_check_rotation(data);
> > > > > +
> > > > > for_each_pipe_with_valid_output(display, pipe, output)
> > > > > {
> > > > > igt_plane_t *plane;
> > > > > int i, j;
> > > > >
> > > > > - if (IS_CHERRYVIEW(data->devid) && pipe !=
> > > > > PIPE_B)
> > > > > - continue;
> > > > > -
> > > > > igt_output_set_pipe(output, pipe);
> > > > >
> > > > > + if (IS_CHERRYVIEW(data->devid) && (data-
> > > > > >rotation
> > > > > &
> > > > > IGT_REFLECT_X) &&
> > > > > + pipe != kmstest_pipe_to_index('B'))
> > > > > + continue;
> > > > > +
> > > > Why do this?
> > > >
> > On braswell reflectx is supported only on Pipe B and the test case
> > needs to be skipped for pipes A,C.
> >
> >
> > Regards,
> > Radhhakrishna(RK) Sripada
> > >
> > > >
> > > > >
> > > > >
> > > > > plane = igt_output_get_plane_type(output,
> > > > > plane_type);
> > > > > igt_require(igt_plane_has_prop(plane,
> > > > > IGT_PLANE_ROTATION));
> > > > >
> > > > > @@ -521,14 +536,13 @@ igt_main
> > > > > };
> > > > >
> > > > > data_t data = {};
> > > > > - int gen = 0;
> > > > >
> > > > > igt_skip_on_simulation();
> > > > >
> > > > > igt_fixture {
> > > > > data.gfx_fd =
> > > > > drm_open_driver_master(DRIVER_INTEL);
> > > > > data.devid = intel_get_drm_devid(data.gfx_fd);
> > > > > - gen = intel_gen(data.devid);
> > > > > + data.gen = intel_gen(data.devid);
> > > > >
> > > > > kmstest_set_vt_graphics_mode();
> > > > >
> > > > > @@ -541,16 +555,12 @@ igt_main
> > > > > igt_subtest_f("%s-rotation-%s",
> > > > > plane_test_str(subtest->plane),
> > > > > rot_test_str(subtest->rot)) {
> > > > > - igt_require(!(subtest->rot &
> > > > > - (IGT_ROTATION_90 |
> > > > > IGT_ROTATION_270)) ||
> > > > > - gen >= 9);
> > > > > data.rotation = subtest->rot;
> > > > > test_plane_rotation(&data, subtest-
> > > > > >plane,
> > > > > false);
> > > > > }
> > > > > }
> > > > >
> > > > > igt_subtest_f("sprite-rotation-90-pos-100-0") {
> > > > > - igt_require(gen >= 9);
> > > > > data.rotation = IGT_ROTATION_90;
> > > > > data.pos_x = 100,
> > > > > data.pos_y = 0;
> > > > > @@ -560,7 +570,6 @@ igt_main
> > > > > data.pos_y = 0;
> > > > >
> > > > > igt_subtest_f("bad-pixel-format") {
> > > > > - igt_require(gen >= 9);
> > > > > data.rotation = IGT_ROTATION_90;
> > > > > data.override_fmt = DRM_FORMAT_RGB565;
> > > > > test_plane_rotation(&data,
> > > > > DRM_PLANE_TYPE_PRIMARY,
> > > > > true);
> > > > > @@ -568,7 +577,6 @@ igt_main
> > > > > data.override_fmt = 0;
> > > > >
> > > > > igt_subtest_f("bad-tiling") {
> > > > > - igt_require(gen >= 9);
> > > > > data.rotation = IGT_ROTATION_90;
> > > > > data.override_tiling =
> > > > > LOCAL_I915_FORMAT_MOD_X_TILED;
> > > > > test_plane_rotation(&data,
> > > > > DRM_PLANE_TYPE_PRIMARY,
> > > > > true);
> > > > > @@ -579,9 +587,6 @@ igt_main
> > > > > igt_subtest_f("primary-%s-reflect-x-%s",
> > > > > tiling_test_str(reflect_x-
> > > > > >tiling),
> > > > > rot_test_str(reflect_x->rot)) {
> > > > > - igt_require(gen >= 10 ||
> > > > > - (IS_CHERRYVIEW(data.devid)
> > > > > &&
> > > > > reflect_x->rot == IGT_ROTATION_0
> > > > > - && reflect_x->tiling ==
> > > > > LOCAL_I915_FORMAT_MOD_X_TILED));
> > > > > data.rotation = (IGT_REFLECT_X |
> > > > > reflect_x-
> > > > > >
> > > > > >
> > > > > > rot);
> > > > > data.override_tiling = reflect_x-
> > > > > >tiling;
> > > > > test_plane_rotation(&data,
> > > > > DRM_PLANE_TYPE_PRIMARY, false);
> > > > > @@ -596,7 +601,7 @@ igt_main
> > > > > enum pipe pipe;
> > > > > igt_output_t *output;
> > > > >
> > > > > - igt_require(gen >= 9);
> > > > > + igt_require(data.gen >= 9);
> > > > > igt_display_require_output(&data.display);
> > > > >
> > > > > for_each_pipe_with_valid_output(&data.display,
> > > > > pipe,
> > > > > output) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases
@ 2018-07-27 22:26 ` Radhakrishna Sripada
0 siblings, 0 replies; 14+ messages in thread
From: Radhakrishna Sripada @ 2018-07-27 22:26 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: Anusha Srivatsa, intel-gfx, igt-dev, Daniel Vetter
On Fri, Jul 27, 2018 at 03:06:19PM -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-07-27 at 13:43 -0700, Radhakrishna Sripada wrote:
> > On Mon, Jul 23, 2018 at 08:26:06PM -0700, Dhinakaran Pandiyan wrote:
> > >
> > > On Mon, 2018-07-23 at 19:52 -0700, Dhinakaran Pandiyan wrote:
> > > >
> > > > On Mon, 2018-07-23 at 11:25 -0700, Radhakrishna Sripada wrote:
> > > > >
> > > > >
> > > > > From: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > >
> > > > > Cleanup the testcases by moving the platform checks to a single
> > > > > function.
> > > > >
> > > > > The earlier version of the path is posted here [1]
> > > > >
> > > > > v2: Make use of the property enums to get the supported
> > > > > rotations
> > > > > v3: Move hardcodings to a single function(Ville)
> > > > > v4: Include the cherryview exception for reflect
> > > > > subtest(Maarten)
> > > > > v5: Rebase and move the check from CNL to ICL for reflect-x
> > > > > case
> > > > > v6: Fix the CI regression
> > > > > v7: rebase
> > > > >
> > > > > [1]: https://patchwork.freedesktop.org/patch/209647/
> > > > >
> > > > Oh well, I wrote my comments below and then read this link.
> > > > Please
> > > > add
> > > > new test requirements in separate patches. Only have the code
> > > > movement
> > > > here.
> > > >
> > > Quoting Ville from [1] above
> > >
> > > "Perhaps the best solution would be to make it as generic as
> > > possible
> > > by checking the plane supported rotations, while still keeoing the
> > > manual checks for the few exceptions I listed above. Might even be
> > > nice to put the generic stuff into something like
> > > igt_plane_has_rotation(). And maybe the exceptions should be there
> > > as well?"
> > >
> > > If I am reading this correctly, this patch should have retained the
> > > plane property checks in addition to exceptions you have added.
> > Based on the feedback received from patch v2 [1], we moved back to
> > using
> > the platform checks instead of using the connector property.
> >
>
> Okay, I am not reading the comment, specifically "make it as generic as
> possible by checking the plane supported rotations", the way you are.
> Check with Ville.
Based on that feedback incorporated the changes in v3[2].
>
> Anyway, if you want to add new platform checks, I do not recommend
> adding all of them in a single patch.
No new checks were added apart from the ones already supported by the driver.
>
>
> >
> > [1] https://patchwork.freedesktop.org/patch/209647/
[2] https://patchwork.freedesktop.org/patch/212409/
-RK
> > >
> > >
> > > -DK
> > >
> > > >
> > > > >
> > > > >
> > > > > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel
> > > > > .com
> > > > > >
> > > > > >
> > > > > ---
> > > > > tests/kms_rotation_crc.c | 35 ++++++++++++++++++++----------
> > > > > -----
> > > > > 1 file changed, 20 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/tests/kms_rotation_crc.c
> > > > > b/tests/kms_rotation_crc.c
> > > > > index 6cb5858adb0f..f20b8a6d4ba1 100644
> > > > > --- a/tests/kms_rotation_crc.c
> > > > > +++ b/tests/kms_rotation_crc.c
> > > > > @@ -43,6 +43,7 @@ typedef struct {
> > > > > uint32_t override_fmt;
> > > > > uint64_t override_tiling;
> > > > > int devid;
> > > > > + int gen;
> > > > > } data_t;
> > > > >
> > > > > typedef struct {
> > > > > @@ -284,6 +285,17 @@ static void prepare_fbs(data_t *data,
> > > > > igt_output_t *output,
> > > > > igt_plane_set_position(plane, data->pos_x,
> > > > > data-
> > > > > >
> > > > > >
> > > > > > pos_y);
> > > > > }
> > > > >
> > > > > +static void igt_check_rotation(data_t *data)
> > > > > +{
> > > > > + if (data->rotation & (IGT_ROTATION_90 |
> > > > > IGT_ROTATION_270))
> > > > > + igt_require(data->gen >= 9);
> > > > > + if (data->rotation & IGT_REFLECT_X)
> > > > > + igt_require(data->gen >= 11 ||
> > > > This check used to be igt_require(gen >= 10
> > Will move it back to gen10.
> > >
> > > >
> > > >
> > > > >
> > > > >
> > > > > + (IS_CHERRYVIEW(data->devid) &&
> > > > > (data-
> > > > > >
> > > > > >
> > > > > > rotation & IGT_ROTATION_0)));
> > > > There was also a check for tiling format
> > > > - (IS_CHERRYVIEW(data.devid) &&
> > > > reflect_x->rot == IGT_ROTATION_0
> > > > - && reflect_x->tiling ==
> > > > LOCAL_I915_FORMAT_MOD_X_TILED));
> > > >
> > > >
> > > > >
> > > > >
> > > > > + if (data->rotation & IGT_ROTATION_180)
> > > > > + igt_require(data->gen >= 4);
> > > > Doesn't look like this requirement was is in the test earlier.
> > > >
> > This is the requirement present in the kernel and got included here
> > on suggestion from here[1].
> >
> > >
> > > >
> > > > >
> > > > >
> > > > > +}
> > > > > +
> > > > > static void test_single_case(data_t *data, enum pipe pipe,
> > > > > igt_output_t *output, igt_plane_t
> > > > > *plane,
> > > > > enum rectangle_type rect,
> > > > > @@ -352,15 +364,18 @@ static void test_plane_rotation(data_t
> > > > > *data,
> > > > > int plane_type, bool test_bad_form
> > > > >
> > > > > igt_display_require_output(display);
> > > > >
> > > > > + igt_check_rotation(data);
> > > > > +
> > > > > for_each_pipe_with_valid_output(display, pipe, output)
> > > > > {
> > > > > igt_plane_t *plane;
> > > > > int i, j;
> > > > >
> > > > > - if (IS_CHERRYVIEW(data->devid) && pipe !=
> > > > > PIPE_B)
> > > > > - continue;
> > > > > -
> > > > > igt_output_set_pipe(output, pipe);
> > > > >
> > > > > + if (IS_CHERRYVIEW(data->devid) && (data-
> > > > > >rotation
> > > > > &
> > > > > IGT_REFLECT_X) &&
> > > > > + pipe != kmstest_pipe_to_index('B'))
> > > > > + continue;
> > > > > +
> > > > Why do this?
> > > >
> > On braswell reflectx is supported only on Pipe B and the test case
> > needs to be skipped for pipes A,C.
> >
> >
> > Regards,
> > Radhhakrishna(RK) Sripada
> > >
> > > >
> > > > >
> > > > >
> > > > > plane = igt_output_get_plane_type(output,
> > > > > plane_type);
> > > > > igt_require(igt_plane_has_prop(plane,
> > > > > IGT_PLANE_ROTATION));
> > > > >
> > > > > @@ -521,14 +536,13 @@ igt_main
> > > > > };
> > > > >
> > > > > data_t data = {};
> > > > > - int gen = 0;
> > > > >
> > > > > igt_skip_on_simulation();
> > > > >
> > > > > igt_fixture {
> > > > > data.gfx_fd =
> > > > > drm_open_driver_master(DRIVER_INTEL);
> > > > > data.devid = intel_get_drm_devid(data.gfx_fd);
> > > > > - gen = intel_gen(data.devid);
> > > > > + data.gen = intel_gen(data.devid);
> > > > >
> > > > > kmstest_set_vt_graphics_mode();
> > > > >
> > > > > @@ -541,16 +555,12 @@ igt_main
> > > > > igt_subtest_f("%s-rotation-%s",
> > > > > plane_test_str(subtest->plane),
> > > > > rot_test_str(subtest->rot)) {
> > > > > - igt_require(!(subtest->rot &
> > > > > - (IGT_ROTATION_90 |
> > > > > IGT_ROTATION_270)) ||
> > > > > - gen >= 9);
> > > > > data.rotation = subtest->rot;
> > > > > test_plane_rotation(&data, subtest-
> > > > > >plane,
> > > > > false);
> > > > > }
> > > > > }
> > > > >
> > > > > igt_subtest_f("sprite-rotation-90-pos-100-0") {
> > > > > - igt_require(gen >= 9);
> > > > > data.rotation = IGT_ROTATION_90;
> > > > > data.pos_x = 100,
> > > > > data.pos_y = 0;
> > > > > @@ -560,7 +570,6 @@ igt_main
> > > > > data.pos_y = 0;
> > > > >
> > > > > igt_subtest_f("bad-pixel-format") {
> > > > > - igt_require(gen >= 9);
> > > > > data.rotation = IGT_ROTATION_90;
> > > > > data.override_fmt = DRM_FORMAT_RGB565;
> > > > > test_plane_rotation(&data,
> > > > > DRM_PLANE_TYPE_PRIMARY,
> > > > > true);
> > > > > @@ -568,7 +577,6 @@ igt_main
> > > > > data.override_fmt = 0;
> > > > >
> > > > > igt_subtest_f("bad-tiling") {
> > > > > - igt_require(gen >= 9);
> > > > > data.rotation = IGT_ROTATION_90;
> > > > > data.override_tiling =
> > > > > LOCAL_I915_FORMAT_MOD_X_TILED;
> > > > > test_plane_rotation(&data,
> > > > > DRM_PLANE_TYPE_PRIMARY,
> > > > > true);
> > > > > @@ -579,9 +587,6 @@ igt_main
> > > > > igt_subtest_f("primary-%s-reflect-x-%s",
> > > > > tiling_test_str(reflect_x-
> > > > > >tiling),
> > > > > rot_test_str(reflect_x->rot)) {
> > > > > - igt_require(gen >= 10 ||
> > > > > - (IS_CHERRYVIEW(data.devid)
> > > > > &&
> > > > > reflect_x->rot == IGT_ROTATION_0
> > > > > - && reflect_x->tiling ==
> > > > > LOCAL_I915_FORMAT_MOD_X_TILED));
> > > > > data.rotation = (IGT_REFLECT_X |
> > > > > reflect_x-
> > > > > >
> > > > > >
> > > > > > rot);
> > > > > data.override_tiling = reflect_x-
> > > > > >tiling;
> > > > > test_plane_rotation(&data,
> > > > > DRM_PLANE_TYPE_PRIMARY, false);
> > > > > @@ -596,7 +601,7 @@ igt_main
> > > > > enum pipe pipe;
> > > > > igt_output_t *output;
> > > > >
> > > > > - igt_require(gen >= 9);
> > > > > + igt_require(data.gen >= 9);
> > > > > igt_display_require_output(&data.display);
> > > > >
> > > > > for_each_pipe_with_valid_output(&data.display,
> > > > > pipe,
> > > > > output) {
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-07-27 22:26 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-23 18:25 [PATCH i-g-t v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases Radhakrishna Sripada
2018-07-23 18:25 ` [igt-dev] " Radhakrishna Sripada
2018-07-23 18:58 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases (rev5) Patchwork
2018-07-23 20:23 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
2018-07-24 2:52 ` [igt-dev] [PATCH i-g-t v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases Dhinakaran Pandiyan
2018-07-24 2:52 ` [Intel-gfx] " Dhinakaran Pandiyan
2018-07-24 3:26 ` Dhinakaran Pandiyan
2018-07-24 3:26 ` [Intel-gfx] " Dhinakaran Pandiyan
2018-07-27 20:43 ` Radhakrishna Sripada
2018-07-27 20:43 ` Radhakrishna Sripada
2018-07-27 22:06 ` Dhinakaran Pandiyan
2018-07-27 22:06 ` Dhinakaran Pandiyan
2018-07-27 22:26 ` Radhakrishna Sripada
2018-07-27 22:26 ` Radhakrishna Sripada
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.