[PATCH v14 22/41] compositor-drm: Use plane_state_coords_for_view for scanout

[Pekka Paalanen]

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

> Use the shiny new helper we have for working through scanout as well.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor-drm.c | 81 ++++++++++++++--------------------------------
>  1 file changed, 25 insertions(+), 56 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 10bc53ba0..d97efd761 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -1575,13 +1575,6 @@ drm_output_assign_state(struct drm_output_state *state,
>  	}
>  }
>  
> -static int
> -drm_view_transform_supported(struct weston_view *ev)
> -{
> -	return !ev->transform.enabled ||
> -		(ev->transform.matrix.type < WESTON_MATRIX_TRANSFORM_ROTATE);
> -}
> -
>  static bool
>  drm_view_is_opaque(struct weston_view *ev)
>  {
> @@ -1613,7 +1606,6 @@ drm_output_prepare_scanout_view(struct drm_output_state *output_state,
>  	struct drm_plane *scanout_plane = output->scanout_plane;
>  	struct drm_plane_state *state;
>  	struct weston_buffer *buffer = ev->surface->buffer_ref.buffer;
> -	struct weston_buffer_viewport *viewport = &ev->surface->buffer_viewport;
>  	struct gbm_bo *bo;
>  
>  	/* Don't import buffers which span multiple outputs. */
> @@ -1629,28 +1621,6 @@ drm_output_prepare_scanout_view(struct drm_output_state *output_state,
>  	if (wl_shm_buffer_get(buffer->resource))
>  		return NULL;
>  
> -	/* Make sure our view is exactly compatible with the output. */
> -	if (ev->geometry.x != output->base.x ||
> -	    ev->geometry.y != output->base.y)
> -		return NULL;
> -	if (buffer->width != output->base.current_mode->width ||
> -	    buffer->height != output->base.current_mode->height)
> -		return NULL;
> -
> -	if (ev->transform.enabled)
> -		return NULL;
> -	if (ev->geometry.scissor_enabled)
> -		return NULL;
> -	if (viewport->buffer.transform != output->base.transform)
> -		return NULL;
> -	if (viewport->buffer.scale != output->base.current_scale)
> -		return NULL;
> -	if (!drm_view_transform_supported(ev))
> -		return NULL;
> -
> -	if (ev->alpha != 1.0f)
> -		return NULL;
> -
>  	state = drm_output_state_get_plane(output_state, scanout_plane);
>  	if (state->fb) {
>  		/* If there is already a framebuffer on the scanout plane,
> @@ -1660,44 +1630,50 @@ drm_output_prepare_scanout_view(struct drm_output_state *output_state,
>  		return NULL;
>  	}
>  
> +	state->output = output;
> +	drm_plane_state_coords_for_view(state, ev);
> +
> +	/* The legacy API does not let us perform cropping or scaling. */

Hi Daniel,

should this not be checking the src coordinates against the buffer
dimensions as well? If the buffer is bigger than the mode but clipped
to the mode size, it might pass all the checks here.

Sources of clipping: layer mask, view mask (scissor).

> +	if (state->src_x != 0 || state->src_y != 0 ||
> +	    state->src_w != state->dest_w << 16 ||
> +	    state->src_h != state->dest_h << 16 ||
> +	    state->dest_x != 0 || state->dest_y != 0 ||
> +	    state->dest_w != (unsigned) output->base.current_mode->width ||
> +	    state->dest_h != (unsigned) output->base.current_mode->height)
> +		goto err;
> +
> +	if (ev->alpha != 1.0f)
> +		goto err;
> +
>  	bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_WL_BUFFER,
>  			   buffer->resource, GBM_BO_USE_SCANOUT);
>  
>  	/* Unable to use the buffer for scanout */
>  	if (!bo)
> -		return NULL;
> +		goto err;
>  
>  	state->fb = drm_fb_get_from_bo(bo, b, drm_view_is_opaque(ev),
>  				       BUFFER_CLIENT);
>  	if (!state->fb) {
> -		drm_plane_state_put_back(state);
> +		/* We need to explicitly destroy the BO. */
>  		gbm_bo_destroy(bo);
> -		return NULL;
> +		goto err;
>  	}
>  
>  	/* Can't change formats with just a pageflip */
>  	if (state->fb->format->format != output->gbm_format) {
>  		/* No need to destroy the GBM BO here, as it's now owned
>  		 * by the FB. */
> -		drm_plane_state_put_back(state);
> -		return NULL;
> +		goto err;
>  	}
>  
>  	drm_fb_set_buffer(state->fb, buffer);
>  
> -	state->output = output;
> -
> -	state->src_x = 0;
> -	state->src_y = 0;
> -	state->src_w = state->fb->width << 16;
> -	state->src_h = state->fb->height << 16;
> -
> -	state->dest_x = 0;
> -	state->dest_y = 0;
> -	state->dest_w = output->base.current_mode->width;
> -	state->dest_h = output->base.current_mode->height;
> -
>  	return &scanout_plane->base;
> +
> +err:
> +	drm_plane_state_put_back(state);
> +	return NULL;
>  }

I like the above, it's a good direction. The motivation of this patch
is to simplify the code, and it is not supposed to introduce any
behavioral changes like allowing more cases through, right? I'd like to
see that mentioned in the commit message.

The below looks like it's reverting a fix from upstream master, a
rebasing mistake?

>  
>  static struct drm_fb *
> @@ -3006,7 +2982,6 @@ drm_assign_planes(struct weston_output *output_base, void *repaint_data)
>  	struct weston_view *ev, *next;
>  	pixman_region32_t overlap, surface_overlap;
>  	struct weston_plane *primary, *next_plane;
> -	bool picked_scanout = false;
>  
>  	assert(!output->state_last);
>  	state = drm_output_state_duplicate(output->state_cur,
> @@ -3054,19 +3029,13 @@ drm_assign_planes(struct weston_output *output_base, void *repaint_data)
>  					  &ev->transform.boundingbox);
>  
>  		next_plane = NULL;
> -		if (pixman_region32_not_empty(&surface_overlap) || picked_scanout)
> +		if (pixman_region32_not_empty(&surface_overlap))
>  			next_plane = primary;
>  		if (next_plane == NULL)
>  			next_plane = drm_output_prepare_cursor_view(state, ev);
>  
> -		/* If a higher-stacked view already got assigned to scanout, it's incorrect to
> -		 * assign a subsequent (lower-stacked) view to scanout.
> -		 */
> -		if (next_plane == NULL) {
> +		if (next_plane == NULL)
>  			next_plane = drm_output_prepare_scanout_view(state, ev);
> -			if (next_plane)
> -				picked_scanout = true;
> -		}
>  
>  		if (next_plane == NULL)
>  			next_plane = drm_output_prepare_overlay_view(state, ev);


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