Re: [Dri-devel] [PATCH] Fixes for GLX_SGI_video_sync

[Ian R. <id...@us...>]

Leif Delgass wrote:
> On Tue, 13 May 2003, Ian Romanick wrote:
> 
> 
>>Leif Delgass wrote:

[snip]

>>>- Added a generic driGetMSC32() to vblank.c, which gets the current MSC by
>>>calling drmWaitForVBlank() with a relative wait of 0 (i.e., don't wait,
>>>just return the current MSC value).  I should point out that since the
>>>vblank counter in the kernel is an unsigned int rather than an unsigned
>>>long, this will return a 32-bit value (then cast to an int64_t) even for
>>>64-bit platforms (not sure if this is true for all platforms, but I
>>>checked it on Alpha and Sparc64 on the compile farm and it's 32-bit).
>>
>>Hmm...now that's interesting.  Is there any (easy) way to find out what 
>>it is on IA64 or x86-64?  If it's 32-bit on those platforms, I would 
>>*very* much be in favor of changing it from int to int32_t (or something 
>>similar) that explicitly says that it's a 32-bit value.
> 
> I think someone needs to donate an Opteron server to the sourceforge
> compile farm. :)  If it ends up being 32-bit on all platforms we care
> about, then I think it's a good idea to make it explicit (we can use u32
> in the Linux kernel).  Then maybe we can use the ioctl flags to select a 
> 64-bit version as Michel suggested.

Eric Anholt (in another message) says that it's the same there.  My vote 
is to change all of the unsized 'int' and 'unsigned' fields to be u32 or 
i32 (is there such a beast?).  We can then think about making proper 
64-bit versions of some of the structures / ioctls later.

>>>- The radeon and r200 drivers' implementations of GetMSC was using the
>>>wrong counter (frame age instead of vblank counter), and mga was doing a 
>>>wait for "absolute 0" rather than a relative wait for 0.  I changed all 
>>>the drivers to use the new generic implementation.  One possible problem 
>>>of using the counter for both the SGI/OML extensions is that the OML 
>>>version wants the counter incremented for each field with an interlaced 
>>>mode, and the SGI version does not.
>>
>>I think they way to fix it would be in libGL.so.  In the direct 
>>rendering case, if glXGetVideoSyncSGI detects that the display is 
>>interlaced, it would divide 'count' by two.  It would have do similar 
>>trickery for glXWaitVideoSyncSGI as well.  I figured that most people 
>>aren't using interlaced displays much any more, so I didn't worry about 
>>it. :)  I suppose we should make a note of it at least.
> 
> Sure.  Actually, I remember seeing something about interlaced and 
> doublescan modes in glxcmds.c now that I think about it.  That sounds like 
> the easist solution if we can hide that detail from the drivers.

Yeah.  That's in the code that determines the monitor's refresh rate 
(for glXGetMscRateOML).

[snip]

>>> The last fix affects both extensions:  the calculation
>>>of the next target MSC (from the divisor/remainder) needed a tweak:
>>>MSC - (MSC % divisor) + remainder gives you the refresh closest to the 
>>>current one, but it can be _after_ the current one, so we only need to add 
>>>divisor if that value is less than or equal to the current MSC.
>>
>>Hmm...I've managed to convince myself that, technically, you're right. 
>>I think that will be a *very* uncommon edge case.  I the useful cases, 
>>(target_msc % divisor) == remainder.  Based on that, I couldn't come up 
>>with a case off the top of my head where divisor wouldn't be added to next.
> 
> 
> Well, it turned out to be very common when I got to testing the extension
> with the glxgears patch.  I was getting half the framerate I was expecting
> as a result of this.  I spent a lot of time messing around with the
> equations.  Basically, if (MSC % D) < R (where D and R are the divisor and
> remainder values passed by the app and MSC is the current value), then you
> don't need to add the divisor, because R - (MSC % D) is positive, so 
> MSC + R - (MSC % D) > MSC.  If (MSC % D) >= R, then you need to add the
> divisor, because R - (MSC % D) is less than or equal to zero, so MSC + R -
> (MSC % D) <= MSC.  I think the entire set of possible values that satisfy
> the equation can be expressed as:
> 
> next = MSC + R - (MSC % D) + kD, where k is an integer.
> 
> In our case, we want k=0 or k=1 to get the smallest value for next which
> is > MSC, depending on the sign of R - (MSC % D).  If you take mod D of
> the equation above, it reduces to (next % D) = R.

/me digs out a pencil to do some math for a change...

>>>Index: glxgears.c
>>>===================================================================
>>>RCS file: /cvsroot/mesa3d/Mesa/xdemos/glxgears.c,v
>>>retrieving revision 1.5
>>>diff -u -r1.5 glxgears.c
>>>--- glxgears.c	29 Apr 2003 07:15:50 -0000	1.5
>>>+++ glxgears.c	13 May 2003 15:09:52 -0000
>>>@@ -32,6 +32,7 @@
>>>  *    -swap N        Attempt to set the swap interval to 1/N second
>>>  *    -forcegetrate  Get the display refresh rate even if the required GLX
>>>  *                   extension is not supported.
>>>+ *    -sync          Synchronize buffer swaps to the monitor refresh
>>
>>I don't think that's a good name for that.  It's too similar to what 
>>-swap does.  Perhaps it could be re-worked to act like -swap, but select 
>>between using the swap-interval extensions and the video-sync 
>>extensions.  It won't give the "expected" results if both -sync and 
>>-swap are used together.
>>
>>The same idea could be applied to counting the missed frames using 
>>either the video-sync extension or the frame-usage extension.  Part of 
>>my patch that fixes the driWaitVBlank problems also adds support to the 
>>drivers and glxgears for MESA_swap_frame_usage.  glxgears is really 
>>growning! :)
> 
> This was just a quick test really.  It could be extended to work with 
> either OML_sync_control or SGI_video_sync.  Regarding the swap value, I 
> wasn't sure how that is supposed to interact with the SGI_video_sync 
> extension, since it's not dealt with in the SGI extension specs.

Well, there isn't any interaction, per-se.  The only reason we have 
interaction in this case is because glXWaitVideoSyncSGI is being used to 
emulate setting the swap-interval.