| Message ID | 1485955089-30267-2-git-send-email-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 01-02-17 om 14:18 schreef Mika Kahola: > Add an option to force atomic commits to do commits with TEST_ONLY flag > first before doing the actual commit. > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > --- > lib/igt_kms.c | 18 +++++++++++++++++- > lib/igt_kms.h | 1 + > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c > index 4ba6316..c513ef8 100644 > --- a/lib/igt_kms.c > +++ b/lib/igt_kms.c > @@ -2453,7 +2453,23 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_ > igt_atomic_prepare_connector_commit(output, req); > } > > - ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data); > + if (display->force_test_atomic && > + !(flags & DRM_MODE_ATOMIC_TEST_ONLY)) { > + unsigned int test_flags = flags & ~DRM_MODE_PAGE_FLIP_EVENT; > + int test_ret; > + > + test_flags |= DRM_MODE_ATOMIC_TEST_ONLY; > + > + test_ret = drmModeAtomicCommit(display->drm_fd, req, test_flags, user_data); > + ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data); > + > + if (test_ret) > + igt_assert_eq(test_ret, ret); > + else > + igt_assert(ret != -EINVAL); > + } else > + ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data); > + > drmModeAtomicFree(req); > return ret; > > diff --git a/lib/igt_kms.h b/lib/igt_kms.h > index 2562618..e45fc21 100644 > --- a/lib/igt_kms.h > +++ b/lib/igt_kms.h > @@ -338,6 +338,7 @@ struct igt_display { > igt_pipe_t *pipes; > bool has_cursor_plane; > bool is_atomic; > + bool force_test_atomic; > }; > > void igt_display_init(igt_display_t *display, int drm_fd); Sorry, didn't notice it before but force_test_atomic is a big behavioral switch that might force other tests to potentially fail. If this a subtest fails and the flag is not explictly cleared. This is also why you need to set it separately even if false. I really wish there was a igt_subtest_exit_handler, I could remove a lot of boilerplate in the KMS tests with that. My personal wishlist for it: - Complete all sw_sync fences - Remove all allocated pipe_crc's, so next test won't fail due to fd already being assigned. - Clean igt_display_t with a synchronous commit. - Clear any pending drm events without blocking. - Remove any leftover framebuffers allocated in the subtest. This may fail if fb's become invalid, like in the kms_rmfb test. Be careful with dereferencing fb's, the previous pointer may not be valid any more if it was allocated on the stack. It should probably require an extra memory allocation during fb alloc to keep track. This would allow us to kill most of the teardown boilerplate in igt kms tests, and increases reliability when a subtest fails, the next subtest is a lot more likely to succeed. ~Maarten
On Wed, 2017-03-08 at 15:59 +0100, Maarten Lankhorst wrote: > Op 01-02-17 om 14:18 schreef Mika Kahola: > > > > Add an option to force atomic commits to do commits with TEST_ONLY > > flag > > first before doing the actual commit. > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > --- > > lib/igt_kms.c | 18 +++++++++++++++++- > > lib/igt_kms.h | 1 + > > 2 files changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c > > index 4ba6316..c513ef8 100644 > > --- a/lib/igt_kms.c > > +++ b/lib/igt_kms.c > > @@ -2453,7 +2453,23 @@ static int igt_atomic_commit(igt_display_t > > *display, uint32_t flags, void *user_ > > igt_atomic_prepare_connector_commit(output, req); > > } > > > > - ret = drmModeAtomicCommit(display->drm_fd, req, flags, > > user_data); > > + if (display->force_test_atomic && > > + !(flags & DRM_MODE_ATOMIC_TEST_ONLY)) { > > + unsigned int test_flags = flags & > > ~DRM_MODE_PAGE_FLIP_EVENT; > > + int test_ret; > > + > > + test_flags |= DRM_MODE_ATOMIC_TEST_ONLY; > > + > > + test_ret = drmModeAtomicCommit(display->drm_fd, > > req, test_flags, user_data); > > + ret = drmModeAtomicCommit(display->drm_fd, req, > > flags, user_data); > > + > > + if (test_ret) > > + igt_assert_eq(test_ret, ret); > > + else > > + igt_assert(ret != -EINVAL); > > + } else > > + ret = drmModeAtomicCommit(display->drm_fd, req, > > flags, user_data); > > + > > drmModeAtomicFree(req); > > return ret; > > > > diff --git a/lib/igt_kms.h b/lib/igt_kms.h > > index 2562618..e45fc21 100644 > > --- a/lib/igt_kms.h > > +++ b/lib/igt_kms.h > > @@ -338,6 +338,7 @@ struct igt_display { > > igt_pipe_t *pipes; > > bool has_cursor_plane; > > bool is_atomic; > > + bool force_test_atomic; > > }; > > > > void igt_display_init(igt_display_t *display, int drm_fd); > Sorry, didn't notice it before but force_test_atomic is a big > behavioral switch that > might force other tests to potentially fail. If this a subtest fails > and the flag is > not explictly cleared. This is also why you need to set it separately > even if false. So for this test it would be adequate if we clear this flag in case of error? I'll spin another round of this patch series. It also requires some rebasing too. > > I really wish there was a igt_subtest_exit_handler, I could remove a > lot of boilerplate in the KMS tests with that. > > My personal wishlist for it: > - Complete all sw_sync fences > - Remove all allocated pipe_crc's, so next test won't fail due to fd > already being assigned. > - Clean igt_display_t with a synchronous commit. > - Clear any pending drm events without blocking. > - Remove any leftover framebuffers allocated in the subtest. This may > fail if fb's become > invalid, like in the kms_rmfb test. > This would be nice. Asserts may leave things in unstable state. > Be careful with dereferencing fb's, the previous pointer may not be > valid any more if it > was allocated on the stack. It should probably require an extra > memory allocation during > fb alloc to keep track. > > This would allow us to kill most of the teardown boilerplate in igt > kms tests, and increases > reliability when a subtest fails, the next subtest is a lot more > likely to succeed. > > ~Maarten >
diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 4ba6316..c513ef8 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -2453,7 +2453,23 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_ igt_atomic_prepare_connector_commit(output, req); } - ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data); + if (display->force_test_atomic && + !(flags & DRM_MODE_ATOMIC_TEST_ONLY)) { + unsigned int test_flags = flags & ~DRM_MODE_PAGE_FLIP_EVENT; + int test_ret; + + test_flags |= DRM_MODE_ATOMIC_TEST_ONLY; + + test_ret = drmModeAtomicCommit(display->drm_fd, req, test_flags, user_data); + ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data); + + if (test_ret) + igt_assert_eq(test_ret, ret); + else + igt_assert(ret != -EINVAL); + } else + ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data); + drmModeAtomicFree(req); return ret; diff --git a/lib/igt_kms.h b/lib/igt_kms.h index 2562618..e45fc21 100644 --- a/lib/igt_kms.h +++ b/lib/igt_kms.h @@ -338,6 +338,7 @@ struct igt_display { igt_pipe_t *pipes; bool has_cursor_plane; bool is_atomic; + bool force_test_atomic; }; void igt_display_init(igt_display_t *display, int drm_fd);
Add an option to force atomic commits to do commits with TEST_ONLY flag first before doing the actual commit. Signed-off-by: Mika Kahola <mika.kahola@intel.com> --- lib/igt_kms.c | 18 +++++++++++++++++- lib/igt_kms.h | 1 + 2 files changed, 18 insertions(+), 1 deletion(-)