[PATCH v16 08/23] compositor-drm: Use plane_coords_for_view for cursor

[Pekka Paalanen]

On Thu,  5 Jul 2018 18:16:35 +0100
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.
> 
> As this now fully takes care of buffer transformations, HiDPI client
> cursors work, and we also clip the cursor plane completely to CRTC
> bounds.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> Reported-by: Derek Foreman <derekf at osg.samsung.com>
> Tested-by: Emre Ucan <eucan at de.adit-jv.com>
> Fixes: https://gitlab.freedesktop.org/wayland/weston/issues/118
> ---
>  libweston/compositor-drm.c | 68 ++++++++++++++++++--------------------
>  1 file changed, 32 insertions(+), 36 deletions(-)

The subject should say plane_state_coords_for_view.

> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 2927a0ebf..9aab6e523 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -2841,14 +2841,14 @@ err:
>  /**
>   * Update the image for the current cursor surface
>   *
> - * @param b DRM backend structure
> - * @param bo GBM buffer object to write into
> - * @param ev View to use for cursor image
> + * @param plane_state DRM cursor plane state
> + * @param ev Source view for cursor
>   */
>  static void
> -cursor_bo_update(struct drm_backend *b, struct gbm_bo *bo,
> -		 struct weston_view *ev)
> +cursor_bo_update(struct drm_plane_state *plane_state, struct weston_view *ev)
>  {
> +	struct drm_backend *b = plane_state->plane->backend;
> +	struct gbm_bo *bo = plane_state->fb->bo;
>  	struct weston_buffer *buffer = ev->surface->buffer_ref.buffer;
>  	uint32_t buf[b->cursor_width * b->cursor_height];
>  	int32_t stride;
> @@ -2857,18 +2857,16 @@ cursor_bo_update(struct drm_backend *b, struct gbm_bo *bo,
>  
>  	assert(buffer && buffer->shm_buffer);
>  	assert(buffer->shm_buffer == wl_shm_buffer_get(buffer->resource));
> -	assert(ev->surface->width <= b->cursor_width);
> -	assert(ev->surface->height <= b->cursor_height);

Rather than removing these asserts, it would be better to fix them to
use buffer size. It would catch overflowing buf.

An assert to ensure bytes-per-pixel is the hardcoded four below would
be a nice addition.

>  
>  	memset(buf, 0, sizeof buf);
>  	stride = wl_shm_buffer_get_stride(buffer->shm_buffer);
>  	s = wl_shm_buffer_get_data(buffer->shm_buffer);
>  
>  	wl_shm_buffer_begin_access(buffer->shm_buffer);
> -	for (i = 0; i < ev->surface->height; i++)
> +	for (i = 0; i < buffer->height; i++)
>  		memcpy(buf + i * b->cursor_width,
>  		       s + i * stride,
> -		       ev->surface->width * 4);
> +		       buffer->width * 4);
>  	wl_shm_buffer_end_access(buffer->shm_buffer);
>  
>  	if (gbm_bo_write(bo, buf, sizeof buf) < 0)
> @@ -2883,10 +2881,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;
> @@ -2916,26 +2912,25 @@ 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;
> -

I'm happy to see checks like above...

>  	plane_state =
>  		drm_output_state_get_plane(output_state, output->cursor_plane);
>  
>  	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;
> +	if (!drm_plane_state_coords_for_view(plane_state, ev))
> +		goto err;
> +
> +	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;
> +

...are being replaced with checks like this. It's a step towards
inspecting the full end-to-end transformation.

Looks good, so in any case:

Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>


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