[PATCH v14 23/41] compositor-drm: Use plane_coords_for_view for cursor

[Pekka Paalanen]

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

> Use the new helper to populate the cursor state as well, with some
> special-case handling to account for how we always upload a full-size
> BO.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> Reported-by: Derek Foreman <derekf at osg.samsung.com>
> ---
>  libweston/compositor-drm.c | 48 +++++++++++++++++++++++-----------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 

Hi,

I like the idea, but there are some details.

> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index d97efd761..dbe53513b 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -2827,10 +2827,8 @@ drm_output_prepare_cursor_view(struct drm_output_state *output_state,
>  	struct drm_backend *b = to_drm_backend(output->base.compositor);
>  	struct drm_plane *plane = output->cursor_plane;
>  	struct drm_plane_state *plane_state;
> -	struct weston_buffer_viewport *viewport = &ev->surface->buffer_viewport;
>  	struct wl_shm_buffer *shmbuf;
>  	bool needs_update = false;
> -	float x, y;
>  
>  	if (!plane)
>  		return NULL;
> @@ -2860,16 +2858,6 @@ drm_output_prepare_cursor_view(struct drm_output_state *output_state,
>  	if (wl_shm_buffer_get_format(shmbuf) != WL_SHM_FORMAT_ARGB8888)
>  		return NULL;
>  
> -	if (output->base.transform != WL_OUTPUT_TRANSFORM_NORMAL)
> -		return NULL;
> -	if (ev->transform.enabled &&
> -	    (ev->transform.matrix.type > WESTON_MATRIX_TRANSFORM_TRANSLATE))
> -		return NULL;
> -	if (viewport->buffer.scale != output->base.current_scale)
> -		return NULL;
> -	if (ev->geometry.scissor_enabled)
> -		return NULL;
> -
>  	if (ev->surface->width > b->cursor_width ||
>  	    ev->surface->height > b->cursor_height)
>  		return NULL;

This check left in is comparing surface units with buffer units, so it
has the same problem as cursor_bo_update() below. I also think it is
somewhat redundant with the src_w/src_h checks below.

> @@ -2880,6 +2868,26 @@ drm_output_prepare_cursor_view(struct drm_output_state *output_state,
>  	if (plane_state && plane_state->fb)
>  		return NULL;
>  
> +	/* We can't scale with the legacy API, and we don't try to account for
> +	 * simple cropping/translation in cursor_bo_update. */
> +	plane_state->output = output;
> +	drm_plane_state_coords_for_view(plane_state, ev);
> +	if (plane_state->src_x != 0 || plane_state->src_y != 0 ||
> +	    plane_state->src_w > (unsigned) b->cursor_width << 16 ||
> +	    plane_state->src_h > (unsigned) b->cursor_height << 16 ||
> +	    plane_state->src_w != plane_state->dest_w << 16 ||
> +	    plane_state->src_h != plane_state->dest_h << 16)
> +		goto err;
> +

cursor_bo_update() has this:

	for (i = 0; i < ev->surface->height; i++)
		memcpy(buf + i * b->cursor_width,
		       s + i * stride,
		       ev->surface->width * 4);

Using surface->width and surface->height means that it cannot work on
buffer scale > 1. I think we didn't catch that before or after this
patch.

Should drm_output_prepare_cursor_view() not check that src size matches
the size cursor_bo_update() is going to copy?


> +	/* The cursor API is somewhat special: in cursor_bo_update(), we upload
> +	 * a buffer which is always cursor_width x cursor_height, even if the
> +	 * surface we want to promote is actually smaller than this. Manually
> +	 * mangle the plane state to deal with this. */
> +	plane_state->src_w = b->cursor_width << 16;
> +	plane_state->src_h = b->cursor_height << 16;
> +	plane_state->dest_w = b->cursor_width;
> +	plane_state->dest_h = b->cursor_height;
> +
>  	/* Since we're setting plane state up front, we need to work out
>  	 * whether or not we need to upload a new cursor. We can't use the
>  	 * plane damage, since the planes haven't actually been calculated
> @@ -2896,26 +2904,18 @@ drm_output_prepare_cursor_view(struct drm_output_state *output_state,
>  	}
>  
>  	output->cursor_view = ev;
> -	weston_view_to_global_float(ev, 0, 0, &x, &y);
> -	plane->base.x = x;
> -	plane->base.y = y;
>  
>  	plane_state->fb =
>  		drm_fb_ref(output->gbm_cursor_fb[output->current_cursor]);
> -	plane_state->output = output;
> -	plane_state->src_x = 0;
> -	plane_state->src_y = 0;
> -	plane_state->src_w = b->cursor_width << 16;
> -	plane_state->src_h = b->cursor_height << 16;
> -	plane_state->dest_x = (x - output->base.x) * output->base.current_scale;
> -	plane_state->dest_y = (y - output->base.y) * output->base.current_scale;
> -	plane_state->dest_w = b->cursor_width;
> -	plane_state->dest_h = b->cursor_height;
>  
>  	if (needs_update)
>  		cursor_bo_update(b, plane_state->fb->bo, ev);
>  
>  	return &plane->base;
> +
> +err:
> +	drm_plane_state_put_back(plane_state);
> +	return NULL;
>  }
>  
>  static void


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