Hi, On 22 March 2017 at 14:35, Pekka Paalanen <ppaala...@gmail.com> wrote: > On Fri, 3 Mar 2017 23:05:25 +0000 > Daniel Stone <dani...@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. 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). '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. 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. > 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. 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 ... ? 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 ... ? >> @@ -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. >> @@ -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. >> 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. 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? Cheers, Daniel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel