On Wed, 20 Dec 2017 12:26:28 +0000 Daniel Stone <dani...@collabora.com> 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 | 366
> ++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 292 insertions(+), 74 deletions(-)
Hi Daniel,
this is still a tricky one, sorry.
I looked at the details here hard and long, and then came to a
revelation: is it at all possible to ever call drm_set_dpms() between
repaint_begin and repaint_flush/cancel?
I don't think it is and that would make many of my comments below just
moot.
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 87c2603c7..2bb13f3a2 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -272,6 +272,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;
> };
>
> @@ -348,13 +349,13 @@ struct drm_output {
> /* Holds the properties for the connector */
> struct drm_property_info props_conn[WDRM_CONNECTOR__COUNT];
>
> - enum dpms_enum dpms;
> struct backlight *backlight;
>
> int vblank_pending;
> 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;
> @@ -1149,6 +1150,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);
> @@ -1225,6 +1227,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
> *
> @@ -1297,6 +1323,9 @@ drm_pending_state_get_output(struct drm_pending_state
> *pending_state,
> return NULL;
> }
>
> +static int drm_pending_state_apply(struct drm_pending_state *state);
> +static int drm_pending_state_apply_sync(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
> @@ -1306,6 +1335,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;
>
> @@ -1325,6 +1355,21 @@ 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);
See the question about drm_pending_state_apply vs. apply_sync() far
down below.
> + output->dpms_off_pending = 0;
> + return;
> + } else if (output->state_cur->dpms == WESTON_DPMS_OFF &&
> + output->base.repaint_status != REPAINT_AWAITING_COMPLETION) {
> + /* DPMS can happen to us either in the middle of a repaint
> + * cycle (when we have painted fresh content, only to throw it
> + * away for DPMS off), or at any other random point. If the
> + * latter is true, then we cannot go through finish_frame,
> + * because the repaint machinery does not expect this. */
When turning an output off, there should be no pageflip event, so how
can this ever be reached?
drm_output_apply_state() on the DPMS-off path does not schedule a
pageflip event.
> return;
> }
>
> @@ -1387,14 +1432,13 @@ drm_output_assign_state(struct drm_output_state
> *state,
> output->page_flip_pending = 1;
> }
>
> - if (output->dpms == WESTON_DPMS_ON) {
> + if (state->dpms == WESTON_DPMS_ON) {
> wl_array_remove_uint32(&b->unused_connectors,
> output->connector_id);
> wl_array_remove_uint32(&b->unused_crtcs, output->crtc_id);
> }
> }
>
> -
> static int
> drm_view_transform_supported(struct weston_view *ev)
> {
> @@ -1687,37 +1731,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.
> @@ -1728,10 +1755,45 @@ 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_STATE_APPLY_SYNC);
> +
> weston_compositor_read_presentation_clock(output->base.compositor, &now);
> + weston_output_finish_frame(&output->base,
> + &now,
> +
> WP_PRESENTATION_FEEDBACK_KIND_HW_COMPLETION);
See the weston_output_finish_frame() comment further down below.
finish_frame here is necessary if and only if drm_set_dpms(off) was
called between repaint_begin and repaint_flush... but that's not
possible, is it?
> +
> + 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. */
> @@ -1758,7 +1820,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,
> @@ -1824,13 +1885,140 @@ 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_STATE_APPLY_ASYNC);
> +
> return 0;
>
> err:
> output->cursor_view = NULL;
> -
> drm_output_state_free(state);
> + return -1;
> +}
> @@ -3058,9 +3229,6 @@ drm_output_find_special_plane(struct drm_backend *b,
> struct drm_output *output,
> static void
> drm_plane_destroy(struct drm_plane *plane)
> {
> - if (plane->plane_id > 0)
> - drmModeSetPlane(plane->backend->drm.fd, plane->plane_id,
> - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
Did you have a rationale for this change? I still found it surprising,
and for v12 I wrote:
: This was a slightly surprising piece in this patch. Is the rationale
: here that Weston should not reset CRTC state on exit? Shouldn't that
: wait for the "compositor-drm: Don't restore original CRTC mode" patch?
:
: Feels like this hunk doesn't belong in this patch.
:
: I suppose this might leave an overlay up, but since we have overlays
: disabled by default, it shouldn't cause issues.
> drm_plane_state_free(plane->state_cur, true);
> drm_property_info_free(plane->props, WDRM_PLANE__COUNT);
> weston_plane_release(&plane->base);
> @@ -3230,28 +3398,76 @@ 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 (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);
> + /* 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.
> + *
> + * However, we need to be careful: we can be called whilst another
> + * output is in its repaint cycle (pending_state exists), but our
> + * output still has an incomplete state application outstanding.
> + * In that case, we need to wait until that completes. */
> + if (pending_state && !output->state_last) {
Umm... now that I look at it, I cannot see how this could ever be
called with pending_state not NULL. output_repaint_timer_handler() and
anything it calls do not really leave any opportunity to call
weston_compositor_dpms() or weston_compositor_sleep()?!
> + /* 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);
We set the disable state here, then:
drm_repaint_flush() -> drm_pending_state_apply() ->
drm_output_apply_state() ->
hit the "state->dpms != WESTON_DPMS_ON" path which will call
weston_output_finish_frame() unconditionally.
Would that not lead to a repaint state machine assertion violation,
which Michael Tretter was complaining about?
> + state->dpms = level;
get_disable_state() already sets state->dpms.
> return;
> }
>
> - output->dpms = level;
> + /* 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;
> + }
> +
> + 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_get_disable_state(pending_state, output);
> + ret = drm_pending_state_apply(pending_state);
Should this not be calling drm_pending_state_apply_sync() instead?
It's a little confusing as it does not matter whether one calls apply
or apply_sync, it's the DPMS state that determines whether it will be
sync or not.
Otherwise this code hints to me that it's an async apply.
> + if (ret != 0)
> + weston_log("drm_set_dpms: couldn't disable output?\n");
> }
>
> static const char * const connector_type_names[] = {
> @@ -4123,24 +4339,26 @@ 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;
> + int ret;
>
> if (output->page_flip_pending || output->vblank_pending) {
> output->disable_pending = 1;
> return -1;
> }
>
> + weston_log("Disabling output %s\n", output->base.name);
> + pending_state = drm_pending_state_alloc(b);
> + drm_output_get_disable_state(pending_state, output);
> + ret = drm_pending_state_apply_sync(pending_state);
This is using apply_sync.
> + if (ret)
> + weston_log("Couldn't disable output %s\n", output->base.name);
> +
> if (output->base.enabled)
> drm_output_deinit(&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);
> -
> return 0;
> }
>
Thanks,
pq
pgpO5C4QHQcrK.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
