Limit DRI2Drawable reference leak

[Chris Wilson]

On Sun, Feb 22, 2015 at 02:09:36AM +0200, Ville Syrjälä wrote:
> On Sat, Feb 21, 2015 at 10:52:49PM +0000, Chris Wilson wrote:
> > On Sun, Feb 22, 2015 at 12:13:38AM +0200, Ville Syrjälä wrote:
> > > On Sat, Feb 21, 2015 at 09:31:07PM +0000, Chris Wilson wrote:
> > > > With the current protocol and implementations, we have to often call
> > > > DRI2CreateDrawable but can never call DRI2DestroyDrawable. This ends up
> > > > with us leaking references to DRI2Drawables based on the assumption that
> > > > the references have identical lifetimes to the Drawable going astray.
> > > > This was spotted by Daniel Drake as the mali driver would create a new
> > > > reference to the DRI2Drawable on every GetBuffers, but it can also be
> > > > observed in mesa when running synthetic benchmarks (creating lots of
> > > > contexts/glxdrawables for each window) and to a lesser extent with
> > > > normal composited operation.
> > > > 
> > > > The first two patches implement the capping of the unnamed internal
> > > > reference used by DRI2CreateDrawable to just one per Client.
> > > 
> > > IIRC we had many issues around the dri2 reference stuff during the
> > > Maemo days. Pauli fixed tons of problems in the dri2 code but some
> > > of the patches never made it in.
> > > 
> > > These seem somewhat relevant:
> > > http://lists.x.org/archives/xorg-devel/2010-November/014783.html
> > > http://lists.x.org/archives/xorg-devel/2010-November/014782.html
> > 
> > Indeed. Same problem, similar solution. I was a bit dubious as to
> > whether we could hook up DRI2DestroyDrawable after all this time (for
> > example mesa ignores it except for GLXPixmaps) and feared there was some
> > corner case that was going to explode.
> 
> Yeah dunno. This code did ship in the N9/N950 and our client side did
> issue these protocol requests appropriately. But then again, Pauli did
> a boatload of additional work on the dri2 code after these that didn't
> make it into upstream either.
> 
> As I recall one of the issues was clients getting BadDrawables from
> DRI2DestroyDrawable if the X drawable already went away. And the
> solution was to decouple the lifetimes of the two.

Yes, that is what I think the mesa patch is about. I think the solution
would have been that the DRI2Drawable take a refcnt on its parent and
then DRI2DestroyDrawable could be made to work. However, if we did that
today we would end up with a massive Drawable leak - and pushing the
change to mesa would also then expose us to the GLXDrawable vs Drawable
close race (and BadDrawable errors again). This also sinks using named
references - unless only named references also reference their parent.

I think this is something we cannot fix, but we can at least apply
damage limitation.

> I've probably forgotten most of what we did back then, but interested
> parties may go dig through
> https://www.gitorious.org/meego-w40/xserver-xorg if they wish.

Ta,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre