On 12/05/2014 03:41 AM, Eric Anholt wrote:
Mario Kleiner <mario.kleiner...@gmail.com> writes:
A slightly updated and extended series of the dri3/present fixes for Mesa i
sent last week.
Patch 1 and 2 are same as before. Patch 3 now has signed off by Frank Binns
and reviewed by Chris Wilson. Patch 4 and 5 are additional fixes. The last
one makes INTEL_swap_events behave properly again when swap completion events
come in out of order due to skipped present requests. Before the first out
of sequence sbc event caused the INTEL_swap_events extension to become
completely
unuseable for the rest of a session.
Sent out review for 3 of them (time for me to head out instead of
reviewing more code), and thanks for working on this. 2/5 it's not
clear to me from my first read of the spec that you shouldn't expect to
get the most recent values from either cause in your return. 5/5 my
first thoughts were "I hate wraparound logic, I'll review this later.
Also, should we even be saving off msc/ust for an out-of-order sbc
event? Needs more thought."
I'll try to think more about these later, but I wouldn't block on me.
Wrt. 2/5: It's a bit ambiguous how to read that bit of the spec, and i
agree that one could read it in a way that the current mesa dri3
behaviour is not (completely) violating the spec. When we implemented
the DRI2 version, we understood it in the way i want to restore with
2/5. The first reason is because the DRI2 / patch 2/5 interpretation
makes the OML_sync_control extension very useful and robust for swap
timing, whereas the alternative reading makes the extension borderline
useless / its use somewhat fragile due to the race described in the
commit. The second reason is backwards compatibility: It would be
awesome not to break timing sensitive apps written against DRI2,
especially given that users of those apps will certainly not understand
why a simple distribution upgrade or graphics stack update pushed by the
distro can suddenly cause such regressions.
In new app releases one could sort of work around such breakage by use
of the INTEL_swap_events extension assuming everything would be nicely
bug free. Unfortunately there were quite a few bugs and omissions and
limitations in various ddx and kms drivers even until very recently
which require funky workarounds, which require funky use of the
different bits of the OML_sync_control extension, in ways that would be
difficult to do without 2/5 restoring the DRI2 semantic.
As an example see
https://github.com/kleinerm/Psychtoolbox-3/blob/master/PsychSourceGL/Source/Linux/Screen/PsychWindowGlue.c#L1372
for my own toolkits backend implementation. It's a nice horror-show of
all the relevant bugs in DRI2/DRI3 since about 2010 with the collection
of workarounds needed to keep stuff working reasonably well on currently
shipping distros.
Wrt. 5/5: As a little helper, attached a little C test thingy for the
wraparound logic, running through a large range of simulated swap
events, exercising the logic for both overflow and underflow wraparound,
which i used to convince myself that i didn't screw up there with
integer overflows etc., apart from testing on the actual server for
normal use. In practice i don't think the wraparound logic in its
revised form will ever trigger. It takes a lot of time for a running app
to accumulate 2^32 bufferswaps.
As far as saving mst/ust for out-of-order sbc events: Yes, imho! The
Present extension allows to complete swaps in a order different from
their submission, e.g., you could submit a present request for target
msc 10000, then for 9000, then for 8000..., and Present would complete
them in order 8000, 9000, 10000 - this would cause the completion events
to come in in reverse order with decrementing sbc, instead of
incrementing. A valid case for dri3/present, and as INTEL_swap_events is
not restricted to ascending order, it should be able to handle that
case. Another case where one can see smaller inversions of the order is
when switching between vsynced and non-vsynced swaps, possibly when the
Present extension skips presents. I think a client could get mightily
confused if it would try to collect swap events for submitted swaps and
they don't show up.
However, what would be useful would be to extend INTEL_swap_events with
new enums for completion type. Something like GLX_ERROR_INTEL for a
present/swap that failed due to some error, e.g., out of memory, gpu
wedged, whatever, and then GLX_SKIPPED_INTEL for a skipped present.
Currently PresentCompleteModeSkip gets mapped to
GLX_COPY_COMPLETE_INTEL, losing that information, which would be
valuable for client applications like mine, which really need to know
for sure if specific content made it to the actual display unharmed, and
in which order.
Another thought i had: It would be good if we could expose somewhere if
mesa is using DRI2 or DRI3/Present in a given session, e.g., in the
GL_RENDERER string or maybe MESA_query_renderer extension. There are
some workarounds one needs for DRI2 which are not needed for DRI3,
potentially even harmful. There are things i (hope) i can do when i know
i'm running under a well working DRI3/Present backend, which i couldn't
do under DRI2. So i'd ideally need two different code paths and then
some way to decide which one to choose based on the mesa backend in use.
Ok, brain totally crashing, need to sleep.
Thanks,
-mario
#include <stdlib.h>
#include <stdio.h>
#include <math.h>
static unsigned int awiresbc, lastEventSbc = 0;
static unsigned long long eventSbcWrap = 0;
static long long inval = 0;
static unsigned long long retval = 0;
void remap(void)
{
awiresbc = (unsigned int) (inval & 0xffffffff);
if ((long long) awiresbc < ((long long) lastEventSbc - 0x40000000))
eventSbcWrap += 0x100000000;
if ((long long) awiresbc > ((long long) lastEventSbc + 0x40000000))
eventSbcWrap -= 0x100000000;
lastEventSbc = awiresbc;
retval = awiresbc + eventSbcWrap;
if (inval != retval)
printf("Mismatch!!! inval %llx, awiresb %x --> retval %llx \n", inval, awiresbc, retval);
//else printf("GOOD inval %llx, awiresb %x --> retval %llx \n", inval, awiresbc, retval);
}
void main(void)
{
for (inval = 0; inval <= 0x800000001; inval+= 10000) {
remap();
}
for (; inval >= 0; inval-= 10000) {
remap();
}
return;
}
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
