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

[Chris Wilson]

On Wed, May 14, 2014 at 06:49:10AM -0700, Jamey Sharp wrote:
> 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.

It's a FAIL currently.
 
> 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.

With divisor=0 swaps, the driver is allowed to perform immediate completion
of swaps if target_msc == current_msc. So it is possible for the driver
to receive a swap request and reply that it completed it that msc just
prior to an interrupt occuring.

> 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."

Right. But using the divisor=0 logic above we don't have queued frames
if a client never requests a swap in the future. As soon as that happens,
we end up with a steady state of swaps and fail to break out of the rut
never to return to the expected fast behaviour.
> 
> > 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.

Except when there is not an outstanding swap because the spec allows the
driver to complete the swap immediately for the target_msc ==
current_msc with divisor == 0.

> 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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre