[PATCH weston v12 05/40] compositor-drm: Use drm_plane for scanout plane

[Pekka Paalanen]

On Tue, 26 Sep 2017 18:15:38 +0100
Daniel Stone <daniels at collabora.com> wrote:

> Use a real drm_plane to back the scanout plane, displacing
> output->fb_{last,cur,pending} to their plane-tracked equivalents.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor-drm.c | 167 +++++++++++++++++++++++++++------------------
>  1 file changed, 101 insertions(+), 66 deletions(-)

Hi Daniel,

my comments from last time are mostly fixed.

> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 33555d00..23ac2dec 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c

> @@ -2565,10 +2605,6 @@ drm_output_switch_mode(struct weston_output *output_base, struct weston_mode *mo
>  	 *      sledgehammer modeswitch first, and only later showing new
>  	 *      content.
>  	 */
> -	drm_fb_unref(output->fb_current);
> -	assert(!output->fb_last);
> -	assert(!output->fb_pending);
> -	output->fb_last = output->fb_current = NULL;

I know this path is all kinds of broken, but how about at least setting

	output->state_invalid = true;

here to guarantee we hit the SetCrtc path the repaint?

>  
>  	if (b->use_pixman) {
>  		drm_output_fini_pixman(output);
> @@ -2909,14 +2945,16 @@ drm_output_find_special_plane(struct drm_backend *b, struct drm_output *output,
>  		 * same plane for two outputs. */
>  		wl_list_for_each(tmp, &b->compositor->pending_output_list,
>  				 base.link) {
> -			if (tmp->cursor_plane == plane) {
> +			if (tmp->cursor_plane == plane ||
> +			    tmp->scanout_plane == plane) {
>  				found_elsewhere = true;
>  				break;
>  			}
>  		}
>  		wl_list_for_each(tmp, &b->compositor->output_list,
>  				 base.link) {
> -			if (tmp->cursor_plane == plane) {
> +			if (tmp->cursor_plane == plane ||
> +			    tmp->scanout_plane == plane) {
>  				found_elsewhere = true;
>  				break;
>  			}
> @@ -3839,8 +3877,6 @@ drm_output_enable(struct weston_output *base)
>  	    output->connector->connector_type == DRM_MODE_CONNECTOR_eDP)
>  		output->base.connection_internal = true;
>  
> -	weston_plane_init(&output->scanout_plane, b->compositor, 0, 0);
> -
>  	if (output->cursor_plane)
>  		weston_compositor_stack_plane(b->compositor,
>  					      &output->cursor_plane->base,
> @@ -3848,7 +3884,8 @@ drm_output_enable(struct weston_output *base)
>  	else
>  		b->cursors_are_broken = 1;
>  
> -	weston_compositor_stack_plane(b->compositor, &output->scanout_plane,
> +	weston_compositor_stack_plane(b->compositor,
> +				      &output->scanout_plane->base,
>  				      &b->compositor->primary_plane);
>  
>  	weston_log("Output %s, (connector %d, crtc %d)\n",
> @@ -3877,20 +3914,11 @@ 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_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);
> -	drm_fb_unref(output->fb_current);
> -	output->fb_current = NULL;
> -
>  	if (b->use_pixman)
>  		drm_output_fini_pixman(output);
>  	else
>  		drm_output_fini_egl(output);
>  
> -	weston_plane_release(&output->scanout_plane);
> -

I would assume this has the same issues as the cursor plane patch does.
Deinit should undo weston_compositor_stack_plane().

It probably has the same shutdown problem as well: destroy_sprites()
destroying the plane, then output destruction hitting use-after-free.

>  	if (output->cursor_plane) {
>  		/* Turn off hardware cursor */
>  		drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
> @@ -3960,10 +3988,6 @@ drm_output_disable(struct weston_output *base)
>  	if (output->base.enabled)
>  		drm_output_deinit(&output->base);
>  
> -	assert(!output->fb_last);
> -	assert(!output->fb_current);
> -	assert(!output->fb_pending);
> -
>  	output->disable_pending = 0;
>  
>  	weston_log("Disabling output %s\n", output->base.name);
> @@ -4058,6 +4082,16 @@ create_output_for_connector(struct drm_backend *b,
>  		}
>  	}
>  
> +	output->scanout_plane =
> +		drm_output_find_special_plane(b, output,
> +					      WDRM_PLANE_TYPE_PRIMARY);
> +	if (!output->scanout_plane) {
> +		weston_log("Failed to find primary plane for output %s\n",
> +			   output->base.name);
> +		drm_output_destroy(&output->base);
> +		return -1;
> +	}
> +
>  	/* Failing to find a cursor plane is not fatal, as we'll fall back
>  	 * to software cursor. */
>  	output->cursor_plane =


Other than those, seems fine to me.

I also re-read your scene-graph planes vs. KMS planes comment from last
time, and yeah, agreed. It's a little funny that primary_plane (by
libweston core) is used with renderer damage tracking even when
scanout_plane is used for scanning it out. Maybe when I or someone gets
to fixing the renderer-based clone mode it will get cleaned up.


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/20171002/0166a3b2/attachment.sig>