[PATCH] DRI2: fixup handling of last_swap_target

[Jesse Barnes]

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