[PATCH] DRI2: By default, throttle after two pending swaps.

[Jamey Sharp]

On Wed, May 14, 2014 at 01:31:14PM +0100, Chris Wilson wrote:
> On Tue, May 13, 2014 at 09:20:45AM -0700, Jamey Sharp wrote:
> >    Yeah, we have new tests that haven't quite been merged yet as we're
> >    addressing Eric's review comments. Our current version, revised yesterday,
> >    is available here:
> > 
> >    [2]https://github.com/ThirteenFish/piglit
> > 
> >    Make sure you do a full 'piglit-run.py -t OML_sync_control ...' as we run
> >    the new tests in a variety of configurations, and the full-screen ones are
> >    particularly interesting here. Though IIRC, SNA fails some of the others
> >    as well.
> 
> Looks like there are quite a few bugs remaining in the test cases.

Thanks for reviewing them, Chris!

> glXGetSyncValuesOML returns the current hardware frame counter and time
> of last frame update, not the last client swap frame or swap time. You
> make an assumption in the timing loop that the calls to SwapBuffers
> and GetSyncValues are instantaneous and do not account for a vblank
> interrupt that can happen in between updating the hardware counters.

We do make that assumption, but if the assumption is violated we only
emit a warning.

And it would be very surprising if a correctly-functioning driver
violated that assumption, because WaitForMsc/Sbc are required to return
as soon as possible once the swap completes. So we should have something
like 16ms to issue the follow-up GetSyncValues before the values will
change.

We included that warning because drivers call DRI2SwapComplete with
zeroes for UST and MSC, which is clearly always wrong, and this also
lets us continue on to test whether they're at least syncing to vblank.

> The computation of interframe jitter is wonky, thanks to using
> (new_timestamp - -1) on the first pass.

Oops, we broke that while addressing Eric's review comments on Monday.
We'll fix it.

(Theo, if I don't get to it first: both the if and else starting on line
182 need to be guarded with (last_timestamp >= 0). We'll have to deal
with the 80-column limit some other way.)

> "glXSwapBuffersMscOML: If <divisor> = 0,
> the swap will occur when MSC becomes greater than or equal to
> <target_msc>."

Yes, except:

"If there are multiple outstanding swaps for the same window, at most
one such swap can be satisfied per increment of MSC."

> The minimum expected_msc is therefore the target_msc in many cases and
> no time may pass at all during the timing loop if msc-delta=0.

Note that timing.c's main() prohibits using WaitForMsc without also
setting either msc-delta or divisor, because that combination would fail
in the way you describe. However, because of the above-quoted sentence
in the spec, we believe that SwapBuffersMsc must behave as if we'd set
msc-delta non-zero.

In the open-source implementations, swap_interval defaults to 1 and so
it does behave this way. If you override swap_interval to 0, you get
non-conforming behavior, but I don't believe that's a test bug.

> In repeat-swapbuffers.c, you query the current hw counters before the
> swap may have completed,

That's because that test's purpose is, as the file comment says,
"Verifies that glXSwapBuffersMscOML does not increment SBC more often
than MSC". In that test we don't want to wait for any swaps to complete.

> and runs afoul of the divisor==0 logic above.

Since that test only uses SwapBuffersMsc, the same reasoning applies.

Again, thanks for looking this over! Did you catch anything else when
you ran the new tests?

Jamey
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg-devel/attachments/20140514/f8fa097c/attachment.sig>