[Intel-gfx] [PATCH i-g-t v5 7/7] kms_flip: Change __wait_for_vblank to use helper function.

[Marius Vlad]

On Tue, May 17, 2016 at 10:48:02AM -0400, Robert Foss wrote:
> 
> 
> On 2016-05-17 06:32 AM, Marius Vlad wrote:
> >On Mon, May 16, 2016 at 09:38:32AM -0400, robert.foss at collabora.com wrote:
> >>From: Robert Foss <robert.foss at collabora.com>
> >>
> >>Change __wait_for_vblank() to use kmstest_get_vbl_flag() helper function.
> >>
> >>Signed-off-by: Robert Foss <robert.foss at collabora.com>
> >>---
> >>  tests/kms_flip.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/tests/kms_flip.c b/tests/kms_flip.c
> >>index eda2fcc..0ecca07 100644
> >>--- a/tests/kms_flip.c
> >>+++ b/tests/kms_flip.c
> >>@@ -481,15 +481,15 @@ static int __wait_for_vblank(unsigned int flags, int crtc_idx,
> >>  {
> >>  	drmVBlank wait_vbl;
> >>  	int ret;
> >>-	unsigned crtc_idx_mask;
> >>+	uint32_t pipe_id_flag;
> >>  	bool event = !(flags & TEST_VBLANK_BLOCK);
> >>
> >>  	memset(&wait_vbl, 0, sizeof(wait_vbl));
> >>
> >>-	crtc_idx_mask = crtc_idx << DRM_VBLANK_HIGH_CRTC_SHIFT;
> >>-	igt_assert(!(crtc_idx_mask & ~DRM_VBLANK_HIGH_CRTC_MASK));
> >If crtc_idx is 1 (pipe B), crtc_idx_mask = (1 << DRM_VBLANK_HIGH_CRTC_SHIFT) = 2.
> >>+	pipe_id_flag = kmstest_get_vbl_flag(crtc_idx);
> >>+	igt_assert(!(pipe_id_flag & ~DRM_VBLANK_HIGH_CRTC_MASK));
> >If crtc_idx is 1 (pipe B), kmstest_get_vbl_flag(crtc_idx) = DRM_VBLANK_SECONDARY = 0x20000000
> >
> >And the assertion fails as !(0x20000000 & ~0x0000003e) = 0.
> >
> >Should kmstest_get_vbl_flag() always return pipe_id << 1?
> 
> From re-reading the pipe id parsing code in drm_irq.c drm_wait_vblank()
> it would seem to me that the assertion is incorrect, specifically for the
> case of _DRM_VBLANK_SECONDARY.
> 
> Supplying the flag _DRM_VBLANK_SECONDARY and setting the high crtc id field
> to 1 fboth seem to be valid ways to communicate the same thing.
Indeed.
> 
> 	flags = vblwait->request.type & _DRM_VBLANK_FLAGS_MASK;
> 	high_pipe = (vblwait->request.type &_DRM_VBLANK_HIGH_CRTC_MASK);
> 	if (high_pipe)
> 		pipe = high_pipe >> _DRM_VBLANK_HIGH_CRTC_SHIFT;
> 	else
> 		pipe = flags & _DRM_VBLANK_SECONDARY ? 1 : 0;
> 
> 
> Maybe adding correct and more thorough assertion to kmstest_get_vbl_flag()
> and removing the failing assertion is the way forward.
Sounds reasonable to me, though I assume that the assertion had/has
some point in determining that you have a valid crtc_idx.
> 
> >
> >>
> >>-	wait_vbl.request.type = crtc_idx_mask;
> >>+	wait_vbl.request.type = pipe_id_flag;
> >>  	if (flags & TEST_VBLANK_ABSOLUTE)
> >>  		wait_vbl.request.type |= DRM_VBLANK_ABSOLUTE;
> >>  	else
> >>--
> >>2.7.4
> >>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20160518/5adb3911/attachment.sig>