[PATCH weston v10 15/61] compositor-drm: Clean up page_flip_pending path

[Pekka Paalanen]

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

> page_flip_pending is only be set when do a pageflip to a newly-rendered
> buffer; if the flag is not set, we have landed in the start_repaint_loop
> path where the vblank query fails, and thus we must pageflip to the same
> buffer.
> 
> This test was not sufficient for what it was supposed to guard:
> releasing framebuffers back. When using client-supplied framebuffers, it
> is possible to reuse the same buffer multiple times, and we would send a
> framebuffer-release event too early.
> 
> However, since we have a properly reference-counted drm_fb now, we can
> just drop this test, and rely on the reference counting to prevent
> too-early release of client framebuffers.
> 
> page_flip_pending now becomes exactly what the name suggests: a flag
> which indicates whether or not we are expecting a pageflip event. Add
> asserts here to verify that we never receive a pageflip event we weren't
> expecting.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> 
> Differential Revision: https://phabricator.freedesktop.org/D1417
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor-drm.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 21415775..88975007 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -883,6 +883,7 @@ drm_output_repaint(struct weston_output *output_base,
>  	output->fb_current = output->fb_pending;
>  	output->fb_pending = NULL;
>  
> +	assert(!output->page_flip_pending);
>  	output->page_flip_pending = 1;
>  
>  	if (output->pageflip_timer)
> @@ -1008,6 +1009,9 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
>  	 */
>  	fb_id = output->fb_current->fb_id;
>  
> +	assert(!output->page_flip_pending);
> +	assert(!output->fb_last);
> +
>  	if (drmModePageFlip(backend->drm.fd, output->crtc_id, fb_id,
>  			    DRM_MODE_PAGE_FLIP_EVENT, output) < 0) {
>  		weston_log("queueing pageflip failed: %m\n");
> @@ -1018,6 +1022,9 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
>  		wl_event_source_timer_update(output->pageflip_timer,
>  		                             backend->pageflip_timeout);
>  
> +	output->fb_last = drm_fb_ref(output->fb_current);
> +	output->page_flip_pending = 1;
> +
>  	return;
>  
>  finish_frame:
> @@ -1081,16 +1088,12 @@ page_flip_handler(int fd, unsigned int frame,
>  
>  	drm_output_update_msc(output, frame);
>  
> -	/* We don't set page_flip_pending on start_repaint_loop, in that case
> -	 * 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_last);
> -		output->fb_last = NULL;
> -	}
> -
> +	assert(output->page_flip_pending);
>  	output->page_flip_pending = 0;
>  
> +	drm_fb_unref(output->fb_last);
> +	output->fb_last = NULL;
> +
>  	if (output->destroy_pending)
>  		drm_output_destroy(&output->base);
>  	else if (output->disable_pending)

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

Btw. I'll get rid of the phab tags and dual-SoB when I push. Unless
someone else pushes first. :-)

The reason we cannot just remove page_flip_pending completely in favour
of the other variables like fb_last is that then we would miss the
initial flip from nothing to the very first FB, right?


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/c34796dd/attachment.sig>