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

[Daniel Stone]

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.

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

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

Cheers,
Daniel