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

[Pekka Paalanen]

On Tue, 26 Sep 2017 18:15:42 +0100
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.

Hi Daniel,

yes, it's messy, but probably not worth the effort to redesign how DPMS
state gets applied as a prerequisite. You did fine with the logic
anyway, I couldn't spot a mistake.

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

> +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;
> +	struct drm_plane_state *scanout_state;
> +
> +	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);
> +	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);

Others already pointed out the use of uninitialized 'state'. I'll check
that and raise with a
Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
given a trivial fix.

>  	return -1;
>  }
>  

> @@ -3026,8 +3147,6 @@ drm_output_find_special_plane(struct drm_backend *b, struct drm_output *output,
>  static void
>  drm_plane_destroy(struct drm_plane *plane)
>  {
> -	drmModeSetPlane(plane->backend->drm.fd, plane->plane_id, 0, 0, 0,
> -			0, 0, 0, 0, 0, 0, 0, 0);

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

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/20171003/162a99fa/attachment.sig>