On Wed, 11 Jul 2018 13:16:17 +0100 Daniel Stone <dani...@collabora.com> wrote:
> In the plane-only mode, we try to place every view on a hardware plane,
> and fail if we can't do this. This requires a full walk of the scene
> graph to come up with a complete configuration in order to be able to
> test.
>
> In mixed mode, we know at least some visible views will fail to be
> promoted to planes and must be composited via the renderer. In order to
> still use some planes where possible, we use atomic modesetting's
> test-only mode to incrementally test configurations.
>
> We know that the renderer output will always be visible, and because it
> is the renderer, that it will be occupying the scanout plane underneath
> everything else. The actual renderer buffer doesn't materialise until
> after assign_planes, because it cannot know what to render until then.
>
> However, in order to test whether a configuration is valid, we need the
> renderer buffer in the scanout plane. For testing, we fake this by
> temporarily stealing the old buffer - if it seems sufficiently
> compatible - and placing it in the state we construct. This is used to
> test whether or not a renderer buffer will work with the addition of
> overlay planes.
>
> Doing this incremental testing will allow us to enable plane usage for
> atomic by default, since we know ahead of time that our chosen plane
> configuration will work.
>
> Signed-off-by: Daniel Stone <dani...@collabora.com>
> ---
> libweston/compositor-drm.c | 118 +++++++++++++++++++++++++++----------
> 1 file changed, 86 insertions(+), 32 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 486fb71d9..d192eec16 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -1949,7 +1949,8 @@ enum drm_output_propose_state_mode {
>
> static struct drm_plane_state *
> drm_output_prepare_scanout_view(struct drm_output_state *output_state,
> - struct weston_view *ev)
> + struct weston_view *ev,
> + enum drm_output_propose_state_mode mode)
> {
> struct drm_output *output = output_state->output;
> struct drm_backend *b = to_drm_backend(output->base.compositor);
> @@ -1959,6 +1960,7 @@ drm_output_prepare_scanout_view(struct drm_output_state
> *output_state,
> pixman_box32_t *extents;
>
> assert(!b->sprites_are_broken);
> + assert(mode == DRM_OUTPUT_PROPOSE_STATE_PLANES_ONLY);
>
> /* Check the view spans exactly the output size, calculated in the
> * logical co-ordinate space. */
> @@ -1983,15 +1985,13 @@ drm_output_prepare_scanout_view(struct
> drm_output_state *output_state,
> }
>
> state = drm_output_state_get_plane(output_state, scanout_plane);
> - if (state->fb) {
> - /* If there is already a framebuffer on the scanout plane,
> - * a client view has already been placed on the scanout
> - * view. In that case, do not free or put back the state,
> - * but just leave it in place and quietly exit. */
> - drm_fb_unref(fb);
> - return NULL;
> - }
>
> + /* The only way we can already have a buffer in the scanout plane is
> + * if we are in mixed mode, or if a client buffer has already been
> + * placed into scanout. The former case will never call into here,
> + * and in the latter case, the view must have been marked as occluded,
> + * meaning we should never have ended up here. */
> + assert(!state->fb);
> state->fb = fb;
> state->ev = ev;
> state->output = output;
> @@ -2007,6 +2007,8 @@ drm_output_prepare_scanout_view(struct drm_output_state
> *output_state,
> state->dest_h != (unsigned) output->base.current_mode->height)
> goto err;
>
> + /* In plane-only mode, we don't need to test the state now, as we
> + * will only test it once at the end. */
> return state;
>
> err:
> @@ -3049,7 +3051,8 @@ atomic_flip_handler(int fd, unsigned int frame,
> unsigned int sec,
>
> static struct drm_plane_state *
> drm_output_prepare_overlay_view(struct drm_output_state *output_state,
> - struct weston_view *ev)
> + struct weston_view *ev,
> + enum drm_output_propose_state_mode mode)
> {
> struct drm_output *output = output_state->output;
> struct weston_compositor *ec = output->base.compositor;
> @@ -3058,6 +3061,7 @@ drm_output_prepare_overlay_view(struct drm_output_state
> *output_state,
> struct drm_plane_state *state = NULL;
> struct drm_fb *fb;
> unsigned int i;
> + int ret;
>
> assert(!b->sprites_are_broken);
>
Hi,
this is much much better!
> @@ -3098,31 +3102,37 @@ drm_output_prepare_overlay_view(struct
> drm_output_state *output_state,
> continue;
> }
>
> - break;
> - }
> + state->ev = ev;
> + state->output = output;
> + drm_plane_state_coords_for_view(state, ev);
Forgot to check that drm_plane_state_coords_for_view() succeeded.
> + if (state->src_w != state->dest_w << 16 ||
> + state->src_h != state->dest_h << 16) {
> + drm_plane_state_put_back(state);
Missing state = NULL;
The above two are the only bugs I could see, so with those fixed:
Reviewed-by: Pekka Paalanen <pekka.paala...@collabora.co.uk>
But, I think there might be one more case to add as follow-up. If we
first manage to put a view on an overlay, and then put another view on
the cursor plane, we don't actually atomic-test and undo the cursor
plane assignment if it wouldn't work. The result is falling back to
renderer-only mode, so it's not fatal.
Thanks,
pq
> + continue;
> + }
>
> - /* No sprites available */
> - if (!state) {
> - drm_fb_unref(fb);
> - return NULL;
> - }
> + /* We hold one reference for the lifetime of this function;
> + * from calling drm_fb_get_from_view, to the out label where
> + * we unconditionally drop the reference. So, we take another
> + * reference here to live within the state. */
> + state->fb = drm_fb_ref(fb);
>
> - state->fb = fb;
> - state->ev = ev;
> - state->output = output;
> + /* In planes-only mode, we don't have an incremental state to
> + * test against, so we just hope it'll work. */
> + if (mode == DRM_OUTPUT_PROPOSE_STATE_PLANES_ONLY)
> + goto out;
>
> - if (!drm_plane_state_coords_for_view(state, ev))
> - goto err;
> + ret = drm_pending_state_test(output_state->pending_state);
> + if (ret == 0)
> + goto out;
>
> - if (state->src_w != state->dest_w << 16 ||
> - state->src_h != state->dest_h << 16)
> - goto err;
> + drm_plane_state_put_back(state);
> + state = NULL;
> + }
>
> +out:
> + drm_fb_unref(fb);
> return state;
> -
> -err:
> - drm_plane_state_put_back(state);
> - return NULL;
> }
>
> /**
> @@ -3316,6 +3326,7 @@ drm_output_propose_state(struct weston_output
> *output_base,
> struct drm_output *output = to_drm_output(output_base);
> struct drm_backend *b = to_drm_backend(output->base.compositor);
> struct drm_output_state *state;
> + struct drm_plane_state *scanout_state = NULL;
> struct weston_view *ev;
> pixman_region32_t surface_overlap, renderer_region, occluded_region;
> bool planes_ok = (mode != DRM_OUTPUT_PROPOSE_STATE_RENDERER_ONLY);
> @@ -3327,6 +3338,35 @@ drm_output_propose_state(struct weston_output
> *output_base,
> pending_state,
> DRM_OUTPUT_STATE_CLEAR_PLANES);
>
> + /* We implement mixed mode by progressively creating and testing
> + * incremental states, of scanout + overlay + cursor. Since we
> + * walk our views top to bottom, the scanout plane is last, however
> + * we always need it in our scene for the test modeset to be
> + * meaningful. To do this, we steal a reference to the last
> + * renderer framebuffer we have, if we think it's basically
> + * compatible. If we don't have that, then we conservatively fall
> + * back to only using the renderer for this repaint. */
> + if (mode == DRM_OUTPUT_PROPOSE_STATE_MIXED) {
> + struct drm_plane *plane = output->scanout_plane;
> + struct drm_fb *scanout_fb = plane->state_cur->fb;
> +
> + if (!scanout_fb ||
> + (scanout_fb->type != BUFFER_GBM_SURFACE &&
> + scanout_fb->type != BUFFER_PIXMAN_DUMB)) {
> + drm_output_state_free(state);
> + return NULL;
> + }
> +
> + if (scanout_fb->width != output_base->current_mode->width ||
> + scanout_fb->height != output_base->current_mode->height) {
> + drm_output_state_free(state);
> + return NULL;
> + }
> +
> + scanout_state = drm_plane_state_duplicate(state,
> + plane->state_cur);
> + }
> +
> /*
> * Find a surface for each sprite in the output using some heuristics:
> * 1) size
> @@ -3411,10 +3451,14 @@ drm_output_propose_state(struct weston_output
> *output_base,
> if (!ps && !drm_view_is_opaque(ev))
> force_renderer = true;
>
> + /* Only try to place scanout surfaces in planes-only mode; in
> + * mixed mode, we have already failed to place a view on the
> + * scanout surface, forcing usage of the renderer on the
> + * scanout plane. */
> + if (!ps && !force_renderer && !renderer_ok)
> + ps = drm_output_prepare_scanout_view(state, ev, mode);
> if (!ps && !force_renderer)
> - ps = drm_output_prepare_scanout_view(state, ev);
> - if (!ps && !force_renderer)
> - ps = drm_output_prepare_overlay_view(state, ev);
> + ps = drm_output_prepare_overlay_view(state, ev, mode);
>
> if (ps) {
> /* If we have been assigned to an overlay or scanout
> @@ -3454,6 +3498,16 @@ drm_output_propose_state(struct weston_output
> *output_base,
> if (ret != 0)
> goto err;
>
> + /* Counterpart to duplicating scanout state at the top of this
> + * function: if we have taken a renderer framebuffer and placed it in
> + * the pending state in order to incrementally test overlay planes,
> + * remove it now. */
> + if (mode == DRM_OUTPUT_PROPOSE_STATE_MIXED) {
> + assert(scanout_state->fb->type == BUFFER_GBM_SURFACE ||
> + scanout_state->fb->type == BUFFER_PIXMAN_DUMB);
> + drm_plane_state_put_back(scanout_state);
> + }
> +
> return state;
>
> err_region:
pgpN90vB5HYZ9.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
