On Wed, 20 Dec 2017 12:26:40 +0000 Daniel Stone <[email protected]> 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 <[email protected]> > Reported-by: Derek Foreman <[email protected]> > --- > 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
pgpbXcCUKJV4c.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
