Re: [PATCH weston 6/6] libweston: Implement touch timestamps for input_timestamps_unstable_v1
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, > > + &zwp_input_timestamps_v1_interface, > > + 1, id); > > + if (!input_ts) { > > + wl_client_post_no_memory(client); > > + return; > > + } > > + > > + wl_resource_set_implementation(input_ts, > > + &input_timestamps_interface, > > + touch_resource, > > + unbind_resource); > > + > > + wl_list_insert(&seat->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
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(&test->seat, test->compositor, "test-seat"); + test->is_seat_initialized = true; + + /* add devices */ + weston_seat_init_pointer(&test->seat); + if (weston_seat_init_keyboard(&test->seat, NULL) < 0) + return -1; + weston_seat_init_touch(&test->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(&test->seat, ec, "test-seat"); - - /* add devices */ - weston_seat_init_pointer(&test->seat); - if (weston_seat_init_keyboard(&test->seat, NULL) < 0) + if (test_seat_init(test) == -1) return -1; - weston_seat_init_touch(&test->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
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
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(&inp->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, &wl_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, &client->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, &client->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(&global->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
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
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(&inp->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, &client->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, &wl_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, &wl_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(&inp->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, &cl->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(&cl->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 --- a/tests/weston-tes
[PATCH weston 1/8] libweston: Make weston_pointer destruction safe
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, &pointer_client->pointer_resources) { + wl_resource_set_user_data(resource, NULL); + } + + wl_resource_for_each(resource, +&pointer_client->relative_pointer_resources) { + wl_resource_set_user_data(resource, NULL); + } + + wl_list_remove(&pointer_client->pointer_resources); + wl_list_remove(&pointer_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(&pointer->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->pointer_clients, + link) { + wl_list_remove(&pointer_client->link); + weston_pointer_client_destroy(pointer_client); + } wl_list_remove(&pointer->focus_resource_listener.link); wl_list_remove(&pointer->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, &zwp_locked_pointer_v1_interface, &locked_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, &zwp_confined_pointer_v1_interface, &confined_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
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, &touch->resource_list) { + wl_resource_set_user_data(resource, NULL); + } + + wl_resource_for_each(resource, &touch->focus_resource_list) { + wl_resource_set_user_data(resource, NULL); + } + wl_list_remove(&touch->resource_list); + wl_list_remove(&touch->focus_resource_list); wl_list_remove(&touch->focus_view_listener.link); wl_list_remove(&touch->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, &touch_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
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, &seat->base_resource_list) { + wl_resource_set_user_data(resource, NULL); + } + + wl_resource_for_each(resource, &seat->drag_resource_list) { + wl_resource_set_user_data(resource, NULL); + } + + wl_list_remove(&seat->base_resource_list); + wl_list_remove(&seat->drag_resource_list); + wl_list_remove(&seat->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
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, &keyboard->resource_list) { + wl_resource_set_user_data(resource, NULL); + } + + wl_resource_for_each(resource, &keyboard->focus_resource_list) { + wl_resource_set_user_data(resource, NULL); + } + + wl_list_remove(&keyboard->resource_list); + wl_list_remove(&keyboard->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(&keyboard->resource_list, wl_resource_get_link(cr)); wl_resource_set_implementation(cr, &keyboard_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
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 mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH weston 4/4] ivi-shell: fix the layer assignment change from one screen to another
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(&addlayer->pending.link); wl_list_insert(&iviscrn->pending.layer_list, &addlayer->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
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->layout_layer.view_list, &ivi_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 2/4] ivi-shell: don't schedule compositor repaint
it is not necessary to repaint all outputs after each commit_changes. Only outputs with modified views has to be repainted. We need to call weston_view_update_transform for assigning views to outputs first. Then, We can call weston_view_schedule_repaint to trigger repaint for outputs. Signed-off-by: Emre Ucan --- ivi-shell/ivi-layout.c | 13 ++--- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c index 394179b..3c52ce1 100644 --- a/ivi-shell/ivi-layout.c +++ b/ivi-shell/ivi-layout.c @@ -27,7 +27,7 @@ * Implementation of ivi-layout library. The actual view on ivi_screen is * not updated until ivi_layout_commit_changes is called. An overview from * calling API for updating properties of ivi_surface/ivi_layer to asking - * compositor to compose them by using weston_compositor_schedule_repaint, + * compositor to compose them by using weston_view_schedule_repaint, * 0/ initialize this library by ivi_layout_init_with_compositor *with (struct weston_compositor *ec) from ivi-shell. * 1/ When an API for updating properties of ivi_surface/ivi_layer, it updates @@ -51,8 +51,8 @@ * 4/ According properties, set transformation by using weston_matrix and *weston_view per ivi_surfaces and ivi_layers in while loop. * 5/ Set damage and trigger transform by using weston_view_geometry_dirty. - * 6/ Notify update of properties. - * 7/ Trigger composition by weston_compositor_schedule_repaint. + * 6/ Schedule repaint for each view by using weston_view_schedule_repaint. + * 7/ Notify update of properties. * */ #include "config.h" @@ -577,13 +577,13 @@ update_prop(struct ivi_layout_view *ivi_view) &ivi_view->transform.link); weston_view_set_transform_parent(ivi_view->view, NULL); + weston_view_geometry_dirty(ivi_view->view); + weston_view_update_transform(ivi_view->view); } ivisurf->update_count++; - weston_view_geometry_dirty(ivi_view->view); - - weston_surface_damage(ivisurf->surface); + weston_view_schedule_repaint(ivi_view->view); } static void @@ -1752,7 +1752,6 @@ ivi_layout_commit_changes(void) commit_changes(layout); send_prop(layout); - weston_compositor_schedule_repaint(layout->compositor); return IVI_SUCCEEDED; } -- 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
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
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 = &output_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(&surface_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(&renderer_region, > &renderer_region, > &clipped_view); > - else if (output->cursor_plane && > - next_plane != &output->cursor_plane->base) > + } else if (output->cursor_plane && > +next_plane != &output->cursor_plane->base) { > pixman_region32_union(&occluded_region, > &occluded_region, > &clipped_view); > + } > pixman_region32_fini(&clipped_view); > } > pixman_region32_fini(&renderer_region); > pixman_region32_fini(&occluded_region); > > return state; > + > +err: > + pixman_region32_fini(&renderer_region); > + pixman_region32_fini(&oc
Re: [PATCH v14 34/41] compositor-drm: Ignore occluded views
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 = &output_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(&renderer_region); > + pixman_region32_init(&occluded_region); > > wl_list_for_each(ev, &output_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(&clipped_view); > + pixman_region32_intersect(&clipped_view, > + &ev->transform.boundingbox, > + &output->base.region); > + > + pixman_region32_init(&surface_overlap); > + pixman_region32_subtract(&surface_overlap, &clipped_view, > + &occluded_region); > + occluded = !pixman_region32_not_empty(&surface_overlap); > + if (occluded) { > + pixman_region32_fini(&surface_overlap); > + pixman_region32_fini(&clipped_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(&surface_overlap); > pixman_region32_intersect(&surface_overlap, &renderer_region, > - &ev->transform.boundingbox); > - > + &clipped_view); > if (pixman_region32_not_empty(&surface_overlap)) > next_plane = primary; > pixman_region32_fini(&surface_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(&renderer_region, > &renderer_region, > - &ev->transform.boundingbox); > + &clipped_view); > + else if (output->cursor_plane && > + next_plane != &output->cursor_plane->base) Should this not be: if (!output->cursor_plane || next_plane != &output->cursor_plane->base) so that lack of a cursor plane goes straight to occluded? > + pixman_region32_union(&occluded_region, > + &occluded_region, > + &clipped_view); > + pixman_region32_fini(&clipped_view); > } > pixman_region32_fini(&renderer_region); > + pixman_region32_fini(&occluded_region); > > return state; > } I cannot find any other fault here, and I stared at this for a l
Re: [PATCH v14 33/41] compositor-drm: Ignore views on other outputs
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, &output_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, &output_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
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 = &output_base->compositor->primary_plane; > + > + state = drm_output_propose_state(output_base, pending_state); > > - if (next_plane == primary || > - (output->cursor_plane && > - next_plane == &output->cursor_plane->base)) { > - /* cursor plane involves a copy */ > + wl_list_for_each(ev, &output_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, &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, &target_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
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(&overlap); > + pixman_region32_init(&renderer_region); > primary = &output_base->compositor->primary_plane; > > wl_list_for_each(ev, &output_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(&surface_overlap); > - pixman_region32_intersect(&surface_overlap, &overlap, > + pixman_region32_intersect(&surface_overlap, &renderer_region, > &ev->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(&overlap, &overlap, > + pixman_region32_union(&renderer_region, > + &renderer_region, > &ev->transform.boundingbox); > > if (next_plane == primary || > @@ -3142,7 +3143,7 @@ drm_assign_planes(struct weston_output *output_base, > void *repaint_data) > > pixman_region32_fini(&surface_overlap); > } > - pixman_region32_fini(&overlap); > + pixman_region32_fini(&renderer_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
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(&overlap); > primary = &output_base->compositor->primary_plane; > > - wl_list_for_each_safe(ev, next, &output_base->compositor->view_list, > link) { > + wl_list_for_each(ev, &output_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
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, > +&import_mod, > +GBM_BO_USE_SCANOUT); > + } else { > + fb->bo = gbm_bo_import(backend->gbm, GBM_BO_
Re: [PATCH v14 28/41] compositor-drm: Support modifiers for drm_fb
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->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->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
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(&destroy_arg, 0, sizeof(destroy_arg)); > - destroy_arg.handle = fb->handle; > + destroy_arg.handle = fb->handles[0]; > drmIoctl(fb->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_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->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->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.
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(&compositor->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 read-back, not mixed-up display. That is, if there is an FB set up by process A, then CRTC is handed over to process B, then process B can q