On Wed, 20 Dec 2017 12:26:28 +0000 Daniel Stone <[email protected]> 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 <[email protected]> > --- > 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
