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

[Miklós Máté]

On 04/07/2016 08:50 PM, Ian Romanick wrote:
> 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?
Well, if Chrome properly uses glXDestroyWindow, then this patch doesn't 
introduce
a resource leak there. The leak only affects applications that render to 
multiple
windows without using glx 1.3.

MM

>
>>> 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
>>