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

[Bryce Harrington]

On Mon, Aug 15, 2016 at 06:57:52PM -0500, Derek Foreman wrote:
> 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...

I'm not sure what you mean?
 
> >  	    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?

Not sure exactly what you mean by 'wrong', but I suppose it could
benefit from being updated to mention that idled outputs won't be
toggled off.  Is that what you're referring to?

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

Suggestions?  If the surface in in the view_list and assigned to an
output, then the idle inhibition will be honored.

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

That functionality got axed in an earlier review, this bit of doc was
missed.  Just ignore everything after "while".

Thanks,
Bryce
 
> 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
> >