[PATCH weston v4 2/3] compositor: Implement idle inhibition
Bryce Harrington
bryce at osg.samsung.com
Wed Aug 17 02:17:32 UTC 2016
On Tue, Aug 16, 2016 at 09:57:31AM -0500, Derek Foreman wrote:
> On 15/08/16 09:15 PM, Bryce Harrington wrote:
> > On Mon, Aug 15, 2016 at 07:01:24PM -0500, Derek Foreman wrote:
> >> On 10/08/16 07:25 PM, Bryce Harrington wrote:
> >>> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> >>> ---
> >>> Makefile.am | 4 ++-
> >>> configure.ac | 2 ++
> >>> libweston/compositor.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>> 3 files changed, 93 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/Makefile.am b/Makefile.am
> >>> index 32627f5..951f9ed 100644
> >>> --- a/Makefile.am
> >>> +++ b/Makefile.am
> >>> @@ -139,7 +139,9 @@ nodist_libweston_ at LIBWESTON_MAJOR@_la_SOURCES = \
> >>> protocol/relative-pointer-unstable-v1-protocol.c \
> >>> protocol/relative-pointer-unstable-v1-server-protocol.h \
> >>> protocol/pointer-constraints-unstable-v1-protocol.c \
> >>> - protocol/pointer-constraints-unstable-v1-server-protocol.h
> >>> + protocol/pointer-constraints-unstable-v1-server-protocol.h \
> >>> + protocol/idle-inhibit-unstable-v1-protocol.c \
> >>> + protocol/idle-inhibit-unstable-v1-server-protocol.h
> >>>
> >>> BUILT_SOURCES += $(nodist_libweston_ at LIBWESTON_MAJOR@_la_SOURCES)
> >>>
> >>> diff --git a/configure.ac b/configure.ac
> >>> index 74f931d..de26599 100644
> >>> --- a/configure.ac
> >>> +++ b/configure.ac
> >>> @@ -7,6 +7,8 @@ m4_define([libweston_major_version], [0])
> >>> m4_define([libweston_minor_version], [0])
> >>> m4_define([libweston_patch_version], [0])
> >>>
> >>> +# Note: Inhibition patchset requires inhibition protocol in wayland-protocol
> >>> +
> >>
> >> That's landed in wayland-protocol now - can we just make sure we require
> >> the right version of it instead of having a comment?
> >
> > Yeah it now requires 1.7 so no need for this comment.
> >
> >>> AC_PREREQ([2.64])
> >>> AC_INIT([weston],
> >>> [weston_version],
> >>> diff --git a/libweston/compositor.c b/libweston/compositor.c
> >>> index aa14504..e4ce273 100644
> >>> --- a/libweston/compositor.c
> >>> +++ b/libweston/compositor.c
> >>> @@ -51,6 +51,8 @@
> >>> #include <time.h>
> >>> #include <errno.h>
> >>>
> >>> +#include <idle-inhibit-unstable-v1-server-protocol.h>
> >>> +
> >>> #include "timeline.h"
> >>>
> >>> #include "compositor.h"
> >>> @@ -4665,6 +4667,88 @@ compositor_bind(struct wl_client *client,
> >>> compositor, NULL);
> >>> }
> >>>
> >>> +struct weston_idle_inhibitor {
> >>> + struct weston_surface *surface;
> >>> +};
> >>> +
> >>> +static void
> >>> +destroy_idle_inhibitor(struct wl_resource *resource)
> >>> +{
> >>> + struct weston_idle_inhibitor *inhibitor = wl_resource_get_user_data(resource);
> >>> +
> >>> + inhibitor->surface = NULL;
> >>> + free(inhibitor);
> >>> +}
> >>> +
> >>> +static void
> >>> +idle_inhibitor_destroy(struct wl_client *client, struct wl_resource *resource)
> >>> +{
> >>> + struct weston_idle_inhibitor *inhibitor = wl_resource_get_user_data(resource);
> >>> +
> >>> + assert(inhibitor);
> >>> +
> >>> + inhibitor->surface->inhibit_idling = false;
> >>
> >> Is there some mechanism in place that causes a re-assessment of screen
> >> blanker state when this happens?
> >>
> >> ie) if I destroy my inhibition will the monitor the video player was on
> >> immediately join the other monitors in sleep?
> >
> > Yep, that's right.
>
> Ok, I saw the code path for going to sleep when the surface is
> destroyed, but can't figure out how it gets put to sleep when the
> inhibit is destroyed. Is that done when it's next considered for redraw
> (what if it's not updating...)
>
> Is there a test client? I realize this is a bit hard to test in make
> check, so I'm not asking for that, but IMHO we can't land the series
> without a weston-idle-inhibit client or something that can be trivially
> used to check this all works manually.
Yeah I had a test client included in earlier revs. Last round asked
that it be broken out as a standalone client, but I haven't gotten
around to writing it. Was hoping to at least get the server bits signed
off. But yes having a client would help reviewers to verify that it
works in all corner cases like the one you're describing.
> >>> +}
> >>> +
> >>> +static const struct zwp_idle_inhibitor_v1_interface idle_inhibitor_interface = {
> >>> + idle_inhibitor_destroy
> >>> +};
> >>> +
> >>> +static void
> >>> +idle_inhibit_manager_destroy(struct wl_client *client, struct wl_resource *resource)
> >>> +{
> >>> +}
> >>> +
> >>> +static void
> >>> +idle_inhibit_manager_create_inhibitor(struct wl_client *client, struct wl_resource *resource,
> >>> + uint32_t id, struct wl_resource *surface_resource)
> >>> +{
> >>> + struct weston_surface *surface = wl_resource_get_user_data(surface_resource);
> >>> + struct weston_idle_inhibitor *inhibitor;
> >>> + struct wl_resource *cr;
> >>> +
> >>> + cr = wl_resource_create(client, &zwp_idle_inhibitor_v1_interface,
> >>> + wl_resource_get_version(resource), id);
> >>> + if (cr == NULL) {
> >>> + wl_client_post_no_memory(client);
> >>> + return;
> >>> + }
> >>> +
> >>> + inhibitor = zalloc(sizeof *inhibitor);
> >>> + if (inhibitor == NULL) {
> >>> + wl_client_post_no_memory(client);
> >>> + return;
> >>> + }
> >>> +
> >>> + inhibitor->surface = surface;
> >>> + inhibitor->surface->inhibit_idling = true;
> >>
> >> Will there be an immediate wake up of the monitor if it was already off now?
> >
> > No, in that case the user would need to re-wake the screen and let it
> > re-idle; the inhibition would take effect at that point.
>
> I don't like that - I think it should wake the display if something
> inhibits the screen blanker, even if it's already blank. (I'm thinking
> some kind of critical notification or alarm could wake the display this
> way...)
>
> But I could probably be convinced either way, would love to hear what
> others have to say about it...
What if the system is not just blanked but locked? By default weston is
set to lock the screen and require user input. Should clients be
permitted to bypass that?
>From a technical perspective, when weston idles the client surfaces are
removed from the view_list. In other words, no displays are assigned
for the client surfaces. Thus, there is technically not a mechanism to
determine which displays would need to be un-idled.
Bryce
> Thanks,
> Derek
>
> > Bryce
> >
> >> Thanks,
> >> Derek
> >>
> >>> +
> >>> + wl_resource_set_implementation(cr, &idle_inhibitor_interface,
> >>> + inhibitor, destroy_idle_inhibitor);
> >>> +}
> >>> +
> >>> +static const struct zwp_idle_inhibit_manager_v1_interface idle_inhibit_manager_interface = {
> >>> + idle_inhibit_manager_destroy,
> >>> + idle_inhibit_manager_create_inhibitor
> >>> +};
> >>> +
> >>> +static void
> >>> +bind_idle_inhibit_manager(struct wl_client *client,
> >>> + void *data, uint32_t version, uint32_t id)
> >>> +{
> >>> + struct wl_resource *resource;
> >>> +
> >>> + resource = wl_resource_create(client, &zwp_idle_inhibit_manager_v1_interface,
> >>> + version, id);
> >>> + if (resource == NULL) {
> >>> + wl_client_post_no_memory(client);
> >>> + return;
> >>> + }
> >>> +
> >>> + wl_resource_set_implementation(resource, &idle_inhibit_manager_interface,
> >>> + NULL, NULL);
> >>> +}
> >>> +
> >>> WL_EXPORT int
> >>> weston_environment_get_fd(const char *env)
> >>> {
> >>> @@ -4760,6 +4844,10 @@ weston_compositor_create(struct wl_display *display, void *user_data)
> >>> if (weston_input_init(ec) != 0)
> >>> goto fail;
> >>>
> >>> + if (!wl_global_create(ec->wl_display, &zwp_idle_inhibit_manager_v1_interface, 1,
> >>> + ec, bind_idle_inhibit_manager))
> >>> + goto fail;
> >>> +
> >>> wl_list_init(&ec->view_list);
> >>> wl_list_init(&ec->plane_list);
> >>> wl_list_init(&ec->layer_list);
> >>>
More information about the wayland-devel
mailing list