[igt-dev] [PATCH i-g-t v7] tests/kms_rotation_crc: Move platform checks to one place for non exhaust fence cases
Dhinakaran Pandiyan
dhinakaran.pandiyan at intel.com
Fri Jul 27 22:06:19 UTC 2018
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 at 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 at intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter at intel.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > > Cc: Mika Kahola <mika.kahola at intel.com>
> > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
> > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at 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) {
More information about the igt-dev
mailing list