Hi Daniel,
On Tue, 26 Sep 2017 18:15:42 +0100, Daniel Stone wrote:
> Split repaint into two stages, as implied by the grouped-repaint
> interface: drm_output_repaint generates the repaint state only, and
> drm_repaint_flush applies it.
>
> This also moves DPMS into output state. Previously, the usual way to
> DPMS off was that repaint would be called and apply its state,
> followed by set_dpms being called afterwards to push the DPMS state
> separately. As this happens before the repaint_flush hook, with no
> change to DPMS we would set DPMS off, then immediately re-enable the
> output by posting the repaint. Not ideal.
>
> Moving DPMS application at the same time complicates this patch, but I
> couldn't find a way to split it; if we keep set_dpms before
> begin_flush then we break DPMS off, or if we try to move DPMS to
> output state before using the repaint flush, we get stuck as the
> repaint hook generates an asynchronous state update, followed
> immediately by set_dpms generating a synchronous state update.
>
> Signed-off-by: Daniel Stone <dani...@collabora.com>
> ---
> libweston/compositor-drm.c | 313
> ++++++++++++++++++++++++++++++++++----------- 1 file changed, 240
> insertions(+), 73 deletions(-)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 530b0fd5..6c1c6881 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -267,6 +267,7 @@ struct drm_output_state {
> struct drm_pending_state *pending_state;
> struct drm_output *output;
> struct wl_list link;
> + enum dpms_enum dpms;
> struct wl_list plane_list;
> };
>
> @@ -350,6 +351,7 @@ struct drm_output {
> int page_flip_pending;
> int destroy_pending;
> int disable_pending;
> + int dpms_off_pending;
>
> struct drm_fb *gbm_cursor_fb[2];
> struct drm_plane *cursor_plane;
> @@ -1124,6 +1126,7 @@ drm_output_state_alloc(struct drm_output
> *output,
> assert(state);
> state->output = output;
> + state->dpms = WESTON_DPMS_OFF;
> state->pending_state = pending_state;
> if (pending_state)
> wl_list_insert(&pending_state->output_list,
> &state->link); @@ -1200,6 +1203,30 @@ drm_output_state_free(struct
> drm_output_state *state) free(state);
> }
>
> +/**
> + * Get output state to disable output
> + *
> + * Returns a pointer to an output_state object which can be used to
> disable
> + * an output (e.g. DPMS off).
> + *
> + * @param pending_state The pending state object owning this update
> + * @param output The output to disable
> + * @returns A drm_output_state to disable the output
> + */
> +static struct drm_output_state *
> +drm_output_get_disable_state(struct drm_pending_state *pending_state,
> + struct drm_output *output)
> +{
> + struct drm_output_state *output_state;
> +
> + output_state = drm_output_state_duplicate(output->state_cur,
> + pending_state,
> +
> DRM_OUTPUT_STATE_CLEAR_PLANES);
> + output_state->dpms = WESTON_DPMS_OFF;
> +
> + return output_state;
> +}
> +
> /**
> * Allocate a new drm_pending_state
> *
> @@ -1272,6 +1299,8 @@ drm_pending_state_get_output(struct
> drm_pending_state *pending_state, return NULL;
> }
>
> +static int drm_pending_state_apply(struct drm_pending_state *state);
> +
> /**
> * Mark a drm_output_state (the output's last state) as complete.
> This handles
> * any post-completion actions such as updating the repaint timer,
> disabling the @@ -1281,6 +1310,7 @@ static void
> drm_output_update_complete(struct drm_output *output, uint32_t flags,
> unsigned int sec, unsigned int usec)
> {
> + struct drm_backend *b =
> to_drm_backend(output->base.compositor); struct drm_plane_state *ps;
> struct timespec ts;
>
> @@ -1300,6 +1330,13 @@ drm_output_update_complete(struct drm_output
> *output, uint32_t flags, } else if (output->disable_pending) {
> weston_output_disable(&output->base);
> output->disable_pending = 0;
> + output->dpms_off_pending = 0;
> + return;
> + } else if (output->dpms_off_pending) {
> + struct drm_pending_state *pending =
> drm_pending_state_alloc(b);
> + drm_output_get_disable_state(pending, output);
> + drm_pending_state_apply(pending);
> + output->dpms_off_pending = 0;
> return;
> }
>
> @@ -1360,7 +1397,6 @@ drm_output_assign_state(struct drm_output_state
> *state, }
> }
>
> -
> static int
> drm_view_transform_supported(struct weston_view *ev)
> {
> @@ -1653,37 +1689,20 @@ drm_waitvblank_pipe(struct drm_output *output)
> }
>
> static int
> -drm_output_repaint(struct weston_output *output_base,
> - pixman_region32_t *damage,
> - void *repaint_data)
> +drm_output_apply_state(struct drm_output_state *state)
> {
> - struct drm_pending_state *pending_state = repaint_data;
> - struct drm_output_state *state = NULL;
> - struct drm_output *output = to_drm_output(output_base);
> - struct drm_backend *backend =
> - to_drm_backend(output->base.compositor);
> + struct drm_output *output = state->output;
> + struct drm_backend *backend =
> to_drm_backend(output->base.compositor); struct drm_plane
> *scanout_plane = output->scanout_plane;
> + struct drm_property_info *dpms_prop =
> + &output->props_conn[WDRM_CONNECTOR_DPMS];
> struct drm_plane_state *scanout_state;
> struct drm_plane_state *ps;
> struct drm_plane *p;
> struct drm_mode *mode;
> + struct timespec now;
> int ret = 0;
>
> - if (output->disable_pending || output->destroy_pending)
> - goto err;
> -
> - 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); -
> -
> /* If disable_planes is set then assign_planes() wasn't
> * called for this render, so we could still have a stale
> * cursor plane set up.
> @@ -1694,10 +1713,46 @@ drm_output_repaint(struct weston_output
> *output_base, output->cursor_plane->base.y = INT32_MIN;
> }
>
> - drm_output_render(state, damage);
> - scanout_state = drm_output_state_get_plane(state,
> scanout_plane);
> - if (!scanout_state || !scanout_state->fb)
> - goto err;
> + if (state->dpms != WESTON_DPMS_ON) {
> + wl_list_for_each(ps, &state->plane_list, link) {
> + p = ps->plane;
> + assert(ps->fb == NULL);
> + assert(ps->output == NULL);
> +
> + if (p->type != WDRM_PLANE_TYPE_OVERLAY)
> + continue;
> +
> +
> + ret = drmModeSetPlane(backend->drm.fd,
> p->plane_id,
> + 0, 0, 0, 0, 0, 0, 0,
> 0, 0, 0, 0);
> + if (ret)
> + weston_log("drmModeSetPlane failed
> disable: %m\n");
> + }
> +
> + if (output->cursor_plane) {
> + ret = drmModeSetCursor(backend->drm.fd,
> output->crtc_id,
> + 0, 0, 0);
> + if (ret)
> + weston_log("drmModeSetCursor failed
> disable: %m\n");
> + }
> +
> + ret = drmModeSetCrtc(backend->drm.fd,
> output->crtc_id, 0, 0, 0,
> + &output->connector_id, 0, NULL);
> + if (ret)
> + weston_log("drmModeSetCrtc failed disabling:
> %m\n"); +
> + drm_output_assign_state(state,
> +
> DRM_OUTPUT_STATE_UPDATE_SYNCHRONOUS);
> +
> weston_compositor_read_presentation_clock(output->base.compositor,
> &now);
> + weston_output_finish_frame(&output->base,
> + &now,
> +
> WP_PRESENTATION_FEEDBACK_KIND_HW_COMPLETION); +
> + return 0;
> + }
> +
> + scanout_state =
> + drm_output_state_get_existing_plane(state,
> scanout_plane);
> /* The legacy SetCrtc API doesn't allow us to do scaling,
> and the
> * legacy PageFlip API doesn't allow us to do clipping
> either. */ @@ -1724,7 +1779,6 @@ drm_output_repaint(struct
> weston_output *output_base, weston_log("set mode failed: %m\n");
> goto err;
> }
> - output_base->set_dpms(output_base, WESTON_DPMS_ON);
> }
>
> if (drmModePageFlip(backend->drm.fd, output->crtc_id,
> @@ -1790,13 +1844,101 @@ drm_output_repaint(struct weston_output
> *output_base, }
> }
>
> + if (dpms_prop->prop_id && state->dpms !=
> output->state_cur->dpms) {
> + ret = drmModeConnectorSetProperty(backend->drm.fd,
> +
> output->connector_id,
> + dpms_prop->prop_id,
> + state->dpms);
> + if (ret) {
> + weston_log("DRM: DPMS: failed property set
> for %s\n",
> + output->base.name);
> + }
> + }
> +
> + drm_output_assign_state(state,
> DRM_OUTPUT_STATE_UPDATE_ASYNCHRONOUS); +
> return 0;
>
> err:
> output->cursor_view = NULL;
> -
> drm_output_state_free(state);
> + return -1;
> +}
> +
> +static int
> +drm_pending_state_apply(struct drm_pending_state *pending_state)
> +{
> + struct drm_backend *b = pending_state->backend;
> + struct drm_output_state *output_state, *tmp;
> + uint32_t *unused;
> +
> + if (b->state_invalid) {
> + /* If we need to reset all our state (e.g. because
> we've
> + * just started, or just been VT-switched in),
> explicitly
> + * disable all the CRTCs we aren't using. This also
> disables
> + * all connectors on these CRTCs, so we don't need
> to do that
> + * separately with the pre-atomic API. */
> + wl_array_for_each(unused, &b->unused_crtcs)
> + drmModeSetCrtc(b->drm.fd, *unused, 0, 0, 0,
> NULL, 0,
> + NULL);
> + }
> +
> + wl_list_for_each_safe(output_state, tmp,
> &pending_state->output_list,
> + link) {
> + struct drm_output *output = output_state->output;
> + int ret;
> +
> + ret = drm_output_apply_state(output_state);
> + if (ret != 0) {
> + weston_log("Couldn't apply state for output
> %s\n",
> + output->base.name);
> + }
> + }
> +
> + b->state_invalid = false;
> +
> + assert(wl_list_empty(&pending_state->output_list));
>
> + drm_pending_state_free(pending_state);
> +
> + return 0;
> +}
> +
> +static int
> +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 *output = to_drm_output(output_base);
> + struct drm_output_state *state;
> + struct drm_plane_state *scanout_state;
> +
> + if (output->disable_pending || output->destroy_pending)
> + goto err;
> +
> + 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);
> + state->dpms = WESTON_DPMS_ON;
> +
> + drm_output_render(state, damage);
> + scanout_state = drm_output_state_get_plane(state,
> +
> output->scanout_plane);
> + if (!scanout_state || !scanout_state->fb)
> + goto err;
> +
> + return 0;
> +
> +err:
> + drm_output_state_free(state);
> return -1;
> }
>
> @@ -1922,6 +2064,9 @@ drm_output_update_msc(struct drm_output
> *output, unsigned int seq) output->base.msc = (msc_hi << 32) + seq;
> }
>
> +static void
> +drm_output_destroy(struct weston_output *base);
> +
> static void
> vblank_handler(int fd, unsigned int frame, unsigned int sec,
> unsigned int usec, void *data)
> @@ -1944,9 +2089,6 @@ vblank_handler(int fd, unsigned int frame,
> unsigned int sec, unsigned int usec,
> drm_output_update_complete(output, flags, sec, usec); }
>
> -static void
> -drm_output_destroy(struct weston_output *base);
> -
> static void
> page_flip_handler(int fd, unsigned int frame,
> unsigned int sec, unsigned int usec, void *data)
> @@ -2000,29 +2142,8 @@ drm_repaint_flush(struct weston_compositor
> *compositor, void *repaint_data) {
> struct drm_backend *b = to_drm_backend(compositor);
> struct drm_pending_state *pending_state = repaint_data;
> - struct drm_output_state *output_state, *tmp;
> - uint32_t *unused;
>
> - if (b->state_invalid) {
> - /* If we need to reset all our state (e.g. because
> we've
> - * just started, or just been VT-switched in),
> explicitly
> - * disable all the CRTCs we aren't using. This also
> disables
> - * all connectors on these CRTCs, so we don't need
> to do that
> - * separately with the pre-atomic API. */
> - wl_array_for_each(unused, &b->unused_crtcs)
> - drmModeSetCrtc(b->drm.fd, *unused, 0, 0, 0,
> NULL, 0,
> - NULL);
> - }
> -
> - wl_list_for_each_safe(output_state, tmp,
> &pending_state->output_list,
> - link) {
> - drm_output_assign_state(output_state,
> -
> DRM_OUTPUT_STATE_UPDATE_ASYNCHRONOUS);
> - }
> -
> - b->state_invalid = false;
> -
> - drm_pending_state_free(pending_state);
> + drm_pending_state_apply(pending_state);
> b->repaint_data = NULL;
> }
>
> @@ -3026,8 +3147,6 @@ drm_output_find_special_plane(struct
> drm_backend *b, struct drm_output *output, static void
> drm_plane_destroy(struct drm_plane *plane)
> {
> - drmModeSetPlane(plane->backend->drm.fd, plane->plane_id, 0,
> 0, 0,
> - 0, 0, 0, 0, 0, 0, 0, 0);
> drm_plane_state_free(plane->state_cur, true);
> drm_property_info_free(plane->props, WDRM_PLANE__COUNT);
> weston_plane_release(&plane->base);
> @@ -3197,28 +3316,72 @@ drm_set_backlight(struct weston_output
> *output_base, uint32_t value)
> backlight_set_brightness(output->backlight, new_brightness); }
>
> +/**
> + * Power output on or off
> + *
> + * The DPMS/power level of an output is used to switch it on or off.
> This
> + * is DRM's hook for doing so, which can called either as part of
> repaint,
> + * or independently of the repaint loop.
> + *
> + * If we are called as part of repaint, we simply set the relevant
> bit in
> + * state and return.
> + */
> static void
> drm_set_dpms(struct weston_output *output_base, enum dpms_enum level)
> {
> struct drm_output *output = to_drm_output(output_base);
> - struct weston_compositor *ec = output_base->compositor;
> - struct drm_backend *b = to_drm_backend(ec);
> - struct drm_property_info *prop =
> - &output->props_conn[WDRM_CONNECTOR_DPMS];
> + struct drm_backend *b =
> to_drm_backend(output_base->compositor);
> + struct drm_pending_state *pending_state = b->repaint_data;
> + struct drm_output_state *state;
> int ret;
>
> - if (!prop->prop_id)
> + /* If we're being called during the repaint loop, then this
> is
> + * simple: discard any previously-generated state, and
> create a new
> + * state where we disable everything. When we come to flush,
> this
> + * will be applied. */
> + if (pending_state) {
> + /* The repaint loop already sets DPMS on; we don't
> need to
> + * explicitly set it on here, as it will already
> happen
> + * whilst applying the repaint state. */
> + if (level == WESTON_DPMS_ON)
> + return;
> +
> + state = drm_pending_state_get_output(pending_state,
> output);
> + if (state)
> + drm_output_state_free(state);
> + state = drm_output_get_disable_state(pending_state,
> output);
> + state->dpms = level;
> + return;
> + }
> +
> + if (output->state_cur->dpms == level)
> return;
>
> - ret = drmModeConnectorSetProperty(b->drm.fd,
> output->connector_id,
> - prop->prop_id, level);
> - if (ret) {
> - weston_log("DRM: DPMS: failed property set for %s\n",
> - output->base.name);
> + /* As we throw everything away when disabling, just send us
> back through
> + * a repaint cycle. */
> + if (level == WESTON_DPMS_ON) {
> + if (output->dpms_off_pending)
> + output->dpms_off_pending = 0;
> + weston_output_schedule_repaint(output_base);
> return;
> }
>
> - output->dpms = level;
> + if (output->state_cur->dpms == WESTON_DPMS_OFF)
> + return;
> +
> + /* If we've already got a request in the pipeline, then we
> need to
> + * park our DPMS request until that request has quiesced. */
> + if (output->state_last) {
> + output->dpms_off_pending = 1;
> + return;
> + }
> +
> + pending_state = drm_pending_state_alloc(b);
> + drm_output_state_duplicate(output->state_cur, pending_state,
> + DRM_OUTPUT_STATE_CLEAR_PLANES);
> + ret = drm_pending_state_apply(pending_state);
The page flip from this call violates the assertion repaint_state ==
REPAINT_AWAITING_COMPLETION in weston_output_finish_frame(). The
FADE_OUT animation seems to mask this case, but I can trigger it by
directly calling weston_compositor_sleep().
As a workaround, I check for the repaint_status in
drm_output_update_complete() and skip the call to
weston_output_frame_finish(), but I don't really like, because it
exposes the compositor status to compositor-drm.
Michael
> + if (ret != 0)
> + weston_log("drm_set_dpms: couldn't disable
> output?\n"); }
>
> static const char * const connector_type_names[] = {
> @@ -4025,7 +4188,9 @@ drm_output_disable(struct weston_output *base)
> {
> struct drm_output *output = to_drm_output(base);
> struct drm_backend *b = to_drm_backend(base->compositor);
> + struct drm_pending_state *pending_state;
> uint32_t *unused;
> + int ret;
>
> if (output->page_flip_pending || output->vblank_pending) {
> output->disable_pending = 1;
> @@ -4038,11 +4203,13 @@ drm_output_disable(struct weston_output *base)
> output->disable_pending = 0;
>
> weston_log("Disabling output %s\n", output->base.name);
> - drmModeSetCrtc(b->drm.fd, output->crtc_id,
> - 0, 0, 0, 0, 0, NULL);
> -
> - drm_output_state_free(output->state_cur);
> - output->state_cur = drm_output_state_alloc(output, NULL);
> + pending_state = drm_pending_state_alloc(b);
> + drm_output_get_disable_state(pending_state, output);
> + ret = drm_pending_state_apply(pending_state);
> + if (ret) {
> + weston_log("Couldn't disable output output %s\n",
> + output->base.name);
> + }
>
> unused = wl_array_add(&b->unused_connectors,
> sizeof(*unused)); *unused = output->connector_id;
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
