[PATCH v14 21/41] [XXX] compositor-drm: Only disallow scaling for overlay planes

[Pekka Paalanen]

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

> Now we have a more comprehensive overview of the transform we're going
> to apply, use this to explicitly disallow scaling and rotation.
> 
> XXX: This does not actually disallow rotation for square surfaces.
>      We would have to build up a full buffer->view->output transform
>      matrix, and then analyse that.

Hi,

I think the most important bit is missing in the commit message: why?
What's wrong with the old checks?

Is it only the wp_viewport state or something more?

Maybe we could use this patch and add a helper that explicitly checks
all the little bits of transformations to ensure there is no rotation?
It would not have to allow all cases, e.g. if the transformation matrix
inverts the buffer/output transform rotation we could just ignore and
reject that case. The helper would essentially be:

bool
supported()
{
	/* XXX: This is safe, but still rejects some valid configurations */
	if (viewport->buffer.transform != output->base.transform)
		return false;
	if (!drm_view_transform_supported(ev))
		return false;
	return true;
}

wp_viewport cannot produce a rotation, so there is no need to check it.

That might allow to keep this and the following patches that unify the
transformation handling between primary/overlay/cursor planes.

It might even pay off to integrate this check into
drm_plane_state_coords_for_view(), adding a success/reject return
value. That function does assume that there is no end-to-end rotation
at all. It won't work for arbitrarily rotated views, and for 90-degree
step rotations it may produce negative width and/or height to imply a
flip. I think it should explicitly fail rather than produce garbage
when the preconditions are violated.


Thanks,
pq

> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor-drm.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 001eb5278..10bc53ba0 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -2684,7 +2684,6 @@ drm_output_prepare_overlay_view(struct drm_output_state *output_state,
>  {
>  	struct drm_output *output = output_state->output;
>  	struct weston_compositor *ec = output->base.compositor;
> -	struct weston_buffer_viewport *viewport = &ev->surface->buffer_viewport;
>  	struct drm_backend *b = to_drm_backend(ec);
>  	struct wl_resource *buffer_resource;
>  	struct drm_plane *p;
> @@ -2710,13 +2709,6 @@ drm_output_prepare_overlay_view(struct drm_output_state *output_state,
>  	if (wl_shm_buffer_get(buffer_resource))
>  		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;
>  
> @@ -2740,6 +2732,12 @@ drm_output_prepare_overlay_view(struct drm_output_state *output_state,
>  	if (!state)
>  		return NULL;
>  
> +	state->output = output;
> +	drm_plane_state_coords_for_view(state, ev);
> +	if (state->src_w != state->dest_w << 16 ||
> +	    state->src_h != state->dest_h << 16)
> +		goto err;
> +
>  	if ((dmabuf = linux_dmabuf_buffer_get(buffer_resource))) {
>  #ifdef HAVE_GBM_FD_IMPORT
>  		/* XXX: TODO:
> @@ -2769,7 +2767,7 @@ drm_output_prepare_overlay_view(struct drm_output_state *output_state,
>  		if (dmabuf->attributes.n_planes != 1 ||
>                      dmabuf->attributes.offset[0] != 0 ||
>  		    dmabuf->attributes.flags)
> -			return NULL;
> +			goto err;
>  
>  		bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_FD, &gbm_dmabuf,
>  				   GBM_BO_USE_SCANOUT);
> @@ -2785,8 +2783,12 @@ drm_output_prepare_overlay_view(struct drm_output_state *output_state,
>  
>  	state->fb = drm_fb_get_from_bo(bo, b, drm_view_is_opaque(ev),
>  				       BUFFER_CLIENT);
> -	if (!state->fb)
> +	if (!state->fb) {
> +		/* Destroy the BO as we've allocated it, but it won't yet
> +		 * be deallocated by the state. */
> +		gbm_bo_destroy(bo);
>  		goto err;
> +	}
>  
>  	/* Check whether the format is supported */
>  	for (i = 0; i < p->count_formats; i++)
> @@ -2797,15 +2799,10 @@ drm_output_prepare_overlay_view(struct drm_output_state *output_state,
>  
>  	drm_fb_set_buffer(state->fb, ev->surface->buffer_ref.buffer);
>  
> -	state->output = output;
> -	drm_plane_state_coords_for_view(state, ev);
> -
>  	return &p->base;
>  
>  err:
>  	drm_plane_state_put_back(state);
> -	if (bo)
> -		gbm_bo_destroy(bo);
>  	return NULL;
>  }
>  

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