Re: [PATCH i-g-t] tests: atomic: add test to verify page flip event emissions

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Apr 22, 2016 at 02:27:28PM +0100, Lionel Landwerlin wrote:
> On 22/04/16 13:59, Daniel Vetter wrote:
> >On Thu, Apr 21, 2016 at 05:01:31PM +0100, Lionel Landwerlin wrote:
> >>It seems we don't have a test verifying events with atomic commits
> >>yet. Here is a first step.
> >>
> >>Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> >Feel like also making a testcase variant for the other cases of crtc's
> >ACTIVE property?
> >- off -> off: kernel should reject event generation
> >- off -> on: kernel should generate event, and the frame counter should be
> >   just a few frames less (at most) than what you get from the vblank ioctl
> >   right after the modeset completes
> >- on -> off: same as off -> on but in reverse
> >
> >Would be awesome ...
> >-Daniel
> 
> Sure, I will add more test for those cases.
> 
> Though right now, I would like to know whether this test seems wrong or
> broken in any way.
> As I mentioned on IRC, I'm not getting any event when doing atomic commits
> and this test fails.
> I'm concerned that our driver doesn't behave properly with regards to events
> with atomic enabled.

Oh I forgot that. lgtm (without checking details, just top-level
semantics), one comment below.


> >>+static void plane_pageflip_events(struct kms_atomic_crtc_state *crtc,
> >>+				  struct kms_atomic_plane_state *plane_old)
> >>+{
> >>+	struct drm_mode_modeinfo *mode = crtc->mode.data;
> >>+	struct kms_atomic_plane_state plane = *plane_old;
> >>+	uint32_t format = plane_get_igt_format(&plane);
> >>+	drmModeAtomicReq *req = drmModeAtomicAlloc();
> >>+	struct igt_fb fb[2];
> >>+	int i, pageflip_count = 0, vblank_count = 0;
> >>+	int flags = DRM_MODE_PAGE_FLIP_EVENT;
> >>+
> >>+	igt_require(format != 0);
> >>+
> >>+	for (i = 0; i < ARRAY_SIZE(fb); i++)
> >>+		igt_create_pattern_fb(plane.state->desc->fd,
> >>+				      plane.crtc_w, plane.crtc_h,
> >>+				      format, I915_TILING_NONE, &fb[i]);
> >>+
> >>+	plane.src_x = 0;
> >>+	plane.src_y = 0;
> >>+	plane.src_w = mode->hdisplay << 16;
> >>+	plane.src_h = mode->vdisplay << 16;
> >>+	plane.crtc_x = 0;
> >>+	plane.crtc_y = 0;
> >>+	plane.crtc_w = mode->hdisplay;
> >>+	plane.crtc_h = mode->vdisplay;
> >>+	plane.crtc_id = crtc->obj;
> >>+	plane.fb_id = fb[0].fb_id;
> >>+
> >>+	/* Flip the primary plane using the atomic API, and double-check
> >>+	 * state is what we think it should be. */
> >>+	crtc_commit_atomic(crtc, &plane, req, ATOMIC_RELAX_NONE);
> >>+
> >>+	get_events(crtc, &pageflip_count, &vblank_count, 0);
> >>+	igt_assert_eq(1, pageflip_count);
> >>+
> >>+	drmModeAtomicFree(req);
> >>+	req = drmModeAtomicAlloc();
> >>+
> >>+	/* Change the framebuffer on the plane, we should get one
> >>+	 * pageflip event. */
> >>+	plane.fb_id = fb[1].fb_id;
> >>+	plane_commit_atomic(&plane, req, flags, ATOMIC_RELAX_NONE);
> >>+	get_events(crtc, &pageflip_count, &vblank_count, 20);
> >>+	igt_assert_eq(2, pageflip_count);
> >>+
> >>+	/* No change, page flip event count should remain the same. */
> >>+	plane.fb_id = fb[1].fb_id;
> >>+	plane_commit_atomic(&plane, req, flags, ATOMIC_RELAX_NONE);
> >>+	get_events(crtc, &pageflip_count, &vblank_count, 20);
> >>+	igt_assert_eq(2, pageflip_count);

no-op atomic updates should still result in successful event generation.
But there's no guarantee whether we'll short-circuit the entire thing, or
whether we'll do a dummy flip and hence the vblank count will be
incremented by 1 compared to the last test.

Otherwise the assumption of the test that every atomic commit should
result in an even (if you ask for it) is correct.
-Daniel

> >>+	/* Change back the plane's framebuffer to its original one, we
> >>+	 * should get a page flip event. */
> >>+	plane.fb_id = fb[0].fb_id;
> >>+	plane_commit_atomic(&plane, req, flags, ATOMIC_RELAX_NONE);
> >>+	get_events(crtc, &pageflip_count, &vblank_count, 20);
> >>+	igt_assert_eq(3, pageflip_count);
> >>+
> >>+	drmModeAtomicFree(req);
> >>+}
> >>+
> >>  igt_main
> >>  {
> >>  	struct kms_atomic_desc desc;
> >>@@ -1373,6 +1479,17 @@ igt_main
> >>  		atomic_state_free(scratch);
> >>  	}
> >>+	igt_subtest("atomic_pageflip_events") {
> >>+		struct kms_atomic_state *scratch = atomic_state_dup(current);
> >>+		struct kms_atomic_crtc_state *crtc = find_crtc(scratch, true);
> >>+		struct kms_atomic_plane_state *plane =
> >>+			find_plane(scratch, PLANE_TYPE_PRIMARY, crtc);
> >>+
> >>+		igt_require(plane);
> >>+		plane_pageflip_events(crtc, plane);
> >>+		atomic_state_free(scratch);
> >>+	}
> >>+
> >>  	atomic_state_free(current);
> >>  	igt_fixture
> >>-- 
> >>2.8.0.rc3.226.g39d4020
> >>
> >>_______________________________________________
> >>Intel-gfx mailing list
> >>Intel-gfx@lists.freedesktop.org
> >>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx