[PATCH weston v12 09/40] compositor-drm: Move repaint state application to flush

[Pekka Paalanen]

On Fri, 10 Nov 2017 14:53:56 +0100
Michael Tretter <m.tretter at 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 <daniels at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180117/ae67e474/attachment.sig>