Re: [PATCH xf86-video-intel 2/2] Fix handling of target_msc, divisor, remainder in I830DRI2ScheduleSwap()
On Mon, 8 Mar 2010 01:23:11 +0100 Mario Kleiner mario.klei...@tuebingen.mpg.de wrote: On Mar 7, 2010, at 6:18 PM, Jesse Barnes wrote: Arg, I did botch that patch. And of course I only tested the swap buffers behavior and not OML's WaitMSC so I didn't catch it. I'll improve the test and push the fix. No problem. Just fyi: I noticed you added a test in I830DRI2ScheduleSwap() that outputs X_Warnings if a swap is scheduled more than 100 vblanks ahead. At least for the users of my toolkit, scheduling a swap more than 100 vblanks ahead wouldn't be something noteworthy but quite a typical case of normal use of the system. At the end of a work day some users would probably find ten thousands of such warnings in some log, assuming those warnings get logged at the normal log level. So this usage case is less weird than one would think :-) Ok just pushed these fixes; omlsync seems to do the right thing now with both waits and swaps. -- Jesse Barnes, Intel Open Source Technology Center ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xf86-video-intel 2/2] Fix handling of target_msc, divisor, remainder in I830DRI2ScheduleSwap()
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 = 0x;
divisor = 0x;
remainder = 0x;
in I830DRI2ScheduleSwap()
*target_msc = 0x;
divisor = 0x;
remainder = 0x;
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
xorg-devel@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xf86-video-intel 2/2] Fix handling of target_msc, divisor, remainder in I830DRI2ScheduleSwap()
On Mar 5, 2010, at 10:09 PM, Jesse Barnes wrote:
I just fixed these up and pushed them into the tree along with another
fix for handling offscreen drawables better. Tests indicate that OML
swap divisor/remainder stuff is working correctly now.
Thanks for doing that. But stuff got seriously garbled in
I830ScheduleWaitMSC() inside src/i830_dri.c - this won't work
correctly for any glXWaitForMscOML(target_msc, divisor, remainder)
call, except for the special cases with divisor == 0!
This passage...
/*
* If divisor is zero, or current_msc is smaller than
target_msc,
* we just need to make sure target_msc passes before
waking up the
* client.
*/
if (divisor == 0) {
...
should read as...
if (divisor == 0 || current_msc target_msc) {
This passage ...
/*
* If the calculated remainder and the condition isn't
satisified, it
* means we've passed the last point where seq % divisor ==
remainder,
* so we need to wait for the next time that will happen.
*/
if ((current_msc % divisor) != remainder)
vbl.request.sequence += divisor;
... should be replaced by ...
vbl.request.sequence = current_msc - (current_msc % divisor) +
remainder;
/*
* If calculated remainder is larger than requested remainder,
it means
* we've passed the last point where seq % divisor == remainder,
so we need
* to wait for the next time that will happen.
*/
if ((current_msc % divisor) remainder)
vbl.request.sequence += divisor;
Other than that, it's fine.
Oh and in I830DRI2ScheduleSwap() , this statement ...
if (pipe 0)
vbl.request.type |= DRM_VBLANK_SECONDARY;
... accidentally got copied twice into the if () clause, which is not
harmful, but a bit redundant :-)
best,
-mario
*
Mario Kleiner
Max Planck Institute for Biological Cybernetics
Spemannstr. 38
72076 Tuebingen
Germany
e-mail: mario.klei...@tuebingen.mpg.de
office: +49 (0)7071/601-1623
fax:+49 (0)7071/601-616
www:http://www.kyb.tuebingen.mpg.de/~kleinerm
*
For a successful technology, reality must take precedence
over public relations, for Nature cannot be fooled.
(Richard Feynman)
___
xorg-devel mailing list
xorg-devel@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xf86-video-intel 2/2] Fix handling of target_msc, divisor, remainder in I830DRI2ScheduleSwap()
On Sun, 7 Mar 2010 09:15:46 +0100
Mario Kleiner mario.klei...@tuebingen.mpg.de wrote:
On Mar 5, 2010, at 10:09 PM, Jesse Barnes wrote:
I just fixed these up and pushed them into the tree along with another
fix for handling offscreen drawables better. Tests indicate that OML
swap divisor/remainder stuff is working correctly now.
Thanks for doing that. But stuff got seriously garbled in
I830ScheduleWaitMSC() inside src/i830_dri.c - this won't work
correctly for any glXWaitForMscOML(target_msc, divisor, remainder)
call, except for the special cases with divisor == 0!
This passage...
/*
* If divisor is zero, or current_msc is smaller than
target_msc,
* we just need to make sure target_msc passes before
waking up the
* client.
*/
if (divisor == 0) {
...
should read as...
if (divisor == 0 || current_msc target_msc) {
This passage ...
/*
* If the calculated remainder and the condition isn't
satisified, it
* means we've passed the last point where seq % divisor ==
remainder,
* so we need to wait for the next time that will happen.
*/
if ((current_msc % divisor) != remainder)
vbl.request.sequence += divisor;
... should be replaced by ...
vbl.request.sequence = current_msc - (current_msc % divisor) +
remainder;
/*
* If calculated remainder is larger than requested remainder,
it means
* we've passed the last point where seq % divisor == remainder,
so we need
* to wait for the next time that will happen.
*/
if ((current_msc % divisor) remainder)
vbl.request.sequence += divisor;
Other than that, it's fine.
Oh and in I830DRI2ScheduleSwap() , this statement ...
if (pipe 0)
vbl.request.type |= DRM_VBLANK_SECONDARY;
... accidentally got copied twice into the if () clause, which is not
harmful, but a bit redundant :-)
Arg, I did botch that patch. And of course I only tested the swap
buffers behavior and not OML's WaitMSC so I didn't catch it. I'll
improve the test and push the fix.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
___
xorg-devel mailing list
xorg-devel@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xf86-video-intel 2/2] Fix handling of target_msc, divisor, remainder in I830DRI2ScheduleSwap()
On Mar 7, 2010, at 6:18 PM, Jesse Barnes wrote: Arg, I did botch that patch. And of course I only tested the swap buffers behavior and not OML's WaitMSC so I didn't catch it. I'll improve the test and push the fix. No problem. Just fyi: I noticed you added a test in I830DRI2ScheduleSwap() that outputs X_Warnings if a swap is scheduled more than 100 vblanks ahead. At least for the users of my toolkit, scheduling a swap more than 100 vblanks ahead wouldn't be something noteworthy but quite a typical case of normal use of the system. At the end of a work day some users would probably find ten thousands of such warnings in some log, assuming those warnings get logged at the normal log level. So this usage case is less weird than one would think :-) -mario ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xf86-video-intel 2/2] Fix handling of target_msc, divisor, remainder in I830DRI2ScheduleSwap()
On Mon, 8 Mar 2010 01:23:11 +0100 Mario Kleiner mario.klei...@tuebingen.mpg.de wrote: On Mar 7, 2010, at 6:18 PM, Jesse Barnes wrote: Arg, I did botch that patch. And of course I only tested the swap buffers behavior and not OML's WaitMSC so I didn't catch it. I'll improve the test and push the fix. No problem. Just fyi: I noticed you added a test in I830DRI2ScheduleSwap() that outputs X_Warnings if a swap is scheduled more than 100 vblanks ahead. At least for the users of my toolkit, scheduling a swap more than 100 vblanks ahead wouldn't be something noteworthy but quite a typical case of normal use of the system. At the end of a work day some users would probably find ten thousands of such warnings in some log, assuming those warnings get logged at the normal log level. So this usage case is less weird than one would think :-) Ok, I was worried about that. I'll remove the message; I guess there's no easy way to tell between legitimate infrequent swaps and bad MSC requests... -- Jesse Barnes, Intel Open Source Technology Center ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xf86-video-intel 2/2] Fix handling of target_msc, divisor, remainder in I830DRI2ScheduleSwap()
On Sun, 21 Feb 2010 18:45:48 +0100 Mario Kleiner mario.klei...@tuebingen.mpg.de wrote: The current code in I830DRI2ScheduleSwap() only schedules the correct vblank events for the case divisor == 0, i.e., the simple glXSwapBuffers() case. In a glXSwapBuffersMscOML() request, divisor can be 0, which would go wrong. This modified code should handle target_msc, divisor, remainder and the different cases defined in the OML_sync_control extension correctly for the divisor 0 case. It also tries to make sure that the effective framecount of swap satisfies all constraints, taking the 1 frame delay in pageflipping mode and possible delays in blitting/exchange mode due to DRM_VBLANK_NEXTONMISS into account. The swap_interval logic in the X-Servers DRI2SwapBuffers() call expects the returned swap_target from the DDX to be reasonably accurate, otherwise implementation of swap_interval for the glXSwapBuffers() as defined in the SGI_swap_interval extension may become unreliable. For non-pageflipped mode, the returned swap_target is always correct due to the adjustments done by drmWaitVBlank(), as DRM_VBLANK_NEXTONMISS is set. In pageflipped mode, DRM_VBLANK_NEXTONMISS can't be used without severe impact on performance, so the code in I830DRI2ScheduleSwap() must make manual adjustments to the returned vbl.reply.sequence number. This patch adds the needed adjustments. Signed-off-by: Mario Kleiner mario.klei...@tuebingen.mpg.de --- I just fixed these up and pushed them into the tree along with another fix for handling offscreen drawables better. Tests indicate that OML swap divisor/remainder stuff is working correctly now. Thanks! -- Jesse Barnes, Intel Open Source Technology Center ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xf86-video-intel 2/2] Fix handling of target_msc, divisor, remainder in I830DRI2ScheduleSwap()
The current code in I830DRI2ScheduleSwap() only schedules the
correct vblank events for the case divisor == 0, i.e., the
simple glXSwapBuffers() case.
In a glXSwapBuffersMscOML() request, divisor can be 0, which would go
wrong.
This modified code should handle target_msc, divisor, remainder and
the different cases defined in the OML_sync_control extension
correctly for the divisor 0 case.
It also tries to make sure that the effective framecount of swap satisfies
all constraints, taking the 1 frame delay in pageflipping mode and possible
delays in blitting/exchange mode due to DRM_VBLANK_NEXTONMISS into account.
The swap_interval logic in the X-Servers DRI2SwapBuffers() call expects
the returned swap_target from the DDX to be reasonably accurate, otherwise
implementation of swap_interval for the glXSwapBuffers() as defined in the
SGI_swap_interval extension may become unreliable.
For non-pageflipped mode, the returned swap_target is always correct due to the
adjustments done by drmWaitVBlank(), as DRM_VBLANK_NEXTONMISS is set.
In pageflipped mode, DRM_VBLANK_NEXTONMISS can't be used without
severe impact on performance, so the code in I830DRI2ScheduleSwap()
must make manual adjustments to the returned vbl.reply.sequence number.
This patch adds the needed adjustments.
Signed-off-by: Mario Kleiner mario.klei...@tuebingen.mpg.de
---
src/i830_dri.c | 110 +++
1 files changed, 70 insertions(+), 40 deletions(-)
diff --git a/src/i830_dri.c b/src/i830_dri.c
index d5e085a..f3cd739 100644
--- a/src/i830_dri.c
+++ b/src/i830_dri.c
@@ -586,6 +586,7 @@ I830DRI2ScheduleSwap(ClientPtr client, DrawablePtr draw,
DRI2BufferPtr front,
int ret, pipe = I830DRI2DrawablePipe(draw), flip = 0;
DRI2FrameEventPtr swap_info;
enum DRI2FrameEventType swap_type = DRI2_SWAP;
+ CARD64 current_msc;
swap_info = xcalloc(1, sizeof(DRI2FrameEventRec));
@@ -629,6 +630,8 @@ I830DRI2ScheduleSwap(ClientPtr client, DrawablePtr draw,
DRI2BufferPtr front,
return FALSE;
}
+ current_msc = vbl.reply.sequence;
+
/* Flips need to be submitted one frame before */
if (DRI2CanFlip(draw) !intel-shadow_present
intel-use_pageflipping) {
@@ -638,55 +641,81 @@ I830DRI2ScheduleSwap(ClientPtr client, DrawablePtr draw,
DRI2BufferPtr front,
swap_info-type = swap_type;
+ /* Correct target_msc by 'flip' if swap_type == DRI2_FLIP.
+* Do it early, so handling of different timing constraints
+* for divisor, remainder and msc vs. target_msc works.
+*/
+ if (*target_msc 0)
+ *target_msc -= flip;
+
/*
-* If divisor is zero, we just need to make sure target_msc passes
-* before waking up the client.
+* If divisor is zero, or current_msc is smaller than target_msc,
+* we just need to make sure target_msc passes before initiating the
swap.
*/
- if (divisor == 0) {
- vbl.request.type = DRM_VBLANK_NEXTONMISS |
- DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
- if (pipe 0)
- vbl.request.type |= DRM_VBLANK_SECONDARY;
-
- vbl.request.sequence = *target_msc;
- vbl.request.sequence -= flip;
- vbl.request.signal = (unsigned long)swap_info;
- ret = drmWaitVBlank(intel-drmSubFD, vbl);
- if (ret) {
- xf86DrvMsg(scrn-scrnIndex, X_WARNING,
- divisor 0 get vblank counter failed: %s\n,
- strerror(errno));
- return FALSE;
- }
-
- *target_msc = vbl.reply.sequence;
- swap_info-frame = *target_msc;
-
- return TRUE;
+ if (divisor == 0 || current_msc *target_msc) {
+ vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
+ /* If non-pageflipping, but blitting/exchanging, we need to use
+* DRM_VBLANK_NEXTONMISS to avoid unreliable timestamping later on.
+*/
+ if (flip == 0)
+ vbl.request.type |= DRM_VBLANK_NEXTONMISS;
+ if (pipe 0)
+ vbl.request.type |= DRM_VBLANK_SECONDARY;
+
+ /* If target_msc already reached or passed, set it to
+* current_msc to ensure we return a reasonable value back
+* to the caller. This makes swap_interval logic more robust.
+*/
+ if (current_msc = *target_msc)
+ *target_msc = current_msc;
+
+ vbl.request.sequence = *target_msc;
+ vbl.request.signal = (unsigned long)swap_info;
+ ret = drmWaitVBlank(intel-drmSubFD, vbl);
+ if (ret) {
+ xf86DrvMsg(scrn-scrnIndex, X_WARNING,
+ divisor 0 get vblank counter failed: %s\n,
+
