On Tue, Nov 14, 2017 at 04:58:35PM +0200, Juha-Pekka Heikkila wrote:
> Gen10 onwards 90 and 270 degree rotations are supported for RGB565 format.
>
> v2 (Ville Syrjälä):
> As a side effect to keep bad-pixel-format test valid on all supported
> platforms it need to use DRM_FORMAT_C8 now.
>
> While at it clean up kms_rotation_crc test a bit, take out
> test_plane_rotation_ytiled_obj() function as
> test_plane_rotation() can basically do the same.
>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@xxxxxxxxx>
> ---
> tests/kms_rotation_crc.c | 101 ++++++++++-------------------------------------
> 1 file changed, 20 insertions(+), 81 deletions(-)
>
> diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
> index 27d1f80..0ed2ce3 100644
> --- a/tests/kms_rotation_crc.c
> +++ b/tests/kms_rotation_crc.c
> @@ -376,7 +376,7 @@ static void test_plane_rotation(data_t *data, int plane_type)
> igt_plane_set_size(plane, data->fb.height, data->fb.width);
>
> ret = igt_display_try_commit2(display, commit);
> - if (data->override_fmt || data->override_tiling) {
> + if (data->override_fmt == DRM_FORMAT_C8) {
I think it would be cleaner to have some kind of data->expected_error
and then do
if (data->expected_error) {
igt_assertt(ret, data->expected_error);
contine;
}
That way it's obvious which subtests expect an error and which don't.
> igt_assert_eq(ret, -EINVAL);
> continue;
> }
> @@ -421,70 +421,6 @@ static void test_plane_rotation(data_t *data, int plane_type)
> igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
> }
>
> -static void test_plane_rotation_ytiled_obj(data_t *data,
> - igt_output_t *output,
> - int plane_type)
> -{
> - igt_display_t *display = &data->display;
> - uint64_t tiling = LOCAL_I915_FORMAT_MOD_Y_TILED;
> - uint32_t format = DRM_FORMAT_XRGB8888;
> - int bpp = igt_drm_format_to_bpp(format);
> - enum igt_commit_style commit = COMMIT_LEGACY;
> - int fd = data->gfx_fd;
> - igt_plane_t *plane;
> - drmModeModeInfo *mode;
> - unsigned int stride, size, w, h;
> - uint32_t gem_handle;
> - int ret;
> -
> - plane = igt_output_get_plane_type(output, plane_type);
> - igt_require(igt_plane_has_prop(plane, IGT_PLANE_ROTATION));
> -
> - if (plane_type == DRM_PLANE_TYPE_PRIMARY || plane_type == DRM_PLANE_TYPE_CURSOR)
> - commit = COMMIT_UNIVERSAL;
> -
> - if (plane_type == DRM_PLANE_TYPE_CURSOR)
> - igt_require(display->has_cursor_plane);
> -
> - if (data->display.is_atomic)
> - commit = COMMIT_ATOMIC;
> -
> - mode = igt_output_get_mode(output);
> - w = mode->hdisplay;
> - h = mode->vdisplay;
> -
> - for (stride = 512; stride < (w * bpp / 8); stride *= 2)
> - ;
> - for (size = 1024*1024; size < stride * h; size *= 2)
> - ;
> -
> - gem_handle = gem_create(fd, size);
> - ret = __gem_set_tiling(fd, gem_handle, I915_TILING_Y, stride);
> - igt_assert_eq(ret, 0);
> -
> - do_or_die(__kms_addfb(fd, gem_handle, w, h, stride,
> - format, tiling, LOCAL_DRM_MODE_FB_MODIFIERS,
> - &data->fb.fb_id));
> - data->fb.width = w;
> - data->fb.height = h;
> - data->fb.gem_handle = gem_handle;
> -
> - igt_plane_set_fb(plane, NULL);
> - igt_display_commit(display);
> -
> - igt_plane_set_rotation(plane, data->rotation);
> - igt_plane_set_fb(plane, &data->fb);
> - igt_plane_set_size(plane, h, w);
> -
> - ret = igt_display_try_commit2(display, commit);
> -
> - igt_output_set_pipe(output, PIPE_NONE);
> -
> - kmstest_restore_vt_mode();
> - igt_remove_fb(fd, &data->fb);
> - igt_assert_eq(ret, 0);
> -}
> -
> static void test_plane_rotation_exhaust_fences(data_t *data,
> igt_output_t *output,
> int plane_type)
> @@ -697,7 +633,12 @@ igt_main
> data.pos_x = 0,
> data.pos_y = 0;
> data.rotation = IGT_ROTATION_90;
> - data.override_fmt = DRM_FORMAT_RGB565;
> + /*
> + * Inside test_plane_rotation() there's
> + * check for DRM_FORMAT_C8 to make this
> + * test work correctly.
> + */
> + data.override_fmt = DRM_FORMAT_C8;
> test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
> }
>
> @@ -709,24 +650,22 @@ igt_main
> test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
> }
>
> - igt_subtest_f("primary-rotation-90-Y-tiled") {
> - enum pipe pipe;
> - igt_output_t *output;
> - int valid_tests = 0;
> + igt_subtest_f("primary-rotation-90-Y-tiled-16bpp") {
> + /*
> + * this test requires hw greater than gen9
> + */
> + igt_require(gen > 9);
gen >= 10 is a more customary way of writing this. The comment is
redundant.
> + data.rotation = IGT_ROTATION_90;
> + data.override_fmt = DRM_FORMAT_RGB565;
> + data.override_tiling = LOCAL_I915_FORMAT_MOD_Y_TILED;
> + test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
> + }
>
> + igt_subtest_f("primary-rotation-90-Y-tiled") {
> igt_require(gen >= 9);
> data.rotation = IGT_ROTATION_90;
I wonder where we're resetting data.override_tiling/fmt etc. between
the subtests? Maybe we're not and this whole thing is super buggy if you
do multiple subtests in the same run?
> -
> - for_each_pipe_with_valid_output(&data.display, pipe, output) {
> - igt_output_set_pipe(output, pipe);
> -
> - test_plane_rotation_ytiled_obj(&data, output, DRM_PLANE_TYPE_PRIMARY);
> -
> - valid_tests++;
> - break;
> - }
> -
> - igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
> + data.override_fmt = DRM_FORMAT_XRGB8888;
> + test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
> }
>
> igt_subtest_f("exhaust-fences") {
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
