[PATCH weston v9 14/62] compositor-drm: Introduce fb_last member

Thu Mar 30 11:08:09 UTC 2017

On Wed, 29 Mar 2017 18:46:03 +0100
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi Pekka,
> 
> On 24 March 2017 at 10:30, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Thu, 23 Mar 2017 15:56:55 +0000
> > Daniel Stone <daniel at fooishbar.org> wrote:  
> >> On 22 March 2017 at 14:35, Pekka Paalanen <ppaalanen at gmail.com> wrote:  
> >> > I think this patch could use a better explanation, particularly if all
> >> > my speculations were in fact incorrect. ;-)  
> >>
> >> Yeah, your speculation (and how I've outlined my thinking / view of
> >> things above) is accurate, up until the question, which I don't
> >> entirely understand. I'll phrase this entirely in terms of atomic
> >> state rather than fb, because this is purely an incremental step along
> >> the way.  
> >
> > The question came from the fact that page_flip_handler() sets fb_last
> > to NULL rather than keeping it around for further use. To me that looks
> > like you have no need to keep atomic state that is currently on screen,
> > if not kept in some other place.  
> 
> Er ... ?
> 
> Let's say buffer A is current on screen, with an idle repaint loop.
> last == NULL, current == A. (I'm ignoring pending, since it is purely
> a staging area used between repaint_begin and repaint_flush.)
> 
> A repaint operation generates buffer B to be displayed, and calls
> drmModePageFlip. last == A, current == B. A is still displayed on
> screen at the point drmModePageFlip is called.
> 
> The page flip operation completes: B has now turned to light. last (A)
> is unref'ed: it is not part of the current state, nor is it being
> displayed on screen (since page_flip_handler signals that the last
> completed state request, has completed _in hardware_ as well). We have
> no need for it. last == NULL, current == B.
> 
> Does page_flip_handler need more commentary on how it signals the
> completion of drmModePageFlip? :\

Maybe not, perhaps I just didn't fully realize that page_flip_handler()
does not move a buffer from one state to another like it used to do,
but it only clears fb_last.

Pageflip completing is not a state change, it is purely a clean-up.
That was hard to grasp. I mean it does push the repaint state machine
from one state to another, but it does not change the buffer states
from pending to current or anything like that.

I am still dazzled that the current on-screen buffer is not a single
variable, but: fb_last ? fb_last : fb_current.

It makes sense, but it requires throwing out the old mental model of
things.

> >> I need to retain the superseded (state_last) state, because that needs
> >> to get freed when an update completes. Mind you, this can be
> >> 'floating', with some work: we could retain the drm_pending_state
> >> ownership of the drm_output_state until we get a completion event
> >> (page_flip_handler/atomic_complete_handler), and not retain it in
> >> drm_output. This would mean that
> >> {vblank,page_flip,atomic_complete,destroy,disable,dpms_off}_complete
> >> could be replaced by ... two enums? One for where we are in the
> >> repaint cycle (between begin/flush, post-flush but pre-complete,
> >> idle), and one for our target state when we can next change it
> >> (active, dpms off, disable, destroy). This would even make some of the
> >> ownership tracking slightly easier, I think, and drm_output would only
> >> have state rather than state_last. Was that what you were asking ... ?  
> >
> > Umm... maybe? :-)
> >
> > I might be dense here, but why not just destroy the state instead of
> > putting it into state_last?  
> 
> We don't want to send a wl_buffer.release until the hardware has
> stopped using the buffer. That only occurs when the completion event
> (page_flip_handler) for the request which made that state _not_
> current, has been delivered.

Right.

> > I notice that state_last does not get destroyed without
> > page_flip_pending, which means you keep it around only because of
> > start_repaint_loop.  
> 
> It doesn't have anything to do with start_repaint_loop ...
> 
> state_last only gets populated (set non-NULL) when we submit new state
> which will be asynchronously applied. This occurs when we use
> drmModePageFlip in the fallback path for start_repaint_loop, later in
> the series (cf. the page_flip_pending flag, which is currently set
> when we're flipping to a new buffer and should avoid releasing the
> old; I can expand further on this if it's helpful to do so, but the
> later commit does have a fairly complete commit message I think). This
> also occurs when we use drmModePageFlip in the regular
> drm_output_repaint path.
> 
> page_flip_handler is called for both these cases. Its job is to note
> that the buffer last requested has now turned to light
> (drm_output_finish_frame -> presentation time feedback). Its job is
> also to release the old buffer which is no longer used anywhere
> (drm_fb_unref).
> 
> > Ahh, but the actual use of fb_last comes in patch 16 which I had not
> > read yet! I was missing the greater reason on why do this in the first
> > place.  
> 
> 'Actual use'? It's very much used here, per the example above.

All the above confusion stems from page_flip_pending flag. As I read
the patch series patch by patch, I did not know that it was cleaned up
in a later patch. That clean-up seems to be crucial for understanding
how this patch works. I assumed page_flip_pending served a purpose and
was trying to understand what it is and how it relates to the new
things - why you still use it to gate drm_fb_unref(fb_last).

Maybe a note in the commit message, that page_flip_pending is no longer
needed for it old special purpose and a named later patch will clean it
up?

> >> > That's an interesting comment, not quite sure what it does in this
> >> > patch.  
> >>
> >> It's a non-sequitur. Since I fix it later in the series, I could just
> >> drop it from this patch.  
> >
> > So... does it mean that we may at this point (or already in upstream?)
> > attempt to place planes on a wrong output?  
> 
> Upstream already does viciously incorrect things with planes on
> multihead. But luckily, thanks to the issue which 'Ignore views on
> other outputs' solves, planes on multi-head are completely broken
> anyway.

Okay. I just never saw such breakage when testing simple-egl on a
plane with two outputs before anything atomic, and I'm fairly sure I
tested it and simple-egl did end up on a hw-plane if it was completely
on either output.

> >> > Wait, even before this patch, what unrefs all the FBs in
> >> > current/pending/next when an output is disabled or destroyed?
> >> >
> >> > Did we miss something with "compositor-drm: Refcount drm_fb" after all?
> >> > Reading "compositor-drm: Use refcounted FBs for Pixman" it seems we do
> >> > unref 'dumb', we ref for 'next', but do not unref for 'current' on
> >> > destroy but only when being replaced by 'next' in page_flip_handler().  
> >>
> >> Yes, I think you're right. Mind you, since gbm_bos are not themselves
> >> refcounted, destroying the gbm_surface in drm_output_fini_egl will
> >> inherently destroy the gbm_bo, which will call our destroy handler to
> >> remove the FB, etc etc. But we're still leaking a ref.  
> >
> > What about pixman buffers? Are we not leaking those ever since
> > "compositor-drm: Refcount drm_fb" at least one for each output
> > hot-unplug?
> >
> > Actually, maybe not that patch, but the following that makes
> > refcounting work for dumb buffers. Very subtle.  
> 
> I think what I need to do to make this work is to squash the 'refcount
> drm_fb' and 'refcount dumb buffer' patches together, as well as
> introduce a drm_fb_unref(fb->current) in drm_output_deinit from there.
> That seems to make all my fbs drop to 0 ref on unplug.

Sounds good.

> >> This is solved later with state by unref'ing state_cur, but I think
> >> you're right that when we add the refcounting, we need to balance it
> >> with an unref on destroy. Would doing that solve your concerns?  
> >
> > Yes, please!
> >
> > Then I'd like to go over that patch once more to verify the feeling of
> > not adding up is gone. (Referring to
> > https://lists.freedesktop.org/archives/wayland-devel/2017-February/033182.html
> > )  
> 
> I'm pretty sure that it's just one unref missing in deinit, and the
> rest is fine. If you can think of any others off the top of your head,
> please let me know before I rebase everything on top of this patch
> tomorrow?

No, that should be it.

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