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 <br...@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_@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_@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);
> >>>
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
