On Fri, 10 Nov 2017 14:53:56 +0100 Michael Tretter <m.tret...@pengutronix.de> wrote:
> 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
> > @@ -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().
Hi,
when reviewing v14, I have been going through old review comments, and
this one made me think hard. Turning an output DPMS off should not
generate a pageflip event, and since a fresh pending_state is being
allocated and applied here, there should be no other cause for a
pagelip event either.
Finally I realized, the above code is missing
output_state->dpms = WESTON_DPMS_OFF;
So it's not even turning off the output, it's just committing a normal
state with no framebuffers... changed?
> 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.
v14 has a similar workaround, and I wonder if that is unnecessary when
the turning off is properly synchronous.
Thanks,
pq
pgplHZRDJiHqk.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
