[PATCH weston v9 14/62] compositor-drm: Introduce fb_last member
Pekka Paalanen
ppaalanen at gmail.com
Wed Mar 22 14:35:23 UTC 2017
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.
>
> Signed-off-by: Daniel Stone <daniels at collabora.com>
>
> Differential Revision: https://phabricator.freedesktop.org/D1411
>
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
> libweston/compositor-drm.c | 42 ++++++++++++++++++++++++++++++------------
> 1 file changed, 30 insertions(+), 12 deletions(-)
Hi,
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.
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
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. ;-)
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 3f6fafc..0f47d4f 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -189,7 +189,7 @@ struct drm_output {
> uint32_t gbm_format;
>
> struct weston_plane fb_plane;
> - struct drm_fb *fb_current, *fb_pending;
> + struct drm_fb *fb_current, *fb_pending, *fb_last;
>
> struct drm_fb *dumb[2];
> pixman_image_t *image[2];
> @@ -209,7 +209,7 @@ struct drm_sprite {
>
> struct weston_plane plane;
>
> - struct drm_fb *fb_current, *fb_pending;
> + struct drm_fb *fb_current, *fb_pending, *fb_last;
> struct drm_output *output;
> struct drm_backend *backend;
>
> @@ -794,6 +794,8 @@ drm_output_repaint(struct weston_output *output_base,
> if (output->disable_pending || output->destroy_pending)
> return -1;
>
> + assert(!output->fb_last);
> +
> drm_output_render(output, damage);
> if (!output->fb_pending)
> return -1;
> @@ -819,6 +821,10 @@ drm_output_repaint(struct weston_output *output_base,
> goto err_pageflip;
> }
>
> + output->fb_last = output->fb_current;
> + output->fb_current = output->fb_pending;
> + output->fb_pending = NULL;
> +
> output->page_flip_pending = 1;
>
> drm_output_set_cursor(output);
> @@ -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.
> if ((!s->fb_current && !s->fb_pending) ||
> !drm_sprite_crtc_supported(output, s))
> continue;
> @@ -864,6 +872,9 @@ drm_output_repaint(struct weston_output *output_base,
> }
>
> s->output = output;
> + s->fb_last = s->fb_current;
> + s->fb_current = s->fb_pending;
> + s->fb_pending = NULL;
> output->vblank_pending = 1;
> }
>
> @@ -973,9 +984,9 @@ vblank_handler(int fd, unsigned int frame, unsigned int sec, unsigned int usec,
> drm_output_update_msc(output, frame);
> output->vblank_pending = 0;
>
> - drm_fb_unref(s->fb_current);
> - s->fb_current = s->fb_pending;
> - s->fb_pending = NULL;
> + assert(s->fb_last || s->fb_current);
> + drm_fb_unref(s->fb_last);
> + s->fb_last = NULL;
>
> if (!output->page_flip_pending) {
> ts.tv_sec = sec;
> @@ -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.
> }
>
> output->page_flip_pending = 0;
> @@ -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.
> 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.
>
> if (b->use_pixman) {
> drm_output_fini_pixman(output);
> @@ -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?
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().
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/20170322/dab6e663/attachment-0001.sig>
More information about the wayland-devel
mailing list