[PATCH weston] Add comments and whitespace to repaint machinery

[Pekka Paalanen]

On Thu, 19 Jan 2017 14:05:01 +0000
Daniel Stone <daniels at collabora.com> wrote:

> repaint_needed / repaint_scheduled are surprisingly subtle. Explode the
> conditional with side-effects into more obvious separate calls, and
> document what they do.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/libweston/compositor.c b/libweston/compositor.c
> index d79122d..61a84a9 100644
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c
> @@ -2335,16 +2335,31 @@ output_repaint_timer_handler(void *data)
>  {
>  	struct weston_output *output = data;
>  	struct weston_compositor *compositor = output->compositor;
> +	int ret;
>  
> -	if (output->repaint_needed &&
> -	    compositor->state != WESTON_COMPOSITOR_SLEEPING &&
> -	    compositor->state != WESTON_COMPOSITOR_OFFSCREEN &&
> -	    weston_output_repaint(output) == 0)
> -		return 0;
> +	/* If we're sleeping, drop the repaint machinery entirely; we will
> +	 * explicitly repaint all outputs when we come back. */
> +	if (compositor->state == WESTON_COMPOSITOR_SLEEPING ||
> +	    compositor->state == WESTON_COMPOSITOR_OFFSCREEN)
> +		goto err;
>  
> -	weston_output_schedule_repaint_reset(output);
> +	/* We don't actually need to repaint this output; drop it from
> +	 * repaint until something causes damage. */
> +	if (!output->repaint_needed)
> +		goto err;

Hi,

FYI, if my memory serves right, this path is the exit condition from the
continuous repaint loop. We need to wait a bit (the timer) to see if
we are going to get something to repaint for the very next vblank. If
we didn't, we cancel the repaint and drop out from the continuous
repaint.

If we dropped out immediately when in weston_output_finish_frame(), we
would be dropping out all the time even when app were continuously
updating.

The cost of dropping out of continuous repaint is the the next time we
have to call weston_output::start_repaint_loop() which in the worst
case has to wait for a vblank, or at least do kernel roundtrip to ask
the timestamp.

Just to answer the "why would you ever schedule repaint without
damage?"...

> +
> +	/* If repaint fails, we aren't going to get weston_output_finish_frame
> +	 * to trigger a new repaint, so drop it from repaint and hope
> +	 * something later schedules a successful repaint. */
> +	ret = weston_output_repaint(output);
> +	if (ret != 0)
> +		goto err;
>  
>  	return 0;
> +
> +err:
> +	weston_output_schedule_repaint_reset(output);
> +	return 0;
>  }
>  
>  WL_EXPORT void

Everything is spot on.

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


Thanks,
pq