[PATCH v15 03/34] compositor-drm: Move repaint state application to flush

[Pekka Paalanen]

On Mon,  5 Feb 2018 18:44:12 +0000
Daniel Stone <daniels at 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.
> 
> In drm_output_update_complete, the *_pending flags are cleared
> before any of the pending actions are taken; this ensures that the
> actions cannot recurse.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor-drm.c | 381 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 308 insertions(+), 73 deletions(-)
> 

Hi Daniel,

I found some little details to fix, but other than those, it looks good.

One thing I noticed in the legacy KMS path is that we don't tend to
program DPMS state when turning the CRTC off. Does the kernel do the
right thing, or should we always do DPMS-off when doing CRTC-off?

I'm more interested in the theory there, I don't mind if we leave the
monitor saying "no signal". Atomic will get that right anyway, I
presume, and monitors will probably turn off by themselves.

> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 22bf75aec..0b18a92f8 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c

> @@ -1305,6 +1331,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);

The above line seems unnecessary.

> +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

> @@ -3234,28 +3420,75 @@ 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) {
> +		/* 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);
> +		return;
> +	}
> +
> +	/* 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;

Redundant check, the output->state_cur->dpms == level check above
already does this.

> +
> +	/* 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;
>  	}
>  
> -	output->dpms = level;
> +	pending_state = drm_pending_state_alloc(b);
> +	drm_output_get_disable_state(pending_state, output);
> +	ret = drm_pending_state_apply(pending_state);

This was supposed to be drm_pending_state_apply_sync().

> +	if (ret != 0)
> +		weston_log("drm_set_dpms: couldn't disable output?\n");
>  }
>  
>  static const char * const connector_type_names[] = {

With the last note above fixed:

Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

The rest are optional.


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/20180206/ac81f651/attachment.sig>