[PATCH] DRI2: fixup handling of last_swap_target
Jesse Barnes
jbarnes at virtuousgeek.org
Sun Mar 7 09:10:51 PST 2010
On Sun, 7 Mar 2010 08:44:51 +0100
Mario Kleiner <mario.kleiner at tuebingen.mpg.de> wrote:
> On Mar 5, 2010, at 6:50 PM, Jesse Barnes wrote:
>
> >
> > Ok pushed fixes for these issues to the repo above. I know you've put
> > a lot of time into this already, but can you take another look to make
> > sure I got everything?
>
> No problem.
>
> > Note I left the swap interval default at 1
> > since GLX_MESA_swap_control should let us set it to 0 to get
> > unthrottled behavior, which I just fixed and tested.
>
>
> Agreed, that's ok. GLX_MESA_swap_control is a way to do it.
>
> The current state looks almost good, esp. your fixes to
> DRI2SwapBuffers for swap_interval == 0 and for target_msc == 0.
>
> But a few more things:
>
> In DRI2SwapBuffers, in the body of ...
>
> /* Old DDX or no swap interval, just blit */
> if (!ds->ScheduleSwap || !pPriv->swap_interval) {
> ...
>
> ... it calls DRI2SwapComplete and passes target_msc as the 'msc'
> value of swap completion. Would be probably better to pass a zero
> value, just as it is done in the Intel DDX when the swap doesn't
> happen at all at the requested target_msc, but rather immediately due
> to some problems. If ds->ScheduleSwap isn't available then the 'msc'
> is basically undefined and unknown, so zero is a better return value.
> In the !pPriv->swap_interval case one could call a *ds->getMSC() to
> get a more meaningful return value for 'msc'. Don't know if it's
> worth the effort, but a zero return would be better than 'target_msc'
> imho.
Right, good idea. That way software using the new event won't get
bogus values with old DDXes.
> We need to add another patch to Mesa. Mesa translates a glXSwapBuffers
> () call into a DRI2SwapBuffers() call with target_msc, divisor and
> remainder all zero and DRI2SwapBuffers takes this as hint that
> glXSwapBuffers behaviour is requested. However these are also legal
> arguments to a glXSwapBuffersMscOML(..., ,0,0,0) call, which has to
> behave differently than a glXSwapBuffers call --> In this case,
> DRI2SwapBuffers would confuse this with a glXSwapBuffers request and
> do the wrong thing.
>
> I'd propose adding this line to the __glXSwapBuffersMscOML() routine
> in mesa's /src/glx/x11/glxcmds.c file:
>
> if (target_msc == 0 && divisor == 0 && remainder == 0) remainder = 1;
>
> -> if target_msc and divisor are zero, then the remainder is ignored
> inside the DRI implementation, so the actual value of remainder
> doesn't matter for swap behaviour, but can be used to disambiguate
> glXSwapBuffers vs. glXSwapBuffersMsc... for a glXSwapBuffersMscOML
> (0,0,0) call.
Ok, I'll have to check the behaviors again, but this sounds reasonable.
> Your deferred pPriv deletion logic looks now good to me. But if
> deletion is deferred then i think almost all "public" functions
> inside dri2.c should return 'BadDrawable' not only for pPriv == NULL
> but also for pPriv->refCount == 0, e.g., DRI2SwapBuffers,
> DRI2GetMSC, ... From the viewpoint of client code, a refCount == 0 is
> the same as a destroyed drawable. For all practical purposes it is
> already gone. Maybe it would make sense to consolidate the (pPriv ==
> NULL || pPriv->refCount == 0) check into a macro or function,
> something like DRI2IsDrawableAlive(pPriv); and then call that in most
> DRI2xxx functions?
Yeah, that would make things cleaner, I'll do that.
> I believe there's also another problem with the pPriv->blockedClient
> logic. The implementation only keeps track of blockedClient, but not
> for the reason why the client blocked. This can cause trouble in
> DRI2WakeClient. Consider this example which i think would cause the
> client to hang:
>
> 1. Assume current msc is 1000 and there are 2 threads in the client.
>
> // Thread 1: Request swap at target_msc 1010:
> 2. glXSwapBuffersMscOML(dpy, drawable, 1010, 0, 0);
>
> // Thread 2: Request wait until target_msc 2000:
> 3. glXWaitForMscOML(dpu, drawable, 2000, 0, 0, ....);
>
> 4. Thread 1: Calls XDestroyWindow(dpy, drawable);
>
> Step 2 will schedule a swap for msc = 1010.
> Step 3 will pPriv->blockedClient = client and IgnoreClient(client)
> the client and schedule a wakeup at msc >= 2000.
> The xconnection dpy is now blocked.
>
> Step 4 doesn't execute yet, because the xconnection is blocked.
>
> Now at msc = 1010 -> swap completes -> DRI2SwapComplete ->
> DRI2WakeClient -> executes the elseif branch:
>
> ...
> } else if (pPriv->target_sbc == -1) {
> if (pPriv->blockedClient)
> AttendClient(pPriv->blockedClient);
> pPriv->blockedClient = NULL;
> ...
>
> -> this will AttendClient() the client and release pPriv-
> >blockedClient, i.e., release at msc 1010 although the block was
> meant to be released at msc 2000. Thread 2 still waits for a _XReply
> before it can return from the glXWaitForMscOML...
>
> Now step 4 can execute due to the "unfrozen" xconnection dpy.
> DRI2DestroyDrawable() executes and DRI2FreeDrawable() gets called
> becuase there are no pending flips or blockedClients anymore. pPriv
> is gone.
>
> At msc = 2000, DRI2WaitMSCComplete() gets called due to the received
> vblank event. It finds that pPriv == NULL and returns immediately.
>
> -> ProcDRI2WaitMSCReply() doesn't ever get called and thread 2 hangs
> (forever?) waiting for a _XReply.
>
>
> In short, i think the implementation should keep track of why a
> client is blocked in pPriv->blockedClient and only unblock it for the
> reason it was blocked, otherwise there are a race conditions.
Yeah, this could also happen without OML I think, if a few swaps were
queued (resulting in a block) and then a SGI_video_sync call occurred.
I'll fix it up.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
More information about the xorg-devel
mailing list