[PATCH weston v10 13/61] compositor-drm: Introduce fb_last member

[Pekka Paalanen]

On Tue,  4 Apr 2017 17:54:31 +0100
Daniel Stone <daniels at collabora.com> wrote:

> Previously, framebuffers were stored as fb_current and fb_pending.
> In this scheme, current was the last buffer that the kernel/hardware had
> acknowledged displaying: a framebuffer would be created, set as
> fb_pending, and Weston would request the kernel display it. When the
> kernel signals that the request was completed and the hardware had made
> the buffer current (i.e. page_flip_handler / vblank_handler), we would
> unreference the old fb_current, and promote fb_pending to fb_current.
> 
> In other words, the view is 'which buffer has turned to light?'.
> 
> This patch changes them to a tristate of fb_last, fb_current and
> fb_pending, based around the kernel's view of the current state.
> fb_pending is used purely as a staging area for request construction;
> when the kernel acknowledges a request (e.g. drmModePageFlip returns 0),
> the previous buffer is moved to fb_last, and this new buffer to
> fb_current. When the kernel signals that the request has completed and
> the hardware has made the buffer current, we simply unreference and
> clear fb_last, without touching fb_current/fb_pending.
> 
> The view here is now 'which state is current in the kernel?'.
> 
> As all state changes are incremental on the last state submitted to the
> kernel, even if the hardware has not yet been able to make it current,
> this simplifies state tracking: all state submissions will always be
> relative to fb_current, rather than the previous
> (fb_pending) ? fb_pending : fb_current.
> 
> The use of fb_pending is strictly bounded between a repaint cycle
> (including a grouped set of repaints) beginning, and those repaints
> being flushed to the kernel.
> 
> fb_current will always be valid between an output's first repaint
> flush, and when a disable/destroy request has been processed. For a
> plane, it will be valid when a repaint cycle enabling that plane has
> been flushed, and when a repaint cycle disabling that plane has been
> flushed.
> 
> fb_last is only present when a repaint request for the output/plane has
> been submitted, but not yet completed by the hardware.
> 
> This is the same set of constructs which will be used for storing
> plane/output state objects in future patches.

Hi,

this explanation is wonderful. :-)


> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor-drm.c | 61 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 48 insertions(+), 13 deletions(-)
> 
> v10: Rewrite commit message. Add comments.
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index e27bc1d4..7004ef9a 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -190,7 +190,15 @@ struct drm_output {
>  	uint32_t gbm_format;
>  
>  	struct weston_plane fb_plane;
> -	struct drm_fb *fb_current, *fb_pending;
> +
> +	/* The last framebuffer submitted to the kernel for this CRTC. */
> +	struct drm_fb *fb_current;
> +	/* The previously-submitted framebuffer, where the hardware has not
> +	 * yet acknowledged display of fb_current. */
> +	struct drm_fb *fb_last;
> +	/* Framebuffer we are going to submit to the kernel when the current
> +	 * repaint is flushed. */
> +	struct drm_fb *fb_pending;
>  
>  	struct drm_fb *dumb[2];
>  	pixman_image_t *image[2];
> @@ -212,7 +220,6 @@ struct drm_sprite {
>  
>  	struct weston_plane plane;
>  
> -	struct drm_fb *fb_current, *fb_pending;
>  	struct drm_output *output;
>  	struct drm_backend *backend;
>  
> @@ -220,6 +227,15 @@ struct drm_sprite {
>  	uint32_t plane_id;
>  	uint32_t count_formats;
>  
> +	/* The last framebuffer submitted to the kernel for this plane. */
> +	struct drm_fb *fb_current;
> +	/* The previously-submitted framebuffer, where the hardware has not
> +	 * yet acknowledged display of fb_current. */
> +	struct drm_fb *fb_last;
> +	/* Framebuffer we are going to submit to the kernel when the current
> +	 * repaint is flushed. */
> +	struct drm_fb *fb_pending;
> +
>  	int32_t src_x, src_y;
>  	uint32_t src_w, src_h;
>  	uint32_t dest_x, dest_y;
> @@ -836,6 +852,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;
> @@ -861,6 +879,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;
>  
>  	if (output->pageflip_timer)
> @@ -879,6 +901,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. */
>  		if ((!s->fb_current && !s->fb_pending) ||
>  		    !drm_sprite_crtc_supported(output, s))
>  			continue;
> @@ -910,6 +934,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;
>  	}
>  
> @@ -1023,9 +1050,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) {
>  		/* Stop the pageflip timer instead of rearming it here */
> @@ -1057,9 +1084,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;
>  	}
>  
>  	output->page_flip_pending = 0;
> @@ -1593,10 +1619,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.
> +	 */
>  	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);

I was about to complain that these asserts could be randomly hit by
clients that invoke temporary video mode changes through fullscreen
scaling modes. But, that code has not existed for some time now.

The only user left for the mode switching API is the fullscreen shell,
which calls weston_output_mode_switch_to_{native,temporary}().

There is potential here to randomly break fullscreen shell and external
plugins that might be using this API. I'm not sure I care though.

> +	output->fb_last = output->fb_current = NULL;
>  
>  	if (b->use_pixman) {
>  		drm_output_fini_pixman(output);
> @@ -2632,8 +2664,9 @@ drm_output_deinit(struct weston_output *base)
>  	struct drm_output *output = to_drm_output(base);
>  	struct drm_backend *b = to_drm_backend(base->compositor);
>  
> -	/* output->fb_pending must not be set here;
> +	/* output->fb_last and output->fb_pending must not be set here;
>  	 * destroy_pending/disable_pending exist to guarantee exactly this. */
> +	assert(!output->fb_last);
>  	assert(!output->fb_pending);

First I was a little bit worried about these asserts in case we shut
down the compositor while a pageflip is pending, but it's harmless.
drm_output_destroy() just returns early, we never service any events
again, and exit the process. I tested and it doesn't abort.

>  	drm_fb_unref(output->fb_current);
>  	output->fb_current = NULL;
> @@ -2823,6 +2856,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;
> @@ -2854,8 +2888,9 @@ destroy_sprites(struct drm_backend *backend)
>  				sprite->plane_id,
>  				output->crtc_id, 0, 0,
>  				0, 0, 0, 0, 0, 0, 0, 0);
> +		assert(!sprite->fb_last);
> +		assert(!sprite->fb_pending);
>  		drm_fb_unref(sprite->fb_current);
> -		drm_fb_unref(sprite->fb_pending);
>  		weston_plane_release(&sprite->plane);
>  		free(sprite);
>  	}

Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

I shall wait till Monday until pushing this in case someone wants to
scream about the remote possibility of breaking video mode switching.


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/20170407/4942ae6a/attachment.sig>