On Tue, 28 Feb 2017 14:46:21 +0000 Daniel Stone <dan...@fooishbar.org> wrote:
> Hi, > > On 28 February 2017 at 10:59, Pekka Paalanen <ppaala...@gmail.com> wrote: > > On Mon, 27 Feb 2017 22:58:48 +0000 Daniel Stone <dan...@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
pgpidiirhxRro.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
