[PATCH weston v4 1/3] compositor: If the output is inhibited, don't idle it off

[Derek Foreman]

On 10/08/16 07:25 PM, Bryce Harrington wrote:
> Adds a helper routine weston_output_inhibited_outputs() which returns a
> mask of outputs that should inhibit screen idling.
> 
> Use this routine to check for inhibiting outputs for handling of idle
> behaviors in core:  In sleep mode, only halt repainting outputs that
> don't have valid inhibits.  Don't send these monitors DPMS off commands
> either, if the system would otherwise be powering them down.
> 
> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> ---
>  libweston/compositor.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------
>  libweston/compositor.h | 10 +++++++++
>  2 files changed, 61 insertions(+), 6 deletions(-)
> 
> diff --git a/libweston/compositor.c b/libweston/compositor.c
> index b17c76d..aa14504 100644
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c
> @@ -485,6 +485,8 @@ weston_surface_create(struct weston_compositor *compositor)
>  
>  	wl_list_init(&surface->pointer_constraints);
>  
> +	surface->inhibit_idling = false;
> +
>  	return surface;
>  }
>  
> @@ -2320,15 +2322,42 @@ weston_output_schedule_repaint_reset(struct weston_output *output)
>  	TL_POINT("core_repaint_exit_loop", TLP_OUTPUT(output), TLP_END);
>  }
>  
> +/** Retrieves a mask of outputs that should inhibit screensaving
> + *
> + * \param compositor The compositor instance.
> + * \return An output mask indicating the ids of all inhibiting outputs
> + *
> + *  Checks for surfaces whose clients have requested that they
> + *  disable the screenserver and display powersaving.  Note
> + *  the output ids for these surfaces.
> + */
> +WL_EXPORT uint32_t
> +weston_compositor_inhibited_outputs(struct weston_compositor *compositor)
> +{
> +	struct weston_view *view;
> +	uint32_t inhibited_outputs_mask = 0;
> +
> +	wl_list_for_each(view, &compositor->view_list, link) {
> +		/* Does the view's surface inhibit this output? */
> +		if (!view->surface->inhibit_idling)
> +			continue;
> +
> +		inhibited_outputs_mask |= view->output_mask;
> +	}
> +	return inhibited_outputs_mask;
> +}
> +
>  static int
>  output_repaint_timer_handler(void *data)
>  {
>  	struct weston_output *output = data;
>  	struct weston_compositor *compositor = output->compositor;
> +	uint32_t inhibited_outputs_mask = weston_compositor_inhibited_outputs(compositor);
>  
>  	if (output->repaint_needed &&
> -	    compositor->state != WESTON_COMPOSITOR_SLEEPING &&

I don't like that this patch changes the meaning of
"WESTON_COMPOSITOR_SLEEPING" - should at least make an effort to update
all relevant comments such as the one immediately before
"weston_compositor_sleep" and the one in the enum definition in
compositor.h...

>  	    compositor->state != WESTON_COMPOSITOR_OFFSCREEN &&
> +	    (compositor->state != WESTON_COMPOSITOR_SLEEPING
> +	     || inhibited_outputs_mask & (1 << output->id)) &&
>  	    weston_output_repaint(output) == 0)
>  		return 0;
>  
> @@ -2450,9 +2479,15 @@ weston_output_schedule_repaint(struct weston_output *output)
>  {
>  	struct weston_compositor *compositor = output->compositor;
>  	struct wl_event_loop *loop;
> +	uint32_t inhibited_outputs_mask = weston_compositor_inhibited_outputs(compositor);
>  
> -	if (compositor->state == WESTON_COMPOSITOR_SLEEPING ||
> -	    compositor->state == WESTON_COMPOSITOR_OFFSCREEN)
> +	/* If we're offscreen, or if we're sleeping and the monitor
> +	 * isn't currently being inhibited by an active surface, then
> +	 * skip repainting.
> +	 */
> +	if (compositor->state == WESTON_COMPOSITOR_OFFSCREEN ||
> +	    (compositor->state == WESTON_COMPOSITOR_SLEEPING &&
> +	     !(inhibited_outputs_mask & (1 << output->id))))
>  		return;
>  
>  	if (!output->repaint_needed)
> @@ -3859,10 +3894,20 @@ weston_compositor_dpms(struct weston_compositor *compositor,
>  		       enum dpms_enum state)
>  {

The doxy block before this function has now become wrong, hasn't it?

>          struct weston_output *output;
> +	struct weston_view *view;
> +	uint32_t inhibited_outputs_mask = weston_compositor_inhibited_outputs(compositor);
>  
> -        wl_list_for_each(output, &compositor->output_list, link)
> -		if (output->set_dpms)
> -			output->set_dpms(output, state);
> +	wl_list_for_each(output, &compositor->output_list, link) {
> +		if (!output->set_dpms)
> +			continue;
> +
> +		/* If output is idle-inhibited, don't toggle to any DPMS state except ON. */
> +		if (state != WESTON_DPMS_ON &&
> +		    inhibited_outputs_mask & (1 << output->id))
> +			continue;
> +
> +		output->set_dpms(output, state);
> +	}
>  }
>  
>  /** Restores the compositor to active status
> diff --git a/libweston/compositor.h b/libweston/compositor.h
> index 0133084..3a9a098 100644
> --- a/libweston/compositor.h
> +++ b/libweston/compositor.h
> @@ -1140,6 +1140,14 @@ struct weston_surface {
>  
>  	/* An list of per seat pointer constraints. */
>  	struct wl_list pointer_constraints;
> +
> +	/*
> +	 * Indicates the surface prefers no screenblanking, screensaving,
> +	 * or other automatic obscurement to kick in while the surface is
> +	 * considered "active" by the shell.
> +	 */
> +	bool inhibit_idling;

I find this a bit unclear - "idling" doesn't feel properly descriptive,
and the comment gives me the impression that an "idle inhibit surface"
will be redrawn even if it's obscured - does that mean surface frame
events for the surface will be generated even if it's not visible in any
way?

Also, what's the bit about the surface being considered active by the
shell mean?

Thanks,
Derek

> +
>  };
>  
>  struct weston_subsurface {
> @@ -1316,6 +1324,8 @@ void
>  weston_output_schedule_repaint(struct weston_output *output);
>  void
>  weston_output_damage(struct weston_output *output);
> +uint32_t
> +weston_compositor_inhibited_outputs(struct weston_compositor *compositor);
>  void
>  weston_compositor_schedule_repaint(struct weston_compositor *compositor);
>  void
>