[PATCH weston v5 3/7] compositor: Add public interface support for client-requested idle inhibition

[Bryce Harrington]

On Tue, Aug 30, 2016 at 12:53:46PM +0200, Quentin Glidic wrote:
> On 30/08/2016 03:34, Bryce Harrington wrote:
> >Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> >
> >v5: Improve comments
> >Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> >---
> > Makefile.am            |  4 ++-
> > libweston/compositor.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > libweston/compositor.h |  8 ++---
> > 3 files changed, 95 insertions(+), 5 deletions(-)
> >
> >diff --git a/Makefile.am b/Makefile.am
> >index 6c3f4f7..ec561d2 100644
> >--- a/Makefile.am
> >+++ b/Makefile.am
> >@@ -168,7 +168,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/libweston/compositor.c b/libweston/compositor.c
> >index 7f92288..92f74aa 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"
> >@@ -4723,6 +4725,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);
> 
> Why setting to NULL if you
> >+}
> >+
> >+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);
> 
> If you assert, you must set_user_data(NULL) in the destructor,
> otherwise it’ll always pass, but with a dangling pointer, and
> ->surface being NULL.
> 
> 
> >+
> >+	inhibitor->surface->inhibit_idling = false;
> 
> This belongs to the destructor, "destroy_idle_inhibitor".
> 
> And you need wl_resource_destroy() (alone) here.
> 
> 
> >+}
> >+
> >+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)
> >+{
> 
> You need wl_resource_destroy() here.

Ok, thanks for the advice on cleaning up the destructor behaviors.

> >+}
> >+
> >+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;
> >+
> >+	wl_resource_set_implementation(cr, &idle_inhibitor_interface,
> >+				       inhibitor, destroy_idle_inhibitor);
> 
> I think you can use the surface as user_data directly here?

Do you mean instead of setting it as inhibitor->surface, pass it as the
user_data parameter for wl_resource_set_implementation()?

> >+}
> >+
> >+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);
> 
> Maybe add a comment that a NULL destructor is fine because you don’t
> have any memory to free here?
> Or maybe you need some memory, to keep track of inhibitors? Is the
> behaviour defined if you destroy the manager while having
> inhibitors?

That behavior has not been defined, to be honest I didn't give it any
consideration.  Is there a use case where one might want to do it?

When the client exits normally (or abnormally), the inhibitors are
getting the destroy events appropriately, so I'm not certain we would
need to deliberately track and destroy the inhibitors individually.

Bryce

> Cheers,
> 
> >+}
> >+
> > WL_EXPORT int
> > weston_environment_get_fd(const char *env)
> > {
> >@@ -4818,6 +4902,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);
> >diff --git a/libweston/compositor.h b/libweston/compositor.h
> >index 64d314d..eb76c8a 100644
> >--- a/libweston/compositor.h
> >+++ b/libweston/compositor.h
> >@@ -624,8 +624,9 @@ enum {
> > 	WESTON_COMPOSITOR_ACTIVE,	/* normal rendering and events */
> > 	WESTON_COMPOSITOR_IDLE,		/* shell->unlock called on activity */
> > 	WESTON_COMPOSITOR_OFFSCREEN,	/* no rendering, no frame events */
> >-	WESTON_COMPOSITOR_SLEEPING	/* same as offscreen, but also set dpms
> >-                                         * to off */
> >+	WESTON_COMPOSITOR_SLEEPING	/* same as offscreen, but also
> >+					 * attempt to set dpms to off where
> >+					 * applicable */
> > };
> >
> > struct weston_layer_entry {
> >@@ -1165,8 +1166,7 @@ struct weston_surface {
> >
> > 	/*
> > 	 * Indicates the surface prefers no screenblanking, screensaving,
> >-	 * or other automatic obscurement to kick in while the surface is
> >-	 * considered "active" by the shell.
> >+	 * or other automatic obscurement to kick in.
> 
> Maybe squash these in the relevant commit?
> 
> 
> > 	 */
> > 	bool inhibit_idling;
> >
> >
> 
> 
> -- 
> 
> Quentin “Sardem FF7” Glidic