[PATCH weston v9 14/62] compositor-drm: Introduce fb_last member
Pekka Paalanen
ppaalanen at gmail.com
Fri Mar 24 10:30:02 UTC 2017
On Thu, 23 Mar 2017 15:56:55 +0000
Daniel Stone <daniel at fooishbar.org> wrote:
> Hi,
>
> On 22 March 2017 at 14:35, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Fri, 3 Mar 2017 23:05:25 +0000
> > Daniel Stone <daniels at collabora.com> wrote:
> >> Clean up some ambiguity around current/next: current could previously
> >> have referred to a buffer which was being displayed, or the last buffer
> >> being displayed whilst we waited for a configuration we'd requested to
> >> take effect.
> >>
> >> Introduce a new variable, fb_last, which exclusively holds the latter
> >> case, thus leaving us with three unambiguous members: fb_pending is used
> >> as a scratch space for a buffer we're about to post, fb_current holds
> >> the buffer we last requested to display (whether active yet or not), and
> >> fb_last is again scratch space for a buffer which is no longer being
> >> displayed after a previous configuration reqeust.
> >
> > the minor problem I have with this patch is that "current" is no longer
> > the current fb on screen if we have programmed a new flip. Your
> > new definition "(whether active yet or not)" contains very much an
> > ambiguity: whether it is actually on screen or not, which I think would
> > be a fairly important difference.
> >
> > 'current' has never referred to an FB that is no longer on screen,
> > because page_flip_handler() replaces it with the FB that just came on
> > screen.
>
> 'current' is no longer on screen, in between the hardware completing
> the pageflip, and Weston processing the kernel's flip-complete event.
>
> > I would argue an FB is "pending" until the flip completes. That makes
> > me think of the following terminology:
> >
> > pending: the FB in a programmed flip, but not completed the flip yet.
> >
> > current: the FB currently on screen
>
> The reason I landed on pending/current/last is because I was looking
> at it from the point of view of the kernel's state tracking. pending
> is something we haven't submitted to the kernel yet, current is what
> the kernel last accepted as state, and last is the thing which _was_
> current before the last update, but hasn't yet been released by the
> kernel. I'm trying to look at it from that point of view rather than
> 'FB turned to light' / pageflip events, because the latter is subject
> to a race we can't close, and is also entirely uninteresting to our
> state machinery.
This explanation is something I would like to see in the commit
message, because it sets the expections on what you are talking about,
from which point of view.
To me it's natural to think in terms of a buffer being prepared vs. in
use, and particularly as this patch is shuffling around buffer
references (drm_fb) the initial (wrong) point of view is enhanced.
I believe the intent will be more clear once the code starts shuffling
around state objects instead of just FBs.
> Before this patch, 'next' was a mixture of a staging area whilst
> building the state, and state which had been submitted to the kernel
> but we had not received a pageflip event for (your 'pending' above).
Right.
> 'current' was a mixture of what is the last set of state to be
> submitted, and what is the last-but-one state to be submitted.
Don't you mean "was submitted", not "to be submitted"?
> As soon as the kernel accepts an update, there is no possibility for
> it to later reject it. Any updates which are going to be made, will be
> made incrementally on the last state submitted. This patch means that
> the latter ('last state submitted') is _always_
> fb_current/state_current. Before this patch, this was either
> output->pending or output->current, depending on if we're in
> assign_planes/repaint or not.
That is another very good explanation.
> > And there is no need for a third state, because when 'current' stop
> > being current, it only needs to be unreffed, it does not need to be
> > stored anymore.
> >
> > But that's how it was, and you need a third state.
> >
> > If we look at it from the FB point of view:
> >
> > prepared: the FB (and state) we are building up for the next screen
> > update.
> >
> > queued: the FB in a programmed flip, but not completed the flip
> > yet.
> >
> > current: the FB currently on screen.
> >
> > 'prepared' would become 'queued' when we issue drmModePageFlip().
> > When the pageflip completes, 'current' is unreffed, and 'queued'
> > becomes 'current'.
> >
> >
> > However, that's a "wrong" point of view. I believe your ultimate aim is
> > *not* to track FB state, but to track collections of atomic state. An
> > atomic state set is constructed (fb_pending), programmed (fb_current),
> > and finished (fb_last). When an atomic state set is finished and
> > destroyed, the FB is only "current" as it is now on screen. You have no
> > need to keep the atomic state set that is currently on screen, do you?
> >
> > 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.
> 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?
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.
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.
> I need to retain the programmed (state_current, with the update not
> yet complete), because all future state updates are incremental upon
> the current state. So I need to be able to compare state_current to my
> target state in order to figure out what to do. Is that what you were
> asking instead ... ?
Umm... not really. :-)
> >> @@ -833,6 +839,8 @@ drm_output_repaint(struct weston_output *output_base,
> >> .request.sequence = 1,
> >> };
> >>
> >> + /* XXX: Set output much earlier, so we don't attempt to place
> >> + * planes on entirely the wrong output. */
> >
> > 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?
> >> @@ -1003,9 +1014,8 @@ page_flip_handler(int fd, unsigned int frame,
> >> * we just want to page flip to the current buffer to get an accurate
> >> * timestamp */
> >> if (output->page_flip_pending) {
> >> - drm_fb_unref(output->fb_current);
> >> - output->fb_current = output->fb_pending;
> >> - output->fb_pending = NULL;
> >> + drm_fb_unref(output->fb_last);
> >> + output->fb_last = NULL;
> >
> > Hmm, would this change actually make output->page_flip_pending
> > unnecessary? At least here? Hmm, probably not for this patch, anyway.
>
> Probably not - see 16/62 for how page_flip_pending is, at this point
> in the series, frustratingly special.
>
> >> @@ -1535,10 +1545,16 @@ drm_output_switch_mode(struct weston_output *output_base, struct weston_mode *mo
> >> output->base.current_mode->flags =
> >> WL_OUTPUT_MODE_CURRENT | WL_OUTPUT_MODE_PREFERRED;
> >>
> >> - /* reset rendering stuff. */
> >> + /* XXX: This drops our current buffer too early, before we've started
> >> + * displaying it. Ideally this should be much more atomic and
> >> + * integrated with a full repaint cycle, rather than doing a
> >> + * sledgehammer modeswitch first, and only later showing new
> >> + * content.
> >> + */
> >
> > True dat.
>
> Also it's not introduced by this patch; it's pre-existing behaviour
> which I have lovingly preserved at this point.
Yup, all good.
> >> drm_fb_unref(output->fb_current);
> >> - drm_fb_unref(output->fb_pending);
> >> - output->fb_current = output->fb_pending = NULL;
> >> + assert(!output->fb_last);
> >> + assert(!output->fb_pending);
> >> + output->fb_last = output->fb_current = NULL;
> >
> > Unnecessary to set fb_last to NULL if it already must be NULL.
>
> Yes, true.
>
> >> @@ -2753,6 +2769,7 @@ create_sprites(struct drm_backend *b)
> >>
> >> sprite->possible_crtcs = plane->possible_crtcs;
> >> sprite->plane_id = plane->plane_id;
> >> + sprite->fb_last = NULL;
> >> sprite->fb_current = NULL;
> >> sprite->fb_pending = NULL;
> >> sprite->backend = b;
> >> @@ -2784,8 +2801,9 @@ destroy_sprites(struct drm_backend *backend)
> >> sprite->plane_id,
> >> output->crtc_id, 0, 0,
> >> 0, 0, 0, 0, 0, 0, 0, 0);
> >> + drm_fb_unref(sprite->fb_last);
> >> drm_fb_unref(sprite->fb_current);
> >> - drm_fb_unref(sprite->fb_pending);
> >> + assert(!sprite->fb_pending);
> >> weston_plane_release(&sprite->plane);
> >> free(sprite);
> >> }
> >
> > Shouldn't drm_output_deinit() have some asserts or unrefs added?
>
> Yes, probably.
>
> > 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.
> 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
)
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/20170324/1bb3b375/attachment.sig>
More information about the wayland-devel
mailing list