[PATCH v14 35/41] compositor-drm: Add modes to drm_output_propose_state

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:52 +0000
Daniel Stone <daniels at collabora.com> wrote:

> Add support for multiple modes: toggling whether or not the renderer
> and/or planes are acceptable. This will be used to implement a smarter
> plane-placement heuristic when we have support for testing output
> states.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor-drm.c | 39 +++++++++++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 71027a5ae..ff70f9c29 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -1766,6 +1766,11 @@ drm_output_assign_state(struct drm_output_state *state,
>  	}
>  }
>  
> +enum drm_output_propose_state_mode {
> +	DRM_OUTPUT_PROPOSE_STATE_MIXED, /**< mix renderer & planes */
> +	DRM_OUTPUT_PROPOSE_STATE_PLANES_ONLY, /**< only assign to planes */
> +};

Hi,

what is the reason to have a planes-only mode? I saw the patch
"compositor-drm: Add test-only mode to state application" will first
attempt a planes-only mode and then fall back to mixed mode. Why does
the planes-only mode not fall out of mixed mode naturally when it runs
out of views to show? This would be good to explain in the commit
message to justify the modes.

> +
>  static struct weston_plane *
>  drm_output_prepare_scanout_view(struct drm_output_state *output_state,
>  				struct weston_view *ev)
> @@ -3049,13 +3054,17 @@ err:
>  
>  static struct drm_output_state *
>  drm_output_propose_state(struct weston_output *output_base,
> -			 struct drm_pending_state *pending_state)
> +			 struct drm_pending_state *pending_state,
> +			 enum drm_output_propose_state_mode mode)
>  {
>  	struct drm_output *output = to_drm_output(output_base);
> +	struct drm_backend *b = to_drm_backend(output_base->compositor);
>  	struct drm_output_state *state;
>  	struct weston_view *ev;
>  	pixman_region32_t surface_overlap, renderer_region, occluded_region;
>  	struct weston_plane *primary = &output_base->compositor->primary_plane;
> +	bool renderer_ok = (mode != DRM_OUTPUT_PROPOSE_STATE_PLANES_ONLY);
> +	bool planes_ok = !b->sprites_are_broken;

Would you want to remove the sprites_are_broken check from
drm_output_prepare_overlay_view()?

>  
>  	assert(!output->state_last);
>  	state = drm_output_state_duplicate(output->state_cur,
> @@ -3118,36 +3127,49 @@ drm_output_propose_state(struct weston_output *output_base,
>  			next_plane = primary;
>  		pixman_region32_fini(&surface_overlap);
>  
> -		if (next_plane == NULL)
> +		/* The cursor plane is 'special' in the sense that we can still
> +		 * place it in the legacy API, and we gate that with a separate
> +		 * cursors_are_broken flag. */
> +		if (next_plane == NULL && !b->cursors_are_broken)

Would you want to remove the cursors_are_broken check from
drm_output_prepare_cursor_view()?

>  			next_plane = drm_output_prepare_cursor_view(state, ev);
>  
>  		if (next_plane == NULL && !drm_view_is_opaque(ev))
>  			next_plane = primary;
>  
> +		if (next_plane == NULL && !planes_ok)
> +			next_plane = primary;

This prevents promoting fullscreen surfaces to the primary plane when
sprites_are_broken, which it did not before.

> +
>  		if (next_plane == NULL)
>  			next_plane = drm_output_prepare_scanout_view(state, ev);
>  
>  		if (next_plane == NULL)
>  			next_plane = drm_output_prepare_overlay_view(state, ev);
>  
> -		if (next_plane == NULL)
> -			next_plane = primary;
> +		if (!next_plane || next_plane == primary) {
> +			if (!renderer_ok)
> +				goto err;

This path is missing fini for clipped_view.

>  
> -		if (next_plane == primary)
>  			pixman_region32_union(&renderer_region,
>  					      &renderer_region,
>  					      &clipped_view);
> -		else if (output->cursor_plane &&
> -			 next_plane != &output->cursor_plane->base)
> +		} else if (output->cursor_plane &&
> +			   next_plane != &output->cursor_plane->base) {
>  			pixman_region32_union(&occluded_region,
>  					      &occluded_region,
>  					      &clipped_view);
> +		}
>  		pixman_region32_fini(&clipped_view);
>  	}
>  	pixman_region32_fini(&renderer_region);
>  	pixman_region32_fini(&occluded_region);
>  
>  	return state;
> +
> +err:
> +	pixman_region32_fini(&renderer_region);
> +	pixman_region32_fini(&occluded_region);
> +	drm_output_state_free(state);
> +	return NULL;
>  }
>  
>  static void
> @@ -3161,7 +3183,8 @@ drm_assign_planes(struct weston_output *output_base, void *repaint_data)
>  	struct weston_view *ev;
>  	struct weston_plane *primary = &output_base->compositor->primary_plane;
>  
> -	state = drm_output_propose_state(output_base, pending_state);
> +	state = drm_output_propose_state(output_base, pending_state,
> +					 DRM_OUTPUT_PROPOSE_STATE_MIXED);
>  
>  	wl_list_for_each(ev, &output_base->compositor->view_list, link) {
>  		struct drm_plane *target_plane = NULL;

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/20180126/5dcc671e/attachment.sig>