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

[Miklós Máté]

On 02/12/2016 06:56 AM, Michel Dänzer wrote:
> 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?
In theory, yes. In practice, no. I tried to add refcount=1 to the 
glXCreate* functions, but
for example, you don't need to call glXCreateWindow:
  win=XCreateWindow(....); glXMakeCurrent(dpy, win, glc);
is fine. This is what the example on OpenGL wiki does, and more 
importantly, Wine does this, too.
When should refcount=1 be set in such cases? I couldn't figure it out.

>
>
>> 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.
This is an interesting question. The GLX 1.4 specification says on page 
28 that "If draw is
destroyed after glXMakeContextCurrent is called, then subsequent 
rendering commands will
be processed and the context state will be updated, but the frame buffer 
state becomes
undefined." This suggests that the lifetime of the draw buffer ends with 
destroy, not unbind.
If this is true, we don't need refcount on makecurrent.

However, on page 23 it says "The storage for the GLX pixmap will be 
freed when it is not
current to any client." This is interesting, because one pixmap can be 
current to more than
one client, and thus it needs to be refcounted across multiple 
processes. I think "storage" in
this case means the server-side resources, and not the dri drawable, but 
I could be wrong.

MM