On Tue, Aug 16, 2016 at 09:51:01AM -0500, Derek Foreman wrote:
> On 15/08/16 09:33 PM, Bryce Harrington wrote:
> > 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 <br...@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?
>
> Well, as a quick example, in compositor.h, the comment after
> WESTON_COMPOSITOR_SLEEPING is:
> /* same as offscreen, but also set dpms to off */
>
> This isn't accurate anymore as it's possible for us to be
> WESTON_COMPOSITOR_SLEEPING and not actually have any displays with dpms
> off at all, isn't it?
Yes that's true, if there are idle inhibits being honored, some or all
of the displays would not be dpms'd off.
> It becomes very confusing to figure out what's going on when "sleeping"
> doesn't necessarily mean anything is asleep at all. Maybe a new state
> should be introduced, but at a minimum all of the comments related to
> sleeping should be updated to be in sync with the new behaviour.
Do you mean rename WESTON_COMPOSITOR_SLEEPING to something more
descriptive, or to actually split SLEEP state into two different states?
If the former, what would be a better term than "sleeping" in this case?
WESTON_COMPOSITOR_INACTIVE? _STOPPED? ... _DROWSY?
Or, for just updating the comments, would tacking on "...where
applicable" be adequate, or would you want something more elaborate?
> >>> 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?
>
> Well, the doxy says "Set a DPMS mode on all of the compositor's outputs"
>
> After the patch it doesn't do that, it conditionally turns off monitors
> that aren't being governed by a blanker inhibitor.
Ok, does this strike you as better?
/** Apply a DPMS mode to the compositor's outputs.
*
* Outputs may skip setting the DPMS state if they are being used to
* display an surface that is requesting idle behaviors be inhibited.
*
* \param compositor The compositor instance
* \param state The DPMS state the outputs will be set 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.
>
> I guess I'm fine with it, no need to bike shed this point.
Ok.
> >> 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".
>
> Ah, ok, thought that might be the case. Would be good to see a patch
> with the text fixed.
I've applied this fix locally. I can repost the patchset once the
remainder of your review comments are resolved (and rebased to
trunk... which is proving to be a bit of a head scratcher due to the
libweston-desktop port.)
Thanks again for reviewing!
Bryce
> Thanks,
> Derek
>
> > 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
> >>>
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
