Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Full drawing tablet support. #4570

Merged
merged 1 commit into from
Sep 26, 2019

Conversation

jchv
Copy link
Contributor

@jchv jchv commented Sep 18, 2019

This implements the wlr tablet v2 protocol, including fallback events for surfaces that do not support it. This is mostly based on the code in Rootston, but differs enough thanks to Sway's input system being reasonably different from Rootston's.

Most things seem to work correctly at this point. I do not own a tablet with certain features (like tilt, or a ring) so I can not test all functionality.

Applications tested:

  • GIMP (XWayland)
  • Krita (XWayland)
  • Xournalpp (native Wayland)
  • swaynag (native Wayland, uses fallback)

Fixes #2288.

@jchv jchv force-pushed the drawing-tablet-support branch 2 times, most recently from bb4485b to 493c50b Compare September 22, 2019 08:09
@jchv jchv changed the title Drawing tablet support. (#2288) Full drawing tablet support. Sep 22, 2019
@jchv jchv marked this pull request as ready for review September 22, 2019 08:12
@jchv
Copy link
Contributor Author

jchv commented Sep 22, 2019

OK, this is probably close to ready for review at this point. This ended up being rather large, so it is possible it may make sense to split some of the work into separate commits/PRs, but I was having trouble deciding how exactly to split it, so I left it as-is for now.

@ddevault
Copy link
Contributor

This looks pretty good overall. From testing:

  • Pen button events are not sent to clients
  • I think setting the cursor image for the pen is not always working

I do not own a tablet with certain features (like tilt, or a ring) so I can not test all functionality.

I have one of these, for this express purpose :) tilt and ring both works, but I found that mode switching events are not sent to clients.

@ddevault
Copy link
Contributor

ddevault commented Sep 23, 2019

Segfault:

#0  0x00007f7d80ac8c50 in wl_client_get_display () at /lib64/libwayland-server.so.0
#1  0x00007f7d811001b1 in wlr_seat_client_next_serial (client=0x55bd8bd20b50) at ../subprojects/wlroots/types/seat/wlr_seat.c:375
        serial = 21949
        set = 0x55bd8c1fdb88
#2  0x00007f7d81101ddb in wlr_send_tablet_v2_tablet_pad_leave (pad=0x55bd8bf53240, surface=0x55bd8bad95a0) at ../subprojects/wlroots/types/tablet_v2/wlr_tablet_v2_pad.c:532
        client = 0x55bd8c08af30
        serial = 32637
#3  0x00007f7d811023cc in default_pad_leave (grab=0x55bd8bf532a8, surface=0x55bd8bad95a0) at ../subprojects/wlroots/types/tablet_v2/wlr_tablet_v2_pad.c:681
#4  0x00007f7d81102179 in wlr_tablet_v2_tablet_pad_notify_leave (pad=0x55bd8bf53240, surface=0x55bd8bad95a0) at ../subprojects/wlroots/types/tablet_v2/wlr_tablet_v2_pad.c:619
#5  0x000055bd8a7f0bb8 in handle_pad_tablet_surface_destroy (listener=0x55bd8bf7bf70, data=0x55bd8bad95a0) at ../sway/input/tablet.c:309
        tablet_pad = 0x55bd8bf7bee0
#6  0x00007f7d81134593 in wlr_signal_emit_safe (signal=0x55bd8bad9860, data=0x55bd8bad95a0) at ../subprojects/wlroots/util/signal.c:29
        pos = 0x55bd8bf7bf70
        l = 0x55bd8bf7bf70
        cursor = {link = {prev = 0x55bd8bf7bf70, next = 0x55bd8befcf50}, notify = 0x7f7d811344dd <handle_noop>}
        end = {link = {prev = 0x55bd8b8d2288, next = 0x55bd8bad9860}, notify = 0x7f7d811344dd <handle_noop>}
#7  0x00007f7d8112d9ee in surface_handle_resource_destroy (resource=0x55bd8bf9f0e0) at ../subprojects/wlroots/types/wlr_surface.c:567
        surface = 0x55bd8bad95a0
#8  0x00007f7d80ac9d6f in  () at /lib64/libwayland-server.so.0
#9  0x00007f7d80ace182 in  () at /lib64/libwayland-server.so.0
#10 0x00007f7d80ace68f in  () at /lib64/libwayland-server.so.0
#11 0x00007f7d80ac9eef in wl_client_destroy () at /lib64/libwayland-server.so.0
#12 0x00007f7d80ac9fcb in  () at /lib64/libwayland-server.so.0
#13 0x00007f7d80acb7f2 in wl_event_loop_dispatch () at /lib64/libwayland-server.so.0
#14 0x00007f7d80aca39c in wl_display_run () at /lib64/libwayland-server.so.0
#15 0x000055bd8a7d5e27 in server_run (server=0x55bd8a840020 <server>) at ../sway/server.c:205
#16 0x000055bd8a7d5481 in main (argc=3, argv=0x7fffc8c4e408) at ../sway/main.c:400
        verbose = 0
        debug = 1
        validate = 0
        allow_unsupported_gpu = 0
        long_options =
            {{name = 0x55bd8a825028 "help", has_arg = 0, flag = 0x0, val = 104}, {name = 0x55bd8a82502d "config", has_arg = 1, flag = 0x0, val = 99}, {name = 0x55bd8a825034 "validate", has_arg = 0, flag = 0x0, val = 67}, {name = 0x55bd8a82503d "debug", has_arg = 0, flag = 0x0, val = 100}, {name = 0x55bd8a825043 "version", has_arg = 0, flag = 0x0, val = 118}, {name = 0x55bd8a82504b "verbose", has_arg = 0, flag = 0x0, val = 86}, {name = 0x55bd8a825053 "get-socketpath", has_arg = 0, flag = 0x0, val = 112}, {name = 0x55bd8a825062 "unsupported-gpu", has_arg = 0, flag = 0x0, val = 117}, {name = 0x55bd8a825072 "my-next-gpu-wont-be-nvidia", has_arg = 0, flag = 0x0, val = 117}, {name = 0x0, has_arg = 0, flag = 0x0, val = 0}}
        config_path = 0x0
        usage = 0x55bd8a824ad0 "Usage: sway [options] [command]\n\n  -h, --help", ' ' <repeats 13 times>, "Show help message and quit.\n  -c, --config <config>  Specify a config file.\n  -C, --validate         Check the validity of the config file, th"...
        c = -1

@jchv
Copy link
Contributor Author

jchv commented Sep 23, 2019

Thanks for testing! I will take a look at these tonight. I think set cursor has the least thought put into it, it’s mostly the code from rootston, probably some bad assumptions. I didn’t notice many issues but I think my testing was probably not thorough enough.

TODO

  • Fix segfault in handle_pad_tablet_surface_destroy
  • Fix spotty set_cursor implementation on tool.
  • (just realized) Make sure wlr tablet v2 structures are cleaned up when the tablets are destroyed
  • pen button events

@jchv
Copy link
Contributor Author

jchv commented Sep 25, 2019

Last night I added support for pen buttons with fallback. The fallback just maps all of them to right button like it did before.

I am also working to reproduce the crash that occurs when a surface is destroyed. Do you have any idea how to reproduce? I think it is most likely safe to simply not send the leave event if the surface is being destroyed, but I wasn’t sure if that would cause any weird edge cases, and in testing it never seemed to cause any memory/crashing issues for me.

I’ve found another annoying issue that I believe is in libinput: it looks like it is creating sporadic proximity-out/proximity-in events when the tablet pen is held fairly still. I can see the events in libinput debug-events, but watching with evtest the kernel driver does not appear to be emitting anything relevant. Not sure what exactly to do about that one, may have to debug libinput itself a bit. (Not my bug; Filed libinput#365.)

@ddevault
Copy link
Contributor

I am also working to reproduce the crash that occurs when a surface is destroyed. Do you have any idea how to reproduce?

I think what I had done was open Xournalpp, mess around with it, then closed it. Or maybe SIGINT'd it. One common mistake with this stuff is to tear down the listener without setting it back up again - make sure to wl_list_init afterwards.

@ddevault
Copy link
Contributor

Tested again, everything seems to be working pretty well.

@ddevault
Copy link
Contributor

Famous last words. Hotplugging a tablet gave me this:

#0  0x0000559b44c2a7ea in sway_configure_tablet (tablet=0x559b4642bda0) at ../sway/input/tablet.c:73
        pad_device = 0x559b46478e10
        pad_group = 0x559b4642bda0
        device = 0x559b464229f0
        seat = 0x559b45ba2ab0
        group = 0x559b4646d2a0
        tablet_pad = 0x559b4640b430
#1  0x0000559b44c241c1 in seat_configure_tablet_tool (seat=0x559b45ba2ab0, sway_device=0x559b4644e480) at ../sway/input/seat.c:657
#2  0x0000559b44c2437f in seat_configure_device (seat=0x559b45ba2ab0, input_device=0x559b46413090) at ../sway/input/seat.c:704
        seat_device = 0x559b4644e480
#3  0x0000559b44c245b2 in seat_add_device (seat=0x559b45ba2ab0, input_device=0x559b46413090) at ../sway/input/seat.c:763
        seat_device = 0x559b4644e480
#4  0x0000559b44c1b911 in handle_new_input (listener=0x559b45ba28e0, data=0x559b464229f0) at ../sway/input/input-manager.c:245
        seat_config = 0x559b45bd7c50
        input = 0x559b45ba28b0
        device = 0x559b464229f0
        input_device = 0x559b46413090
        __PRETTY_FUNCTION__ = "handle_new_input"
        added = false
        seat = 0x559b45ba2ab0
#5  0x00007f2a6e0a9593 in wlr_signal_emit_safe (signal=0x559b457413b8, data=0x559b464229f0) at ../subprojects/wlroots/util/signal.c:29
        pos = 0x559b45ba28e0
        l = 0x559b45ba28e0
        cursor = {link = {prev = 0x559b45ba28e0, next = 0x7ffed1e018f0}, notify = 0x7f2a6e0a94dd <handle_noop>}
        end = {link = {prev = 0x7ffed1e018d0, next = 0x559b457413b8}, notify = 0x7f2a6e0a94dd <handle_noop>}
#6  0x00007f2a6e05b858 in new_input_reemit (listener=0x559b45746b00, data=0x559b464229f0) at ../subprojects/wlroots/backend/multi/backend.c:137
        state = 0x559b45746af0
#7  0x00007f2a6e0a9593 in wlr_signal_emit_safe (signal=0x559b45746a58, data=0x559b464229f0) at ../subprojects/wlroots/util/signal.c:29
        pos = 0x559b45746b00
        l = 0x559b45746b00
        cursor = {link = {prev = 0x559b45746b00, next = 0x7ffed1e01990}, notify = 0x7f2a6e0a94dd <handle_noop>}
        end = {link = {prev = 0x7ffed1e01970, next = 0x559b45746a58}, notify = 0x7f2a6e0a94dd <handle_noop>}
#8  0x00007f2a6e058186 in handle_device_added (backend=0x559b45746a40, libinput_dev=0x559b46433780) at ../subprojects/wlroots/backend/libinput/events.c:150
        wlr_dev = 0x559b464229f0
        vendor = 1386
        product = 827
        name = 0x559b464201a0 "Wacom Intuos S 2 Pen"
        wlr_devices = 0x559b463eac10
        dev = 0x14646286e
        tmp_dev = 0xb4aa14119fe93500
#9  0x00007f2a6e05871e in handle_libinput_event (backend=0x559b45746a40, event=0x559b463c7ac0) at ../subprojects/wlroots/backend/libinput/events.c:231
        libinput_dev = 0x559b46433780
        event_type = LIBINPUT_EVENT_DEVICE_ADDED
#10 0x00007f2a6e0575da in handle_libinput_readable (fd=30, mask=1, _backend=0x559b45746a40) at ../subprojects/wlroots/backend/libinput/backend.c:41
        backend = 0x559b45746a40
        event = 0x559b463c7ac0
#11 0x00007f2a6da407f2 in wl_event_loop_dispatch () at /lib64/libwayland-server.so.0
#12 0x00007f2a6da3f39c in wl_display_run () at /lib64/libwayland-server.so.0
#13 0x0000559b44c0fe78 in server_run (server=0x559b44c7a040 <server>) at ../sway/server.c:205
#14 0x0000559b44c0f4d2 in main (argc=3, argv=0x7ffed1e01df8) at ../sway/main.c:402
        verbose = 0
        debug = 1
        validate = 0
        allow_unsupported_gpu = 0
        long_options =

@jchv
Copy link
Contributor Author

jchv commented Sep 25, 2019

Oh, oops. I think I have that one fixed locally and just haven’t pushed it up yet. If I remember correctly, I just forgot to unlink the tablet pad on destroy, so when it scans for tablet pads it does a use after free.

sway/input/cursor.c Outdated Show resolved Hide resolved
&surface, &sx, &sy);

// TODO: is this how the fallback check should work? Should this be kept
// track of elsewhere?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

sway/input/tablet.c Outdated Show resolved Hide resolved
sway/input/tablet.c Outdated Show resolved Hide resolved
Copy link
Contributor

@ddevault ddevault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style issues fixed and this is 👍 from me

@ddevault
Copy link
Contributor

Rebase?

@ddevault
Copy link
Contributor

Squash, rather

@jchv
Copy link
Contributor Author

jchv commented Sep 26, 2019

Yeah, I'll squash it, hang on a sec.

Sway has basic support for drawing tablets, but does not expose
properties such as pressure sensitivity. This implements the wlr tablet
v2 protocol, providing tablet events to Wayland clients.
@ddevault
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Implement drawing tablet protocol
2 participants