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

[Philipp Zabel]

Hi Daniel,

On Tue, 2017-09-26 at 18:15 +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
[...]
> @@ -1360,7 +1397,6 @@ drm_output_assign_state(struct drm_output_state *state,
> >  	}
>  }
>  
> -
>  static int
>  drm_view_transform_supported(struct weston_view *ev)
>  {
> @@ -1653,37 +1689,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;

Here state was initialized ...

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

... so this may return ...

> -
> > -	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.
[...]
> @@ -1790,13 +1844,101 @@ 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_OUTPUT_STATE_UPDATE_ASYNCHRONOUS);
> +
> >  	return 0;
>  
>  err:
> >  	output->cursor_view = NULL;
> -
>  	drm_output_state_free(state);

... without causing issues here.

> > +	return -1;
> +}
> +
> +static int
> +drm_pending_state_apply(struct drm_pending_state *pending_state)
> +{
> > +	struct drm_backend *b = pending_state->backend;
> > +	struct drm_output_state *output_state, *tmp;
> > +	uint32_t *unused;
> +
> > +	if (b->state_invalid) {
> > +		/* If we need to reset all our state (e.g. because we've
> > +		 * just started, or just been VT-switched in), explicitly
> > +		 * disable all the CRTCs we aren't using. This also disables
> > +		 * all connectors on these CRTCs, so we don't need to do that
> > +		 * separately with the pre-atomic API. */
> > +		wl_array_for_each(unused, &b->unused_crtcs)
> > +			drmModeSetCrtc(b->drm.fd, *unused, 0, 0, 0, NULL, 0,
> > +				       NULL);
> > +	}
> +
> > +	wl_list_for_each_safe(output_state, tmp, &pending_state->output_list,
> > +			      link) {
> > +		struct drm_output *output = output_state->output;
> > +		int ret;
> +
> > +		ret = drm_output_apply_state(output_state);
> > +		if (ret != 0) {
> > +			weston_log("Couldn't apply state for output %s\n",
> > +				   output->base.name);
> > +		}
> > +	}
> +
> > +	b->state_invalid = false;
> +
> > +	assert(wl_list_empty(&pending_state->output_list));
>  
> > +	drm_pending_state_free(pending_state);
> +
> > +	return 0;
> +}
> +
> +static int
> +drm_output_repaint(struct weston_output *output_base,
> > +		   pixman_region32_t *damage,
> > +		   void *repaint_data)
> +{
> > +	struct drm_pending_state *pending_state = repaint_data;
> > +	struct drm_output *output = to_drm_output(output_base);
> +	struct drm_output_state *state;

Here state is not initialized anymore ...

> > +	struct drm_plane_state *scanout_state;
> +
> > +	if (output->disable_pending || output->destroy_pending)
> +		goto err;

... so this error path ...

> +
> > +	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);
> > +	state->dpms = WESTON_DPMS_ON;
> +
> > +	drm_output_render(state, damage);
> > +	scanout_state = drm_output_state_get_plane(state,
> > +						   output->scanout_plane);
> > +	if (!scanout_state || !scanout_state->fb)
> > +		goto err;
> +
> > +	return 0;
> +
> +err:
> +	drm_output_state_free(state);

... will cause drm_output_state_free to be called with uninitialized state.

> >  	return -1;
>  }

regards
Philipp