[PATCH weston 19/68] compositor-drm: Refcount drm_fb

[Pekka Paalanen]

On Tue, 28 Feb 2017 14:46:21 +0000
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi,
> 
> On 28 February 2017 at 10:59, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Mon, 27 Feb 2017 22:58:48 +0000 Daniel Stone <daniel at fooishbar.org> wrote:  
> >> It's indeed almost certainly a bugfix: if you have one client buffer
> >> with multiple views, both of which could be promoted to scanout,
> >> you'll get the same user data (drm_fb) handed back for the same BO,
> >> and could attempt to destroy it twice. Previously you'd probably
> >> mostly luck out due to the memory not yet being reused, but now you'd
> >> be near-guaranteed to hit the assert in drm_fb_unref, which seemed
> >> harsh. So I squashed it in here.
> >>
> >> drm_fb_ref doesn't exist before this patch, so the split would have to
> >> be this in a later patch. Would that work for you? I don't mind either
> >> way.  
> >
> > I think I'd be fine if those points were explained in the commit
> > message. To me, it's important to justify every hunk in a patch if it
> > somehow stands out as not being part of the main topic. And this one
> > caugh my eye as an unexplained change.  
> 
> Fair enough. I think the change from unref to actually performing an
> unref makes it more or less balancing, but sure.
> 
> > The important thing to document about this change is that is exchanges
> > a (almost never hit) double-unref into a (always hit) leak of the
> > drm_fb. It would be even better to mention what patch shall fix the
> > introduced leak.  
> 
> I ... don't think it does leak? At least not if it gets successfully displayed.

Hmm, yes, on a second look after sleeping several nights, I see calls
to drm_fb_get_from_bo() are pretty much balanced with drm_fb_unref.

But because this patch adds refcounting, and adds only calls to
drm_fb_ref() without adding corresponding calls to unref, then I think
the code just before this patch was somehow broken, because this patch
as is definitely changes behaviour.

Was it so that before this patch adding the refcounting, the drm_fbs
were destroyed and re-created repeatedly, and it no longer happens?

I'm trying to find the explanation on the change of behaviour caused by
this patch, because I cannot understand how adding a ref but not adding
unref could not change the behaviour.

> > To clarify, the leak I am imagining is: drm_fb gets created with
> > refcount=1, then every drm_fb_get_from_bo() will increment the
> > refcount, but according to the old not-refcounted architecture, there
> > is only ever a single call to unref, which means it will never be
> > released. And that would apply to *all* drm_fbs ever used. Am I wrong?
> >
> > Seems like a fairly big temporary regression compared to the
> > double-free which can never happen in Weston since we don't use
> > multiple views (yet).  
> 
> Every drm_fb_get_from_bo() whilst the BO is live, sure. But these are
> balanced by drm_fb_unref (formerly drm_output_release_fb) calls in,
> say, vblank_handler. So I can't really see a leak, and even if there
> were, planes are disabled by default. Maybe scanout, but that's
> trivially broken right now anyway, so not the biggest of regressions
> as far as I'm concerned, if it even is one - and I can't quite see it.

Scanout is broken? Since when?

I used it successfully when reproducing
https://bugs.freedesktop.org/show_bug.cgi?id=98833 or is that exactly
the breakage you refer to?

> Other libweston users can and do use multiple views ...

Then they should have been broken to begin with, no? As you said,
planes are disabled by default.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170301/77822478/attachment.sig>