[Intel-gfx] [PATCH 2/3] ddx/dri2: Repair broken pageflip swap scheduling.

[Mario Kleiner]

On 11.12.12 00:00, Jesse Barnes wrote:
> On Mon, 10 Dec 2012 10:48:29 -0800
> Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
>
>>> On 15.10.12 16:52, Chris Wilson wrote:
>>>   > On Mon, 15 Oct 2012 16:46:52 +0200, Mario Kleiner
>>> <mario.kleiner at tuebingen.mpg.de> wrote:
>>>   >> Hi Chris,
>>>   >>
>>>   >> can you please check & merge at least the first two patches of the
>>>   >> series into the intel ddx?
>>>
>>> Thanks for the quick reply.
>>>
>>>   >
>>>   > The first is along the right path, but I think you want to base the
>>>   > split on divisor==0.
>>>
>>> I don't think so, unless i misunderstand what you mean? The optimization
>>> of I830DRI2ScheduleFlip()'ing immediately should only happen in the case
>>> where current_msc is >= target_msc, ie., if the client really requests
>>> swap at next possible vblank, regardless what divisor is, once we've
>>> entered the whole ...
>>>
>>> if (divisor == 0 || current_msc < *target_msc) {
>>>
>>> ... block. Checking for divisor == 0 would be a nice little cleanup if
>>> only either (divisor == 0) or (current_msc < *target_msc) could be true.
>>> But it can happen that both (divisor == 0) and (current_msc <
>>> *target_msc) and then a check for (divisor == 0) wouldn't tell you if
>>> target_msc is in the future and the kernel vblank mechanism should
>>> schedule swap in the future, or if it is time for an immediate flip.
>>>
>>> Also i tested with various distances between successive swap and with
>>> divisor 0 vs. non-zero, so at least it works as advertised with the
>>> current patch.
>>>
>>> So that patch should be ok.
>>
>> Yeah I don't understand the flip schedule at the top there either; if
>> target_msc is out in the future, why would we schedule a page flip
>> immediately just because divisor == 0?
>>
>> Maybe it should look like this instead?
>>
>> if (divisor == 0 || current_msc < *target_msc) {
>> 	if (divisor == 0 && current_msc >= *target_msc)
>> 		if (flip && I830DRI2ScheduleFlip(intel,	draw, swap_info))
>> 			return TRUE;
>
> commit 2ab29a1688cd313768d928e87e145570f35b4a70
> Author: Jesse Barnes <jbarnes at virtuousgeek.org>
> Date:   Mon Dec 10 14:55:32 2012 -0800
>
>      dri2: don't schedule a flip prematurely at ScheduleSwap time
>
> Mario, can you make sure this works for you?
>

Looks good for my purposes, ie., should fix glXSwapBuffersMscOML(), 
which was my main point of grief, thanks a lot! I'll retest soonish for 
extra paranoia. The divisor == 0 check is not really needed for the 
logic to work, but doesn't hurt and maybe still nice to leave there for 
readability to clarify the condition when the optimization should work.

However, it doesn't fix some cases for normal glXSwapBuffers() with a 
swapinterval > 1. That requires always updating *target_msc properly, 
and the early exit if the optimization is taken prevents that. Also the 
swap_info->frame doesn't get updated in this case, which effectively 
disables/skips some correctness test in I830DRI2FlipEventHandler().

If you want to fix those as well you'll basically end up with my 
original patch, which moves the optimization a few lines down in the 
function and adds updating of those two variables, minus lots of 
comments in the code and commit message.

But i'm happy, i mostly need glXSwapBuffersMscOML() and the pageflip 
timestamping to work properly.
-mario