| Message ID | 20180723182545.19461-1-radhakrishna.sripada@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t,v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases | expand |
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) {
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) {
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) {
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) {
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) {
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) {