[PATCH xf86-video-intel 2/2] Fix handling of target_msc, divisor, remainder in I830DRI2ScheduleSwap()

[Mario Kleiner]

On Mar 8, 2010, at 8:34 PM, Jesse Barnes wrote:
> Ok just pushed these fixes; omlsync seems to do the right thing now
> with both waits and swaps.
>

Sorry to torture you further, almost there ;-) -- the devil is in the  
details.

In I830DRI2ScheduleWaitMSC():

At the end, in this if statement...

         if ((current_msc % divisor) > remainder)
             vbl.request.sequence += divisor;

... the comparison should be >= instead of > , that is:

         if ((current_msc % divisor) >= remainder)

I reread the spec and realized the true meaning of this little  
snippet "...Otherwise, the function will block until the MSC value is  
incremented to a value such that MSC % <divisor> = <remainder>..."

It doesn't say "until the msc *is* such that msc % divisor =  
remainder", but it says "until the *msc is **incremented** to a  
value*". That means they don't want it to return immediately if the  
current msc already satisfies the constraint, but they want it to  
return basically at the start of the vblank interval when the msc  
reaches a value that satisfies the constraint. The idea is to  
synchronize the client's execution to the vblank interval. If a
client just wants to wait for a certain msc without precise sync to  
the vblank interval, it can simply pass a divisor==0 and then will  
get that behaviour.

Changing the comparison from a > to a >= above achieves that goal.  
This makes lots of sense if i think about how i would actually use  
that function in my toolkit or similar apps. I'd mostly want it to  
synchronize to the exact *start* of certain video refesh cycles.

Then we should add this check to the if(divisor == 0 || current_msc <  
target_msc) { .... } branch:

                 if (current_msc >= target_msc)
                         target_msc = current_msc;

This is similar to the check in I830DRI2ScheduleSwap. It guarantees  
that the target_msc can't fall behind the current_msc. This is  
important for all scheduled vblank events because the DRM will do the  
wrong thing if the requested vblank count is more than 2^23 counts  
behind the current vblank count. Events would get stuck forever if  
this happened due to a 32 bit wraparound, not good.


What else? I rethought the idea of virtualizing the msc into a 64 bit  
value from the 32 count of the kernel. It is a bit more tricky than  
one would expect, so probably not a quick thing do to correctly - and  
not a thing to do now.

But we could add some simple masking to the very top of  
I830DRI2ScheduleSwap() and I830DRI2ScheduleWaitMSC() to truncate all  
msc related input values from the server to 32 bits, ie.

in I830DRI2ScheduleWaitMSC()

target_msc &= 0xffffffff;
divisor &= 0xffffffff;
remainder &= 0xffffffff;

in I830DRI2ScheduleSwap()

*target_msc &= 0xffffffff;
divisor &= 0xffffffff;
remainder &= 0xffffffff;

At least the few simple cases my brain can handle with "paper &  
pencil testing" seem to do the right thing if a 32 bit vblank counter  
wraparound happens. At worst, there would be a small glitch every  
time the counter wraps around - about once every 4-12 months.  
Inbetween everything should work. I doubt that an app will regularly  
schedule swaps or waits multiple months ahead of time and still  
expect it to work perfectly ;-).

Finally in I830DRI2ScheduleWaitMSC() the error handling is a bit  
inconsistent. Sometimes it returns failure to calling code, which  
would provoke a client hang due to a missing _XReply, sometimes it  
recovers and "unstucks" the client by providing a fake response.  
Similar to the blit_fallback: path in I830DRI2ScheduleSwap() you  
could maybe have a common error handler that at least calls  
DRI2WaitMSCComplete(client, draw, 0, 0, 0); to prevent the client  
from hanging?

Ok, i'm hopefully running out of more ideas for now ;-)
-mario