[PATCH v14 37/41] compositor-drm: Add test-only mode to state application
Pekka Paalanen
ppaalanen at gmail.com
Mon Jan 29 13:09:31 UTC 2018
On Wed, 20 Dec 2017 12:26:54 +0000
Daniel Stone <daniels at collabora.com> wrote:
> The atomic API can allow us to test state before we apply it, to see if
> it will be valid. Add support for this, which we will later use in
> assign_planes to ensure our plane configuration is valid.
>
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
> libweston/compositor-drm.c | 176 +++++++++++++++++++++++++++++++++++----------
> 1 file changed, 137 insertions(+), 39 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 8709ecff3..340e27cb5 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -214,6 +214,7 @@ enum drm_output_state_duplicate_mode {
> enum drm_state_apply_mode {
> DRM_STATE_APPLY_SYNC, /**< state fully processed */
> DRM_STATE_APPLY_ASYNC, /**< state pending event delivery */
> + DRM_STATE_TEST_ONLY, /**< test if the state can be applied */
> };
>
> struct drm_backend {
> @@ -1646,6 +1647,7 @@ drm_pending_state_get_output(struct drm_pending_state *pending_state,
>
> static int drm_pending_state_apply(struct drm_pending_state *state);
> static int drm_pending_state_apply_sync(struct drm_pending_state *state);
> +static int drm_pending_state_test(struct drm_pending_state *state);
>
> /**
> * Mark a drm_output_state (the output's last state) as complete. This handles
> @@ -1773,12 +1775,15 @@ 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_plane *scanout_plane = output->scanout_plane;
> struct drm_plane_state *state;
> + struct drm_plane_state *state_old = NULL;
> struct drm_fb *fb;
> + int ret;
>
> fb = drm_fb_get_from_view(output_state, ev);
> if (!fb)
> @@ -1791,11 +1796,18 @@ 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. */
I think the above comment would be nice to keep.
> +
> + /* Check if we've already placed a buffer on this plane; if there's a
> + * buffer there but it comes from GBM, then it's the result of
> + * drm_output_propose_state placing it here for testing purposes. */
> + if (state->fb &&
> + (state->fb->type == BUFFER_GBM_SURFACE ||
> + state->fb->type == BUFFER_PIXMAN_DUMB)) {
> + state_old = calloc(1, sizeof(*state_old));
> + memcpy(state_old, state, sizeof(*state_old));
> + state_old->output_state = NULL;
> + wl_list_init(&state_old->link);
If we hit this path and then...
> + } else if (state->fb) {
> drm_fb_unref(fb);
> return NULL;
> }
> @@ -1814,10 +1826,24 @@ drm_output_prepare_scanout_view(struct drm_output_state *output_state,
> state->dest_h != (unsigned) output->base.current_mode->height)
> goto err;
>
> + if (mode == DRM_OUTPUT_PROPOSE_STATE_PLANES_ONLY) {
> + drm_plane_state_free(state_old, false);
..come here, the drm_fb that was saved into state_old will be unreffed
regardless whether it actually is in plane->state_cur. But above we did
not take a ref to it, so the drm_fb will probably get destroyed
prematurely, no?
> + return state;
> + }
> +
> + ret = drm_pending_state_test(output_state->pending_state);
> + if (ret != 0)
> + goto err;
> +
> + drm_plane_state_free(state_old, false);
The same problem here.
> return state;
>
> err:
> - drm_plane_state_put_back(state);
> + drm_plane_state_free(state, false);
This would not unref fb if state happens to be plane->state_cur, but it
should.
> + if (state_old) {
> + wl_list_insert(&output_state->plane_list, &state_old->link);
> + state_old->output_state = output_state;
> + }
This function was quite hard to follow now.
> return NULL;
> }
>
> @@ -1892,7 +1918,9 @@ drm_output_render(struct drm_output_state *state, pixman_region32_t *damage)
> * want to render. */
> scanout_state = drm_output_state_get_plane(state,
> output->scanout_plane);
> - if (scanout_state->fb)
> + if (scanout_state->fb &&
> + scanout_state->fb->type != BUFFER_GBM_SURFACE &&
> + scanout_state->fb->type != BUFFER_PIXMAN_DUMB)
This is becoming a recurring pattern. How about
bool
drm_plane_state_has_renderer_fb()
helper?
> return;
>
> if (!pixman_region32_not_empty(damage) &&
> @@ -1915,6 +1943,7 @@ drm_output_render(struct drm_output_state *state, pixman_region32_t *damage)
> return;
> }
>
> + drm_fb_unref(scanout_state->fb);
What if scanout_state == plane->state_cur?
> scanout_state->fb = fb;
> scanout_state->output = output;
>
> @@ -2424,9 +2453,16 @@ drm_pending_state_apply_atomic(struct drm_pending_state *pending_state,
> case DRM_STATE_APPLY_ASYNC:
> flags |= DRM_MODE_PAGE_FLIP_EVENT | DRM_MODE_ATOMIC_NONBLOCK;
> break;
> + case DRM_STATE_TEST_ONLY:
> + flags |= DRM_MODE_ATOMIC_TEST_ONLY;
> + break;
> }
>
> ret = drmModeAtomicCommit(b->drm.fd, req, flags, b);
> +
> + if (mode == DRM_STATE_TEST_ONLY)
Agreed with Philipp.
> + return ret;
> +
> if (ret != 0) {
> weston_log("atomic: couldn't commit new state: %m\n");
> goto out;
> @@ -3063,12 +3122,42 @@ drm_output_propose_state(struct weston_output *output_base,
> struct weston_view *ev;
> pixman_region32_t surface_overlap, renderer_region, occluded_region;
> bool renderer_ok = (mode != DRM_OUTPUT_PROPOSE_STATE_PLANES_ONLY);
> - bool planes_ok = !b->sprites_are_broken;
> + bool planes_ok = false;
> + bool duplicate_renderer = false;
> + int ret;
> +
> + /* We can only propose planes if we're in plane-only mode (i.e. just
> + * get as much as possible into planes and test at the end), or mixed
> + * mode, where we have our previous renderer buffer (and it's broadly
> + * compatible) to test with. If we don't have these things, then we
> + * don't know whether or not the planes will work, so we conservatively
> + * fall back to just using the renderer. */
> + if (mode == DRM_OUTPUT_PROPOSE_STATE_PLANES_ONLY) {
> + if (b->sprites_are_broken)
> + return NULL;
> + planes_ok = true;
> + } else if (output->scanout_plane->state_cur->fb) {
> + struct drm_fb *scanout_fb =
> + output->scanout_plane->state_cur->fb;
> + if ((scanout_fb->type == BUFFER_GBM_SURFACE ||
> + scanout_fb->type == BUFFER_PIXMAN_DUMB) &&
> + scanout_fb->width == output_base->current_mode->width &&
> + scanout_fb->height == output_base->current_mode->height) {
> + planes_ok = true;
> + duplicate_renderer = true;
> + }
> + }
>
> assert(!output->state_last);
> state = drm_output_state_duplicate(output->state_cur,
> pending_state,
> DRM_OUTPUT_STATE_CLEAR_PLANES);
> + if (!planes_ok)
> + return state;
> +
> + if (duplicate_renderer)
> + drm_plane_state_duplicate(state,
> + output->scanout_plane->state_cur);
>
> /*
> * Find a surface for each sprite in the output using some heuristics:
> @@ -3102,6 +3191,9 @@ drm_output_propose_state(struct weston_output *output_base,
> if (ev->output_mask != (1u << output->base.id))
> force_renderer = true;
>
> + if (!ev->surface->buffer_ref.buffer)
> + force_renderer = true;
> +
This is a slight optimization, but not actually necessary, right?
> /* Ignore views we know to be totally occluded. */
> pixman_region32_init(&clipped_view);
> pixman_region32_intersect(&clipped_view,
> @@ -3137,17 +3229,13 @@ drm_output_propose_state(struct weston_output *output_base,
> * cursors_are_broken flag. */
> if (!force_renderer && !b->cursors_are_broken)
> ps = drm_output_prepare_cursor_view(state, ev);
> -
> if (!planes_ok || !drm_view_is_opaque(ev))
> force_renderer = true;
>
> - if (!drm_view_is_opaque(ev))
> - force_renderer = true;
> -
> if (!force_renderer && !ps)
> - ps = drm_output_prepare_scanout_view(state, ev);
> + ps = drm_output_prepare_scanout_view(state, ev, mode);
> if (!force_renderer && !ps)
> - ps = drm_output_prepare_overlay_view(state, ev);
> + ps = drm_output_prepare_overlay_view(state, ev, mode);
>
> if (ps) {
>
> @@ -3157,7 +3245,6 @@ drm_output_propose_state(struct weston_output *output_base,
> pixman_region32_fini(&clipped_view);
> continue;
> }
> -
> if (!renderer_ok) {
> pixman_region32_fini(&clipped_view);
> goto err;
> @@ -3171,6 +3258,11 @@ drm_output_propose_state(struct weston_output *output_base,
> pixman_region32_fini(&renderer_region);
> pixman_region32_fini(&occluded_region);
>
> + /* Check to see if this state will actually work. */
> + ret = drm_pending_state_test(state->pending_state);
> + if (ret != 0)
> + goto err;
The failure path finis renderer_region and occluded_region twice.
> +
> return state;
>
> err:
> @@ -3191,8 +3283,14 @@ drm_assign_planes(struct weston_output *output_base, void *repaint_data)
> struct weston_view *ev;
> struct weston_plane *primary = &output_base->compositor->primary_plane;
>
> +
> state = drm_output_propose_state(output_base, pending_state,
> - DRM_OUTPUT_PROPOSE_STATE_MIXED);
> + DRM_OUTPUT_PROPOSE_STATE_PLANES_ONLY);
> + if (!state)
> + state = drm_output_propose_state(output_base, pending_state,
> + DRM_OUTPUT_PROPOSE_STATE_MIXED);
> +
> + assert(state);
>
> wl_list_for_each(ev, &output_base->compositor->view_list, link) {
> struct drm_plane *target_plane = NULL;
Lots of related comments in patch 35/41.
Other bits looked good.
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/20180129/fc86a25a/attachment.sig>
More information about the wayland-devel
mailing list