Re: [PATCH] compositor-fbdev: Wait and retry before failing on reconnect to the framebuffer
Hi On Fri, May 15, 2015 at 8:39 AM, Pekka Paalanen ppaala...@gmail.com wrote: On Wed, 13 May 2015 21:43:39 -0400 nerdopolis bluescreen_aven...@verizon.net wrote: Resolving https://bugs.freedesktop.org/show_bug.cgi?id=73782 udev might be configured to set the permissions on framebuffer devices with the UACCESS attribute. Weston currently attempts to reconnect to the framebuffer device before udev can set the permissions back. It waits 3 times in case if the system is heavily paging, or slowed down, and prevents udev from working. In my testing the delay is long enough where it works on the first try --- src/compositor-fbdev.c | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c index 7c505ce..42d2881 100644 --- a/src/compositor-fbdev.c +++ b/src/compositor-fbdev.c @@ -663,13 +663,23 @@ fbdev_output_reenable(struct fbdev_compositor *compositor, struct fbdev_output *output = to_fbdev_output(base); struct fbdev_screeninfo new_screen_info; int fb_fd; + int retries; const char *device; weston_log(Re-enabling fbdev output.\n); /* Create the frame buffer. */ - fb_fd = fbdev_frame_buffer_open(output, output-device, + fb_fd = -1; + retries = 0; + while (fb_fd 0 retries 3) { + usleep(1); + fb_fd = fbdev_frame_buffer_open(output, output-device, new_screen_info); + if (fb_fd 0) { + weston_log(Creating frame buffer failed. Retrying...\n); + } + retries++; + } if (fb_fd 0) { weston_log(Creating frame buffer failed.\n); goto err; Hi, I hate sleeps. I'd really want an explanation in the commit message on why this cannot be fixed properly at all, and you need a delay to have it work. It is possible there is no better way, but I would like to understand why first. For instance, should reenable() be waiting for some sort of udev advertisement of the fb device? Is there any event from udev to signal this? Do you have systemd involved in your use case? Could this be solved with systemd or logind? Or should we let weston-launch open the fb device instead? I'm CC'ing David, maybe he might have an idea? systemd-logind listens to VT events via poll(2) on /sys/class/tty/tty0/active. On VT switches it changes ACL permissions on dev-nodes underneath /dev, if the 'uaccess' tag is set on a device. But it cannot delay a VT-switch, it just reacts to it. It is inherent to this approach, that the permissions are set _after_ the VT switch. Therefore, there's a race and I guess it's what is hit here. However, systemd-logind sends out dbus signals after it fully changed the permissions. Hence, if you rely on the 'uaccess' functionality by systemd, then you better also use the systemd APIs (either sd_login_monitor_new() or DBus). If you don't want to use the systemd APIs, then you cannot rely on 'uaccess' (i.e., you have to set static group/user permissions on your device nodes; it will not interfere with 'uaccess', logind allows both in parallel). Long story short: Please run weston directly (_without_ weston-launch, instead directly invoking weston from within a logged-in VT), with systemd support compiled it. It should work fine with 'uaccess'. If you don't have systemd support compiled in, please use static access permissions. Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] compositor-fbdev: Wait and retry before failing on reconnect to the framebuffer
Hi On Fri, May 15, 2015 at 4:44 PM, Derek Foreman der...@osg.samsung.com wrote: On 15/05/15 05:30 AM, David Herrmann wrote: systemd-logind listens to VT events via poll(2) on /sys/class/tty/tty0/active. On VT switches it changes ACL permissions on dev-nodes underneath /dev, if the 'uaccess' tag is set on a device. But it cannot delay a VT-switch, it just reacts to it. It is inherent to this approach, that the permissions are set _after_ the VT switch. Therefore, there's a race and I guess it's what is hit here. However, systemd-logind sends out dbus signals after it fully changed the permissions. Hence, if you rely on the 'uaccess' functionality by systemd, then you better also use the systemd APIs (either sd_login_monitor_new() or DBus). If you don't want to use the systemd APIs, then you cannot rely on 'uaccess' (i.e., you have to set static group/user permissions on your device nodes; it will not interfere with 'uaccess', logind allows both in parallel). Long story short: Please run weston directly (_without_ weston-launch, instead directly invoking weston from within a logged-in VT), with systemd support compiled it. It should work fine with 'uaccess'. If you don't have systemd support compiled in, please use static access permissions. Does this mean weston-launch is always the wrong thing to do if systemd support is compiled in? If systemd is running, weston-launch should probably execve() 'weston' directly without setting up WESTON_LAUNCHER_FD. ie) should we refuse to build weston-launch if systemd support is enabled? This should be a run-time detection, not build-time, imho. Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] Set O_CLOEXEC when opening devices
Hi On Thu, Apr 30, 2015 at 6:43 PM, Derek Foreman der...@osg.samsung.com wrote: We'd rather keep these out of the hands of children. Signed-off-by: Derek Foreman der...@osg.samsung.com --- src/evdev.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) Reviewed-by: David Herrmann dh.herrm...@gmail.com Thanks David diff --git a/src/evdev.c b/src/evdev.c index d997d24..af36127 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -1953,7 +1953,8 @@ evdev_device_create(struct libinput_seat *seat, /* Use non-blocking mode so that we can loop on read on * evdev_device_data() until all events on the fd are * read. mtdev_get() also expects this. */ - fd = open_restricted(libinput, devnode, O_RDWR | O_NONBLOCK); + fd = open_restricted(libinput, devnode, +O_RDWR | O_NONBLOCK | O_CLOEXEC); if (fd 0) { log_info(libinput, opening input device '%s' failed (%s).\n, @@ -2436,7 +2437,8 @@ evdev_device_resume(struct evdev_device *device) return -ENODEV; devnode = udev_device_get_devnode(device-udev_device); - fd = open_restricted(libinput, devnode, O_RDWR | O_NONBLOCK); + fd = open_restricted(libinput, devnode, +O_RDWR | O_NONBLOCK | O_CLOEXEC); if (fd 0) return -errno; -- 2.1.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 2/2] input: Set CLOEXEC on all opened fds
Hi On Thu, Apr 30, 2015 at 6:20 PM, Derek Foreman der...@osg.samsung.com wrote: We'd rather not have these fds available in all child processes, even if using old versions of libinput that don't bother to set CLOEXEC Signed-off-by: Derek Foreman der...@osg.samsung.com Reviewed-by: David Herrmann dh.herrm...@gmail.com Thanks David --- src/libinput-seat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libinput-seat.c b/src/libinput-seat.c index c0a87ea..6eae1a4 100644 --- a/src/libinput-seat.c +++ b/src/libinput-seat.c @@ -186,7 +186,7 @@ open_restricted(const char *path, int flags, void *user_data) struct udev_input *input = user_data; struct weston_launcher *launcher = input-compositor-launcher; - return weston_launcher_open(launcher, path, flags); + return weston_launcher_open(launcher, path, flags | O_CLOEXEC); } static void -- 2.1.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/2] logind: actually close fd in weston_logind_close()
Hi On Thu, Apr 30, 2015 at 6:20 PM, Derek Foreman der...@osg.samsung.com wrote: You had one job... Signed-off-by: Derek Foreman der...@osg.samsung.com --- src/logind-util.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/logind-util.c b/src/logind-util.c index e4e20eb..1327b48 100644 --- a/src/logind-util.c +++ b/src/logind-util.c @@ -248,6 +248,8 @@ weston_logind_close(struct weston_logind *wl, int fd) weston_logind_release_device(wl, major(st.st_rdev), minor(st.st_rdev)); + + close(fd); You should really close the FD in the error-paths above, too. Or just go to launcher-util.c, weston_launcher_close() and drop the return before weston_logind_close(). Thanks David } WL_EXPORT void -- 2.1.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] weston-launch: Fixed TTY switching
Hi On Wed, Apr 8, 2015 at 2:03 AM, Bryce Harrington br...@osg.samsung.com wrote: On Wed, Apr 01, 2015 at 08:10:44AM +0100, mateuszx.potr...@intel.com wrote: From: Mateusz Polrola mateuszx.potr...@intel.com After weston-launch is executing weston it cannot close TTY file, because it is still required to properly handle SIGUSR1 and SIGUSR2 signals that are used for switching TTY. Additionally after opening TTY it has to be activated, so that user don't have to manually switch to TTY used by weston first to be able to switch to other TTY. During shutdown TTY file has to be reopened, as device was already deinitialize by child process, but it is still required to cleanup VT handling and leave system in sane state. Signed-off-by: Mateusz Polrola mateuszx.potr...@intel.com --- src/weston-launch.c | 20 +--- 1 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/weston-launch.c b/src/weston-launch.c index 10c66de..312b955 100644 --- a/src/weston-launch.c +++ b/src/weston-launch.c @@ -46,6 +46,7 @@ #include linux/vt.h #include linux/major.h #include linux/kd.h +#include linux/limits.h #include pwd.h #include grp.h @@ -105,6 +106,7 @@ struct weston_launch { pid_t child; int verbose; char *new_user; + char tty_path[PATH_MAX]; }; union cmsg_data { unsigned char b[4]; int fd; }; @@ -419,6 +421,13 @@ quit(struct weston_launch *wl, int status) pam_end(wl-ph, err); } + /* tty will be deinitialized by child process, so it has to be + * opened again to correctly cleanup vt handling. */ + if (wl-tty != STDIN_FILENO) { + close(wl-tty); + wl-tty = open(wl-tty_path, O_RDWR | O_NOCTTY); + } + Why this? I don't get why wl-tty is not good enough and you need to reopen the fd. if (ioctl(wl-tty, KDSKBMUTE, 0) ioctl(wl-tty, KDSKBMODE, wl-kb_mode)) fprintf(stderr, failed to restore keyboard mode: %m\n); @@ -521,8 +530,10 @@ setup_tty(struct weston_launch *wl, const char *tty) t = ttyname(STDIN_FILENO); if (t strcmp(t, tty) == 0) wl-tty = STDIN_FILENO; - else + else { wl-tty = open(tty, O_RDWR | O_NOCTTY); + strcpy(wl-tty_path, tty); + } I'm sure this is not going to ever be a problem since tty filenames and paths are on the short side, but since the tty string is an input parameter to this routine, it would be better defensive programming to use strncpy. } else { int tty0 = open(/dev/tty0, O_WRONLY | O_CLOEXEC); char filename[16]; @@ -535,6 +546,7 @@ setup_tty(struct weston_launch *wl, const char *tty) snprintf(filename, sizeof filename, /dev/tty%d, wl-ttynr); wl-tty = open(filename, O_RDWR | O_NOCTTY); + strcpy(wl-tty_path, filename); Since filename is set to a fixed length, I'm less worried about the strcpy here, but for code consistency you might use strncpy here as well. close(tty0); } @@ -555,6 +567,10 @@ setup_tty(struct weston_launch *wl, const char *tty) wl-ttynr = minor(buf.st_rdev); } + /* Activate tty, otherwise tty switches won't work right away. */ + ioctl(wl-tty, VT_ACTIVATE, wl-ttynr); + ioctl(wl-tty, VT_WAITACTIVE, wl-ttynr); + Check the ioctl returns and issue perror() on failure. Googling VT_WAITACTIVE shows misc. reports about it causing hangs in years past. No idea if that's at all likely here in Wayland. But if it is, it might be better to use a timed wait, checking the active tty each cycle, like was done in this fix: http://permalink.gmane.org/gmane.linux.kernel.suspend.devel/7119 Why do this at all? There is no reason to wait for the VT to become active. Just issue the VT_ACTIVATE and continue, it's async and that's good. Btw., I'm no big fan of activating a VT when starting a compositor. Xorg requires this as it cannot initialize in background, but new compositors should really support this. Imagine you're started by a login-manager. You really want the compositor to initialize in background and wait to be switched to by the login manager. Yes, weston-launch is special, as it is its own login-manager. So I'd be fine with the VT_ACTIVATE. But i cannot see why we'd need VT_WAITACTIVE? if (ioctl(wl-tty, KDGKBMODE, wl-kb_mode)) error(1, errno, failed to get current keyboard mode: %m\n); @@ -744,8 +760,6 @@ main(int argc, char *argv[]) launch_compositor(wl, argc - optind, argv + optind); close(wl.sock[1]); - if (wl.tty != STDIN_FILENO) - close(wl.tty); I'm having some trouble following the move of the close logic. I trust what you've done is correct but it's not evident to me why? This is weird, indeed. No idea why the TTY is
Re: [RFC v2 libinput 0/2] Buttonset interface - numbered axes
Hi On Wed, Mar 18, 2015 at 7:58 AM, Peter Hutterer peter.hutte...@who-t.net wrote: This is a re-vamped version of the buttonset interface. Still WIP but I'd like to get some comments on the API. Changes to the last version: This version now uses numbered axes instead of typed axes. Previously the interface used a unique axis type to deal with axes (e.g. LIBINPUT_BUTTONSET_AXIS_RING_LEFT). This approach was ditched, instead a device has a number of axes, each with a type. A caller can check how many axes are there and the type for each axis. In the event interface, the axis number is used, not the type. A whole bunch of axes have been added too though the wacom pad interface doesn't expose them, only wheel and ring. Branch is available here: https://github.com/whot/libinput/tree/wip/buttonbox-interface It's still missing a couple of details, tests, checks, etc. but it should show the interface well enough. Awesome! Really appreciated! Few assorted comments: 1) You introduce an enum for axis-types, instead of using the ABS_* and REL_* values. On purpose? What's the reason? You don't do this for buttons. I can come up with several reasons, but a clear statement would be nice: - you want to merge ABS_* and REL_* into a single namespace - you want to get rid of the ABS_X/Y/Z and ABS_HAT* misuse for anything that doesn't have its own ABS_* type - ABS2_* still didn't land upstream, so there's no way to introduce new ABS_* values, but we really need them (yeah, my bad, I know!) - the kernel ABS_* values just never made any sense 2) Mapping from ABS_*/REL_* to axis-types is static? That is, we do this based on the devices we know, instead of a magic translation table for all types. I really like this, as we can get rid of the crappy mappings we used to have for gamepads and so on. I just wonder why this isn't done for buttons. Yeah, the namespace is bigger for buttons and it used to have less bad mappings, but they still exist. 3) What is an axis-source? I don't get it. What else should I use to control an axis, but my finger? My tongue? :) Ok, seriously, the description is a bit short. Is this meant to distinguish between touch-based input and other input? So you get release-events for the 'finger'-based input? 4) Why hard-code 'relative' in the axis-name, instead of making it a property? AXIS_X/Y/Z and ROTATION_X/Y/Z already are two examples that can be absolute _and_ relative. Wouldn't it make sense to make 'relative' a boolean attribute of an axis on a device? I don't know what RING and STRIP is, so cannot comment on that. A lot of other axes have this implicit (accelerometer/gyro/compass are always absolute, for instance), so not sure it's a good idea. Just wanted to bring it up and see whether you have any input on it. 5) If a device has multiple axes of the same type, is the order supposed to be ABI? Example: gamepads tend to have two analog-sticks. How do applications reliably find the left and right stick? 6) *_get_*_transformed() is linear? Can you mention that in the description? It's not clear just from the name 'transform' and a min/max range. 7) libinput_event_buttonset_get_axis_delta_transformed()? What is that thing doing? libinput_event_buttonset_get_axis_delta() returns data for relative axes, but the *_transformed() version says it always returns 0 for relative axes? There's a big FIXME, but the function description doesn't make sense to me. Can you elaborate? 8) What is libinput_event_buttonset_get_seat_button_count() good for? Not that I care much, but it made me curious :) 9) There's only libinput_device_buttonset_has_button(), but no way to get a list of buttons enabled on that device. Do you expect applications to iterate all 2^32 buttons or are applications not supposed to retrieve the set of buttons on a device? Last point: Will there be some generic type for libinput-devices so applications can select them? For instance: this is a wacom-tablet-style-thing or this is a gamepad, .. etc. Or are applications supposed to look exactly at the capabilities of a device and say Hey, this has two analog-sticks, a dpad, 4 action-buttons and a bit more; lets use it as gamepad input? I like the latter, but wanna be sure what the plans are. Again, much appreciated! If this lands upstream, it'd be definitely worth a look to add gamepad/etc. support. Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH weston 1/4] logind: use SIGRTMIN to not conflict with xwayland
xwayland uses SIGUSR1 as startup notification. Make sure to use SIGRTMIN and SIGRTMIN+1 for VT handling. A bonus is SIGRT* signals can be queued multiple times, so we will be able to correctly track them and will no longer loose signals (which wouldn't really matter, but is confusing in logs). Signed-off-by: David Herrmann dh.herrm...@gmail.com --- src/logind-util.c | 28 ++-- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/logind-util.c b/src/logind-util.c index 554e64d..132a9a2 100644 --- a/src/logind-util.c +++ b/src/logind-util.c @@ -692,14 +692,10 @@ signal_event(int fd, uint32_t mask, void *data) return 0; } - switch (sig.ssi_signo) { - case SIGUSR1: + if (sig.ssi_signo == (unsigned int)SIGRTMIN) ioctl(wl-vt, VT_RELDISP, 1); - break; - case SIGUSR2: + else if (sig.ssi_signo == (unsigned int)SIGRTMIN + 1) ioctl(wl-vt, VT_RELDISP, VT_ACKACQ); - break; - } return 0; } @@ -767,9 +763,21 @@ weston_logind_setup_vt(struct weston_logind *wl) goto err_kbmode; } + /* +* SIGRTMIN is used as global VT-release signal, SIGRTMIN + 1 is used +* as VT-acquire signal. Note that SIGRT* must be tested on runtime, as +* their exact values are not known at compile-time. POSIX requires 32 +* of them to be available, though. +*/ + if (SIGRTMIN + 1 SIGRTMAX) { + weston_log(logind: not enough RT signals available: %u-%u\n, + SIGRTMIN, SIGRTMAX); + return -EINVAL; + } + sigemptyset(mask); - sigaddset(mask, SIGUSR1); - sigaddset(mask, SIGUSR2); + sigaddset(mask, SIGRTMIN); + sigaddset(mask, SIGRTMIN + 1); sigprocmask(SIG_BLOCK, mask, NULL); wl-sfd = signalfd(-1, mask, SFD_NONBLOCK | SFD_CLOEXEC); @@ -790,8 +798,8 @@ weston_logind_setup_vt(struct weston_logind *wl) } mode.mode = VT_PROCESS; - mode.relsig = SIGUSR1; - mode.acqsig = SIGUSR2; + mode.relsig = SIGRTMIN; + mode.acqsig = SIGRTMIN + 1; if (ioctl(wl-vt, VT_SETMODE, mode) 0) { r = -errno; weston_log(logind: cannot take over VT: %m\n); -- 2.2.1 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH weston 3/4] logind: forward Active=true changes for non-DRM backends
Logind sends us notification whenever the Active attribute of our session changes. However, due to the way compositor-drm.c relies on the master DRM device to be synced with the session, we used to delay Active=true handling until the DRM device was up, too. See: commit aedc7732ebd9bc7b4f51ee247ea857ffec6260a7 Author: David Herrmann dh.herrm...@gmail.com Date: Sat Nov 30 11:25:45 2013 +0100 logind: delay wakeup until DRM-device is resumed However, the other compositor backends do not use DRM, so logind-util will never get notified about any DRM device. Therefore, we have to forward the Active=true change immediately. This commit fixes logind-util to take sync_drm as argument. If it is true, we do DRM-device synchronisation, otherwise we don't. Signed-off-by: David Herrmann dh.herrm...@gmail.com --- src/compositor-drm.c | 2 +- src/compositor-fbdev.c | 5 +++-- src/compositor-rpi.c | 5 +++-- src/launcher-util.c| 4 ++-- src/launcher-util.h| 2 +- src/logind-util.c | 16 +++- src/logind-util.h | 4 ++-- 7 files changed, 23 insertions(+), 15 deletions(-) diff --git a/src/compositor-drm.c b/src/compositor-drm.c index 9b4d4dc..4473254 100644 --- a/src/compositor-drm.c +++ b/src/compositor-drm.c @@ -2774,7 +2774,7 @@ drm_compositor_create(struct wl_display *display, /* Check if we run drm-backend using weston-launch */ ec-base.launcher = weston_launcher_connect(ec-base, param-tty, - param-seat_id); + param-seat_id, true); if (ec-base.launcher == NULL) { weston_log(fatal: drm backend should be run using weston-launch binary or as root\n); diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c index 65bb035..805a195 100644 --- a/src/compositor-fbdev.c +++ b/src/compositor-fbdev.c @@ -891,8 +891,9 @@ fbdev_compositor_create(struct wl_display *display, int *argc, char *argv[], compositor-session_listener.notify = session_notify; wl_signal_add(compositor-base.session_signal, compositor-session_listener); - compositor-base.launcher = - weston_launcher_connect(compositor-base, param-tty, seat0); + compositor-base.launcher = weston_launcher_connect(compositor-base, + param-tty, seat0, + false); if (!compositor-base.launcher) { weston_log(fatal: fbdev backend should be run using weston-launch binary or as root\n); diff --git a/src/compositor-rpi.c b/src/compositor-rpi.c index 150e9e1..a064a86 100644 --- a/src/compositor-rpi.c +++ b/src/compositor-rpi.c @@ -480,8 +480,9 @@ rpi_compositor_create(struct wl_display *display, int *argc, char *argv[], compositor-session_listener.notify = session_notify; wl_signal_add(compositor-base.session_signal, compositor -session_listener); - compositor-base.launcher = - weston_launcher_connect(compositor-base, param-tty, seat0); + compositor-base.launcher = weston_launcher_connect(compositor-base, + param-tty, seat0, + false); if (!compositor-base.launcher) { weston_log(Failed to initialize tty.\n); goto out_udev; diff --git a/src/launcher-util.c b/src/launcher-util.c index ad81aef..e89710b 100644 --- a/src/launcher-util.c +++ b/src/launcher-util.c @@ -386,7 +386,7 @@ weston_launcher_activate_vt(struct weston_launcher *launcher, int vt) struct weston_launcher * weston_launcher_connect(struct weston_compositor *compositor, int tty, - const char *seat_id) + const char *seat_id, bool sync_drm) { struct weston_launcher *launcher; struct wl_event_loop *loop; @@ -418,7 +418,7 @@ weston_launcher_connect(struct weston_compositor *compositor, int tty, } } else { r = weston_logind_connect(launcher-logind, compositor, - seat_id, tty); + seat_id, tty, sync_drm); if (r 0) { launcher-logind = NULL; if (geteuid() == 0) { diff --git a/src/launcher-util.h b/src/launcher-util.h index d5b2fc9..a60f8a1 100644 --- a/src/launcher-util.h +++ b/src/launcher-util.h @@ -31,7 +31,7 @@ struct weston_launcher; struct weston_launcher * weston_launcher_connect(struct weston_compositor *compositor, int tty, - const char *seat_id); + const char *seat_id, bool sync_drm); void weston_launcher_destroy(struct weston_launcher *launcher
[PATCH weston 2/4] launcher: use SIGRTMIN to not conflict with xwayland
xwayland uses SIGUSR1 as startup notification. Make sure to use SIGRTMIN for VT handling to avoid conflicts. A bonus is SIGRT* signals can be queued multiple times, so we will be able to correctly track them and will no longer loose signals (which wouldn't really matter, but is confusing in logs). Signed-off-by: David Herrmann dh.herrm...@gmail.com --- src/launcher-util.c | 18 +++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/launcher-util.c b/src/launcher-util.c index ac764dc..ad81aef 100644 --- a/src/launcher-util.c +++ b/src/launcher-util.c @@ -342,9 +342,21 @@ setup_tty(struct weston_launcher *launcher, int tty) goto err_close; } + /* +* SIGRTMIN is used as global VT-acquire+release signal. Note that +* SIGRT* must be tested on runtime, as their exact values are not +* known at compile-time. POSIX requires 32 of them to be available. +*/ + if (SIGRTMIN SIGRTMAX) { + weston_log(not enough RT signals available: %u-%u\n, + SIGRTMIN, SIGRTMAX); + ret = -EINVAL; + goto err_close; + } + mode.mode = VT_PROCESS; - mode.relsig = SIGUSR1; - mode.acqsig = SIGUSR1; + mode.relsig = SIGRTMIN; + mode.acqsig = SIGRTMIN; if (ioctl(launcher-tty, VT_SETMODE, mode) 0) { weston_log(failed to take control of vt handling\n); goto err_close; @@ -352,7 +364,7 @@ setup_tty(struct weston_launcher *launcher, int tty) loop = wl_display_get_event_loop(launcher-compositor-wl_display); launcher-vt_source = - wl_event_loop_add_signal(loop, SIGUSR1, vt_handler, launcher); + wl_event_loop_add_signal(loop, SIGRTMIN, vt_handler, launcher); if (!launcher-vt_source) goto err_close; -- 2.2.1 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH weston 4/4] logind: fix PropertiesChanged parser
The current parser directly reads a BOOLEAN on the PropertiesChanged signal for 'Active' properties. However, all property-values are packed in a VARIANT, otherwise, we wouldn't know the type. Fix the parser to recurse into the variant before reading the boolean. To avoid such bugs in the future, we extract the 'Active' parser into a helper function parse_active(), which is then shared between the PropertiesChanged and Get handlers. Signed-off-by: David Herrmann dh.herrm...@gmail.com --- src/logind-util.c | 56 +++ 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/src/logind-util.c b/src/logind-util.c index db23606..e4e20eb 100644 --- a/src/logind-util.c +++ b/src/logind-util.c @@ -287,33 +287,18 @@ weston_logind_set_active(struct weston_logind *wl, bool active) } static void -get_active_cb(DBusPendingCall *pending, void *data) +parse_active(struct weston_logind *wl, DBusMessage *m, DBusMessageIter *iter) { - struct weston_logind *wl = data; - DBusMessage *m; - DBusMessageIter iter, sub; - int type; + DBusMessageIter sub; dbus_bool_t b; - dbus_pending_call_unref(wl-pending_active); - wl-pending_active = NULL; - - m = dbus_pending_call_steal_reply(pending); - if (!m) + if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_VARIANT) return; - type = dbus_message_get_type(m); - if (type != DBUS_MESSAGE_TYPE_METHOD_RETURN) - goto err_unref; - - if (!dbus_message_iter_init(m, iter) || - dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_VARIANT) - goto err_unref; - - dbus_message_iter_recurse(iter, sub); + dbus_message_iter_recurse(iter, sub); if (dbus_message_iter_get_arg_type(sub) != DBUS_TYPE_BOOLEAN) - goto err_unref; + return; dbus_message_iter_get_basic(sub, b); @@ -322,8 +307,28 @@ get_active_cb(DBusPendingCall *pending, void *data) * other backends, we immediately forward the Active-change event. */ if (!wl-sync_drm || !b) weston_logind_set_active(wl, b); +} + +static void +get_active_cb(DBusPendingCall *pending, void *data) +{ + struct weston_logind *wl = data; + DBusMessageIter iter; + DBusMessage *m; + int type; + + dbus_pending_call_unref(wl-pending_active); + wl-pending_active = NULL; + + m = dbus_pending_call_steal_reply(pending); + if (!m) + return; + + type = dbus_message_get_type(m); + if (type == DBUS_MESSAGE_TYPE_METHOD_RETURN + dbus_message_iter_init(m, iter)) + parse_active(wl, m, iter); -err_unref: dbus_message_unref(m); } @@ -408,7 +413,6 @@ property_changed(struct weston_logind *wl, DBusMessage *m) { DBusMessageIter iter, sub, entry; const char *interface, *name; - dbus_bool_t b; if (!dbus_message_iter_init(m, iter) || dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_STRING) @@ -433,12 +437,8 @@ property_changed(struct weston_logind *wl, DBusMessage *m) goto error; if (!strcmp(name, Active)) { - if (dbus_message_iter_get_arg_type(entry) == DBUS_TYPE_BOOLEAN) { - dbus_message_iter_get_basic(entry, b); - if (!b) - weston_logind_set_active(wl, false); - return; - } + parse_active(wl, m, entry); + return; } dbus_message_iter_next(sub); -- 2.2.1 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 1/1] weston-launch: alter tty command line parameter semantics
Hi On Tue, Dec 16, 2014 at 9:19 AM, Daniel Stone dan...@fooishbar.org wrote: I reasoned that user access to the tty should be set up by the kernel policies, and we should not enforce the policy at weston level. If the system is configured in this way, then a user with enough permissions can start up weston under his account without having to have root permissions. In the end, I can use the openvt workaround, if you are concerned about the security implications. Well, given that weston-launch is suid and opens the device on behalf of weston, you're actually bypassing all of the kernel policies and enforcement, since the kernel will just see root attempting to open it. This is what makes me nervous. Previously weston-launch would only allow arbitrary TTY selection if you were actually root (user can only be set when getuid() == geteuid()), but this change allows any user with weston-launch access to open any VT that root can access. I could definitely be swayed, but in the absence of someone who knows definitively whether or not this is a good idea (David?), I'd lean towards not changing the current behaviour - except to produce an error message when --tty is specified but not --user. So far VT allocation was left to your login-manager and you shouldn't mess with it. The openvt-logic (whether that is 'openvt' or 'VT_OPENQRY') is meant as workaround for people who want minimal login-managers (or rather no login-manager at all). So if we can avoid supporting any more sophisticated options, I'd welcome that. So I agree with Daniel here. Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] Fix bug https://bugs.freedesktop.org/show_bug.cgi?id=86889 by emitting session signals on TTY switches so that the weston backends can handle VT switching when not called from weston-launc
Hi On Mon, Dec 15, 2014 at 11:04 AM, Pekka Paalanen ppaala...@gmail.com wrote: On Sun, 07 Dec 2014 10:15:29 -0500 nerdopolis bluescreen_aven...@verizon.net wrote: --- src/logind-util.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/logind-util.c b/src/logind-util.c index 6a1b498..4308bb6 100644 --- a/src/logind-util.c +++ b/src/logind-util.c @@ -695,9 +695,15 @@ signal_event(int fd, uint32_t mask, void *data) switch (sig.ssi_signo) { case SIGUSR1: ioctl(wl-vt, VT_RELDISP, 1); + wl-compositor-session_active=0; + wl_signal_emit(wl-compositor-session_signal, +wl-compositor); break; case SIGUSR2: ioctl(wl-vt, VT_RELDISP, VT_ACKACQ); + wl-compositor-session_active=1; + wl_signal_emit(wl-compositor-session_signal, +wl-compositor); break; } Hi, reading the commit http://cgit.freedesktop.org/wayland/weston/commit/?id=aedc7732ebd9bc7b4f51ee247ea857ffec6260a7 I'm not too sure this patch is right. David, would you mind taking a look? This patch breaks compositor-drm.c, indeed. The underlying problem is the way compositor-drm was written (by relying on the DRM device to define session status). As that is non-trivial to change, logind-util.c syns session state with the master DRM device. I wrote a patch that should fix this issue for !DRM backends. I also attached the patch in case the inlined version is whitespace damaged. Thanks David From 9b487b2177633c86f934ad3e682910e777d7b543 Mon Sep 17 00:00:00 2001 From: David Herrmann dh.herrm...@gmail.com Date: Mon, 15 Dec 2014 13:34:35 +0100 Subject: [PATCH] logind: forward Active=true changes for non-DRM backends Logind sends us notification whenever the Active attribute of our session changes. However, due to the way compositor-drm.c relies on the master DRM device to be synced with the session, we used to delay Active=true handling until the DRM device was up, too. See: commit aedc7732ebd9bc7b4f51ee247ea857ffec6260a7 Author: David Herrmann dh.herrm...@gmail.com Date: Sat Nov 30 11:25:45 2013 +0100 logind: delay wakeup until DRM-device is resumed However, the other compositor backends do not use DRM, so logind-util will never get notified about any DRM device. Therefore, we have to forward the Active=true change immediately. This commit fixes logind-util to take sync_drm as argument. If it is true, we do DRM-device synchronisation, otherwise we don't. Signed-off-by: David Herrmann dh.herrm...@gmail.com --- src/compositor-drm.c | 2 +- src/compositor-fbdev.c | 5 +++-- src/compositor-rpi.c | 5 +++-- src/launcher-util.c| 4 ++-- src/launcher-util.h| 2 +- src/logind-util.c | 12 +--- src/logind-util.h | 4 ++-- 7 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/compositor-drm.c b/src/compositor-drm.c index 9b4d4dc..4473254 100644 --- a/src/compositor-drm.c +++ b/src/compositor-drm.c @@ -2774,7 +2774,7 @@ drm_compositor_create(struct wl_display *display, /* Check if we run drm-backend using weston-launch */ ec-base.launcher = weston_launcher_connect(ec-base, param-tty, -param-seat_id); +param-seat_id, true); if (ec-base.launcher == NULL) { weston_log(fatal: drm backend should be run using weston-launch binary or as root\n); diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c index 65bb035..805a195 100644 --- a/src/compositor-fbdev.c +++ b/src/compositor-fbdev.c @@ -891,8 +891,9 @@ fbdev_compositor_create(struct wl_display *display, int *argc, char *argv[], compositor-session_listener.notify = session_notify; wl_signal_add(compositor-base.session_signal, compositor-session_listener); - compositor-base.launcher = - weston_launcher_connect(compositor-base, param-tty, seat0); + compositor-base.launcher = weston_launcher_connect(compositor-base, +param-tty, seat0, +false); if (!compositor-base.launcher) { weston_log(fatal: fbdev backend should be run using weston-launch binary or as root\n); diff --git a/src/compositor-rpi.c b/src/compositor-rpi.c index 150e9e1..a064a86 100644 --- a/src/compositor-rpi.c +++ b/src/compositor-rpi.c @@ -480,8 +480,9 @@ rpi_compositor_create(struct wl_display *display, int *argc, char *argv[], compositor-session_listener.notify = session_notify; wl_signal_add(compositor-base.session_signal, compositor -session_listener); - compositor-base.launcher = - weston_launcher_connect(compositor-base, param-tty, seat0); + compositor-base.launcher = weston_launcher_connect(compositor-base, +param-tty, seat0, +false); if (!compositor-base.launcher) { weston_log(Failed to initialize tty.\n); goto out_udev; diff --git a/src/launcher-util.c b/src/launcher-util.c index ac764dc..2d9fb9c 100644 --- a/src/launcher-util.c +++ b/src/launcher-util.c @@ -374,7 +374,7
Re: [PATCH libinput v2 1/2] util: introduce ratelimit helpers
Hi On Thu, Nov 6, 2014 at 6:51 AM, Peter Hutterer peter.hutte...@who-t.net wrote: On Wed, Nov 05, 2014 at 01:32:16PM +0100, David Herrmann wrote: diff --git a/test/misc.c b/test/misc.c index 1512180..aa411ec 100644 --- a/test/misc.c +++ b/test/misc.c @@ -508,6 +508,40 @@ START_TEST(matrix_helpers) } END_TEST +START_TEST(ratelimit_helpers) +{ + struct ratelimit rl; + unsigned int i, j; + + /* 10 attempts every 10ms */ + ratelimit_init(rl, 10, 10); + + for (j = 0; j 100; ++j) { + /* a burst of 9 attempts must succeed */ + for (i = 0; i 9; ++i) + ck_assert(ratelimit_test(rl) == RATELIMIT_PASS); + + /* the 10th attempt reaches the threshold */ + ck_assert(ratelimit_test(rl) == RATELIMIT_THRESHOLD); merged both, with changes to ck_assert_int_eq: ck_assert_int_eq(ratelimit_test(rl), RATELIMIT_THRESHOLD); a bit quicker to debug when it fails, the error message will print both values so you get an idea of what the mismatch is. Whoops, yeah, totally forgot about the other ck_assert_* macros. Thanks for amending the changes! Thanks David thanks Cheers, Peter + + /* ..then further attempts must fail.. */ + ck_assert(ratelimit_test(rl) == RATELIMIT_EXCEEDED); + + /* ..regardless of how often we try. */ + for (i = 0; i 100; ++i) + ck_assert(ratelimit_test(rl) == RATELIMIT_EXCEEDED); + + /* ..even after waiting 5ms */ + msleep(5); + for (i = 0; i 100; ++i) + ck_assert(ratelimit_test(rl) == RATELIMIT_EXCEEDED); + + /* but after 10ms the counter is reset */ + msleep(6); /* +1ms to account for time drifts */ + } +} +END_TEST + int main (int argc, char **argv) { litest_add_no_device(events:conversion, event_conversion_device_notify); litest_add_no_device(events:conversion, event_conversion_pointer); @@ -519,5 +553,7 @@ int main (int argc, char **argv) { litest_add_no_device(config:status string, config_status_string); litest_add_no_device(misc:matrix, matrix_helpers); + litest_add_no_device(misc:ratelimit, ratelimit_helpers); + return litest_run(argc, argv); } -- 2.1.3 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v3 libinput] util: introduce ratelimit helpers
Hi On Thu, Nov 6, 2014 at 11:39 PM, Peter Hutterer peter.hutte...@who-t.net wrote: From: David Herrmann dh.herrm...@gmail.com This adds struct ratelimit and ratelimit_test(). It's a very simple rate-limit helper modeled after Linux' lib/ratelimit.c by Dave Young. This comes in handy to limit log-messages in possible busy loops etc.. Signed-off-by: David Herrmann dh.herrm...@gmail.com Reviewed-by: Peter Hutterer peter.hutte...@who-t.net Signed-off-by: Peter Hutterer peter.hutte...@who-t.net --- The test succeeded but not in valgrind which slows everything down enough that we didn't trigger the rate limit (valgrind is part of make check). I though of adding extra flags to detect when we're running valgrind but it's simpler to just extend the timeouts a bit. So instead of 10 msg per 10ms it's 10 per 100ms, the wait is 20ms and then another 90ms to go definitely above the 100ms. And only loop three times, otherwise we trigger the generic test timeout detection. Changes to v3: - ck_assert(a == b) - ck_assert_int_eq(a, b) - timeouts tweaked to avoid test failures when running through valgrind Wow, I'm no longer the one with the slowest laptop! ;) Btw., the ck_assert_*() helpers are the bottleneck, not valgrind (at least that's my guess based on previous performance-problems with ck_assert). So we could maybe also try something like: if (!$condition) ck_assert($condition); That fixed my code last time, but totally untested and maybe libcheck does that automatically now. Your patch is fine, too. No objections from my side. Thanks David src/libinput-util.c | 48 src/libinput-util.h | 16 test/misc.c | 42 ++ 3 files changed, 106 insertions(+) diff --git a/src/libinput-util.c b/src/libinput-util.c index eeb9786..34d5549 100644 --- a/src/libinput-util.c +++ b/src/libinput-util.c @@ -65,3 +65,51 @@ list_empty(const struct list *list) { return list-next == list; } + +void +ratelimit_init(struct ratelimit *r, uint64_t ival_ms, unsigned int burst) +{ + r-interval = ival_ms; + r-begin = 0; + r-burst = burst; + r-num = 0; +} + +/* + * Perform rate-limit test. Returns RATELIMIT_PASS if the rate-limited action + * is still allowed, RATELIMIT_THRESHOLD if the limit has been reached with + * this call, and RATELIMIT_EXCEEDED if you're beyond the threshold. + * It's safe to treat the return-value as boolean, if you're not interested in + * the exact state. It evaluates to true if the threshold hasn't been + * exceeded, yet. + * + * The ratelimit object must be initialized via ratelimit_init(). + * + * Modelled after Linux' lib/ratelimit.c by Dave Young + * hidave.darks...@gmail.com, which is licensed GPLv2. + */ +enum ratelimit_state +ratelimit_test(struct ratelimit *r) +{ + struct timespec ts; + uint64_t mtime; + + if (r-interval = 0 || r-burst = 0) + return RATELIMIT_PASS; + + clock_gettime(CLOCK_MONOTONIC, ts); + mtime = ts.tv_sec * 1000 + ts.tv_nsec / 1000 / 1000; + + if (r-begin = 0 || r-begin + r-interval mtime) { + /* reset counter */ + r-begin = mtime; + r-num = 1; + return RATELIMIT_PASS; + } else if (r-num r-burst) { + /* continue burst */ + return (++r-num == r-burst) ? RATELIMIT_THRESHOLD + : RATELIMIT_PASS; + } + + return RATELIMIT_EXCEEDED; +} diff --git a/src/libinput-util.h b/src/libinput-util.h index 51759e8..909c9db 100644 --- a/src/libinput-util.h +++ b/src/libinput-util.h @@ -280,4 +280,20 @@ matrix_to_farray6(const struct matrix *m, float out[6]) out[5] = m-val[1][2]; } +enum ratelimit_state { + RATELIMIT_EXCEEDED, + RATELIMIT_THRESHOLD, + RATELIMIT_PASS, +}; + +struct ratelimit { + uint64_t interval; + uint64_t begin; + unsigned int burst; + unsigned int num; +}; + +void ratelimit_init(struct ratelimit *r, uint64_t ival_ms, unsigned int burst); +enum ratelimit_state ratelimit_test(struct ratelimit *r); + #endif /* LIBINPUT_UTIL_H */ diff --git a/test/misc.c b/test/misc.c index 1512180..38320f3 100644 --- a/test/misc.c +++ b/test/misc.c @@ -508,6 +508,46 @@ START_TEST(matrix_helpers) } END_TEST +START_TEST(ratelimit_helpers) +{ + struct ratelimit rl; + unsigned int i, j; + + /* 10 attempts every 100ms */ + ratelimit_init(rl, 100, 10); + + for (j = 0; j 3; ++j) { + /* a burst of 9 attempts must succeed */ + for (i = 0; i 9; ++i) { + ck_assert_int_eq(ratelimit_test(rl), +RATELIMIT_PASS); + } + + /* the 10th attempt reaches
Re: [PATCH libinput 1/2] util: introduce ratelimit helpers
Hi On Wed, Nov 5, 2014 at 6:11 AM, Peter Hutterer peter.hutte...@who-t.net wrote: On Tue, Nov 04, 2014 at 09:35:37AM +0100, David Herrmann wrote: This adds struct ratelimit and ratelimit_test(). It's a very simple rate-limit helper modeled after Linux' lib/ratelimit.c by Dave Young. This comes in handy to limit log-messages in possible busy loops etc.. Signed-off-by: David Herrmann dh.herrm...@gmail.com --- src/libinput-util.c | 48 src/libinput-util.h | 19 +++ test/misc.c | 37 + 3 files changed, 104 insertions(+) diff --git a/src/libinput-util.c b/src/libinput-util.c index eeb9786..19594e3 100644 --- a/src/libinput-util.c +++ b/src/libinput-util.c @@ -65,3 +65,51 @@ list_empty(const struct list *list) { return list-next == list; } + +/* + * Perform rate-limit test. Returns true if the rate-limited action is still + * allowed, false if it should be suppressed. + * + * The ratelimit object must be initialized via RATELIMIT_INIT(). + * + * Modelled after Linux' lib/ratelimit.c by Dave Young + * hidave.darks...@gmail.com, which is licensed GPLv2. + */ +bool ratelimit_test(struct ratelimit *r) libinput style is: return type on a separate line Fixed. +{ + struct timespec ts; + uint64_t utime; + + if (r-interval = 0 || r-burst = 0) + return true; + + clock_gettime(CLOCK_MONOTONIC, ts); + utime = ts.tv_sec * 1000 * 1000 + ts.tv_nsec / 1000; + + if (r-begin = 0 || r-begin + r-interval utime) { + /* reset counter */ + r-begin = utime; + r-num = 1; + return true; + } else if (r-num r-burst) { + /* continue burst */ + r-num++; + return true; + } + + /* rate-limit with overflow check */ + if (r-num + 1 r-num) + ++r-num; + + return false; +} + +/* + * Return true if the ratelimit counter just crossed the cutoff value. That is, + * this function returns true iff the last call to ratelimit_test() was the s/iff/if/ Iff is widely used for if, and only if, [1]. Should I still change it? + * first call to exceed the burst value in this interval. + */ +bool ratelimit_cutoff(struct ratelimit *r) bool on separate line please Fixed. +{ + return r-num == r-burst + 1; +} I'm wondering: why have two separate functions here? how about an enum ratelimit { RATELIMIT_PASS, RATELIMIT_THRESHOLD, RATELIMIT_EXCEEDED, }; then return that from ratelimit_test and then use the return value to decide on the rest of the handling? so the dispatch code would be: if ((rc = ratelimit_test(...)) != RATELIMIT_EXCEEDED)) { log_info(SYN_DROPPED); if (rc == RATELIMIT_THRESHOLD) { log_info(SYN_DROPPED flood); } } or the same with a switch statement. Sure, can do that. diff --git a/src/libinput-util.h b/src/libinput-util.h index 51759e8..8ff8778 100644 --- a/src/libinput-util.h +++ b/src/libinput-util.h @@ -25,6 +25,7 @@ #include unistd.h #include math.h +#include stdbool.h #include string.h #include time.h @@ -280,4 +281,22 @@ matrix_to_farray6(const struct matrix *m, float out[6]) out[5] = m-val[1][2]; } +struct ratelimit { + uint64_t interval; + uint64_t begin; + unsigned burst; + unsigned num; unsigned int please Fixed. +} RateLimit; well, hello. what are you doing here? are you lost? :) Weird.. gcc didn't warn me about this unused variable.. Fixed. + +#define RATELIMIT_INIT(_interval, _burst)\ + ((struct ratelimit){\ + .interval = (_interval),\ + .begin = 0, \ + .burst = (_burst), \ + .num = 0, \ + }) any reason you didn't make this into a function of void ratelimit_init(struct ratelimit *rl)? I don't see a lot of benefits having this as a macro given that it's only called once anyway (per context). If you want it as global variable, you cannot use a function to initialize it. I usually prefer literals to initialize objects as it can be optimized by the compiler. But I can provide ratelimit_init(), if you want. For the single use-case we have, both are fine. + +bool ratelimit_test(struct ratelimit *r); +bool ratelimit_cutoff(struct ratelimit *r); + #endif /* LIBINPUT_UTIL_H */ diff --git a/test/misc.c b/test/misc.c index 1512180..70b3e57 100644 --- a/test/misc.c +++ b/test/misc.c @@ -508,6 +508,42 @@ START_TEST(matrix_helpers) } END_TEST +START_TEST(ratelimit_helpers) +{ + /* 10 attempts every 10ms */ + struct ratelimit rl = RATELIMIT_INIT(1, 10); + unsigned int i, j; + + for (j = 0; j 100; ++j
[PATCH libinput v2 1/2] util: introduce ratelimit helpers
This adds struct ratelimit and ratelimit_test(). It's a very simple rate-limit helper modeled after Linux' lib/ratelimit.c by Dave Young. This comes in handy to limit log-messages in possible busy loops etc.. Signed-off-by: David Herrmann dh.herrm...@gmail.com --- src/libinput-util.c | 48 src/libinput-util.h | 16 test/misc.c | 36 3 files changed, 100 insertions(+) diff --git a/src/libinput-util.c b/src/libinput-util.c index eeb9786..34d5549 100644 --- a/src/libinput-util.c +++ b/src/libinput-util.c @@ -65,3 +65,51 @@ list_empty(const struct list *list) { return list-next == list; } + +void +ratelimit_init(struct ratelimit *r, uint64_t ival_ms, unsigned int burst) +{ + r-interval = ival_ms; + r-begin = 0; + r-burst = burst; + r-num = 0; +} + +/* + * Perform rate-limit test. Returns RATELIMIT_PASS if the rate-limited action + * is still allowed, RATELIMIT_THRESHOLD if the limit has been reached with + * this call, and RATELIMIT_EXCEEDED if you're beyond the threshold. + * It's safe to treat the return-value as boolean, if you're not interested in + * the exact state. It evaluates to true if the threshold hasn't been + * exceeded, yet. + * + * The ratelimit object must be initialized via ratelimit_init(). + * + * Modelled after Linux' lib/ratelimit.c by Dave Young + * hidave.darks...@gmail.com, which is licensed GPLv2. + */ +enum ratelimit_state +ratelimit_test(struct ratelimit *r) +{ + struct timespec ts; + uint64_t mtime; + + if (r-interval = 0 || r-burst = 0) + return RATELIMIT_PASS; + + clock_gettime(CLOCK_MONOTONIC, ts); + mtime = ts.tv_sec * 1000 + ts.tv_nsec / 1000 / 1000; + + if (r-begin = 0 || r-begin + r-interval mtime) { + /* reset counter */ + r-begin = mtime; + r-num = 1; + return RATELIMIT_PASS; + } else if (r-num r-burst) { + /* continue burst */ + return (++r-num == r-burst) ? RATELIMIT_THRESHOLD + : RATELIMIT_PASS; + } + + return RATELIMIT_EXCEEDED; +} diff --git a/src/libinput-util.h b/src/libinput-util.h index 51759e8..909c9db 100644 --- a/src/libinput-util.h +++ b/src/libinput-util.h @@ -280,4 +280,20 @@ matrix_to_farray6(const struct matrix *m, float out[6]) out[5] = m-val[1][2]; } +enum ratelimit_state { + RATELIMIT_EXCEEDED, + RATELIMIT_THRESHOLD, + RATELIMIT_PASS, +}; + +struct ratelimit { + uint64_t interval; + uint64_t begin; + unsigned int burst; + unsigned int num; +}; + +void ratelimit_init(struct ratelimit *r, uint64_t ival_ms, unsigned int burst); +enum ratelimit_state ratelimit_test(struct ratelimit *r); + #endif /* LIBINPUT_UTIL_H */ diff --git a/test/misc.c b/test/misc.c index 1512180..aa411ec 100644 --- a/test/misc.c +++ b/test/misc.c @@ -508,6 +508,40 @@ START_TEST(matrix_helpers) } END_TEST +START_TEST(ratelimit_helpers) +{ + struct ratelimit rl; + unsigned int i, j; + + /* 10 attempts every 10ms */ + ratelimit_init(rl, 10, 10); + + for (j = 0; j 100; ++j) { + /* a burst of 9 attempts must succeed */ + for (i = 0; i 9; ++i) + ck_assert(ratelimit_test(rl) == RATELIMIT_PASS); + + /* the 10th attempt reaches the threshold */ + ck_assert(ratelimit_test(rl) == RATELIMIT_THRESHOLD); + + /* ..then further attempts must fail.. */ + ck_assert(ratelimit_test(rl) == RATELIMIT_EXCEEDED); + + /* ..regardless of how often we try. */ + for (i = 0; i 100; ++i) + ck_assert(ratelimit_test(rl) == RATELIMIT_EXCEEDED); + + /* ..even after waiting 5ms */ + msleep(5); + for (i = 0; i 100; ++i) + ck_assert(ratelimit_test(rl) == RATELIMIT_EXCEEDED); + + /* but after 10ms the counter is reset */ + msleep(6); /* +1ms to account for time drifts */ + } +} +END_TEST + int main (int argc, char **argv) { litest_add_no_device(events:conversion, event_conversion_device_notify); litest_add_no_device(events:conversion, event_conversion_pointer); @@ -519,5 +553,7 @@ int main (int argc, char **argv) { litest_add_no_device(config:status string, config_status_string); litest_add_no_device(misc:matrix, matrix_helpers); + litest_add_no_device(misc:ratelimit, ratelimit_helpers); + return litest_run(argc, argv); } -- 2.1.3 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH libinput v2 2/2] evdev: ratelimit SYN_DROPPED logging
Use the ratelimit helpers for SYN_DROPPED logging. This guarantees that we will still receive SYN_DROPPED log-messages after multiple days of runtime, even though there might have been a SYN_DROPPED flood at one point in time. Signed-off-by: David Herrmann dh.herrm...@gmail.com --- src/evdev.c | 18 -- src/evdev.h | 4 +--- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/evdev.c b/src/evdev.c index 3aa87a7..0e2d54d 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -926,15 +926,19 @@ evdev_device_dispatch(void *data) rc = libevdev_next_event(device-evdev, LIBEVDEV_READ_FLAG_NORMAL, ev); if (rc == LIBEVDEV_READ_STATUS_SYNC) { - if (device-syn_drops_received 10) { - device-syn_drops_received++; + switch (ratelimit_test(device-syn_drop_limit)) { + case RATELIMIT_PASS: log_info(libinput, SYN_DROPPED event from \%s\ - some input events have been lost.\n, device-devname); - if (device-syn_drops_received == 10) - log_info(libinput, No longer logging -SYN_DROPPED events for -\%s\\n, device-devname); + break; + case RATELIMIT_THRESHOLD: + log_info(libinput, SYN_DROPPED flood +from \%s\\n, +device-devname); + break; + case RATELIMIT_EXCEEDED: + break; } /* send one more sync event so we handle all @@ -1296,6 +1300,8 @@ evdev_device_create(struct libinput_seat *seat, device-scroll.threshold = 5.0; /* Default may be overridden */ device-scroll.direction = 0; device-dpi = DEFAULT_MOUSE_DPI; + /* at most 5 SYN_DROPPED log-messages per 30s */ + ratelimit_init(device-syn_drop_limit, 30ULL * 1000, 5); matrix_init_identity(device-abs.calibration); matrix_init_identity(device-abs.usermatrix); diff --git a/src/evdev.h b/src/evdev.h index 666c8dc..eefbb79 100644 --- a/src/evdev.h +++ b/src/evdev.h @@ -137,9 +137,7 @@ struct evdev_device { } buttons; int dpi; /* HW resolution */ - /* The number of times libevdev processes a SYN_DROPPED, so we can -* stop logging them to avoid flooding the logs. */ - int syn_drops_received; + struct ratelimit syn_drop_limit; /* ratelimit for SYN_DROPPED logging */ }; #define EVDEV_UNHANDLED_DEVICE ((struct evdev_device *) 1) -- 2.1.3 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH libinput 1/2] util: introduce ratelimit helpers
This adds struct ratelimit and ratelimit_test(). It's a very simple rate-limit helper modeled after Linux' lib/ratelimit.c by Dave Young. This comes in handy to limit log-messages in possible busy loops etc.. Signed-off-by: David Herrmann dh.herrm...@gmail.com --- src/libinput-util.c | 48 src/libinput-util.h | 19 +++ test/misc.c | 37 + 3 files changed, 104 insertions(+) diff --git a/src/libinput-util.c b/src/libinput-util.c index eeb9786..19594e3 100644 --- a/src/libinput-util.c +++ b/src/libinput-util.c @@ -65,3 +65,51 @@ list_empty(const struct list *list) { return list-next == list; } + +/* + * Perform rate-limit test. Returns true if the rate-limited action is still + * allowed, false if it should be suppressed. + * + * The ratelimit object must be initialized via RATELIMIT_INIT(). + * + * Modelled after Linux' lib/ratelimit.c by Dave Young + * hidave.darks...@gmail.com, which is licensed GPLv2. + */ +bool ratelimit_test(struct ratelimit *r) +{ + struct timespec ts; + uint64_t utime; + + if (r-interval = 0 || r-burst = 0) + return true; + + clock_gettime(CLOCK_MONOTONIC, ts); + utime = ts.tv_sec * 1000 * 1000 + ts.tv_nsec / 1000; + + if (r-begin = 0 || r-begin + r-interval utime) { + /* reset counter */ + r-begin = utime; + r-num = 1; + return true; + } else if (r-num r-burst) { + /* continue burst */ + r-num++; + return true; + } + + /* rate-limit with overflow check */ + if (r-num + 1 r-num) + ++r-num; + + return false; +} + +/* + * Return true if the ratelimit counter just crossed the cutoff value. That is, + * this function returns true iff the last call to ratelimit_test() was the + * first call to exceed the burst value in this interval. + */ +bool ratelimit_cutoff(struct ratelimit *r) +{ + return r-num == r-burst + 1; +} diff --git a/src/libinput-util.h b/src/libinput-util.h index 51759e8..8ff8778 100644 --- a/src/libinput-util.h +++ b/src/libinput-util.h @@ -25,6 +25,7 @@ #include unistd.h #include math.h +#include stdbool.h #include string.h #include time.h @@ -280,4 +281,22 @@ matrix_to_farray6(const struct matrix *m, float out[6]) out[5] = m-val[1][2]; } +struct ratelimit { + uint64_t interval; + uint64_t begin; + unsigned burst; + unsigned num; +} RateLimit; + +#define RATELIMIT_INIT(_interval, _burst) \ + ((struct ratelimit){\ + .interval = (_interval),\ + .begin = 0, \ + .burst = (_burst), \ + .num = 0, \ + }) + +bool ratelimit_test(struct ratelimit *r); +bool ratelimit_cutoff(struct ratelimit *r); + #endif /* LIBINPUT_UTIL_H */ diff --git a/test/misc.c b/test/misc.c index 1512180..70b3e57 100644 --- a/test/misc.c +++ b/test/misc.c @@ -508,6 +508,42 @@ START_TEST(matrix_helpers) } END_TEST +START_TEST(ratelimit_helpers) +{ + /* 10 attempts every 10ms */ + struct ratelimit rl = RATELIMIT_INIT(1, 10); + unsigned int i, j; + + for (j = 0; j 100; ++j) { + /* a burst of 10 attempts must succeed */ + for (i = 0; i 10; ++i) { + ck_assert(ratelimit_test(rl)); + ck_assert(!ratelimit_cutoff(rl)); + } + + /* ..then further attempts must fail.. */ + ck_assert(!ratelimit_test(rl)); + ck_assert(ratelimit_cutoff(rl)); + + /* ..regardless of how often we try. */ + for (i = 0; i 100; ++i) { + ck_assert(!ratelimit_test(rl)); + ck_assert(!ratelimit_cutoff(rl)); + } + + /* ..even after waiting 5ms */ + usleep(5000); + for (i = 0; i 100; ++i) { + ck_assert(!ratelimit_test(rl)); + ck_assert(!ratelimit_cutoff(rl)); + } + + /* but after 10ms the counter is reset */ + usleep(6000); /* +1ms to account for time drifts */ + } +} +END_TEST + int main (int argc, char **argv) { litest_add_no_device(events:conversion, event_conversion_device_notify); litest_add_no_device(events:conversion, event_conversion_pointer); @@ -519,5 +555,6 @@ int main (int argc, char **argv) { litest_add_no_device(config:status string, config_status_string); litest_add_no_device(misc:matrix, matrix_helpers); + litest_add_no_device(misc:ratelimit, ratelimit_helpers); return litest_run(argc, argv); } -- 2.1.3
[PATCH libinput 2/2] evdev: ratelimit SYN_DROPPED logging
Use the ratelimit helpers for SYN_DROPPED logging. This guarantees that we will still receive SYN_DROPPED log-messages after multiple days of runtime, even though there might have been a SYN_DROPPED flood at one point in time. Signed-off-by: David Herrmann dh.herrm...@gmail.com --- src/evdev.c | 14 +++--- src/evdev.h | 4 +--- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/evdev.c b/src/evdev.c index 3aa87a7..836ce56 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -926,16 +926,14 @@ evdev_device_dispatch(void *data) rc = libevdev_next_event(device-evdev, LIBEVDEV_READ_FLAG_NORMAL, ev); if (rc == LIBEVDEV_READ_STATUS_SYNC) { - if (device-syn_drops_received 10) { - device-syn_drops_received++; + if (ratelimit_test(device-syn_drop_limit)) log_info(libinput, SYN_DROPPED event from \%s\ - some input events have been lost.\n, device-devname); - if (device-syn_drops_received == 10) - log_info(libinput, No longer logging -SYN_DROPPED events for -\%s\\n, device-devname); - } + else if (ratelimit_cutoff(device-syn_drop_limit)) + log_info(libinput, SYN_DROPPED flood +for \%s\\n, +device-devname); /* send one more sync event so we handle all currently pending events before we sync up @@ -1296,6 +1294,8 @@ evdev_device_create(struct libinput_seat *seat, device-scroll.threshold = 5.0; /* Default may be overridden */ device-scroll.direction = 0; device-dpi = DEFAULT_MOUSE_DPI; + /* at most 5 SYN_DROPPED log-messages per 30s */ + device-syn_drop_limit = RATELIMIT_INIT(30ULL * 1000 * 1000, 5); matrix_init_identity(device-abs.calibration); matrix_init_identity(device-abs.usermatrix); diff --git a/src/evdev.h b/src/evdev.h index 666c8dc..eefbb79 100644 --- a/src/evdev.h +++ b/src/evdev.h @@ -137,9 +137,7 @@ struct evdev_device { } buttons; int dpi; /* HW resolution */ - /* The number of times libevdev processes a SYN_DROPPED, so we can -* stop logging them to avoid flooding the logs. */ - int syn_drops_received; + struct ratelimit syn_drop_limit; /* ratelimit for SYN_DROPPED logging */ }; #define EVDEV_UNHANDLED_DEVICE ((struct evdev_device *) 1) -- 2.1.3 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 0/4] some acceleration fixes, mostly for high DPI mice
Hi On Tue, Nov 4, 2014 at 4:51 AM, Peter Hutterer peter.hutte...@who-t.net wrote: On Mon, Nov 03, 2014 at 11:56:59AM +0100, David Herrmann wrote: I haven't spent much time thinking it through, but so far I'd prefer a solid, but basic, heuristic to guess the DPI and then use hwdb for anything that doesn't fit. This allows us to change the heuristic at any time and we don't have to introduce any APIs. We can even ship the hwdb files with libinput. tbh, I don't think heuristics will work. there is no reference point and when you're looking at relative motion you can't tell if a delta of 10 means the pointer has moved a large distance at low-res or a small distance at high-res. I was more talking about the MIN/MAX values reported by the device. I assumed that high-dpi devices just reported higher precision for the same interval as low-dpi devices. But as your data shows, this turns out to be not true... what a bummer! Anyway, for some data: I recorded my Logitech G500s here with three different dpi settings (400, 800, 2000) and a MS Comfort Optical Mouse 3000 (1000 dpi). A couple of interesting things: regardless of dpi, virtually all events are within [-3, +3] dx/dy and there's nothing over 10. Exception here is the MS mouse which goes up to 78 for a delta, for reasons I can't explain yet. Interesting. This is probably to throttle HID throughput and use report-on-change, instead of continuous-reporting. Anyway, if you want, I can come up with a hwdb format to retrieve DPI settings. Just lemme know what you want to match on for the devices. Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput v2] evdev: Log evdev event queue overflows
Hi On Tue, Nov 4, 2014 at 12:17 AM, Peter Hutterer peter.hutte...@who-t.net wrote: On Mon, Nov 03, 2014 at 11:49:12AM +0100, David Herrmann wrote: Hi On Wed, Oct 29, 2014 at 3:56 PM, Derek Foreman der...@osg.samsung.com wrote: Log a message when the kernel event queue overflows and events are dropped. After 10 messages logging stops to avoid flooding the logs if the condition is persistent. --- src/evdev.c | 11 +++ src/evdev.h | 4 2 files changed, 15 insertions(+) diff --git a/src/evdev.c b/src/evdev.c index 1b4ce10..9026f5c 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -924,6 +924,17 @@ evdev_device_dispatch(void *data) rc = libevdev_next_event(device-evdev, LIBEVDEV_READ_FLAG_NORMAL, ev); if (rc == LIBEVDEV_READ_STATUS_SYNC) { + if (device-syn_drops_received 10) { + device-syn_drops_received++; + log_info(libinput, SYN_DROPPED event from +\%s\ - some input events have +been lost.\n, device-devname); + if (device-syn_drops_received == 10) + log_info(libinput, No longer logging +SYN_DROPPED events for +\%s\\n, device-devname); + } + I really appreciate those log-messages, but can we use rate-limiting here, rather than a per-device counter? I mean, my compositor usually runs for multiple days, or even weeks. I really don't want it to stop reporting SYN_DROPPED events just because it got 100 of those during startup (or something similar). Something like this: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/lib/ratelimit.c I can prep a patch if you want. good idea, let's do that. I sent 2 patches to wayland-devel. Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput v2] evdev: Log evdev event queue overflows
Hi On Wed, Oct 29, 2014 at 3:56 PM, Derek Foreman der...@osg.samsung.com wrote: Log a message when the kernel event queue overflows and events are dropped. After 10 messages logging stops to avoid flooding the logs if the condition is persistent. --- src/evdev.c | 11 +++ src/evdev.h | 4 2 files changed, 15 insertions(+) diff --git a/src/evdev.c b/src/evdev.c index 1b4ce10..9026f5c 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -924,6 +924,17 @@ evdev_device_dispatch(void *data) rc = libevdev_next_event(device-evdev, LIBEVDEV_READ_FLAG_NORMAL, ev); if (rc == LIBEVDEV_READ_STATUS_SYNC) { + if (device-syn_drops_received 10) { + device-syn_drops_received++; + log_info(libinput, SYN_DROPPED event from +\%s\ - some input events have +been lost.\n, device-devname); + if (device-syn_drops_received == 10) + log_info(libinput, No longer logging +SYN_DROPPED events for +\%s\\n, device-devname); + } + I really appreciate those log-messages, but can we use rate-limiting here, rather than a per-device counter? I mean, my compositor usually runs for multiple days, or even weeks. I really don't want it to stop reporting SYN_DROPPED events just because it got 100 of those during startup (or something similar). Something like this: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/lib/ratelimit.c I can prep a patch if you want. Thanks David /* send one more sync event so we handle all currently pending events before we sync up to the current state */ diff --git a/src/evdev.h b/src/evdev.h index c0d6577..9e84623 100644 --- a/src/evdev.h +++ b/src/evdev.h @@ -135,6 +135,10 @@ struct evdev_device { /* Checks if buttons are down and commits the setting */ void (*change_to_left_handed)(struct evdev_device *device); } buttons; + + /* The number of times libevdev processes a SYN_DROPPED, so we can +* stop logging them to avoid flooding the logs. */ + int syn_drops_received; }; #define EVDEV_UNHANDLED_DEVICE ((struct evdev_device *) 1) -- 2.1.1 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 0/4] some acceleration fixes, mostly for high DPI mice
Hi On Fri, Oct 31, 2014 at 5:33 AM, Peter Hutterer peter.hutte...@who-t.net wrote: On Thu, Oct 30, 2014 at 04:34:12PM -0500, Derek Foreman wrote: The acceleration filter currently isn't particularly pleased with gaming mice. They generally have high DPI (can be over 8000 DPI) and can have high update rates (1000+ per second). This can result in the accel curve being biased heavily towards the high points on the accel curve. This patch set allows a way to normalize the deltas to 400DPI so when the DPI setting of the mouse is correct, it should feel much the same as a normal mouse. This is, of course, only a partial solution. Setting a reasonable default is a massive problem that needs to be addressed in the future - for now we just set it to 400 (which may actually not be that prevalent any more but there doesn't seem to be such thing as a standard DPI mouse). Thanks, I've pushed 1 to 3 (with added comments), not quite happy with 4 yet. As a configuration interface, it's mismatched with the usual quartett of hooks that we provide for all other config options. But I think providing a visible API may not be the right approach here anyway. One idea I had here was to have this provided through udev, set as something like: LIBINPUT_MOUSE_DPI=400@80 # dpi @ poll rate over time, the udev hwdb could add those settings, or we ship them as extra rules, or users configure them themselves. this is unfortunately a rather nasty task given the amount of HW out there, but I really can't think of any other way (short of heuristics which will fail in too many ways). Any ideas on feasability or better approaches welcome. hwdb can be used easily here, but it's indeed a huge amount to gather all that data. HID doesn't have a way to query this information, sadly, and I haven't found any generic vendor extensions (maybe Benjamin knows more). We could use the REL_X/REL_Y MAX values to guess the DPI setting and ignore it if it's out of the expected range (sth. like 100-10k). I haven't spent much time thinking it through, but so far I'd prefer a solid, but basic, heuristic to guess the DPI and then use hwdb for anything that doesn't fit. This allows us to change the heuristic at any time and we don't have to introduce any APIs. We can even ship the hwdb files with libinput. Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libxkbcommon 2/4] compose: add xkbcommon-compose - implementation
Hi On Sun, Sep 14, 2014 at 11:05 PM, Ran Benita ran...@gmail.com wrote: Signed-off-by: Ran Benita ran...@gmail.com --- [snip] diff --git a/src/compose/state.c b/src/compose/state.c new file mode 100644 index 000..06e4ce5 --- /dev/null +++ b/src/compose/state.c @@ -0,0 +1,196 @@ +/* + * Copyright © 2013 Ran Benita ran...@gmail.com + * + * 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 table.h +#include utils.h +#include keysym.h + +struct xkb_compose_state { +int refcnt; +enum xkb_compose_state_flags flags; +struct xkb_compose_table *table; + +/* + * Offsets into xkb_compose_table::nodes. + * + * They maintain the current and previous position in the trie; see + * xkb_compose_state_feed(). + * + * This is also sufficient for inferring the current status; see + * xkb_compose_state_get_status(). + */ +uint32_t prev_context; +uint32_t context; +}; + +XKB_EXPORT struct xkb_compose_state * +xkb_compose_state_new(struct xkb_compose_table *table, + enum xkb_compose_state_flags flags) +{ +struct xkb_compose_state *state; + +state = calloc(1, sizeof(*state)); +if (!state) +return NULL; + +state-refcnt = 1; +state-table = xkb_compose_table_ref(table); + +state-flags = flags; +state-prev_context = 0; +state-context = 0; + +return state; +} + +XKB_EXPORT struct xkb_compose_state * +xkb_compose_state_ref(struct xkb_compose_state *state) +{ +state-refcnt++; +return state; +} + +XKB_EXPORT void +xkb_compose_state_unref(struct xkb_compose_state *state) +{ +if (!state || --state-refcnt 0) +return; + +xkb_compose_table_unref(state-table); +free(state); +} + +XKB_EXPORT struct xkb_compose_table * +xkb_compose_state_get_compose_table(struct xkb_compose_state *state) +{ +return state-table; +} + +XKB_EXPORT enum xkb_compose_feed_result +xkb_compose_state_feed(struct xkb_compose_state *state, xkb_keysym_t keysym) +{ +uint32_t context; +const struct compose_node *node; + +/* + * Modifiers do not affect the sequence directly. In particular, + * they do not cancel a sequence; otherwise it'd be impossible to + * have a sequence like dead_acuteA (needs Shift in the middle). + * + * The following test is not really accurate - in order to test if + * a key is modifier key, we really need the keymap, but we don't + * have it here. However, this is (approximately) what libX11 does + * as well. + */ +if (xkb_keysym_is_modifier(keysym)) +return XKB_COMPOSE_FEED_IGNORED; Why don't we require a keymap in xkb_compose_state_new()? Should be easy to do and we can then track modifiers reliably. Ok, a keysym which is a modifier in one keymap should never be a normal key in another.. unlike keycodes. But I don't see why we need to rely on that when we can just take a keymap. Otherwise, looks good (I didn't read through the parser, though). Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libxkbcommon 1/4] compose: add xkbcommon-compose - API
Hi On Sun, Sep 14, 2014 at 11:05 PM, Ran Benita ran...@gmail.com wrote: xkbcommon-compose is a Compose implementation for xkbcommon. It mostly behaves like libX11's Compose, but the support is somewhat low-level and is not transparent like in libX11. The user must add some supporting code in order to utilize it. The intended audience are users who use xkbcommon but not a full-blown input method. With this they can add Compose support in a straightforward manner, so they have a fairly complete keyboard input for Latin-like languages at least. See the header documentation for details. Signed-off-by: Ran Benita ran...@gmail.com --- xkbcommon/xkbcommon-compose.h | 457 ++ 1 file changed, 457 insertions(+) create mode 100644 xkbcommon/xkbcommon-compose.h diff --git a/xkbcommon/xkbcommon-compose.h b/xkbcommon/xkbcommon-compose.h new file mode 100644 index 000..ed35250 --- /dev/null +++ b/xkbcommon/xkbcommon-compose.h @@ -0,0 +1,457 @@ +/* + * Copyright © 2013 Ran Benita + * + * 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. + */ + +#ifndef _XKBCOMMON_COMPOSE_H +#define _XKBCOMMON_COMPOSE_H + +#include xkbcommon/xkbcommon.h + +#ifdef __cplusplus +extern C { +#endif + +/** + * @file + * libxkbcommon Compose API - support for Compose and dead-keys. + */ + +/** + * @defgroup compose Compose and dead-keys support + * Support for Compose and dead-keys. + * @since TBD + * + * @{ + */ + +/** + * @page compose-overview Overview + * @parblock + * + * Compose and dead-keys are a common feature of many keyboard input + * systems. They extend the range of the keysysm that can be produced + * directly from a keyboard by using a sequence of key strokes, instead + * of just one. + * + * Here are some example sequences, in the libX11 Compose file format: + * + * dead_acute a : á aacute # LATIN SMALL LETTER A WITH ACUTE + * Multi_key A T : @ at # COMMERCIAL AT + * + * When the user presses a key which produces the \dead_acute keysym, + * nothing initially happens (thus the key is dubbed a dead-key). But + * when the user enters a, á is composed, in place of a. If + * instead the user had entered a keysym which does not follow + * \dead_acute\ in any compose sequence, the sequence is said to be + * cancelled. + * + * Compose files define many such sequences. For a description of the + * common file format for Compose files, see the Compose(5) man page. + * + * A successfuly-composed sequence has two results: a keysym and a UTF-8 + * string. At least one of the two is defined for each sequence. If only + * a keysym is given, the keysym's string representation is used for the + * result string (using xkb_keysym_to_utf8()). + * + * This library provides low-level support for Compose file parsing and + * processing. Higher-level APIs (such as libX11's Xutf8LookupString(3)) + * may be built upon it, or it can be used directly. + * + * @endparblock + */ + +/** + * @page compose-conflicting Conflicting Sequences + * @parblock + * + * To avoid ambiguity, a sequence is not allowed to be a prefix of another. + * In such a case, the conflict is resolved thus: + * + * 1. A longer sequence overrides a shorter one. + * 2. An equal sequence overrides an existing one. + * 3. A shorter sequence does not override a longer one. + * + * Sequences of length 1 are allowed, although they are not common. + * + * @endparblock + */ + +/** + * @page compose-cancellation Cancellation Behavior + * @parblock + * + * What should happen when a sequence is cancelled? For example, consider + * there are only the above sequences, and the input kesysms are + * \dead_acute\ \b\. There are a few approaches: + * + * 1. Swallow the cancelling keysym; that is, no keysym is produced. + *This is the
Re: [PATCH libxkbcommon 1/4] compose: add xkbcommon-compose - API
Hi On Mon, Sep 15, 2014 at 1:48 PM, Ran Benita ran...@gmail.com wrote: On Mon, Sep 15, 2014 at 08:41:37AM +0200, David Herrmann wrote: Hi Hi David On Sun, Sep 14, 2014 at 11:05 PM, Ran Benita ran...@gmail.com wrote: [snip] +/** + * @page compose-cancellation Cancellation Behavior + * @parblock + * + * What should happen when a sequence is cancelled? For example, consider + * there are only the above sequences, and the input kesysms are + * \dead_acute\ \b\. There are a few approaches: + * + * 1. Swallow the cancelling keysym; that is, no keysym is produced. + *This is the approach taken by libX11. + * 2. Let the cancelling keysym through; that is, \b\ is produced. + * 3. Replay the entire sequence; that is, \dead_acute\ \b\ is produced. + *This is the approach taken by Microsoft Windows (approximately; + *instead of \dead_acute\, the underlying key is used. This is + *difficult to simulate with XKB keymaps). + * + * You can program whichever approach best fits users' expectations. Hm, implementing 3) is a pain as we have to track the keysyms separately. Your compose-API does not provide a way to retrieve the parsed/failed sequence. I think alternative 3 is the nicest really, so I want to make it possible, without too much work if possible. I agree! Tracking the sequence is possible - I've added a return value to xkb_compose_state_feed() for this purpose. It can be done with e.g. a wrapper over xkb_compose_state_feed(), something like: Yes, sure, but I wanted to avoid tracking it separately. We could just climb up the compose-trie after we cancelled it and recreate the list of keysyms? If that's not possible, tracking it separately should be fine. It isn't that much work.. But given that we have no dead-key = normal-key conversion right now, it's probably fine. If we want it, we can add an API for both later on (assuming a trivial keysym conversion from dead_key = normal is possible). Yes, maybe we can add such a function, once we have more experience. Another option, which relies entirely on convention, is to feed the dead key twice, and see what comes out: $ grep -P '^dead_(.*) dead_\1' /usr/share/X11/locale/en_US.UTF-8/Compose dead_tilde dead_tilde : ~ asciitilde # TILDE dead_acute dead_acute : ´ acute # ACUTE ACCENT dead_grave dead_grave : ` grave # GRAVE ACCENT dead_circumflex dead_circumflex : ^ asciicircum # CIRCUMFLEX ACCENT dead_abovering dead_abovering : ° degree # DEGREE SIGN dead_macron dead_macron : ¯ macron # MACRON dead_breve dead_breve : ˘ breve # BREVE dead_abovedot dead_abovedot : ˙ abovedot # DOT ABOVE dead_diaeresis dead_diaeresis : ¨ diaeresis # DIAERESIS dead_doubleacute dead_doubleacute : ˝ U2dd # DOUBLE ACUTE ACCENT dead_caron dead_caron : ˇ caron # CARON dead_cedilla dead_cedilla : ¸ cedilla # CEDILLA dead_ogonek dead_ogonek : ˛ ogonek # OGONEK dead_iota dead_iota : ͺ U37a # GREEK YPOGEGRAMMENI dead_belowdot dead_belowdot : ̣ U0323 # COMBINING DOT BELOW dead_belowcomma dead_belowcomma : , comma # COMMA dead_currency dead_currency : ¤ currency # CURRENCY SIGN dead_greek dead_greek : µ U00B5 # MICRO SIGN dead_hook dead_hook : ̉ U0309 # COMBINING HOOK ABOVE dead_horn dead_horn : ̛ U031B # COMBINING HORN dead_stroke dead_stroke : / slash # SOLIDUS Probably not a good idea.. Ewww! +/** Status of the Compose sequence state machine. */ +enum xkb_compose_status { +/** The initial state; no sequence has started yet. */ +XKB_COMPOSE_NOTHING, +/** In the middle of a sequence. */ +XKB_COMPOSE_COMPOSING, +/** A complete sequence has been matched. */ +XKB_COMPOSE_COMPOSED, +/** The last sequence was cancelled due to an invalid keysym. */ +XKB_COMPOSE_CANCELLED It is unclear what happens if a keysym is pressed that is _not_ part of a compose sequence (that is, most keys). 'context' is 0 but no matching compose node is found. I assume it generates XKB_COMPOSE_NOTHING, but the comment here is unclear. Maybe the _feed() or _get_state() description should mention how keys are treated that are not part of compose sequences (and which are fed while no compose sequence is active). I assume we do *not* return XKB_COMPOSE_COMPOSED in those cases? Exactly, it is NOTHING, not COMPOSED. I'll make this more clear. Thanks! If you get multiple keysyms, you should not feed them, because this is not something that the current Compose format expects or supports. I'll mention that. The way to use multiple-keysysm is still up to interpretation I guess, since it hasn't been used yet. But my notion
Re: [PATCH libxkbcommon 2/4] compose: add xkbcommon-compose - implementation
Hi On Mon, Sep 15, 2014 at 1:58 PM, Ran Benita ran...@gmail.com wrote: On Mon, Sep 15, 2014 at 08:21:34AM +0200, David Herrmann wrote: Why don't we require a keymap in xkb_compose_state_new()? Should be easy to do and we can then track modifiers reliably. Ok, a keysym which is a modifier in one keymap should never be a normal key in another.. unlike keycodes. But I don't see why we need to rely on that when we can just take a keymap. I prefer not adding a dependency on a keymap just for this one little thing. That would preclude people who don't use xkbcommon keymaps from using xkbcommon-compose (not likely, but maybe), and complicate the API slightly. For not much gain.. We could allow NULL and then use this fallback.. Also, that's what the de facto specification here (Xlib) says, so it's not a bug, it's a feature :) Fair enough. You might guess that I don't care much about Xlib, but this is just a nitpick so I'm fine either way. Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] Wrong bo handles can be referenced in func call, drmModeAddFB2 due to uninitialized array elements in handles[4]
Hi On Tue, Apr 29, 2014 at 10:59 PM, Matt Roper matthew.d.ro...@intel.com wrote: On Tue, Apr 29, 2014 at 01:24:31PM -0700, Kristian Høgsberg wrote: On Mon, Apr 28, 2014 at 03:26:18PM -0700, Dongwon Kim wrote: Need all bo handles in the array, handles[4] to be initialized with integer value that indicates NOT VALID handle to prevent any of those uninitialized from being processed as VALID handles. The format code determines which bo handles are used. For non-planar formats only the first handle is used and the rest can be uninitialized. Kristian Some hardware allows/expects stereo 3D to be supported by programming the hardware with separate buffers for the left eye and right eye content. Drivers for such hardware need both handles[0] and handles[1] to pass the left and right content buffers that get wrapped into a single DRM fb, even though the pixel format is just RGB. At the time AddFB2 gets called, you don't know yet whether it's going to get displayed on a stereo mode or a mono mode, so if there's something sitting in handles[1], the driver will try to look it up. You cannot break backwards-compat this way. A lot of user-space assumes that only handles[0] is used. Afaik, the kernel has a 3d-stereo user-capability, so unless it is set, you shouldn't do that in the kernel. However, given that weston might set this flag at some point, the patch is still right. Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [ANNOUNCE] libxkbcommon-0.4.1
Hi On Thu, Mar 27, 2014 at 11:41 PM, Ran Benita ran...@gmail.com wrote: Regarding intended use-case for multiple-keysyms, I consider it mainly to be for sequences with combining characters - not everything has precomposed codepoints, so if you want one of these, you don't have a way to do it with single-keysym, but it still conceptually fits in a keymap. However for the original intent you have to ask Daniel. Ok, so should be handled as atomic key-press. Sure, the old functions are still useful for getting the raw translation if you want it. And you're right about the docs - I'll add some hopefully-not-too-confusing details instead of just prefer the new ones. Full details are in the bugs and commit msgs but of course I don't expect anyone to read that :) Yeah, commit-message is great for xkbcommon developers, not so much for new API users. Thanks! Btw., same is true for the implicit caps-lock magic in xkb_state_key_get_one_sym(). I'm now quite confused whether xkb_state_key_get_syms() users have to do caps-lock handling explicitly? Or is that done by keymaps? Currently to get the implicit capitalization with get_syms(), you have to do this: int nsyms; const xkb_keysym_t *syms; xkb_keysym_t sym; nsyms = xkb_state_key_get_syms(..., syms); if (nsyms == 1) { sym = xkb_state_key_get_one_sym(...); syms = sym; } I imagine the disgust, but given the set of constraints we had I couldn't think of any way to make this work. New API is still possible, but then we'd have *three* ways to get keysyms... As I may have mentioned, I wanted to change the *keymaps* so the difference doesn't matter here. I still plan to send some patches for easy cases, but fixing other cases would require major xkeyboard-config surgery. So we're stuck with it. That somehow gives me the impression capslock is currently not handled by keymaps? Oh, didn't even notice that. Sounds like a gross hack. But yes, fixing all keymaps sounds like rather cumbersome work given that there's no real gain in it. Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [ANNOUNCE] libxkbcommon-0.4.1
Hi On Thu, Mar 27, 2014 at 8:22 PM, Ran Benita ran...@gmail.com wrote: - Added two new functions, xkb_state_key_get_utf{8,32}(). They combine the operations of xkb_state_key_get_syms() and xkb_keysym_to_utf{8,32}(), and provide a nicer interface for it (espcially for multiple-keysyms-per-level). Slightly off-topic, but looking at the code, you ignore any multi-sym values (nsyms != 1). Users of that API might rely on that behavior, so have we at some point defined what exactly multi-sym means? Because if we add more and more APIs and if the user-base grows, we might at some point be unable to make use of multi-symbol behavior. I'm still not sure whether nsyms 1 just means multiple sequential keysyms, or whether they should be handled as _one_ atomic combined keysym? - The xkb_state_key_get_utf{8,32}() functions now apply Control transformation: when the Control modifier is active, the string is converted to an appropriate control character. This matches the behavior of libX11's XLookupString(3), and required by the XKB specification: http://www.x.org/releases/current/doc/kbproto/xkbproto.html#Interpreting_the_Control_Modifier https://bugs.freedesktop.org/show_bug.cgi?id=75892 So xkb_state_key_get_utf8(state, code) != xkb_keysym_to_utf8(xkb_state_key_get_one_sym(state, code))? (at least for ctrl+char) Could we at least add a note/warning to the documentation? I cannot find anything here: http://xkbcommon.org/doc/current/group__state.html#ga0774b424063b45c88ec0354c77f9a247 For instance, for libtsm, I accept combined modifier+keysym+ucs4string input from the GUI layer and transform that internally. If the upper layer uses xkb_state_key_get_utf32() instead of xkb_keysym_to_utf32(), the logic will force the control-seq even though it might be mapped differently by the vte-layer (for instance, there's stuff like shifting values by 64 if ALT is pressed, and more legacy crap we need to support for eternity). Sure, most libtsm code uses the keysym, but for glyph transformation we have to use the ucs4 value. Imagine the ctrl+c input was shifted by the term-layer, you still end up with the \003 glyph, because the ucs4 string was mapped by xkbcommon. I'm not saying the patch is wrong, in fact, the layout-search logic is actually what you wrote for kmscon and I appreciate that. I'm just saying that it's a really subtle API difference that can introduce weird bugs. Lets see how it works out, but if people start using xkb_state_key_get_utf32(), I might send a patch adding an xkb-state flag that disables this transformation. Or just force users to not use it (which would be unfortunate). Btw., same is true for the implicit caps-lock magic in xkb_state_key_get_one_sym(). I'm now quite confused whether xkb_state_key_get_syms() users have to do caps-lock handling explicitly? Or is that done by keymaps? Anyhow, thanks for the release! David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: How synchronisation works in Framebuffer backend
Hi On Sat, Mar 22, 2014 at 6:33 PM, Sannu K sannumail4f...@gmail.com wrote: Hi, I was going through weston's framebuffer backend code. It does not call FBIO_WAITFORVSYNC ioctl. http://cgit.freedesktop.org/wayland/weston/tree/src/compositor-fbdev.c#n188 says that it is not used intentionally. In this case, how weston syncs frame with the framebuffer clock? How tearing is avoided? Thanks in advance for the help. It doesn't. Don't use fbdev if you want vsync. Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [RFC PATCH libinput] udev: add libinput_udev_rescan_devices()
Hi On Fri, Mar 21, 2014 at 6:14 AM, Peter Hutterer peter.hutte...@who-t.net wrote: On Fri, Mar 21, 2014 at 12:27:45AM -0400, Jasper St. Pierre wrote: So you're saying that every time we're suspended, we simply throw out the entire context and drop all the devices on the floor, as if you just unplugged all of them? fwiw, this is effectively what happens internally anyway, you get removed events for every device on suspend, and added events on resume for those still there. I suppose I just never thought of that. On first though, I don't see anything wrong with that. If that's what we should do, should we remove libinput_suspend / libinput_resume then? libinput_suspend/resume only tear down the devices, but not anything else. there isn't much global state that's kept across suspend/resume yet (seats are one example though) but the biggest difference is that that you can't use _any_ object around after libinput_destroy(). suspend/resume keeps them alive until you unref them. Just to verify the assumptions: Yes, the kernel throws away the whole context, but in user-space you can try to keep your devices around and just resync them on resume. At least that's what I did. And use the real udev-events to decide when the device has really gone away. Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Announcing Termistor, a drop-down, tabbed, terminal for Wayland
Hi On Thu, Dec 26, 2013 at 10:47 PM, Giulio Camuffo giuliocamu...@gmail.com wrote: Hi all, I've finally found a way to squeeze good performances out of my terminal embryo I've had for a few months, so I hereby announce the birth of the first drop-down (optionally), tabbed, terminal for Wayland! Obligatory screenshoot: http://i.imgur.com/OpOvxHK.png It's a Qt5 client, and uses libtsm for the background work. Given Wayland clients don't know where they are nor they can tell the compositor where they want to be I came up with a draft of a protocol[0] where the client sends a surface and the server then arranges for showing and hiding it. There are some things yet to solve though. The biggest one is the binding to show/hide the surface. Like, should the client or the server dictate what binding to use? Should it allow many drop-down clients to exist at the same time? To be visible at the same time? I've implemented the protocol in my shell plugin for Weston, 'Nuclear'[1], with an hardcoded F11 binding. Termistor is still lacking many features, the selection for one, but this is yet another step to ditching X for me. Nice to see this getting somewhere, looks quite promising! If you have questions regarding libtsm, let me know. I have skimmed through your code and the Screen object is actually not needed anymore. Since libtsm-3, we have ageing support in libtsm so it only renders changed lines. I rewrote wlterm [1] to use gtk+ and it now serves as minimal libtsm example (in case you want some reference code). I'm looking forward to further improvements. If something's missing, feel free to send me patches. Thanks David [1] wlterm: http://www.freedesktop.org/wiki/Software/kmscon/wlterm/ ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] logind: delay wakeup until DRM-device is resumed
Hi On Tue, Dec 3, 2013 at 10:42 PM, Kristian Høgsberg k...@bitplanet.net wrote: On Sat, Nov 30, 2013 at 2:25 AM, David Herrmann dh.herrm...@gmail.com wrote: The logind API was designed to allow any kind of devices and any number of devices. It has no idea of main DRM device or similar. However, the weston DRM backend was designed with a single DRM device as master. Therefore, we wake it up unconditionally on session-wakeup. But this may fail with logind as a session may be awake, but not all devices have been resumed, yet. Therefore, we change the weston-logind backend to deal with this case correctly. Instead of waking up the compositor on session-wakeup, we wait for the main DRM device to wake up. Once we get the event, we notify the compositor. For sleep, we reverse this logic. On *any* of the following events we tell the compositor to go to sleep: - Session gets inactive - DRM device gets inactive - DRM device is removed This guarantees, that weston is only active if *both*, the session and the main DRM device are awake/active. Note that we could actually rely solely on the DRM-device Pause/Resume events from logind and drop all the Active-Prop-Changed handling. logind guarantees proper ordering of both. However, in case we ever change weston to support multiple GPUs, we need the per-device notification. Thus, keep the code. This also makes weston more fail-safe in case logind fails to send the PauseDevice event (for whatever reason..). This explains the intermittent pageflip failures I was seeing. The patch makes sense and makes VT switching work almost reliably. The one missing thing is that occasionally my pointer doesn't work. It looks like it remains locked by the X driver, and since it's only my touchpad (keyboard always works) I'm guessing it's a xf86-input-synaptics problem. It happens with weston-launch too, but it doesn't look like a race. I can reproduce with just VT switching from X to a VT and running evtest. That is, the device remains locked long enough for me to type evtest, select the device and then see the Device is grabbed... message. It doesn't get ungrabbed until I VT switch back to X and then back to the VT. At this point I'm calling it a synaptics bug and merging your patch. You can set: Option GrabEventDevice no in the synaptics xorg config, btw. I tried reading through the synaptics code but couldn't find anything related to that bug. Hmm.. but I'm not really familiar with the xinput drivers code base.. Thanks for merging! Now everything should be in-place of the logind work. A bunch of changes for CONFIG_VT=n will be needed, but I'm working on that. David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH] logind: delay wakeup until DRM-device is resumed
The logind API was designed to allow any kind of devices and any number of devices. It has no idea of main DRM device or similar. However, the weston DRM backend was designed with a single DRM device as master. Therefore, we wake it up unconditionally on session-wakeup. But this may fail with logind as a session may be awake, but not all devices have been resumed, yet. Therefore, we change the weston-logind backend to deal with this case correctly. Instead of waking up the compositor on session-wakeup, we wait for the main DRM device to wake up. Once we get the event, we notify the compositor. For sleep, we reverse this logic. On *any* of the following events we tell the compositor to go to sleep: - Session gets inactive - DRM device gets inactive - DRM device is removed This guarantees, that weston is only active if *both*, the session and the main DRM device are awake/active. Note that we could actually rely solely on the DRM-device Pause/Resume events from logind and drop all the Active-Prop-Changed handling. logind guarantees proper ordering of both. However, in case we ever change weston to support multiple GPUs, we need the per-device notification. Thus, keep the code. This also makes weston more fail-safe in case logind fails to send the PauseDevice event (for whatever reason..). --- src/logind-util.c | 58 +++ 1 file changed, 37 insertions(+), 21 deletions(-) diff --git a/src/logind-util.c b/src/logind-util.c index 5d163a6..40d811b 100644 --- a/src/logind-util.c +++ b/src/logind-util.c @@ -44,6 +44,8 @@ #include dbus.h #include logind-util.h +#define DRM_MAJOR 226 + #ifndef KDSKBMUTE #define KDSKBMUTE 0x4B51 #endif @@ -275,10 +277,10 @@ weston_logind_activate_vt(struct weston_logind *wl, int vt) static void weston_logind_set_active(struct weston_logind *wl, bool active) { - if (active) - wl-compositor-session_active = 1; - else - wl-compositor-session_active = 0; + if (!wl-compositor-session_active == !active) + return; + + wl-compositor-session_active = active; wl_signal_emit(wl-compositor-session_signal, wl-compositor); @@ -314,7 +316,8 @@ get_active_cb(DBusPendingCall *pending, void *data) goto err_unref; dbus_message_iter_get_basic(sub, b); - weston_logind_set_active(wl, b); + if (!b) + weston_logind_set_active(wl, false); err_unref: dbus_message_unref(m); @@ -428,7 +431,8 @@ property_changed(struct weston_logind *wl, DBusMessage *m) if (!strcmp(name, Active)) { if (dbus_message_iter_get_arg_type(entry) == DBUS_TYPE_BOOLEAN) { dbus_message_iter_get_basic(entry, b); - weston_logind_set_active(wl, b); + if (!b) + weston_logind_set_active(wl, false); return; } } @@ -478,31 +482,43 @@ device_paused(struct weston_logind *wl, DBusMessage *m) /* pause means synchronous pausing. Acknowledge it unconditionally * as we support asynchronous device shutdowns, anyway. -* force means asynchronous pausing. We ignore it as the following -* session-deactivation will suffice as notification. -* gone means the device is gone. We ignore it as we receive a -* udev notification, anyway. */ +* force means asynchronous pausing. +* gone means the device is gone. We handle it the same as force as +* a following udev event will be caught, too. +* +* If it's our main DRM device, tell the compositor to go asleep. */ if (!strcmp(type, pause)) weston_logind_pause_device_complete(wl, major, minor); + + if (major == DRM_MAJOR) + weston_logind_set_active(wl, false); } static void device_resumed(struct weston_logind *wl, DBusMessage *m) { - /* -* DeviceResumed messages provide us a new file-descriptor for + bool r; + uint32_t major; + + r = dbus_message_get_args(m, NULL, + DBUS_TYPE_UINT32, major, + /*DBUS_TYPE_UINT32, minor, + DBUS_TYPE_UNIX_FD, fd,*/ + DBUS_TYPE_INVALID); + if (!r) { + weston_log(logind: cannot parse ResumeDevice dbus signal\n); + return; + } + + /* DeviceResumed messages provide us a new file-descriptor for * resumed devices. For DRM devices it's the same as before, for evdev * devices it's a new open-file. As we reopen evdev devices, anyway, -* there is no need for us to handle this event. If we ever optimize -* our evdev code to support resuming devices, this
Re: [PATCH] logind: Use dbus_bool_t for bool types in dbus calls
Hi On Thu, Nov 21, 2013 at 1:39 AM, Kristian Høgsberg k...@bitplanet.net wrote: The gcc built-in 'bool' type is not the same size as dbus_bool_t, which is an uint32_t. Passing a pointer to bool where dbus expects a uint32_t * doesn't work. Nice catch! I hate dbus for that type.. happens to me all the time. Reviewed-by: David Herrmann dh.herrm...@gmail.com --- src/logind-util.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) David, This fixes a crash that Artie hit when testing this. Btw, did you get a chance to update the logind error paths to return -1 and set errno like we discussed? I tried that, but systemd and dbus both return negative error codes instead of errno+-1. So I have to change like 90 places to do: errno = -r; r = -1; I can do that, but I thought it looked ugly. As a compromise, I can only make the public logind-functions do that in the error-path? That would be only 5 places. Comments? Thanks David diff --git a/src/logind-util.c b/src/logind-util.c index 6bd0c26..a58265c 100644 --- a/src/logind-util.c +++ b/src/logind-util.c @@ -69,8 +69,9 @@ weston_logind_take_device(struct weston_logind *wl, uint32_t major, uint32_t minor, bool *paused_out) { DBusMessage *m, *reply; - bool b, paused; + bool b; int r, fd; + dbus_bool_t paused; m = dbus_message_new_method_call(org.freedesktop.login1, wl-spath, @@ -287,7 +288,7 @@ get_active_cb(DBusPendingCall *pending, void *data) DBusMessage *m; DBusMessageIter iter, sub; int type; - bool b; + dbus_bool_t b; dbus_pending_call_unref(wl-pending_active); wl-pending_active = NULL; -- 1.8.3.1 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [RFC] clients: add simple-dmabuf
Hi Axel On Mon, Oct 28, 2013 at 12:34 AM, Axel Davy d...@clipper.ens.fr wrote: On 22/10/2013 17:23, David Herrmann wrote : Btw., I got this working with i915 by allowing GEM_OPEN/GEM_FLINK on the render-node. So if someone else tests this, you might need the same hacks. I will try to find the code in mesa that requires this. David This comes from 'intel_region_alloc_for_handle', which is called in 'intel_process_dri2_buffer'. I've just talked with Christopher James Halse Rogers, and he already made patches that should solve that. The patches to support driImage version 7 were posted recently on Mesa, And his older patches (probably need rebasing): https://github.com/RAOF/mesa/commit/cc797647eaa0e59929eca09ad1c5936cab74144b https://github.com/RAOF/mesa/commit/1ff8df28f25f98b2e4a12f7fb3085b7d0adbfdf3 https://github.com/RAOF/mesa/commit/59e6361e9c76bb32c3fe96482ae0b31493dc3b3c Combined with setting the name field to null (instead of generating a name) and setting the fd field to a prime dma-buf when creating a buffer (in get_back_bo), it should avoid using names at all, and use dma-buf in Mesa, thus enabling using render-nodes with Mesa. Yepp, exactly these patches are needed! I don't know who it was I talked to in #intel-gfx, but some-one said he's working on it. Note that for dma-buf we're still missing allocation negotiation and sync fences. But these are also being worked on. With these patches (and your name=0; init thing) it works great. There're visible sync issues (really awful tearing, need to look into that), but apart from that it's fine. Regarding your earlier mail: We tried to _not_ support flink and friends on render-nodes. Yes, we could have provided some dummy ioctls to trick user-space, and yes, it would probably have worked well. But we intentionally tried to break the API to maybe some day be able to drop it. If we keep dummies, we never know who's still using it. If we don't, we simply say render-nodes are DRM-2.0 and be done with it. Thanks for your investigations! David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 2/2] compositor: finish frame if redraw fails
Hi On Tue, Oct 22, 2013 at 9:45 PM, Kristian Høgsberg hoegsb...@gmail.com wrote: On Tue, Oct 22, 2013 at 05:11:26PM +0200, David Herrmann wrote: If we are about to finish a frame, but a redraw is pending and we let the compositor redraw, we need to check for errors. If the redraw fails and the backend cannot schedule a page-flip, we need to finish the frame, anyway. All backends except DRM use a timer to schedule frames. Hence, they cannot fail. But for DRM, we need to be able to handle drmModePageFlip() failures in case access got revoked. This fixes a bug where logind+drm caused keyboard input to be missed as we didn't reenable it after a failed page-flip during deactivation. Yes, that's better, applied both of these patches. Aside from being unable to turn off sprites and hw cursor, the async deactivate also means that we can lose drm master at any time, in particular between starting repaint and pageflip. So we need to handle this better and I think these two patches covers it. There's one last thing that doesn't quite work yet - when I switch to weston, it doesn't immediately (re)set the current framebuffers. I have to trigger a repaint by moving the mouse or such. I'm not sure why that doesn't work, we still have the call to drm_compositor_set_modes() and that should run with drm master set and restore the last mode and buffer. It works for me. Does the log show anything? Thanks for testing! Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 1/2] compositor-drm: finish frame if initial page-flip fails
If the initial page-flip fails, immediately finish the frame to avoid being stuck in the given frame. We already do this if we have no fbo available. Now we do the same if the page-flip fails. --- src/compositor-drm.c | 19 +++ 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/compositor-drm.c b/src/compositor-drm.c index cbdbb3c..8a70674 100644 --- a/src/compositor-drm.c +++ b/src/compositor-drm.c @@ -674,7 +674,7 @@ drm_output_start_repaint_loop(struct weston_output *output_base) struct drm_compositor *compositor = (struct drm_compositor *) output_base-compositor; uint32_t fb_id; - + uint32_t msec; struct timespec ts; if (output-destroy_pending) @@ -682,12 +682,7 @@ drm_output_start_repaint_loop(struct weston_output *output_base) if (!output-current) { /* We can't page flip if there's no mode set */ - uint32_t msec; - - clock_gettime(compositor-clock, ts); - msec = ts.tv_sec * 1000 + ts.tv_nsec / 100; - weston_output_finish_frame(output_base, msec); - return; + goto finish_frame; } fb_id = output-current-fb_id; @@ -695,8 +690,16 @@ drm_output_start_repaint_loop(struct weston_output *output_base) if (drmModePageFlip(compositor-drm.fd, output-crtc_id, fb_id, DRM_MODE_PAGE_FLIP_EVENT, output) 0) { weston_log(queueing pageflip failed: %m\n); - return; + goto finish_frame; } + + return; + +finish_frame: + /* if we cannot page-flip, immediately finish frame */ + clock_gettime(compositor-clock, ts); + msec = ts.tv_sec * 1000 + ts.tv_nsec / 100; + weston_output_finish_frame(output_base, msec); } static void -- 1.8.4.1 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [RFC] clients: add simple-dmabuf
Hi On Mon, Oct 21, 2013 at 11:32 PM, David Herrmann dh.herrm...@gmail.com wrote: simple-dmabuf is an example client which shows how to write wayland clients that use render-nodes for hardware-accelerated rendering and pass the buffer via dmabuf to the compositor. No mesa/EGL extensions are needed! Instead we pass dmabufs as wl_shm buffers to the compositors. This allows us to use hardware-accelerated rendering even with fbdev or other backends. Hurray! This still contains some hacks and is more a proof-of-concept than something useful. However, it shows what render-nodes allow us to do without modifying the compositor at all. This example requires a 3.12-rc kernel with drm.rnodes=1 on the kernel command line (must be set during boot!). Furthermore, this currently only works with drivers that implement dmabuf-mmap (which is rcar-du, shmob, omapdrm and others). I have some hacks which implement it for i915 (but flushing seems to not work..). I haven't succeeded in making rendering work with i915. I currently have no access to other drivers so any further testing/debugging welcome. We either still miss some render-nodes ioctls that are needed by mesa (but I didn't get any mesa errors..) or my i915-dmabuf-mmap is just wrong. If someone gets this working on a specific card (working means non-black window), please let me know. Btw., I got this working with i915 by allowing GEM_OPEN/GEM_FLINK on the render-node. So if someone else tests this, you might need the same hacks. I will try to find the code in mesa that requires this. David --- clients/.gitignore | 1 + clients/Makefile.am | 7 +- clients/simple-dmabuf.c | 675 configure.ac| 2 +- 4 files changed, 683 insertions(+), 2 deletions(-) create mode 100644 clients/simple-dmabuf.c diff --git a/clients/.gitignore b/clients/.gitignore index 23959cc..b77b537 100644 --- a/clients/.gitignore +++ b/clients/.gitignore @@ -11,6 +11,7 @@ weston-image weston-nested weston-nested-client weston-resizor +weston-simple-dmabuf weston-simple-egl weston-simple-shm weston-simple-touch diff --git a/clients/Makefile.am b/clients/Makefile.am index 4f9dc48..39dca7f 100644 --- a/clients/Makefile.am +++ b/clients/Makefile.am @@ -56,11 +56,16 @@ endif if BUILD_SIMPLE_EGL_CLIENTS simple_egl_clients_programs = \ - weston-simple-egl + weston-simple-egl \ + weston-simple-dmabuf weston_simple_egl_SOURCES = simple-egl.c weston_simple_egl_CPPFLAGS = $(SIMPLE_EGL_CLIENT_CFLAGS) weston_simple_egl_LDADD = $(SIMPLE_EGL_CLIENT_LIBS) -lm + +weston_simple_dmabuf_SOURCES = simple-dmabuf.c +weston_simple_dmabuf_CPPFLAGS = $(SIMPLE_EGL_CLIENT_CFLAGS) +weston_simple_dmabuf_LDADD = $(SIMPLE_EGL_CLIENT_LIBS) -lm endif if BUILD_CLIENTS diff --git a/clients/simple-dmabuf.c b/clients/simple-dmabuf.c new file mode 100644 index 000..570c929 --- /dev/null +++ b/clients/simple-dmabuf.c @@ -0,0 +1,675 @@ +/* + * Copyright © 2012-2013 David Herrmann dh.herrm...@gmail.com + * + * Permission to use, copy, modify, distribute, and sell this software and its + * documentation for any purpose is hereby granted without fee, provided that + * the above copyright notice appear in all copies and that both that copyright + * notice and this permission notice appear in supporting documentation, and + * that the name of the copyright holders not be used in advertising or + * publicity pertaining to distribution of the software without specific, + * written prior permission. The copyright holders make no representations + * about the suitability of this software for any purpose. It is provided as + * is without express or implied warranty. + * + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE, + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE + * OF THIS SOFTWARE. + */ + +/* + * Simple dmabuf/render-nodes Client + * This is a simple example client which uses dmabuf to pass buffers via wl_shm + * (instead of wl_drm or other EGL extensions) to the compositor. To render the + * images, we use DRM render-nodes or software-rendering as fallback. + * + * Note that this is a very basic example how to use wl_shm to share buffers + * which were rendered using a dedicated GPU. No mesa/EGL internal extensions + * are required. + * However, this is *not* the recommended way to do it! The mesa/EGL extensions + * allow much better integration and control. This client serves as a + * proof-of-concept and generic HOWTO
[RFC] clients: add simple-dmabuf
simple-dmabuf is an example client which shows how to write wayland clients that use render-nodes for hardware-accelerated rendering and pass the buffer via dmabuf to the compositor. No mesa/EGL extensions are needed! Instead we pass dmabufs as wl_shm buffers to the compositors. This allows us to use hardware-accelerated rendering even with fbdev or other backends. Hurray! This still contains some hacks and is more a proof-of-concept than something useful. However, it shows what render-nodes allow us to do without modifying the compositor at all. This example requires a 3.12-rc kernel with drm.rnodes=1 on the kernel command line (must be set during boot!). Furthermore, this currently only works with drivers that implement dmabuf-mmap (which is rcar-du, shmob, omapdrm and others). I have some hacks which implement it for i915 (but flushing seems to not work..). I haven't succeeded in making rendering work with i915. I currently have no access to other drivers so any further testing/debugging welcome. We either still miss some render-nodes ioctls that are needed by mesa (but I didn't get any mesa errors..) or my i915-dmabuf-mmap is just wrong. If someone gets this working on a specific card (working means non-black window), please let me know. --- clients/.gitignore | 1 + clients/Makefile.am | 7 +- clients/simple-dmabuf.c | 675 configure.ac| 2 +- 4 files changed, 683 insertions(+), 2 deletions(-) create mode 100644 clients/simple-dmabuf.c diff --git a/clients/.gitignore b/clients/.gitignore index 23959cc..b77b537 100644 --- a/clients/.gitignore +++ b/clients/.gitignore @@ -11,6 +11,7 @@ weston-image weston-nested weston-nested-client weston-resizor +weston-simple-dmabuf weston-simple-egl weston-simple-shm weston-simple-touch diff --git a/clients/Makefile.am b/clients/Makefile.am index 4f9dc48..39dca7f 100644 --- a/clients/Makefile.am +++ b/clients/Makefile.am @@ -56,11 +56,16 @@ endif if BUILD_SIMPLE_EGL_CLIENTS simple_egl_clients_programs = \ - weston-simple-egl + weston-simple-egl \ + weston-simple-dmabuf weston_simple_egl_SOURCES = simple-egl.c weston_simple_egl_CPPFLAGS = $(SIMPLE_EGL_CLIENT_CFLAGS) weston_simple_egl_LDADD = $(SIMPLE_EGL_CLIENT_LIBS) -lm + +weston_simple_dmabuf_SOURCES = simple-dmabuf.c +weston_simple_dmabuf_CPPFLAGS = $(SIMPLE_EGL_CLIENT_CFLAGS) +weston_simple_dmabuf_LDADD = $(SIMPLE_EGL_CLIENT_LIBS) -lm endif if BUILD_CLIENTS diff --git a/clients/simple-dmabuf.c b/clients/simple-dmabuf.c new file mode 100644 index 000..570c929 --- /dev/null +++ b/clients/simple-dmabuf.c @@ -0,0 +1,675 @@ +/* + * Copyright © 2012-2013 David Herrmann dh.herrm...@gmail.com + * + * Permission to use, copy, modify, distribute, and sell this software and its + * documentation for any purpose is hereby granted without fee, provided that + * the above copyright notice appear in all copies and that both that copyright + * notice and this permission notice appear in supporting documentation, and + * that the name of the copyright holders not be used in advertising or + * publicity pertaining to distribution of the software without specific, + * written prior permission. The copyright holders make no representations + * about the suitability of this software for any purpose. It is provided as + * is without express or implied warranty. + * + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE, + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE + * OF THIS SOFTWARE. + */ + +/* + * Simple dmabuf/render-nodes Client + * This is a simple example client which uses dmabuf to pass buffers via wl_shm + * (instead of wl_drm or other EGL extensions) to the compositor. To render the + * images, we use DRM render-nodes or software-rendering as fallback. + * + * Note that this is a very basic example how to use wl_shm to share buffers + * which were rendered using a dedicated GPU. No mesa/EGL internal extensions + * are required. + * However, this is *not* the recommended way to do it! The mesa/EGL extensions + * allow much better integration and control. This client serves as a + * proof-of-concept and generic HOWTO. + */ + +#define EGL_EGLEXT_PROTOTYPES +#define GL_GLEXT_PROTOTYPES + +#include config.h + +#include assert.h +#include dirent.h +#include EGL/egl.h +#include EGL/eglext.h +#include errno.h +#include error.h +#include fcntl.h +#include gbm.h +#include GLES2/gl2.h +#include GLES2/gl2ext.h +#include signal.h +#include stdio.h +#include stdlib.h +#include string.h +#include stdbool.h +#include sys/mman.h +#include
[PATCH v2 1/4] Add optional dbus helpers
This adds optional libdbus integration for weston. If libdbus is available and not disabled via --disable-dbus during weston build, we now provide basic DBusConnection main-loop integration for weston. The dbus.c file provides a new helper to integrate any DBusConnection object into a wl_event_loop object. This avoids any glib/qt/.. dependencies but instead only uses the low-level libdbus library. Note that we do not provide dummy fallbacks for dbus helpers in case dbus-support is disabled. The reason for that is that you need dbus/dbus.h for nearly any operation you want to do via dbus. Therefore, only the most basic helpers which can be used independently provide a static inline dummy fallback to avoid #ifdef all over the code. --- configure.ac| 23 src/Makefile.am | 8 ++ src/dbus.c | 336 src/dbus.h | 67 +++ 4 files changed, 434 insertions(+) create mode 100644 src/dbus.c create mode 100644 src/dbus.h diff --git a/configure.ac b/configure.ac index 950086d..2661aad 100644 --- a/configure.ac +++ b/configure.ac @@ -379,6 +379,28 @@ if test x$enable_colord != xno; then fi AM_CONDITIONAL(ENABLE_COLORD, test x$enable_colord = xyes) +# dbus support +AC_ARG_ENABLE(dbus, + AS_HELP_STRING([--disable-dbus], + [do not build with dbus support]),, + enable_dbus=auto) +if test x$enable_dbus != xno; then +PKG_CHECK_MODULES(DBUS, + dbus-1 = 1.6, + have_dbus=yes, + have_dbus=no) +if test x$have_dbus = xno -a x$enable_dbus = xyes; then +AC_MSG_ERROR([dbus support explicitly requested, but libdbus couldn't be found]) +fi +if test x$have_dbus = xyes; then +enable_dbus=yes +AC_DEFINE([HAVE_DBUS], [1], [Build with dbus support]) +else +enable_dbus=no +fi +fi +AM_CONDITIONAL(ENABLE_DBUS, test x$enable_dbus = xyes) + AC_ARG_ENABLE(wcap-tools, [ --disable-wcap-tools],, enable_wcap_tools=yes) AM_CONDITIONAL(BUILD_WCAP_TOOLS, test x$enable_wcap_tools = xyes) if test x$enable_wcap_tools = xyes; then @@ -467,6 +489,7 @@ AC_MSG_RESULT([ EGL ${enable_egl} libxkbcommon${enable_xkbcommon} XWayland${enable_xwayland} + dbus${enable_dbus} Build wcap utility ${enable_wcap_tools} Build Tablet Shell ${enable_tablet_shell} diff --git a/src/Makefile.am b/src/Makefile.am index b0eae7c..8cd7cca 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -49,6 +49,14 @@ weston_SOURCES = \ weston-launch.h \ weston-egl-ext.h +if ENABLE_DBUS +weston_SOURCES += \ + dbus.h \ + dbus.c +weston_CFLAGS += $(DBUS_CFLAGS) +weston_LDADD += $(DBUS_LIBS) +endif + git-version.h : .FORCE $(AM_V_GEN)(echo #define BUILD_ID \$(shell git --git-dir=$(top_srcdir)/.git describe --always --dirty) $(shell git --git-dir=$(top_srcdir)/.git log -1 --format='%s (%ci)')\ $@-new; \ cmp -s $@ $@-new || cp $@-new $@; \ diff --git a/src/dbus.c b/src/dbus.c new file mode 100644 index 000..4de8c9c --- /dev/null +++ b/src/dbus.c @@ -0,0 +1,336 @@ +/* + * Copyright © 2013 David Herrmann dh.herrm...@gmail.com + * + * Permission to use, copy, modify, distribute, and sell this software and + * its documentation for any purpose is hereby granted without fee, provided + * that the above copyright notice appear in all copies and that both that + * copyright notice and this permission notice appear in supporting + * documentation, and that the name of the copyright holders not be used in + * advertising or publicity pertaining to distribution of the software + * without specific, written prior permission. The copyright holders make + * no representations about the suitability of this software for any + * purpose. It is provided as is without express or implied warranty. + * + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +/* + * DBus Helpers + * This file contains the dbus mainloop integration and several helpers to + * make lowlevel libdbus access easier. + */ + +#include config.h + +#include dbus/dbus.h +#include errno.h +#include fcntl.h +#include stdbool.h +#include stdlib.h
[PATCH v2 2/4] dbus: add dbus-match helpers
These helpers simplify adding dbus-matches by allowing var-arg arguments to assemble the matching rules. --- src/dbus.c | 68 ++ src/dbus.h | 40 2 files changed, 108 insertions(+) diff --git a/src/dbus.c b/src/dbus.c index 4de8c9c..a1abbd5 100644 --- a/src/dbus.c +++ b/src/dbus.c @@ -334,3 +334,71 @@ void weston_dbus_close(DBusConnection *c, struct wl_event_source *ctx) dbus_connection_close(c); dbus_connection_unref(c); } + +int weston_dbus_add_match(DBusConnection *c, const char *format, ...) +{ + DBusError err; + int r; + va_list list; + char *str; + + va_start(list, format); + r = vasprintf(str, format, list); + va_end(list); + + if (r 0) + return -ENOMEM; + + dbus_error_init(err); + dbus_bus_add_match(c, str, err); + free(str); + if (dbus_error_is_set(err)) { + dbus_error_free(err); + return -EIO; + } + + return 0; +} + +int weston_dbus_add_match_signal(DBusConnection *c, const char *sender, +const char *iface, const char *member, +const char *path) +{ + return weston_dbus_add_match(c, +type='signal', +sender='%s', +interface='%s', +member='%s', +path='%s', +sender, iface, member, path); +} + +void weston_dbus_remove_match(DBusConnection *c, const char *format, ...) +{ + int r; + va_list list; + char *str; + + va_start(list, format); + r = vasprintf(str, format, list); + va_end(list); + + if (r 0) + return; + + dbus_bus_remove_match(c, str, NULL); + free(str); +} + +void weston_dbus_remove_match_signal(DBusConnection *c, const char *sender, +const char *iface, const char *member, +const char *path) +{ + return weston_dbus_remove_match(c, + type='signal', + sender='%s', + interface='%s', + member='%s', + path='%s', + sender, iface, member, path); +} diff --git a/src/dbus.h b/src/dbus.h index 90ab8f6..06fe762 100644 --- a/src/dbus.h +++ b/src/dbus.h @@ -62,6 +62,46 @@ int weston_dbus_open(struct wl_event_loop *loop, DBusBusType bus, */ void weston_dbus_close(DBusConnection *c, struct wl_event_source *ctx); +/* + * weston_dbus_add_match() - Add dbus match + * + * Configure a dbus-match on the given dbus-connection. This match is saved + * on the dbus-server as long as the connection is open. See dbus-manual + * for information. Compared to the dbus_bus_add_match() this allows a + * var-arg formatted match-string. + */ +int weston_dbus_add_match(DBusConnection *c, const char *format, ...); + +/* + * weston_dbus_add_match_signal() - Add dbus signal match + * + * Same as weston_dbus_add_match() but does the dbus-match formatting for + * signals internally. + */ +int weston_dbus_add_match_signal(DBusConnection *c, const char *sender, +const char *iface, const char *member, +const char *path); + +/* + * weston_dbus_remove_match() - Remove dbus match + * + * Remove a previously configured dbus-match from the dbus server. There is + * no need to remove dbus-matches if you close the connection, anyway. + * Compared to dbus_bus_remove_match() this allows a var-arg formatted + * match string. + */ +void weston_dbus_remove_match(DBusConnection *c, const char *format, ...); + +/* + * weston_dbus_remove_match_signal() - Remove dbus signal match + * + * Same as weston_dbus_remove_match() but does the dbus-match formatting for + * signals internally. + */ +void weston_dbus_remove_match_signal(DBusConnection *c, const char *sender, +const char *iface, const char *member, +const char *path); + #endif /* HAVE_DBUS */ #endif // _WESTON_DBUS_H_ -- 1.8.4.1 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH v2 3/4] launcher: add weston_launcher_close() dummy
If you request a device via weston_launcher_open(), you should now release it via weston_launcher_close() instead of close(). This is currently not needed but will be required for logind devices. --- src/launcher-util.c | 6 ++ src/launcher-util.h | 3 +++ src/udev-seat.c | 15 ++- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/launcher-util.c b/src/launcher-util.c index 4f77d11..8b496c8 100644 --- a/src/launcher-util.c +++ b/src/launcher-util.c @@ -174,6 +174,12 @@ weston_launcher_open(struct weston_launcher *launcher, } void +weston_launcher_close(struct weston_launcher *launcher, int fd) +{ + close(fd); +} + +void weston_launcher_restore(struct weston_launcher *launcher) { struct vt_mode mode = { 0 }; diff --git a/src/launcher-util.h b/src/launcher-util.h index 3e7ceb5..9de5237 100644 --- a/src/launcher-util.h +++ b/src/launcher-util.h @@ -39,6 +39,9 @@ int weston_launcher_open(struct weston_launcher *launcher, const char *path, int flags); +void +weston_launcher_close(struct weston_launcher *launcher, int fd); + int weston_launcher_activate_vt(struct weston_launcher *launcher, int vt); diff --git a/src/udev-seat.c b/src/udev-seat.c index 4ef7ff3..11a34fb 100644 --- a/src/udev-seat.c +++ b/src/udev-seat.c @@ -83,11 +83,11 @@ device_added(struct udev_device *udev_device, struct udev_input *input) device = evdev_device_create(seat-base, devnode, fd); if (device == EVDEV_UNHANDLED_DEVICE) { - close(fd); + weston_launcher_close(c-launcher, fd); weston_log(not using input device '%s'.\n, devnode); return 0; } else if (device == NULL) { - close(fd); + weston_launcher_close(c-launcher, fd); weston_log(failed to create input device '%s'.\n, devnode); return 0; } @@ -216,9 +216,11 @@ evdev_udev_handler(int fd, uint32_t mask, void *data) if (!strcmp(device-devnode, devnode)) { weston_log(input device %s, %s removed\n, device-devname, device-devnode); + weston_launcher_close(input-compositor-launcher, + device-fd); evdev_device_destroy(device); - break; - } + break; + } } } @@ -273,8 +275,11 @@ udev_input_remove_devices(struct udev_input *input) struct udev_seat *seat; wl_list_for_each(seat, input-compositor-seat_list, base.link) { - wl_list_for_each_safe(device, next, seat-devices_list, link) + wl_list_for_each_safe(device, next, seat-devices_list, link) { + weston_launcher_close(input-compositor-launcher, + device-fd); evdev_device_destroy(device); + } if (seat-base.keyboard) notify_keyboard_focus_out(seat-base); -- 1.8.4.1 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH v2 4/4] launcher: add logind backend
@@ +/* + * Copyright © 2013 David Herrmann + * + * Permission to use, copy, modify, distribute, and sell this software and + * its documentation for any purpose is hereby granted without fee, provided + * that the above copyright notice appear in all copies and that both that + * copyright notice and this permission notice appear in supporting + * documentation, and that the name of the copyright holders not be used in + * advertising or publicity pertaining to distribution of the software + * without specific, written prior permission. The copyright holders make + * no representations about the suitability of this software for any + * purpose. It is provided as is without express or implied warranty. + * + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +#include config.h + +#include errno.h +#include dbus.h +#include fcntl.h +#include linux/vt.h +#include linux/kd.h +#include linux/major.h +#include signal.h +#include stdarg.h +#include stdbool.h +#include stdio.h +#include stdlib.h +#include string.h +#include sys/ioctl.h +#include sys/signalfd.h +#include sys/stat.h +#include systemd/sd-login.h +#include unistd.h + +#include compositor.h +#include dbus.h +#include logind-util.h + +#ifndef KDSKBMUTE +#define KDSKBMUTE 0x4B51 +#endif + +struct weston_logind { + struct weston_compositor *compositor; + char *seat; + char *sid; + unsigned int vtnr; + int vt; + int kb_mode; + int sfd; + struct wl_event_source *sfd_source; + + DBusConnection *dbus; + struct wl_event_source *dbus_ctx; + char *spath; + DBusPendingCall *pending_active; +}; + +static int +weston_logind_take_device(struct weston_logind *wl, uint32_t major, + uint32_t minor, bool *paused_out) +{ + DBusMessage *m, *reply; + bool b, paused; + int r, fd; + + m = dbus_message_new_method_call(org.freedesktop.login1, +wl-spath, +org.freedesktop.login1.Session, +TakeDevice); + if (!m) + return -ENOMEM; + + b = dbus_message_append_args(m, +DBUS_TYPE_UINT32, major, +DBUS_TYPE_UINT32, minor, +DBUS_TYPE_INVALID); + if (!b) { + r = -ENOMEM; + goto err_unref; + } + + reply = dbus_connection_send_with_reply_and_block(wl-dbus, m, + -1, NULL); + if (!reply) { + r = -ENODEV; + goto err_unref; + } + + b = dbus_message_get_args(reply, NULL, + DBUS_TYPE_UNIX_FD, fd, + DBUS_TYPE_BOOLEAN, paused, + DBUS_TYPE_INVALID); + if (!b) { + r = -ENODEV; + goto err_reply; + } + + r = fd; + if (paused_out) + *paused_out = paused; + +err_reply: + dbus_message_unref(reply); +err_unref: + dbus_message_unref(m); + return r; +} + +static void +weston_logind_release_device(struct weston_logind *wl, uint32_t major, +uint32_t minor) +{ + DBusMessage *m; + bool b; + + m = dbus_message_new_method_call(org.freedesktop.login1, +wl-spath, +org.freedesktop.login1.Session, +ReleaseDevice); + if (m) { + b = dbus_message_append_args(m, +DBUS_TYPE_UINT32, major, +DBUS_TYPE_UINT32, minor, +DBUS_TYPE_INVALID); + if (b) + dbus_connection_send(wl-dbus, m, NULL); + dbus_message_unref(m); + } +} + +static void +weston_logind_pause_device_complete(struct weston_logind *wl, uint32_t major, + uint32_t minor) +{ + DBusMessage *m; + bool b; + + m = dbus_message_new_method_call(org.freedesktop.login1, +wl-spath, +org.freedesktop.login1.Session, +PauseDeviceComplete); + if (m) { + b = dbus_message_append_args(m
[PATCH] event-loop: fix blocking timerfd read
If we call wl_event_source_check() on a timerfd, our dispatcher will block on the timerfd read(). Avoid calling read() if no epoll-event was reported. Note: The internal tick-count of a timerfd is never decreased. So once we get signaled a read event, a following read() is guaranteed to succeed _if_ no other operation is called on the timerfd in between. Therefore, there is no need for us to make the read() non-blocking (even with clock-changes we never block). However, to be safe for future changed, just ignore EAGAIN safely. Also remove the comment about read() failure. The only reasons the read can currently fail is EINTR or invalid parameters. The latter cannot happen in our setup so ignore it. EINTR shouldn't happen as syscall-restart is the default behavior so we simply return if a user changed it. Better safe than sorry. Signed-off-by: David Herrmann dh.herrm...@gmail.com --- src/event-loop.c | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/event-loop.c b/src/event-loop.c index d323601..d340e3c 100644 --- a/src/event-loop.c +++ b/src/event-loop.c @@ -172,10 +172,15 @@ wl_event_source_timer_dispatch(struct wl_event_source *source, uint64_t expires; int len; - len = read(source-fd, expires, sizeof expires); - if (len != sizeof expires) - /* Is there anything we can do here? Will this ever happen? */ - fprintf(stderr, timerfd read error: %m\n); + if (ep-events != 0) { + len = read(source-fd, expires, sizeof expires); + if (len != sizeof expires) { + if (len = 0 || (errno != EINTR errno != EAGAIN)) + fprintf(stderr, + timerfd read error (%d): %m\n, len); + return 0; + } + } return timer_source-func(timer_source-base.data); } -- 1.8.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 3/7] Make weston spawn weston-launch
Hi On Tue, Oct 15, 2013 at 8:07 PM, Kristian Høgsberg hoegsb...@gmail.com wrote: On Tue, Oct 15, 2013 at 02:29:58PM +0200, David Herrmann wrote: This is a rather complete rewrite of weston-launch. Instead of spawning weston from weston-launch, we do the inverse now. Whenever weston is spawned with a backend that uses launcher-util, we spawn weston-launch as child process. weston-launch still handles VT switching and open() for session-devices and passes them back to weston. However, this drops all PAM, --user and session handling from weston-launch. If we spawn weston-launch from weston, we cannot do any PAM handling as it wouldn't affect weston, anymore. But this doesn't hurt as the same effects can be achieved via su, sudo or /bin/login. There is no real reason to do this in weston. If you want weston to run in a new session, use one of these helpers to create the session and then spawn weston from within the new session. Some more differences are: (1) We have now two sockets from weston to weston-launch. One for OPEN and one for EVENTS. The reason for that is that if we send an OPEN request and weston-launch schedules some VT-EVENT before sending the OPEN response, launcher-util needs to handle the VT-EVENT from within weston_launcher_open(). This isn't very hard to do, but may cause weird side-effects as the callstack might already be an EVENT handling. There are other ways to handle that, but the simplest way is to have two independent queues. I thought I handled this in current weston-launch by scheduling an idle callback to handle the event if I get an event while waiting for a response. But I don't see the code there, so I must have failed to do that, but I talked to Giovanni about this for mutter-launch and he did it there. Using two socketpairs has much the same effect and shouldn't be a big deal. Problem with idle-callback is that we can schedule it only once. So multiple events may be merged. It might work if we remember each event individually, but I thought two pipes is the easier way. (2) VT_ACTIVATE is privileged, so we pass the request to weston-launch which then issues the VT_ACTIVATE ioctl. Are you sure? I can login in text mode and use chvt to vt switch away from that text login. I think if your tty is a vt and that vt is currently active, you're allowed to switch away. That is, you can't run on an inactive VT and then vt switch to your VT, but you're allowed to switch away. Which is all weston needs and it's how launcher-util.c currently does it. As for the initial VT switch to the weston VT, that is done in weston-launch. Actually, I just tried to log in on vt2, do a sleep 2; chvt 3 and then switch to vt 1. After the two seconds I'm switched back to vt 2. chvt isn't setuid and I run is as user krh. It fails under X because it tries to open /prof/self/fd/0, which is the pty of your X terminal. But if I try [krh@tokamak weston]$ chvt 2 0/dev/tty1 chvt: VT_ACTIVATE: Operation not permitted so I'm not really sure what the rules are. Mainly, I'd just like to not have this in the weston-launch protocol. For logind it doesn't matter since we use session.Activate there, but if we can keep the weston-launch case simpler by not having more protocol than we need to, that's better. VT ioctls are allowed by: - CAP_SYS_TTY_CONFIG - processes with the VT as controlling-tty Problem with the latter is that only a single session can have a given TTY as controlling TTY. Overwriting this requires root. So if you start weston from within a shell on a VT, you cannot call setsid()+TIOCSCTTY from weston, as the shell still has the VT as controlling-TTY... That's why I thought the easiest way is to let weston-launch do that as root. (3) weston-launch is mandatory now. Every backend which uses launcher-util requires weston-launch now. However, setuid is *not* mandatory! If weston-launch does not have the setuid bit, you have to run weston as root (as usual). That's fine, the reason I kept the ability to run without weston-launch was that having to run weston through weston-launch gets in the way of debugging and testing. Now that we're running weston directly even when it's using weston-launch, we can drop that code path. Ok, Benjamin's comment actually made me reconsider this. If weston-launch is spawned by weston, any other session may also use weston-launch. I doubt there's a safe way for us to verify we're called from weston and not from a random attacker. Attack-scenarios would be: Attacker spawns weston-launch, retrieves all input file-descriptors and then kills weston-launch. We currently do not track FDs in weston-launch so we cannot revoke them during exit(). The attacker can now listen on the input-FDs if any other compositor starts on the VT. Ideas: - require root or weston-launch group to spawn weston-launch. (that is, we drop the systemd-session check) - keep the current style by spawning weston from
[PATCH 1/7] evdev: release devices on read() error
If read() fails without EAGAIN/EINTR, the device is very likely dead. However, we must not remove the device as it might be muted/revoked. So we simply remove the event-source to avoid polling the device and simply wait for the udev-remove signal now. Note that we cannot call evdev_device_destroy() as the caller created the FD and might need custom code to close it (like weston_launcher_close()). --- src/evdev.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/evdev.c b/src/evdev.c index edea396..7397ea1 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -22,6 +22,7 @@ #include config.h +#include errno.h #include stdlib.h #include string.h #include linux/input.h @@ -414,7 +415,13 @@ evdev_device_data(int fd, uint32_t mask, void *data) len = read(fd, ev, sizeof ev); if (len 0 || len % sizeof ev[0] != 0) { - /* FIXME: call evdev_device_destroy when errno is ENODEV. */ + if (len 0 errno != EAGAIN errno != EINTR) { + weston_log(device %s died\n, + device-devnode); + wl_event_source_remove(device-source); + device-source = NULL; + } + return 1; } -- 1.8.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 2/7] fbdev: open launcher only once
We currently call launcher_connect() twice, which is redundant and amazingly works (ugh?). Fix this and connect only once to the launcher. --- src/compositor-fbdev.c | 12 ++-- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c index 002ce0b..4376345 100644 --- a/src/compositor-fbdev.c +++ b/src/compositor-fbdev.c @@ -883,15 +883,6 @@ fbdev_compositor_create(struct wl_display *display, int *argc, char *argv[], config) 0) goto out_free; - /* Check if we run fbdev-backend using weston-launch */ - compositor-base.launcher = - weston_launcher_connect(compositor-base, param-tty); - if (compositor-base.launcher == NULL geteuid() != 0) { - weston_log(fatal: fbdev backend should be run - using weston-launch binary or as root\n); - goto out_compositor; - } - compositor-udev = udev_new(); if (compositor-udev == NULL) { weston_log(Failed to initialize udev context.\n); @@ -905,7 +896,8 @@ fbdev_compositor_create(struct wl_display *display, int *argc, char *argv[], compositor-base.launcher = weston_launcher_connect(compositor-base, param-tty); if (!compositor-base.launcher) { - weston_log(Failed to set up launcher.\n); + weston_log(fatal: fbdev backend should be run + using weston-launch binary or as root\n); goto out_udev; } -- 1.8.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 3/7] Make weston spawn weston-launch
This is a rather complete rewrite of weston-launch. Instead of spawning weston from weston-launch, we do the inverse now. Whenever weston is spawned with a backend that uses launcher-util, we spawn weston-launch as child process. weston-launch still handles VT switching and open() for session-devices and passes them back to weston. However, this drops all PAM, --user and session handling from weston-launch. If we spawn weston-launch from weston, we cannot do any PAM handling as it wouldn't affect weston, anymore. But this doesn't hurt as the same effects can be achieved via su, sudo or /bin/login. There is no real reason to do this in weston. If you want weston to run in a new session, use one of these helpers to create the session and then spawn weston from within the new session. Some more differences are: (1) We have now two sockets from weston to weston-launch. One for OPEN and one for EVENTS. The reason for that is that if we send an OPEN request and weston-launch schedules some VT-EVENT before sending the OPEN response, launcher-util needs to handle the VT-EVENT from within weston_launcher_open(). This isn't very hard to do, but may cause weird side-effects as the callstack might already be an EVENT handling. There are other ways to handle that, but the simplest way is to have two independent queues. (2) VT_ACTIVATE is privileged, so we pass the request to weston-launch which then issues the VT_ACTIVATE ioctl. (3) weston-launch is mandatory now. Every backend which uses launcher-util requires weston-launch now. However, setuid is *not* mandatory! If weston-launch does not have the setuid bit, you have to run weston as root (as usual). (4) Reading VTNR from systemd is now supported (requires systemd-207, actually without the unreleased systemd-209 it will segfault due to a bug in the implementation of sd_session_get_vt()) (5) I pass -v by default to weston-launch now. If someone doesn't like that kind of verbosity, feel free to add a new command-line argument to weston which forwards this flag. --- configure.ac| 30 ++-- src/Makefile.am | 38 ++--- src/launcher-util.c | 430 ++-- src/weston-launch.c | 401 +--- src/weston-launch.h | 6 + 5 files changed, 386 insertions(+), 519 deletions(-) diff --git a/configure.ac b/configure.ac index 950086d..93a38ac 100644 --- a/configure.ac +++ b/configure.ac @@ -53,7 +53,7 @@ AC_CHECK_DECL(CLOCK_MONOTONIC,[], [[#include time.h]]) AC_CHECK_HEADERS([execinfo.h]) -AC_CHECK_FUNCS([mkostemp strchrnul initgroups]) +AC_CHECK_FUNCS([mkostemp strchrnul]) COMPOSITOR_MODULES=wayland-server = 1.2.91 pixman-1 @@ -316,21 +316,18 @@ AC_ARG_ENABLE(resize-optimization, AS_IF([test x$enable_resize_optimization = xyes], [AC_DEFINE([USE_RESIZE_POOL], [1], [Use resize memory pool as a performance optimization])]) -AC_ARG_ENABLE(weston-launch, [ --enable-weston-launch],, enable_weston_launch=yes) -AM_CONDITIONAL(BUILD_WESTON_LAUNCH, test x$enable_weston_launch == xyes) -if test x$enable_weston_launch == xyes; then - PKG_CHECK_MODULES(WESTON_LAUNCH, [libdrm]) - PKG_CHECK_MODULES(SYSTEMD_LOGIN, [libsystemd-login], - [have_systemd_login=yes], [have_systemd_login=no]) - AS_IF([test x$have_systemd_login = xyes], - [AC_DEFINE([HAVE_SYSTEMD_LOGIN], [1], [Have systemd-login])]) - - AC_CHECK_LIB([pam], [pam_open_session], [have_pam=yes], [have_pam=no]) - if test x$have_pam == xno; then -AC_ERROR([weston-launch requires pam]) - fi - WESTON_LAUNCH_LIBS=$WESTON_LAUNCH_LIBS -lpam -fi +PKG_CHECK_MODULES(SYSTEMD_LOGIN, [libsystemd-login], + [have_systemd_login=yes], [have_systemd_login=no]) +AS_IF([test x$have_systemd_login = xyes], + [AC_DEFINE([HAVE_SYSTEMD_LOGIN], [1], [Have systemd-login])]) + +PKG_CHECK_MODULES(SYSTEMD_LOGIN_207, [libsystemd-login = 207], + [have_systemd_login_207=yes], [have_systemd_login_207=no]) +AS_IF([test x$have_systemd_login_207 = xyes], + [AC_DEFINE([HAVE_SYSTEMD_LOGIN_207], [1], [Have systemd-login = 207])]) + +PKG_CHECK_MODULES(LAUNCHER_UTIL, [libsystemd-login],,) +PKG_CHECK_MODULES(WESTON_LAUNCH, [libdrm libsystemd-login],,) if test x$enable_egl = xyes; then PKG_CHECK_MODULES(GLU, [glu], [have_glu=yes], [have_glu=no]) @@ -471,7 +468,6 @@ AC_MSG_RESULT([ Build wcap utility ${enable_wcap_tools} Build Tablet Shell ${enable_tablet_shell} - weston-launch utility ${enable_weston_launch} weston-launch systemd support ${have_systemd_login} DRM Compositor ${enable_drm_compositor} diff --git a/src/Makefile.am b/src/Makefile.am index b0eae7c..2521b45 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1,5 +1,4 @@ -bin_PROGRAMS = weston \ - $(weston_launch) +bin_PROGRAMS = weston AM_CPPFLAGS =
[PATCH 4/7] Add optional dbus helpers
This adds optional libdbus integration for weston. If libdbus is available and not disabled via --disable-dbus during weston build, we now provide basic DBusConnection main-loop integration for weston. The dbus.c file provides a new helper to integrate any DBusConnection object into a wl_event_loop object. This avoids any glib/qt/.. dependencies but instead only uses the low-level libdbus library. Note that we do not provide dummy fallbacks for dbus helpers in case dbus-support is disabled. The reason for that is that you need dbus/dbus.h for nearly any operation you want to do via dbus. Therefore, only the most basic helpers which can be used independently provide a static inline dummy fallback to avoid #ifdef all over the code. --- configure.ac| 23 src/Makefile.am | 8 ++ src/dbus.c | 336 src/dbus.h | 67 +++ 4 files changed, 434 insertions(+) create mode 100644 src/dbus.c create mode 100644 src/dbus.h diff --git a/configure.ac b/configure.ac index 93a38ac..643209c 100644 --- a/configure.ac +++ b/configure.ac @@ -376,6 +376,28 @@ if test x$enable_colord != xno; then fi AM_CONDITIONAL(ENABLE_COLORD, test x$enable_colord = xyes) +# dbus support +AC_ARG_ENABLE(dbus, + AS_HELP_STRING([--disable-dbus], + [do not build with dbus support]),, + enable_dbus=auto) +if test x$enable_dbus != xno; then +PKG_CHECK_MODULES(DBUS, + dbus-1 = 1.6, + have_dbus=yes, + have_dbus=no) +if test x$have_dbus = xno -a x$enable_dbus = xyes; then +AC_MSG_ERROR([dbus support explicitly requested, but libdbus couldn't be found]) +fi +if test x$have_dbus = xyes; then +enable_dbus=yes +AC_DEFINE([HAVE_DBUS], [1], [Build with dbus support]) +else +enable_dbus=no +fi +fi +AM_CONDITIONAL(ENABLE_DBUS, test x$enable_dbus = xyes) + AC_ARG_ENABLE(wcap-tools, [ --disable-wcap-tools],, enable_wcap_tools=yes) AM_CONDITIONAL(BUILD_WCAP_TOOLS, test x$enable_wcap_tools = xyes) if test x$enable_wcap_tools = xyes; then @@ -464,6 +486,7 @@ AC_MSG_RESULT([ EGL ${enable_egl} libxkbcommon${enable_xkbcommon} XWayland${enable_xwayland} + dbus${enable_dbus} Build wcap utility ${enable_wcap_tools} Build Tablet Shell ${enable_tablet_shell} diff --git a/src/Makefile.am b/src/Makefile.am index 2521b45..44ebfc2 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -48,6 +48,14 @@ weston_SOURCES = \ weston-launch.h \ weston-egl-ext.h +if ENABLE_DBUS +weston_SOURCES += \ + dbus.h \ + dbus.c +weston_CFLAGS += $(DBUS_CFLAGS) +weston_LDADD += $(DBUS_LIBS) +endif + git-version.h : .FORCE $(AM_V_GEN)(echo #define BUILD_ID \$(shell git --git-dir=$(top_srcdir)/.git describe --always --dirty) $(shell git --git-dir=$(top_srcdir)/.git log -1 --format='%s (%ci)')\ $@-new; \ cmp -s $@ $@-new || cp $@-new $@; \ diff --git a/src/dbus.c b/src/dbus.c new file mode 100644 index 000..4de8c9c --- /dev/null +++ b/src/dbus.c @@ -0,0 +1,336 @@ +/* + * Copyright © 2013 David Herrmann dh.herrm...@gmail.com + * + * Permission to use, copy, modify, distribute, and sell this software and + * its documentation for any purpose is hereby granted without fee, provided + * that the above copyright notice appear in all copies and that both that + * copyright notice and this permission notice appear in supporting + * documentation, and that the name of the copyright holders not be used in + * advertising or publicity pertaining to distribution of the software + * without specific, written prior permission. The copyright holders make + * no representations about the suitability of this software for any + * purpose. It is provided as is without express or implied warranty. + * + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +/* + * DBus Helpers + * This file contains the dbus mainloop integration and several helpers to + * make lowlevel libdbus access easier. + */ + +#include config.h + +#include dbus/dbus.h +#include errno.h +#include fcntl.h +#include stdbool.h +#include stdlib.h
[PATCH 6/7] launcher: add weston_launcher_close() dummy
If you request a device via weston_launcher_open(), you should now release it via weston_launcher_close() instead of close(). This is currently not needed but will be required for logind devices. --- src/launcher-util.c | 6 ++ src/launcher-util.h | 3 +++ src/udev-seat.c | 15 ++- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/launcher-util.c b/src/launcher-util.c index 3f19083..30e62fc 100644 --- a/src/launcher-util.c +++ b/src/launcher-util.c @@ -138,6 +138,12 @@ weston_launcher_open(struct weston_launcher *launcher, } void +weston_launcher_close(struct weston_launcher *launcher, int fd) +{ + close(fd); +} + +void weston_launcher_restore(struct weston_launcher *launcher) { struct vt_mode mode = { 0 }; diff --git a/src/launcher-util.h b/src/launcher-util.h index 3e7ceb5..9de5237 100644 --- a/src/launcher-util.h +++ b/src/launcher-util.h @@ -39,6 +39,9 @@ int weston_launcher_open(struct weston_launcher *launcher, const char *path, int flags); +void +weston_launcher_close(struct weston_launcher *launcher, int fd); + int weston_launcher_activate_vt(struct weston_launcher *launcher, int vt); diff --git a/src/udev-seat.c b/src/udev-seat.c index 4ef7ff3..11a34fb 100644 --- a/src/udev-seat.c +++ b/src/udev-seat.c @@ -83,11 +83,11 @@ device_added(struct udev_device *udev_device, struct udev_input *input) device = evdev_device_create(seat-base, devnode, fd); if (device == EVDEV_UNHANDLED_DEVICE) { - close(fd); + weston_launcher_close(c-launcher, fd); weston_log(not using input device '%s'.\n, devnode); return 0; } else if (device == NULL) { - close(fd); + weston_launcher_close(c-launcher, fd); weston_log(failed to create input device '%s'.\n, devnode); return 0; } @@ -216,9 +216,11 @@ evdev_udev_handler(int fd, uint32_t mask, void *data) if (!strcmp(device-devnode, devnode)) { weston_log(input device %s, %s removed\n, device-devname, device-devnode); + weston_launcher_close(input-compositor-launcher, + device-fd); evdev_device_destroy(device); - break; - } + break; + } } } @@ -273,8 +275,11 @@ udev_input_remove_devices(struct udev_input *input) struct udev_seat *seat; wl_list_for_each(seat, input-compositor-seat_list, base.link) { - wl_list_for_each_safe(device, next, seat-devices_list, link) + wl_list_for_each_safe(device, next, seat-devices_list, link) { + weston_launcher_close(input-compositor-launcher, + device-fd); evdev_device_destroy(device); + } if (seat-base.keyboard) notify_keyboard_focus_out(seat-base); -- 1.8.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 5/7] dbus: add dbus-match helpers
These helpers simplify adding dbus-matches by allowing var-arg arguments to assemble the matching rules. --- src/dbus.c | 68 ++ src/dbus.h | 40 2 files changed, 108 insertions(+) diff --git a/src/dbus.c b/src/dbus.c index 4de8c9c..a1abbd5 100644 --- a/src/dbus.c +++ b/src/dbus.c @@ -334,3 +334,71 @@ void weston_dbus_close(DBusConnection *c, struct wl_event_source *ctx) dbus_connection_close(c); dbus_connection_unref(c); } + +int weston_dbus_add_match(DBusConnection *c, const char *format, ...) +{ + DBusError err; + int r; + va_list list; + char *str; + + va_start(list, format); + r = vasprintf(str, format, list); + va_end(list); + + if (r 0) + return -ENOMEM; + + dbus_error_init(err); + dbus_bus_add_match(c, str, err); + free(str); + if (dbus_error_is_set(err)) { + dbus_error_free(err); + return -EIO; + } + + return 0; +} + +int weston_dbus_add_match_signal(DBusConnection *c, const char *sender, +const char *iface, const char *member, +const char *path) +{ + return weston_dbus_add_match(c, +type='signal', +sender='%s', +interface='%s', +member='%s', +path='%s', +sender, iface, member, path); +} + +void weston_dbus_remove_match(DBusConnection *c, const char *format, ...) +{ + int r; + va_list list; + char *str; + + va_start(list, format); + r = vasprintf(str, format, list); + va_end(list); + + if (r 0) + return; + + dbus_bus_remove_match(c, str, NULL); + free(str); +} + +void weston_dbus_remove_match_signal(DBusConnection *c, const char *sender, +const char *iface, const char *member, +const char *path) +{ + return weston_dbus_remove_match(c, + type='signal', + sender='%s', + interface='%s', + member='%s', + path='%s', + sender, iface, member, path); +} diff --git a/src/dbus.h b/src/dbus.h index 90ab8f6..06fe762 100644 --- a/src/dbus.h +++ b/src/dbus.h @@ -62,6 +62,46 @@ int weston_dbus_open(struct wl_event_loop *loop, DBusBusType bus, */ void weston_dbus_close(DBusConnection *c, struct wl_event_source *ctx); +/* + * weston_dbus_add_match() - Add dbus match + * + * Configure a dbus-match on the given dbus-connection. This match is saved + * on the dbus-server as long as the connection is open. See dbus-manual + * for information. Compared to the dbus_bus_add_match() this allows a + * var-arg formatted match-string. + */ +int weston_dbus_add_match(DBusConnection *c, const char *format, ...); + +/* + * weston_dbus_add_match_signal() - Add dbus signal match + * + * Same as weston_dbus_add_match() but does the dbus-match formatting for + * signals internally. + */ +int weston_dbus_add_match_signal(DBusConnection *c, const char *sender, +const char *iface, const char *member, +const char *path); + +/* + * weston_dbus_remove_match() - Remove dbus match + * + * Remove a previously configured dbus-match from the dbus server. There is + * no need to remove dbus-matches if you close the connection, anyway. + * Compared to dbus_bus_remove_match() this allows a var-arg formatted + * match string. + */ +void weston_dbus_remove_match(DBusConnection *c, const char *format, ...); + +/* + * weston_dbus_remove_match_signal() - Remove dbus signal match + * + * Same as weston_dbus_remove_match() but does the dbus-match formatting for + * signals internally. + */ +void weston_dbus_remove_match_signal(DBusConnection *c, const char *sender, +const char *iface, const char *member, +const char *path); + #endif /* HAVE_DBUS */ #endif // _WESTON_DBUS_H_ -- 1.8.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 7/7] launcher: add logind backend
weston_launcher *launcher) { struct vt_mode mode = { 0 }; + if (launcher-logind) + return weston_logind_restore(launcher-logind); + ioctl(launcher-tty, KDSKBMUTE, 0); ioctl(launcher-tty, KDSKBMODE, launcher-kb_mode); ioctl(launcher-tty, KDSETMODE, KD_TEXT); @@ -197,6 +208,9 @@ weston_launcher_activate_vt(struct weston_launcher *launcher, int vt) ssize_t len; struct weston_launcher_switchvt message; + if (launcher-logind) + return weston_logind_activate_vt(launcher-logind, vt); + /* VT_ACTIVATE is a privileged ioctl so ask weston-launch */ message.header.opcode = WESTON_LAUNCHER_SWITCHVT; @@ -398,7 +412,8 @@ setup_tty(struct weston_launcher *wl, int ttyid) } struct weston_launcher * -weston_launcher_connect(struct weston_compositor *compositor, int tty) +weston_launcher_connect(struct weston_compositor *compositor, int tty, + const char *seat_id) { struct weston_launcher *launcher; struct wl_event_loop *loop; @@ -414,6 +429,12 @@ weston_launcher_connect(struct weston_compositor *compositor, int tty) launcher-fd = -1; launcher-event_fd = -1; + r = weston_logind_connect(launcher-logind, compositor, seat_id, tty); + if (r = 0) + return launcher; + + launcher-logind = NULL; + r = setup_tty(launcher, tty); if (r 0) goto err_free; @@ -448,11 +469,16 @@ err_free: void weston_launcher_destroy(struct weston_launcher *launcher) { - wl_list_remove(launcher-launcher.link); - wl_event_source_remove(launcher-source); - close(launcher-event_fd); - close(launcher-fd); - weston_launcher_restore(launcher); - close(launcher-tty); + if (launcher-logind) { + weston_logind_destroy(launcher-logind); + } else { + wl_list_remove(launcher-launcher.link); + wl_event_source_remove(launcher-source); + close(launcher-event_fd); + close(launcher-fd); + weston_launcher_restore(launcher); + close(launcher-tty); + } + free(launcher); } diff --git a/src/launcher-util.h b/src/launcher-util.h index 9de5237..d5b2fc9 100644 --- a/src/launcher-util.h +++ b/src/launcher-util.h @@ -30,7 +30,8 @@ struct weston_launcher; struct weston_launcher * -weston_launcher_connect(struct weston_compositor *compositor, int tty); +weston_launcher_connect(struct weston_compositor *compositor, int tty, + const char *seat_id); void weston_launcher_destroy(struct weston_launcher *launcher); diff --git a/src/logind-util.c b/src/logind-util.c new file mode 100644 index 000..066f751 --- /dev/null +++ b/src/logind-util.c @@ -0,0 +1,913 @@ +/* + * Copyright © 2013 David Herrmann + * + * Permission to use, copy, modify, distribute, and sell this software and + * its documentation for any purpose is hereby granted without fee, provided + * that the above copyright notice appear in all copies and that both that + * copyright notice and this permission notice appear in supporting + * documentation, and that the name of the copyright holders not be used in + * advertising or publicity pertaining to distribution of the software + * without specific, written prior permission. The copyright holders make + * no representations about the suitability of this software for any + * purpose. It is provided as is without express or implied warranty. + * + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +#include config.h + +#include errno.h +#include dbus.h +#include fcntl.h +#include linux/vt.h +#include linux/kd.h +#include linux/major.h +#include signal.h +#include stdarg.h +#include stdbool.h +#include stdio.h +#include stdlib.h +#include string.h +#include sys/ioctl.h +#include sys/signalfd.h +#include sys/stat.h +#include systemd/sd-login.h +#include unistd.h + +#include compositor.h +#include dbus.h +#include logind-util.h + +#ifndef KDSKBMUTE +#define KDSKBMUTE 0x4B51 +#endif + +struct weston_logind { + struct weston_compositor *compositor; + char *seat; + char *sid; + unsigned int vtnr; + int vt; + int kb_mode; + int sfd; + struct wl_event_source *sfd_source; + + DBusConnection *dbus; + struct wl_event_source *dbus_ctx; + char *spath; + DBusPendingCall *pending_active; +}; + +static int +weston_logind_take_device(struct weston_logind
Re: [PATCH 7/7] launcher: add logind backend
Hi On Tue, Oct 15, 2013 at 8:47 PM, Kristian Høgsberg hoegsb...@gmail.com wrote: On Tue, Oct 15, 2013 at 02:30:02PM +0200, David Herrmann wrote: Instead of spawning weston-launch from the launcher-util, we now try to connect to logind first. If logind provides session-devices, we use them. If not, we fall back to the old weston-launch facility. --- configure.ac | 1 + src/Makefile.am| 8 + src/compositor-drm.c | 2 +- src/compositor-fbdev.c | 2 +- src/compositor-rpi.c | 2 +- src/launcher-util.c| 40 ++- src/launcher-util.h| 3 +- src/logind-util.c | 913 + src/logind-util.h | 94 + 9 files changed, 1054 insertions(+), 11 deletions(-) create mode 100644 src/logind-util.c create mode 100644 src/logind-util.h I applied the whole patch series and stubbed out the call to sd_session_get_vt() (instead of updating systemd), but I then get Cool, thanks for testing. [11:35:04.658] logind: cannot take control over session 4 in the weston log and journalctl tells me Oct 15 11:34:52 tokamak.local dbus-daemon[444]: dbus[444]: [system] Rejected send message, 2 matched rules; type=method_call, sender=:1.75 (uid=1000 pid=17613 comm=weston --tty=2 ) interface=org.freedesktop.login1.Session member=TakeControl error name=(unset) requested_reply=0 destination=org.freedesktop.login1 (uid=0 pid=441 comm=/usr/lib/systemd/systemd-logind ) Oct 15 11:34:52 tokamak.local dbus[444]: [system] Rejected send message, 2 matched rules; type=method_call, sender=:1.75 (uid=1000 pid=17613 comm=weston --tty=2 ) interface=org.freedesktop.login1.Session member=TakeControl error name=(unset) requested_reply=0 destination=org.freedesktop.login1 (uid=0 pid=441 comm=/usr/lib/systemd/systemd-logind ) whoops, You need to adjust /etc/dbus-1/system.d/org.freedesktop.login1.conf: http://cgit.freedesktop.org/systemd/systemd/commit/?id=d7d1c8f983599dca6ee30229375215978657c072 I pushed the fix. I thought the dbus-policies are package-manager generated.. sorry. Also, logind-util.c does all the vt setup and teardown, but I wonder what happens if weston crashes. Will logind reset the vt (unmute, KD_TEXT etc) or will the system get stuck? One of the benefits of the recent weston-launch rewrite was that weston-launch will clean up if weston crashes, which is much more reliable. I thought leaving the VT in VT_AUTO mode would be enough. Session-switching would still work. Then today I noticed that in VT_AUTO+KD_GRAPHICS the kernel *ignores* all VT-switch request.. *sigh* That means, for a crashing weston we can just recover the VT in logind (as Session.ReleaseControl() is implicitly called on dbus disconnect of session-controllers). However, for a hung session we either have to set VT_PROCESS in logind itself or add some recovery option. I don't know.. Have to think about it. Note that this is still rudimentary. The weston-launch rewrite should be fine, but the dbus+logind patches still need testing and extensions. Btw., I can run weston without setuid, without weston-launch and without root here. Feels awesome! Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [xkbcommon] Use an integer type for modifiers bit mask.
Hi On Fri, Oct 4, 2013 at 2:34 PM, Daniel Stone dan...@fooishbar.org wrote: On 4 October 2013 13:09, Wander Lairson Costa wander.lair...@gmail.com wrote: That's what the patch is about: avoid casts. Whenever you use a cast, you are giving up the help the compiler may give to you regarding invalid type conversions. So, I always use the rule of thumb of avoiding casts whenever I can. IMO, this is not a harmful patch and will make the C++ programmers a little bit easier. I can see where it's going, but on the other hand xkb_mod_mask_t is _so_ not the right type. That's a bitmask of actual modifiers (i.e. the return value), not a bitmask of components. So it avoids the cast, but also makes the API look extremely misleading, as we've documented exactly what xkb_mod_mask_t is. I really like the type safety aspect, so to be honest I'm just inclined to make C++ wear the cast for now. We had a similar discussion when I pointed out that enum may change the underlying type without notice when adding new enum-values. But I agree with Daniel, the GCC type-safety for enums in C is a very handy feature. So I'm also no big fan of the patch, sorry. Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [RFC DRAFT] graphics tablet protocol extension
Hi Peter On Wed, Oct 2, 2013 at 11:13 PM, Peter Hutterer peter.hutte...@who-t.net wrote: On Wed, Oct 02, 2013 at 05:44:29PM +0200, David Herrmann wrote: Hi Peter On Fri, Sep 20, 2013 at 12:35 PM, Peter Hutterer diff --git a/protocol/wayland.xml b/protocol/wayland.xml index aeb0412..8d10746 100644 --- a/protocol/wayland.xml +++ b/protocol/wayland.xml @@ -1235,7 +1235,7 @@ /request /interface - interface name=wl_seat version=3 + interface name=wl_seat version=4 description summary=group of input devices A seat is a group of keyboards, pointer and touch devices. This object is published as a global during start up, or when such a @@ -1251,6 +1251,7 @@ entry name=pointer value=1 summary=The seat has pointer devices/ entry name=keyboard value=2 summary=The seat has one or more keyboards/ entry name=touch value=4 summary=The seat has touch devices/ + entry name=tablets value=8 summary=The seat has one or more graphics tablet devices since=4/ What's actually the reason to allow multiple graphics-tablets per seat? I thought wl_seat objects respresent a single user interacting with your desktop. So if you have two tablets, why not force them to be in two different wl_seat objects? Is there ever a reason to have multiple tablets in a single seat? What would the use-case be? my laptop has a built-in wacom tablet (lenovo x220t), but I also have an Intuos4 plugged in. depending on the use-case I use either, but they should be in the same seat. two wl_seat objects means two different keyboard foci (only one of which actually works if you have only one keyboard), etc. I also know of professional setups that require two tablets or more. ideally there'd be just one generic wl_tablet per seat, same as wl_pointer but the tablets differ too much to make that useable. Hence the multiple wl_tablet objects. Yepp, merging them might be not as easy as with keyboards/mice. Point taken. We could even go further and put tablets into their own seats. Always. A pointer and keyboard may be used by a single user at the same time. But for a graphics-tablet that doesn't sound like a legitimate use-case, does it? But maybe I just have a different view of wl_seat.. don't know. I somehow have the feeling we never really agreed on how to define wl_seat objects. Currently they are just user-defined groups of devices with some specific policies (only one keyboard-object per seat). my understanding of wl_seat is one group of input devices. that would usually map to one user. it's already not ideal for the bimanual input but then again that's even more of a corner case than multiple seats. once you start moving devices out into other seats, you're in trouble. a seat defines keyboard events, so anything that has an interaction with the keyboard can't easily be unpaired. a tablet is a fancy pointer device, users expect to click somewhere and have it work as a mouse, i.e. to move the keyboard focus to wherever you just clicked. moving it into a different seat means you'd have to switch to a mouse to change the keyboard focus. or, if you have two wl_seats you now need to pair them somehow to have the right focus behaviour. and that is going to be horrible. Ok, so wl_seat is just about input focus. I had a much stricter view of it but obviously it makes it hard to control focus of seats without mice/keyboards. So yeah, agreed. /enum event name=capabilities @@ -1306,6 +1307,19 @@ arg name=name type=string/ /event +!-- Version 4 additions -- +request name=get_tablet_manager since=4 + description summary=return tablet manager object +The ID provided will be initialized to the wl_tablet_manager +interface for this seat. This can then be used to retrieve the +objects representing the actual tablet devices. + +This request only takes effect if the seat has the tablets +capability. + /description + arg name=id type=new_id interface=wl_tablet_manager/ +/request + /interface interface name=wl_pointer version=3 @@ -1617,6 +1631,223 @@ /event /interface + interface name=wl_tablet_manager version=1 +description summary=controller object for graphic tablet devices + A tablet manager object provides requests to access the graphics + tablets available on this system. +/description + +enum name=tablet_type + description summary=tablet type +Describes the type of tablet. + /description + entry name=external value=0 summary=The tablet is an external tablet, such as an Intuos / + entry name=internal value=1 summary=The tablet is an built-in tablet, usually in a laptop / + entry name=display value =2 summary=The tablet is a display tablet, such as a Cintiq / +/enum
Re: [RFC DRAFT] graphics tablet protocol extension
Hi Peter On Fri, Sep 20, 2013 at 12:35 PM, Peter Hutterer peter.hutte...@who-t.net wrote: I've been working on a protocol extension to support graphics tablets such as the Wacom set of tablets, and I'm now at the stage where I'd like a few comments. I was hoping that I'd get a full implementation before XDC but unfortunately that didn't happen, so for now I'll just show the protocol. I've got a PoC implementation, but it's missing a few too many pieces to be actually usable just yet. Any feedback appreciated. This is a relatively early stage, so there are still many changes expected. The goal is to make it possible to access graphics tablets. One important thing to note is that this interface does _not_ cover the touch ability of some tablets. This should go through wl_touch (for touchscreen-like tablets) or wl_pointer (for external touchpad-like tablets). There are a few notable differences to the wl_pointer interface: * tablets have a tool type that matters, it lets applications such as the GIMP select paint tools based on the physical tool. those tools also often have HW serial numbers to uniquely identify them. * extra axes mattter: pressure, tilt, distance - all influence the stylus behaviour * more than one device may be present, so it's important to have access to all devices on a one-by-one basis. unlike wl_pointer, where we just have one virtual pointer. * proximity matters since we can leave proximity from directly above a surface. the pointer can't do that, it moves from one surface to the next. so in some ways it's closer to wl_touch in that regard. Some design notes: * generally most axes change at the same time, hence the choice to send a wl_array instead of separate events. * x/y would have to be adjusted relative to the surface, but technically the same would have to be done to e.g. distance on a true 3D desktop. * not sure at all about the relative events at all or if there's a need for it. iirc only some styly have REL_*WHEEL, do we need something else? Ping, Jason? * I don't have a specific touch event, I figured BTN_TOUCH would do the job. * focus handling for the stylus is easy. focus handling for the buttons on the pad isn't. they could technically be focused elsewhere, like a keyboard focus. some buttons are definitely stylus based (BTN_STYLUS, BTN_STYLUS2, etc.) so should go where the stylus is. Should look at what Win/OSX do here. * bind/binding/unbind - this is like windowed mode in GIMP. do we still need this? who's actually using this instead of a full-screen app? * tablet_manager is a bit meh, but the only alternative would be to have a wl_seat::get_tablets request and a wl_seat::tablet_added event. Possible, but doesn't look that much nicer. but it does away with the indirection. (read the diff below to understand what I mean here) * if we stick with the tablet_manager, do we need a wl_tablet_manager::get_tablets request in case the client releases a tablet it needs again later? or do we expect to re-bind to wl_tablet_manager? * fuzz/flat should be dropped, I just haven't yet. * I'd really like to enscribe in the protocol that ABS_PRESSURE means just that and damn anyone else who wants to use 0x18 as axis code. weston does this for wl_pointer::button, but it's not actually documented. what's the deal here? * does this even make sense as wl_tablet or should I try first as experimental weston-tablet interface that then (maybe) moves later to wayland proper. That's it so far, again, any feedback appreciated. diff below. Cheers, Peter diff --git a/protocol/wayland.xml b/protocol/wayland.xml index aeb0412..8d10746 100644 --- a/protocol/wayland.xml +++ b/protocol/wayland.xml @@ -1235,7 +1235,7 @@ /request /interface - interface name=wl_seat version=3 + interface name=wl_seat version=4 description summary=group of input devices A seat is a group of keyboards, pointer and touch devices. This object is published as a global during start up, or when such a @@ -1251,6 +1251,7 @@ entry name=pointer value=1 summary=The seat has pointer devices/ entry name=keyboard value=2 summary=The seat has one or more keyboards/ entry name=touch value=4 summary=The seat has touch devices/ + entry name=tablets value=8 summary=The seat has one or more graphics tablet devices since=4/ What's actually the reason to allow multiple graphics-tablets per seat? I thought wl_seat objects respresent a single user interacting with your desktop. So if you have two tablets, why not force them to be in two different wl_seat objects? Is there ever a reason to have multiple tablets in a single seat? What would the use-case be? We could even go further and put tablets into their own seats. Always. A pointer and keyboard may be used by a single user at the same time. But for a graphics-tablet that doesn't sound like a
Re: [RFC wayland] System compositor protocol
Hi On Tue, Aug 27, 2013 at 9:16 PM, microcai micro...@fedoraproject.org wrote: 2013/8/25 David Herrmann dh.herrm...@gmail.com: [...] The idea behind a system-compositor was to provide a system daemon that runs _outside_ a session. Its sole responsibility is to control access to graphics and input hardware. So session-compositors no longer access hardware directly, but instead tunnel it through the system-compositor. But this means, the system-compositor must know of session-switches and correctly display only the session-compositor of the active session. However, session-switching is controlled by logind, so the system-compositor gets the session-switch notification _after_ the session was actually switched, making this kind of racy (but still ok!). when the user wants to switch, logind should be notified by system-compositor. system-compositer controls the screen and the input, not logind, so there should be no race. Yeah, this is *not* how it works. Currently a session-switch is controlled by either the kernel (for VTs) or by logind. I don't see any reason why this should change. Please explain yourself if you disagree. The bigger problem is, the system-compositor is not part of a session so it has to be active *all the time*. You cannot have some sessions using the system-compositor and other sessions doing it the old way. You cannot do device-access handover from the system-compositor to a self-hosting or legacy session. This would require ugly racy hacks and conflict with VT! But if the system-compositor is always active, you cannot use VTs in text-mode. Because VTs in text-mode access graphics hardware *directly*. if we want to kill VT, then better we have system-compositor. I see no reason to support kernel VTs when you could have system-compositor. I don't see any reason to support VTs at all. But it's not me to decide, so we will always support VTs for backwards compatibility. system-compositor is not always active. In fact, it only got wake up when you do session switch. the session compositor directly render to the screen. there is no need to wake up system-compositor here. In this scenario, how does the system-compositor know whether the session that you switch to renders directly or requires the system-compositor? sessions that does not use system-compositor is a dead end. All sessions should and must output to system-compositor. system-compositor should be socket activated, and guaranteed to be there. Please elaborate. You are just claiming stuff without explaining why.. Or is this just your opinion? [...] Yepp, that's all we need. Just rename it from system_compositor to wl_fullscreen_shell (I bet you can come up with some fancier name). No other criticism on this proposal from my side. Feel free to disagree ;) I am open for suggestions or criticism. Cheers David with system-compositor, we can do cross-session visual effects that's hard to be done without a system-compositor. And hard to be done is bad? Choosing the convenient way here means adding latency (by routing everything through the system-compositor). We don't want that. And cross-session visual effects are still possible. I will present that at XDC in 1 month. with system-compositor, we provide unified input handing, multi-GPU handing, multi-monitor handing, and integrate that well with logind. Please show to me how this integrates well with logind? Multi-GPU, multi-monitor, unified input,.. that has nothing to do with a system-compositor. You can have that with session-compositors, too. In fact, we already have all that.. with system-compositor, we killed kernel VTs. we can still provide text terminal as clients of system-compositor. Has nothing to do with system-compositors.. with system-compositor, we secured our GPUs. What? In your claim above you said system-compositors *don't* always have to be active. How do you protect your GPU during such switches? with system-compositor, we can make system-compositor to load old Xorg drivers to bring up OGL and do user-mode setting while native KMS drivers absent. Again, this has nothing to do with system-compositors. You can implement that in any compositor. Regards David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [RFC wayland] System compositor protocol
Hi On Mon, Aug 26, 2013 at 10:48 AM, Cedric BAIL moa.blueb...@gmail.com wrote: Hello, Cedric Bail On Aug 26, 2013 2:11 AM, David Herrmann dh.herrm...@gmail.com wrote: Hi On Fri, Aug 23, 2013 at 11:55 PM, Jason Ekstrand ja...@jlekstrand.net wrote: Hello All, I am in the process of picking back up the old idea of system compositors. I am not, at the moment, looking for a review of the code; simply a review of the concept and the proposed protocol. If you would like to look at my implementation or try it out, it can be found in the system-compositor branch of my personal [weston fork on github][1]. What follows is what I envision for system compositors. (Others may have a different idea which is why I'm writing this RFC email.) The basic idea behind a system compositor is to provide an interface for compositors whose only job it is to display other compositors or other stand-alone full-screen interfaces. I image three primary purposes for system compositors: 1. As an abstraction layer. Every time someone wants to write a RDP server, VNC server, android backend, or the like the standard answer is write it as a weston plugin. The problem with this is that, to my knowledge, none of the major desktop environments (GNOME, KDE, EFL, etc.) plan to run their compositor as a weston shell plugin. This makes a weston plugin a poor solution in the long term. On the other hand, a system compositor is fairly simple to implement and provides a backend to any compositor that can run on top of a system compositor. KDE planned on using weston as underlying compositor. But I doubt GNOME or EFL want to use something like that.. Indeed, Enlightenment is not planning to use Weston. But if a common library dealing with all those drm / kms, android, rpi,... did exist, I am sure we would use it and contribute to it. I am not a big fan of adding a system compositor as it will impact speed and power consumption. As long as we can avoid it, and work on systemd / logind seems to go this way, I think it would be better. I did try this with kmscon, but I never moved the library out of the kmscon-repo as it is a quite fast moving target. So it will probably take a while until we figure out some proper API. And interest seems pretty low.. [...] So back to your proposal. I'd like to see something like you did but as a session-compositor (call it whatever you want ;)). So a session that doesn't want to deal with DRM directly (like for instance gdm/xdm/kdm) could avoid starting an xserver or weston and start your session-compositor instead. It then displays it's content via the standard wayland client APIs on this compositor. But this compositor imho should run in a session. So it does *not* allow clients from different sessions to connect. It is *no* system compositor. It's just a daemon which provides graphics-access to the session. It may even allow switching between multiple surfaces. And if you continue this thought, you will notice that it is nothing more than weston but with a *very* reduced wl_shell. Precisely a wl_shell that displays only one surface at a time. What would be the benefit over a library that does the job in the compositor/single application ? A library would work as long as you have only a single client. But imagine your login-session also wants a screensaver. You could make this compositor support a shell which allows a full-screen client (login-screen) and additionally a screen-saver client. So as long as your wl_whatever_shell in this compositor is limited to a single client, I agree, a library would be better. But if you make it just a little more complex, you should rather use a compositor. Regards David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 00/10] Device Management for systemd-logind
is inactive, all devices on this session are inactive, too. However, when a session is active, a device might stay inactive (for instance if reactivation failed). But logind guarantees that a device can never be used by anyone else than the foreground session. Compositors can rely on that for security reasons. logind itselfs takes care of revoking device access for inactive sessions (synchronized with session-switches!). It also tries to resume every device when a session is activated. But session-devices must not be used to watch session state! A compositor has to use the PropertiesChanged() signal plus the Active property of sessions for that! Session-devices do not replace this! On sessions with VTs, this is obviously replaced by the VT_SETMODE interface as usual. Now additionally to the interface mentioned above, this series introduces session-controllers. These try to prevent multiple compositors from running in parallel. That is, when a compositor starts up, it calls RequestControl() on the session in question. If there is already another compositor running, this will fail. The new device-management functions are limited to the active controller. No other functions make use of controllers. Note that the RequestControl() call might get a scope argument. So you can have a controller with scope graphics and one for scope sound, for example. On each scope only a single controller is allowed, but the scopes don't interfere. So logind makes sure that RequestDevice() on a graphics or input device requires the graphics scope. But now the API: RequestControl(bool force): For now this misses a scope argument, so it currently implies graphics scope. This function will make the caller the new controller of the given session. logind watches the system bus and automatically drops the controller once it disconnects. If there is already a controller for the given scope, this returns EBUSY. If @force is true, the active controller will be dropped and the caller will get the new controller. This function is restricted to callers with the same UID as the User of the given session. If @force is true, the caller must be root. So this call in combination with RequestDevice() allows us to run a compositor in a session as normal user. Yay! DropControl(): Drop control again. If the caller is not the current controller, this does nothing. Note that this call is optional. logind watches the bus for disconnect events and invokes this implicitly if a controller exits. David Herrmann (10): logind: listen actively for session devices logind: add infrastructure to watch busnames logind: add session controllers logind: make Session.Activate() lazy logind: introduce session-devices logind: rename vtconsole to seat0 logind: fix seat_can_tty() to check for VTs logind: fix session_activate(vtnr = 0) logind: extract has_vts() from can_multi_session() logind: implement generic multi-session Makefile.am | 2 + src/login/logind-dbus.c | 35 ++- src/login/logind-device.c | 39 ++- src/login/logind-device.h | 5 +- src/login/logind-seat.c | 73 -- src/login/logind-seat.h | 6 +- src/login/logind-session-dbus.c | 156 src/login/logind-session-device.c | 517 ++ src/login/logind-session-device.h | 58 + src/login/logind-session.c| 112 - src/login/logind-session.h| 8 + src/login/logind.c| 151 +-- src/login/logind.h| 12 +- 13 files changed, 1109 insertions(+), 65 deletions(-) create mode 100644 src/login/logind-session-device.c create mode 100644 src/login/logind-session-device.h -- 1.8.3.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 01/10] logind: listen actively for session devices
Session compositors need access to fbdev, DRM and evdev devices if they control a session. To make logind pass them to sessions, we need to listen for them actively. However, we avoid creating new seats for non master-of-seat devices. Only once a seat is created, we start remembering all other session devices. If the last master-device is removed (even if there are other non-master devices still available), we destroy the seat. This is the current behavior, but we need to explicitly implement it now as there may be non-master devices in the seat-devices list. Unlike master devices, we don't care whether our list of non-master devices is complete. We don't export this list but use it only as cache if sessions request these devices. Hence, if a session requests a device that is not in the list, we will simply look it up. However, once a session requested a device, we must be notified of remove udev events. So we must link the devices somehow into the device-list. Regarding the implementation, we now sort the device list by the master flag. This guarantees that master devices are at the front and non-master devices at the tail of the list. Thus, we can easily test whether a seat has a master device attached. --- src/login/logind-device.c | 35 ++--- src/login/logind-device.h | 3 +- src/login/logind-seat.c | 11 +-- src/login/logind-seat.h | 1 + src/login/logind.c| 79 +-- src/login/logind.h| 6 ++-- 6 files changed, 116 insertions(+), 19 deletions(-) diff --git a/src/login/logind-device.c b/src/login/logind-device.c index 51b1535..a9a9633 100644 --- a/src/login/logind-device.c +++ b/src/login/logind-device.c @@ -25,7 +25,7 @@ #include logind-device.h #include util.h -Device* device_new(Manager *m, const char *sysfs) { +Device* device_new(Manager *m, const char *sysfs, bool master) { Device *d; assert(m); @@ -48,6 +48,7 @@ Device* device_new(Manager *m, const char *sysfs) { } d-manager = m; +d-master = master; dual_timestamp_get(d-timestamp); return d; @@ -75,11 +76,16 @@ void device_detach(Device *d) { LIST_REMOVE(Device, devices, d-seat-devices, d); d-seat = NULL; -seat_add_to_gc_queue(s); -seat_send_changed(s, CanGraphical\0); +if (!seat_has_master_device(s)) { +seat_add_to_gc_queue(s); +seat_send_changed(s, CanGraphical\0); +} } void device_attach(Device *d, Seat *s) { +Device *i; +bool had_master; + assert(d); assert(s); @@ -90,7 +96,26 @@ void device_attach(Device *d, Seat *s) { device_detach(d); d-seat = s; -LIST_PREPEND(Device, devices, s-devices, d); +had_master = seat_has_master_device(s); + +/* We keep the device list sorted by the master flag. That is, master + * devices are at the front, other devices at the tail. As there is no + * way to easily add devices at the list-tail, we need to iterate the + * list to find the first non-master device when adding non-master + * devices. We assume there is only a few (normally 1) master devices + * per seat, so we iterate only a few times. */ + +if (d-master || !s-devices) { +LIST_PREPEND(Device, devices, s-devices, d); +} else { +LIST_FOREACH(devices, i, s-devices) { +if (!i-devices_next || !i-master) { +LIST_INSERT_AFTER(Device, devices, s-devices, i, d); +break; +} +} +} -seat_send_changed(s, CanGraphical\0); +if (!had_master d-master) +seat_send_changed(s, CanGraphical\0); } diff --git a/src/login/logind-device.h b/src/login/logind-device.h index 3b15356..315f0e6 100644 --- a/src/login/logind-device.h +++ b/src/login/logind-device.h @@ -33,13 +33,14 @@ struct Device { char *sysfs; Seat *seat; +bool master; dual_timestamp timestamp; LIST_FIELDS(struct Device, devices); }; -Device* device_new(Manager *m, const char *sysfs); +Device* device_new(Manager *m, const char *sysfs, bool master); void device_free(Device *d); void device_attach(Device *d, Seat *s); void device_detach(Device *d); diff --git a/src/login/logind-seat.c b/src/login/logind-seat.c index 470d08b..2c60b8a 100644 --- a/src/login/logind-seat.c +++ b/src/login/logind-seat.c @@ -448,10 +448,17 @@ bool seat_can_tty(Seat *s) { return seat_is_vtconsole(s); } +bool seat_has_master_device(Seat *s) { +assert(s); + +/* device list is ordered by master flag */ +return !!s-devices s-devices-master; +} + bool seat_can_graphical(Seat *s) { assert(s); -return !!s-devices; +return
[PATCH 02/10] logind: add infrastructure to watch busnames
If we want to track bus-names to allow exclusive resource-access, we need a way to get notified when a bus-name is gone. We make logind watch for NameOwnerChanged dbus events and check whether the name is currently watched. If it is, we remove it from the watch-list (notification for other objects can be added in follow-up patches). --- src/login/logind-dbus.c | 17 src/login/logind.c | 54 - src/login/logind.h | 4 3 files changed, 74 insertions(+), 1 deletion(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 345df9f..eb62d28 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -2458,6 +2458,23 @@ DBusHandlerResult bus_message_filter( HASHMAP_FOREACH(session, m-sessions, i) session_add_to_gc_queue(session); } + +} else if (dbus_message_is_signal(message, DBUS_INTERFACE_DBUS, NameOwnerChanged)) { +const char *name, *old, *new; +char *key; + +if (!dbus_message_get_args(message, error, + DBUS_TYPE_STRING, name, + DBUS_TYPE_STRING, old, + DBUS_TYPE_STRING, new, + DBUS_TYPE_INVALID)) { +log_error(Failed to parse NameOwnerChanged message: %s, bus_error_message(error)); +goto finish; +} + +if (*old !*new (key = hashmap_remove(m-busnames, old))) { +free(key); +} } finish: diff --git a/src/login/logind.c b/src/login/logind.c index 54fb391..f4cef54 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -74,6 +74,7 @@ Manager *manager_new(void) { m-users = hashmap_new(trivial_hash_func, trivial_compare_func); m-inhibitors = hashmap_new(string_hash_func, string_compare_func); m-buttons = hashmap_new(string_hash_func, string_compare_func); +m-busnames = hashmap_new(string_hash_func, string_compare_func); m-user_units = hashmap_new(string_hash_func, string_compare_func); m-session_units = hashmap_new(string_hash_func, string_compare_func); @@ -82,7 +83,7 @@ Manager *manager_new(void) { m-inhibitor_fds = hashmap_new(trivial_hash_func, trivial_compare_func); m-button_fds = hashmap_new(trivial_hash_func, trivial_compare_func); -if (!m-devices || !m-seats || !m-sessions || !m-users || !m-inhibitors || !m-buttons || +if (!m-devices || !m-seats || !m-sessions || !m-users || !m-inhibitors || !m-buttons || !m-busnames || !m-user_units || !m-session_units || !m-session_fds || !m-inhibitor_fds || !m-button_fds) { manager_free(m); @@ -111,6 +112,7 @@ void manager_free(Manager *m) { Seat *s; Inhibitor *i; Button *b; +char *n; assert(m); @@ -132,12 +134,16 @@ void manager_free(Manager *m) { while ((b = hashmap_first(m-buttons))) button_free(b); +while ((n = hashmap_first(m-busnames))) +free(hashmap_remove(m-busnames, n)); + hashmap_free(m-devices); hashmap_free(m-seats); hashmap_free(m-sessions); hashmap_free(m-users); hashmap_free(m-inhibitors); hashmap_free(m-buttons); +hashmap_free(m-busnames); hashmap_free(m-user_units); hashmap_free(m-session_units); @@ -361,6 +367,40 @@ int manager_add_button(Manager *m, const char *name, Button **_button) { return 0; } +int manager_watch_busname(Manager *m, const char *name) { +char *n; +int r; + +assert(m); +assert(name); + +if (hashmap_get(m-busnames, name)) +return 0; + +n = strdup(name); +if (!n) +return -ENOMEM; + +r = hashmap_put(m-busnames, n, n); +if (r 0) { +free(n); +return r; +} + +return 0; +} + +void manager_drop_busname(Manager *m, const char *name) { +char *key; + +assert(m); +assert(name); + +key = hashmap_remove(m-busnames, name); +if (key) +free(key); +} + int manager_process_seat_device(Manager *m, struct udev_device *d) { Device *device; int r; @@ -1039,6 +1079,18 @@ static int manager_connect_bus(Manager *m) { dbus_bus_add_match(m-bus, type='signal', + sender='DBUS_SERVICE_DBUS', + interface='DBUS_INTERFACE_DBUS', + member='NameOwnerChanged', + path='DBUS_PATH_DBUS', + error); +if
[PATCH 03/10] logind: add session controllers
A session usually has only a single compositor or other application that controls graphics and input devices on it. To avoid multiple applications from hijacking each other's devices or even using the devices in parallel, we add session controllers. A session controller is an application that manages a session. Specific API calls may be limited to controllers to avoid others from getting unprivileged access to restricted resources. A session becomes a controller by calling the RequestControl() dbus API call. It can drop it via ReleaseControl(). logind tracks bus-names to release the controller once an application closes the bus. We use the new bus-name tracking to do that. Note that during ReleaseControl() we need to check whether some other session also tracks the name before we remove it from the bus-name tracking list. Currently, we only allow one controller at a time. However, the public API does not enforce this restriction. So if it makes sense, we can allow multiple controllers in parallel later. Or we can add a scope parameter, which allows a different controller for graphics-devices, sound-devices and whatever you want. Note that currently you get -EBUSY if there is already a controller. You can force the RequestControl() call (root-only) to drop the current controller and recover the session during an emergency. To recover a seat, this is not needed, though. You can simply create a new session or force-activate it. To become a session controller, a dbus caller must either be root or the same user as the user of the session. This allows us to run a session compositor as user and we no longer need any CAP_SYS_ADMIN. --- src/login/logind-dbus.c | 8 +++ src/login/logind-session-dbus.c | 42 src/login/logind-session.c | 48 + src/login/logind-session.h | 6 ++ src/login/logind.c | 10 + 5 files changed, 114 insertions(+) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index eb62d28..a703e59 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -2472,8 +2472,16 @@ DBusHandlerResult bus_message_filter( goto finish; } +/* drop all controllers owned by this name */ if (*old !*new (key = hashmap_remove(m-busnames, old))) { +Session *session; +Iterator i; + free(key); + +HASHMAP_FOREACH(session, m-sessions, i) +if (session_is_controller(session, old)) +session_drop_controller(session); } } diff --git a/src/login/logind-session-dbus.c b/src/login/logind-session-dbus.c index 2cc4d85..b8b32cd 100644 --- a/src/login/logind-session-dbus.c +++ b/src/login/logind-session-dbus.c @@ -40,6 +40,10 @@ arg name=\who\ type=\s\/\n \ arg name=\signal\ type=\s\/\n\ /method\n \ + method name=\RequestControl\/\n \ + arg name=\force\ type=\b\/\n \ + /method\n \ + method name=\DropControl\/\n\ signal name=\Lock\/\n \ signal name=\Unlock\/\n \ property name=\Id\ type=\s\ access=\read\/\n\ @@ -366,6 +370,44 @@ static DBusHandlerResult session_message_dispatch( if (!reply) goto oom; +} else if (dbus_message_is_method_call(message, org.freedesktop.login1.Session, RequestControl)) { +dbus_bool_t force; +unsigned long ul; + +if (!dbus_message_get_args( +message, +error, +DBUS_TYPE_BOOLEAN, force, +DBUS_TYPE_INVALID)) +return bus_send_error_reply(connection, message, error, -EINVAL); + +ul = dbus_bus_get_unix_user(connection, dbus_message_get_sender(message), error); +if (ul == (unsigned long) -1) +return bus_send_error_reply(connection, message, error, -EIO); + +if (ul != 0 (force || ul != s-user-uid)) +return bus_send_error_reply(connection, message, NULL, -EPERM); + +r = session_set_controller(s, bus_message_get_sender_with_fallback(message), force); +if (r 0) +return bus_send_error_reply(connection, message, NULL, r); + +reply =
[PATCH 04/10] logind: make Session.Activate() lazy
Currently, Activate() calls chvt(), which does an ioctl(VT_ACTIVATE) and immediately calls seat_set_active(). However, VTs are allowed to prevent being deactivated. Therefore, logind cannot be sure the VT_ACTIVATE call was actually successful. Furthermore, compositors often need to clean up their devices before they acknowledge the VT switch. The immediate call to seat_set_active() may modify underlying ACLs, though. Thus, some compositors may fail cleaning up their stuff. Moreover, the compositor being switched to (if listening to logind instead of VTs) will not be able to activate its devices if the old VT still has them active. We could simply add an VT_WAITACTIVE call, which blocks until the given VT is active. However, this can block forever if the compositor hangs. So to fix this, we make Activate() lazy. That is, it only schedules a session-switch but does not wait for it to complete. The caller can no longer rely on it being immediate. Instead, a caller is required to wait for the PropertiesChanged signal and read the Active field. We could make Activate() wait asynchronously for the session-switch to complete and then send the return-message afterwards. However, this would add a lot of state-tracking with no real gain: 1) Sessions normally don't care whether Activate() was actually successful as they currently _must_ wait for the VT activation to do anything for real. 2) Error messages for failed session switches can be printed by logind instead of the session issuing Activate(). 3) Sessions that require synchronous Activate() calls can simply issue the call and then wait for Active properties to change. This also allows them to implement their own timeout. This change prepares for multi-session on seats without VTs. Forced VT switches are always bad as compositors cannot perform any cleanup. This isn't strictly required, but may lead to loss of information and ambiguous error messages. So for multi-session on seats without VTs, we must wait for the current session to clean-up before finalizing the session-switch. This requires Activate() to be lazy as we cannot block here. Note that we can always implement a timeout which allows us to guarantee the session switch to happen. Nevertheless, this calls for a lazy Activate(). --- src/login/logind-session.c | 8 +--- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/login/logind-session.c b/src/login/logind-session.c index 6c6a2c8..a11804a 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -359,8 +359,6 @@ int session_load(Session *s) { } int session_activate(Session *s) { -int r; - assert(s); assert(s-user); @@ -375,11 +373,7 @@ int session_activate(Session *s) { assert(seat_is_vtconsole(s-seat)); -r = chvt(s-vtnr); -if (r 0) -return r; - -return seat_set_active(s-seat, s); +return chvt(s-vtnr); } static int session_link_x11_socket(Session *s) { -- 1.8.3.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 06/10] logind: rename vtconsole to seat0
The seat-vtconsole member always points to the default seat seat0. Even if VTs are disabled, it's used as default seat. Therefore, rename it to seat0 to correctly state what it is. This also changes the seat files in /run from IS_VTCONSOLE to IS_SEAT0. It wasn't used by any code, yet, so this seems fine. While we are at it, we also remove every if (s-vtconsole) as this pointer is always valid! --- src/login/logind-dbus.c| 8 src/login/logind-seat.c| 14 +++--- src/login/logind-seat.h| 2 +- src/login/logind-session.c | 2 +- src/login/logind.c | 8 src/login/logind.h | 2 +- 6 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index a703e59..8940aeb 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -404,8 +404,8 @@ static int bus_manager_create_session(Manager *m, DBusMessage *message) { int v; if (!seat) -seat = m-vtconsole; -else if (seat != m-vtconsole) +seat = m-seat0; +else if (seat != m-seat0) return -EINVAL; v = vtnr_from_tty(tty); @@ -420,8 +420,8 @@ static int bus_manager_create_session(Manager *m, DBusMessage *message) { } else if (tty_is_console(tty)) { if (!seat) -seat = m-vtconsole; -else if (seat != m-vtconsole) +seat = m-seat0; +else if (seat != m-seat0) return -EINVAL; if (vtnr != 0) diff --git a/src/login/logind-seat.c b/src/login/logind-seat.c index dcaf0ac..88fd724 100644 --- a/src/login/logind-seat.c +++ b/src/login/logind-seat.c @@ -105,11 +105,11 @@ int seat_save(Seat *s) { fprintf(f, # This is private data. Do not parse.\n -IS_VTCONSOLE=%i\n +IS_SEAT0=%i\n CAN_MULTI_SESSION=%i\n CAN_TTY=%i\n CAN_GRAPHICAL=%i\n, -seat_is_vtconsole(s), +seat_is_seat0(s), seat_can_multi_session(s), seat_can_tty(s), seat_can_graphical(s)); @@ -429,16 +429,16 @@ int seat_attach_session(Seat *s, Session *session) { return 0; } -bool seat_is_vtconsole(Seat *s) { +bool seat_is_seat0(Seat *s) { assert(s); -return s-manager-vtconsole == s; +return s-manager-seat0 == s; } bool seat_can_multi_session(Seat *s) { assert(s); -if (!seat_is_vtconsole(s)) +if (!seat_is_seat0(s)) return false; /* If we can't watch which VT is in the foreground, we don't @@ -450,7 +450,7 @@ bool seat_can_multi_session(Seat *s) { bool seat_can_tty(Seat *s) { assert(s); -return seat_is_vtconsole(s); +return seat_is_seat0(s); } bool seat_has_master_device(Seat *s) { @@ -508,7 +508,7 @@ int seat_check_gc(Seat *s, bool drop_not_started) { if (drop_not_started !s-started) return 0; -if (seat_is_vtconsole(s)) +if (seat_is_seat0(s)) return 1; return seat_has_master_device(s); diff --git a/src/login/logind-seat.h b/src/login/logind-seat.h index bd5390f..47fe89a 100644 --- a/src/login/logind-seat.h +++ b/src/login/logind-seat.h @@ -60,7 +60,7 @@ int seat_preallocate_vts(Seat *s); int seat_attach_session(Seat *s, Session *session); -bool seat_is_vtconsole(Seat *s); +bool seat_is_seat0(Seat *s); bool seat_can_multi_session(Seat *s); bool seat_can_tty(Seat *s); bool seat_has_master_device(Seat *s); diff --git a/src/login/logind-session.c b/src/login/logind-session.c index 69a8ad7..ae91650 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -386,7 +386,7 @@ int session_activate(Session *s) { if (s-seat-active == s) return 0; -assert(seat_is_vtconsole(s-seat)); +assert(seat_is_seat0(s-seat)); return chvt(s-vtnr); } diff --git a/src/login/logind.c b/src/login/logind.c index ad3ab81..d18bb08 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -852,7 +852,7 @@ int manager_dispatch_vcsa_udev(Manager *m) { * VTs, to make sure our auto VTs never go away. */ if (name startswith(name, vcsa) streq_ptr(udev_device_get_action(d), remove)) -r = seat_preallocate_vts(m-vtconsole); +r = seat_preallocate_vts(m-seat0); udev_device_unref(d); @@ -877,9 +877,9 @@ int manager_dispatch_button_udev(Manager *m) { int manager_dispatch_console(Manager *m) { assert(m); +assert(m-seat0); -if (m-vtconsole) -seat_read_active_vt(m-vtconsole); +seat_read_active_vt(m-seat0); return 0; } @@ -1537,7 +1537,7 @@ int
[PATCH 07/10] logind: fix seat_can_tty() to check for VTs
A seat provides text-logins if it has VTs. This is always limited to seat0 so the seat_is_seat0() check is correct. However, if VTs are disabled, no seat provides text-logins so we also need to check for the console-fd. This was previously: return seat_is_vtconsole(); It looked right, but was functionally equivalent to seat_is_seat0(). The rename of this helper made it more obvious that it is missing the VT test. --- src/login/logind-seat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/login/logind-seat.c b/src/login/logind-seat.c index 88fd724..4c2c424 100644 --- a/src/login/logind-seat.c +++ b/src/login/logind-seat.c @@ -450,7 +450,7 @@ bool seat_can_multi_session(Seat *s) { bool seat_can_tty(Seat *s) { assert(s); -return seat_is_seat0(s); +return seat_is_seat0(s) s-manager-console_active_fd = 0; } bool seat_has_master_device(Seat *s) { -- 1.8.3.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 08/10] logind: fix session_activate(vtnr = 0)
VT numbers start with 1. If a session has vtnr == 0, we must not assume it is running on a VT. Note that this could trigger the assert() below as CreateSession() sets vtnr to 0, not 0. --- src/login/logind-session.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/login/logind-session.c b/src/login/logind-session.c index ae91650..50ba6b8 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -377,7 +377,7 @@ int session_activate(Session *s) { assert(s); assert(s-user); -if (s-vtnr 0) +if (s-vtnr = 0) return -ENOTSUP; if (!s-seat) -- 1.8.3.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 09/10] logind: extract has_vts() from can_multi_session()
We currently use seat_can_multi_session() to test for two things: * whether the seat can handle session-switching * whether the seat has VTs As both are currently logically equivalent, we didn't care. However, we want to allow session-switching on seats without VTs, so split this helper into: * seat_can_multi_session(): whether session-switching is supported * seat_has_vts(): whether the seat has VTs Note that only one seat on a system can have VTs. There is only one set of them. We automatically assign them to seat0 as usual. With this patch in place, we can easily add new session-switching/tracking methods without breaking any VT code as it is now protected by has_vts(), no longer by can_multi_session(). --- src/login/logind-dbus.c| 2 +- src/login/logind-seat.c| 32 ++-- src/login/logind-seat.h| 1 + src/login/logind-session.c | 6 +++--- 4 files changed, 19 insertions(+), 22 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 8940aeb..5e42c0a 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -429,7 +429,7 @@ static int bus_manager_create_session(Manager *m, DBusMessage *message) { } if (seat) { -if (seat_can_multi_session(seat)) { +if (seat_has_vts(seat)) { if (vtnr 63) return -EINVAL; } else { diff --git a/src/login/logind-seat.c b/src/login/logind-seat.c index 4c2c424..f88738a 100644 --- a/src/login/logind-seat.c +++ b/src/login/logind-seat.c @@ -201,7 +201,7 @@ int seat_preallocate_vts(Seat *s) { if (s-manager-n_autovts = 0) return 0; -if (!seat_can_multi_session(s)) +if (!seat_has_vts(s)) return 0; for (i = 1; i = s-manager-n_autovts; i++) { @@ -282,7 +282,7 @@ int seat_active_vt_changed(Seat *s, int vtnr) { assert(s); assert(vtnr = 1); -if (!seat_can_multi_session(s)) +if (!seat_has_vts(s)) return -EINVAL; log_debug(VT changed to %i, vtnr); @@ -306,7 +306,7 @@ int seat_read_active_vt(Seat *s) { assert(s); -if (!seat_can_multi_session(s)) +if (!seat_has_vts(s)) return 0; lseek(s-manager-console_active_fd, SEEK_SET, 0); @@ -417,18 +417,20 @@ int seat_attach_session(Seat *s, Session *session) { seat_send_changed(s, Sessions\0); -/* Note that even if a seat is not multi-session capable it - * still might have multiple sessions on it since old, dead - * sessions might continue to be tracked until all their - * processes are gone. The most recently added session - * (i.e. the first in s-sessions) is the one that matters. */ - -if (!seat_can_multi_session(s)) +/* On seats with VTs, the VT logic defines which session is active. On + * seats without VTs, we automatically activate the first session. */ +if (!seat_has_vts(s) !s-active) seat_set_active(s, session); return 0; } +bool seat_has_vts(Seat *s) { +assert(s); + +return seat_is_seat0(s) s-manager-console_active_fd = 0; +} + bool seat_is_seat0(Seat *s) { assert(s); @@ -438,19 +440,13 @@ bool seat_is_seat0(Seat *s) { bool seat_can_multi_session(Seat *s) { assert(s); -if (!seat_is_seat0(s)) -return false; - -/* If we can't watch which VT is in the foreground, we don't - * support VT switching */ - -return s-manager-console_active_fd = 0; +return seat_has_vts(s); } bool seat_can_tty(Seat *s) { assert(s); -return seat_is_seat0(s) s-manager-console_active_fd = 0; +return seat_has_vts(s); } bool seat_has_master_device(Seat *s) { diff --git a/src/login/logind-seat.h b/src/login/logind-seat.h index 47fe89a..d3438b8 100644 --- a/src/login/logind-seat.h +++ b/src/login/logind-seat.h @@ -60,6 +60,7 @@ int seat_preallocate_vts(Seat *s); int seat_attach_session(Seat *s, Session *session); +bool seat_has_vts(Seat *s); bool seat_is_seat0(Seat *s); bool seat_can_multi_session(Seat *s); bool seat_can_tty(Seat *s); diff --git a/src/login/logind-session.c b/src/login/logind-session.c index 50ba6b8..e7fe0f6 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -205,7 +205,7 @@ int session_save(Session *s) { if (s-service) fprintf(f, SERVICE=%s\n, s-service); -if (s-seat seat_can_multi_session(s-seat)) +if (s-seat seat_has_vts(s-seat)) fprintf(f, VTNR=%i\n, s-vtnr); if (s-leader 0) @@ -315,7 +315,7 @@ int session_load(Session *s) { seat_attach_session(o, s); } -if (vtnr s-seat seat_can_multi_session(s-seat)) { +if (vtnr s-seat seat_has_vts(s-seat)) {
[PATCH 10/10] logind: implement generic multi-session
This enables the multi-session capability for seats that don't have VTs. For legacy seats with VTs, everything stays the same. However, all other seats now also get the multi-session capability. The only feature that was missing was session-switching. As logind can force a session-switch and signal that via the Active property, we only need a way to allow synchronized/delayed session switches. Compositors need to cleanup some devices before acknowledging the session switch. Therefore, we use the session-devices to give compositors a chance to block a session-switch until they cleaned everything up. If you activate a session on a seat without VTs, we send a PauseDevice signal to the active session for every active device. Only once the session acknowledged all these with a PauseDeviceComplete() call, we perform the final session switch. One important note is that delayed session-switching is meant for backwards compatibility. New compositors or other sessions should really try to deal correctly with forced session switches! They only need to handle EACCES/EPERM from syscalls and treat them as PauseDevice signal. Following logind patches will add a timeout to session-switches which forces the switch if the active session does not react in a timely fashion. Moreover, explicit ForceActivate() calls might also be supported. Hence, sessions must not crash if their devices get paused. --- src/login/logind-seat.c | 15 +++ src/login/logind-seat.h | 2 ++ src/login/logind-session-device.c | 28 src/login/logind-session-device.h | 1 + src/login/logind-session.c| 31 ++- 5 files changed, 72 insertions(+), 5 deletions(-) diff --git a/src/login/logind-seat.c b/src/login/logind-seat.c index f88738a..4a4d40a 100644 --- a/src/login/logind-seat.c +++ b/src/login/logind-seat.c @@ -425,6 +425,21 @@ int seat_attach_session(Seat *s, Session *session) { return 0; } +void seat_complete_switch(Seat *s) { +Session *session; + +assert(s); + +/* if no session-switch is pending or if it got canceled, do nothing */ +if (!s-pending_switch) +return; + +session = s-pending_switch; +s-pending_switch = NULL; + +seat_set_active(s, session); +} + bool seat_has_vts(Seat *s) { assert(s); diff --git a/src/login/logind-seat.h b/src/login/logind-seat.h index d3438b8..be6db6e 100644 --- a/src/login/logind-seat.h +++ b/src/login/logind-seat.h @@ -38,6 +38,7 @@ struct Seat { LIST_HEAD(Device, devices); Session *active; +Session *pending_switch; LIST_HEAD(Session, sessions); bool in_gc_queue:1; @@ -59,6 +60,7 @@ int seat_read_active_vt(Seat *s); int seat_preallocate_vts(Seat *s); int seat_attach_session(Seat *s, Session *session); +void seat_complete_switch(Seat *s); bool seat_has_vts(Seat *s); bool seat_is_seat0(Seat *s); diff --git a/src/login/logind-session-device.c b/src/login/logind-session-device.c index 7c6b07f..7123955 100644 --- a/src/login/logind-session-device.c +++ b/src/login/logind-session-device.c @@ -452,10 +452,21 @@ void session_device_free(SessionDevice *sd) { } void session_device_complete_pause(SessionDevice *sd) { +SessionDevice *iter; +Iterator i; + if (!sd-active) return; session_device_stop(sd); + +/* if not all devices are paused, wait for further completion events */ +HASHMAP_FOREACH(iter, sd-session-devices, i) +if (iter-active) +return; + +/* complete any pending session switch */ +seat_complete_switch(sd-session-seat); } void session_device_resume_all(Session *s) { @@ -487,3 +498,20 @@ void session_device_pause_all(Session *s) { } } } + +unsigned int session_device_try_pause_all(Session *s) { +SessionDevice *sd; +Iterator i; +unsigned int num_pending = 0; + +assert(s); + +HASHMAP_FOREACH(sd, s-devices, i) { +if (sd-active) { +session_device_notify(sd, SESSION_DEVICE_TRY_PAUSE); +++num_pending; +} +} + +return num_pending; +} diff --git a/src/login/logind-session-device.h b/src/login/logind-session-device.h index 98e61e6..61732bc 100644 --- a/src/login/logind-session-device.h +++ b/src/login/logind-session-device.h @@ -55,3 +55,4 @@ void session_device_complete_pause(SessionDevice *sd); void session_device_resume_all(Session *s); void session_device_pause_all(Session *s); +unsigned int session_device_try_pause_all(Session *s); diff --git a/src/login/logind-session.c b/src/login/logind-session.c index e7fe0f6..61366c1 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -99,6 +99,8 @@ void session_free(Session *s) { if (s-seat) {
Re: [PATCH wayland-web] building: Recommend disabling setuid for non-root installs
Hi On Sun, Aug 25, 2013 at 9:57 AM, Pekka Paalanen ppaala...@gmail.com wrote: On Fri, 23 Aug 2013 19:54:17 + Bryce W. Harrington b.harring...@samsung.com wrote: The build directions guide the user to build and install Wayland in a user directory without using superuser privs. However, the weston build process includes a `chown root` on weston-launcher done by default, which results in a failed build when running make install as non-superuser, unless we pass --enable-setuid-install=no to disable the feature. (Probably would be better to fix weston so make install works as non-root, but for now just document the workaround...) Signed-off-by: Bryce Harrington b.harring...@samsung.com --- building.html |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/building.html b/building.html index b7a1158..a47744e 100644 --- a/building.html +++ b/building.html @@ -228,7 +228,7 @@ gio-2.0./p pre$ git clone git://anongit.freedesktop.org/wayland/weston $ cd weston -$ ./autogen.sh --prefix=$WLD +$ ./autogen.sh --prefix=$WLD --enable-setuid-install=no $ make $ make install /pre I believe setuid weston-launch is mandatory for all backends that use evdev, that is at least drm and fbdev. Otherwise they cannot open input devices on normal system setups. Maybe prefer 'sudo make install' then? It is also mandatory for DRM (see drmSetMaster()). So I'd recommend sudo, too. Cheers David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 10/10] logind: implement generic multi-session
Hi On Sun, Aug 25, 2013 at 5:17 PM, Tom Gundersen t...@jklm.no wrote: Hi David, On Sun, Aug 25, 2013 at 8:46 PM, David Herrmann dh.herrm...@gmail.com wrote: One important note is that delayed session-switching is meant for backwards compatibility. New compositors or other sessions should really try to deal correctly with forced session switches! They only need to handle EACCES/EPERM from syscalls and treat them as PauseDevice signal. Following logind patches will add a timeout to session-switches which forces the switch if the active session does not react in a timely fashion. Moreover, explicit ForceActivate() calls might also be supported. Hence, sessions must not crash if their devices get paused. [...] Do you have reason to believe that handling EACCESS would be significantly harder than just porting to the PauseDevice interface? [...] Yes. For the XServer device handling is done in drivers. There are many of xf86-video-* drivers and I have no intention to fix all of them. Note that device-enumeration (udev etc) is done in the xserver core. This could be fixed more easily. But as said in the introduction-text, this is *only* for compatibility. And the longer I think about it, I get more and more convinced to drop it all-together. It's handy to test it and play around with it, but maybe the final revision would be better off without it. If anyone wants to run an xserver with it, they ought to use a driver that can deal with asynchronous session-switches. Cheers David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [RFC] weston: Sony clickpad support
Hi On Sat, Aug 17, 2013 at 9:49 AM, Peter Hutterer peter.hutte...@who-t.net wrote: On Thu, Aug 15, 2013 at 02:48:48PM -0700, Kristian Høgsberg wrote: On Thu, Aug 15, 2013 at 12:10:30PM +0100, Daniel Stone wrote: Hi, On 15 August 2013 11:52, Peter Hutterer peter.hutte...@who-t.net wrote: one of the things that should be done is to figure out _where_ features such as this are going to be handled. In the compositor, the compositor's input module, on the client side, ... ? I'm trying to figure out how to handle this correctly, but don't have much to show here just yet. For example, wl_pointer only has a button event, which means that a client cannot differ between a tap and a button click. no doubt this should be in a piece of shared code, but right now it's not quite sure what can be shared where yet. ... does it need to? I don't think we do... I think that from a client point of view, a touchpad/clickpad looks exactly like a wl_pointer. Gestures such as two-finger scrolling, double-tap-drag, two-finger clicks, interpreting clicking in different as different buttons etc can all be done in the compositor and sent as either wl_pointer button and move events or axis events (scroll, pinch, rotate). then you need gestures on the protocol. tapping is relatively uncomplicated, scrolling somewhere along the same lines (until you consider that swipe and two-finger scroll are overlapping and context-dependent). pinch and rotate require either the raw touch point data for re-interpretation, or at least the delta positions, possibly the pressure of the touchpoints, etc. then you need the number of touchpoints, and to tell that the client beforehand so they know whether they can expect a four-finger gesture or not. I think wl_pointer was intended as interfaces without gestures. All state-machines are inside the compositor. And honestly I think that's what most applications want. What you are suggesting is to allow applications to get raw touch data for touchpads/trackpads. As this data stream is quite heavy, I think we should avoid sending it to uninterested clients. So how about keeping wl_pointer as it is (and probably adding additional events similar to scrolling), but adding a get_touch() call to wl_pointer, which returns a wl_touch object for this exact pointer. It's the clients responsibility to notice that this is now used for relative, not absolute motion. Note that we have the cancel event on wl_touch which can cancel gestures if the parent wl_pointer already interpreted it. We can also add a bitmask to get_touch() which lets the client tell the compositor which gesture-detection to disable. So for instance a client could do 2-finger-scroll detection and convert it to whatever they want. They'd have to tell the compositor to disable it's own gesture detection, to avoid getting a cancel event and the axis-movement. Regards David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v2 wayland] pkg-config: scanner isn't arch-independent
Hi On Fri, Aug 16, 2013 at 2:26 PM, Daniel Stone dan...@fooishbar.org wrote: $(datadir) rather than $(libdir), is for architecture-independent data. pkg-config files should only be installed there if they're shareable across architectures, e.g. headers/data only, which wayland-scanner is not. Move it to $(libdir) with the other pkg-config files. Signed-off-by: Daniel Stone dan...@fooishbar.org --- v2: Oops, wasn't looking when I pulled this over from my other tree. Fix = vs. +=. src/Makefile.am | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 4226f63..efdfaf4 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -58,8 +58,7 @@ wayland_scanner_LDADD = $(EXPAT_LIBS) libwayland-util.la $(BUILT_SOURCES) : wayland-scanner -scannerpkgconfigdir = $(datadir)/pkgconfig -scannerpkgconfig_DATA = wayland-scanner.pc +pkgconfig_DATA += wayland-scanner.pc Yepp, we should definitely use pkgconfig_DATA. Reviewed-by: David Herrmann dh.herrm...@gmail.com Cheers David endif BUILT_SOURCES =\ -- 1.8.3.1 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v3] Fixes CJK wide character display
Hi On Wed, Aug 14, 2013 at 2:30 AM, Kristian Høgsberg hoegsb...@gmail.com wrote: On Thu, Aug 01, 2013 at 01:49:03PM +0800, Peng Wu wrote: By jumping two columns when wide character prints, and draw wide cursor under wide character. Hi Peng, I do want this feature in weston-terminal, but there's still a few issues to address - see below. --- clients/Makefile.am | 2 +- clients/terminal.c | 26 -- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/clients/Makefile.am b/clients/Makefile.am index 09963cc..e7c0ece 100644 --- a/clients/Makefile.am +++ b/clients/Makefile.am @@ -113,7 +113,7 @@ weston_screenshooter_SOURCES =\ weston_screenshooter_LDADD = libtoytoolkit.la weston_terminal_SOURCES = terminal.c -weston_terminal_LDADD = libtoytoolkit.la -lutil +weston_terminal_LDADD = libtoytoolkit.la -lutil $(PANGO_LIBS) Again, I don't want to link to pango or glib... remember, the only reason for weston-terminal to exist is that it doesn't have any dependencies except cairo and the wayland libraries. weston_image_SOURCES = image.c weston_image_LDADD = libtoytoolkit.la diff --git a/clients/terminal.c b/clients/terminal.c index 0d4f726..dc56745 100644 --- a/clients/terminal.c +++ b/clients/terminal.c @@ -33,6 +33,10 @@ #include ctype.h #include cairo.h #include sys/epoll.h +#include glib.h +#define __USE_XOPEN +#include wchar.h +#include locale.h #include wayland-client.h @@ -93,6 +97,11 @@ union utf8_char { uint32_t ch; }; +static bool is_wide(union utf8_char utf8){ + gunichar unichar = g_utf8_get_char((const char*) utf8.byte); + return wcwidth(unichar) 1; +} ... but I just committed a patch to make the utf-8 state machine assemble the unicode value from the utf-8, so you can now use get_unicode() instead of g_utf8_get_char(). Please follow the coding conventions and leave 'static int' on it's own line and put the opening '{' of the function on it's own line. enum utf8_state { utf8state_start, utf8state_accept, @@ -942,6 +951,7 @@ redraw_handler(struct widget *widget, void *data) struct glyph_run run; cairo_font_extents_t extents; double average_width; + double unichar_width; surface = window_get_surface(terminal-window); widget_get_allocation(terminal-widget, allocation); @@ -967,6 +977,7 @@ redraw_handler(struct widget *widget, void *data) allocation.y + top_margin); /* paint the background */ for (row = 0; row terminal-height; row++) { + p_row = terminal_get_row(terminal, row); for (col = 0; col terminal-width; col++) { /* get the attributes for this character cell */ terminal_decode_attr(terminal, row, col, attr); @@ -974,12 +985,17 @@ redraw_handler(struct widget *widget, void *data) if (attr.attr.bg == terminal-color_scheme-border) continue; + if(is_wide(p_row[col])) + unichar_width = 2 * average_width; + else + unichar_width = average_width; + terminal_set_color(terminal, cr, attr.attr.bg); cairo_move_to(cr, col * average_width, row * extents.height); - cairo_rel_line_to(cr, average_width, 0); + cairo_rel_line_to(cr, unichar_width, 0); cairo_rel_line_to(cr, 0, extents.height); - cairo_rel_line_to(cr, -average_width, 0); + cairo_rel_line_to(cr, -unichar_width, 0); cairo_close_path(cr); cairo_fill(cr); } @@ -1871,6 +1887,10 @@ handle_char(struct terminal *terminal, union utf8_char utf8) row[terminal-column] = utf8; attr_row[terminal-column++] = terminal-curr_attr; + /* cursor jump for wide character. */ + if (is_wide(utf8)) + row[terminal-column++].ch = 0; Inserting this 0 here confuses the selection logic which thinks 0 means end of line. I think we can use an invalid utf-8 character instead to indicate that this cell is a wide-char filler. We then also need to handle that in terminal_send_selection(), where we must ignore those filler cells when writing the selection to the fd. if (utf8.ch != terminal-last_char.ch) terminal-last_char = utf8; } @@ -2711,6 +2731,8 @@ int main(int argc, char *argv[]) struct terminal *terminal; int config_fd; + setlocale(LC_ALL, ); What is this for? This is to work around the horrible glibc API. wcwidth() depends on LC_CTYPE, that is, LC_ALL. I avoided forcing any locale and added my own wcwidth() implementation to kmscon:
Re: [RFC] weston: Sony clickpad support
Hi On Tue, Aug 13, 2013 at 6:50 AM, Alexander E. Patrakov patra...@gmail.com wrote: 2013/8/12 David Herrmann dh.herrm...@gmail.com: The implementation looks quite nice. I will not comment on the code individually, though. I'd really like to see a libtouchpad which implements all that logic. Once we start adding device drivers to weston, we will end up with a mess where we have to port it to each compositor we write. If gnome-shell becomes a compositor, it needs to implement this again. libxkbcommon already abstracted the keyboard handling. Why not do the same for touchpads? The wl_touch interface to weston is already standardized, so all this library needs to provide is a state-machine that converts evdev events into something similar to wl_touch. It's currently on my TODO list, so feel free to ignore it. But imho we should try to keep user-space input drivers separate to allow reuse and uniform configurations. If you set up a git repository for libtouchpad, I will help with the code there. OTOH I don't yet agree with the use of wl_touch as the output of such library. In the wl_touch documentation, touchscreens are mentioned, not touchpads. With touchpads, people often talk about moving the pointer, hovering, clicking and scrolling (even though scrolling is a gesture), and that fits better under wl_pointer. Whoops, sure, wl_mouse it is. I just confused wl_touch with touchpads, sorry. I did think about it a bit more and I guess pushing into weston is fine so far. But once we start duplicating it for gnome-shell/etc. we should start thinking about a separate library. I will happily create a repository, but that will have to wait until late September as I am swamped with gsoc work until then. Cheers David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [RFC] protocol: Extend wayland seat with interfaces for sensor inputs.
Hi On Thu, Aug 1, 2013 at 10:39 AM, Stefan Schmidt s.schm...@samsung.com wrote: Treating some specific sensors as input devices. In particular adding support for wl_compass, wl_gyroscope and wl_accelerometer here. We have requests to start and stop sensor event receiving as well as events to receive the different axis values for these sensors. Signed-off-by: Stefan Schmidt s.schm...@samsung.com --- protocol/wayland.xml | 123 ++ 1 file changed, 123 insertions(+) diff --git a/protocol/wayland.xml b/protocol/wayland.xml index 3cfa953..066a718 100644 --- a/protocol/wayland.xml +++ b/protocol/wayland.xml @@ -1251,6 +1251,9 @@ entry name=pointer value=1 summary=The seat has pointer devices/ entry name=keyboard value=2 summary=The seat has one or more keyboards/ entry name=touch value=4 summary=The seat has touch devices/ + entry name=compass value=8 summary=The seat has compass devices/ + entry name=gyroscope value=16 summary=The seat has gyroscope devices/ + entry name=accelerometer value=32 summary=The seat has accelerometer devices/ If we add all kinds of devices here, we will very soon run out of space for 32bit bitfields (which I think wayland uses for uint). I don't have a solution, but we should keep this in mind. /enum event name=capabilities @@ -1297,6 +1300,39 @@ !-- Version 2 of additions -- +request name=get_compass since=2 + description summary=return compass object +The ID provided will be initialized to the wl_compass interface + for this seat. + +This request only takes effect if the seat has the compass +capability. + /description + arg name=id type=new_id interface=wl_compass/ +/request + +request name=get_gyroscope since=2 + description summary=return gyroscope object +The ID provided will be initialized to the wl_gyroscope interface + for this seat. + +This request only takes effect if the seat has the gyroscope +capability. + /description + arg name=id type=new_id interface=wl_gyroscope/ +/request + +request name=get_accelerometer since=2 + description summary=return accelerometer object +The ID provided will be initialized to the wl_accelerometer interface + for this seat. + +This request only takes effect if the seat has the accelerometer +capability. + /description + arg name=id type=new_id interface=wl_accelerometer/ +/request + This interface limits each seat to one device of each. For keyboards or mouse we internally merge these into a single virtual device, but we cannot do this for accelerometers or gyros. We had a discussion about that recently, I am not sure about the conclusion, though. If there was one, please include it in the commit message. Besides, why are these devices bound to seats? Where are these devices supposed to be? An accelerometer can be attached to an input device, an HDD, a laptop, a tablet, ... If you look at this from a smartphone/tablet view, it is obvious. But for desktops it really isn't clear whose data an accelerometer device reports. I guess these devices are meant to be attached to the input device that this seat/user uses. In this case it makes sense to allow only a single device and bind them to seats. But please clarify this in the description. No one cares if the description is 1000 words long, so feel free to be verbose. event name=name since=2 description summary=unique identifier for this seat In a multiseat configuration this can be used by the client to help @@ -1605,6 +1641,93 @@ /event /interface + interface name=wl_compass version=2 +description summary=compass sensor device + The wl_compass interface represents a compass + associated with a seat. +/description + +request name=start + description summary=Start receiving events + Sent to enable event receiving for the compass sensor. + /description +/request + +request name=stop + description summary=Stop receiving events + Sent to disable event receiving for the compass sensor. + /description +/request So this is a shared device. Please note that down. A compositor has to broadcast events to all interested clients, otherwise this API doesn't work (or is inconsistent). Did you think of the implications of that? Might be fine, but I don't see any discussion, so please elaborate. What are the reasons to allow background applications to access compass/accelerometer/gyro? That is, why didn't you follow the enter/leave API? I don't want change your API, but the short descriptions give me the impression that you didn't think about these implications so I'd like to see some explanations. And the longer the descriptions, the less bikeshed you
Re: [RFC] weston: Sony clickpad support
Hi On Mon, Aug 5, 2013 at 12:34 PM, Alexander E. Patrakov patra...@gmail.com wrote: This patch series adds support to weston for a special type of touchpads found in some laptops. These touchpads contain one physical button that covers the whole surface of the touchpad. Unlike the well-known Apple touchpad, these touchpads have markings painted on them, that designate virtual button areas. So the user interaction is quite different from one gets from Apple touchpads. +---+ |\/\/\/\/\/\/\/\/\/\/\/\/\/\| |/\/\/\/\/\/\/\/\/\/\/\/\/\/| |\/\/\/\/\/\/\/\/\/\/\/\/\/\| |/\ Cursor-moving area /\/| |\/\/\/\/\/\/\/\/\/\/\/\/\/\| |/\/\/\/\/\/\/\/\/\/\/\/\/\/| |\/\/\/\/\/\/\/\/\/\/\/\/\/\| +---+ -- painted line | Left | Right | | virtual| virtual| | button | button | +-+-+ ^ | painted line E.g., currently, in order to get a right-click on such clickpad, one has to click anywhere with two fingers. This is, however, inappropriate for touchpads I am talking about. See: one needs to drag something, he places a finger on the upper part of the touchpad surface (on the part of surface designated for moving the cursor), then places another finger into the left half of the button area, clicks with the intention to move the first finger. However, what he currently gets is a right-click. Also, if one wants to make a right click, he places his finger into the right half of the button area, clicks, but currently gets a left click. In other words, the touchpad is unusable for users that are accustomed to the way the interaction with the touchpad is done in Windows. I have implemented the interaction model that looks more like what one gets in Windows. In order to do so, I had to refactor the existing code a bit and add support for multitouch protocol to evdev-touchpad.c. Tested all of this on my Sony VAIO VPC-Z23A4R laptop. Here is what works: * Moving the cursor by moving the finger in the cursor-moving area of the touchpad. * Not moving the cursor if one moves the finger in the virtual button area. * Tap-to-click. * Tap-and-drag gesture (unreliable). * Left and right virtual buttons. * Dragging, as explaining above. * Insensitivity to bad clicks (those outside the designated button area). * Two-finger scrolling. ...i.e. the touchpad is now quite usable. Zoom gestures are not implemented, though. I have also added autodetection of the interaction model based on the laptops with clickpads that GUADEC participants brought with themselves. Result: the current interaction model is valid only for Apple and Chromebook Pixel laptops, for everyone else the alternative model in this patch is needed because there are buttons painted on the clickpad. The implementation looks quite nice. I will not comment on the code individually, though. I'd really like to see a libtouchpad which implements all that logic. Once we start adding device drivers to weston, we will end up with a mess where we have to port it to each compositor we write. If gnome-shell becomes a compositor, it needs to implement this again. libxkbcommon already abstracted the keyboard handling. Why not do the same for touchpads? The wl_touch interface to weston is already standardized, so all this library needs to provide is a state-machine that converts evdev events into something similar to wl_touch. It's currently on my TODO list, so feel free to ignore it. But imho we should try to keep user-space input drivers separate to allow reuse and uniform configurations. Cheers David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v2 1/3] Handle CJK wide glyph rendering
Hi On Tue, Jul 23, 2013 at 4:44 AM, Peng Wu peng.e...@gmail.com wrote: Yes, I just followed the gnome vte terminal widget code. I will try to send v3 patch set to remove glib dependencies soon. Please first address the issues I mentioned in v1. You cannot just redraw characters and be fine. That's not how it works. You need to adjust handle_char() to move the cursor two columns for wide-characters. Imagine MODE_AUTOWRAP is set and you draw 10 wide-characters on a 10 cell-line. We expect a vt320-compat terminal to draw 5 chars on the first line and the other 5 chars on the next line. Assume MODE_AUTOWRAP not set and you draw 10 wide-characters on a 10 cell-line. We expect the first 4 chars on the line and the last char to be the last character written. Really, multi-width characters isn't about drawing. It's about correctly interpreting the incoming characters in your vt100-like state-machine. Why do you want to support that in terminal.c anyway? It's an example, it misses support for a lot of things and I recommend avoiding adding complex features. Anyhow, here is an example how to move the cursor for multi-width characters: https://github.com/dvdhrm/kmscon/blob/master/src/tsm_screen.c#L1000 Regards David On Sun, 2013-07-21 at 23:22 -0500, Thomas Daede wrote: This is for a VT-100 style terminal emulator, right? It's designed to implement a grid of characters. Therefore, drawing it like a grid based on single or double character widths makes a lot more sense than detecting the length of a string. On any other UI element, detecting the string length would be more appropriate. Ideally I would imagine you would always pick a fixed width font (aka only single or double widths) but the terminal should work okay with a proportional font, forcing it into fixed positions. I think Peng's implementation is in line with other virtual terminals. On Sun, Jul 21, 2013 at 9:46 PM, Peng Wu peng.e...@gmail.com wrote: Frankly speaking, I suspect that your new fix will break some program which uses newt with proportional font. PS: newt is a library for text mode user interfaces. On Thu, 2013-07-18 at 11:23 -0700, Bill Spitzak wrote: I propose the drawing code be changed to something like this: char* string = the_line_to_draw; cairo_draw_text_at(x, y, string); And this: int where_is_position_n(int n, char* string) { return cairo_text_width(string, n); } ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v2 1/3] Handle CJK wide glyph rendering
Hi On Tue, Jul 23, 2013 at 9:49 AM, Peng Wu peng.e...@gmail.com wrote: Hi, On Tue, 2013-07-23 at 09:22 +0200, David Herrmann wrote: Please first address the issues I mentioned in v1. You cannot just redraw characters and be fine. That's not how it works. You need to adjust handle_char() to move the cursor two columns for wide-characters. Actually I also adjusted the cursor positioning and drawing in v2 patch set. [PATCH v2 2/3] Fixes cursor rendering for wide glyphs Show the cursor at the right position, and use wide cursor under wide glyph. That's not what I am talking about. This is about _drawing_ the cursor at the right position. I am talking about positioning the cursor _internally_. That is, the x/y cursor position. Your patch just moves the visual cursor according to your wide-characters. See the two examples below about internal cursor positions. Imagine MODE_AUTOWRAP is set and you draw 10 wide-characters on a 10 cell-line. We expect a vt320-compat terminal to draw 5 chars on the first line and the other 5 chars on the next line. Assume MODE_AUTOWRAP not set and you draw 10 wide-characters on a 10 cell-line. We expect the first 4 chars on the line and the last char to be the last character written. I plan to skip to support MODE_AUTOWRAP feature. Ugh? I gave two examples, one with and one without MODE_AUTOWRAP. So it doesn't matter whether you support it or not, in both cases it will fail. If you don't implement it properly, you will soon notice that stuff like vim will fail. They rely on these features. Really, multi-width characters isn't about drawing. It's about correctly interpreting the incoming characters in your vt100-like state-machine. Why do you want to support that in terminal.c anyway? It's an example, it misses support for a lot of things and I recommend avoiding adding complex features. Because we want weston terminal has i18n support, such as CJK glyph display. I recommend keeping it simple. But it's not me to judge. If you refuse to implement it properly, I have no reason to review that. You need to convince krh to apply it. Anyhow, here is an example how to move the cursor for multi-width characters: https://github.com/dvdhrm/kmscon/blob/master/src/tsm_screen.c#L1000 Cheers David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 2/2] Some CJK glyphs are wide, which occupy two columns. If the glyph is wide, then use two columns instead of one.
Hi On Thu, Jun 6, 2013 at 9:32 AM, Peng Wu peng.e...@gmail.com wrote: --- Please cut the headline and instead provide a proper commit message. clients/Makefile.am | 2 +- clients/terminal.c | 18 -- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/clients/Makefile.am b/clients/Makefile.am index cad0d40..d37d66a 100644 --- a/clients/Makefile.am +++ b/clients/Makefile.am @@ -104,7 +104,7 @@ weston_screenshooter_SOURCES = \ weston_screenshooter_LDADD = libtoytoolkit.la weston_terminal_SOURCES = terminal.c -weston_terminal_LDADD = libtoytoolkit.la -lutil +weston_terminal_LDADD = libtoytoolkit.la -lutil $(PANGO_LIBS) image_SOURCES = image.c image_LDADD = libtoytoolkit.la diff --git a/clients/terminal.c b/clients/terminal.c index 0d4f726..4495530 100644 --- a/clients/terminal.c +++ b/clients/terminal.c @@ -33,6 +33,7 @@ #include ctype.h #include cairo.h #include sys/epoll.h +#include glib.h #include wayland-client.h @@ -942,6 +943,9 @@ redraw_handler(struct widget *widget, void *data) struct glyph_run run; cairo_font_extents_t extents; double average_width; + gunichar unichar; + gboolean iswide; + int extracol; surface = window_get_surface(terminal-window); widget_get_allocation(terminal-widget, allocation); @@ -991,22 +995,32 @@ redraw_handler(struct widget *widget, void *data) glyph_run_init(run, terminal, cr); for (row = 0; row terminal-height; row++) { p_row = terminal_get_row(terminal, row); + extracol = 0; for (col = 0; col terminal-width; col++) { /* get the attributes for this character cell */ terminal_decode_attr(terminal, row, col, attr); glyph_run_flush(run, attr); - text_x = col * average_width; + /* check dual width unicode character */ + unichar = g_utf8_get_char((const char*) p_row[col].byte); + iswide = g_unichar_iswide(unichar); + + text_x = (col + extracol) * average_width; text_y = extents.ascent + row * extents.height; if (attr.attr.a ATTRMASK_UNDERLINE) { terminal_set_color(terminal, cr, attr.attr.fg); cairo_move_to(cr, text_x, (double)text_y + 1.5); - cairo_line_to(cr, text_x + average_width, (double) text_y + 1.5); + if (iswide) + cairo_line_to(cr, text_x + average_width * 2, (double) text_y + 1.5); + else + cairo_line_to(cr, text_x + average_width, (double) text_y + 1.5); cairo_stroke(cr); } glyph_run_add(run, text_x, text_y, p_row[col]); + if (iswide) + extracol++; This patch is actually wrong. multi-width character support requires more than just drawing the glyphs with multiple columns. In fact, if a client sends a multi-width character, the terminal emulation is supposed to jump two columns so the next character will be inserted at the correct position (this includes automatic newline, tabs, etc..). What you currently do is just dropping the glyph which is inserted after the double-col glyph. Are you sure this works as expected? And for a wcwidth() implementation, see here: https://github.com/dvdhrm/kmscon/blob/master/external/wcwidth.c Just copy it verbatim and use wcwidth() instead of glib. I recommend just leaving multi-width support out of terminal.c. It adds complexity (including wcwidth()) and seems misplaced in a debug helper like weston-terminal. But feel free to resend and I will try to review it. Cheers David } } -- 1.8.1.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 05/18] fbdev: initialize yoffset of varinfo
Hi On Fri, Jun 21, 2013 at 10:49 AM, mchalain [marc.chal...@gmail.com] marc.chal...@gmail.com wrote: From: mchalain marc.chal...@gmail.com it initializes varinfo.yoffset. varinfo.yoffset has to point on the beginning of the video memory. The card uses this value to push on the screen a part of the video memory when this one is larger than the screen. --- weston/src/compositor-fbdev.c |6 ++ 1 file changed, 6 insertions(+) diff --git a/weston/src/compositor-fbdev.c b/weston/src/compositor-fbdev.c index adfb67a..d2aee9b 100644 --- a/weston/src/compositor-fbdev.c +++ b/weston/src/compositor-fbdev.c @@ -368,6 +368,11 @@ fbdev_query_screen_info(struct fbdev_output *output, int fd, return -1; } + if (varinfo.yoffset != 0) { + varinfo.yoffset = 0; + if (ioctl(fd, FBIOPAN_DISPLAY, varinfo) 0) + return -1; + } Why do you need this? It's unnecessary. We call FBIOPUT_VSCREENINFO after fbdev_query_screen_info(), anyway. Furthermore, not all drivers support panning even though the yoffset may be non-zero (you need FBIOPUT_VSCREENINFO then). I think you can just drop this here but keep the yoffset=0 below. return 1; } @@ -404,6 +409,7 @@ fbdev_set_screen_info(struct fbdev_output *output, int fd, varinfo.blue.length = 8; varinfo.blue.msb_right = 0; + varinfo.yoffset = 0; Yep, this is definitely needed. Regards David /* Set the device's screen information. */ if (ioctl(fd, FBIOPUT_VSCREENINFO, varinfo) 0) { return -1; -- 1.7.9.5 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 1/2] Handle SYN_DROPPED, filter keys with a bitmap
Hi On Tue, Jun 4, 2013 at 8:47 PM, Martin Minarik minari...@student.fiit.stuba.sk wrote: Since evdev keys are unreliable, they might randomly get dropped, such as, on SYN_DROPPED. Even SYN_DROPPED is sometimes not delivered. Ugh, if events are dropped but SYN_DROPPED is missing, it's a kernel bug. Please report it to linux-in...@vger.kernel.org if you see it. We shouldn't assume this broken behavior in user-space! There is also a fix pending on linux-input@vger which fixes duplicate press/release events on EVIOCGKEY. So lets fix such bugs upstream instead of introducing hacks. Nevertheless, SYN_DROPPED handling is needed, indeed. Clients, compositor are not able to recover from duplicate press/release. This fixes this bug, thereby making the compositor and clients useable even under critical system conditions such as, but not limited to, high system load, device malfunction. This improves reliability. Every device contains a key bitmap. Each passing key press/release is checked and recorded. Duplicated presses, releases are handled by emitting the opposite type event in between (the time is faked -1 time unit). This is a last resort solution and happens only very rarely. When a SYN_DROPPED is detected: 1 wait until after the next SYN_REPORT. Why wait for SYN_REPORT? Re-reading system state should be a silent operation. But see below 2 Ask the kernel for the actual state of buttons 3 Compare the buttons with the internal key bitmap, to see what changed. 4 Send each different key via notify to the compositor, with timestamp. 5 Update the internal key bitmap to match the kernel's Disadvantages of this approach Each evdev device consumes 96 bytes more memory. Table lookup+bit toggle for almost key. Don't know the timestamp of missing keys. The key_notify from kernel bitmap compare occurs later the real missing event. That's a timestamp issue. --- src/evdev-touchpad.c | 51 ++- src/evdev.c | 176 ++ src/evdev.h | 35 +++--- 3 files changed, 208 insertions(+), 54 deletions(-) diff --git a/src/evdev-touchpad.c b/src/evdev-touchpad.c index a21ae0b..890ddad 100644 --- a/src/evdev-touchpad.c +++ b/src/evdev-touchpad.c @@ -533,6 +533,26 @@ on_release(struct touchpad_dispatch *touchpad) push_fsm_event(touchpad, FSM_EVENT_RELEASE); } +struct evdev_dispatch_interface touchpad_interface; +struct evdev_dispatch_interface touchpad_syn_drop_interface; + +static inline void +process_syn(struct touchpad_dispatch *touchpad, +struct evdev_device *device, +struct input_event *e, uint32_t time) +{ + switch (e-code) { + case SYN_DROPPED: + if (device-dispatch-interface == touchpad_interface) + device-dispatch-interface = touchpad_syn_drop_interface; + break; + case SYN_REPORT: + default: + touchpad-event_mask |= TOUCHPAD_EVENT_REPORT; + break; + } +} + static inline void process_absolute(struct touchpad_dispatch *touchpad, struct evdev_device *device, @@ -588,7 +608,7 @@ process_key(struct touchpad_dispatch *touchpad, case BTN_FORWARD: case BTN_BACK: case BTN_TASK: - notify_button(device-seat, + notify_button(device-seat, time, e-code, e-value ? WL_POINTER_BUTTON_STATE_PRESSED : WL_POINTER_BUTTON_STATE_RELEASED); @@ -634,8 +654,7 @@ touchpad_process(struct evdev_dispatch *dispatch, switch (e-type) { case EV_SYN: - if (e-code == SYN_REPORT) - touchpad-event_mask |= TOUCHPAD_EVENT_REPORT; + process_syn(touchpad, device, e, time); break; case EV_ABS: process_absolute(touchpad, device, e); @@ -664,6 +683,32 @@ struct evdev_dispatch_interface touchpad_interface = { touchpad_destroy }; +static void +touchpad_syn_drop_process(struct evdev_dispatch *dispatch, +struct evdev_device *device, +struct input_event *event, +uint32_t time) +{ + if ((event-code != EV_SYN) || (event-code != SYN_REPORT)) + return; + + if (device-dispatch-interface == touchpad_syn_drop_interface) + device-dispatch-interface = touchpad_interface; + + evdev_keys_state_sync(device, time); +} + +static void +touchpad_syn_drop_destroy(struct evdev_dispatch *dispatch) +{ + free(dispatch); +} + +struct evdev_dispatch_interface touchpad_syn_drop_interface = { + touchpad_syn_drop_process, + touchpad_syn_drop_destroy +}; + static int touchpad_init(struct touchpad_dispatch *touchpad, struct evdev_device
Re: [PATCH 05/18] fbdev: initialize yoffset of varinfo
Hi On Fri, Jun 21, 2013 at 11:43 AM, Marc Chalain marc.chal...@gmail.com wrote: 2013/6/21 David Herrmann dh.herrm...@gmail.com Hi On Fri, Jun 21, 2013 at 10:49 AM, mchalain [marc.chal...@gmail.com] marc.chal...@gmail.com wrote: From: mchalain marc.chal...@gmail.com it initializes varinfo.yoffset. varinfo.yoffset has to point on the beginning of the video memory. The card uses this value to push on the screen a part of the video memory when this one is larger than the screen. --- weston/src/compositor-fbdev.c |6 ++ 1 file changed, 6 insertions(+) diff --git a/weston/src/compositor-fbdev.c b/weston/src/compositor-fbdev.c index adfb67a..d2aee9b 100644 --- a/weston/src/compositor-fbdev.c +++ b/weston/src/compositor-fbdev.c @@ -368,6 +368,11 @@ fbdev_query_screen_info(struct fbdev_output *output, int fd, return -1; } + if (varinfo.yoffset != 0) { + varinfo.yoffset = 0; + if (ioctl(fd, FBIOPAN_DISPLAY, varinfo) 0) + return -1; + } Why do you need this? It's unnecessary. We call FBIOPUT_VSCREENINFO after fbdev_query_screen_info(), anyway. Furthermore, not all drivers support panning even though the yoffset may be non-zero (you need FBIOPUT_VSCREENINFO then). I think you can just drop this here but keep the yoffset=0 below. where do you use FBIOPUT_VSCREENINFO ? I only find inside fbdev_set_screen_info but this function is called only when the output is reenable not at the startup. My bad! But still, you now call FBIOPAN_DISPLAY on _every_ fbdev_query_screen_info() while it is only needed during initial setup as VT_ENTER calls fbdev_set_screen_info(), anyway. So instead, I'd recommend to just call fbdev_set_screen_info() during initial setup as well. It seems strange that FBIOPAN_DISPLAY is not supported by some devices. I used this one to not set all the varinfo on the device and to be faster. See ./drivers/video/fbmem.c fb_pan_display(). Trivial fbdev drivers might not support it, but user-space can still set the offset via FBIOPUT_VSCREENINFO. If you want, you can call FBIOPAN_DISPLAY and if it fails, you use FBIOPUT_VSCREENINFO. If yoffset may be non-zero this backend shouldn't work this kind of devices and we have to change the painting functions. yoffset=0; is the correct thing to do. Cheers David return 1; } @@ -404,6 +409,7 @@ fbdev_set_screen_info(struct fbdev_output *output, int fd, varinfo.blue.length = 8; varinfo.blue.msb_right = 0; + varinfo.yoffset = 0; Yep, this is definitely needed. Regards David /* Set the device's screen information. */ if (ioctl(fd, FBIOPUT_VSCREENINFO, varinfo) 0) { return -1; -- 1.7.9.5 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 05/18] fbdev: initialize yoffset of varinfo
Hi On Fri, Jun 21, 2013 at 12:05 PM, Marc Chalain marc.chal...@gmail.com wrote: 2013/6/21 David Herrmann dh.herrm...@gmail.com Hi On Fri, Jun 21, 2013 at 11:43 AM, Marc Chalain marc.chal...@gmail.com wrote: 2013/6/21 David Herrmann dh.herrm...@gmail.com Hi On Fri, Jun 21, 2013 at 10:49 AM, mchalain [marc.chal...@gmail.com] marc.chal...@gmail.com wrote: From: mchalain marc.chal...@gmail.com it initializes varinfo.yoffset. varinfo.yoffset has to point on the beginning of the video memory. The card uses this value to push on the screen a part of the video memory when this one is larger than the screen. --- weston/src/compositor-fbdev.c |6 ++ 1 file changed, 6 insertions(+) diff --git a/weston/src/compositor-fbdev.c b/weston/src/compositor-fbdev.c index adfb67a..d2aee9b 100644 --- a/weston/src/compositor-fbdev.c +++ b/weston/src/compositor-fbdev.c @@ -368,6 +368,11 @@ fbdev_query_screen_info(struct fbdev_output *output, int fd, return -1; } + if (varinfo.yoffset != 0) { + varinfo.yoffset = 0; + if (ioctl(fd, FBIOPAN_DISPLAY, varinfo) 0) + return -1; + } Why do you need this? It's unnecessary. We call FBIOPUT_VSCREENINFO after fbdev_query_screen_info(), anyway. Furthermore, not all drivers support panning even though the yoffset may be non-zero (you need FBIOPUT_VSCREENINFO then). I think you can just drop this here but keep the yoffset=0 below. where do you use FBIOPUT_VSCREENINFO ? I only find inside fbdev_set_screen_info but this function is called only when the output is reenable not at the startup. My bad! But still, you now call FBIOPAN_DISPLAY on _every_ fbdev_query_screen_info() while it is only needed during initial setup as VT_ENTER calls fbdev_set_screen_info(), anyway. So instead, I'd recommend to just call fbdev_set_screen_info() during initial setup as well. No I call FBIOPAN_DISPLAY only at startup after yoffset is set to 0 You call it everytime yoffset isn't 0. So if you VT_LEAVE, another application changes the yoffset for whatever reason and we VT_ENTER, you will end up calling FBIOPAN_DISPLAY again followed by fbdev_set_screen_info() if the screen-info changed. Don't expect applications on other VTs to behave as expected. They may do anything! They may even use some acceleration which we in weston aren't aware of. It seems strange that FBIOPAN_DISPLAY is not supported by some devices. I used this one to not set all the varinfo on the device and to be faster. See ./drivers/video/fbmem.c fb_pan_display(). Trivial fbdev drivers might not support it, but user-space can still set the offset via FBIOPUT_VSCREENINFO. If you want, you can call FBIOPAN_DISPLAY and if it fails, you use FBIOPUT_VSCREENINFO. I read the yoffset hasn't to be null in FB_VMODE_YWRAP . But yoffset is only used for panning. It's useless to set it if it doesn't use (fbmem.c uses yoffset only for panning in your example too) A driver may not support FBIOPAN_DISPLAY but still support panning via FBIOPUT_VSCREENINFO. If any framebuffer parameter change requires a full modeset, there is no reason to support FBIOPAN_DISPLAY. But panning may still be supported via yoffset and FBIOPUT_VSCREENINFO. Cheers David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 05/18] fbdev: initialize yoffset of varinfo
Hi Marc On Fri, Jun 21, 2013 at 2:17 PM, Marc Chalain marc.chal...@gmail.com wrote: 2013/6/21 David Herrmann dh.herrm...@gmail.com Hi On Fri, Jun 21, 2013 at 12:05 PM, Marc Chalain marc.chal...@gmail.com wrote: 2013/6/21 David Herrmann dh.herrm...@gmail.com Hi On Fri, Jun 21, 2013 at 11:43 AM, Marc Chalain marc.chal...@gmail.com wrote: 2013/6/21 David Herrmann dh.herrm...@gmail.com Hi On Fri, Jun 21, 2013 at 10:49 AM, mchalain [marc.chal...@gmail.com] marc.chal...@gmail.com wrote: From: mchalain marc.chal...@gmail.com it initializes varinfo.yoffset. varinfo.yoffset has to point on the beginning of the video memory. The card uses this value to push on the screen a part of the video memory when this one is larger than the screen. --- weston/src/compositor-fbdev.c |6 ++ 1 file changed, 6 insertions(+) diff --git a/weston/src/compositor-fbdev.c b/weston/src/compositor-fbdev.c index adfb67a..d2aee9b 100644 --- a/weston/src/compositor-fbdev.c +++ b/weston/src/compositor-fbdev.c @@ -368,6 +368,11 @@ fbdev_query_screen_info(struct fbdev_output *output, int fd, return -1; } + if (varinfo.yoffset != 0) { + varinfo.yoffset = 0; + if (ioctl(fd, FBIOPAN_DISPLAY, varinfo) 0) + return -1; + } Why do you need this? It's unnecessary. We call FBIOPUT_VSCREENINFO after fbdev_query_screen_info(), anyway. Furthermore, not all drivers support panning even though the yoffset may be non-zero (you need FBIOPUT_VSCREENINFO then). I think you can just drop this here but keep the yoffset=0 below. where do you use FBIOPUT_VSCREENINFO ? I only find inside fbdev_set_screen_info but this function is called only when the output is reenable not at the startup. My bad! But still, you now call FBIOPAN_DISPLAY on _every_ fbdev_query_screen_info() while it is only needed during initial setup as VT_ENTER calls fbdev_set_screen_info(), anyway. So instead, I'd recommend to just call fbdev_set_screen_info() during initial setup as well. No I call FBIOPAN_DISPLAY only at startup after yoffset is set to 0 You call it everytime yoffset isn't 0. So if you VT_LEAVE, another application changes the yoffset for whatever reason and we VT_ENTER, you will end up calling FBIOPAN_DISPLAY again followed by fbdev_set_screen_info() if the screen-info changed. Don't expect applications on other VTs to behave as expected. They may do anything! They may even use some acceleration which we in weston aren't aware of. If another application set yoffset, with VT_ENTER we pass inside fbdev_set_screen_info and yoffset is set to 0. After fbdev_set_screen_info is buggy and this patch is not a bugfix... the bug is you set varinfo.bits_per_pixel with a value that could be different of 32 and after you set the colors as A8R8G8B8 ... I don't want to use it, because I can't check all cases of this function, my card is too simple... Sorry, I cannot follow. It's the following scenario that I think can be handled better: WESTON-START = fbdev initialization = FBIOPAN_DISPLAY VT_LEAVE (some other app changes the fbdev state and offsets) VT_ENTER = fbdev_output_reenable() = fbdev_frame_buffer_open() = fbdev_query_screen_info() = FBIOPAN_DISPLAY = fbdev_set_screen_info() = FBIOPUT_VSCREENINFO So after a VT_ENTER, we call FBIOPAN_DISPLAY even though we reset the screeninfo later (because it changed). So why not modify compare_screen_info to return 1 if the offsets are not 0? This avoids any additional call to FBIOPAN_DISPLAY. As you noticed before, this would require fbdev_output_create to call fbdev_set_screen_info() as we do not reset the offsets during fbdev output setup. It seems strange that FBIOPAN_DISPLAY is not supported by some devices. I used this one to not set all the varinfo on the device and to be faster. See ./drivers/video/fbmem.c fb_pan_display(). Trivial fbdev drivers might not support it, but user-space can still set the offset via FBIOPUT_VSCREENINFO. If you want, you can call FBIOPAN_DISPLAY and if it fails, you use FBIOPUT_VSCREENINFO. I read the yoffset hasn't to be null in FB_VMODE_YWRAP . But yoffset is only used for panning. It's useless to set it if it doesn't use (fbmem.c uses yoffset only for panning in your example too) A driver may not support FBIOPAN_DISPLAY but still support panning via FBIOPUT_VSCREENINFO. If any framebuffer parameter change requires a full modeset, there is no reason to support FBIOPAN_DISPLAY. But panning may still be supported via yoffset and FBIOPUT_VSCREENINFO. You tell that kernel bug must be corrected by kernel developers in the thread about SYN_DROPPED and now you tell me that we have to check that the driver should not be completed
Re: [RFC] libinputmapper: Input device configuration for graphic-servers
Hi Peter On Mon, May 27, 2013 at 12:35 PM, Peter Hutterer peter.hutte...@who-t.net wrote: On Fri, May 24, 2013 at 05:07:14PM +0200, David Herrmann wrote: [...] This library is intended to solve the classification/detection problem. While the kernel evdev-interface provides us a bunch of information for each device, it doesn't provide any classification of the device (mostly because it would be unreliable). So libinputmapper as I see it is supposed to replace the heuristics that we currently have (KEY_A-KEY_Z = keyboard, REL_X/Y = mouse, ...) with a more user-controlled interface. So a *compositor* defines the type of devices, that it wants to support. As an example, it needs mouse+touchpad devices as mouse-input, keyboards as keyboard input. It then listens for all input hotplug events via udev, calls libinputmapper to provide tags for the devices and then checks, whether it's one of the devices that it wants. Client-side support is not involved, yet. ok, that makes sense to me and is a good idea. it's a combined version of (formerly) udev's input_id tool and the Match* system that X has, in the form of a library with simple adjustment based on textfiles. good. and sorry, I did somewhat tie all this in with the client side as well, which is where the confusion came from. I think the tagging library will be quite private to the compositor implementation (even if that means all compositors). in the X case, we have evdev and the wacom driver for tablet devices since they're different enough to basic tablets. I suspect something like that may be required for wayland sooner or later. input_id+Match* seems to be a good description. That's what I had in mind. But I don't see a reason to keep it private to the compositor. The compositor is the #1 user of the library, but whatever API is used to forward events to a client, we shouldn't hide this source. We can save all other kinds of useful information in these files. A client could retrieve device-geometry information or other static data. I designed the configuration to allow a global database with local additions. So a compositor can have a different configuration than a client. But both have the same global base/fallback configuration. ok. the reason I assumed private is because you need to decide where the level of abstraction sits and I think we don't think of it the same way there. you can tag a device as tablet, but that doesn't tell you which driver to use in the compositor to parse the data (see wacom example). so now the compositor has another lookup table so it knows which module to load for this device. this lookup table can either be duplicated across compositors, or be moved into a library. and now you have your libinputmodulemapper on top (well, on the side) of libinputmapper. I don't understand. What's wrong with linking device-TAGs to drivers in the compositor? So the compositor loads all drivers dynamically and each driver specifies which tags it supports. A new device is then assigned to the driver that currently is associated with the tag. If you want to overwrite that, add a [driver] section to the device-config/rule and use bind = some-driver-name. The compositor then needs to support the [driver] section and can see whether it should force-bind another driver than the default. Or even better, if you have multiple tags for tablets, like TAG_TABLET and TAG_WACOM_TABLET, you can define both on wacom tablets but only TAG_TABLET on other random tablets. Then bind the normal tablet/mouse/whatever driver to TAG_TABLET and your wacom driver to TAG_WACOM_TABLET. Of course, TAG_WACOM_TABLET then needs a higher priority than TAG_TABLET. This will work whether the wacom-driver is loaded or not. likewise, as in another part of this thread: the compositor may not care about the distinction between joystick/gamepad but the client may. so again we have two different use-cases and mapping requirements here. Then the compositor just binds it's gaming-device-driver to (TAG_JOYSTICK | TAG_GAMEPAD). A client may use TAG_JOYSTICK or TAG_GAMEPAD separately. I don't see the problem here? so the tagging library would have to tag a device not just as graphics tablet but more as input module selection library. which again, means it's a library version of input_id and Match*, etc. what you will also need then is for drivers to provide hooks to notify what they can deal with. i.e. libtouchpadcommon needs to install data files for libinputmapper to notify that it's now taking over touchpads. I wanted to keep this private to the compositor. So the compositor has a list of drivers and a libinputmapper context. It then retrieves the tags for a device and assigns the device depending on the tags to the right driver. So libinputmapper always returns the same information. It's up to the compositor to use this according to it's capabilities. Or what would
Re: [RFC] libinputmapper: Input device configuration for graphic-servers
Hi On Thu, May 23, 2013 at 7:32 AM, Peter Hutterer peter.hutte...@who-t.net wrote: On Tue, May 21, 2013 at 04:30:03PM +0200, David Herrmann wrote: Hi Peter On Tue, May 21, 2013 at 6:37 AM, Peter Hutterer peter.hutte...@who-t.net wrote: On Thu, May 16, 2013 at 03:16:11PM +0200, David Herrmann wrote: Hi Peter On Thu, May 16, 2013 at 7:37 AM, Peter Hutterer peter.hutte...@who-t.net wrote: On Sun, May 12, 2013 at 04:20:59PM +0200, David Herrmann wrote: [..] So what is the proposed solution? My recommendation is, that compositors still search for devices via udev and use device drivers like libxkbcommon. So linux evdev handling is still controlled by the compositor. However, I'd like to see something like my libinputmapper proposal being used for device detection and classification. libinputmapper provides an inmap_evdev object which reads device information from an evdev-fd or sysfs /sys/class/input/inputnum path, performs some heuristics to classify it and searches it's global database for known fixups for broken devices. It then provides capabilities to the caller, which allow them to see what drivers to load on the device. And it provides a very simple mapping table that allows to apply fixup mappings for broken devices. These mappings are simple 1-to-1 mappings that are supposed to be applied before drivers handle the input. This is to avoid device-specific fixup in the drivers and move all this to the inputmapper. An example would be a remapping for gamepads that report BTN_A instead of BTN_NORTH, but we cannot fix them in the kernel for backwards-compatibility reasons. The gamepad-driver can then assume that if it receives BTN_NORTH, it is guaranteed to be BTN_NORTH and doesn't need to special case xbox360/etc. controllers, because they're broken. I think evdev is exactly that interface and apparently it doesn't work. if you want a mapping table, you need a per-client table because sooner or later you have a client that needs BTN_FOO when the kernel gives you BTN_BAR and you can't change the client to fix it. i.e. the same issue evdev has now, having a global remapping table just moves the problem down by 2 years. a mapping table is good, but you probably want two stages of mapping: one that's used in the compositor for truly broken devices that for some reason can't be fixed in the kernel, and one that's used on a per-client basis. and you'll likely want to be able to overide the client-specific from outside the client too. IMHO, the problem with evdev is, that it doesn't provide device classes. The only class we have is this is an input device. All other event-flags can be combined in whatever way we want. So like 10 years ago when the first gamepad driver was introduced, we added some mapping that was unique to this device (the device was probably unique, too). Some time later, we added some other gamepad-like driver with a different mapping (as it was probably a very different device-type, back then, and we didn't see it coming that this will become a wide-spread device-type). However, today we notice that a GamePad is an established type of device (like a touchpad), but we have tons of different mappings in the kernel for backwards-compatibility reasons. I can see that this kind of development can happen again (and very likely it _will_ happen again) and it will happen for all kinds of devices. But that's why I designed the proposal from a compositor's view instead of from a kernel's view. A touchpad driver of the compositor needs to know exactly what kind of events it gets from the kernel. If it gets wrong events, it will misbehave. As we cannot guarantee that all kernel drivers behave the same way, the compositor's touchpad driver needs to work around all these little details on a per-device basis. To avoid this, I tried to abstract the touchpad-protocol and moved per-device handling into a separate library. It detects all devices that can serve as a touchpad and fixes trivial (1-to-1 mapping) incompatibilities. This removes all per-device handling from the touchpad driver and it can expect all input it gets to be conform with a touchpad protocol. And in fact, it removes this from all the compositor's input drivers. So I think of it more like a lib-detect-and-make-compat. All devices that do not fall into one of the categories (I called it capability), will be handled as custom devices. So if we want an input driver for a new fancy device, then we need a custom driver, anyway (or adjust a generic driver to handle both). If at some point it turns out, that this kind of device becomes more established, we can add a new capability for it. Or we try extending an existing capability in a backwards-compatible way. We can then remove the custom-device handling from
Re: [RFC] libinputmapper: Input device configuration for graphic-servers
Hi Peter On Tue, May 21, 2013 at 6:37 AM, Peter Hutterer peter.hutte...@who-t.net wrote: On Thu, May 16, 2013 at 03:16:11PM +0200, David Herrmann wrote: Hi Peter On Thu, May 16, 2013 at 7:37 AM, Peter Hutterer peter.hutte...@who-t.net wrote: On Sun, May 12, 2013 at 04:20:59PM +0200, David Herrmann wrote: [..] So what is the proposed solution? My recommendation is, that compositors still search for devices via udev and use device drivers like libxkbcommon. So linux evdev handling is still controlled by the compositor. However, I'd like to see something like my libinputmapper proposal being used for device detection and classification. libinputmapper provides an inmap_evdev object which reads device information from an evdev-fd or sysfs /sys/class/input/inputnum path, performs some heuristics to classify it and searches it's global database for known fixups for broken devices. It then provides capabilities to the caller, which allow them to see what drivers to load on the device. And it provides a very simple mapping table that allows to apply fixup mappings for broken devices. These mappings are simple 1-to-1 mappings that are supposed to be applied before drivers handle the input. This is to avoid device-specific fixup in the drivers and move all this to the inputmapper. An example would be a remapping for gamepads that report BTN_A instead of BTN_NORTH, but we cannot fix them in the kernel for backwards-compatibility reasons. The gamepad-driver can then assume that if it receives BTN_NORTH, it is guaranteed to be BTN_NORTH and doesn't need to special case xbox360/etc. controllers, because they're broken. I think evdev is exactly that interface and apparently it doesn't work. if you want a mapping table, you need a per-client table because sooner or later you have a client that needs BTN_FOO when the kernel gives you BTN_BAR and you can't change the client to fix it. i.e. the same issue evdev has now, having a global remapping table just moves the problem down by 2 years. a mapping table is good, but you probably want two stages of mapping: one that's used in the compositor for truly broken devices that for some reason can't be fixed in the kernel, and one that's used on a per-client basis. and you'll likely want to be able to overide the client-specific from outside the client too. IMHO, the problem with evdev is, that it doesn't provide device classes. The only class we have is this is an input device. All other event-flags can be combined in whatever way we want. So like 10 years ago when the first gamepad driver was introduced, we added some mapping that was unique to this device (the device was probably unique, too). Some time later, we added some other gamepad-like driver with a different mapping (as it was probably a very different device-type, back then, and we didn't see it coming that this will become a wide-spread device-type). However, today we notice that a GamePad is an established type of device (like a touchpad), but we have tons of different mappings in the kernel for backwards-compatibility reasons. I can see that this kind of development can happen again (and very likely it _will_ happen again) and it will happen for all kinds of devices. But that's why I designed the proposal from a compositor's view instead of from a kernel's view. A touchpad driver of the compositor needs to know exactly what kind of events it gets from the kernel. If it gets wrong events, it will misbehave. As we cannot guarantee that all kernel drivers behave the same way, the compositor's touchpad driver needs to work around all these little details on a per-device basis. To avoid this, I tried to abstract the touchpad-protocol and moved per-device handling into a separate library. It detects all devices that can serve as a touchpad and fixes trivial (1-to-1 mapping) incompatibilities. This removes all per-device handling from the touchpad driver and it can expect all input it gets to be conform with a touchpad protocol. And in fact, it removes this from all the compositor's input drivers. So I think of it more like a lib-detect-and-make-compat. All devices that do not fall into one of the categories (I called it capability), will be handled as custom devices. So if we want an input driver for a new fancy device, then we need a custom driver, anyway (or adjust a generic driver to handle both). If at some point it turns out, that this kind of device becomes more established, we can add a new capability for it. Or we try extending an existing capability in a backwards-compatible way. We can then remove the custom-device handling from the input-driver and instead extend/write a generic driver for the new capability. So I cannot follow how you think this will have the same problems as evdev? Or, let's ask the inverse question: How does this differ from the X11 model where we move
Re: [RFC] libinputmapper: Input device configuration for graphic-servers
Hi Peter On Thu, May 16, 2013 at 7:37 AM, Peter Hutterer peter.hutte...@who-t.net wrote: On Sun, May 12, 2013 at 04:20:59PM +0200, David Herrmann wrote: [..] So what is the proposed solution? My recommendation is, that compositors still search for devices via udev and use device drivers like libxkbcommon. So linux evdev handling is still controlled by the compositor. However, I'd like to see something like my libinputmapper proposal being used for device detection and classification. libinputmapper provides an inmap_evdev object which reads device information from an evdev-fd or sysfs /sys/class/input/inputnum path, performs some heuristics to classify it and searches it's global database for known fixups for broken devices. It then provides capabilities to the caller, which allow them to see what drivers to load on the device. And it provides a very simple mapping table that allows to apply fixup mappings for broken devices. These mappings are simple 1-to-1 mappings that are supposed to be applied before drivers handle the input. This is to avoid device-specific fixup in the drivers and move all this to the inputmapper. An example would be a remapping for gamepads that report BTN_A instead of BTN_NORTH, but we cannot fix them in the kernel for backwards-compatibility reasons. The gamepad-driver can then assume that if it receives BTN_NORTH, it is guaranteed to be BTN_NORTH and doesn't need to special case xbox360/etc. controllers, because they're broken. I think evdev is exactly that interface and apparently it doesn't work. if you want a mapping table, you need a per-client table because sooner or later you have a client that needs BTN_FOO when the kernel gives you BTN_BAR and you can't change the client to fix it. i.e. the same issue evdev has now, having a global remapping table just moves the problem down by 2 years. a mapping table is good, but you probably want two stages of mapping: one that's used in the compositor for truly broken devices that for some reason can't be fixed in the kernel, and one that's used on a per-client basis. and you'll likely want to be able to overide the client-specific from outside the client too. IMHO, the problem with evdev is, that it doesn't provide device classes. The only class we have is this is an input device. All other event-flags can be combined in whatever way we want. So like 10 years ago when the first gamepad driver was introduced, we added some mapping that was unique to this device (the device was probably unique, too). Some time later, we added some other gamepad-like driver with a different mapping (as it was probably a very different device-type, back then, and we didn't see it coming that this will become a wide-spread device-type). However, today we notice that a GamePad is an established type of device (like a touchpad), but we have tons of different mappings in the kernel for backwards-compatibility reasons. I can see that this kind of development can happen again (and very likely it _will_ happen again) and it will happen for all kinds of devices. But that's why I designed the proposal from a compositor's view instead of from a kernel's view. A touchpad driver of the compositor needs to know exactly what kind of events it gets from the kernel. If it gets wrong events, it will misbehave. As we cannot guarantee that all kernel drivers behave the same way, the compositor's touchpad driver needs to work around all these little details on a per-device basis. To avoid this, I tried to abstract the touchpad-protocol and moved per-device handling into a separate library. It detects all devices that can serve as a touchpad and fixes trivial (1-to-1 mapping) incompatibilities. This removes all per-device handling from the touchpad driver and it can expect all input it gets to be conform with a touchpad protocol. And in fact, it removes this from all the compositor's input drivers. So I think of it more like a lib-detect-and-make-compat. All devices that do not fall into one of the categories (I called it capability), will be handled as custom devices. So if we want an input driver for a new fancy device, then we need a custom driver, anyway (or adjust a generic driver to handle both). If at some point it turns out, that this kind of device becomes more established, we can add a new capability for it. Or we try extending an existing capability in a backwards-compatible way. We can then remove the custom-device handling from the input-driver and instead extend/write a generic driver for the new capability. So I cannot follow how you think this will have the same problems as evdev? Or, let's ask the inverse question: How does this differ from the X11 model where we move the custom device handling into the drivers? libinputmapper would use some static heuristics for all this, but additionally parse user-configuration. A configuration file contains [match] entries, which specify device-configurations to load
Re: Input and games.
Hi Pekka On Mon, May 6, 2013 at 8:54 AM, Pekka Paalanen ppaala...@gmail.com wrote: On Sun, 5 May 2013 15:27:54 -0400 Todd Showalter t...@electronjump.com wrote: Having given it some thought, I'd be inclined to be cautious about how much you consider the gamepad-with-builtin-keyboard case. They really made those things to make MMOs viable on game consoles. As far as I know, not a lot of people have them, and the main argument for them is on consoles which don't have native keyboards. On a PC, the kinds of games that need keyboards are the kinds of games you tend to want access to the mouse. That's not to say that nobody will ever use a gamepad keyboard in a game on Linux, but I'd argue it's on thin enough ground that I wouldn't let it drive the design considerations. I could imagine the Wii pointer exposed as a wl_pointer with the gamepad... hrm, that's another curious input device that does not fit well in our categories: it needs a cursor image, but provides absolute positions unlike a mouse, right? I wrote the Wii-Remote kernel driver and I can assure you that there is _no_ way you can handle this as a generic device. It is a good example for a device that needs client support to make sense. Of course, you can write a compositor input driver that emulates a mouse via the raw Wii-Remote input, but then you should also advertise it as a mouse to the clients. Depending on the method you use, it can report absolute or relative events (accel: relative, IR: absolute) I think we shouldn't try to support such special devices. It doesn't make sense. Instead, provide generic devices via generic interfaces (mouse, keyboard, touch, gamepad) but also provide a raw interface so clients can have device-specific support. And if it turns out that a new device-class emerges, we can always add support for them. Regards David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston v3 01/13] protocol: add sub-surfaces
Hi Pekka On Mon, Apr 29, 2013 at 8:58 AM, Pekka Paalanen ppaala...@gmail.com wrote: On Fri, 26 Apr 2013 11:38:12 -0500 Jason Ekstrand ja...@jlekstrand.net wrote: On Fri, Apr 26, 2013 at 9:14 AM, Jason Ekstrand ja...@jlekstrand.netwrote: pq, On Fri, Apr 26, 2013 at 7:09 AM, David Herrmann dh.herrm...@gmail.comwrote: Hi Pekka ... How's this? http://cgit.collabora.com/git/user/pq/weston.git/commit/?h=subsurface-wip2id=0dce7236606e0a433390c970560c3050247c4e60context=6 Perfect, that solves all my concerns. My thoughts were basically the same as David's and those changes solve my concerns as well. I'm still not sure I'm 100% happy with how it requires you to build strange trees in certain cases. That said, I think it looks pretty good and certainly good enough to go into weston so we can all start playing with it. I'm hoping I can get sub-surfaces implemented in my java compositor soonish. Looking good! --Jason Ekstrand Pekka asked me to send an e-mail with a little clarification as to what I meant by strange trees. Consider an EGL video surface with SHM CSD (see attachment). To make things more complicated, let's say that the video surface is controlled by an external library or (worse yet) is a foreign surface controlled by a different process. Due to current commit behavior and the way commits cascade, the EGL surface (which really is the primary surface in this case) can't be used as the primary surface. Instead, you would have to use one of the decoration surfaces as the parent surface (again, see the picture). Thank you, that makes your concern crystal clear. One solution to this would be to allow some sort of surface that contains no actual pixels but gets mapped anyway. Right now you can hack this with a 1x1 transparent buffer or something similar. However, I would rather not see such hacks common place. One way to achieve this would be to have some sort of empty buffer that has a size but no data. Then you could attach one of these empty buffers to a surface to get it mapped. I consider a special empty wl_buffer with fake size a hack, just like the 1x1 completely transparent wl_buffer, except worse, because it needs more special protocol to create. The other advantage of an empty buffer is that, if it's used as the main window, could be the full size of the window instead of being 1x1. This simplifies the problem of handling input across subsurfaces as you can have the parent surface handle them. It would also allow us (if we wanted) to clip subsurfaces to the parent surface without causing any major problems. Not that we necessarily should, but it would leave open the option. The 1x1 buffer could be scaled to any size with a generic crop scale extension, but that of course is yet another optional extension to depend on. Let's see how others feel about this, is the awkward surface structure too bad. Like I said in another email, I'm starting to think we are now beginning to accumulate hacks on top of hacks, because the original design turned out to be not flexible enough. We don't have an abstract window object in the protocol, which might have provided the sub-surface functionality in a straightforward way. A wl_window, defining what we now call the surface-local coordinate system, having no content or size on its own but attach point for an arbitrary number of wl_surfaces, being the item for role assignment but doing that in the shell level of the protocol instead of core would be problematic for re-usable components like video sink elements, since it does not provide nesting with wl_surfaces. Just as a side-note: a wl_window object doesn't really help here. Assume you have an independent client library that provides two sub-surfaces that ought to be displayed right next to each other (like a video-screen with a video-list-sidebar). You have two choices here: 1) make the video-screen the parent of the sidebar 2) provide two independent sub-surfaces to the caller Maybe the example isn't chosen well enough, but in my eyes both solutions seem wrong. Instead, a dummy wl_surface that can be used by the library as explicit parent would solve this case in a nice way. And always keep in mind that windows are not static. So in Jason's case, switching the window to fullscreen causes the decorations to be hidden and a client would have to de-couple the video-screen from the parent and instead make it the main-window (while fullscreen). So a wl_window object would again require a wl_subwindow object and it would basically just be a wl_surface with a transparent buffer. So why not add a call to bind a dummy 100% transparent buffer to a surface. This allows clients to insert invisible parent windows anywhere they want so it is easier to move/change/restack sub-surfaces. And it allows compositors to handle these surfaces as special surfaces to avoid overhead. Regards David
Re: [PATCH weston v3 01/13] protocol: add sub-surfaces
Hi On Mon, Apr 29, 2013 at 9:11 AM, Pekka Paalanen ppaala...@gmail.com wrote: On Sat, 27 Apr 2013 15:18:29 -0500 Jason Ekstrand ja...@jlekstrand.net wrote: Sorry to spam the list, but I had another idea kicking around my head this weekend that I thought was worth sharing. Please note that I don't think this is a stopper. I just think it's worth throwing out there and seeing if others think it's useful. I don't think there are any major holes in commit behavior in the current protocol. That said, there seems to be some confusion on commit modes and how they interact with the tree of subalgebras that I think is justified. I think this could be (somewhat) solved by the following simplification. We could replace the explicit commit modes that then impose commit modes on the children with a single cache_child_commits request which would begin caching the data for child surfaces until commit is called on the same surface. In terms of the current mechanism, cache_child_commits would set synced and commit would automatically unset synced. The advantage I see to this approach is that it makes it more clear that it affects the entire tree and how it does so. Also, it requires the client to explicitly put any synced commits between a cached_child_commits and commit which I personally think is cleaner. The disadvantage is that it is a little less flexible in that you can't cache single children. However, if you can have dummy nodes (see previous ML posts), this shouldn't be a problem in most cases. Again, I don't think this is a show-stopper and I think the protocol as-is is ok. I just thought it might be worth mentioning. --Jason Ekstrand Hi Jason, isn't this racy? If you want to keep all children always synchronized, don't you have to re-send cache_child_commits every time you commit the main surface, and hope that the child components to do not manage to send a new commit between your commit and cache_child_commits? We could just rename set_sync and set_desync to something more obvious, any suggestions? So the reason for this proposal is to make the protocol more obvious and easier to understand? Imo, that's a reason to improve documentation, not to change the protocol. And honestly, I don't think the design is hard to understand. All my concerns were about documentation, not about the design. Or what exactly is the hard to understand part of the current design? Cheers David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston v3 01/13] protocol: add sub-surfaces
Hi Pekka On Fri, Apr 26, 2013 at 1:12 PM, Pekka Paalanen ppaala...@gmail.com wrote: diff --git a/protocol/subsurface.xml b/protocol/subsurface.xml new file mode 100644 index 000..60b4002 --- /dev/null +++ b/protocol/subsurface.xml @@ -0,0 +1,236 @@ +?xml version=1.0 encoding=UTF-8? +protocol name=subsurface + + copyright +Copyright © 2012-2013 Collabora, Ltd. + +Permission to use, copy, modify, distribute, and sell this +software and its documentation for any purpose is hereby granted +without fee, provided that the above copyright notice appear in +all copies and that both that copyright notice and this permission +notice appear in supporting documentation, and that the name of +the copyright holders not be used in advertising or publicity +pertaining to distribution of the software without specific, +written prior permission. The copyright holders make no +representations about the suitability of this software for any +purpose. It is provided as is without express or implied +warranty. + +THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS +SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND +FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY +SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES +WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN +AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, +ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF +THIS SOFTWARE. + /copyright + + interface name=wl_subcompositor version=1 +description summary=sub-surface compositing + The global interface exposing sub-surface compositing capabilities. + A wl_surface, that has sub-surfaces associated, is called the + parent surface. Sub-surfaces can be arbitrarily nested and create + a tree of sub-surfaces. + + The root surface in a tree of sub-surfaces is the main + surface. The main surface cannot be a sub-surface, because + sub-surfaces must always have a parent. + + A main surface with its sub-surfaces forms a (compound) window. + For window management purposes, this set of wl_surface objects is + to be considered as a single window, and it should also behave as + such. + + The aim of sub-surfaces is to offload some of the compositing work + within a window from clients to the compositor. A prime example is + a video player with decorations and video in separate wl_surface + objects. This should allow the compositor to pass YUV video buffer + processing to dedicated overlay hardware when possible. +/description + +request name=destroy type=destructor + description summary=unbind from the subcompositor interface + Informs the server that the client will not be using this + protocol object anymore. This does not affect any other + objects, wl_subsurface objects included. + /description +/request + +enum name=error + entry name=bad_surface value=0 + summary=the to-be sub-surface is invalid/ + entry name=bad_parent value=1 + summary=the given parent is a sub-surface/ +/enum + +request name=get_subsurface + description summary=give a surface the role sub-surface + Create a sub-surface interface for the given surface, and + associate it with the given parent surface. This turns a + plain wl_surface into a sub-surface. + + The to-be sub-surface must not already have a dedicated + purpose, like any shell surface type, cursor image, drag icon, + or sub-surface. Otherwise a protocol error is raised. + /description + + arg name=id type=new_id interface=wl_subsurface + summary=the new subsurface object id/ + arg name=surface type=object interface=wl_surface + summary=the surface to be turned into a sub-surface/ + arg name=parent type=object interface=wl_surface + summary=the parent surface/ +/request + /interface + + interface name=wl_subsurface version=1 +description summary=sub-surface interface to a wl_surface + An additional interface to a wl_surface object, which has been + made a sub-surface. A sub-surface has one parent surface. + + A sub-surface becomes mapped, when a non-NULL wl_buffer is applied + and the parent surface is mapped. The order of which one happens + first is irrelevant. A sub-surface is hidden if the parent becomes + hidden, or if a NULL wl_buffer is applied. These rules apply + recursively through the tree of surfaces. +
Re: [PATCH weston v3 01/13] protocol: add sub-surfaces
Hi Pekka On Thu, Apr 25, 2013 at 12:57 PM, Pekka Paalanen ppaala...@gmail.com wrote: Add protocol for sub-surfaces, wl_subcompositor as the global interface, and wl_subsurface as the per-surface interface extension. This patch is meant to be reverted, once sub-surfaces are moved into Wayland core. Changes in v2: - Rewrite wl_subcompositor.get_subsurface description, and move mapping and commit details into wl_subsurface description. Check the wording in wl_subsurface.set_position description. - Add wl_subsurface.set_commit_mode request, and document it, with the commit_mode enum. Add bad_value error code for wl_subsurface. - Moved the protocol into Weston repository so we can land it upstream sooner for public exposure. It is to be moved into Wayland core later. - Add destroy requests to both wl_subcompositor and wl_subsurface, and document them. Experience has showed, that interfaces should always have a destructor unless there is a good and future-proof reason to not have it. Changes in v3: - Specify, that wl_subsurface will become inert, if the corresponding wl_surface is destroyed, instead of requiring a certain destruction order. - Replaced wl_subsurface.set_commit_mode with wl_subsurface.set_sync and wl_subsurface.set_desync. Parent-cached commit mode is now called synchronized, and independent mode is desynchronized. Removed commit_mode enum, and bad_vBut if we introduce other protocol operations that cause state to be applied, a recursive definition is alue error. - Added support for nested sub-surfaces. Nice! \o/ Signed-off-by: Pekka Paalanen ppaala...@gmail.com --- clients/.gitignore | 2 + clients/Makefile.am | 4 + clients/window.h| 1 + protocol/subsurface.xml | 236 src/.gitignore | 3 + src/Makefile.am | 4 + src/compositor.h| 1 + tests/.gitignore| 2 + tests/Makefile.am | 4 + 9 files changed, 257 insertions(+) create mode 100644 protocol/subsurface.xml diff --git a/clients/.gitignore b/clients/.gitignore index dcd4564..16088e8 100644 --- a/clients/.gitignore +++ b/clients/.gitignore @@ -20,6 +20,8 @@ simple-egl simple-shm simple-touch smoke +subsurface-client-protocol.h +subsurface-protocol.c tablet-shell-client-protocol.h tablet-shell-protocol.c text-client-protocol.h diff --git a/clients/Makefile.am b/clients/Makefile.am index 8c9bcd4..5f83acd 100644 --- a/clients/Makefile.am +++ b/clients/Makefile.am @@ -81,6 +81,8 @@ libtoytoolkit_la_SOURCES =\ window.h\ text-cursor-position-protocol.c \ text-cursor-position-client-protocol.h \ + subsurface-protocol.c \ + subsurface-client-protocol.h\ workspaces-protocol.c \ workspaces-client-protocol.h @@ -185,6 +187,8 @@ BUILT_SOURCES = \ desktop-shell-protocol.c\ tablet-shell-client-protocol.h \ tablet-shell-protocol.c \ + subsurface-client-protocol.h\ + subsurface-protocol.c \ workspaces-client-protocol.h\ workspaces-protocol.c diff --git a/clients/window.h b/clients/window.h index c2946d8..815b3f1 100644 --- a/clients/window.h +++ b/clients/window.h @@ -27,6 +27,7 @@ #include wayland-client.h #include cairo.h #include ../shared/config-parser.h +#include subsurface-client-protocol.h #define ARRAY_LENGTH(a) (sizeof (a) / sizeof (a)[0]) diff --git a/protocol/subsurface.xml b/protocol/subsurface.xml new file mode 100644 index 000..60b4002 --- /dev/null +++ b/protocol/subsurface.xml @@ -0,0 +1,236 @@ +?xml version=1.0 encoding=UTF-8? +protocol name=subsurface + + copyright +Copyright © 2012-2013 Collabora, Ltd. + +Permission to use, copy, modify, distribute, and sell this +software and its documentation for any purpose is hereby granted +without fee, provided that the above copyright notice appear in +all copies and that both that copyright notice and this permission +notice appear in supporting documentation, and that the name of +the copyright holders not be used in advertising or publicity +pertaining to distribution of the software without specific, +written prior permission. The copyright holders make no +representations about the suitability of this software for any +purpose. It is provided as is without express or implied +warranty. + +THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS +SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND +FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY +SPECIAL, INDIRECT OR
Re: Input and games.
Hi Todd On Tue, Apr 23, 2013 at 4:12 PM, Todd Showalter t...@electronjump.com wrote: On Tue, Apr 23, 2013 at 7:25 AM, Pekka Paalanen ppaala...@gmail.com wrote: what you describe here is very much a keymap-like database for game controllers: translating from button and axis indices to labels or symbols. However, having a brief chat with Daniel Stone, it seems we should not need these. Take a look at /usr/include/linux/input.h There you find definitions for BTN_A, BTN_X, BTN_START, ABS_X, ABS_Y, ABS_RX, ABS_RY, ABS_HATnn, and many more. The kernel evdev interface should alreay be giving out events with the correct label, so we would not need any mapping. Are you saying that the kernel gives out the labels wrong? If so, this should be fixed in the kernel drivers. One thing less to care about in Wayland. We just need to write the protocol for these devices, the labels should be already there. I'm not saying the labels are wrong; I assume they are correct. The problem is that the labels are hardware-specific, at least for the buttons. That said, it looks like the axis values are being properly labelled, which means we're way closer to sane behaviour than we were last time I looked into this. I grabbed evtest and ran it with three devices; an xbox controller, an xbox 360 controller, and a ps3 controller. The results: xbox: - button A B X Y START THUMBL THUMBR Z (white button) C (black button) SELECT (back button) - axis ABS_X ABS_Y ABS_Z ABS_RX ABS_RY ABS_RZ ABS_HAT0X ABS_HAT0Y (dpad) xbox 360: - button A B X Y START THUMBL THUMBR TL TR MODE (home button) SELECT (back button) - axis ABS_X ABS_Y ABS_Z ABS_RX ABS_RY ABS_RZ ABS_HAT0X ABS_HAT0Y (dpad) ps3: - button TRIGGER THUMB THUMB2 TOP TOP2 PINKIE BASE BASE2 BASE3 BASE3 BASE4 BASE5 BASE6 DEAD TRIGGER_HAPPY17 TRIGGER_HAPPY18 TRIGGER_HAPPY19 - axis ABS_X ABS_Y ABS_Z ABS_RZ ABS_MISC The xbox controller and the xbox 360 controller are more or less the same; the 360 controller has a couple of shoulder buttons instead of a the black and white buttons, and (somewhat oddly) the back buttons come in as select, but that's workable. It all rather goes pear-shaped when we get beyond that, though. The PS3 controller, while physically quite similar to the other two, even down to the placement of controls and how the controls are clustered, comes in completely differently. There is not a single button in common between the PS3 controller and the XBox controllers as reported by evdev, despite the PS3 controller having buttons physically labelled start and select, plus direct equivalents to many of the XBox 360 controller's parts (ie: TL, TR, MODE, ABS_HAT0X, ABS_HAT0Y, ABS_RX, ABS_RY...). That's a known problem that isn't easy to fix. Of course, we can adjust the kernel driver, but this breaks applications that expect the current behavior. The problem I see is that we never paid enough attention how keys are mapped when adding kernel drivers and now we have a mess of different mappings that cannot be fixed. You can try to send patches to linux-in...@vger.kernel.org, but chances are low that they will get merged. Nevertheless, the problem is actually easy to fix in userspace. You just need to create buttons mappings for the different devices. There is no complex logic involved. It would be enough to have a bunch of static tables indexed by input-device names which map the input keycode to the correct/expected output keycode. But I cannot see a reason why a compositor should do this, though. This can be easily put into a library and every game that needs it performs the mappings. Table-mappings add a single memory-read which shouldn't affect performance and clients can share the library. The compositor isn't interested in joystick/gamepad events so my first approach would be to let clients handle them. The PS3 controller also has several (?) entries for buttons and axis values, and also appears to have (if I understand correctly) a bunch of codes for a multitouch panel? I couldn't tell you what the right or left stick axis values are in the above, because though I did build my kernel with ps3 controller support, and evtest did see it and dump the supported event list, I get no events logged from... ah, ok, I have to hit the PS button to get it to actually work. And now there's a torrent of what I assume is accelerometer data coming in on (?) events. It turns out the left stick is ABS_X and ABS_Y, and right stick is ABS_Z and ABS_RZ. I suspect this is just broken somehow. Maybe the ps3 gamepad kernel driver is still a work in progress? But this is the kind of thing I was talking about; the data I get from a ps3 gamepad is mapped totally differently from the data I get from an xbox gamepad, so from a
Re: [PATCH 1/3] Extract the EDID blob when adding a DRM output
Hi On Fri, Apr 19, 2013 at 9:19 PM, Kai-Uwe Behrmann k...@gmx.de wrote: Am 19.04.2013 21:12, schrieb David Herrmann: Why do you keep the blob around all the time? Just free it in create_output_for_connector() after you parsed it. If we somehow need it later, we can always change it again. But your patches don't depend on it being around all the time so lets just free it right away. The EDID blob is exposed under X11, which is a good thing in many cases. Do you have a special reason to suggest otherwise on Wayland/Weston? This patchset doesn't expose the blob. Why do you want to keep it around then? Regards David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 1/3] Extract the EDID blob when adding a DRM output
Hi On Fri, Apr 19, 2013 at 5:02 PM, Richard Hughes hughsi...@gmail.com wrote: --- src/compositor-drm.c | 26 ++ 1 file changed, 26 insertions(+) diff --git a/src/compositor-drm.c b/src/compositor-drm.c index da1ba79..61ef97e 100644 --- a/src/compositor-drm.c +++ b/src/compositor-drm.c @@ -139,6 +139,7 @@ struct drm_output { int pipe; uint32_t connector_id; drmModeCrtcPtr original_crtc; + drmModePropertyBlobPtr edid_blob; int vblank_pending; int page_flip_pending; @@ -1011,6 +1012,9 @@ drm_output_destroy(struct weston_output *output_base) (struct drm_compositor *) output-base.compositor; drmModeCrtcPtr origcrtc = output-original_crtc; + if (output-edid_blob) + drmModeFreePropertyBlob(output-edid_blob); + Why do you keep the blob around all the time? Just free it in create_output_for_connector() after you parsed it. If we somehow need it later, we can always change it again. But your patches don't depend on it being around all the time so lets just free it right away. if (output-backlight) backlight_destroy(output-backlight); @@ -1499,6 +1503,7 @@ create_output_for_connector(struct drm_compositor *ec, drmModeEncoder *encoder; drmModeModeInfo crtc_mode; drmModeCrtc *crtc; + drmModePropertyPtr property; We use drmXY * instead of drmXYPtr here, so lets be consistent. int i; char name[32]; const char *type_name; @@ -1642,6 +1647,27 @@ create_output_for_connector(struct drm_compositor *ec, wl_list_insert(ec-base.output_list.prev, output-base.link); + /* find the EDID blob */ + for (i = 0; i connector-count_props !output-edid_blob; i++) { + property = drmModeGetProperty(ec-drm.fd, connector-props[i]); + if (!property) + continue; + if ((property-flags DRM_MODE_PROP_BLOB) + !strcmp(property-name, EDID)) { + output-edid_blob = drmModeGetPropertyBlob(ec-drm.fd, + connector-prop_values[i]); + } + drmModeFreeProperty(property); + } + + /* parse the EDID blob */ + if (output-edid_blob) { + weston_log(Got EDID blob %p of size %u.\n, + output-edid_blob-data, + output-edid_blob-length); + /* FIXME: actually parse EDID */ + } + output-base.origin = output-base.current; output-base.start_repaint_loop = drm_output_start_repaint_loop; output-base.repaint = drm_output_repaint; Regards David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH libxkbcommon v2] keymap: add xkb_keymap_new_from_buffer()
The current API doesn't allow the caller to create keymaps from mmap()'ed files. The problem is, xkb_keymap_new_from_string() requires a terminating 0 byte. However, there is no way to guarantee that when using mmap() so a user currently has to copy the whole file just to get the terminating zero byte (assuming they cannot use xkb_keymap_new_from_file()). This adds a new entry xkb_keymap_new_from_buffer() which takes a memory location and the buffer size in bytes. Internally, we depend on yy_scan_{string,byte}() helpers. According to flex documentation these already copy the input string because they are wrappers around yy_scan_buffer(). yy_scan_buffer() on the other hand has some insane requirements. The buffer must be writeable and the last two bytes must be ASCII-NUL. But the buffer may contain other 0 bytes just fine. Because we don't want these constraints in our public API, xkb_keymap_new_from_buffer() needs to create a copy of the input memory. But it then calls yy_scan_buffer() directly. Hence, we have the same number of buffer-copies as with *_from_string() but without the terminating 0 requirement. The explicit yy_scan_buffer() call is preferred over yy_scan_byte() so the buffer-copy operation is not hidden somewhere in flex. Maybe some day we no longer depend on flex and can have a zero-copy API. A user could mmap() a file and it would get parsed right from this buffer. But until then, we shouldn't expose this limitation in the API but instead provide an API that some day can work with zero-copy. Signed-off-by: David Herrmann dh.herrm...@gmail.com --- Makefile.am| 2 ++ src/xkbcomp/scanner.l | 32 + src/xkbcomp/xkbcomp-priv.h | 4 +++ src/xkbcomp/xkbcomp.c | 45 +++ test/.gitignore| 1 + test/buffercomp.c | 90 ++ test/common.c | 15 test/test.h| 3 ++ xkbcommon/xkbcommon.h | 14 9 files changed, 206 insertions(+) create mode 100644 test/buffercomp.c diff --git a/Makefile.am b/Makefile.am index 6ecb8f5..3cb4219 100644 --- a/Makefile.am +++ b/Makefile.am @@ -140,6 +140,7 @@ TESTS = \ test/context \ test/rules-file \ test/stringcomp \ + test/buffercomp \ test/keyseq \ test/log TESTS_LDADD = libtest.la @@ -152,6 +153,7 @@ test_context_LDADD = $(TESTS_LDADD) test_rules_file_CFLAGS = $(AM_CFLAGS) -Wno-declaration-after-statement test_rules_file_LDADD = $(TESTS_LDADD) -lrt test_stringcomp_LDADD = $(TESTS_LDADD) +test_buffercomp_LDADD = $(TESTS_LDADD) test_keyseq_LDADD = $(TESTS_LDADD) test_log_LDADD = $(TESTS_LDADD) test_interactive_LDADD = $(TESTS_LDADD) diff --git a/src/xkbcomp/scanner.l b/src/xkbcomp/scanner.l index 5ccf3e9..f30462d 100644 --- a/src/xkbcomp/scanner.l +++ b/src/xkbcomp/scanner.l @@ -267,6 +267,38 @@ XkbParseString(struct xkb_context *ctx, const char *string, return xkb_file; } +/* + * yy_scan_buffer() requires the last two bytes of \buf to be 0. These two bytes + * are not scanned. Other zero bytes in the buffer are scanned normally, though. + * Due to these terminating zeroes, \length must be greater than 2. + * Furthermore, the buffer must be writable and you cannot make any assumptions + * about it after the scanner finished. + * All this must be guaranteed by the caller of this function! + */ +XkbFile * +XkbParseBuffer(struct xkb_context *ctx, char *buf, size_t length, + const char *file_name) +{ +yyscan_t scanner; +struct scanner_extra extra; +YY_BUFFER_STATE state; +XkbFile *xkb_file; + +if (!init_scanner(scanner, extra, ctx, file_name)) +return NULL; + +xkb_file = NULL; +state = yy_scan_buffer(buf, length, scanner); +if (state) { +xkb_file = parse(ctx, scanner, NULL); +yy_delete_buffer(state, scanner); +} + +clear_scanner(scanner); + +return xkb_file; +} + XkbFile * XkbParseFile(struct xkb_context *ctx, FILE *file, const char *file_name, const char *map) diff --git a/src/xkbcomp/xkbcomp-priv.h b/src/xkbcomp/xkbcomp-priv.h index b2b40fd..0d35255 100644 --- a/src/xkbcomp/xkbcomp-priv.h +++ b/src/xkbcomp/xkbcomp-priv.h @@ -45,6 +45,10 @@ XkbFile * XkbParseString(struct xkb_context *ctx, const char *string, const char *file_name); +XkbFile * +XkbParseBuffer(struct xkb_context *ctx, char *buf, size_t length, + const char *file_name); + void FreeXkbFile(XkbFile *file); diff --git a/src/xkbcomp/xkbcomp.c b/src/xkbcomp/xkbcomp.c index 5025dc6..9bb2725 100644 --- a/src/xkbcomp/xkbcomp.c +++ b/src/xkbcomp/xkbcomp.c @@ -149,6 +149,51 @@ xkb_keymap_new_from_string(struct xkb_context *ctx, } XKB_EXPORT struct xkb_keymap * +xkb_keymap_new_from_buffer(struct xkb_context *ctx, + const char *buffer, + size_t length, + enum