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
_______________________________________________
xorg-devel mailing list
[email protected]
http://lists.x.org/mailman/listinfo/xorg-devel
