OML_sync_control semantics

[Jamey Sharp]

[We should probably have moved this conversation to the piglit list when
it shifted to discussing these new OML_sync_control tests.]

On Wed, May 14, 2014 at 04:12:32PM +0100, Chris Wilson wrote:
> 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.

Hmm, right. I've rewritten those checks to just verify that alternating
between calls to WaitForSbc and GetSyncValues always have monotonically
increasing values for both UST and MSC. That's clearly required by the
spec, and still catches the driver bugs I was concerned about.

Regarding the behavior of divisor==0: I had to think very hard about
what you were saying, what the spec says, what the X.Org implementation
does, whether SwapInterval affects any of the above, etc. (In case
anyone wondered, I did not enjoy that.) Theo and I have concluded that
you're right, regardless of what SwapInterval is set to. (It was
confusing since a non-zero SwapInterval would in some cases make
divisor==0 sync to vblank almost as if you'd set divisor==swap_interval,
but not in all cases.)

As a result, we've changed timing.c to never use divisor==0, because we
want to validate that the driver syncs to vblank when required, and as
you say, the spec only requires that if divisor>0. Instead, when testing
in -msc-delta mode we set divisor=1, to specify that if the swap misses
our target_msc, it should wait until the next vblank.

We're also retracting the repeat-swapbuffers test. There may still be
utility in a test like it but it isn't testing anything we actually care
about and we don't want to think about it further.

You can review/test these changes and others, currently in separate
patches, at https://github.com/ThirteenFish/piglit. We'll rebase them
into the series when we send it out for review again.

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

Now fixed in the above git repo.

Thanks,
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/20140516/67f20e04/attachment.sig>