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

From: Robert Foss <robert.foss@collabora.com>
To: daniel.vetter@ffwll.ch, daniel.stone@collabora.com,
	ville.syrjala@linux.intel.com, jani.nikula@intel.com,
	chris@chris-wilson.co.uk, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH i-g-t v5 7/7] kms_flip: Change __wait_for_vblank to use helper function.
Date: Tue, 17 May 2016 10:48:02 -0400	[thread overview]
Message-ID: <573B2F22.4090804@collabora.com> (raw)
In-Reply-To: <20160517103227.GA8717@mcvlad-wk.rb.intel.com>

On 2016-05-17 06:32 AM, Marius Vlad wrote:
> On Mon, May 16, 2016 at 09:38:32AM -0400, robert.foss@collabora.com wrote:
>> From: Robert Foss <robert.foss@collabora.com>
>>
>> Change __wait_for_vblank() to use kmstest_get_vbl_flag() helper function.
>>
>> Signed-off-by: Robert Foss <robert.foss@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.

	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.

>
>>
>> -	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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx