[Mesa-dev] [PATCH 7/8] glx: fix the refcounting of dri drawables

[Ian Romanick]

On 04/07/2016 10:57 AM, Stéphane Marchesin wrote:
> On Thu, Apr 7, 2016 at 10:44 AM, Miklós Máté <mtmkls at gmail.com> wrote:
>> On 04/07/2016 12:40 PM, Stéphane Marchesin wrote:
>>>
>>> On Thu, Apr 7, 2016 at 3:37 AM, Miklós Máté <mtmkls at gmail.com> wrote:
>>>>
>>>> On 04/07/2016 12:25 PM, Stéphane Marchesin wrote:
>>>>
>>>>
>>>> On Apr 7, 2016 02:27, "Michel Dänzer" <michel at daenzer.net> wrote:
>>>>>
>>>>> On 07.04.2016 18:01, Marek Olšák wrote:
>>>>>>
>>>>>> On Thu, Apr 7, 2016 at 7:32 AM, Ian Romanick <idr at freedesktop.org>
>>>>>> wrote:
>>>>>>>
>>>>>>> Why would you do that?  I've NAKed this patch several times.
>>>>>>
>>>>>> You NAKed the previous version of the patch, not this one. I guess
>>>>>> that means NAK for this one too.
>>>>>
>>>>> I'd be interested in a specific justification for NAKing this version.
>>>>> Seems to me like this might be the best that can be done.
>>>>
>>>> We asked ourselves the same question.
>>>>
>>>> It is the best that can be done to fix glx 1.2 for sure. But it will
>>>> break
>>>> glx 1.3, so for example it will break chrome (I have a vested interest
>>>> here
>>>> :).
>>>>
>>>> So what we figured out is, if we have to choose between the old and new
>>>> api,
>>>> why break the newer (and arguably better) one? Instead we should consider
>>>> the old one deprecated/legacy and move on.
>>>>
>>>> Stéphane
>>>>
>>>> How does this break glX 1.3, and how does this break Chrome?
>>>
>>> I don't have the source here, so it's off the top of my head. But
>>> isn't this going to start leaking dri drawables if you constantly
>>> makecurrent?
>>
>> No, it doesn't leak anything in makecurrent.
>>
>>>   By itself this is ok (because they all point to the same
>>> buffer) but there are pieces of glx 1.2 (like swapbuffers) that
>>> iterate over all the dri drawables and then they become really slow.
>>>
>>> Stéphane
>>>
>> The problem with this patch, as I was informed, is that people don't call
>> glXDestroyPixmap (or glXDestroyGLXPixmap) before calling XFreePixmap, and
> 
> Yes, because those don't exist in glx 1.2.

glXDestroyGLXPixmap has existed since GLX 1.0.  My impression was that
the lack of glXDestroyWindow was the problem.

My goal is to avoid leaks in long-running applications that lots and
lots of people care about.  Chrome is (the?) one such application that I
know of from the top of my head.  My recollection is that this kind of
leak would cause Chrome to leak every time a tab is closed.  I know that
Stéphane is a busy guy, so I was trying to keep the discussion more
generic so that he wouldn't have to step in. :)  Now that you're here...

If Stéphane and Miklós can come up with a solution that makes you both
happy, I won't get in the way.  If there is still some potential for a
leak, I'd like to have at least a plan (or a fallback via environment
variable) to get non-leaking behavior... in case we find another app
with this problem.  I don't want to be left holding the bag if Miklós
wanders back to more interesting projects.  Right?

>> thus the resources allocated by glx are leaked (until the process exits). In
>> current Mesa, the dri drawable associated with the glx object only lives
>> while the glx object is current to a context, which prevents the above
>> problem in a crude way.
> 
> Yes, and you end up accumulating lots of dri drawables which are all
> iterated at swapbuffers time. That makes swapbuffers really slow
> eventually.
> 
> Stéphane
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>