Re: [PATCH i-g-t v3 1/7] lib/igt_kms: Add forcing TEST_ONLY for atomic commits

[Mika Kahola <mika.kahola@intel.com>]

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
> 
-- 
Mika Kahola - Intel OTC

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