[PATCH] DRI2: fixup handling of last_swap_target

[Jesse Barnes]

On Thu, 4 Mar 2010 23:42:06 +0100
Mario Kleiner <mario.kleiner at tuebingen.mpg.de> wrote:
> Ok Jesse, i checked them. Our patches are happy with each other as  
> far as i can see :-) -- thanks for cleaning this up.

Excellent, thanks for checking.

> I went over the whole hw/xfree86/dri2/dri2.c file again and found a  
> couple of new bugs and problems. Don't have the time for preparing  
> patches myself now, so i'll just list them, from severe to harmless:
> 
> * In DRI2CreateDrawable(), in the new last_swap_target init code, you  
> call...
> 
>   ret = (*ds->GetMSC)(pDraw, &ust, &pPriv->last_swap_target);
> 
> ... without checking for if (!ds->GetMSC) ...

Oh that could be crashy with a DDX w/o GetMSC support, will fix.

> * In DRI2CreateDrawable(), there is pPriv->swap_interval = 1;
> 
> Default swap interval is set to 1 at init, ie., sync to retrace is  
> on. I'd suggest considering a 0 swap interval as default. The  
> glXSwapIntervalSGI() call only allows to set swap_interval to > 0 due  
> to this bit from the SGI_swap_control spec: "glXSwapIntervalSGI  
> returns GLX_BAD_VALUE if parameter <interval> is less than or equal  
> to zero."
> 
> That means once you have a non-zero swap_interval, applications can't  
> opt-out of vsyn'ed bufferswaps, they can only opt-in. Iow there ain't  
> no way for an app to start without vsync.

Yeah, that's a valid issue.  Though really SGI_swap_interval should
take 0 as valid...  I think there's a Mesa extension for it?

> Related, in DRI2SwapInterval(), a check for...
> 
>      if (pPriv == NULL)
>          return BadDrawable;
> 
> ... seems to be missing. All other routines check for this, so this  
> one should probably do so as well.

Yep, good idea.

> * There seems to be some inconsistency or missing pieces in the way a  
> drawable is destroyed, but i may miss something.
> 
> In DRI2DestroyDrawable(), once the pPriv->refcount drops to zero, the  
> drawable is destroyed if there aren't any swaps pending. If swaps are  
> pending, according to the comments there, destruction is postponed  
> until DRI2SwapComplete() gets called. Makes sense.
> 
> But in DRI2SwapComplete() there isn't any logic that would check if  
> (pPriv->swapsPending == 0 && pPriv->refcount == 0) and then free the  
> drawable. Instead it checks for pPriv->refcount == 0 and if so,  
> throws an error and free's the drawable. It shouldn't throw an error  
> and it should only free it at the very end of the routine after  
> DRI2WakeClient() was called, if (pPriv->swapsPending == 0). Current  
> implementation has a swap limit of 1, so this is always true, but if  
> the allowable number of outstanding swaps would be increased at some  
> time, this would cause problems.

Oh good point; we wouldn't want to free until the last swap was
completed or we'd try to deref a bad drawable.

> Another issue is DRI2WaitMsc() and DRI2WaitSbc(). They should  
> probably complete immediately or throw an error if called while the  
> drawable is already awaiting destruction with a pPriv->refcount == 0?

Yeah, probably, will fix.

> If a wait for a certain target_msc or target_sbc is already  
> scheduled, one should probably also defer the destruction of the  
> drawable until they complete?

Or otherwise prevent a crash somehow.  I just realized there's a
potential resource starvation issue here; malicious apps could queue a
bunch of swaps or waits that won't happen for a long them, then destroy
their windows etc and eat up server memory.  Should handle that somehow
by destroying client state asap.

> Proposals:
> 
> DRI2WaitSBC and DRI2WaitMSC, maybe others as well (e.g.,  
> DRI2WaitSwap?) should probably check for pPriv->refcount == 0 and  
> return either an error or return immediately on such an "undead"  
> drawable. Error return is probably better, given that the  
> OML_sync_control spec requires an error return if no context/drawable  
> is bound at time of call, and the drawable has been kind'a destroyed  
> already if refcount == 0.
> 
> DRI2DestroyDrawable() should probably postpone destruction if (pPriv- 
>  >swapsPending > 0 || pPriv->blockedClient != NULL), ie., if any  
> waitsbc/waitmsc is already scheduled.
> 
> DRI2SwapComplete at its end, and DRI2WaitMSCComplete should probably  
> check if it is time to destroy the drawable before returning and do  
> so  if neccessary to avoid leaks.

Agree.

> Finally at a quick glance, there are more places in the code were  
> there are potential problems with CARD64 values vs. 32 bit integers.  
> The OML_sync_control spec wants CARD64 as return values for msc and  
> and as target_msc for glXWaitForMscOML and glXSwapbuffersMscOML(),  
> but the DRM in the kernel only operates with 32 bit vbl.request/ 
> reply.sequence numbers and some of the DRI2 functions pass values as  
> 32 bit signed integers. Assuming a video refresh rate of 200 Hz for  
> the fastest crt monitors i'm aware of, this could cause some overflow  
> problems after 2^31 / 200 / 3600 / 24 = 124 days of uptime - not that  
> unrealistic for a desktop machine. This will probably need fixes to  
> the DRM or some wraparound handling or range checking in the xserver  
> to make it really robust for unlucky applications that happen to get  
> launched at the 124th day of uptime.

Yeah, the DDX could probably handle that by virtualizing the target_msc
passed in by the X server.

-- 
Jesse Barnes, Intel Open Source Technology Center