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

[Pekka Paalanen]

On Wed, 1 Mar 2017 11:17:14 +0000
Daniel Stone <daniel at fooishbar.org> wrote:

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

Hi,

the thing I care highly about is that the commit message explains
all the consequences of a patch. Or at least all the consequences the
author and reviewers can think of, particularly the subtle ones.

Which patch adds the drm_fb_ref() is secondary to that in my opinion.

I believe that is important for the atomic series in particular,
because the series is very long, and we'd prefer landing it piece by
piece once reviews agree. That means upstream master will remain at
intermediate steps for some time.

Some more words in the commit message should be fine, I think, to show
that the detail has been acknowledged. A counter example is the stray
timer_arm() in the global output repaint patch. :-)

In general, anything to explain odd things to avoid reviewers getting
confused would be nice, IMHO. ;-)

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

Which release do you mean?

If we were to destroy the drm_fb too early, wouldn't that lead to CRTC
being turned off, which would essentially kill the output completely,
since we don't know to mode-set again? Or can page flip turn it on?

If we were to send wl_buffer.release too early, then the client might
drawn into it too early... which actually sounds like it might be
relevant to fdo#98833.

Anyway, that's not too important here.


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/20170302/695010d3/attachment.sig>