[PATCH weston v11 07/13] compositor-drm: Introduce drm_output_state structure
Pekka Paalanen
ppaalanen at gmail.com
Thu Jul 20 17:01:05 UTC 2017
On Tue, 18 Jul 2017 14:14:29 +0100
Daniel Stone <daniels at collabora.com> wrote:
> Currently this doesn't actually really do anything, but will be used in
> the future to track the state for both modeset and repaint requests.
> Completion of the request gives us a single request-completion path for
> both pageflip and vblank events.
Hi,
this patch makes subtle changes in how vblank_pending affects output
disable/destroy sequences, but I don't see anything wrong in the new
sequence. The old sequence might have destroyed the output while
vblanks were pending. Maybe worth noting this in the commit message.
The note could just avoid any deeper explanation by saying vblanks were
not used to begin with since they were disabled by the
sprites_are_broken flag.
>
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
> libweston/compositor-drm.c | 334 ++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 281 insertions(+), 53 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index eae78ea3..3fca7f40 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -903,11 +942,64 @@ drm_fb_unref(struct drm_fb *fb)
> }
> }
>
> -static int
> -drm_view_transform_supported(struct weston_view *ev)
> +/**
> + * Allocate a new, empty drm_output_state. This should not generally be used
> + * in the repaint cycle; see drm_output_state_duplicate.
> + */
> +static struct drm_output_state *
> +drm_output_state_alloc(struct drm_output *output,
> + struct drm_pending_state *pending_state)
> {
> - return !ev->transform.enabled ||
> - (ev->transform.matrix.type < WESTON_MATRIX_TRANSFORM_ROTATE);
> + struct drm_output_state *state = calloc(1, sizeof(*state));
Use zalloc().
> +
> + assert(state);
> + state->output = output;
> + state->pending_state = pending_state;
> + if (pending_state)
> + wl_list_insert(&pending_state->output_list, &state->link);
> + else
> + wl_list_init(&state->link);
> +
> + return state;
> +}
> +
> +/**
> + * Duplicate an existing drm_output_state into a new one. This is generally
> + * used during the repaint cycle, to capture the existing state of an output
> + * and modify it to create a new state to be used.
> + *
> + * The mode determines whether the output will be reset to an a blank state,
> + * or an exact mirror of the current state.
> + */
> +static struct drm_output_state *
> +drm_output_state_duplicate(struct drm_output_state *src,
> + struct drm_pending_state *pending_state,
> + enum drm_output_state_duplicate_mode plane_mode)
> +{
> + struct drm_output_state *dst = malloc(sizeof(*dst));
> +
> + assert(dst);
> + memcpy(dst, src, sizeof(*dst));
Use a whole-struct assignment instead of memcpy()?
> + dst->pending_state = pending_state;
> + if (pending_state)
> + wl_list_insert(&pending_state->output_list, &dst->link);
> + else
> + wl_list_init(&dst->link);
> +
> + return dst;
> +}
> +
> +/**
> + * Free an unused drm_output_state.
> + */
> +static void
> +drm_output_state_free(struct drm_output_state *state)
> +{
> + if (!state)
> + return;
> +
> + wl_list_remove(&state->link);
> + free(state);
> }
>
> /**
> @@ -929,6 +1021,7 @@ drm_pending_state_alloc(struct drm_backend *backend)
> return NULL;
>
> ret->backend = backend;
> + wl_list_init(&ret->output_list);
Should drm_pending_state_free() assert that the list is empty there?
Or is it missing to free the output states completely?
>
> return ret;
> }
> +/**
> + * Mark an output state as current on the output, i.e. it has been
> + * submitted to the kernel. The mode argument determines whether this
> + * update will be applied synchronously (e.g. when calling drmModeSetCrtc),
> + * or asynchronously (in which case we wait for events to complete).
> + */
> +static void
> +drm_output_assign_state(struct drm_output_state *state,
> + enum drm_output_state_update_mode mode)
> +{
> + struct drm_output *output = state->output;
> +
> + assert(!output->state_last);
> +
> + if (mode == DRM_OUTPUT_STATE_UPDATE_ASYNCHRONOUS)
> + output->state_last = output->state_cur;
> + else
> + drm_output_state_free(output->state_cur);
> +
> + wl_list_remove(&state->link);
> + wl_list_init(&state->link);
> + output->state_cur = state;
Should this function not reset state->pending_state?
> +}
> @@ -1176,6 +1367,8 @@ drm_output_repaint(struct weston_output *output_base,
> pixman_region32_t *damage,
> void *repaint_data)
> {
> + struct drm_pending_state *pending_state = repaint_data;
> + struct drm_output_state *state;
> struct drm_output *output = to_drm_output(output_base);
> struct drm_backend *backend =
> to_drm_backend(output->base.compositor);
> @@ -1184,7 +1377,18 @@ drm_output_repaint(struct weston_output *output_base,
> int ret = 0;
>
> if (output->disable_pending || output->destroy_pending)
> - return -1;
> + goto err;
Local variable 'state' should be initialized to NULL, otherwise this
error path will call drm_output_state_free() on an uninitialized
pointer.
> +
> + assert(!output->state_last);
> +
> + /* If planes have been disabled in the core, we might not have
> + * hit assign_planes at all, so might not have valid output state
> + * here. */
> + state = drm_pending_state_get_output(pending_state, output);
> + if (!state)
> + state = drm_output_state_duplicate(output->state_cur,
> + pending_state,
> + DRM_OUTPUT_STATE_CLEAR_PLANES);
>
> assert(!output->fb_last);
>
> @@ -1198,9 +1402,9 @@ drm_output_repaint(struct weston_output *output_base,
> output->cursor_plane.y = INT32_MIN;
> }
>
> - drm_output_render(output, damage);
> + drm_output_render(state, damage);
> if (!output->fb_pending)
> - return -1;
> + goto err;
>
> mode = container_of(output->base.current_mode, struct drm_mode, base);
> if (!output->fb_current ||
> @@ -1211,7 +1415,7 @@ drm_output_repaint(struct weston_output *output_base,
> &mode->mode_info);
> if (ret) {
> weston_log("set mode failed: %m\n");
> - goto err_pageflip;
> + goto err;
> }
> output_base->set_dpms(output_base, WESTON_DPMS_ON);
> }
> @@ -1220,7 +1424,7 @@ drm_output_repaint(struct weston_output *output_base,
> output->fb_pending->fb_id,
> DRM_MODE_PAGE_FLIP_EVENT, output) < 0) {
> weston_log("queueing pageflip failed: %m\n");
> - goto err_pageflip;
> + goto err;
> }
>
> output->fb_last = output->fb_current;
> @@ -1288,12 +1492,13 @@ drm_output_repaint(struct weston_output *output_base,
>
> return 0;
>
> -err_pageflip:
> +err:
> output->cursor_view = NULL;
> if (output->fb_pending) {
> drm_fb_unref(output->fb_pending);
> output->fb_pending = NULL;
> }
> + drm_output_state_free(state);
>
> return -1;
> }
> @@ -1302,6 +1507,8 @@ static void
> drm_output_start_repaint_loop(struct weston_output *output_base)
> {
> struct drm_output *output = to_drm_output(output_base);
> + struct drm_pending_state *pending_state;
> + struct drm_output_state *state;
> struct drm_backend *backend =
> to_drm_backend(output_base->compositor);
> uint32_t fb_id;
> @@ -1356,10 +1563,16 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
>
> assert(!output->page_flip_pending);
> assert(!output->fb_last);
> + assert(!output->state_last);
> +
> + pending_state = drm_pending_state_alloc(backend);
> + state = drm_output_state_duplicate(output->state_cur, pending_state,
> + DRM_OUTPUT_STATE_PRESERVE_PLANES);
>
> if (drmModePageFlip(backend->drm.fd, output->crtc_id, fb_id,
> DRM_MODE_PAGE_FLIP_EVENT, output) < 0) {
> weston_log("queueing pageflip failed: %m\n");
> + drm_output_state_free(state);
> goto finish_frame;
> }
>
> @@ -1370,6 +1583,9 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
> output->fb_last = drm_fb_ref(output->fb_current);
> output->page_flip_pending = 1;
>
> + drm_output_assign_state(state, DRM_OUTPUT_STATE_UPDATE_ASYNCHRONOUS);
> + free(pending_state);
This should be drm_pending_state_free(pending_state).
> +
> return;
>
> finish_frame:
I think pending_state is leaked in this branch.
All in all, looks very good, just some minor things to fix.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170720/e9ab035e/attachment.sig>
More information about the wayland-devel
mailing list