[Mesa-dev] [PATCH 6/7] glx: remove incorrect refcounting of DRIdrawable

[Michel Dänzer]

On 12.02.2016 05:58, Miklós Máté wrote:
> On 02/09/2016 05:02 PM, Ian Romanick wrote:
>> On 02/08/2016 05:11 PM, Ian Romanick wrote:
>>> On 02/05/2016 01:11 PM, Miklós Máté wrote:
>>>> dri drawables must never be released when unbound from a context
>>>> as long as their corresponding glx objects (window, pixmap, pbuffer)
>>>> still exist
>>> I'd really like to have Kristian weigh in, since DRI2 was his design,
>>> and this is all his code being affected.  That said, I'm a little unsure
>>> about this change.
>>>
>>> The DRI drawables and associated resources will still get freed when the
>>> application eventually calls glXDestroyPBuffer or glXDestroyWindow.
>>>
>>> But I'm not 100% sure what will happen with GLX 1.2 applications...
>>> there are no glXDestroy* functions in GLX 1.2.  In that case the
>>> application will have a soft leak because calling XDestroyWindow won't
>>> do anything on the client (or server?) for the DRI resources.  I suspect
>>> that's what this code was trying to prevent.
>> And this commit affirms that:
>>
>> commit bf69ce37f0dcbb479078ee676d5100ac63e20750
>> Author: Stéphane Marchesin <marcheu at chromium.org>
>> Date:   Wed Jun 15 15:09:12 2011 -0700
>>
>>      glx: implement drawable refcounting.
>>           The current dri context unbind logic will leak drawables
>> until the process
>>      dies (they will then get released by the GEM code). There are two
>> ways to fix
>>      this: either always call driReleaseDrawables every time we unbind
>> a context
>>      (but that costs us round trips to the X server at getbuffers()
>> time) or
>>      implement proper drawable refcounting. This patch implements the
>> latter.
>>           Signed-off-by: Antoine Labour <piman at chromium.org>
>>      Signed-off-by: Stéphane Marchesin <marcheu at chromium.org>
>>      Reviewed-by: Adam Jackson <ajax at redhat.com>
>>
>> Since we don't have any way to know when the corresponding GLX object
>> ceases to exist, we don't know when it's safe to destroy the DRI
>> object.  I don't know how we can not leak and not prematurely destroy
>> the object. :(
> That commit message exaggerates a little when it says "proper
> refcounting", because it only refcounts on bind (in driFetchDrawable),
> but not on create/destroy. With current Mesa the following happens:
> create glXPbuffer: dri drawable is created, refcount=0
> bind it: refcount++
> unbind it: refcount--, drawable is destroyed
> bind again: create new drawable, refcount=1
> unbind again: refcount--, drawable is destroyed
> etc.

Could that be fixed by starting with refcount=1 instead of 0?


> They also say that "current unbind logic will leak drawables until the
> process dies", which is simply not true. The glXDestroy* calls properly
> dispose of the corresponding dri objects.
> 
> With GLX 1.3 the dri drawables are correctly created and destroyed along
> with their corresponding glx objects even without the above commit. IMHO
> if someone deliberately uses GLX 1.2 or earlier, they know what they are
> doing (1.3 was released in 1998). I'd say that it's never safe to
> destroy a dri object until it's explicitly destroyed by the user, so I'm
> still convinced that my patch is correct.

Unless I'm missing something, one issue I see with your patch is that a
DRI drawable could be destroyed while it's still current for a context.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer