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

[Daniel Stone]

Hi,

On 1 March 2017 at 10:29, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Tue, 28 Feb 2017 14:46:21 +0000 Daniel Stone <daniel at fooishbar.org> wrote:
>> On 28 February 2017 at 10:59, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>> > 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.

Sure, I can move the drm_fb_ref() addition into a follow-up patch if
you'd prefer. Let me know.

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

ISTR the problem was that, if you repeatedly hit the repaint loop (due
to damage, even if everything was occluded), the buffer currently on
scanout could get released too early? It's hard to remember though; so
many bugs, and this series has been going such a long time.

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

Indeed they have been!

Cheers,
Daniel