On Wed, 20 Dec 2017 12:26:39 +0000 Daniel Stone <dani...@collabora.com> wrote:
> Use the shiny new helper we have for working through scanout as well.
>
> Signed-off-by: Daniel Stone <dani...@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
pgpJJiqydWNaj.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
