Re: [PATCH weston 6/6] libweston: Implement touch timestamps for input_timestamps_unstable_v1

[Alexandros Frantzis]

On Fri, Jan 19, 2018 at 12:18:38PM +0200, Pekka Paalanen wrote:
> On Wed, 20 Dec 2017 16:18:01 +0200
> Alexandros Frantzis  wrote:
> 
> > Implement the zwp_input_timestamps_v1.get_touch_timestamps request to
> > subscribe to timestamp events for wl_touch resources.
> > 
> > Signed-off-by: Alexandros Frantzis 
> > ---
> >  libweston/compositor.h |  2 ++
> >  libweston/input.c  | 53 
> > +++---
> >  tests/touch-test.c | 46 +++
> >  3 files changed, 94 insertions(+), 7 deletions(-)
> 
> > @@ -4643,7 +4665,24 @@ input_timestamps_manager_get_touch_timestamps(struct 
> > wl_client *client,
> >   uint32_t id,
> >   struct wl_resource 
> > *touch_resource)
> >  {
> > -   wl_client_post_no_memory(client);
> > +   struct weston_seat *seat = wl_resource_get_user_data(touch_resource);
> > +   struct wl_resource *input_ts;
> > +
> > +   input_ts = wl_resource_create(client,
> > + _input_timestamps_v1_interface,
> > + 1, id);
> > +   if (!input_ts) {
> > +   wl_client_post_no_memory(client);
> > +   return;
> > +   }
> > +
> > +   wl_resource_set_implementation(input_ts,
> > +  _timestamps_interface,
> > +  touch_resource,
> > +  unbind_resource);
> > +
> > +   wl_list_insert(>touch_state->timestamps_list,
> > +  wl_resource_get_link(input_ts));
> 
> Btw. each of the three patches adds a new list to weston_keyboard,
> weston_pointer, weston_touch, but following the example already set in
> the code, do not handle that struct getting destroyed while client
> resources for it still exist. This would be good to fix, at least for
> the new lists introduced in these patches. I only realized that after
> sending the earlier R-bs.
> 
> You can find the places with:
>   $ git grep -p 'XXX: What about'
> 
> Otherwise the patch looks good, tests included. Nice work with the
> series.
> 
> Thanks,
> pq

Hi Pekka,

thanks for the review.

While investigating how to best solve this I realized that in order to
safely destroy the timestamp(_manager) objects, I had to first ensure
that the input objects are also properly destroyed. I have posted a
proposal for making the destruction of input objects safe here:

https://lists.freedesktop.org/archives/wayland-devel/2018-January/036701.html

When the input object destruction issues are resolved I will post
an updated version of this proposal.

Thanks,
Alexandros


___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 6/8] tests: Support weston_test request for adding a test seat

[Alexandros Frantzis]

Support adding a test seat using the weston_test.device_add request.
This will be used in tests in upcoming commits where we will need to
re-add the seat after having it removed.

We only support one test seat at the moment, so this commit also
introduces checks to ensure the client doesn't try to create multiple
test seats or try to remove an already removed test seat.

Signed-off-by: Alexandros Frantzis 
---
 tests/weston-test.c | 32 +---
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/tests/weston-test.c b/tests/weston-test.c
index 80b3d65b..9a2fd286 100644
--- a/tests/weston-test.c
+++ b/tests/weston-test.c
@@ -50,6 +50,7 @@ struct weston_test {
struct weston_layer layer;
struct weston_process process;
struct weston_seat seat;
+   bool is_seat_initialized;
 };
 
 struct weston_test_surface {
@@ -76,6 +77,22 @@ test_client_sigchld(struct weston_process *process, int 
status)
wl_display_terminate(test->compositor->wl_display);
 }
 
+static int
+test_seat_init(struct weston_test *test)
+{
+   /* create our own seat */
+   weston_seat_init(>seat, test->compositor, "test-seat");
+   test->is_seat_initialized = true;
+
+   /* add devices */
+   weston_seat_init_pointer(>seat);
+   if (weston_seat_init_keyboard(>seat, NULL) < 0)
+   return -1;
+   weston_seat_init_touch(>seat);
+
+   return 0;
+}
+
 static struct weston_seat *
 get_seat(struct weston_test *test)
 {
@@ -253,7 +270,10 @@ device_release(struct wl_client *client,
} else if (strcmp(device, "touch") == 0) {
weston_seat_release_touch(seat);
} else if (strcmp(device, "seat") == 0) {
+   assert(test->is_seat_initialized &&
+  "Trying to release already released test seat");
weston_seat_release(seat);
+   test->is_seat_initialized = false;
} else {
assert(0 && "Unsupported device");
}
@@ -272,6 +292,10 @@ device_add(struct wl_client *client,
weston_seat_init_keyboard(seat, NULL);
} else if (strcmp(device, "touch") == 0) {
weston_seat_init_touch(seat);
+   } else if (strcmp(device, "seat") == 0) {
+   assert(!test->is_seat_initialized &&
+  "Trying to add already added test seat");
+   test_seat_init(test);
} else {
assert(0 && "Unsupported device");
}
@@ -611,14 +635,8 @@ wet_module_init(struct weston_compositor *ec,
 test, bind_test) == NULL)
return -1;
 
-   /* create our own seat */
-   weston_seat_init(>seat, ec, "test-seat");
-
-   /* add devices */
-   weston_seat_init_pointer(>seat);
-   if (weston_seat_init_keyboard(>seat, NULL) < 0)
+   if (test_seat_init(test) == -1)
return -1;
-   weston_seat_init_touch(>seat);
 
loop = wl_display_get_event_loop(ec->wl_display);
wl_event_loop_add_idle(loop, idle_launch_client, test);
-- 
2.14.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 8/8] tests: Add test for seat destruction and creation

[Alexandros Frantzis]

Add a test to check that we can destroy and create the test seat. Since
after test seat destruction the test client releases any associated
input resources, this test also checks that libweston properly handles
release requests for inert input resources.

The test is placed in its own file for the time being, so it can run
independently. This is needed because the desktop shell, which is used
when running tests, doesn't deal well with seat destruction and creation
at the moment and may causes crashes in other tests. When this is fixed
we can merge this test into devices-test.c.

Signed-off-by: Alexandros Frantzis 
---
 Makefile.am   |  5 +
 tests/devices-seat-test.c | 53 +++
 2 files changed, 58 insertions(+)
 create mode 100644 tests/devices-seat-test.c

diff --git a/Makefile.am b/Makefile.am
index e224d606..f0370973 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1234,6 +1234,7 @@ weston_tests =\
subsurface.weston   \
subsurface-shot.weston  \
devices.weston  \
+   devices-seat.weston \
touch.weston
 
 ivi_tests =
@@ -1392,6 +1393,10 @@ devices_weston_SOURCES = tests/devices-test.c
 devices_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
 devices_weston_LDADD = libtest-client.la
 
+devices_seat_weston_SOURCES = tests/devices-seat-test.c
+devices_seat_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
+devices_seat_weston_LDADD = libtest-client.la
+
 text_weston_SOURCES = tests/text-test.c
 nodist_text_weston_SOURCES =   \
protocol/text-input-unstable-v1-protocol.c  \
diff --git a/tests/devices-seat-test.c b/tests/devices-seat-test.c
new file mode 100644
index ..182df1d5
--- /dev/null
+++ b/tests/devices-seat-test.c
@@ -0,0 +1,53 @@
+/*
+ * Copyright © 2018 Collabora, Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining
+ * a copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sublicense, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial
+ * portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT.  IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include "config.h"
+
+#include "weston-test-client-helper.h"
+
+/**
+ * Test destroying/recreating seats
+ *
+ * The seat destroy/recreate test is placed in its own file for the time
+ * being, so it can run independently. This is needed because the desktop
+ * shell, which is used when running tests, doesn't deal well with seat
+ * destruction and recreation at the moment and may causes crashes in other
+ * tests. When this is fixed we can merge this test into devices-test.c.
+ */
+
+TEST(seat_destroy_and_recreate)
+{
+   struct client *cl = create_client_and_test_surface(100, 100, 100, 100);
+
+   weston_test_device_release(cl->test->weston_test, "seat");
+   /* Roundtrip to receive and handle the seat global removal event */
+   client_roundtrip(cl);
+
+   weston_test_device_add(cl->test->weston_test, "seat");
+   /* First roundtrip to send request and receive new seat global */
+   client_roundtrip(cl);
+   /* Second roundtrip to handle seat events and set up input devices */
+   client_roundtrip(cl);
+}
-- 
2.14.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 7/8] tests: Handle removal of seat global in test clients

[Alexandros Frantzis]

The current test client code completely ignores removal of globals.
This commit updates the code to properly handle removal of globals in
general, and of seat globals in particular. This ensures that the test
client objects are in sync with the server and any relevant resources
are released accordingly.

This update will be used by upcoming tests to check that seat removal
and re-addition is working properly.

Signed-off-by: Alexandros Frantzis 
---
 tests/weston-test-client-helper.c | 71 +--
 tests/weston-test-client-helper.h |  1 +
 2 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/tests/weston-test-client-helper.c 
b/tests/weston-test-client-helper.c
index 854978d0..6dc72213 100644
--- a/tests/weston-test-client-helper.c
+++ b/tests/weston-test-client-helper.c
@@ -541,8 +541,23 @@ static const struct weston_test_listener test_listener = {
 static void
 input_destroy(struct input *inp)
 {
+   if (inp->pointer) {
+   if (inp->pointer->wl_pointer)
+   wl_pointer_release(inp->pointer->wl_pointer);
+   free(inp->pointer);
+   }
+   if (inp->keyboard) {
+   if (inp->keyboard->wl_keyboard)
+   wl_keyboard_release(inp->keyboard->wl_keyboard);
+   free(inp->keyboard);
+   }
+   if (inp->touch) {
+   if (inp->touch->wl_touch)
+   wl_touch_release(inp->touch->wl_touch);
+   free(inp->touch);
+   }
wl_list_remove(>link);
-   wl_seat_destroy(inp->wl_seat);
+   wl_seat_release(inp->wl_seat);
free(inp);
 }
 
@@ -748,6 +763,7 @@ handle_global(void *data, struct wl_registry *registry,
} else if (strcmp(interface, "wl_seat") == 0) {
input = xzalloc(sizeof *input);
input->client = client;
+   input->global_name = global->name;
input->wl_seat =
wl_registry_bind(registry, id,
 _seat_interface, version);
@@ -778,8 +794,59 @@ handle_global(void *data, struct wl_registry *registry,
}
 }
 
+static struct global *
+client_find_global_with_name(struct client *client, uint32_t name)
+{
+   struct global *global;
+
+   wl_list_for_each(global, >global_list, link) {
+   if (global->name == name)
+   return global;
+   }
+
+   return NULL;
+}
+
+static struct input *
+client_find_input_with_name(struct client *client, uint32_t name)
+{
+   struct input *input;
+
+   wl_list_for_each(input, >inputs, link) {
+   if (input->global_name == name)
+   return input;
+   }
+
+   return NULL;
+}
+
+static void
+handle_global_remove(void *data, struct wl_registry *registry, uint32_t name)
+{
+   struct client *client = data;
+   struct global *global;
+   struct input *input;
+
+   global = client_find_global_with_name(client, name);
+   assert(global && "Request to remove unknown global");
+
+   if (strcmp(global->interface, "wl_seat") == 0) {
+   input = client_find_input_with_name(client, name);
+   if (input) {
+   if (client->input == input)
+   client->input = NULL;
+   input_destroy(input);
+   }
+   }
+
+   wl_list_remove(>link);
+   free(global->interface);
+   free(global);
+}
+
 static const struct wl_registry_listener registry_listener = {
-   handle_global
+   handle_global,
+   handle_global_remove,
 };
 
 void
diff --git a/tests/weston-test-client-helper.h 
b/tests/weston-test-client-helper.h
index f16356e5..255bbf66 100644
--- a/tests/weston-test-client-helper.h
+++ b/tests/weston-test-client-helper.h
@@ -75,6 +75,7 @@ struct test {
 
 struct input {
struct client *client;
+   uint32_t global_name;
struct wl_seat *wl_seat;
struct pointer *pointer;
struct keyboard *keyboard;
-- 
2.14.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 0/8] libweston: Make input object destruction safe

[Alexandros Frantzis]

When an weston seat or input object is destroyed, any associated client
resources should become inert and put in a state in which they can
safely handle client requests. Currently, client requests to such
resources may lead to crashes and/or memory errors in the server.

This patchset aims to fix (or at least greatly improve) the situation.

Patches (1) to (4) ensure that when the various input objects are
destroyed, any associated resources are made inert and can safely handle
client requests.

Patches (5) to (7) update the test infrastructure to properly support
requests to dynamically add and remove the test seat.

Patch (8) introduces a test for removing and re-adding the test seat.
This test indirectly checks some of the input code fixes made in the
patches 1-4. Two things to note about this test:

1. As mentioned in more detail in the commit, the test needs to be in
its own file for now.  I haven't investigated in depth but the problem
seems to be in desktop-shell's lack of support for wl_seat
removal/re-addition. As a test, I tried running with the
fullscreen-shell and the problem is gone. This was too deep of a rabbit
hole to go into in this patchset, but I will investigate further and
hopefully we can remove this workaround.

2. Even without some of the fixes in the patches 1-4, the test may seem
to pass. However, running with a memory debugger reveals a different
story, since without the fixes we encounter various memory errors.

Alexandros Frantzis (8):
  libweston: Make weston_pointer destruction safe
  libweston: Make weston_keyboard destruction safe
  libweston: Make weston_touch destruction safe
  libweston: Make weston_seat release safe
  tests: Support setting the test client input dynamically
  tests: Support weston_test request for adding a test seat
  tests: Handle removal of seat global in test clients
  tests: Add test for seat destruction and creation

 Makefile.am   |   5 ++
 libweston/input.c | 115 -
 tests/devices-seat-test.c |  53 ++
 tests/weston-test-client-helper.c | 147 +-
 tests/weston-test-client-helper.h |   2 +
 tests/weston-test.c   |  32 +++--
 6 files changed, 294 insertions(+), 60 deletions(-)
 create mode 100644 tests/devices-seat-test.c

-- 
2.14.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 5/8] tests: Support setting the test client input dynamically

[Alexandros Frantzis]

The current test client code waits for all wl_seat globals to arrive
before checking them and deciding which one is the test seat global to
use for the input object. This method doesn't support dynamic addition
of the test seat global (i.e., after client start-up), which will be
needed in upcoming commits.

This commit changes the code to check for the test seat and set up the
input object while handling the wl_seat information events.

Signed-off-by: Alexandros Frantzis 
---
 tests/weston-test-client-helper.c | 78 ++-
 tests/weston-test-client-helper.h |  1 +
 2 files changed, 46 insertions(+), 33 deletions(-)

diff --git a/tests/weston-test-client-helper.c 
b/tests/weston-test-client-helper.c
index 6e0a5246..854978d0 100644
--- a/tests/weston-test-client-helper.c
+++ b/tests/weston-test-client-helper.c
@@ -538,6 +538,14 @@ static const struct weston_test_listener test_listener = {
test_handle_capture_screenshot_done,
 };
 
+static void
+input_destroy(struct input *inp)
+{
+   wl_list_remove(>link);
+   wl_seat_destroy(inp->wl_seat);
+   free(inp);
+}
+
 static void
 input_update_devices(struct input *input)
 {
@@ -598,22 +606,56 @@ seat_handle_capabilities(void *data, struct wl_seat *seat,
 
/* we will create/update the devices only with the right (test) seat.
 * If we haven't discovered which seat is the test seat, just
-* store capabilities and bail out */
-   if (input->seat_name && strcmp(input->seat_name, "test-seat") == 0)
+* store capabilities and bail out. Note that we don't need to
+* check the name contents here, since only the test seat input will
+* have its name set */
+   if (input->seat_name) {
input_update_devices(input);
+   input->client->input = input;
+   }
+
 
fprintf(stderr, "test-client: got seat %p capabilities: %x\n",
input, caps);
 }
 
+static struct input *
+client_find_input_with_seat_name(struct client *client, const char *name)
+{
+   struct input *input;
+
+   wl_list_for_each(input, >inputs, link) {
+   if (input->seat_name && strcmp(input->seat_name, name) == 0)
+   return input;
+   }
+
+   return NULL;
+}
+
 static void
 seat_handle_name(void *data, struct wl_seat *seat, const char *name)
 {
struct input *input = data;
 
+   /* We don't care about seats other than the test seat */
+   if (strcmp(name, "test-seat") != 0) {
+   input_destroy(input);
+   return;
+   }
+
+   assert(!client_find_input_with_seat_name(input->client, name) &&
+  "Multiple test seats detected!");
+
input->seat_name = strdup(name);
assert(input->seat_name && "No memory");
 
+   /* We assume that the test seat always has some capabilities,
+* so caps == 0 just means that we haven't received them yet */
+   if (input->caps != 0) {
+   input_update_devices(input);
+   input->client->input = input;
+   }
+
fprintf(stderr, "test-client: got seat %p name: \'%s\'\n",
input, name);
 }
@@ -705,6 +747,7 @@ handle_global(void *data, struct wl_registry *registry,
 _compositor_interface, version);
} else if (strcmp(interface, "wl_seat") == 0) {
input = xzalloc(sizeof *input);
+   input->client = client;
input->wl_seat =
wl_registry_bind(registry, id,
 _seat_interface, version);
@@ -809,34 +852,6 @@ log_handler(const char *fmt, va_list args)
vfprintf(stderr, fmt, args);
 }
 
-static void
-input_destroy(struct input *inp)
-{
-   wl_list_remove(>link);
-   wl_seat_destroy(inp->wl_seat);
-   free(inp);
-}
-
-/* find the test-seat and set it in client.
- * Destroy other inputs */
-static void
-client_set_input(struct client *cl)
-{
-   struct input *inp, *inptmp;
-   wl_list_for_each_safe(inp, inptmp, >inputs, link) {
-   assert(inp->seat_name && "BUG: input with no name");
-   if (strcmp(inp->seat_name, "test-seat") == 0) {
-   cl->input = inp;
-   input_update_devices(inp);
-   } else {
-   input_destroy(inp);
-   }
-   }
-
-   /* we keep only one input */
-   assert(wl_list_length(>inputs) == 1);
-}
-
 struct client *
 create_client(void)
 {
@@ -862,9 +877,6 @@ create_client(void)
 * events */
client_roundtrip(client);
 
-   /* find the right input for us */
-   client_set_input(client);
-
/* must have WL_SHM_FORMAT_ARGB32 */
assert(client->has_argb);
 
diff --git a/tests/weston-test-client-helper.h 
b/tests/weston-test-client-helper.h
index 09a5df4a..f16356e5 100644
--- 

[PATCH weston 1/8] libweston: Make weston_pointer destruction safe

[Alexandros Frantzis]

Properly clean up all sub-objects (e.g., weston_pointer_client objects)
when a weston_pointer object is destroyed. The clean-up ensures that the
server is able to safely handle client requests to any associated
pointer resources, which, as a consenquence of a weston_pointer
destruction, have now become inert.

The clean-up involves, among other things, unsetting the destroyed
weston_pointer object from the user data of pointer resources, and
handling this NULL user data case where required.

Signed-off-by: Alexandros Frantzis 
---
 libweston/input.c | 41 -
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/libweston/input.c b/libweston/input.c
index 94a3423a..01f0d568 100644
--- a/libweston/input.c
+++ b/libweston/input.c
@@ -105,6 +105,19 @@ weston_pointer_client_create(struct wl_client *client)
 static void
 weston_pointer_client_destroy(struct weston_pointer_client *pointer_client)
 {
+   struct wl_resource *resource;
+
+   wl_resource_for_each(resource, _client->pointer_resources) {
+   wl_resource_set_user_data(resource, NULL);
+   }
+
+   wl_resource_for_each(resource,
+_client->relative_pointer_resources) {
+   wl_resource_set_user_data(resource, NULL);
+   }
+
+   wl_list_remove(_client->pointer_resources);
+   wl_list_remove(_client->relative_pointer_resources);
free(pointer_client);
 }
 
@@ -170,11 +183,14 @@ unbind_pointer_client_resource(struct wl_resource 
*resource)
struct wl_client *client = wl_resource_get_client(resource);
struct weston_pointer_client *pointer_client;
 
-   pointer_client = weston_pointer_get_pointer_client(pointer, client);
-   assert(pointer_client);
-
wl_list_remove(wl_resource_get_link(resource));
-   weston_pointer_cleanup_pointer_client(pointer, pointer_client);
+
+   if (pointer) {
+   pointer_client = weston_pointer_get_pointer_client(pointer,
+  client);
+   assert(pointer_client);
+   weston_pointer_cleanup_pointer_client(pointer, pointer_client);
+   }
 }
 
 static void unbind_resource(struct wl_resource *resource)
@@ -1092,12 +1108,18 @@ weston_pointer_create(struct weston_seat *seat)
 WL_EXPORT void
 weston_pointer_destroy(struct weston_pointer *pointer)
 {
+   struct weston_pointer_client *pointer_client, *tmp;
+
wl_signal_emit(>destroy_signal, pointer);
 
if (pointer->sprite)
pointer_unmap_sprite(pointer);
 
-   /* XXX: What about pointer->resource_list? */
+   wl_list_for_each_safe(pointer_client, tmp, >pointer_clients,
+ link) {
+   wl_list_remove(_client->link);
+   weston_pointer_client_destroy(pointer_client);
+   }
 
wl_list_remove(>focus_resource_listener.link);
wl_list_remove(>focus_view_listener.link);
@@ -2297,6 +2319,9 @@ pointer_set_cursor(struct wl_client *client, struct 
wl_resource *resource,
struct weston_pointer *pointer = wl_resource_get_user_data(resource);
struct weston_surface *surface = NULL;
 
+   if (!pointer)
+   return;
+
if (surface_resource)
surface = wl_resource_get_user_data(surface_resource);
 
@@ -3674,6 +3699,9 @@ pointer_constraints_lock_pointer(struct wl_client *client,
struct weston_region *region = region_resource ?
wl_resource_get_user_data(region_resource) : NULL;
 
+   if (!pointer)
+   return;
+
init_pointer_constraint(resource, id, surface, pointer, region, 
lifetime,
_locked_pointer_v1_interface,
_pointer_interface,
@@ -4467,6 +4495,9 @@ pointer_constraints_confine_pointer(struct wl_client 
*client,
struct weston_region *region = region_resource ?
wl_resource_get_user_data(region_resource) : NULL;
 
+   if (!pointer)
+   return;
+
init_pointer_constraint(resource, id, surface, pointer, region, 
lifetime,
_confined_pointer_v1_interface,
_pointer_interface,
-- 
2.14.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 3/8] libweston: Make weston_touch destruction safe

[Alexandros Frantzis]

Ensure the server can safely handle client requests for wl_touch
resources that have become inert due to a weston_touch object
destruction.

This change involves, among other things, setting the weston_touch
object, instead of the weston_seat object, as the user data for wl_touch
resources. Although this is not strictly required at the moment (since
no code is using the wl_touch user data), it makes the code safer:

 * It makes more sense conceptually.
 * It is consistent with how wl_pointer resources are handled.
 * It allows us to clear the user data during weston_touch
   destruction, so other code can check whether the resource is
   inert.

Signed-off-by: Alexandros Frantzis 
---
 libweston/input.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/libweston/input.c b/libweston/input.c
index 96cabf25..48bcc55c 100644
--- a/libweston/input.c
+++ b/libweston/input.c
@@ -1221,8 +1221,18 @@ weston_touch_create(void)
 WL_EXPORT void
 weston_touch_destroy(struct weston_touch *touch)
 {
-   /* XXX: What about touch->resource_list? */
+   struct wl_resource *resource;
+
+   wl_resource_for_each(resource, >resource_list) {
+   wl_resource_set_user_data(resource, NULL);
+   }
+
+   wl_resource_for_each(resource, >focus_resource_list) {
+   wl_resource_set_user_data(resource, NULL);
+   }
 
+   wl_list_remove(>resource_list);
+   wl_list_remove(>focus_resource_list);
wl_list_remove(>focus_view_listener.link);
wl_list_remove(>focus_resource_listener.link);
free(touch);
@@ -2599,7 +2609,7 @@ seat_get_touch(struct wl_client *client, struct 
wl_resource *resource,
   wl_resource_get_link(cr));
}
wl_resource_set_implementation(cr, _interface,
-  seat, unbind_resource);
+  touch, unbind_resource);
 }
 
 static void
-- 
2.14.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 4/8] libweston: Make weston_seat release safe

[Alexandros Frantzis]

Ensure the server can safely handle client requests for wl_seat resource
that have become inert due to weston_seat object release and subsequent
destruction.

The clean-up involves, among other things, unsetting the destroyed
weston_seat object from the user data of wl_seat resources, and handling
this NULL user data case where required.

Signed-off-by: Alexandros Frantzis 
---
 libweston/input.c | 45 +++--
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/libweston/input.c b/libweston/input.c
index 48bcc55c..e4daa56b 100644
--- a/libweston/input.c
+++ b/libweston/input.c
@@ -2412,6 +2412,13 @@ seat_get_pointer(struct wl_client *client, struct 
wl_resource *resource,
 uint32_t id)
 {
struct weston_seat *seat = wl_resource_get_user_data(resource);
+   struct weston_pointer *pointer;
+   struct wl_resource *cr;
+   struct weston_pointer_client *pointer_client;
+
+   if (!seat)
+   return;
+
/* We use the pointer_state directly, which means we'll
 * give a wl_pointer if the seat has ever had one - even though
 * the spec explicitly states that this request only takes effect
@@ -2420,10 +2427,7 @@ seat_get_pointer(struct wl_client *client, struct 
wl_resource *resource,
 * This prevents a race between the compositor sending new
 * capabilities and the client trying to use the old ones.
 */
-   struct weston_pointer *pointer = seat->pointer_state;
-   struct wl_resource *cr;
-   struct weston_pointer_client *pointer_client;
-
+   pointer = seat->pointer_state;
if (!pointer)
return;
 
@@ -2499,6 +2503,12 @@ seat_get_keyboard(struct wl_client *client, struct 
wl_resource *resource,
  uint32_t id)
 {
struct weston_seat *seat = wl_resource_get_user_data(resource);
+   struct weston_keyboard *keyboard;
+   struct wl_resource *cr;
+
+   if (!seat)
+   return;
+
/* We use the keyboard_state directly, which means we'll
 * give a wl_keyboard if the seat has ever had one - even though
 * the spec explicitly states that this request only takes effect
@@ -2507,9 +2517,7 @@ seat_get_keyboard(struct wl_client *client, struct 
wl_resource *resource,
 * This prevents a race between the compositor sending new
 * capabilities and the client trying to use the old ones.
 */
-   struct weston_keyboard *keyboard = seat->keyboard_state;
-   struct wl_resource *cr;
-
+   keyboard = seat->keyboard_state;
if (!keyboard)
return;
 
@@ -2579,6 +2587,12 @@ seat_get_touch(struct wl_client *client, struct 
wl_resource *resource,
   uint32_t id)
 {
struct weston_seat *seat = wl_resource_get_user_data(resource);
+   struct weston_touch *touch;
+   struct wl_resource *cr;
+
+   if (!seat)
+   return;
+
/* We use the touch_state directly, which means we'll
 * give a wl_touch if the seat has ever had one - even though
 * the spec explicitly states that this request only takes effect
@@ -2587,9 +2601,7 @@ seat_get_touch(struct wl_client *client, struct 
wl_resource *resource,
 * This prevents a race between the compositor sending new
 * capabilities and the client trying to use the old ones.
 */
-   struct weston_touch *touch = seat->touch_state;
-   struct wl_resource *cr;
-
+   touch = seat->touch_state;
if (!touch)
return;
 
@@ -3087,6 +3099,19 @@ weston_seat_init(struct weston_seat *seat, struct 
weston_compositor *ec,
 WL_EXPORT void
 weston_seat_release(struct weston_seat *seat)
 {
+   struct wl_resource *resource;
+
+   wl_resource_for_each(resource, >base_resource_list) {
+   wl_resource_set_user_data(resource, NULL);
+   }
+
+   wl_resource_for_each(resource, >drag_resource_list) {
+   wl_resource_set_user_data(resource, NULL);
+   }
+
+   wl_list_remove(>base_resource_list);
+   wl_list_remove(>drag_resource_list);
+
wl_list_remove(>link);
 
if (seat->saved_kbd_focus)
-- 
2.14.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 2/8] libweston: Make weston_keyboard destruction safe

[Alexandros Frantzis]

Ensure the server can safely handle client requests for wl_keyboard
resources that have become inert due to a weston_keyboard object
destruction.

This change involves, among other things, setting the weston_keyboard
object, instead of the weston_seat object, as the user data for
wl_keyboard resources.  Although this is not strictly required at the
moment (since no code is using the wl_keyboard user data), it makes the
code safer:

 * It makes more sense conceptually.
 * It is consistent with how wl_pointer resources are handled.
 * It allows us to clear the user data during weston_keyboard
   destruction, so other code can check whether the resource is inert.

Signed-off-by: Alexandros Frantzis 
---
 libweston/input.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/libweston/input.c b/libweston/input.c
index 01f0d568..96cabf25 100644
--- a/libweston/input.c
+++ b/libweston/input.c
@@ -1166,7 +1166,18 @@ weston_xkb_info_destroy(struct weston_xkb_info 
*xkb_info);
 WL_EXPORT void
 weston_keyboard_destroy(struct weston_keyboard *keyboard)
 {
-   /* XXX: What about keyboard->resource_list? */
+   struct wl_resource *resource;
+
+   wl_resource_for_each(resource, >resource_list) {
+   wl_resource_set_user_data(resource, NULL);
+   }
+
+   wl_resource_for_each(resource, >focus_resource_list) {
+   wl_resource_set_user_data(resource, NULL);
+   }
+
+   wl_list_remove(>resource_list);
+   wl_list_remove(>focus_resource_list);
 
xkb_state_unref(keyboard->xkb_state.state);
if (keyboard->xkb_info)
@@ -2504,7 +2515,7 @@ seat_get_keyboard(struct wl_client *client, struct 
wl_resource *resource,
 * focused */
wl_list_insert(>resource_list, wl_resource_get_link(cr));
wl_resource_set_implementation(cr, _interface,
-  seat, unbind_resource);
+  keyboard, unbind_resource);
 
if (wl_resource_get_version(cr) >= 
WL_KEYBOARD_REPEAT_INFO_SINCE_VERSION) {
wl_keyboard_send_repeat_info(cr,
-- 
2.14.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 3/5] scanner: introduce --object-type option

[Emil Velikov]

On 26 January 2018 at 02:44, Jonas Ådahl  wrote:
> On Thu, Jan 25, 2018 at 05:56:45PM +, Emil Velikov wrote:
>> On 25 January 2018 at 02:01, Jonas Ådahl  wrote:
>> > On Wed, Jan 24, 2018 at 07:15:09PM +, Emil Velikov wrote:
>> >> On 24 January 2018 at 18:20, Derek Foreman  wrote:
>> >> > On 2018-01-22 09:30 AM, Emil Velikov wrote:
>> >> >>
>> >> >> On 22 August 2017 at 14:02, Emil Velikov  
>> >> >> wrote:
>> >> >>>
>> >> >>> On 18 August 2017 at 13:05, Pekka Paalanen  
>> >> >>> wrote:
>> >> >>>
>> >> >>
>> >> >> The exported configuration would then be:
>> >> >> LOCAL_INTERFACE_DECL=extern
>> >> >> EXTERN_INTERFACE_DECL=extern
>> >> >> LOCAL_INTERFACE_DEF=WL_EXPORT
>> >> >>
>> >> >> That would be far too flexible and no-one would use it right, 
>> >> >> right?
>> >> >>
>> >> > I did not introduce separate tokens, since those are (and should be)
>> >> > used _only_ in the .c file.
>> >> > Personally then do not seem necessary, If you prefer we can add them
>> >> > though.
>> >> 
>> >> 
>> >>  Ah, no, that was just a wild idea of something completely different. 
>> >>  I
>> >>  meant that the user project would be setting those macros before 
>> >>  using
>> >>  scanner-generated files, and if unset, the scanner-emitted code would
>> >>  default to the legacy behaviour. That way there would be no 
>> >>  visibility
>> >>  modes in scanner itself. If it's not obviously better, then 
>> >>  nevermind.
>> >>  It certainly has a lot more room to go wrong than your proposal.
>> >> 
>> >> 
>> >> >>> I see.
>> >> >>>
>> >> >>> Personally I'd lean towards with my approach for now since it is
>> >> >>> simpler, despite that it provides less flexibility.
>> >> >>> As you pointed out the proposal is a bit more fragile, so might be
>> >> >>> better to avoid until there's a real need for it.
>> >> >>>
>> >> >>>
>> >>  ...
>> >> 
>> >> >> The patch looks pretty much correct to me, if we choose to go this
>> >> >> way.
>> >> >>
>> >> > Glad to hear.
>> >> >
>> >> > I'll let me know once you guys are settled in on the approach, and
>> >> > I'll respin the series with all the comments addressed.
>> >> 
>> >> 
>> >>  Cool, let's see if we can get the name conflict issue solved, and 
>> >>  then
>> >>  I'll try to remember to ping you.
>> >> 
>> >> >>> Ack, I'll keep an eye open, just in case.
>> >> >>>
>> >> >> Considering the status of the the name conflict series, should I
>> >> >> re-spin this lot?
>> >> >> I'm more than happy to tweak things - say rename the toggle, etc.
>> >> >
>> >> >
>> >> > I see there were two series proposed to control symbol visibility, 
>> >> > yours and
>> >> > Jonas'?
>> >> >
>> >> > Assuming that once we drop the symbol collision issue they both solve 
>> >> > the
>> >> > same problems, it would be good if we could focus on one going forward.
>> >> >
>> >> > Is this the chosen one?
>> >> >
>> >> Right, the cover letter [1] covers some of the high-lights/differences.
>> >> As a TL;DR: using static/shared is more common and gives us more
>> >> flexibility for the future.
>> >>
>> >> So far Pekka is the only person who commented on the series/approach
>> >> and seemed happy.
>> >> I was expecting others to weight in - say Jonas ;-) I'll respin the
>> >> series tomorrow.
>> >>
>> >> In hindsight --object-type={shared,static} is too much of a mouthful,
>> >> I'll opt for --{static,shared} for v2.
>> >
>> > I have no opinion of what variant to land (I'm slightly too lazy to
>> > search for my own, and this is high up the inbox thanks to the replies,
>> > so lets focus on this one).
>> >
>> > My only nit is using the term "object-type". I think refering to it as
>> > "visibility" ("symbol visibility" if wanting to be extra verbose) where
>> > one can say 'export', 'static' or 'private' is more accurate.
>> >
>> > "objects" are a Wayland protocol thing and that is not what we are
>> > poking at here.
>> >
>> Right using "object" might be misleading. On the other hand,
>> --shared/--static are well known and dead-obvious.
>> Plus it gives us flexibility to sweep more things under it, if we get
>> some funky stuff in the future.
>>
>> Can I buy you on the different name?
>
> --static is pretty clear, but --shared? Both WL_EXPORT and WL_PRIVATE
> are "shared" but just different audiences.
>
The actual symbol notation WL_EXPORT/WL_PRIVATE/other(?) is an
implementation detail.
That is "hidden" within the C files and never exposed outside.

There are two reasons for that:
 - used internally, thus no point in polluting the namespace
 - some compilers (older clang or was it the oracle/sun one?) are
unhappy if the header is annotates, while the C file isn't

-Emil
___
wayland-devel 

[PATCH weston 4/4] ivi-shell: fix the layer assignment change from one screen to another

[Emre Ucan]

if the layer is in order of some screen we need to remove it
from there and mark the screen order as dirty so it will be removed
in commit_screen_list call later
layer should only be assigned to one screen at a time

Signed-off-by: Eugen Friedrich 
Signed-off-by: Emre Ucan 
---
 ivi-shell/ivi-layout.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c
index 4799c25..976a426 100644
--- a/ivi-shell/ivi-layout.c
+++ b/ivi-shell/ivi-layout.c
@@ -1535,6 +1535,11 @@ ivi_layout_screen_add_layer(struct weston_output *output,
 
iviscrn = get_screen_from_output(output);
 
+   /*if layer is already assigned to screen make order of it dirty
+* we are going to remove it (in commit_screen_list)*/
+   if (addlayer->on_screen)
+   addlayer->on_screen->order.dirty = 1;
+
wl_list_remove(>pending.link);
wl_list_insert(>pending.layer_list, >pending.link);
 
-- 
2.7.4

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 3/4] ivi-shell: don't expilicitly assign outputs to views

[Emre Ucan]

it is assigned in weston_view_assign_outputs

Signed-off-by: Emre Ucan 
---
 ivi-shell/ivi-layout.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c
index 3c52ce1..4799c25 100644
--- a/ivi-shell/ivi-layout.c
+++ b/ivi-shell/ivi-layout.c
@@ -821,10 +821,6 @@ commit_screen_list(struct ivi_layout *layout)
 

weston_layer_entry_insert(>layout_layer.view_list,
  
_view->view->layer_link);
-
-   ivi_view->view->output = iviscrn->output;
-   ivi_view->ivisurf->surface->is_mapped = true;
-   ivi_view->view->is_mapped = true;
}
}
}
-- 
2.7.4

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 1/4] ivi-shell: change layer visibility to bool

[Emre Ucan]

ivi_layout_layer_set_visibility has bool
as argument.

Signed-off-by: Emre Ucan 
---
 ivi-shell/ivi-layout-export.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ivi-shell/ivi-layout-export.h b/ivi-shell/ivi-layout-export.h
index 277ac59..f656602 100644
--- a/ivi-shell/ivi-layout-export.h
+++ b/ivi-shell/ivi-layout-export.h
@@ -101,7 +101,7 @@ struct ivi_layout_layer_properties
int32_t dest_width;
int32_t dest_height;
enum wl_output_transform orientation;
-   uint32_t visibility;
+   bool visibility;
int32_t transition_type;
uint32_t transition_duration;
double start_alpha;
-- 
2.7.4

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH v14 35/41] compositor-drm: Add modes to drm_output_propose_state

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:52 +
Daniel Stone  wrote:

> Add support for multiple modes: toggling whether or not the renderer
> and/or planes are acceptable. This will be used to implement a smarter
> plane-placement heuristic when we have support for testing output
> states.
> 
> Signed-off-by: Daniel Stone 
> ---
>  libweston/compositor-drm.c | 39 +++
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 71027a5ae..ff70f9c29 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -1766,6 +1766,11 @@ drm_output_assign_state(struct drm_output_state *state,
>   }
>  }
>  
> +enum drm_output_propose_state_mode {
> + DRM_OUTPUT_PROPOSE_STATE_MIXED, /**< mix renderer & planes */
> + DRM_OUTPUT_PROPOSE_STATE_PLANES_ONLY, /**< only assign to planes */
> +};

Hi,

what is the reason to have a planes-only mode? I saw the patch
"compositor-drm: Add test-only mode to state application" will first
attempt a planes-only mode and then fall back to mixed mode. Why does
the planes-only mode not fall out of mixed mode naturally when it runs
out of views to show? This would be good to explain in the commit
message to justify the modes.

> +
>  static struct weston_plane *
>  drm_output_prepare_scanout_view(struct drm_output_state *output_state,
>   struct weston_view *ev)
> @@ -3049,13 +3054,17 @@ err:
>  
>  static struct drm_output_state *
>  drm_output_propose_state(struct weston_output *output_base,
> -  struct drm_pending_state *pending_state)
> +  struct drm_pending_state *pending_state,
> +  enum drm_output_propose_state_mode mode)
>  {
>   struct drm_output *output = to_drm_output(output_base);
> + struct drm_backend *b = to_drm_backend(output_base->compositor);
>   struct drm_output_state *state;
>   struct weston_view *ev;
>   pixman_region32_t surface_overlap, renderer_region, occluded_region;
>   struct weston_plane *primary = _base->compositor->primary_plane;
> + bool renderer_ok = (mode != DRM_OUTPUT_PROPOSE_STATE_PLANES_ONLY);
> + bool planes_ok = !b->sprites_are_broken;

Would you want to remove the sprites_are_broken check from
drm_output_prepare_overlay_view()?

>  
>   assert(!output->state_last);
>   state = drm_output_state_duplicate(output->state_cur,
> @@ -3118,36 +3127,49 @@ drm_output_propose_state(struct weston_output 
> *output_base,
>   next_plane = primary;
>   pixman_region32_fini(_overlap);
>  
> - if (next_plane == NULL)
> + /* The cursor plane is 'special' in the sense that we can still
> +  * place it in the legacy API, and we gate that with a separate
> +  * cursors_are_broken flag. */
> + if (next_plane == NULL && !b->cursors_are_broken)

Would you want to remove the cursors_are_broken check from
drm_output_prepare_cursor_view()?

>   next_plane = drm_output_prepare_cursor_view(state, ev);
>  
>   if (next_plane == NULL && !drm_view_is_opaque(ev))
>   next_plane = primary;
>  
> + if (next_plane == NULL && !planes_ok)
> + next_plane = primary;

This prevents promoting fullscreen surfaces to the primary plane when
sprites_are_broken, which it did not before.

> +
>   if (next_plane == NULL)
>   next_plane = drm_output_prepare_scanout_view(state, ev);
>  
>   if (next_plane == NULL)
>   next_plane = drm_output_prepare_overlay_view(state, ev);
>  
> - if (next_plane == NULL)
> - next_plane = primary;
> + if (!next_plane || next_plane == primary) {
> + if (!renderer_ok)
> + goto err;

This path is missing fini for clipped_view.

>  
> - if (next_plane == primary)
>   pixman_region32_union(_region,
> _region,
> _view);
> - else if (output->cursor_plane &&
> -  next_plane != >cursor_plane->base)
> + } else if (output->cursor_plane &&
> +next_plane != >cursor_plane->base) {
>   pixman_region32_union(_region,
> _region,
> _view);
> + }
>   pixman_region32_fini(_view);
>   }
>   pixman_region32_fini(_region);
>   pixman_region32_fini(_region);
>  
>   return state;
> +
> +err:
> + pixman_region32_fini(_region);
> + pixman_region32_fini(_region);
> + drm_output_state_free(state);
> + return NULL;
>  }

Re: [PATCH v14 34/41] compositor-drm: Ignore occluded views

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:51 +
Daniel Stone  wrote:

> When trying to assign planes, keep track of the areas which are
> already occluded, and ignore views which are completely occluded. This
> allows us to build a state using planes only, when there are occluded
> views which cannot go into a plane behind views which can.
> 
> Signed-off-by: Daniel Stone 
> ---
>  libweston/compositor-drm.c | 37 -
>  1 file changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 845f43e5b..71027a5ae 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -3054,7 +3054,7 @@ drm_output_propose_state(struct weston_output 
> *output_base,
>   struct drm_output *output = to_drm_output(output_base);
>   struct drm_output_state *state;
>   struct weston_view *ev;
> - pixman_region32_t surface_overlap, renderer_region;
> + pixman_region32_t surface_overlap, renderer_region, occluded_region;
>   struct weston_plane *primary = _base->compositor->primary_plane;
>  
>   assert(!output->state_last);
> @@ -3076,9 +3076,12 @@ drm_output_propose_state(struct weston_output 
> *output_base,
>* as we do for flipping full screen surfaces.
>*/
>   pixman_region32_init(_region);
> + pixman_region32_init(_region);
>  
>   wl_list_for_each(ev, _base->compositor->view_list, link) {
>   struct weston_plane *next_plane = NULL;
> + pixman_region32_t clipped_view;
> + bool occluded = false;
>  
>   /* If this view doesn't touch our output at all, there's no
>* reason to do anything with it. */
> @@ -3090,13 +3093,27 @@ drm_output_propose_state(struct weston_output 
> *output_base,
>   if (ev->output_mask != (1u << output->base.id))
>   next_plane = primary;
>  
> + /* Ignore views we know to be totally occluded. */
> + pixman_region32_init(_view);
> + pixman_region32_intersect(_view,
> +   >transform.boundingbox,
> +   >base.region);
> +
> + pixman_region32_init(_overlap);
> + pixman_region32_subtract(_overlap, _view,
> +  _region);
> + occluded = !pixman_region32_not_empty(_overlap);
> + if (occluded) {
> + pixman_region32_fini(_overlap);
> + pixman_region32_fini(_view);
> + continue;
> + }
> +
>   /* Since we process views from top to bottom, we know that if
>* the view intersects the calculated renderer region, it must
>* be part of, or occluded by, it, and cannot go on a plane. */
> - pixman_region32_init(_overlap);
>   pixman_region32_intersect(_overlap, _region,
> -   >transform.boundingbox);
> -
> +   _view);
>   if (pixman_region32_not_empty(_overlap))
>   next_plane = primary;
>   pixman_region32_fini(_overlap);
> @@ -3104,6 +3121,9 @@ drm_output_propose_state(struct weston_output 
> *output_base,
>   if (next_plane == NULL)
>   next_plane = drm_output_prepare_cursor_view(state, ev);
>  
> + if (next_plane == NULL && !drm_view_is_opaque(ev))
> + next_plane = primary;
> +
>   if (next_plane == NULL)
>   next_plane = drm_output_prepare_scanout_view(state, ev);
>  
> @@ -3116,9 +3136,16 @@ drm_output_propose_state(struct weston_output 
> *output_base,
>   if (next_plane == primary)
>   pixman_region32_union(_region,
> _region,
> -   >transform.boundingbox);
> +   _view);
> + else if (output->cursor_plane &&
> +  next_plane != >cursor_plane->base)

Should this not be:

if (!output->cursor_plane || next_plane != >cursor_plane->base)

so that lack of a cursor plane goes straight to occluded?

> + pixman_region32_union(_region,
> +   _region,
> +   _view);
> + pixman_region32_fini(_view);
>   }
>   pixman_region32_fini(_region);
> + pixman_region32_fini(_region);
>  
>   return state;
>  }

I cannot find any other fault here, and I stared at this for a long
time.


Thanks,
pq


pgpG17crgeJCa.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org

Re: [PATCH v14 33/41] compositor-drm: Ignore views on other outputs

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:50 +
Daniel Stone  wrote:

> When we come to assign_planes, try very hard to ignore views which are
> only visible on other outputs, rather than forcibly moving them to the
> primary plane, which causes damage all round and unnecessary repaints.
> 
> Signed-off-by: Daniel Stone 
> ---
>  libweston/compositor-drm.c | 23 +++
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 049c80932..845f43e5b 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -1389,10 +1389,6 @@ drm_fb_get_from_view(struct drm_output_state *state, 
> struct weston_view *ev)
>   struct linux_dmabuf_buffer *dmabuf;
>   struct drm_fb *fb;
>  
> - /* Don't import buffers which span multiple outputs. */
> - if (ev->output_mask != (1u << output->base.id))
> - return NULL;
> -
>   if (ev->alpha != 1.0f)
>   return NULL;
>  
> @@ -2925,10 +2921,6 @@ drm_output_prepare_cursor_view(struct drm_output_state 
> *output_state,
>   if (plane->state_cur->output && plane->state_cur->output != output)
>   return NULL;
>  
> - /* Don't import buffers which span multiple outputs. */
> - if (ev->output_mask != (1u << output->base.id))
> - return NULL;
> -
>   /* We use GBM to import SHM buffers. */
>   if (b->gbm == NULL)
>   return NULL;
> @@ -3088,6 +3080,16 @@ drm_output_propose_state(struct weston_output 
> *output_base,
>   wl_list_for_each(ev, _base->compositor->view_list, link) {
>   struct weston_plane *next_plane = NULL;
>  
> + /* If this view doesn't touch our output at all, there's no
> +  * reason to do anything with it. */
> + if (!(ev->output_mask & (1u << output->base.id)))
> + continue;
> +
> + /* We only assign planes to views which are exclusively present
> +  * on our output. */
> + if (ev->output_mask != (1u << output->base.id))
> + next_plane = primary;
> +
>   /* Since we process views from top to bottom, we know that if
>* the view intersects the calculated renderer region, it must
>* be part of, or occluded by, it, and cannot go on a plane. */
> @@ -3137,6 +3139,11 @@ drm_assign_planes(struct weston_output *output_base, 
> void *repaint_data)
>   wl_list_for_each(ev, _base->compositor->view_list, link) {
>   struct drm_plane *target_plane = NULL;
>  
> + /* If this view doesn't touch our output at all, there's no
> +  * reason to do anything with it. */
> + if (!(ev->output_mask & (1u << output->base.id)))
> + continue;
> +
>   /* Test whether this buffer can ever go into a plane:
>* non-shm, or small enough to be a cursor.
>*

Ooh, nice find!

Reviewed-by: Pekka Paalanen 


Thanks,
pq


pgpmVt0i4x8xw.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH v14 32/41] compositor-drm: Split drm_assign_planes in two

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:49 +
Daniel Stone  wrote:

> Move drm_assign_planes into two functions: one which proposes a plane
> configuration, and another which applies that state to the Weston
> internal structures. This will be used to try multiple configurations
> and see which is supported.
> 
> Signed-off-by: Daniel Stone 
> ---
>  libweston/compositor-drm.c | 109 
> ++---
>  1 file changed, 74 insertions(+), 35 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 3ff06c01c..049c80932 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -357,6 +357,8 @@ struct drm_plane_state {
>  
>   struct drm_fb *fb;
>  
> + struct weston_view *ev; /**< maintained for drm_assign_planes only */
> +
>   int32_t src_x, src_y;
>   uint32_t src_w, src_h;
>   int32_t dest_x, dest_y;

> +static void
> +drm_assign_planes(struct weston_output *output_base, void *repaint_data)
> +{
> + struct drm_backend *b = to_drm_backend(output_base->compositor);
> + struct drm_pending_state *pending_state = repaint_data;
> + struct drm_output *output = to_drm_output(output_base);
> + struct drm_output_state *state;
> + struct drm_plane_state *plane_state;
> + struct weston_view *ev;
> + struct weston_plane *primary = _base->compositor->primary_plane;
> +
> + state = drm_output_propose_state(output_base, pending_state);
>  
> - if (next_plane == primary ||
> - (output->cursor_plane &&
> -  next_plane == >cursor_plane->base)) {
> - /* cursor plane involves a copy */
> + wl_list_for_each(ev, _base->compositor->view_list, link) {
> + struct drm_plane *target_plane = NULL;
> +
> + /* Test whether this buffer can ever go into a plane:
> +  * non-shm, or small enough to be a cursor.
> +  *
> +  * Also, keep a reference when using the pixman renderer.
> +  * That makes it possible to do a seamless switch to the GL
> +  * renderer and since the pixman renderer keeps a reference
> +  * to the buffer anyway, there is no side effects.
> +  */
> + if (b->use_pixman ||
> + (ev->surface->buffer_ref.buffer &&
> + 
> (!wl_shm_buffer_get(ev->surface->buffer_ref.buffer->resource) ||
> +  (ev->surface->width <= b->cursor_width &&
> +   ev->surface->height <= b->cursor_height
> + ev->surface->keep_buffer = true;
> + else
> + ev->surface->keep_buffer = false;
> +
> + /* This is a bit unpleasant, but lacking a temporary place to
> +  * hang a plane off the view, we have to do a nested walk.
> +  * Our first-order iteration has to be planes rather than
> +  * views, because otherwise we won't reset views which were
> +  * previously on planes to being on the primary plane. */
> + wl_list_for_each(plane_state, >plane_list, link) {

I believe the plane list will be short enough for the foreseeable
future to not warrant a more complicated solution. One option would be
to hang data off the view destroy signal, but then looking that data up
is probably in the same order of magnitude than this trivial loop.

> + if (plane_state->ev == ev) {
> + plane_state->ev = NULL;
> + target_plane = plane_state->plane;
> + break;
> + }
> + }
> +
> + if (target_plane)
> + weston_view_move_to_plane(ev, _plane->base);
> + else
> + weston_view_move_to_plane(ev, primary);
> +
> + if (!target_plane ||
> + target_plane->type == WDRM_PLANE_TYPE_CURSOR) {
> + /* cursor plane & renderer involve a copy */
>   ev->psf_flags = 0;
>   } else {

Reviewed-by: Pekka Paalanen 


Thanks,
pq


pgpAhVSvyHvqX.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH v14 31/41] compositor-drm: Rename region variable

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:48 +
Daniel Stone  wrote:

> Make it a bit more clear what the purpose of the variable is.
> 
> Signed-off-by: Daniel Stone 
> ---
>  libweston/compositor-drm.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 19aeb5326..3ff06c01c 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -3059,7 +3059,7 @@ drm_assign_planes(struct weston_output *output_base, 
> void *repaint_data)
>   struct drm_output_state *state;
>   struct drm_plane_state *plane_state;
>   struct weston_view *ev;
> - pixman_region32_t overlap, surface_overlap;
> + pixman_region32_t surface_overlap, renderer_region;
>   struct weston_plane *primary, *next_plane;
>  
>   assert(!output->state_last);
> @@ -3080,7 +3080,7 @@ drm_assign_planes(struct weston_output *output_base, 
> void *repaint_data)
>* the client buffer can be used directly for the sprite surface
>* as we do for flipping full screen surfaces.
>*/
> - pixman_region32_init();
> + pixman_region32_init(_region);
>   primary = _base->compositor->primary_plane;
>  
>   wl_list_for_each(ev, _base->compositor->view_list, link) {
> @@ -3104,7 +3104,7 @@ drm_assign_planes(struct weston_output *output_base, 
> void *repaint_data)
>   es->keep_buffer = false;
>  
>   pixman_region32_init(_overlap);
> - pixman_region32_intersect(_overlap, ,
> + pixman_region32_intersect(_overlap, _region,
> >transform.boundingbox);
>  
>   next_plane = NULL;
> @@ -3125,7 +3125,8 @@ drm_assign_planes(struct weston_output *output_base, 
> void *repaint_data)
>   weston_view_move_to_plane(ev, next_plane);
>  
>   if (next_plane == primary)
> - pixman_region32_union(, ,
> + pixman_region32_union(_region,
> +   _region,
> >transform.boundingbox);
>  
>   if (next_plane == primary ||
> @@ -3142,7 +3143,7 @@ drm_assign_planes(struct weston_output *output_base, 
> void *repaint_data)
>  
>   pixman_region32_fini(_overlap);
>   }
> - pixman_region32_fini();
> + pixman_region32_fini(_region);
>  
>   /* We rely on output->cursor_view being both an accurate reflection of
>* the cursor plane's state, but also being maintained across repaints

Hi,

let me see... renderer_region starts empty, and accumulates the area
from all views assigned to the primary plane for composition. Iterating
the view list from top to bottom, if a view may overlap with
renderer_region at that point, it cannot be promoted to an overlay
plane, because the primary plane already has something on top of it.

Reviewed-by: Pekka Paalanen 


Thanks,
pq


pgpX835aJ7TwD.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH v14 30/41] compositor-drm: Don't need safe view-list traversal

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:47 +
Daniel Stone  wrote:

> Nothing in this loop reorders views within the compositor's view_list.
> 
> Signed-off-by: Daniel Stone 
> ---
>  libweston/compositor-drm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index b030234e4..19aeb5326 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -3058,7 +3058,7 @@ drm_assign_planes(struct weston_output *output_base, 
> void *repaint_data)
>   struct drm_output *output = to_drm_output(output_base);
>   struct drm_output_state *state;
>   struct drm_plane_state *plane_state;
> - struct weston_view *ev, *next;
> + struct weston_view *ev;
>   pixman_region32_t overlap, surface_overlap;
>   struct weston_plane *primary, *next_plane;
>  
> @@ -3083,7 +3083,7 @@ drm_assign_planes(struct weston_output *output_base, 
> void *repaint_data)
>   pixman_region32_init();
>   primary = _base->compositor->primary_plane;
>  
> - wl_list_for_each_safe(ev, next, _base->compositor->view_list, 
> link) {
> + wl_list_for_each(ev, _base->compositor->view_list, link) {
>   struct weston_surface *es = ev->surface;
>  
>   /* Test whether this buffer can ever go into a plane:

Reviewed-by: Pekka Paalanen 


Thanks,
pq


pgpSHQJLkpoCe.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH v14 29/41] compositor-drm: Add modifiers to GBM dmabuf import

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:46 +
Daniel Stone  wrote:

> Add support for the GBM_BO_IMPORT_FD_MODIFIER path, which allows us to
> import multi-plane dmabufs, as well as format modifiers.
> 
> Signed-off-by: Daniel Stone 
> ---
>  configure.ac   |   6 +-
>  libweston/compositor-drm.c | 181 
> +
>  2 files changed, 137 insertions(+), 50 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 1f3cc28aa..8d0d6582a 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -212,9 +212,9 @@ if test x$enable_drm_compositor = xyes; then
>PKG_CHECK_MODULES(DRM_COMPOSITOR_ATOMIC, [libdrm >= 2.4.78],
>   [AC_DEFINE([HAVE_DRM_ATOMIC], 1, [libdrm supports atomic 
> API])],
>   [AC_MSG_WARN([libdrm does not support atomic modesetting, 
> will omit that capability])])
> -  PKG_CHECK_MODULES(DRM_COMPOSITOR_GBM, [gbm >= 10.2],
> - [AC_DEFINE([HAVE_GBM_FD_IMPORT], 1, [gbm supports dmabuf 
> import])],
> - [AC_MSG_WARN([gbm does not support dmabuf import, will omit 
> that capability])])
> +  PKG_CHECK_MODULES(DRM_COMPOSITOR_GBM, [gbm >= 17.2],
> + [AC_DEFINE([HAVE_GBM_FD_IMPORT], 1, [gbm supports import 
> with modifiers])],
> + [AC_MSG_WARN([GBM does not support dmabuf import, will omit 
> that capability])])
>  fi
>  
>  
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 713bbabdd..b030234e4 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -278,6 +278,7 @@ struct drm_mode {
>  enum drm_fb_type {
>   BUFFER_INVALID = 0, /**< never used */
>   BUFFER_CLIENT, /**< directly sourced from client */
> + BUFFER_DMABUF, /**< imported from linux_dmabuf client */
>   BUFFER_PIXMAN_DUMB, /**< internal Pixman rendering */
>   BUFFER_GBM_SURFACE, /**< internal EGL rendering */
>   BUFFER_CURSOR, /**< internal cursor buffer */
> @@ -971,6 +972,120 @@ drm_fb_ref(struct drm_fb *fb)
>   return fb;
>  }
>  
> +static void
> +drm_fb_destroy_dmabuf(struct drm_fb *fb)
> +{
> + /* We deliberately do not close the GEM handles here; GBM manages
> +  * their lifetime through the BO. */
> + gbm_bo_destroy(fb->bo);
> + drm_fb_destroy(fb);
> +}
> +
> +static struct drm_fb *
> +drm_fb_get_from_dmabuf(struct linux_dmabuf_buffer *dmabuf,
> +struct drm_backend *backend, bool is_opaque)
> +{
> +#ifdef HAVE_GBM_FD_IMPORT
> + struct drm_fb *fb;
> + struct gbm_import_fd_data import_legacy = {
> + .width = dmabuf->attributes.width,
> + .height = dmabuf->attributes.height,
> + .format = dmabuf->attributes.format,
> + .stride = dmabuf->attributes.stride[0],
> + .fd = dmabuf->attributes.fd[0],
> + };
> + struct gbm_import_fd_modifier_data import_mod = {
> + .width = dmabuf->attributes.width,
> + .height = dmabuf->attributes.height,
> + .format = dmabuf->attributes.format,
> + .num_fds = dmabuf->attributes.n_planes,
> + .modifier = dmabuf->attributes.modifier[0],
> + };
> + int i;
> +
> +/* XXX: TODO:
> + *
> + * Currently the buffer is rejected if any dmabuf attribute
> + * flag is set.  This keeps us from passing an inverted /
> + * interlaced / bottom-first buffer (or any other type that may
> + * be added in the future) through to an overlay.  Ultimately,
> + * these types of buffers should be handled through buffer
> + * transforms and not as spot-checks requiring specific
> + * knowledge. */
> + if (dmabuf->attributes.flags)
> + return NULL;
> +
> + fb = zalloc(sizeof *fb);
> + if (fb == NULL)
> + return NULL;
> +
> + fb->refcnt = 1;
> + fb->type = BUFFER_DMABUF;
> +
> + memcpy(import_mod.fds, dmabuf->attributes.fd, sizeof(import_mod.fds));

Ok.

> + memcpy(import_mod.strides, dmabuf->attributes.stride,
> +sizeof(import_mod.fds));

sizeof argument incorrect

type mismatch:
int import_mod.strides
uint32_t dmabuf->attributes.stride

> + memcpy(import_mod.offsets, dmabuf->attributes.offset,
> +sizeof(import_mod.fds));

sizeof argument incorrect

type mismatch:
int import_mod.offsets
uint32_t dmabuf->attributes.offset

Btw. struct gbm_import_fd_data uses uint32_t, unlike struct
gbm_import_fd_modifier_data. Why did the new struct switch to signed
values? Does GBM actually do something with negative offset or stride?

> +
> + if (dmabuf->attributes.modifier[0] != DRM_FORMAT_MOD_INVALID) {
> + fb->bo = gbm_bo_import(backend->gbm, GBM_BO_IMPORT_FD_MODIFIER,
> +_mod,
> +GBM_BO_USE_SCANOUT);
> + } else {
> + 

Re: [PATCH v14 28/41] compositor-drm: Support modifiers for drm_fb

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:45 +
Daniel Stone  wrote:

> Use the new drmModeAddFB2WithModifiers interface to import buffers with
> modifiers.
> 
> Signed-off-by: Daniel Stone 
> ---
>  configure.ac   |  3 +++
>  libweston/compositor-drm.c | 26 +-
>  2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index ba9247773..1f3cc28aa 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -206,6 +206,9 @@ AM_CONDITIONAL(ENABLE_DRM_COMPOSITOR, test 
> x$enable_drm_compositor = xyes)
>  if test x$enable_drm_compositor = xyes; then
>AC_DEFINE([BUILD_DRM_COMPOSITOR], [1], [Build the DRM compositor])
>PKG_CHECK_MODULES(DRM_COMPOSITOR, [libudev >= 136 libdrm >= 2.4.30 gbm])
> +  PKG_CHECK_MODULES(DRM_COMPOSITOR_MODIFIERS, [libdrm >= 2.4.71],
> + [AC_DEFINE([HAVE_DRM_ADDFB2_MODIFIERS], 1, [libdrm supports 
> modifiers])],
> + [AC_MSG_WARN([libdrm does not support AddFB2 with 
> modifiers])])
>PKG_CHECK_MODULES(DRM_COMPOSITOR_ATOMIC, [libdrm >= 2.4.78],
>   [AC_DEFINE([HAVE_DRM_ATOMIC], 1, [libdrm supports atomic 
> API])],
>   [AC_MSG_WARN([libdrm does not support atomic modesetting, 
> will omit that capability])])
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 09fa10f5f..713bbabdd 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -293,6 +293,7 @@ struct drm_fb {
>   uint32_t strides[4];
>   uint32_t offsets[4];
>   const struct pixel_format_info *format;
> + uint64_t modifier;
>   int width, height;
>   int fd;
>   struct weston_buffer_reference buffer_ref;
> @@ -843,7 +844,28 @@ drm_fb_destroy_gbm(struct gbm_bo *bo, void *data)
>  static int
>  drm_fb_addfb(struct drm_fb *fb)
>  {
> - int ret;
> + int ret = -EINVAL;
> +#ifdef HAVE_DRM_ADDFB2_MODIFIERS
> + uint64_t mods[4] = { };
> + int i;
> +#endif
> +
> + /* If we have a modifier set, we must only use the WithModifiers
> +  * entrypoint; we cannot import it through legacy ioctls. */
> + if (fb->modifier != DRM_FORMAT_MOD_INVALID) {
> + /* KMS demands that if a modifier is set, it must be the same
> +  * for all planes. */
> +#ifdef HAVE_DRM_ADDFB2_MODIFIERS
> + for (i = 0; fb->handles[i]; i++)

This will overflow if all four planes are set.

> + mods[i] = fb->modifier;
> + ret = drmModeAddFB2WithModifiers(fb->fd, fb->width, fb->height,
> +  fb->format->format,
> +  fb->handles, fb->strides,
> +  fb->offsets, mods, >fb_id,
> +  DRM_MODE_FB_MODIFIERS);
> +#endif
> + return ret;
> + }
>  
>   ret = drmModeAddFB2(fb->fd, fb->width, fb->height, fb->format->format,
>   fb->handles, fb->strides, fb->offsets, >fb_id,
> @@ -905,6 +927,7 @@ drm_fb_create_dumb(struct drm_backend *b, int width, int 
> height,
>   goto err_fb;
>  
>   fb->type = BUFFER_PIXMAN_DUMB;
> + fb->modifier = DRM_FORMAT_MOD_INVALID;
>   fb->handles[0] = create_arg.handle;
>   fb->strides[0] = create_arg.pitch;
>   fb->size = create_arg.size;
> @@ -972,6 +995,7 @@ drm_fb_get_from_bo(struct gbm_bo *bo, struct drm_backend 
> *backend,
>   fb->strides[0] = gbm_bo_get_stride(bo);
>   fb->handles[0] = gbm_bo_get_handle(bo).u32;
>   fb->format = pixel_format_get_info(gbm_bo_get_format(bo));
> + fb->modifier = DRM_FORMAT_MOD_INVALID;
>   fb->size = fb->strides[0] * fb->height;
>   fb->fd = backend->drm.fd;
>  

Otherwise looks good.


Thanks,
pq


pgpRy_MYqKIwu.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH v14 27/41] compositor-drm: Extract drm_fb_addfb into a helper

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:44 +
Daniel Stone  wrote:

> We currently do the same thing in two places, and will soon have a
> third.
> 
> Signed-off-by: Daniel Stone 
> ---
>  libweston/compositor-drm.c | 93 
> --
>  1 file changed, 48 insertions(+), 45 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index e6b5efba0..09fa10f5f 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -288,7 +288,10 @@ struct drm_fb {
>  
>   int refcnt;
>  
> - uint32_t fb_id, stride, handle, size;
> + uint32_t fb_id, size;
> + uint32_t handles[4];
> + uint32_t strides[4];
> + uint32_t offsets[4];
>   const struct pixel_format_info *format;
>   int width, height;
>   int fd;
> @@ -821,7 +824,7 @@ drm_fb_destroy_dumb(struct drm_fb *fb)
>   munmap(fb->map, fb->size);
>  
>   memset(_arg, 0, sizeof(destroy_arg));
> - destroy_arg.handle = fb->handle;
> + destroy_arg.handle = fb->handles[0];
>   drmIoctl(fb->fd, DRM_IOCTL_MODE_DESTROY_DUMB, _arg);
>  
>   drm_fb_destroy(fb);
> @@ -837,6 +840,32 @@ drm_fb_destroy_gbm(struct gbm_bo *bo, void *data)
>   drm_fb_destroy(fb);
>  }
>  
> +static int
> +drm_fb_addfb(struct drm_fb *fb)
> +{
> + int ret;
> +
> + ret = drmModeAddFB2(fb->fd, fb->width, fb->height, fb->format->format,
> + fb->handles, fb->strides, fb->offsets, >fb_id,
> + 0);
> + if (ret == 0)
> + return 0;
> +
> + /* Legacy AddFB can't always infer the format from depth/bpp alone, so
> +  * check if our format is one of the lucky ones. */
> + if (!fb->format->depth || !fb->format->bpp)
> + return ret;
> +
> + /* Cannot fall back to AddFB for multi-planar formats either. */
> + if (fb->handles[1] || fb->handles[2] || fb->handles[3])
> + return ret;
> +
> + ret = drmModeAddFB(fb->fd, fb->width, fb->height,
> +fb->format->depth, fb->format->bpp,
> +fb->strides[0], fb->handles[0], >fb_id);
> + return ret;
> +}
> +

Reviewed-by: Pekka Paalanen 


Thanks,
pq


pgpbrYOJ2CYDh.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] RFC: libweston/compositor-drm: Add support for drm-lease.

[Pekka Paalanen]

On Thu, 25 Jan 2018 19:07:43 +
Marius-cristian Vlad  wrote:

> -Original Message-
> From: Pekka Paalanen [mailto:pekka.paala...@collabora.co.uk] 
> Sent: Thursday, January 25, 2018 2:06 PM
> To: Daniel Stone 
> Cc: Marius-cristian Vlad ; Keith Packard 
> ; wayland-devel@lists.freedesktop.org
> Subject: Re: [PATCH weston] RFC: libweston/compositor-drm: Add support for 
> drm-lease.
> 
> On Thu, 25 Jan 2018 11:33:34 +
> Daniel Stone  wrote:
> 
> > Hi Marius,
> > 
> > On 25 January 2018 at 11:10, Marius-cristian Vlad 
> >  wrote:  
> 
> > > >> +   wl_signal_emit(>wake_signal, 
> > > >> compositor);
> > > >> +   
> > > >> + wl_event_source_timer_update(compositor->idle_source,
> > > >> +
> > > >> + compositor->idle_time * 1000);  
> > > >
> > > > I assume this is just to force a repaint. If the existing 
> > > > compositor API doesn't quite work for this, we should create API 
> > > > which does, or make sure enabling the output does the right thing. 
> > > > Are you using desktop-shell, or ... ?  
> > >
> > > [mvlad] Indeed. What I've observed is that it could be some time 
> > > until the repaint fires and in that time the fb of the client can 
> > > still be present on that output. Forcing a repaint seems to fix 
> > > that. There's also a longer explanation: If the client destroyes the 
> > > fb this would cause the connector to be disabled. If weston can 
> > > reclaim the connector after it has been disabled there's no issue.
> > > I will need to check this once more, it might not be needed after 
> > > all.  
> > 
> > Right. If we create/enable a new Weston output, this should result in 
> > repaint happening by itself: just like it does with hotplug now.  
> 
> Still, should we have the client wait for the compositor to have
> actually posted a repaint of the output before the client destroys
> its fb?
> 
> [mvlad] I would assume that a client will detroy its fb and,
> afterwards will revoke the lease. You won't be able the access the
> leased fd after revoking it. Weston will not be unable to repaint
> anything if there's currently no ouput to repaint on. I hope I
> understand your question correctly :/.  

Hi,

we can specify the protocol any way we want. If we specify that clients
must follow a certain sequence, we can expect them to follow that
sequence.

Sending the "release the lease" request, i.e. destroying the Wayland
protocol object representing the lease, is independent of actually
closing the DRM fd. If the leased DRM fd is revoked by the server,
would it mean that the lessee can no longer use the still open fd to
destroy FBs?

But, there must be a clean-up mechanism in the kernel as well, so maybe
there is no need for the lessee to explicitly destroy the last FB as
closing the DRM fd and subsequently the server setting its own FB to
the CRTC should cause the now orphaned lessee FB to be
garbage-collected.

For client initiated release, we could require the sequence: client
sends the release the lease request, client waits for the server to
process the release (produces an event), only then client closes the
DRM fd. The client would never rmfb the last FB.

My question is: is there any need for that? Would it make sense or not?

> 
> Do I understand right that the client destroying the fb would cause
> the CRTC and connector to be turned off immediately? Do we want to
> avoid that flicker if Weston is to take that output back into use?
> 
> [mvlad] Yes that is correct. RMFB will lead the CRTC and connector to
> be turned off. If there's no FB present the helpers in DRM atomic
> commit part will disable that CRTC.  

Right.

> That brings to my mind the opposite question: if weston stops using
> an output so that it can lease it out, how's the flicker avoidance in
> that case?
> 
> [mvlad] There seems to be no flicker when destroying the output with
> drm_output_destroy. This is rather blunt, but that's what I see
> happening. 

I would assume that's just a race, or a side-effect of the delayed
drm_output destruction as Daniel explained. I presume that in fact the
CRTC is still running with the server FB when the lessee sets its own.
I would expect there to be flicker if you actually guaranteed the
server's FB has been destroyed and the destruction handled by the
kernel (cross a vblank), before giving the lease to the client.

I believe one should guarantee that either the server FB stays up until
the client has taken over, or the server FB has been removed before the
client takes over.

> What about leaking fb contents between the lessor and lessee?
> 
> [mvlad] The client has its own fbs. What could happen is that the
> ouput "shared" by lessor/lessee can have inter-leaved fbs if the
> lesor is still using the output, but I see that happening only on
> purpose. 

I was thinking about