Re: [Intel-gfx] [igt-dev] [PATCH i-g-t v2] tests/kms_rotation_crc: Remove hardcoding of platforms in igt_require()

2018-03-12 Thread Ville Syrjälä
On Mon, Mar 12, 2018 at 04:09:06PM +0100, Maarten Lankhorst wrote:
> Op 12-03-18 om 15:23 schreef Ville Syrjälä:
> > On Mon, Mar 12, 2018 at 12:03:17PM +0100, Maarten Lankhorst wrote:
> >> Op 09-03-18 om 22:45 schreef Radhakrishna Sripada:
> >>> From: Anusha Srivatsa 
> >>>
> >>> Rework the rotate and reflect subtests by checking the
> >>> crtc supported properties against the ones that the
> >>> test is testing. Remove the hardcoded platform names in
> >>> igt_require()
> >>>
> >>> v2: Make use of the property enums to get the supported rotations
> >>>
> >>> Cc: Radhakrishna Sripada 
> >>> Cc: Daniel Vetter 
> >>> Cc: Rodrigo Vivi 
> >>> Cc: Maarten Lankhorst 
> >>> Cc: Mika Kahola 
> >>> Cc: Manasi Navare 
> >>> Signed-off-by: Anusha Srivatsa 
> >>> Signed-off-by: Radhakrishna Sripada 
> >>> ---
> >>>  lib/igt_kms.h|  1 +
> >>>  tests/kms_rotation_crc.c | 46 
> >>> +-
> >>>  2 files changed, 38 insertions(+), 9 deletions(-)
> >> Just let it rest, it's not worth the effort to remove this, you only end 
> >> up with more complicated code..
> >> Closest I've come is below. Which will still fail because it will try to 
> >> generate the wrong tilings pre-gen9..
> > The current code is actually wrong. chv doesn't require X-tiling for
> > reflection.
> >
> > I think the actual hardware restrictions we have are:
> > gen4+: 0/180 degree rotation supported always
> > chv: supports reflection, except with 180 degree rotation
> > gen9+: 90/270 degree rotation supported with y/yf tiling
> > gen10+: supports reflection, except with linear fb
> So can we leave it the way it is now?

Maybe. I don't really know what we're trying to test here. Someone
should probably remove the bogus x-tiling check from chv at least.

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?

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [igt-dev] [PATCH i-g-t v2] tests/kms_rotation_crc: Remove hardcoding of platforms in igt_require()

2018-03-12 Thread Maarten Lankhorst
Op 12-03-18 om 15:23 schreef Ville Syrjälä:
> On Mon, Mar 12, 2018 at 12:03:17PM +0100, Maarten Lankhorst wrote:
>> Op 09-03-18 om 22:45 schreef Radhakrishna Sripada:
>>> From: Anusha Srivatsa 
>>>
>>> Rework the rotate and reflect subtests by checking the
>>> crtc supported properties against the ones that the
>>> test is testing. Remove the hardcoded platform names in
>>> igt_require()
>>>
>>> v2: Make use of the property enums to get the supported rotations
>>>
>>> Cc: Radhakrishna Sripada 
>>> Cc: Daniel Vetter 
>>> Cc: Rodrigo Vivi 
>>> Cc: Maarten Lankhorst 
>>> Cc: Mika Kahola 
>>> Cc: Manasi Navare 
>>> Signed-off-by: Anusha Srivatsa 
>>> Signed-off-by: Radhakrishna Sripada 
>>> ---
>>>  lib/igt_kms.h|  1 +
>>>  tests/kms_rotation_crc.c | 46 
>>> +-
>>>  2 files changed, 38 insertions(+), 9 deletions(-)
>> Just let it rest, it's not worth the effort to remove this, you only end up 
>> with more complicated code..
>> Closest I've come is below. Which will still fail because it will try to 
>> generate the wrong tilings pre-gen9..
> The current code is actually wrong. chv doesn't require X-tiling for
> reflection.
>
> I think the actual hardware restrictions we have are:
> gen4+: 0/180 degree rotation supported always
> chv: supports reflection, except with 180 degree rotation
> gen9+: 90/270 degree rotation supported with y/yf tiling
> gen10+: supports reflection, except with linear fb
So can we leave it the way it is now?

~Maarten
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [igt-dev] [PATCH i-g-t v2] tests/kms_rotation_crc: Remove hardcoding of platforms in igt_require()

2018-03-12 Thread Ville Syrjälä
On Mon, Mar 12, 2018 at 12:03:17PM +0100, Maarten Lankhorst wrote:
> Op 09-03-18 om 22:45 schreef Radhakrishna Sripada:
> > From: Anusha Srivatsa 
> >
> > Rework the rotate and reflect subtests by checking the
> > crtc supported properties against the ones that the
> > test is testing. Remove the hardcoded platform names in
> > igt_require()
> >
> > v2: Make use of the property enums to get the supported rotations
> >
> > Cc: Radhakrishna Sripada 
> > Cc: Daniel Vetter 
> > Cc: Rodrigo Vivi 
> > Cc: Maarten Lankhorst 
> > Cc: Mika Kahola 
> > Cc: Manasi Navare 
> > Signed-off-by: Anusha Srivatsa 
> > Signed-off-by: Radhakrishna Sripada 
> > ---
> >  lib/igt_kms.h|  1 +
> >  tests/kms_rotation_crc.c | 46 
> > +-
> >  2 files changed, 38 insertions(+), 9 deletions(-)
> 
> Just let it rest, it's not worth the effort to remove this, you only end up 
> with more complicated code..
> Closest I've come is below. Which will still fail because it will try to 
> generate the wrong tilings pre-gen9..

The current code is actually wrong. chv doesn't require X-tiling for
reflection.

I think the actual hardware restrictions we have are:
gen4+: 0/180 degree rotation supported always
chv: supports reflection, except with 180 degree rotation
gen9+: 90/270 degree rotation supported with y/yf tiling
gen10+: supports reflection, except with linear fb

> 
> Lets keep it as it is now?
> ---
> diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
> index 0cd5c6e52b57..9b9da53054c1 100644
> --- a/tests/kms_rotation_crc.c
> +++ b/tests/kms_rotation_crc.c
> @@ -42,7 +42,6 @@ typedef struct {
>   int pos_y;
>   uint32_t override_fmt;
>   uint64_t override_tiling;
> - int devid;
>  } data_t;
>  
>  typedef struct {
> @@ -358,11 +357,22 @@ static void test_single_case(data_t *data, enum pipe 
> pipe,
>   }
>  }
>  
> +static bool supports_rotation(igt_display_t *display, igt_plane_t *plane, 
> uint64_t rotation)
> +{
> + igt_require(igt_plane_has_prop(plane, IGT_PLANE_ROTATION));
> +
> + igt_assert(!igt_display_try_commit_atomic(display, 
> DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL));
> + igt_plane_set_prop_value(plane, IGT_PLANE_ROTATION, rotation);
> +
> + return !igt_display_try_commit_atomic(display, 
> DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> +}
> +
>  static void test_plane_rotation(data_t *data, int plane_type, bool 
> test_bad_format)
>  {
>   igt_display_t *display = &data->display;
>   igt_output_t *output;
>   enum pipe pipe;
> + bool tested = false;
>  
>   if (plane_type == DRM_PLANE_TYPE_CURSOR)
>   igt_require(display->has_cursor_plane);
> @@ -373,13 +383,15 @@ static void test_plane_rotation(data_t *data, int 
> plane_type, bool test_bad_form
>   igt_plane_t *plane;
>   int i, j;
>  
> - if (IS_CHERRYVIEW(data->devid) && pipe != PIPE_B)
> - continue;
> + igt_display_reset(display);
>  
>   igt_output_set_pipe(output, pipe);
> -
>   plane = igt_output_get_plane_type(output, plane_type);
> - igt_require(igt_plane_has_prop(plane, IGT_PLANE_ROTATION));
> +
> + if (!supports_rotation(display, plane, data->rotation))
> + continue;
> +
> + tested = true;
>  
>   prepare_crtc(data, output, pipe, plane);
>  
> @@ -410,6 +422,8 @@ static void test_plane_rotation(data_t *data, int 
> plane_type, bool test_bad_form
>   }
>   }
>   }
> +
> + igt_assert_f(tested, "Rotation for this plane type not supported on any 
> valid pipe.\n");
>  }
>  
>  static void test_plane_rotation_exhaust_fences(data_t *data,
> @@ -538,14 +552,11 @@ 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);
>  
>   kmstest_set_vt_graphics_mode();
>  
> @@ -558,16 +569,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;
> @@ -577,7 +584,6 @@ igt_main