[igt-dev] [PATCH i-g-t 9/9] tests/kms_rotation_crc: Perform lazy cleanup and require atomic.
Mika Kahola
mika.kahola at intel.com
Wed Feb 7 10:37:51 UTC 2018
On Wed, 2018-02-07 at 10:46 +0100, Maarten Lankhorst wrote:
> Op 07-02-18 om 09:55 schreef Mika Kahola:
> >
> > On Thu, 2018-02-01 at 16:39 +0100, Maarten Lankhorst wrote:
> > >
> > > This won't clean up resources between subtests, so if subtests
> > > fail
> > > the next subtest will clean up everything. This allows all
> > > subtests
> > > even if one fails.
> > >
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.c
> > > om>
> > > ---
> > > tests/kms_rotation_crc.c | 213 +++++++++++++++----------------
> > > ----
> > > ------------
> > > 1 file changed, 66 insertions(+), 147 deletions(-)
> > >
> > > diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
> > > index 52a85fb62f9f..a0fb734d02f6 100644
> > > --- a/tests/kms_rotation_crc.c
> > > +++ b/tests/kms_rotation_crc.c
> > > @@ -33,7 +33,6 @@ typedef struct {
> > > struct igt_fb fb;
> > > struct igt_fb fb_reference;
> > > struct igt_fb fb_unrotated;
> > > - struct igt_fb fb_modeset;
> > > struct igt_fb fb_flip;
> > > igt_crc_t ref_crc;
> > > igt_crc_t flip_crc;
> > > @@ -122,68 +121,50 @@ paint_squares(data_t *data, igt_rotation_t
> > > rotation,
> > > igt_put_cairo_ctx(data->gfx_fd, fb, cr);
> > > }
> > >
> > > -static void prepare_crtc(data_t *data, igt_output_t *output,
> > > enum
> > > pipe pipe,
> > > - igt_plane_t *plane, enum
> > > igt_commit_style
> > > commit)
> > > +static void remove_fbs(data_t *data)
> > > {
> > > - drmModeModeInfo *mode;
> > > - unsigned int w, h;
> > > - uint64_t tiling = data->override_tiling ?:
> > > LOCAL_DRM_FORMAT_MOD_NONE;
> > > - uint32_t pixel_format = data->override_fmt ?:
> > > DRM_FORMAT_XRGB8888;
> > > - igt_display_t *display = &data->display;
> > > - igt_plane_t *primary = igt_output_get_plane_type(output,
> > > DRM_PLANE_TYPE_PRIMARY);
> > > + if (data->fb.fb_id)
> > > + igt_remove_fb(data->gfx_fd, &data->fb);
> > >
> > > - igt_output_set_pipe(output, pipe);
> > > - igt_plane_set_rotation(plane, IGT_ROTATION_0);
> > > + if (data->fb_reference.fb_id)
> > > + igt_remove_fb(data->gfx_fd, &data-
> > > >fb_reference);
> > >
> > > - /* create the pipe_crc object for this pipe */
> > > - igt_pipe_crc_free(data->pipe_crc);
> > > - data->pipe_crc = igt_pipe_crc_new(data->gfx_fd, pipe,
> > > INTEL_PIPE_CRC_SOURCE_AUTO);
> > > -
> > > - mode = igt_output_get_mode(output);
> > > + if (data->fb_unrotated.fb_id)
> > > + igt_remove_fb(data->gfx_fd, &data-
> > > >fb_unrotated);
> > >
> > > - w = mode->hdisplay;
> > > - h = mode->vdisplay;
> > > -
> > > - igt_create_fb(data->gfx_fd, w, h, pixel_format, tiling,
> > > &data->fb_modeset);
> > > + if (data->fb_flip.fb_id)
> > > + igt_remove_fb(data->gfx_fd, &data->fb_flip);
> > >
> > > - /*
> > > - * With igt_display_commit2 and COMMIT_UNIVERSAL, we
> > > call
> > > just the
> > > - * setplane without a modeset. So, to be able to call
> > > - * igt_display_commit and ultimately setcrtc to do the
> > > first
> > > modeset,
> > > - * we create an fb covering the crtc and call commit
> > > - *
> > > - * It's also a good idea to set a primary fb on the
> > > primary
> > > plane
> > > - * regardless, to force a underrun when watermarks are
> > > allocated
> > > - * incorrectly for other planes.
> > > - */
> > > - igt_plane_set_fb(primary, &data->fb_modeset);
> > > + data->fb_flip.fb_id = data->fb_reference.fb_id = data-
> > > >
> > > > fb_unrotated.fb_id = data->fb.fb_id = 0;
> > > +}
> > >
> > > - if (commit < COMMIT_ATOMIC) {
> > > - igt_plane_clear_prop_changed(primary,
> > > IGT_PLANE_ROTATION);
> > > - igt_display_commit(display);
> > > +static void cleanup_crtc(data_t *data)
> > > +{
> > > + igt_display_t *display = &data->display;
> > >
> > > - if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > > - igt_plane_set_prop_changed(primary,
> > > IGT_PLANE_ROTATION);
> > > - }
> > > + igt_pipe_crc_free(data->pipe_crc);
> > > + data->pipe_crc = NULL;
> > >
> > > - igt_plane_set_fb(plane, NULL);
> > > + remove_fbs(data);
> > >
> > > - igt_display_commit2(display, commit);
> > > + igt_display_reset(display);
> > > }
> > >
> > > -static void remove_fbs(data_t *data)
> > > +static void prepare_crtc(data_t *data, igt_output_t *output,
> > > enum
> > > pipe pipe,
> > > + igt_plane_t *plane)
> > > {
> > > - if (!data->fb.fb_id)
> > > - return;
> > > + igt_display_t *display = &data->display;
> > >
> > > - igt_remove_fb(data->gfx_fd, &data->fb);
> > > - igt_remove_fb(data->gfx_fd, &data->fb_reference);
> > > - igt_remove_fb(data->gfx_fd, &data->fb_unrotated);
> > > + cleanup_crtc(data);
> > >
> > > - if (data->fb_flip.fb_id)
> > > - igt_remove_fb(data->gfx_fd, &data->fb_flip);
> > > + igt_output_set_pipe(output, pipe);
> > > + igt_plane_set_rotation(plane, IGT_ROTATION_0);
> > > +
> > > + /* create the pipe_crc object for this pipe */
> > > + igt_pipe_crc_free(data->pipe_crc);
> > > + data->pipe_crc = igt_pipe_crc_new(data->gfx_fd, pipe,
> > > INTEL_PIPE_CRC_SOURCE_AUTO);
> > >
> > > - data->fb_flip.fb_id = data->fb.fb_id = 0;
> > > + igt_display_commit2(display, COMMIT_ATOMIC);
> > > }
> > >
> > > enum rectangle_type {
> > > @@ -206,7 +187,7 @@ static void prepare_fbs(data_t *data,
> > > igt_output_t *output,
> > >
> > > if (data->fb.fb_id) {
> > > igt_plane_set_fb(plane, NULL);
> > > - igt_display_commit2(display, display->is_atomic
> > > ?
> > > COMMIT_ATOMIC : COMMIT_UNIVERSAL);
> > > + igt_display_commit2(display, COMMIT_ATOMIC);
> > Should we keep the legacy commit here or should we have a
> > requirement
> > for this test that it runs only atomic?
> Hey,
>
> Only the 180° tests won't run on gen4 without
> i915.nuclear_pageflip=1, at least until gen3 wm patch series is
> finally merged. But they are never run in CI (shards) anyway .
> So in practice the requirement doesn't matter and if we do care then
> we should get those in finally...
Ok. Let's discard this. Otherwise, the patch looks good.
Reviewed-by: Mika Kahola <mika.kahola at intel.com>
>
> ~Maarten
>
> >
> > >
> > >
> > > remove_fbs(data);
> > > }
> > > @@ -259,28 +240,24 @@ static void prepare_fbs(data_t *data,
> > > igt_output_t *output,
> > > /*
> > > * Create a reference software rotated flip framebuffer.
> > > */
> > > - if (plane->type == DRM_PLANE_TYPE_PRIMARY || display-
> > > >
> > > > is_atomic) {
> > > - igt_create_fb(data->gfx_fd, ref_w, ref_h,
> > > pixel_format, tiling,
> > > - &data->fb_flip);
> > > - paint_squares(data, data->rotation, &data-
> > > >fb_flip,
> > > - flip_opacity);
> > > - igt_plane_set_fb(plane, &data->fb_flip);
> > > - if (plane->type != DRM_PLANE_TYPE_CURSOR)
> > > - igt_plane_set_position(plane, data-
> > > >pos_x,
> > > data->pos_y);
> > > - igt_display_commit2(display,
> > > - display->is_atomic ?
> > > - COMMIT_ATOMIC :
> > > COMMIT_UNIVERSAL);
> > > - igt_pipe_crc_collect_crc(data->pipe_crc, &data-
> > > >
> > > > flip_crc);
> > > -
> > > - /*
> > > - * Prepare the non-rotated flip fb.
> > > - */
> > > - igt_remove_fb(data->gfx_fd, &data->fb_flip);
> > > - igt_create_fb(data->gfx_fd, w, h, pixel_format,
> > > tiling,
> > > - &data->fb_flip);
> > > - paint_squares(data, IGT_ROTATION_0, &data-
> > > >fb_flip,
> > > - flip_opacity);
> > > - }
> > > + igt_create_fb(data->gfx_fd, ref_w, ref_h, pixel_format,
> > > tiling,
> > > + &data->fb_flip);
> > > + paint_squares(data, data->rotation, &data->fb_flip,
> > > + flip_opacity);
> > > + igt_plane_set_fb(plane, &data->fb_flip);
> > > + if (plane->type != DRM_PLANE_TYPE_CURSOR)
> > > + igt_plane_set_position(plane, data->pos_x, data-
> > > >
> > > > pos_y);
> > > + igt_display_commit2(display, COMMIT_ATOMIC);
> > > + igt_pipe_crc_collect_crc(data->pipe_crc, &data-
> > > >flip_crc);
> > > +
> > > + /*
> > > + * Prepare the non-rotated flip fb.
> > > + */
> > > + igt_remove_fb(data->gfx_fd, &data->fb_flip);
> > > + igt_create_fb(data->gfx_fd, w, h, pixel_format, tiling,
> > > + &data->fb_flip);
> > > + paint_squares(data, IGT_ROTATION_0, &data->fb_flip,
> > > + flip_opacity);
> > >
> > > /*
> > > * Create a reference CRC for a software-rotated fb.
> > > @@ -292,7 +269,7 @@ static void prepare_fbs(data_t *data,
> > > igt_output_t *output,
> > > igt_plane_set_fb(plane, &data->fb_reference);
> > > if (plane->type != DRM_PLANE_TYPE_CURSOR)
> > > igt_plane_set_position(plane, data->pos_x, data-
> > > >
> > > > pos_y);
> > > - igt_display_commit2(display, display->is_atomic ?
> > > COMMIT_ATOMIC : COMMIT_UNIVERSAL);
> > > + igt_display_commit2(display, COMMIT_ATOMIC);
> > >
> > > igt_pipe_crc_collect_crc(data->pipe_crc, &data-
> > > >ref_crc);
> > >
> > > @@ -305,7 +282,7 @@ static void prepare_fbs(data_t *data,
> > > igt_output_t *output,
> > > igt_plane_set_rotation(plane, IGT_ROTATION_0);
> > > if (plane->type != DRM_PLANE_TYPE_CURSOR)
> > > igt_plane_set_position(plane, data->pos_x, data-
> > > >
> > > > pos_y);
> > > - igt_display_commit2(display, display->is_atomic ?
> > > COMMIT_ATOMIC : COMMIT_UNIVERSAL);
> > > + igt_display_commit2(display, COMMIT_ATOMIC);
> > >
> > > /*
> > > * Prepare the plane with an non-rotated fb let the hw
> > > rotate it.
> > > @@ -318,35 +295,6 @@ static void prepare_fbs(data_t *data,
> > > igt_output_t *output,
> > > igt_plane_set_position(plane, data->pos_x, data-
> > > >
> > > > pos_y);
> > > }
> > >
> > > -static void cleanup_crtc(data_t *data, igt_output_t *output,
> > > igt_plane_t *plane)
> > > -{
> > > - igt_display_t *display = &data->display;
> > > -
> > > - igt_pipe_crc_free(data->pipe_crc);
> > > - data->pipe_crc = NULL;
> > > -
> > > - remove_fbs(data);
> > > -
> > > - igt_remove_fb(data->gfx_fd, &data->fb_modeset);
> > > -
> > > - /* XXX: see the note in prepare_crtc() */
> > > - if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
> > > - igt_plane_t *primary;
> > > -
> > > - primary = igt_output_get_plane_type(output,
> > > DRM_PLANE_TYPE_PRIMARY);
> > > - igt_plane_set_fb(primary, NULL);
> > > - }
> > > -
> > > - igt_plane_set_fb(plane, NULL);
> > > - igt_plane_set_rotation(plane, IGT_ROTATION_0);
> > > -
> > > - igt_display_commit2(display, COMMIT_UNIVERSAL);
> > > -
> > > - igt_output_set_pipe(output, PIPE_ANY);
> > > -
> > > - igt_display_commit(display);
> > > -}
> > > -
> > > static void wait_for_pageflip(int fd)
> > > {
> > > drmEventContext evctx = { .version = 2 };
> > > @@ -371,18 +319,11 @@ static void test_plane_rotation(data_t
> > > *data,
> > > int plane_type, bool test_bad_form
> > > enum pipe pipe;
> > > int valid_tests = 0;
> > > igt_crc_t crc_output;
> > > - enum igt_commit_style commit = COMMIT_LEGACY;
> > > int ret;
> > >
> > > - 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;
> > > -
> > > for_each_pipe_with_valid_output(display, pipe, output) {
> > > igt_plane_t *plane;
> > > int i;
> > > @@ -395,7 +336,7 @@ static void test_plane_rotation(data_t *data,
> > > int
> > > plane_type, bool test_bad_form
> > > plane = igt_output_get_plane_type(output,
> > > plane_type);
> > > igt_require(igt_plane_has_prop(plane,
> > > IGT_PLANE_ROTATION));
> > >
> > > - prepare_crtc(data, output, pipe, plane, commit);
> > > + prepare_crtc(data, output, pipe, plane);
> > >
> > > for (i = 0; i < num_rectangle_types; i++) {
> > > /* Unsupported on i915 */
> > > @@ -415,7 +356,7 @@ static void test_plane_rotation(data_t *data,
> > > int
> > > plane_type, bool test_bad_form
> > > if (data->rotation & (IGT_ROTATION_90 |
> > > IGT_ROTATION_270))
> > > igt_plane_set_size(plane, data-
> > > >
> > > > fb.height, data->fb.width);
> > >
> > > - ret = igt_display_try_commit2(display,
> > > commit);
> > > + ret = igt_display_try_commit2(display,
> > > COMMIT_ATOMIC);
> > > if (test_bad_format) {
> > > igt_assert_eq(ret, -EINVAL);
> > > continue;
> > > @@ -456,41 +397,28 @@ static void test_plane_rotation(data_t
> > > *data,
> > > int plane_type, bool test_bad_form
> > > }
> > >
> > > valid_tests++;
> > > - cleanup_crtc(data, output, plane);
> > > }
> > > igt_require_f(valid_tests, "no valid crtc/connector
> > > combinations found\n");
> > > }
> > >
> > > static void test_plane_rotation_exhaust_fences(data_t *data,
> > > igt_output_t
> > > *output,
> > > - int plane_type)
> > > + igt_plane_t
> > > *plane)
> > > {
> > > 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;
> > > - data_t data2[MAX_FENCES+1] = {};
> > > + struct igt_fb fb[MAX_FENCES+1] = {};
> > > unsigned int stride, size, w, h;
> > > uint32_t gem_handle;
> > > uint64_t total_aperture_size, total_fbs_size;
> > > int i, 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;
> > > @@ -509,7 +437,7 @@ static void
> > > test_plane_rotation_exhaust_fences(data_t *data,
> > > igt_require(total_fbs_size < total_aperture_size * 0.9);
> > >
> > > igt_plane_set_fb(plane, NULL);
> > > - igt_display_commit(display);
> > > + igt_display_commit2(display, COMMIT_ATOMIC);
> > >
> > > for (i = 0; i < MAX_FENCES + 1; i++) {
> > > gem_handle = gem_create(fd, size);
> > > @@ -522,20 +450,20 @@ static void
> > > test_plane_rotation_exhaust_fences(data_t *data,
> > > ret = (__kms_addfb(fd, gem_handle, w, h, stride,
> > > format, tiling, NULL,
> > > LOCAL_DRM_MODE_FB_MODIFIERS,
> > > - &data2[i].fb.fb_id));
> > > + &fb[i].fb_id));
> > > if (ret) {
> > > igt_warn("failed to create
> > > framebuffer\n");
> > > goto err_alloc;
> > > }
> > >
> > > - data2[i].fb.width = w;
> > > - data2[i].fb.height = h;
> > > - data2[i].fb.gem_handle = gem_handle;
> > > + fb[i].width = w;
> > > + fb[i].height = h;
> > > + fb[i].gem_handle = gem_handle;
> > >
> > > - igt_plane_set_fb(plane, &data2[i].fb);
> > > + igt_plane_set_fb(plane, &fb[i]);
> > > igt_plane_set_rotation(plane, IGT_ROTATION_0);
> > >
> > > - ret = igt_display_try_commit2(display, commit);
> > > + igt_display_commit2(display, COMMIT_ATOMIC);
> > > if (ret) {
> > > igt_warn("failed to commit unrotated
> > > fb\n");
> > > goto err_commit;
> > > @@ -544,7 +472,7 @@ static void
> > > test_plane_rotation_exhaust_fences(data_t *data,
> > > igt_plane_set_rotation(plane, IGT_ROTATION_90);
> > > igt_plane_set_size(plane, h, w);
> > >
> > > - igt_display_commit2(display, commit);
> > > + igt_display_commit2(display, COMMIT_ATOMIC);
> > > if (ret) {
> > > igt_warn("failed to commit hardware
> > > rotated
> > > fb: %i\n", ret);
> > > goto err_commit;
> > > @@ -557,19 +485,9 @@ err_alloc:
> > >
> > > i--;
> > > err_commit:
> > > - igt_plane_set_fb(plane, NULL);
> > > - igt_plane_set_rotation(plane, IGT_ROTATION_0);
> > > -
> > > - if (commit < COMMIT_ATOMIC)
> > > - igt_display_commit2(display, commit);
> > > -
> > > - igt_output_set_pipe(output, PIPE_NONE);
> > > - igt_display_commit2(display, display->is_atomic ?
> > > COMMIT_ATOMIC : COMMIT_LEGACY);
> > > -
> > > for (; i >= 0; i--)
> > > - igt_remove_fb(fd, &data2[i].fb);
> > > + igt_remove_fb(fd, &fb[i]);
> > >
> > > - kmstest_restore_vt_mode();
> > > igt_assert_eq(ret, 0);
> > > }
> > >
> > > @@ -726,9 +644,10 @@ igt_main
> > > igt_require(gen >= 9);
> > >
> > > for_each_pipe_with_valid_output(&data.display,
> > > pipe,
> > > output) {
> > > - igt_output_set_pipe(output, pipe);
> > > + igt_plane_t *primary =
> > > &data.display.pipes[pipe].planes[0];
> > > + prepare_crtc(&data, output, pipe,
> > > primary);
> > >
> > > - test_plane_rotation_exhaust_fences(&data
> > > ,
> > > output, DRM_PLANE_TYPE_PRIMARY);
> > > + test_plane_rotation_exhaust_fences(&data
> > > ,
> > > output, primary);
> > >
> > > valid_tests++;
> > > break;
>
--
Mika Kahola - Intel OTC
More information about the igt-dev
mailing list