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

[Quentin Glidic]

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.


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


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


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