Re: libinput drops keys from infrared remotes after changing keymap
Hi, On 30-09-2019 10:33, Sean Young wrote: On Mon, Sep 30, 2019 at 11:34:04AM +1000, Peter Hutterer wrote: On Sun, Sep 29, 2019 at 08:17:38PM +0100, Sean Young wrote: When using IR receivers using libinput, key events get dropped if a new rc keymap is loaded and the key was not in the old keymap. The input device keybit changes and libevdev does not notice this. Then here we end up returning false: https://gitlab.freedesktop.org/libinput/libinput/blob/master/src/libinput.c#L3100 The event is reported via evtest but not via libinput debug-events. basic problem here: evdev isn't really designed for run-time changes of devices, so both libevdev and libinput expect the device to be static. There's SYN_CONFIG which **may** be usable for this but it's unused in the kernel and thus nothing in userspace handles it either. A discussion years ago about defining SYN_CONFIG for devices that change at runtime was put into the too-hard basket, in most cases creating a new device is more sensible. That's a shame. So, for example, mceusb IR devices register with a keymap for MCE remotes. If later a keymap for a different remote is loaded, e.g.: $ ir-keytable -w /lib/udev/rc_keymaps/imon_rsc.toml If I press any button which does not exist in the MCE remote keymap (for example space or backspace), libinput does not report these events. I've tried fixing this by making the kernel input device have all the keybit fields set, but this had to be reverted since it overflowed the size of uevents. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=05f0edadcc5fccdfc0676825b3e70e75dc0a8a84 If libinput receives an EV_KEY event which is unexpected, we could go and check if the input device and see if keybit has added. yeah, well... none of the libinput clients would know how to handle this either, so libinput would have to emulate removal and plug-in of a new device. Which of course is doable but quite a bit of work to get right. However in a diferent sense ir-keytable changed the input device underneath libinput; another way to fix this to add support for loading IR keymaps to libinput and all the way up the stack. I've been wanting to do this anyway but I have no idea if there is any interest in this. So how about libinput handling IR keymap changes? Would patches for that be accepted? libinput would need to load BPF IR decoders for this to work, amongst other things. This would would mostly mean porting it from ir-keytable. Any thoughts on this would be appreciated! libinput relies on udev, either directly or through the X server. So if you trigger your device to get removed and re-added through udev, libinput will add it and re-initialize it with whatever current bits it has. sudo udevadm trigger --action=remove /sys/class/input/event5 sudo udevadm trigger --action=add /sys/class/input/event5 Hmm, this is bit of an ugly workaround, even if generated them from ir-keytable. We could change the kernel to re-create the input device when the keymap changes, but this does not fit the current model very well. Not quite sure how to solve this yet. Just reading along here as an ex libinput contributor. I have the feeling that it will help if you can explain your specific use-case, because it sounds like you really want to change the keymap on the fly, while normally the keymap is something which gets setup once and the loaded by udev add device init time... So can you please explain your specific use-case here? Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: libinput drops keys from infrared remotes after changing keymap
Hi, On 01-10-2019 10:39, Sean Young wrote: Hi Hans, On Mon, Sep 30, 2019 at 01:29:59PM +0200, Hans de Goede wrote: On 30-09-2019 10:33, Sean Young wrote: On Mon, Sep 30, 2019 at 11:34:04AM +1000, Peter Hutterer wrote: On Sun, Sep 29, 2019 at 08:17:38PM +0100, Sean Young wrote: Any thoughts on this would be appreciated! libinput relies on udev, either directly or through the X server. So if you trigger your device to get removed and re-added through udev, libinput will add it and re-initialize it with whatever current bits it has. sudo udevadm trigger --action=remove /sys/class/input/event5 sudo udevadm trigger --action=add /sys/class/input/event5 Hmm, this is bit of an ugly workaround, even if generated them from ir-keytable. We could change the kernel to re-create the input device when the keymap changes, but this does not fit the current model very well. Not quite sure how to solve this yet. Just reading along here as an ex libinput contributor. I have the feeling that it will help if you can explain your specific use-case, because it sounds like you really want to change the keymap on the fly, while normally the keymap is something which gets setup once and the loaded by udev add device init time... You're absolutely right, that is exactly what I'm trying to do. So can you please explain your specific use-case here? Simply loading a new keymap while logged in, or rather solve the issue of "my remote doesn't work after loading the correct keymap". > My longer term goal is to provide gnome-control-center plugin for configuring IR receivers. So what I'm hearing is that from libinput's perspective, an input device should not change once created. In order to support this use-case, either ask the user to logout/login or the kernel should re-create the input device when the keymap changes. Right, so if this is for a gnome-control-center plugin then I guess the idea is the user sets things up once and then they never change, right? So how does the keymap stay set / gets stored ? Do you write it out globally so that udev will do the right thing next boot. Or does the g-c-c plugin register an autostart user service which does this once the user logs in? Assuming you set the mapping globally, then you need some DBUS activated helper daemon running as root, or some such, right (hopefully with access managed by polkit)? Then it wouldn't be too hard to do the: sudo udevadm trigger --action=remove /sys/class/input/event5 sudo udevadm trigger --action=add /sys/class/input/event5 Peter suggested from that daemon, and if this is only done on keymap changes, then this seems like a reasonable solution? Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: libinput drops keys from infrared remotes after changing keymap
Hi Sean, On 02-10-2019 11:33, Sean Young wrote: Hi, On Tue, Oct 01, 2019 at 03:32:52PM +0200, Hans de Goede wrote: On 01-10-2019 10:39, Sean Young wrote: On Mon, Sep 30, 2019 at 01:29:59PM +0200, Hans de Goede wrote: So can you please explain your specific use-case here? Simply loading a new keymap while logged in, or rather solve the issue of "my remote doesn't work after loading the correct keymap". > My longer term goal is to provide gnome-control-center plugin for configuring IR receivers. So what I'm hearing is that from libinput's perspective, an input device should not change once created. In order to support this use-case, either ask the user to logout/login or the kernel should re-create the input device when the keymap changes. Right, so if this is for a gnome-control-center plugin then I guess the idea is the user sets things up once and then they never change, right? Yes, that's the idea. It might need an interface for testing keymaps and/or adding custom keymaps. That depends on the community, really. So how does the keymap stay set / gets stored ? Do you write it out globally so that udev will do the right thing next boot. So ir-keytable comes with a udev rule which automagically load the keymap based on what's in /etc/rc_maps.cfg. I'm not sure how this should be written from g-c-c or whether this is right thing to do. See: https://git.linuxtv.org/v4l-utils.git/tree/utils/keytable/70-infrared.rules Ok. Or does the g-c-c plugin register an autostart user service which does this once the user logs in? I'm not convinced that's the right approach. Right, I agree. I think i would be best for the g-c-c plugin to set the rcmap globally which would mean writing out /etc/rc_maps.cfg. The normal way to do this is a small dbus activated daemon (so that it is not running all the time and is not started unnecessarily at boot) which can write out /etc/rc_maps.cfg for us. Such a daemon would then normally use polkit to check if the process making dbus calls to change the config is allowed to do so (reading it can be allowed to the world I think). Assuming you set the mapping globally, then you need some DBUS activated helper daemon running as root, or some such, right (hopefully with access managed by polkit)? Then it wouldn't be too hard to do the: sudo udevadm trigger --action=remove /sys/class/input/event5 sudo udevadm trigger --action=add /sys/class/input/event5 Peter suggested from that daemon, and if this is only done on keymap changes, then this seems like a reasonable solution? It does seem reasonable. It seemed a bit strange at first. I've been maintaining rc-core (IR) kernel side for some time, I'm trying to move up the stack. Your help is very much appreciated! You're welcome. As mentioned above I believe a dbus activated daemon to do the actual config changes (with polkit as authorization mechanism) would be best here. That is more or less the standard design pattern for cases like this. See e.g. also systemd-localed for writing /etc/vconsole.conf, etc. Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Current state of Window Decorations
Hi, On 6/25/20 11:40 AM, Simon Ser wrote: On Thursday, June 25, 2020 11:01 AM, Brad Robinson wrote: As a toolkit developer coming from Windows/OSX this is fairly shocking. I'm aware of the decoration protocol, but given it's not supported (and by the sound of it never will be) on some of the major distros makes it almost worthless. It's not really about distros, it's mainly about compositors. GNOME doesn't support it, but many other compositors do (KDE, Sway, and other wlroots-based compositors). If you don't know about libdecoration [1], maybe it'll help, but it's still pretty wip. [1]: https://gitlab.gnome.org/jadahl/libdecoration libdecoration is being used to add client-side decoration support to blender, see: https://developer.blender.org/D7989?id=25771 So recently it has been actively worked (and become less WIP). So if you do not want to completely roll client-side decoration support from scratch, then using libdecoration is likely a good option. Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[RFC] drm/kms: control display brightness through drm_connector properties
As discussed already several times in the past: https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/ https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.intel.com/ The current userspace API for brightness control offered by /sys/class/backlight devices has various issues, the biggest 2 being: 1. There is no way to map the backlight device to a specific display-output / panel (1) 2. Controlling the brightness requires root-rights requiring desktop-environments to use suid-root helpers for this. As already discussed on various conference's hallway tracks and as has been proposed on the dri-devel list once before (2), it seems that there is consensus that the best way to to solve these 2 issues is to add support for controlling a video-output's brightness through properties on the drm_connector. This RFC outlines my plan to try and actually implement this, which has 3 phases: Phase 1: Stop registering multiple /sys/class/backlight devs for a single display = On x86 there can be multiple firmware + direct-hw-access methods for controlling the backlight and in some cases the kernel registers multiple backlight-devices for a single internal laptop LCD panel: a) i915 and nouveau unconditionally register their "native" backlight dev even on devices where /sys/class/backlight/acpi_video0 must be used to control the backlight, relying on userspace to prefer the "firmware" acpi_video0 device over "native" devices. b) amdgpu and nouveau rely on the acpi_video driver initializing before them, which currently causes /sys/class/backlight/acpi_video0 to usually show up and then they register their own native backlight driver after which the drivers/acpi/video_detect.c code unregisters the acpi_video0 device. This means that userspace briefly sees 2 devices and the disappearing of acpi_video0 after a brief time confuses the systemd backlight level save/restore code, see e.g.: https://bbs.archlinux.org/viewtopic.php?id=269920 I already have a pretty detailed plan to tackle this, which I will post in a separate RFC email. I plan to start working on this right away, as it will be good to have this fixed regardless. Phase 2: Add drm_connector properties mirroring the matching backlight device = The plan is to add a drm_connector helper function, which optionally takes a pointer to the backlight device for the GPU's native backlight device, which will then mirror the backlight settings from the backlight device in a set of read/write brightness* properties on the connector. This function can then be called by GPU drivers for the drm_connector for the internal panel and it will then take care of everything. When there is no native GPU backlight device, or when it should not be used then (on x86) the helper will use the acpi_video_get_backlight_type() to determine which backlight-device should be used instead and it will find + mirror that one. Phase 3: Deprecate /sys/class/backlight uAPI Once most userspace has moved over to using the new drm_connector brightness props, a Kconfig option can be added to stop exporting the backlight-devices under /sys/class/backlight. The plan is to just disable the sysfs interface and keep the existing backlight-device internal kernel abstraction as is, since some abstraction for (non GPU native) backlight devices will be necessary regardless. An alternative to disabling the sysfs class entirely, would be to allow setting it to read-only through Kconfig. What scale to use for the drm_connector bl_brightness property? === The tricky part of this plan is phase 2 and then esp. defining what the new brightness properties will look like and how they will work. The biggest challenge here is to decide on a fixed scale for the main brightness property, say 0-65535, using scaling where the actual hw scale is different, or if this should simply be a 1:1 mirror of the current backlight interface, with the actual hw scale / brightness_max value exposed as a drm_connector property. 1:1 advantages / 0-65535 disadvantages - Userspace will likely move over to the connector-props quite slowly and we can expect various userspace bits, esp. also custom user scripts, to keep using the old uAPI for a long time. Using the 2 APIs are intermixed is fine when using a 1:1 brightness scale mapping. But if we end up doing a scaling round-trip all the time then eventually the brightness is going do drift. This can even happen if the user never changes the brightness when userspace saves it over suspend/resume or reboots. - Almost all laptops have brightness up/down hotkeys. E.g GNOME decides on a step size for the hotkeys by doing min(brightness_max/20, 1). Some of
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi Simon, On 4/7/22 18:51, Simon Ser wrote: > Very nice plan! Big +1 for the overall approach. Thanks. > On Thursday, April 7th, 2022 at 17:38, Hans de Goede > wrote: > >> The drm_connector brightness properties >> === >> >> bl_brightness: rw 0-int32_max property controlling the brightness setting >> of the connected display. The actual maximum of this will be less then >> int32_max and is given in bl_brightness_max. > > Do we need to split this up into two props for sw/hw state? The privacy screen > stuff needed this, but you're pretty familiar with that. :) Luckily that won't be necessary, since the privacy-screen is a security feature the firmware/embedded-controller may refuse our requests (may temporarily lock-out changes) and/or may make changes without us requesting them itself. Neither is really the case with the brightness setting of displays. >> bl_brightness_max: ro 0-int32_max property giving the actual maximum >> of the display's brightness setting. This will report 0 when brightness >> control is not available (yet). > > I don't think we actually need that one. Integer KMS props all have a > range which can be fetched via drmModeGetProperty. The max can be > exposed via this range. Example with the existing alpha prop: > > "alpha": range [0, UINT16_MAX] = 65535 Right, I already knew that, which is why I explicitly added a range to the props already. The problem is that the range must be set before registering the connector and when the backlight driver only shows up (much) later during boot then we don't know the range when registering the connector. I guess we could "patch-up" the range later. But AFAIK that would be a bit of abuse of the property API as the range is intended to never change, not even after hotplug uevents. At least atm there is no infra in the kernel to change the range later. Which is why I added an explicit bl_brightness_max property of which the value gives the actual effective maximum of the brightness. I did consider using the range for this and updating it on the fly I think nothing is really preventing us from doing so, but it very much feels like abusing the generic properties API. >> bl_brightness_0_is_min_brightness: ro, boolean >> When this is set to true then it is safe to set brightness to 0 >> without worrying that this completely turns the backlight off causing >> the screen to become unreadable. When this is false setting brightness >> to 0 may turn the backlight off, but this is not guaranteed. >> This will e.g. be true when directly driving a PWM and the video-BIOS >> has provided a minimum (non 0) duty-cycle below which the driver will >> never go. > > Hm. It's quite unfortunate that it's impossible to have strong guarantees > here. > > Is there any way we can avoid this prop? Not really, the problem is that we really don't know if 0 is off or min-brightness. In the given example where we actually never go down to a duty-cycle of 0% because the video BIOS tables tell us not to, we can be certain that setting the brightness prop to 0 will not turn of the backlight, since we then set the duty-cycle to the VBT provided minimum. Note the intend here is to only set the boolean to true if the VBT provided minimum is _not_ 0, 0 just means the vendor did not bother to provide a minimum. Currently e.g. GNOME never goes lower then something like 5% of brightness_max to avoid accidentally turning the screen off. Turning the screen off is quite bad to do on e.g. tablets where the GUI is the only way to undo the brightness change and now the user can no longer see the GUI. The idea behind this boolean is to give e.g. GNOME a way to know that it is safe to go down to 0% and for it to use the entire range. > For instance if we can guarantee that the min level won't turn the screen > completely off we could make the range start from 1 instead of 0. > Or allow -1 to mean "minimum value, maybe completely off". Right, the problem is we really don't know and when the range is e.g. 0-65535 then something like 1 will almost always still just turn the screen completely off. There will be a value of say like 150 or some such which is then the actual minimum value to still get the backlight to light up at all. The problem is we have no clue what the actual minimum is. And if the PWM output does not directly drive the LEDs but is used as an input for some LED backlight driver chip, that chip itself may have a lookup table (which may also take care of doing perceived brightness mapping) and may guarantee a minimum backlight even when given a 0% duty cycle PWM signal... This prop is sort of orthogonal to the generic change to drm_connector props, so we could also do this late
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi Daniel, On 4/8/22 10:07, Daniel Vetter wrote: > On Thu, Apr 07, 2022 at 05:05:52PM -0400, Alex Deucher wrote: >> On Thu, Apr 7, 2022 at 1:43 PM Hans de Goede wrote: >>> >>> Hi Simon, >>> >>> On 4/7/22 18:51, Simon Ser wrote: >>>> Very nice plan! Big +1 for the overall approach. >>> >>> Thanks. >>> >>>> On Thursday, April 7th, 2022 at 17:38, Hans de Goede >>>> wrote: >>>> >>>>> The drm_connector brightness properties >>>>> === >>>>> >>>>> bl_brightness: rw 0-int32_max property controlling the brightness setting >>>>> of the connected display. The actual maximum of this will be less then >>>>> int32_max and is given in bl_brightness_max. >>>> >>>> Do we need to split this up into two props for sw/hw state? The privacy >>>> screen >>>> stuff needed this, but you're pretty familiar with that. :) >>> >>> Luckily that won't be necessary, since the privacy-screen is a security >>> feature the firmware/embedded-controller may refuse our requests >>> (may temporarily lock-out changes) and/or may make changes without >>> us requesting them itself. Neither is really the case with the >>> brightness setting of displays. >>> >>>>> bl_brightness_max: ro 0-int32_max property giving the actual maximum >>>>> of the display's brightness setting. This will report 0 when brightness >>>>> control is not available (yet). >>>> >>>> I don't think we actually need that one. Integer KMS props all have a >>>> range which can be fetched via drmModeGetProperty. The max can be >>>> exposed via this range. Example with the existing alpha prop: >>>> >>>> "alpha": range [0, UINT16_MAX] = 65535 >>> >>> Right, I already knew that, which is why I explicitly added a range >>> to the props already. The problem is that the range must be set >>> before registering the connector and when the backlight driver >>> only shows up (much) later during boot then we don't know the >>> range when registering the connector. I guess we could "patch-up" >>> the range later. But AFAIK that would be a bit of abuse of the >>> property API as the range is intended to never change, not >>> even after hotplug uevents. At least atm there is no infra >>> in the kernel to change the range later. >>> >>> Which is why I added an explicit bl_brightness_max property >>> of which the value gives the actual effective maximum of the >>> brightness. > > Uh ... I'm not a huge fan tbh. The thing is, if we allow hotplugging > brightness control later on then we just perpetuate the nonsense we have > right now, forever. > > Imo we should support two kinds of drivers: > > - drivers which are non-crap, and make sure their backlight driver is > loaded before they register the drm_device (or at least the > drm_connector). For those we want the drm_connector->backlight pointer > to bit static over the lifetime of the connector, and then we can also > set up the brightness range correctly. The only problem is that outside of device-tree platforms where we can have a backlight link in a devicetree display-connector node, there are no non crap devices and thus no non crap drivers. > - funny drivers which implement the glorious fallback dance which > libbacklight implements currently in userspace. Imo for these drivers we > should have a libbacklight_heuristics_backlight, which normalizes or > whatever, and is also ways there. And then internally handles the > fallback mess to the "right" backlight driver. So this will be pretty much all of them including i915 and nouveau. My first thoughts where the same as yours and we can mostly guarantee that the drm_connector->backlight pointer is static over lifetime of the connector. But the problem is with the backlight device-s provided by things like the dell-laptop, thinkpad_acpi, etc. drivers which are still necessary / used for backlight control on core2duo era laptops which are still being active used by people. Basically atm the kernel code to determine which backlight-device to use (which assumes a single internal LCD panel) goes like this (1): 1. Check cmdline-override, DMI quirks (and return their value if set) 2. If ACPI video extensions are not supported then expect a backlight device of the dell-laptop, thinkpad_acpi, etc. type, and use that. 3. If the ACPI tables have been written for
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi, On 4/8/22 11:58, Hans de Goede wrote: > Hi Daniel, > > On 4/8/22 10:07, Daniel Vetter wrote: >> On Thu, Apr 07, 2022 at 05:05:52PM -0400, Alex Deucher wrote: >>> On Thu, Apr 7, 2022 at 1:43 PM Hans de Goede wrote: >>>> >>>> Hi Simon, >>>> >>>> On 4/7/22 18:51, Simon Ser wrote: >>>>> Very nice plan! Big +1 for the overall approach. >>>> >>>> Thanks. >>>> >>>>> On Thursday, April 7th, 2022 at 17:38, Hans de Goede >>>>> wrote: >>>>> >>>>>> The drm_connector brightness properties >>>>>> === >>>>>> >>>>>> bl_brightness: rw 0-int32_max property controlling the brightness setting >>>>>> of the connected display. The actual maximum of this will be less then >>>>>> int32_max and is given in bl_brightness_max. >>>>> >>>>> Do we need to split this up into two props for sw/hw state? The privacy >>>>> screen >>>>> stuff needed this, but you're pretty familiar with that. :) >>>> >>>> Luckily that won't be necessary, since the privacy-screen is a security >>>> feature the firmware/embedded-controller may refuse our requests >>>> (may temporarily lock-out changes) and/or may make changes without >>>> us requesting them itself. Neither is really the case with the >>>> brightness setting of displays. >>>> >>>>>> bl_brightness_max: ro 0-int32_max property giving the actual maximum >>>>>> of the display's brightness setting. This will report 0 when brightness >>>>>> control is not available (yet). >>>>> >>>>> I don't think we actually need that one. Integer KMS props all have a >>>>> range which can be fetched via drmModeGetProperty. The max can be >>>>> exposed via this range. Example with the existing alpha prop: >>>>> >>>>> "alpha": range [0, UINT16_MAX] = 65535 >>>> >>>> Right, I already knew that, which is why I explicitly added a range >>>> to the props already. The problem is that the range must be set >>>> before registering the connector and when the backlight driver >>>> only shows up (much) later during boot then we don't know the >>>> range when registering the connector. I guess we could "patch-up" >>>> the range later. But AFAIK that would be a bit of abuse of the >>>> property API as the range is intended to never change, not >>>> even after hotplug uevents. At least atm there is no infra >>>> in the kernel to change the range later. >>>> >>>> Which is why I added an explicit bl_brightness_max property >>>> of which the value gives the actual effective maximum of the >>>> brightness. >> >> Uh ... I'm not a huge fan tbh. The thing is, if we allow hotplugging >> brightness control later on then we just perpetuate the nonsense we have >> right now, forever. >> >> Imo we should support two kinds of drivers: >> >> - drivers which are non-crap, and make sure their backlight driver is >> loaded before they register the drm_device (or at least the >> drm_connector). For those we want the drm_connector->backlight pointer >> to bit static over the lifetime of the connector, and then we can also >> set up the brightness range correctly. > > The only problem is that outside of device-tree platforms where > we can have a backlight link in a devicetree display-connector node, > there are no non crap devices and thus no non crap drivers. > >> - funny drivers which implement the glorious fallback dance which >> libbacklight implements currently in userspace. Imo for these drivers we >> should have a libbacklight_heuristics_backlight, which normalizes or >> whatever, and is also ways there. And then internally handles the >> fallback mess to the "right" backlight driver. > > So this will be pretty much all of them including i915 and nouveau. > > My first thoughts where the same as yours and we can mostly guarantee > that the drm_connector->backlight pointer is static over lifetime of > the connector. But the problem is with the backlight device-s provided > by things like the dell-laptop, thinkpad_acpi, etc. drivers which are > still necessary / used for backlight control on core2duo era laptops > which are still being active used by
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi, On 4/8/22 11:58, Hans de Goede wrote: > Hi Daniel, > > On 4/8/22 10:07, Daniel Vetter wrote: >> On Thu, Apr 07, 2022 at 05:05:52PM -0400, Alex Deucher wrote: >>> On Thu, Apr 7, 2022 at 1:43 PM Hans de Goede wrote: >>>> >>>> Hi Simon, >>>> >>>> On 4/7/22 18:51, Simon Ser wrote: >>>>> Very nice plan! Big +1 for the overall approach. >>>> >>>> Thanks. >>>> >>>>> On Thursday, April 7th, 2022 at 17:38, Hans de Goede >>>>> wrote: >>>>> >>>>>> The drm_connector brightness properties >>>>>> === >>>>>> >>>>>> bl_brightness: rw 0-int32_max property controlling the brightness setting >>>>>> of the connected display. The actual maximum of this will be less then >>>>>> int32_max and is given in bl_brightness_max. >>>>> >>>>> Do we need to split this up into two props for sw/hw state? The privacy >>>>> screen >>>>> stuff needed this, but you're pretty familiar with that. :) >>>> >>>> Luckily that won't be necessary, since the privacy-screen is a security >>>> feature the firmware/embedded-controller may refuse our requests >>>> (may temporarily lock-out changes) and/or may make changes without >>>> us requesting them itself. Neither is really the case with the >>>> brightness setting of displays. >>>> >>>>>> bl_brightness_max: ro 0-int32_max property giving the actual maximum >>>>>> of the display's brightness setting. This will report 0 when brightness >>>>>> control is not available (yet). >>>>> >>>>> I don't think we actually need that one. Integer KMS props all have a >>>>> range which can be fetched via drmModeGetProperty. The max can be >>>>> exposed via this range. Example with the existing alpha prop: >>>>> >>>>> "alpha": range [0, UINT16_MAX] = 65535 >>>> >>>> Right, I already knew that, which is why I explicitly added a range >>>> to the props already. The problem is that the range must be set >>>> before registering the connector and when the backlight driver >>>> only shows up (much) later during boot then we don't know the >>>> range when registering the connector. I guess we could "patch-up" >>>> the range later. But AFAIK that would be a bit of abuse of the >>>> property API as the range is intended to never change, not >>>> even after hotplug uevents. At least atm there is no infra >>>> in the kernel to change the range later. >>>> >>>> Which is why I added an explicit bl_brightness_max property >>>> of which the value gives the actual effective maximum of the >>>> brightness. >> >> Uh ... I'm not a huge fan tbh. The thing is, if we allow hotplugging >> brightness control later on then we just perpetuate the nonsense we have >> right now, forever. >> >> Imo we should support two kinds of drivers: >> >> - drivers which are non-crap, and make sure their backlight driver is >> loaded before they register the drm_device (or at least the >> drm_connector). For those we want the drm_connector->backlight pointer >> to bit static over the lifetime of the connector, and then we can also >> set up the brightness range correctly. > > The only problem is that outside of device-tree platforms where > we can have a backlight link in a devicetree display-connector node, > there are no non crap devices and thus no non crap drivers. > >> - funny drivers which implement the glorious fallback dance which >> libbacklight implements currently in userspace. Imo for these drivers we >> should have a libbacklight_heuristics_backlight, which normalizes or >> whatever, and is also ways there. And then internally handles the >> fallback mess to the "right" backlight driver. > > So this will be pretty much all of them including i915 and nouveau. > > My first thoughts where the same as yours and we can mostly guarantee > that the drm_connector->backlight pointer is static over lifetime of > the connector. But the problem is with the backlight device-s provided > by things like the dell-laptop, thinkpad_acpi, etc. drivers which are > still necessary / used for backlight control on core2duo era laptops > which are still being active used by
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi, On 4/8/22 12:16, Simon Ser wrote: > Would it be an option to only support the KMS prop for Good devices, > and continue using the suboptimal existing sysfs API for Bad devices? > > (I'm just throwing ideas around to see what sticks, feel free to ignore.) Currently suid-root or pkexec helpers are used to deal with the /sys/class/backlight requires root rights issue. I really want to be able to disable these helpers at build time in e.g. GNOME once the new properties are supported in GNOME. So that distros with a new enough kernel can reduce their attack surface this way. Regards, Hans
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi, On 4/8/22 16:08, Alex Deucher wrote: > On Fri, Apr 8, 2022 at 4:07 AM Daniel Vetter wrote: >> >> On Thu, Apr 07, 2022 at 05:05:52PM -0400, Alex Deucher wrote: >>> On Thu, Apr 7, 2022 at 1:43 PM Hans de Goede wrote: >>>> >>>> Hi Simon, >>>> >>>> On 4/7/22 18:51, Simon Ser wrote: >>>>> Very nice plan! Big +1 for the overall approach. >>>> >>>> Thanks. >>>> >>>>> On Thursday, April 7th, 2022 at 17:38, Hans de Goede >>>>> wrote: >>>>> >>>>>> The drm_connector brightness properties >>>>>> === >>>>>> >>>>>> bl_brightness: rw 0-int32_max property controlling the brightness setting >>>>>> of the connected display. The actual maximum of this will be less then >>>>>> int32_max and is given in bl_brightness_max. >>>>> >>>>> Do we need to split this up into two props for sw/hw state? The privacy >>>>> screen >>>>> stuff needed this, but you're pretty familiar with that. :) >>>> >>>> Luckily that won't be necessary, since the privacy-screen is a security >>>> feature the firmware/embedded-controller may refuse our requests >>>> (may temporarily lock-out changes) and/or may make changes without >>>> us requesting them itself. Neither is really the case with the >>>> brightness setting of displays. >>>> >>>>>> bl_brightness_max: ro 0-int32_max property giving the actual maximum >>>>>> of the display's brightness setting. This will report 0 when brightness >>>>>> control is not available (yet). >>>>> >>>>> I don't think we actually need that one. Integer KMS props all have a >>>>> range which can be fetched via drmModeGetProperty. The max can be >>>>> exposed via this range. Example with the existing alpha prop: >>>>> >>>>> "alpha": range [0, UINT16_MAX] = 65535 >>>> >>>> Right, I already knew that, which is why I explicitly added a range >>>> to the props already. The problem is that the range must be set >>>> before registering the connector and when the backlight driver >>>> only shows up (much) later during boot then we don't know the >>>> range when registering the connector. I guess we could "patch-up" >>>> the range later. But AFAIK that would be a bit of abuse of the >>>> property API as the range is intended to never change, not >>>> even after hotplug uevents. At least atm there is no infra >>>> in the kernel to change the range later. >>>> >>>> Which is why I added an explicit bl_brightness_max property >>>> of which the value gives the actual effective maximum of the >>>> brightness. >> >> Uh ... I'm not a huge fan tbh. The thing is, if we allow hotplugging >> brightness control later on then we just perpetuate the nonsense we have >> right now, forever. >> >> Imo we should support two kinds of drivers: >> >> - drivers which are non-crap, and make sure their backlight driver is >> loaded before they register the drm_device (or at least the >> drm_connector). For those we want the drm_connector->backlight pointer >> to bit static over the lifetime of the connector, and then we can also >> set up the brightness range correctly. >> >> - funny drivers which implement the glorious fallback dance which >> libbacklight implements currently in userspace. Imo for these drivers we >> should have a libbacklight_heuristics_backlight, which normalizes or >> whatever, and is also ways there. And then internally handles the >> fallback mess to the "right" backlight driver. >> >> We might have some gaps on acpi systems to make sure the drm driver can >> wait for the backlight driver to show up, but that's about it. >> >> Hotplugging random pieces later on is really not how drivers work nowadays >> with deferred probe and component framework and all that. >> >>>> I did consider using the range for this and updating it >>>> on the fly I think nothing is really preventing us from >>>> doing so, but it very much feels like abusing the generic >>>> properties API. >>>> >>>>>> bl_brightness_0_is_min_brightness: ro, boolean >>>>>> When thi
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi Simon, On 4/8/22 10:22, Simon Ser wrote: > Hi Hans, > > Thanks for your details replies! > > On Thursday, April 7th, 2022 at 19:43, Hans de Goede > wrote: > >>> On Thursday, April 7th, 2022 at 17:38, Hans de Goede >>> wrote: >>> >>>> The drm_connector brightness properties >>>> === >>>> >>>> bl_brightness: rw 0-int32_max property controlling the brightness setting >>>> of the connected display. The actual maximum of this will be less then >>>> int32_max and is given in bl_brightness_max. >>> >>> Do we need to split this up into two props for sw/hw state? The privacy >>> screen >>> stuff needed this, but you're pretty familiar with that. :) >> >> Luckily that won't be necessary, since the privacy-screen is a security >> feature the firmware/embedded-controller may refuse our requests >> (may temporarily lock-out changes) and/or may make changes without >> us requesting them itself. Neither is really the case with the >> brightness setting of displays. > > Cool, makes sense to me! > >>>> bl_brightness_max: ro 0-int32_max property giving the actual maximum >>>> of the display's brightness setting. This will report 0 when brightness >>>> control is not available (yet). >>> >>> I don't think we actually need that one. Integer KMS props all have a >>> range which can be fetched via drmModeGetProperty. The max can be >>> exposed via this range. Example with the existing alpha prop: >>> >>> "alpha": range [0, UINT16_MAX] = 65535 >> >> Right, I already knew that, which is why I explicitly added a range >> to the props already. The problem is that the range must be set >> before registering the connector and when the backlight driver >> only shows up (much) later during boot then we don't know the >> range when registering the connector. I guess we could "patch-up" >> the range later. But AFAIK that would be a bit of abuse of the >> property API as the range is intended to never change, not >> even after hotplug uevents. At least atm there is no infra >> in the kernel to change the range later. >> >> Which is why I added an explicit bl_brightness_max property >> of which the value gives the actual effective maximum of the >> brightness. >> >> I did consider using the range for this and updating it >> on the fly I think nothing is really preventing us from >> doing so, but it very much feels like abusing the generic >> properties API. > > Since this is new uAPI there's no concern about backwards compat here. So it's > pretty much a matter of how we want the uAPI to look like. I was suggesting > using a range because it's self-describing, but maybe it's an abuse. > > Daniel Vetter, what do you think? If a property's range is going to be updated > on the fly, should we go for it, or should we use a separate prop to describe > the max value? > >>>> bl_brightness_0_is_min_brightness: ro, boolean >>>> When this is set to true then it is safe to set brightness to 0 >>>> without worrying that this completely turns the backlight off causing >>>> the screen to become unreadable. When this is false setting brightness >>>> to 0 may turn the backlight off, but this is not guaranteed. >>>> This will e.g. be true when directly driving a PWM and the video-BIOS >>>> has provided a minimum (non 0) duty-cycle below which the driver will >>>> never go. >>> >>> Hm. It's quite unfortunate that it's impossible to have strong guarantees >>> here. >>> >>> Is there any way we can avoid this prop? >> >> Not really, the problem is that we really don't know if 0 is off >> or min-brightness. In the given example where we actually never go >> down to a duty-cycle of 0% because the video BIOS tables tell us >> not to, we can be certain that setting the brightness prop to 0 >> will not turn of the backlight, since we then set the duty-cycle >> to the VBT provided minimum. Note the intend here is to only set >> the boolean to true if the VBT provided minimum is _not_ 0, 0 >> just means the vendor did not bother to provide a minimum. >> >> Currently e.g. GNOME never goes lower then something like 5% >> of brightness_max to avoid accidentally turning the screen off. >> >> Turning the screen off is quite bad to do on e.g. tablets where >> the GUI is the only w
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi, On 4/8/22 17:11, Alex Deucher wrote: > On Fri, Apr 8, 2022 at 10:56 AM Hans de Goede wrote: >> >> Hi, >> >> On 4/8/22 16:08, Alex Deucher wrote: >>> On Fri, Apr 8, 2022 at 4:07 AM Daniel Vetter wrote: >>>> >>>> On Thu, Apr 07, 2022 at 05:05:52PM -0400, Alex Deucher wrote: >>>>> On Thu, Apr 7, 2022 at 1:43 PM Hans de Goede wrote: >>>>>> >>>>>> Hi Simon, >>>>>> >>>>>> On 4/7/22 18:51, Simon Ser wrote: >>>>>>> Very nice plan! Big +1 for the overall approach. >>>>>> >>>>>> Thanks. >>>>>> >>>>>>> On Thursday, April 7th, 2022 at 17:38, Hans de Goede >>>>>>> wrote: >>>>>>> >>>>>>>> The drm_connector brightness properties >>>>>>>> === >>>>>>>> >>>>>>>> bl_brightness: rw 0-int32_max property controlling the brightness >>>>>>>> setting >>>>>>>> of the connected display. The actual maximum of this will be less then >>>>>>>> int32_max and is given in bl_brightness_max. >>>>>>> >>>>>>> Do we need to split this up into two props for sw/hw state? The privacy >>>>>>> screen >>>>>>> stuff needed this, but you're pretty familiar with that. :) >>>>>> >>>>>> Luckily that won't be necessary, since the privacy-screen is a security >>>>>> feature the firmware/embedded-controller may refuse our requests >>>>>> (may temporarily lock-out changes) and/or may make changes without >>>>>> us requesting them itself. Neither is really the case with the >>>>>> brightness setting of displays. >>>>>> >>>>>>>> bl_brightness_max: ro 0-int32_max property giving the actual maximum >>>>>>>> of the display's brightness setting. This will report 0 when brightness >>>>>>>> control is not available (yet). >>>>>>> >>>>>>> I don't think we actually need that one. Integer KMS props all have a >>>>>>> range which can be fetched via drmModeGetProperty. The max can be >>>>>>> exposed via this range. Example with the existing alpha prop: >>>>>>> >>>>>>> "alpha": range [0, UINT16_MAX] = 65535 >>>>>> >>>>>> Right, I already knew that, which is why I explicitly added a range >>>>>> to the props already. The problem is that the range must be set >>>>>> before registering the connector and when the backlight driver >>>>>> only shows up (much) later during boot then we don't know the >>>>>> range when registering the connector. I guess we could "patch-up" >>>>>> the range later. But AFAIK that would be a bit of abuse of the >>>>>> property API as the range is intended to never change, not >>>>>> even after hotplug uevents. At least atm there is no infra >>>>>> in the kernel to change the range later. >>>>>> >>>>>> Which is why I added an explicit bl_brightness_max property >>>>>> of which the value gives the actual effective maximum of the >>>>>> brightness. >>>> >>>> Uh ... I'm not a huge fan tbh. The thing is, if we allow hotplugging >>>> brightness control later on then we just perpetuate the nonsense we have >>>> right now, forever. >>>> >>>> Imo we should support two kinds of drivers: >>>> >>>> - drivers which are non-crap, and make sure their backlight driver is >>>> loaded before they register the drm_device (or at least the >>>> drm_connector). For those we want the drm_connector->backlight pointer >>>> to bit static over the lifetime of the connector, and then we can also >>>> set up the brightness range correctly. >>>> >>>> - funny drivers which implement the glorious fallback dance which >>>> libbacklight implements currently in userspace. Imo for these drivers we >>>> should have a libbacklight_heuristics_backlight, which normalizes or >>>> whatever, and is also ways there. And then internally handles the >>>> fallback me
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi, On 4/7/22 20:58, Carsten Haitzler wrote: > On Thu, 7 Apr 2022 17:38:59 +0200 Hans de Goede said: > > Below you covered our usual /sys/class/backlight device friends... what about > DDC monitor controls? These function similarly but just remotely control a > screen via I2C and also suffer from the same problems of "need root" and "have > to do some fun in mapping them to a given screen". Right, supporting this definitely is part of the plan, this is why my original email had the following footnote: 1) The need to be able to map the backlight device to a specific display has become clear once more with the recent proposal to add DDCDI support: https://lore.kernel.org/lkml/20220403230850.2986-1-yusisameri...@gmail.com/ :) > Otherwise I welcome this de-uglification of the backlight device and putting > it > together with the drm device that controls that monitor. Thx. > Just to make life more fun ... DDC does much more than backlight controls. It > can control essentially anything that is in the OSD for your monitor > (contrast, > brightness, backlight, sharpness, color temperatures, input to display (DP vs > HDMI vs DVI etc.), an for extra fun points can even tel you the current > rotation state of your monitor. All of these do make sense to live as drm > connector properties too. Perhaps not a first iteration but something to > consider in this design. One thing which I do want to take into account is to make sure that userspace can still do DDC/CI for all the other features. I know there is demand for adding brightness control over DDC/CI. I'm not aware of any concrete use-cases for the other DDC/CI settings. Also DDC/CI can include some pretty crazy stuff like setting up picture in picture using 2 different inputs of the monitor, which is very vendor specific. So all in all I think that we should just punt most of the DDC/CI stuff to userspace. With that said I agree that it would be good to think about possibly other some of the other settings in case some use-case does show up for those. The biggest problem with doing this is coming up with some prefix to namespace things. I've gone with bl_brightness to avoid a conflict with the existing TV specific properties which have plain "brightness" put if we want to e.g. also add DDC/CI contrast as a property later then it might be good to come up with another more generic prefix which can be shared between laptop-panel-brightness, DDC/CI-brightness and DDC/CI-contrast ... ? So any suggestions for a better prefix? Regards, Hans > >> As discussed already several times in the past: >> https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/ >> >> https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.intel.com/ >> >> The current userspace API for brightness control offered by >> /sys/class/backlight devices has various issues, the biggest 2 being: >> >> 1. There is no way to map the backlight device to a specific >>display-output / panel (1) >> 2. Controlling the brightness requires root-rights requiring >>desktop-environments to use suid-root helpers for this. >> >> As already discussed on various conference's hallway tracks >> and as has been proposed on the dri-devel list once before (2), >> it seems that there is consensus that the best way to to solve these >> 2 issues is to add support for controlling a video-output's brightness >> through properties on the drm_connector. >> >> This RFC outlines my plan to try and actually implement this, >> which has 3 phases: >> >> >> Phase 1: Stop registering multiple /sys/class/backlight devs for a single >> display >> = >> >> On x86 there can be multiple firmware + direct-hw-access methods >> for controlling the backlight and in some cases the kernel registers >> multiple backlight-devices for a single internal laptop LCD panel: >> >> a) i915 and nouveau unconditionally register their "native" backlight dev >>even on devices where /sys/class/backlight/acpi_video0 must be used >>to control the backlight, relying on userspace to prefer the "firmware" >>acpi_video0 device over "native" devices. >> b) amdgpu and nouveau rely on the acpi_video driver initializing before >>them, which currently causes /sys/class/backlight/acpi_video0 to usually >>show up and then they register their own native backlight driver after >>which the drivers/acpi/video_detect.c code unregisters the acpi_video0 >>device. This means that userspace briefly sees 2 devices and the >>disappearing of acp
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi Simon, On 4/8/22 10:22, Simon Ser wrote: > Hi Hans, > > Thanks for your details replies! > > On Thursday, April 7th, 2022 at 19:43, Hans de Goede > wrote: > >>> On Thursday, April 7th, 2022 at 17:38, Hans de Goede >>> wrote: >>> >>>> The drm_connector brightness properties >>>> === >>>> >>>> bl_brightness: rw 0-int32_max property controlling the brightness setting >>>> of the connected display. The actual maximum of this will be less then >>>> int32_max and is given in bl_brightness_max. >>> >>> Do we need to split this up into two props for sw/hw state? The privacy >>> screen >>> stuff needed this, but you're pretty familiar with that. :) >> >> Luckily that won't be necessary, since the privacy-screen is a security >> feature the firmware/embedded-controller may refuse our requests >> (may temporarily lock-out changes) and/or may make changes without >> us requesting them itself. Neither is really the case with the >> brightness setting of displays. > > Cool, makes sense to me! > >>>> bl_brightness_max: ro 0-int32_max property giving the actual maximum >>>> of the display's brightness setting. This will report 0 when brightness >>>> control is not available (yet). >>> >>> I don't think we actually need that one. Integer KMS props all have a >>> range which can be fetched via drmModeGetProperty. The max can be >>> exposed via this range. Example with the existing alpha prop: >>> >>> "alpha": range [0, UINT16_MAX] = 65535 >> >> Right, I already knew that, which is why I explicitly added a range >> to the props already. The problem is that the range must be set >> before registering the connector and when the backlight driver >> only shows up (much) later during boot then we don't know the >> range when registering the connector. I guess we could "patch-up" >> the range later. But AFAIK that would be a bit of abuse of the >> property API as the range is intended to never change, not >> even after hotplug uevents. At least atm there is no infra >> in the kernel to change the range later. >> >> Which is why I added an explicit bl_brightness_max property >> of which the value gives the actual effective maximum of the >> brightness. >> >> I did consider using the range for this and updating it >> on the fly I think nothing is really preventing us from >> doing so, but it very much feels like abusing the generic >> properties API. > > Since this is new uAPI there's no concern about backwards compat here. So it's > pretty much a matter of how we want the uAPI to look like. I was suggesting > using a range because it's self-describing, but maybe it's an abuse. > > Daniel Vetter, what do you think? If a property's range is going to be updated > on the fly, should we go for it, or should we use a separate prop to describe > the max value? Daniel, as explained in my replies to you, I do believe that dynamically updating the range is unavoidable. Especially once we also add support for setting a monitor's brightness over DDC/CI. Since external monitors (with/without DDC/CI support) can come and go and since properties cannot be added/removed after connector registration, we need a way to let userspace know if brightness control is actually available or not and what the range is. We can use a max value of 0 for not available and other values for the actual range, which I believe is a sane API. But the question from Simon then still remains, do we update the range of the property on the fly, followed by a connector hotplug uevent; or do we use a separate brightness_max property for this? Note that as Rasterman indicated that with DDC/CI support we could also offer other properties (for which I see no reason atm) and if we do say also add a contrast property over DDC/CI then if we go the separate brightness_max route that would mean adding 2 props for each setting which we want to support. Regards, Hans
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi Pekka, On 4/11/22 13:34, Pekka Paalanen wrote: > On Mon, 11 Apr 2022 12:18:30 +0200 > Hans de Goede wrote: > >> Hi, >> >> On 4/8/22 17:11, Alex Deucher wrote: >>> On Fri, Apr 8, 2022 at 10:56 AM Hans de Goede wrote: >>>> > > ... > >>> So set it to a level we can guarantee can call it 0. If we have the >>> flag we are just punting on the problem in my opinion. >> >> Right this an impossible problem to solve so the intent is indeed >> to punt this to userspace, which IMHO is the best thing we can do >> here. The idea behind the "bl_brightness_0_is_min_brightness: >> ro, boolean" property is to provide a hint to userspace to help >> userspace deal with this (and if userspace ends up using e.g. >> systemd's hwdb for this to avoid unnecessary entries in hwdb). >> >>> The kernel >>> driver would seem to have a better idea what is a valid minimum than >>> some arbitrary userspace application. >> >> If the kernel driver knows the valid minimum then it can make 0 >> actually be that valid minimum as you suggest and it can set the >> hint flag to let userspace know this. OTOH there are many cases >> where the kernel's guess is just as bad as userspace's guess and >> there are too many laptops where this is the case to quirk >> ourselves out of this situation. >> >>> Plus then if we need a >>> workaround for what is the minimum valid brightness, we can fix it one >>> place rather than letting every application try and fix it. >> >> I wish we could solve this in the kernel, but at least on >> laptops with Intel integrated gfx many vendors don't bother >> to put a non 0 value in the minimum duty-cycle field of the >> VBT, so there really is no good way to solve this. >> >> If the userspace folks ever want to do a database for this, >> I would expect them to do something with hwdb + udev which >> can then be shared between the different desktop-environments. > > Hi Hans, > > assuming that it is impossible to reach a reasonable user experience by > having a quirk database in the kernel in order to offer a consistent > definition of bl_brightness=0, then should you not be recommending a > userspace hwdb solution with full steam, rather than adding a hint in > the kernel that might be just enough to have no-one ever bother > investing in a proper solution? The problem is we already lack the manpower for a quirk database, and even if we ever get the manpower then it would still be good to avoid the work necessary to add models to the database where the kernel already knows how things work, see below. As for no-one ever bothering coming up with a full-steam hwdb solution for the cases where the kernel has no idea what bl_brightness=0 means, yes that is likely, but that already is the status quo, the hint will at least allow using the full brightness range on devices where the kernel knows (with certainty) that this is correct. > Re-reading your "bl_brightness_0_is_min_brightness" definition, it > seems to be specified as exposing a certain condition in the system. > When it is true, you imply that userspace can safely use value 0 as min > brightness, but that is assuming the hint is correct. How likely > is the hint incorrect? It should never be incorrect, there are cases when the kernel knows reliably that bl_brightness=0 means minimum brightness (and NOT backlight off). > If the hint can be incorrect, does this hint > actually give anything to userspace, or would userspace still choose to > be safer than sorry and ignore the hint by assuming the worst? If the hint is incorrect then that would be a kernel bug and that should be fixed in the kernel. The whole idea behind the hind is that userspace can absolutely trust it to be correct when set to true (false basically means that the kernel does not know of 0=off or not). > Is this situation much different to the quirk database libinput needs > to work beautifully out of the box? libinput's quirk database is relatively pretty small and a lot of effort is done to fix things in generic ways where possible, to avoid growing it. As a general rule quirks should only be used to handle exceptions to general rules, the problem here is that bl_brightness=0 being backlight off is not a true exception it happens quite often. Which is also why I believe that a hwdb for this is not necessarily a great idea, because maintaining it will be a lot of work. > Should desktop environments offer a couple more "advanced > configuration" knobs for the lowest safe brightness value and the > value-to-perceived brightness mapping to calibrate the familiar > brightness
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi, On 4/13/22 10:32, Daniel Vetter wrote: > On Fri, Apr 08, 2022 at 12:26:24PM +0200, Hans de Goede wrote: >> Hi, >> >> On 4/8/22 12:16, Simon Ser wrote: >>> Would it be an option to only support the KMS prop for Good devices, >>> and continue using the suboptimal existing sysfs API for Bad devices? >>> >>> (I'm just throwing ideas around to see what sticks, feel free to ignore.) >> >> Currently suid-root or pkexec helpers are used to deal with the >> /sys/class/backlight requires root rights issue. I really want to >> be able to disable these helpers at build time in e.g. GNOME once >> the new properties are supported in GNOME. So that distros with >> a new enough kernel can reduce their attack surface this way. > > Yeah but otoh perpetuating a bad interface forever isn't a great idea > either. I think the pragmatic plan here is > - Implement this properly on good devices, i.e. anything new. > - Do some runtime disabling in the pkexec helpers if they detect a modern > system (we should be able to put a proper symlink into the drm sysfs > connector directories, to make this easy to detect). It's not as great > as doing this at compile time, but it's a step. > - Figure out a solution for the old crap. We can't really change anything > with the load ordering for existing systems, so if the hacked-up compat > libbacklight-backlight isn't an option then I guess we need some quirk > list/extracted code which makes i915/nouveau/radeon driver load fail > until the right backlight shows up. And that needs to be behind a > Kconfig to avoid breaking existing systems. > > Inflicting hotplug complications on all userspace (including uevent > handling for this hotpluggable backlight and everything) just because > fixing the old crap systems is work is imo really not a good idea. Much > better if we get to the correct future step-by-step. This assumes that we only use the new brightness properties for laptop internal LCD panels. But what about controlling the brightness of external monitors through DDC/CI? If we do that we need hotplug support for this regardless since external monitors can be hotplugged. As I mentioned in other parts of the thread one of the reasons why I've started looking into this again is because of people being interested in this (1). And even just taking internal LCD panels into account, there are hybrid GPU laptops where the backlight is directly controlled by the GPU (native type backlight driver) connected to the panel(2), if we runtime switch the GPU attached to the panel there, then we will get an actual hotplug of the LCD connector and I'm not sure if we can always detect the maximum value of the brightness on the GPU which is not connected to the panel at boot. So in this case we need userspace to support re-reading the brightness max at a hotplug event regardless. So in all in all I feel that supporting hotplug events is unavoidable here. Regards, Hans 1) Including attempting to do this through the old /sys/class/backlight interface which IMHO would be quite bad to do: https://lore.kernel.org/lkml/20220403230850.2986-1-yusisameri...@gmail.com/ 2) E.g. gnome-settings-daemon has special code to detect which native backlight interface to use if there are 2 native backlight devices on a single laptop, see: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/blob/master/plugins/power/gsd-backlight.c#L95
[RFC] drm/kms: Stop registering multiple /sys/class/backlight devs for a single display
Hi All, As discussed in the "[RFC] drm/kms: control display brightness through drm_connector properties" thread, step 1 of the plan is to stop registering multiple /sys/class/backlight devs for a single display. On x86 there can be multiple firmware + direct-hw-access methods for controlling the backlight and in some cases the kernel registers multiple backlight-devices for a single internal laptop LCD panel, 2 common scenarios where this happens are: 1) i915 and nouveau unconditionally register their "native" backlight dev even on devices where /sys/class/backlight/acpi_video0 must be used to control the backlight, relying on userspace to prefer the "firmware" acpi_video0 device over "native" devices. 2) amdgpu and nouveau rely on the acpi_video driver initializing before them, which currently causes /sys/class/backlight/acpi_video0 to usually show up and then they register their own native backlight driver after which the drivers/acpi/video_detect.c code unregisters the acpi_video0 device. This means that userspace briefly sees 2 devices and the disappearing of acpi_video0 after a brief time confuses the systemd backlight level save/restore code, see e.g.: https://bbs.archlinux.org/viewtopic.php?id=269920 Fixing kms driver unconditionally register their "native" backlight dev === The plan for fixing 1) is to add a "bool native_backlight_available" parameter to acpi_video_get_backlight_type(), which will be set to true when called by e.g. the i915 code to check if it should register its native backlight-device. This way acpi_video_get_backlight_type() will know that a native backlight-device is (will be) available even though it has not been registered yet and then it can return acpi_backlight_native if that is the best option. And then the i915 code will only actually register its native backlight when acpi_backlight_native gets returned, thus hiding it in scenario 1. Fixing acpi_video0 getting registered for a brief time == ATM the acpi_video code will delay parsing the ACPI video extensions when an i915 opregion is present and will immediately parse these + register an acpi_video backlight device on laptops without Intel integrated graphics. On laptops with i915 gfx the i915 driver calls acpi_video_register() near the end of its probe() function when things are ready for the acpi_video code to run, avoiding scenario 2. Where as on systems without i915 gfx acpi_video's module_init() immediately calls acpi_video_register() and then later the ACPI video_detect code calls acpi_video_unregister_backlight() to hide the acpi_video# backlight-device on systems where the native backlight-device should be used. The plan to fix this is to add an acpi_video_register_backlight() and to make acpi_video_register() do all the usual ACPI video extension probing, but have it skip the actual registering of the backlight devices and have drivers explicitly call acpi_video_register() after they have setup their own native backlight-device. This way acpi_video_get_backlight_type() already will know that a native backlight-device is available (and preferred) when acpi_video_register_backlight() runs and the registering of the acpi_video# device will then be skipped, removing it briefly showing up and disappearing again. One problem with this approach is that this relies on the GPU driver to call acpi_video_register_backlight() when it is done. One could argue that this is actually a feature, we have had issues with some desktops where acpi_video falsely registers its backlight (even though there is no internal LCD panel), but this will likely cause issues on some systems (1). So the plan is to queue a delayed work with an 8 second (1) delay from acpi_video_register() and have that register the backlight-device if not done already. Other issues The above only takes native vs acpi_video backlight issues into account, there are also a couple of other scenarios which may lead to multiple backlight-devices getting registered: 1) Apple laptops using the apple_gmux driver 2) Nvidia laptops using the (new) nvidia-wmi-ec-backlight driver 3) drivers/platform/x86 drivers calling acpi_video_set_dmi_backlight_type() to override the normal acpi_video_get_backlight_type() heuristics after the native/acpi_video drivers have already loaded The plan for 1) + 2) is to extend the acpi_backlight_type enum with acpi_backlight_gmux and acpi_backlight_nvidia_wmi_ec values and to add detection of these 2 to acpi_video_get_backlight_type(). The plan for 3) is to move the DMI quirks from drivers/platform/x86 drivers which call acpi_video_set_dmi_backlight_type() in to the existing DMI quirk table in drivers/acpi/video_detect.c, so that they will be available during the first/every call of acpi_video_get_backlight_type() and then remove acpi_video_set_dmi_backlight_t
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi, On 4/27/22 16:26, Daniel Vetter wrote: > On Wed, Apr 27, 2022 at 05:23:22PM +0300, Jani Nikula wrote: >> On Wed, 27 Apr 2022, Daniel Vetter wrote: >>> On Thu, Apr 14, 2022 at 01:24:30PM +0300, Jani Nikula wrote: >>>> On Mon, 11 Apr 2022, Alex Deucher wrote: >>>>> On Mon, Apr 11, 2022 at 6:18 AM Hans de Goede wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> On 4/8/22 17:11, Alex Deucher wrote: >>>>>>> On Fri, Apr 8, 2022 at 10:56 AM Hans de Goede >>>>>>> wrote: >>>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> On 4/8/22 16:08, Alex Deucher wrote: >>>>>>>>> On Fri, Apr 8, 2022 at 4:07 AM Daniel Vetter wrote: >>>>>>>>>> >>>>>>>>>> On Thu, Apr 07, 2022 at 05:05:52PM -0400, Alex Deucher wrote: >>>>>>>>>>> On Thu, Apr 7, 2022 at 1:43 PM Hans de Goede >>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> Hi Simon, >>>>>>>>>>>> >>>>>>>>>>>> On 4/7/22 18:51, Simon Ser wrote: >>>>>>>>>>>>> Very nice plan! Big +1 for the overall approach. >>>>>>>>>>>> >>>>>>>>>>>> Thanks. >>>>>>>>>>>> >>>>>>>>>>>>> On Thursday, April 7th, 2022 at 17:38, Hans de Goede >>>>>>>>>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> The drm_connector brightness properties >>>>>>>>>>>>>> === >>>>>>>>>>>>>> >>>>>>>>>>>>>> bl_brightness: rw 0-int32_max property controlling the >>>>>>>>>>>>>> brightness setting >>>>>>>>>>>>>> of the connected display. The actual maximum of this will be >>>>>>>>>>>>>> less then >>>>>>>>>>>>>> int32_max and is given in bl_brightness_max. >>>>>>>>>>>>> >>>>>>>>>>>>> Do we need to split this up into two props for sw/hw state? The >>>>>>>>>>>>> privacy screen >>>>>>>>>>>>> stuff needed this, but you're pretty familiar with that. :) >>>>>>>>>>>> >>>>>>>>>>>> Luckily that won't be necessary, since the privacy-screen is a >>>>>>>>>>>> security >>>>>>>>>>>> feature the firmware/embedded-controller may refuse our requests >>>>>>>>>>>> (may temporarily lock-out changes) and/or may make changes without >>>>>>>>>>>> us requesting them itself. Neither is really the case with the >>>>>>>>>>>> brightness setting of displays. >>>>>>>>>>>> >>>>>>>>>>>>>> bl_brightness_max: ro 0-int32_max property giving the actual >>>>>>>>>>>>>> maximum >>>>>>>>>>>>>> of the display's brightness setting. This will report 0 when >>>>>>>>>>>>>> brightness >>>>>>>>>>>>>> control is not available (yet). >>>>>>>>>>>>> >>>>>>>>>>>>> I don't think we actually need that one. Integer KMS props all >>>>>>>>>>>>> have a >>>>>>>>>>>>> range which can be fetched via drmModeGetProperty. The max can be >>>>>>>>>>>>> exposed via this range. Example with the existing alpha prop: >>>>>>>>>>>>> >>>>>>>>>>>>> "alpha": range [0, UINT16_MAX] = 65535 >>>>>>>>>>>> >>>>>>>>>>>> Right, I already knew that, which is why I explicitly added a range >>>>>>>>>>>> to t
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi, On 4/14/22 15:10, Jani Nikula wrote: > On Thu, 07 Apr 2022, Hans de Goede wrote: >> As discussed already several times in the past: >> https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/ >> >> https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.intel.com/ >> >> The current userspace API for brightness control offered by >> /sys/class/backlight devices has various issues, the biggest 2 being: >> >> 1. There is no way to map the backlight device to a specific >>display-output / panel (1) >> 2. Controlling the brightness requires root-rights requiring >>desktop-environments to use suid-root helpers for this. >> >> As already discussed on various conference's hallway tracks >> and as has been proposed on the dri-devel list once before (2), >> it seems that there is consensus that the best way to to solve these >> 2 issues is to add support for controlling a video-output's brightness >> through properties on the drm_connector. >> >> This RFC outlines my plan to try and actually implement this, >> which has 3 phases: >> >> >> Phase 1: Stop registering multiple /sys/class/backlight devs for a single >> display >> = >> >> On x86 there can be multiple firmware + direct-hw-access methods >> for controlling the backlight and in some cases the kernel registers >> multiple backlight-devices for a single internal laptop LCD panel: >> >> a) i915 and nouveau unconditionally register their "native" backlight dev >>even on devices where /sys/class/backlight/acpi_video0 must be used >>to control the backlight, relying on userspace to prefer the "firmware" >>acpi_video0 device over "native" devices. >> b) amdgpu and nouveau rely on the acpi_video driver initializing before >>them, which currently causes /sys/class/backlight/acpi_video0 to usually >>show up and then they register their own native backlight driver after >>which the drivers/acpi/video_detect.c code unregisters the acpi_video0 >>device. This means that userspace briefly sees 2 devices and the >>disappearing of acpi_video0 after a brief time confuses the systemd >>backlight level save/restore code, see e.g.: >>https://bbs.archlinux.org/viewtopic.php?id=269920 >> >> I already have a pretty detailed plan to tackle this, which I will >> post in a separate RFC email. I plan to start working on this right >> away, as it will be good to have this fixed regardless. >> >> >> Phase 2: Add drm_connector properties mirroring the matching backlight device >> = >> >> The plan is to add a drm_connector helper function, which optionally takes >> a pointer to the backlight device for the GPU's native backlight device, >> which will then mirror the backlight settings from the backlight device >> in a set of read/write brightness* properties on the connector. >> >> This function can then be called by GPU drivers for the drm_connector for >> the internal panel and it will then take care of everything. When there >> is no native GPU backlight device, or when it should not be used then >> (on x86) the helper will use the acpi_video_get_backlight_type() to >> determine which backlight-device should be used instead and it will find >> + mirror that one. >> >> >> Phase 3: Deprecate /sys/class/backlight uAPI >> >> >> Once most userspace has moved over to using the new drm_connector >> brightness props, a Kconfig option can be added to stop exporting >> the backlight-devices under /sys/class/backlight. The plan is to >> just disable the sysfs interface and keep the existing backlight-device >> internal kernel abstraction as is, since some abstraction for (non GPU >> native) backlight devices will be necessary regardless. >> >> An alternative to disabling the sysfs class entirely, would be >> to allow setting it to read-only through Kconfig. >> >> >> What scale to use for the drm_connector bl_brightness property? >> === >> >> The tricky part of this plan is phase 2 and then esp. defining what the >> new brightness properties will look like and how they will work. >> >> The biggest challenge here is to decide on a fixed scale for the main >> brightness property, say 0-65535, using scal
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi, On 5/18/22 16:23, Jani Nikula wrote: > On Wed, 18 May 2022, Hans de Goede wrote: >> So how about: display_brightness or panel_brightness ? > > This is a prime opportunity to look at all the existing properties, and > come up with a new combination of capitalization, spacing, hyphens, > underscores, etc. to accompany the total lack of unification we > currently have. DisPLay_brIgh7ne55. :p > > I think "display_brightness" is probably fine. Interesting remark about the use of space/_/- I checked drm_connector.c and most properties use all lower case with spaces so to try and be consistent, I'll replace the _ with a space. I guess it is time for me to create a v2 of this proposal. Regards, Hans
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
Hi, On 6/3/22 17:22, Simon Ser wrote: > On Friday, June 3rd, 2022 at 17:17, Zack Rusin wrote: > >> >>> On Jun 3, 2022, at 10:56 AM, Simon Ser wrote: >>> ⚠External Email >>> >>> On Friday, June 3rd, 2022 at 16:38, Zack Rusin wrote: >>> > On Jun 3, 2022, at 10:32 AM, Simon Ser wrote: > > ⚠External Email > > On Friday, June 3rd, 2022 at 16:27, Zack Rusin wrote: > >>> In particular: since the driver will ignore the KMS cursor plane >>> position set by user-space, I don't think it's okay to just expose >>> without opt-in from user-space (e.g. with a DRM_CLIENT_CAP). >>> >>> cc wayland-devel and Pekka for user-space feedback. >> >> I think Thomas expressed our concerns and reasons why those wouldn’t >> work for us back then. Is there something else you’d like to add? > > I disagreed, and I don't understand Thomas' reasoning. > > KMS clients will need an update to work correctly. Adding a new prop > without a cap leaves existing KMS clients broken. Adding a cap allows > both existing KMS clients and updated KMS clients to work correctly. I’m not sure what you mean here. They are broken right now. That’s what we’re fixing. That’s the reason why the virtualized drivers are on deny-lists for all atomic kms. So nothing needs to be updated. If you have a kms client that was using virtualized drivers with atomic kms then mouse clicks never worked correctly. So, yes, clients need to set cursor hotspot if they want mouse to work correctly with virtualized drivers. >>> >>> My proposal was: >>> >>> - Introduce DRM_CLIENT_CAP_CURSOR_PLANE_NO_POSITION (or a better name). Only >>> user-space which supports the hotspot props will enable it. >>> - By default, don't expose a cursor plane, because current user-space >>> doesn't >>> support it (Weston might put a window in the cursor plane, and nobody can >>> report hotspot). >>> - If the KMS client enables the cap, advertise the cursor >>> plane, and make it so the plane doesn't have the CRTC_X/CRTC_Y properties >>> since the driver will pick the position. >>> >>> That way both old and new user-space are fixed. >> >> I don’t think I see how that fixes anything. In particular I don’t see a way >> of fixing the old user space at all. We require hotspot info, old user space >> doesn’t set it because there’s no way of setting it on atomic kms. > > Old atomic user-space is fixed by removing the cursor plane. Then old > atomic user-space will fallback to drawing the cursor itself, e.g. via > OpenGL. That is just a terrible idea, the current situation is that userspace has a hardcoded list of drivers which need hotspots, so it uses the old non-atomic APIs on that "hw" since the atomic APIs don't support hotspots. The downsides I see to your proposal are: 1. Falling back to sw cursor rendering instead of using the old APIs would be a pretty significant regression in user experience. I know that in theory sw rendering can be made to not flicker, but in practice it tends to be a flickery mess. 2. It does not help anything since userspace is still hardcoded to use the old, hotspot aware non-atomic API anyways. So it would only make things worse since hiding the cursorplane for userspace which does not set the CAP would mean the the old non-atomic API also stops working. Or this would add extra complications in the kernel to still keep the old APIs working. I do think that a CAP might be a good idea, but the disabling of the cursor plane based on the CAP does not sound like a good idea to me. Regards, Hans
Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS
Hi, On 6/10/22 14:53, Simon Ser wrote: > On Friday, June 10th, 2022 at 14:36, Gerd Hoffmann wrote: > >> Hi, >> As Pekka mentionned, I'd also like to have a conversation of how far we want to push virtualized driver features. I think KMS support is a good feature to have to spin up a VM and have all of the basics working. However I don't think it's a good idea to try to plumb an ever-growing list of fancy features (seamless integration of guest windows into the host, HiDPI, multi-monitor, etc) into KMS. You'd just end up re-inventing Wayland or RDP on top of KMS. Instead of re-inventing these, just use RDP or waypipe or X11 forwarding directly. >> So I think we need to draw a line somewhere, and decide e.g. that virtualized cursors are fine to add in KMS, but HiDPI is not. >> >> >> What is the problem with HiDPI? qemu generates standard edid blobs, >> there should be no need to special-case virtualized drivers in any way. >> >> What is the problem with multi-monitor? That isn't much different than >> physical multi-monitor either. >> >> One little thing though: On physical hardware you just don't know which >> monitor is left and which is right until the user tells you. In case of >> a virtual multi-monitor setup we know how the two windows for the two >> virtual monitors are arranged on the host and can pass that as hint to >> the guest (not sure whenever that is the purpose of the >> suggested_{x,y} properties). > > The problem with suggested_x/y is described here: > https://lore.kernel.org/dri-devel/20220610123629.fgu2em3fto53f...@sirius.home.kraxel.org/T/#m119cfbbf736e43831c3105f0c91bd790da2d58fb > > HiDPI would need a way to propagate the scale factor back-and-forth: > the VM viewer needs to advertise the preferred scale to the guest > compositor, and the guest compositor needs to indicate the scale it > renders with to the VM viewer. > > Sounds familiar? Yup, that's exactly the Wayland protocol. Do we really > want to replicate the Wayland protocol in KMS? I'm not so sure. > >>> It's getting a bit far off-topic, but google cros team has an out-of-tree >>> (at least I think it's not merged yet) wayland-virtio driver for exactly >>> this use-case. Trying to move towards something like that for fancy >>> virtualized setups sounds like the better approach indeed, with kms just >>> as the bare-bones fallback option. >> >> virtio-gpu got the ability to attach uuids to objects, to allow them >> being identified on the host side. So it could be that wayland-virtio >> still uses kms for framebuffers (disclaimer: don't know how >> wayland-virtio works in detail). But, yes, all the scanout + cursor >> handling would be out of the way, virtio-gpu would "only" handle fast >> buffer sharing. > > wayland-virtio is not used with KMS. wayland-virtio proxies the Wayland > protocol between the host and the guest, so the guest doesn't use KMS > in that case. It would be more correct to say: wayland clients inside the guest don't talk to a compositor inside the guest (but rather one outside the guest) and thus also don't depend (indirectly) on\ having kms inside the guest. But the guest likely still needs kms for e.g. the kernel console to e.g. debug boot failures. Note this could be done over a serial console too, so in some cases whatever "video-card" emulation the guest has could theoretically go away. But it is also completely possible for a guest to have both some emulated video-card which offers a kms API to userspace as well as wayland-virtio. Regards, Hans
Re: [PATCH libinput 1/2] filter: change the tracker delta type to device-units
Hi, On 19-01-17 03:34, Peter Hutterer wrote: We were just switching type here without actual normalization, the filter code is in device units as of bdd4264d6150f4a6248eec7. Signed-off-by: Peter Hutterer Series looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/filter.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/filter.c b/src/filter.c index d7a1515..3035234 100644 --- a/src/filter.c +++ b/src/filter.c @@ -156,7 +156,7 @@ filter_get_type(struct motion_filter *filter) #define NUM_POINTER_TRACKERS 16 struct pointer_tracker { - struct normalized_coords delta; /* delta to most recent event */ + struct device_float_coords delta; /* delta to most recent event */ uint64_t time; /* us */ uint32_t dir; }; @@ -230,7 +230,7 @@ static double calculate_tracker_velocity(struct pointer_tracker *tracker, uint64_t time) { double tdelta = time - tracker->time + 1; - return normalized_length(tracker->delta) / tdelta; /* units/us */ + return hypot(tracker->delta.x, tracker->delta.y) / tdelta; /* units/us */ } static inline double ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 1/3] touchpad: mask out ABS_MT if we don't have or disable MT
Hi, On 01/24/2017 06:46 AM, Peter Hutterer wrote: Make sure the events we deal with are the ones we actually honor. This reduces the chance that we accidentally process events we weren't event supposed to get based on some earlier device decision. Signed-off-by: Peter Hutterer Patch-set looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-mt-touchpad.c | 13 + 1 file changed, 13 insertions(+) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index f437c2d..ca00c40 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -1749,6 +1749,16 @@ tp_sync_touch(struct tp_dispatch *tp, libevdev_fetch_slot_value(evdev, slot, ABS_MT_DISTANCE, &t->distance); } +static inline void +tp_disable_abs_mt(struct evdev_device *device) +{ + struct libevdev *evdev = device->evdev; + unsigned int code; + + for (code = ABS_MT_SLOT; code <= ABS_MAX; code++) + libevdev_disable_event_code(evdev, EV_ABS, code); +} + static bool tp_init_slots(struct tp_dispatch *tp, struct evdev_device *device) @@ -1804,6 +1814,9 @@ tp_init_slots(struct tp_dispatch *tp, tp->has_mt = false; } + if (!tp->has_mt) + tp_disable_abs_mt(device); + ARRAY_FOR_EACH(max_touches, m) { if (libevdev_has_event_code(device->evdev, EV_KEY, ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: expand top middle button to cover 40mm to 60mm
Hi, On 08-02-17 01:17, Peter Hutterer wrote: 42 and 58 were within the middle button already, 40/60 are more accurate values. https://bugs.freedesktop.org/show_bug.cgi?id=99212 Signed-off-by: Peter Hutterer Looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-mt-touchpad-buttons.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/evdev-mt-touchpad-buttons.c b/src/evdev-mt-touchpad-buttons.c index 4a68470..b53d2e4 100644 --- a/src/evdev-mt-touchpad-buttons.c +++ b/src/evdev-mt-touchpad-buttons.c @@ -610,13 +610,13 @@ tp_init_top_softbuttons(struct tp_dispatch *tp, evdev_device_get_size(device, &width, &height); - mm.x = width * 0.58; + mm.x = width * 0.60; mm.y = topsize_mm; edges = evdev_device_mm_to_units(device, &mm); tp->buttons.top_area.bottom_edge = edges.y; tp->buttons.top_area.rightbutton_left_edge = edges.x; - mm.x = width * 0.42; + mm.x = width * 0.40; edges = evdev_device_mm_to_units(device, &mm); tp->buttons.top_area.leftbutton_right_edge = edges.x; } else { ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 10/11] touchpad: use pressure values for touch is-down decision
Hi, First of all patches 1-9 and 11 looks good to me and are: Reviewed-by: Hans de Goede I've some remarks inline on this one. On 30-01-17 01:58, Peter Hutterer wrote: Don't rely on BTN_TOUCH for "finger down", the value for that is hardcoded in the kernel and not always suitable. Some devices need a different value to avoid reacting to accidental touches or hovering fingers. Implement a basic Schmitt trigger, same as we have in the synaptics driver. We also take the default values from there but these will likely see some updates. A special case is when we have more fingers down than slots. Since we can't detect the pressure on fake fingers (we only get a bit for 'is down') we assume that *all* fingers are down with sufficient pressure. It's too much of a niche case to have this work any other way. This patch drops the handling of ABS_DISTANCE because it's simply not needed anymore. Signed-off-by: Peter Hutterer --- src/evdev-mt-touchpad.c | 129 +-- src/evdev-mt-touchpad.h | 10 ++- test/litest-device-alps-dualpoint.c | 14 test/litest-device-alps-semi-mt.c| 14 test/litest-device-atmel-hover.c | 14 test/litest-device-synaptics-hover.c | 14 test/litest-device-synaptics-st.c| 15 +++- 7 files changed, 185 insertions(+), 25 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index a47c59f..ff9ec41 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -322,9 +322,6 @@ tp_process_absolute(struct tp_dispatch *tp, case ABS_MT_SLOT: tp->slot = e->value; break; - case ABS_MT_DISTANCE: - t->distance = e->value; - break; case ABS_MT_TRACKING_ID: if (e->value != -1) tp_new_touch(tp, t, time); @@ -365,6 +362,11 @@ tp_process_absolute_st(struct tp_dispatch *tp, t->dirty = true; tp->queued |= TOUCHPAD_EVENT_MOTION; break; + case ABS_PRESSURE: + t->pressure = e->value; + t->dirty = true; + tp->queued |= TOUCHPAD_EVENT_OTHERAXIS; + break; } } @@ -795,26 +797,72 @@ out: } static void -tp_unhover_abs_distance(struct tp_dispatch *tp, uint64_t time) +tp_unhover_pressure(struct tp_dispatch *tp, uint64_t time) { struct tp_touch *t; - unsigned int i; + int i; + unsigned int nfake_touches; + unsigned int real_fingers_down = 0; - for (i = 0; i < tp->ntouches; i++) { + nfake_touches = tp_fake_finger_count(tp); + if (nfake_touches == FAKE_FINGER_OVERFLOW) + nfake_touches = 0; + + for (i = 0; i < (int)tp->num_slots; i++) { t = tp_get_touch(tp, i); - if (!t->dirty) - continue; + if (t->dirty) { + if (t->state == TOUCH_HOVERING) { + if (t->pressure >= tp->pressure.high) { + /* avoid jumps when landing a finger */ + tp_motion_history_reset(t); + tp_begin_touch(tp, t, time); + } + } else { + if (t->pressure < tp->pressure.low) + tp_end_touch(tp, t, time); + } + } + + if (t->state == TOUCH_BEGIN || + t->state == TOUCH_UPDATE) + real_fingers_down++; + } + if (nfake_touches <= tp->num_slots || + tp->nfingers_down == 0) + return; Maybe: if (tp->nfingers_down >= nfake_touches || tp->nfingers_down == 0) return; Otherwise this: + + /* if we have more fake fingers down than slots, we assume +* _all_ fingers have enough pressure, even if some of the slotted +* ones don't. Anything else gets insane quickly. +*/ + for (i = 0; i < (int)tp->ntouches; i++) { + t = tp_get_touch(tp, i); if (t->state == TOUCH_HOVERING) { + /* avoid jumps when landing a finger */ + tp_motion_history_reset(t); + tp_begin_touch(tp, t, time); + + if (tp->nfingers_down >= nfake_touches) + break; + } + } Will always unhover one more hovering touch even if we already have enough, I don't know if we can have more touches in down + hovering state then nfake_touches but otherwise the if (tp->nfingers
Re: [PATCH libinput 1/2] evdev: improve type-safety on dispatch switches
Hi, On 30-01-17 23:21, Peter Hutterer wrote: Set the dispatch type on creation, then check that whenever we try to get the dispatch struct. This avoids a potential mismatch between the backends. Plus, use of container_of means we're not dependent on the exact layout anymore. Signed-off-by: Peter Hutterer Series looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-lid.c | 28 +--- src/evdev-mt-touchpad-tap.c | 22 -- src/evdev-mt-touchpad.c | 14 ++ src/evdev-mt-touchpad.h | 10 ++ src/evdev-tablet-pad.c | 7 --- src/evdev-tablet-pad.h | 10 ++ src/evdev-tablet.c | 21 - src/evdev-tablet.h | 10 ++ src/evdev.c | 17 + src/evdev.h | 27 +++ 10 files changed, 113 insertions(+), 53 deletions(-) diff --git a/src/evdev-lid.c b/src/evdev-lid.c index c5af753..6a2b506 100644 --- a/src/evdev-lid.c +++ b/src/evdev-lid.c @@ -41,13 +41,22 @@ struct lid_switch_dispatch { } keyboard; }; +static inline struct lid_switch_dispatch* +lid_dispatch(struct evdev_dispatch *dispatch) +{ + struct lid_switch_dispatch *l; + + evdev_verify_dispatch_type(dispatch, DISPATCH_LID_SWITCH); + + return container_of(dispatch, l, base); +} + static void lid_switch_keyboard_event(uint64_t time, struct libinput_event *event, void *data) { - struct lid_switch_dispatch *dispatch = - (struct lid_switch_dispatch*)data; + struct lid_switch_dispatch *dispatch = lid_dispatch(data); if (!dispatch->lid_is_closed) return; @@ -127,8 +136,7 @@ lid_switch_process(struct evdev_dispatch *evdev_dispatch, struct input_event *event, uint64_t time) { - struct lid_switch_dispatch *dispatch = - (struct lid_switch_dispatch*)evdev_dispatch; + struct lid_switch_dispatch *dispatch = lid_dispatch(evdev_dispatch); switch (event->type) { case EV_SW: @@ -168,8 +176,7 @@ evdev_read_switch_reliability_prop(struct evdev_device *device) static void lid_switch_destroy(struct evdev_dispatch *evdev_dispatch) { - struct lid_switch_dispatch *dispatch = - (struct lid_switch_dispatch*)evdev_dispatch; + struct lid_switch_dispatch *dispatch = lid_dispatch(evdev_dispatch); free(dispatch); } @@ -179,7 +186,7 @@ lid_switch_pair_keyboard(struct evdev_device *lid_switch, struct evdev_device *keyboard) { struct lid_switch_dispatch *dispatch = - (struct lid_switch_dispatch*)lid_switch->dispatch; + lid_dispatch(lid_switch->dispatch); unsigned int bus_kbd = libevdev_get_id_bustype(keyboard->evdev); if ((keyboard->tags & EVDEV_TAG_KEYBOARD) == 0) @@ -214,8 +221,7 @@ static void lid_switch_interface_device_removed(struct evdev_device *device, struct evdev_device *removed_device) { - struct lid_switch_dispatch *dispatch = - (struct lid_switch_dispatch*)device->dispatch; + struct lid_switch_dispatch *dispatch = lid_dispatch(device->dispatch); if (removed_device == dispatch->keyboard.keyboard) { libinput_device_remove_event_listener( @@ -228,8 +234,7 @@ static void lid_switch_sync_initial_state(struct evdev_device *device, struct evdev_dispatch *evdev_dispatch) { - struct lid_switch_dispatch *dispatch = - (struct lid_switch_dispatch*)evdev_dispatch; + struct lid_switch_dispatch *dispatch = lid_dispatch(device->dispatch); struct libevdev *evdev = device->evdev; bool is_closed = false; @@ -284,6 +289,7 @@ evdev_lid_switch_dispatch_create(struct evdev_device *lid_device) if (dispatch == NULL) return NULL; + dispatch->base.dispatch_type = DISPATCH_LID_SWITCH; dispatch->base.interface = &lid_switch_interface; dispatch->device = lid_device; libinput_device_init_event_listener(&dispatch->keyboard.listener); diff --git a/src/evdev-mt-touchpad-tap.c b/src/evdev-mt-touchpad-tap.c index 9fba521..5fccbc2 100644 --- a/src/evdev-mt-touchpad-tap.c +++ b/src/evdev-mt-touchpad-tap.c @@ -901,7 +901,7 @@ tp_tap_config_count(struct libinput_device *device) struct tp_dispatch *tp = NULL; dispatch = ((struct evdev_device *) device)->dispatch; - tp = container_of(dispatch, tp, base); + tp = tp_dispatch(dispatch); return min(tp->ntouches, 3U); /* we only do up to 3 finger tap */ } @@ -910,10 +910,12 @@ static enum libinput_config_status tp_tap_config_set_enabled(struct libinput_device *device,
Re: [PATCH libinput 10/11] touchpad: use pressure values for touch is-down decision
Hi, On 09-02-17 23:55, Peter Hutterer wrote: On Thu, Feb 09, 2017 at 05:25:36PM +0100, Hans de Goede wrote: Hi, First of all patches 1-9 and 11 looks good to me and are: Reviewed-by: Hans de Goede thx, fwiw, I already pushed these, so fixups will come as a separate patch. I've some remarks inline on this one. On 30-01-17 01:58, Peter Hutterer wrote: Don't rely on BTN_TOUCH for "finger down", the value for that is hardcoded in the kernel and not always suitable. Some devices need a different value to avoid reacting to accidental touches or hovering fingers. Implement a basic Schmitt trigger, same as we have in the synaptics driver. We also take the default values from there but these will likely see some updates. A special case is when we have more fingers down than slots. Since we can't detect the pressure on fake fingers (we only get a bit for 'is down') we assume that *all* fingers are down with sufficient pressure. It's too much of a niche case to have this work any other way. This patch drops the handling of ABS_DISTANCE because it's simply not needed anymore. Signed-off-by: Peter Hutterer --- src/evdev-mt-touchpad.c | 129 +-- src/evdev-mt-touchpad.h | 10 ++- test/litest-device-alps-dualpoint.c | 14 test/litest-device-alps-semi-mt.c| 14 test/litest-device-atmel-hover.c | 14 test/litest-device-synaptics-hover.c | 14 test/litest-device-synaptics-st.c| 15 +++- 7 files changed, 185 insertions(+), 25 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index a47c59f..ff9ec41 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -322,9 +322,6 @@ tp_process_absolute(struct tp_dispatch *tp, case ABS_MT_SLOT: tp->slot = e->value; break; - case ABS_MT_DISTANCE: - t->distance = e->value; - break; case ABS_MT_TRACKING_ID: if (e->value != -1) tp_new_touch(tp, t, time); @@ -365,6 +362,11 @@ tp_process_absolute_st(struct tp_dispatch *tp, t->dirty = true; tp->queued |= TOUCHPAD_EVENT_MOTION; break; + case ABS_PRESSURE: + t->pressure = e->value; + t->dirty = true; + tp->queued |= TOUCHPAD_EVENT_OTHERAXIS; + break; } } @@ -795,26 +797,72 @@ out: } static void -tp_unhover_abs_distance(struct tp_dispatch *tp, uint64_t time) +tp_unhover_pressure(struct tp_dispatch *tp, uint64_t time) { struct tp_touch *t; - unsigned int i; + int i; + unsigned int nfake_touches; + unsigned int real_fingers_down = 0; - for (i = 0; i < tp->ntouches; i++) { + nfake_touches = tp_fake_finger_count(tp); + if (nfake_touches == FAKE_FINGER_OVERFLOW) + nfake_touches = 0; + + for (i = 0; i < (int)tp->num_slots; i++) { t = tp_get_touch(tp, i); - if (!t->dirty) - continue; + if (t->dirty) { + if (t->state == TOUCH_HOVERING) { + if (t->pressure >= tp->pressure.high) { + /* avoid jumps when landing a finger */ + tp_motion_history_reset(t); + tp_begin_touch(tp, t, time); + } + } else { + if (t->pressure < tp->pressure.low) + tp_end_touch(tp, t, time); + } + } + + if (t->state == TOUCH_BEGIN || + t->state == TOUCH_UPDATE) + real_fingers_down++; + } + if (nfake_touches <= tp->num_slots || + tp->nfingers_down == 0) + return; Maybe: if (tp->nfingers_down >= nfake_touches || tp->nfingers_down == 0) return; I don't think that's correct. tp->nfingers_down includes fake touches being down, whereas fake touches merely counts the BTN_TOOL_ bits set. So tp->nfingers_down > nfake_touches can only be true on the bcm5974 when 6 fingers are down (that's the only touchpad that has >5 slots, iirc). and if it's == nfake_touches, then we already have all fingers down, so we would never unhover, returning here isn't right. this condition really checks that "we have more slots than fake fingers down" which is the condition for "don't care about fake fingers". Come to think of it, I wonder if it'd be useful to disable BTN_TOOL_* for those < num_slots at the evdev level to avoi
Re: [PATCH libinput 1/2] touchpad: add a hwdb quirk for (external) touchpad/keyboard combos
Hi, Series looks good, but patch 1/2 introduces tp_is_tpkb_combo_below() and patch 2/2 then moves it around to avoid doing a forward declaration, it would be better IMHO if patch 1/2 simply defined it in the place where 2/2 puts it. With that fixed: Reviewed-by: Hans de Goede Regards, Hans On 10-02-17 06:45, Peter Hutterer wrote: Specify the layout of the combo so we know when to initialize palm detection. This allows us to drop palm detection on external touchpads otherwise, replacing the wacom-specific check with something more generic.. Signed-off-by: Peter Hutterer --- src/evdev-mt-touchpad.c| 19 +-- src/libinput-util.c| 24 src/libinput-util.h| 7 +++ test/test-touchpad.c | 4 udev/90-libinput-model-quirks.hwdb | 7 +++ udev/parse_hwdb.py | 7 ++- 6 files changed, 65 insertions(+), 3 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index c5eeeac..f8c5cc6 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -2192,6 +2192,21 @@ tp_init_dwt(struct tp_dispatch *tp, return; } +static inline bool +tp_is_tpkb_combo_below(struct evdev_device *device) +{ + const char *prop; + enum tpkbcombo_layout layout = TPKBCOMBO_LAYOUT_UNKNOWN; + + prop = udev_device_get_property_value(device->udev_device, + "LIBINPUT_ATTR_TPKBCOMBO_LAYOUT"); + if (!prop) + return false; + + return parse_tpkbcombo_layout_poperty(prop, &layout) && + layout == TPKBCOMBO_LAYOUT_BELOW; +} + static void tp_init_palmdetect(struct tp_dispatch *tp, struct evdev_device *device) @@ -2203,8 +2218,8 @@ tp_init_palmdetect(struct tp_dispatch *tp, tp->palm.right_edge = INT_MAX; tp->palm.left_edge = INT_MIN; - /* Wacom doesn't have internal touchpads */ - if (device->model_flags & EVDEV_MODEL_WACOM_TOUCHPAD) + if (device->tags & EVDEV_TAG_EXTERNAL_TOUCHPAD && + !tp_is_tpkb_combo_below(device)) return; evdev_device_get_size(device, &width, &height); diff --git a/src/libinput-util.c b/src/libinput-util.c index d75955c..351bbe4 100644 --- a/src/libinput-util.c +++ b/src/libinput-util.c @@ -336,6 +336,30 @@ parse_switch_reliability_property(const char *prop, } /** + * Parses a string with the allowed values: "below" + * The value refers to the position of the touchpad (relative to the + * keyboard, i.e. your average laptop would be 'below') + * + * @param prop The value of the property + * @param layout The layout + * @return true on success, false otherwise + */ +bool +parse_tpkbcombo_layout_poperty(const char *prop, + enum tpkbcombo_layout *layout) +{ + if (!prop) + return false; + + if (streq(prop, "below")) { + *layout = TPKBCOMBO_LAYOUT_BELOW; + return true; + } + + return false; +} + +/** * Return the next word in a string pointed to by state before the first * separator character. Call repeatedly to tokenize a whole string. * diff --git a/src/libinput-util.h b/src/libinput-util.h index 00ece58..d86ff12 100644 --- a/src/libinput-util.h +++ b/src/libinput-util.h @@ -379,6 +379,13 @@ double parse_trackpoint_accel_property(const char *prop); bool parse_dimension_property(const char *prop, size_t *width, size_t *height); bool parse_calibration_property(const char *prop, float calibration[6]); +enum tpkbcombo_layout { + TPKBCOMBO_LAYOUT_UNKNOWN, + TPKBCOMBO_LAYOUT_BELOW, +}; +bool parse_tpkbcombo_layout_poperty(const char *prop, + enum tpkbcombo_layout *layout); + enum switch_reliability { RELIABILITY_UNKNOWN, RELIABILITY_RELIABLE, diff --git a/test/test-touchpad.c b/test/test-touchpad.c index aca9f7b..4656443 100644 --- a/test/test-touchpad.c +++ b/test/test-touchpad.c @@ -933,11 +933,15 @@ touchpad_has_palm_detect_size(struct litest_device *dev) { double width, height; unsigned int vendor; + unsigned int bustype; int rc; vendor = libinput_device_get_id_vendor(dev->libinput_device); + bustype = libevdev_get_id_bustype(dev->evdev); if (vendor == VENDOR_ID_WACOM) return 0; + if (bustype == BUS_BLUETOOTH) + return 0; if (vendor == VENDOR_ID_APPLE) return 1; diff --git a/udev/90-libinput-model-quirks.hwdb b/udev/90-libinput-model-quirks.hwdb index 412d872..c1d6235 100644 --- a/udev/90-libinput-model-quirks.hwdb +++ b/udev/90-libinput-model-quirks.hwdb @@ -60,6 +60,13 @@ libinput:name:*ETPS/2 Elantech Touchpad*:dmi:*svnASUSTeKCOMPUTERINC.:pnX555LA
Re: [PATCH libinput] tools: hide key codes by default
Hi, On 13-02-17 02:18, Peter Hutterer wrote: libinput-debug-events prints keycodes as they come in. This makes it dangerous to be run by users (especially in the background) because it will leak sensitive information as it is typed. Obfuscate the base set of keycodes by default, require a --show-keycodes switch to show it. The few times we actually need the keycodes, we can run the switch in the debugging tool. This does not affect keys outside of the main block on the keyboard (F-keys, multimedia keys). Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- tools/event-debug.c | 19 +++ tools/shared.c | 7 +++ tools/shared.h | 3 +++ 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/tools/event-debug.c b/tools/event-debug.c index 779b54a..9022e39 100644 --- a/tools/event-debug.c +++ b/tools/event-debug.c @@ -267,20 +267,31 @@ print_device_notify(struct libinput_event *ev) } static void -print_key_event(struct libinput_event *ev) +print_key_event(struct libinput *li, struct libinput_event *ev) { struct libinput_event_keyboard *k = libinput_event_get_keyboard_event(ev); + struct tools_context *context; + struct tools_options *options; enum libinput_key_state state; uint32_t key; const char *keyname; + context = libinput_get_user_data(li); + options = &context->options; + print_event_time(libinput_event_keyboard_get_time(k)); state = libinput_event_keyboard_get_key_state(k); key = libinput_event_keyboard_get_key(k); - keyname = libevdev_event_code_get_name(EV_KEY, key); + if (!options->show_keycodes && + (key >= KEY_ESC && key < KEY_ZENKAKUHANKAKU)) { + keyname = "***"; + } else { + keyname = libevdev_event_code_get_name(EV_KEY, key); + keyname = keyname ? keyname : "???"; + } printf("%s (%d) %s\n", - keyname ? keyname : "???", + keyname, key, state == LIBINPUT_KEY_STATE_PRESSED ? "pressed" : "released"); } @@ -754,7 +765,7 @@ handle_and_print_events(struct libinput *li) &context.options); break; case LIBINPUT_EVENT_KEYBOARD_KEY: - print_key_event(ev); + print_key_event(li, ev); break; case LIBINPUT_EVENT_POINTER_MOTION: print_motion_event(ev); diff --git a/tools/shared.c b/tools/shared.c index 05fb118..1019184 100644 --- a/tools/shared.c +++ b/tools/shared.c @@ -62,6 +62,7 @@ enum options { OPT_SCROLL_BUTTON, OPT_SPEED, OPT_PROFILE, + OPT_SHOW_KEYCODES, }; LIBINPUT_ATTRIBUTE_PRINTF(3, 0) @@ -103,6 +104,7 @@ tools_usage(void) "--set-profile=[adaptive|flat] set pointer acceleration profile\n" "--set-speed= set pointer acceleration speed (allowed range [-1, 1]) \n" "--set-tap-map=[lrm|lmr] ... set button mapping for tapping\n" + "--show-keycodes show all key codes while typing\n" "\n" "These options apply to all applicable devices, if a feature\n" "is not explicitly specified it is left at each device's default.\n" @@ -137,6 +139,7 @@ tools_init_context(struct tools_context *context) options->seat = "seat0"; options->speed = 0.0; options->profile = LIBINPUT_CONFIG_ACCEL_PROFILE_NONE; + options->show_keycodes = false; } int @@ -173,6 +176,7 @@ tools_parse_args(int argc, char **argv, struct tools_context *context) { "set-profile", 1, 0, OPT_PROFILE }, { "set-tap-map", 1, 0, OPT_TAP_MAP }, { "set-speed", 1, 0, OPT_SPEED }, + { "show-keycodes", 0, 0, OPT_SHOW_KEYCODES }, { 0, 0, 0, 0} }; @@ -337,6 +341,9 @@ tools_parse_args(int argc, char **argv, struct tools_context *context) return 1; } break; + case OPT_SHOW_KEYCODES: + options->show_keycodes = true; + break; default: tools_usage(); return 1; diff --git a/tools/shared.h b/tools/shared.h index 17fdf37..9b1a988 100644 --- a/tools/shared.h +++ b/tools/shared.h @@ -24,6 +24,8 @@ #ifndef _SHARED_H_ #define _SHARED_H_ +#include + #include enum tools_backend { @@ -36,6 +38,7 @@ struct tool
Re: [PATCH libinput] Added missing button range for pad on CTH-680
Hi, On 13-02-17 07:57, Peter Hutterer wrote: From: Sakse Dalum This device has BTN_LEFT, BTN_RIGHT, BTN_FORWARD and BTN_BACK, add the missing range to the pad init function. https://bugs.freedesktop.org/show_bug.cgi?id=99785 Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-tablet-pad.c | 5 + 1 file changed, 5 insertions(+) diff --git a/src/evdev-tablet-pad.c b/src/evdev-tablet-pad.c index bed43b6..6be53b5 100644 --- a/src/evdev-tablet-pad.c +++ b/src/evdev-tablet-pad.c @@ -542,6 +542,11 @@ pad_init_buttons(struct pad_dispatch *pad, pad->button_map[code] = map++; } + for (code = BTN_LEFT; code < BTN_LEFT + 7; code++) { + if (libevdev_has_event_code(device->evdev, EV_KEY, code)) + pad->button_map[code] = map++; + } + pad->nbuttons = map; } ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v2 libinput] tools: print errors as red, info as highlighted
Hi, On 14-02-17 02:58, Peter Hutterer wrote: makes it easier to filter out debugging messages Signed-off-by: Peter Hutterer --- Sorry, used the wrong patch, v1 was an earlier version. v2 looks good to me: Reviewed-by: Hans de Goede Regards, Hans tools/shared.c | 18 ++ 1 file changed, 18 insertions(+) diff --git a/tools/shared.c b/tools/shared.c index 1019184..81608c0 100644 --- a/tools/shared.c +++ b/tools/shared.c @@ -72,7 +72,25 @@ log_handler(struct libinput *li, const char *format, va_list args) { +#define ANSI_HIGHLIGHT "\x1B[0;1;39m" +#define ANSI_RED "\x1B[0;31m" +#define ANSI_NORMAL "\x1B[0m" + static int is_tty = -1; + + if (is_tty == -1) + is_tty = isatty(STDOUT_FILENO); + + if (is_tty) { + if (priority >= LIBINPUT_LOG_PRIORITY_ERROR) + printf(ANSI_RED); + else if (priority >= LIBINPUT_LOG_PRIORITY_INFO) + printf(ANSI_HIGHLIGHT); + } + vprintf(format, args); + + if (is_tty && priority >= LIBINPUT_LOG_PRIORITY_INFO) + printf(ANSI_NORMAL); } void ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 00/12] button-scrolling fixes
Hi, On 20-02-17 21:53, Peter Hutterer wrote: The core bits here are 05/12 and 11/12, the rest is restructuring and various fixes to enable those two. 05/12 enables button-scrolling on the left/right mouse button when middle button emulation is also active. This currently works but generates a log_bug_libinput() because of a negative timer offset. There are some mice (trackballs, mainly) that only have two buttons so both middle button emulation and button scrolling on the same device is useful, even though it's a bit of a niche case. 11/12 extends the button scrolling implementation. Right now, we have three states: idle, button is down and scrolling. The latter is triggered as soon as the scroll button is held past the timeout. But that also results in the button being inconsistent - a short click generates a button click, a long click (but without scrolling) is discarded. With these fixes, a long click now sends button events if there never were any scroll events generated. Series looks good to me: Reviewed-by: Hans de Goede Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] filter: tweak touchpad acceleration code again
Hi, On 24-02-17 05:37, Peter Hutterer wrote: Reduce the magic factor a bit, thus making the pointer more (slow == precise) on 'normal movements'. This coincidentally fixes some of the issues with the Lenovo T450 and friends because the user's base speed must go up, thus papering over the firmware bug that makes the pointer jump at slow movements. Reduce the threshold speed, so acceleration kicks in a little bit earlier. Reduce the threshold range, so at higher speeds accel kicks in earlier but not too early. Flatten the incline, so acceleration applies at a slower rate. This appears to fix many of the 'overshoot' problems seen with the current code. Rework how the accel speed affects the curve and increase the range of the accel speed. This now covers a fairly large range from "tarpit" to "tux racer". At all speeds bar the super-slow ones it's still relatively easy to cross the screen with few motions. The patch also works the touchpad magic slowdown into the factor applied. Since this is a static number now, calculate it once at set-speed and then store the result in the filter. Signed-off-by: Peter Hutterer The code changes look good to me, as for how the new accel feels / works, I trust you've good reasons for these changes: Acked-by: Hans de Goede Regards, Hans --- This is the first time I'm quite happy with the touchpad acceleration. src/filter.c | 37 ++--- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/src/filter.c b/src/filter.c index 7c500f8..4bc6239 100644 --- a/src/filter.c +++ b/src/filter.c @@ -40,7 +40,7 @@ * technically correct but subjectively wrong, we expect a touchpad to be a * lot slower than a mouse. Apply a magic factor to slow down all movements */ -#define TP_MAGIC_SLOWDOWN 0.37 /* unitless factor */ +#define TP_MAGIC_SLOWDOWN 0.29 /* unitless factor */ /* Convert speed/velocity from units/us to units/ms */ static inline double @@ -135,10 +135,10 @@ filter_get_type(struct motion_filter *filter) #define DEFAULT_INCLINE 1.1/* unitless factor */ /* Touchpad acceleration */ -#define TOUCHPAD_DEFAULT_THRESHOLD 254 /* mm/s */ -#define TOUCHPAD_THRESHOLD_RANGE 184 /* mm/s */ -#define TOUCHPAD_ACCELERATION 9.0 /* unitless factor */ -#define TOUCHPAD_INCLINE 0.011 /* unitless factor */ +#define TOUCHPAD_DEFAULT_THRESHOLD 225 /* mm/s */ +#define TOUCHPAD_THRESHOLD_RANGE 25/* mm/s */ +#define TOUCHPAD_ACCELERATION 3.0 /* unitless factor */ +#define TOUCHPAD_INCLINE 0.008 /* unitless factor */ /* for the Lenovo x230 custom accel. do not touch */ #define X230_THRESHOLD v_ms2us(0.4)/* in units/us */ @@ -175,6 +175,7 @@ struct pointer_accelerator { double threshold; /* units/us */ double accel; /* unitless factor */ double incline; /* incline of the function */ + double magic; /* magic scaling factor */ int dpi; }; @@ -583,9 +584,16 @@ touchpad_accelerator_set_speed(struct motion_filter *filter, /* adjust when accel kicks in */ accel_filter->threshold = TOUCHPAD_DEFAULT_THRESHOLD - - TOUCHPAD_THRESHOLD_RANGE * speed_adjustment; + TOUCHPAD_THRESHOLD_RANGE * (1 + speed_adjustment); accel_filter->accel = TOUCHPAD_ACCELERATION; accel_filter->incline = TOUCHPAD_INCLINE; + + /* magic is the factor we scale everything by. For unaccelerated +* motion, this is the baseline factor, for accelerated this scales +* down the gain we otherwise calculate. 0.28 is a result of +* trial&error, don't attach any meaning to it. */ + accel_filter->magic = TP_MAGIC_SLOWDOWN + 0.28 * speed_adjustment; + filter->speed_adjustment = speed_adjustment; return true; @@ -810,7 +818,7 @@ touchpad_accel_profile_linear(struct motion_filter *filter, accel factor - ^ + ^ |/ | _/ | / @@ -823,6 +831,9 @@ touchpad_accel_profile_linear(struct motion_filter *filter, x is speed_in a is the incline of acceleration b is minimum acceleration factor + The flat bit is the unaccelerated 'baseline speed' where we + merely slow down the input speed to provide reasonable pointer + speed. for speeds up to the lower threshold, we decelerate, down to 30% of input speed. @@ -836,7 +847,13 @@ touchpad_accel_profile_linear(struct motion_filter *filter, has no other intrinsic meaning. * 0.3 is chosen simply because it is above the Nyquist frequency for subpixel motion within a pixel. - */ + + The accel
Re: [PATCH libinput 1/2] Fix a crash when requesting invalid mode group indices
Hi, On 24-02-17 05:41, Peter Hutterer wrote: Signed-off-by: Peter Hutterer Patch series looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-tablet-pad-leds.c | 4 1 file changed, 4 insertions(+) diff --git a/src/evdev-tablet-pad-leds.c b/src/evdev-tablet-pad-leds.c index 209ab73..89b3b9d 100644 --- a/src/evdev-tablet-pad-leds.c +++ b/src/evdev-tablet-pad-leds.c @@ -645,5 +645,9 @@ evdev_device_tablet_pad_get_mode_group(struct evdev_device *device, if (!(device->seat_caps & EVDEV_DEVICE_TABLET_PAD)) return NULL; + if (index >= + (unsigned int)evdev_device_tablet_pad_get_num_mode_groups(device)) + return NULL; + return pad_get_mode_group(pad, index); } ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 0/4] logging cleanup
Hi, On 24-02-17 07:15, Peter Hutterer wrote: Brings some consistency to the log output, making it easier to grep for a specific device's output, etc. Patch series looks good to me: Acked-by: Hans de Goede Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: reduce minimum height for horiz edge scrolling to 40mm
Hi, On 27-02-17 02:02, Peter Hutterer wrote: Introduced in commit 8e7f99c27ab39 we only allowed horizontal edge scrolling on devices larger than 50mm to leave enough reactive space on the touchpad. Looking at a ruler, a 50mm high touchpad is still large enough to leave the bottom 7mm as an horizontal edge scroll area. Reduce the minimum size to 40mm instead, that's closer to where it starts to get a bit iffy. https://bugzilla.redhat.com/show_bug.cgi?id=141 Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-mt-touchpad-edge-scroll.c | 4 ++-- test/test-touchpad.c| 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/evdev-mt-touchpad-edge-scroll.c b/src/evdev-mt-touchpad-edge-scroll.c index 1d30bca..5551a8d 100644 --- a/src/evdev-mt-touchpad-edge-scroll.c +++ b/src/evdev-mt-touchpad-edge-scroll.c @@ -291,14 +291,14 @@ tp_edge_scroll_init(struct tp_dispatch *tp, struct evdev_device *device) struct phys_coords mm = { 0.0, 0.0 }; evdev_device_get_size(device, &width, &height); - /* Touchpads smaller than 50mm are not tall enough to have a + /* Touchpads smaller than 40mm are not tall enough to have a horizontal scroll area, it takes too much space away. But clickpads have enough space here anyway because of the software button area (and all these tiny clickpads were built when software buttons were a thing, e.g. Lenovo *20 series) */ if (!tp->buttons.is_clickpad) - want_horiz_scroll = (height >= 50); + want_horiz_scroll = (height >= 40); /* 7mm edge size */ mm.x = width - 7; diff --git a/test/test-touchpad.c b/test/test-touchpad.c index 4656443..29039b3 100644 --- a/test/test-touchpad.c +++ b/test/test-touchpad.c @@ -462,7 +462,7 @@ touchpad_has_horiz_edge_scroll_size(struct litest_device *dev) rc = libinput_device_get_size(dev->libinput_device, &width, &height); - return rc == 0 && height >= 50; + return rc == 0 && height >= 40; } START_TEST(touchpad_edge_scroll_horiz) ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: ignore hovering touches for the software button state
HI, On 01-03-17 02:48, Peter Hutterer wrote: If a touch started hovering in the main area, the button state would start with AREA and never move to the real button state, despite the finger triggering the pressure thresholds correctly in one of the areas. This could even happen across touch sequences if a touch went below pressure in the software button area, it changed to hovering and the button state changed to NONE. On the next event, the touch is still hovering and the current position of the touch is taken for the button state machine. https://bugs.freedesktop.org/show_bug.cgi?id=99976 Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-mt-touchpad-buttons.c | 2 +- test/test-touchpad-buttons.c| 32 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/evdev-mt-touchpad-buttons.c b/src/evdev-mt-touchpad-buttons.c index 176b431..895cf0e 100644 --- a/src/evdev-mt-touchpad-buttons.c +++ b/src/evdev-mt-touchpad-buttons.c @@ -457,7 +457,7 @@ tp_button_handle_state(struct tp_dispatch *tp, uint64_t time) struct tp_touch *t; tp_for_each_touch(tp, t) { - if (t->state == TOUCH_NONE) + if (t->state == TOUCH_NONE || t->state == TOUCH_HOVERING) continue; if (t->state == TOUCH_END) { diff --git a/test/test-touchpad-buttons.c b/test/test-touchpad-buttons.c index 820d8f0..0037ba7 100644 --- a/test/test-touchpad-buttons.c +++ b/test/test-touchpad-buttons.c @@ -1476,6 +1476,37 @@ START_TEST(clickpad_softbutton_right_to_left) } END_TEST +START_TEST(clickpad_softbutton_hover_into_buttons) +{ + struct litest_device *dev = litest_current_device(); + struct libinput *li = dev->libinput; + + litest_drain_events(li); + + litest_hover_start(dev, 0, 50, 50); + libinput_dispatch(li); + litest_hover_move_to(dev, 0, 50, 50, 90, 90, 10, 0); + libinput_dispatch(li); + + litest_touch_move_to(dev, 0, 90, 90, 91, 91, 1, 0); + + litest_button_click(dev, BTN_LEFT, true); + libinput_dispatch(li); + + litest_assert_button_event(li, + BTN_RIGHT, + LIBINPUT_BUTTON_STATE_PRESSED); + litest_assert_empty_queue(li); + + litest_button_click(dev, BTN_LEFT, false); + litest_touch_up(dev, 0); + + litest_assert_button_event(li, + BTN_RIGHT, + LIBINPUT_BUTTON_STATE_RELEASED); +} +END_TEST + START_TEST(clickpad_topsoftbuttons_left) { struct litest_device *dev = litest_current_device(); @@ -1962,6 +1993,7 @@ litest_setup_tests_touchpad_buttons(void) litest_add("touchpad:softbutton", clickpad_softbutton_left_2nd_fg_move, LITEST_CLICKPAD, LITEST_APPLE_CLICKPAD); litest_add("touchpad:softbutton", clickpad_softbutton_left_to_right, LITEST_CLICKPAD, LITEST_APPLE_CLICKPAD); litest_add("touchpad:softbutton", clickpad_softbutton_right_to_left, LITEST_CLICKPAD, LITEST_APPLE_CLICKPAD); + litest_add("touchpad:softbutton", clickpad_softbutton_hover_into_buttons, LITEST_CLICKPAD|LITEST_HOVER, LITEST_APPLE_CLICKPAD); litest_add("touchpad:topsoftbuttons", clickpad_topsoftbuttons_left, LITEST_TOPBUTTONPAD, LITEST_ANY); litest_add("touchpad:topsoftbuttons", clickpad_topsoftbuttons_right, LITEST_TOPBUTTONPAD, LITEST_ANY); ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: add elantech-specific pressure values
Hi, On 07-03-17 04:22, Peter Hutterer wrote: https://bugs.freedesktop.org/show_bug.cgi?id=99975 Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-mt-touchpad.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index c8e434e..e2866df 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -2382,9 +2382,14 @@ tp_init_pressure(struct tp_dispatch *tp, range = abs->maximum - abs->minimum; - /* Approximately the synaptics defaults */ - tp->pressure.high = abs->minimum + 0.12 * range; - tp->pressure.low = abs->minimum + 0.10 * range; + if (device->model_flags & EVDEV_MODEL_ELANTECH_TOUCHPAD) { + tp->pressure.high = 24; + tp->pressure.low = 10; + } else { + /* Approximately the synaptics defaults */ + tp->pressure.high = abs->minimum + 0.12 * range; + tp->pressure.low = abs->minimum + 0.10 * range; + } evdev_log_debug(device, "using pressure-based touch detection\n", ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: add pressure ranges for cyapa touchpads
Hi, On 22-03-17 06:52, Peter Hutterer wrote: On Tue, Mar 21, 2017 at 10:32:03AM +1000, Peter Hutterer wrote: https://bugs.freedesktop.org/show_bug.cgi?id=100122 Signed-off-by: Peter Hutterer --- src/evdev-mt-touchpad.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index e2866df..924e4f0 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -2385,6 +2385,9 @@ tp_init_pressure(struct tp_dispatch *tp, if (device->model_flags & EVDEV_MODEL_ELANTECH_TOUCHPAD) { tp->pressure.high = 24; tp->pressure.low = 10; + } else if (device->model_flags & EVDEV_MODEL_CYAPA) { + tp->pressure.high = 8; + tp->pressure.low = 6; updated locally to 10/8 at the tester's request LGTM: Reviewed-by: Hans de Goede Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: pull the tap exclusion zone down to the full edge zone
Hi, On 10-05-17 04:21, Peter Hutterer wrote: This was originally left outside of the button areas in case users tap in those zones, but we're getting false tap events in that zone. On a 100mm touchpad, the edge zone is merely 5mm, it's acceptable to ignore taps in that area even in the software button. We can revisit this if we see tap detection failures in the future. https://bugzilla.redhat.com/show_bug.cgi?id=1415796 Signed-off-by: Peter Hutterer Sounds reasonable to me and the code changes look good: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-mt-touchpad.c | 11 ++- test/test-touchpad.c| 8 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index e0757e17..17b14bc8 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -557,15 +557,8 @@ tp_palm_tap_is_palm(const struct tp_dispatch *tp, const struct tp_touch *t) t->point.x < tp->palm.right_edge) return false; - /* We're inside the left/right palm edge and not in one of the -* software button areas */ - if (t->point.y < tp->buttons.bottom_area.top_edge) { - evdev_log_debug(tp->device, - "palm: palm-tap detected\n"); - return true; - } - - return false; + evdev_log_debug(tp->device, "palm: palm-tap detected\n"); + return true; } static bool diff --git a/test/test-touchpad.c b/test/test-touchpad.c index 2731500a..d91c2449 100644 --- a/test/test-touchpad.c +++ b/test/test-touchpad.c @@ -1176,15 +1176,15 @@ START_TEST(touchpad_palm_detect_tap_softbuttons) litest_drain_events(li); - litest_touch_down(dev, 0, 95, 5); + litest_touch_down(dev, 0, 99, 99); litest_touch_up(dev, 0); litest_assert_empty_queue(li); - litest_touch_down(dev, 0, 5, 5); + litest_touch_down(dev, 0, 1, 99); litest_touch_up(dev, 0); litest_assert_empty_queue(li); - litest_touch_down(dev, 0, 5, 99); + litest_touch_down(dev, 0, 10, 99); litest_touch_up(dev, 0); litest_assert_button_event(li, BTN_LEFT, @@ -1194,7 +1194,7 @@ START_TEST(touchpad_palm_detect_tap_softbuttons) LIBINPUT_BUTTON_STATE_RELEASED); litest_assert_empty_queue(li); - litest_touch_down(dev, 0, 95, 99); + litest_touch_down(dev, 0, 90, 99); litest_touch_up(dev, 0); litest_assert_button_event(li, BTN_LEFT, ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH libinput 1/2] filter: Add timestamp smoothing support
Some devices, specifically some bluetooth touchpads generate quite unreliable timestamps for their events. The problem seems to be that (some of) these touchpads sample at aprox 90 Hz, but the bluetooth stack only communicates about every 30 ms (*) and then sends mutiple HID input reports in one batch. This results in 2-4 packets / SYNs every 30 ms. With timestamps really close together. The finger coordinate deltas in these packets change by aprox. the same amount between each packet when moving a finger at constant speed. But the time deltas are e.g. 28 ms, 1 ms, 1 ms resulting in calculate_tracker_velocity returning vastly different speeds for the 1st and 2nd packet, which in turn results in very "jerky" mouse pointer movement. *) Maybe it is waiting for a transmit time slot or some such. This commit adds support for a real simple timestamp smoothing algorithm, intended *only* for use with touchpads. Since touchpads will send a contineous stream of events at their sample rate when a finger is down, this filter simply assumes that any events which are under event_delta_smooth_threshold us apart are part of a smooth continuous stream of events with each event being event_delta_smooth_value us apart. Theoritically a very still finger may send the exact same coordinates and pressure twice, but even if this happens that is not a problem because a still finger generates coordinates changes below the hyst treshold so we ignore it anyways. Signed-off-by: Hans de Goede --- src/filter.c | 25 ++--- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/filter.c b/src/filter.c index 7c500f87..49d324eb 100644 --- a/src/filter.c +++ b/src/filter.c @@ -176,6 +176,10 @@ struct pointer_accelerator { double accel; /* unitless factor */ double incline; /* incline of the function */ + /* For smoothing timestamps from devices with unreliable timing */ + uint64_t event_delta_smooth_threshold; + uint64_t event_delta_smooth_value; + int dpi; }; @@ -227,14 +231,21 @@ tracker_by_offset(struct pointer_accelerator *accel, unsigned int offset) } static double -calculate_tracker_velocity(struct pointer_tracker *tracker, uint64_t time) +calculate_tracker_velocity(struct pointer_accelerator *accel, + struct pointer_tracker *tracker, uint64_t time) { - double tdelta = time - tracker->time + 1; - return hypot(tracker->delta.x, tracker->delta.y) / tdelta; /* units/us */ + uint64_t tdelta = time - tracker->time + 1; + + if (tdelta < accel->event_delta_smooth_threshold) + tdelta = accel->event_delta_smooth_value; + + return hypot(tracker->delta.x, tracker->delta.y) / + (double)tdelta; /* units/us */ } static inline double -calculate_velocity_after_timeout(struct pointer_tracker *tracker) +calculate_velocity_after_timeout(struct pointer_accelerator *accel, +struct pointer_tracker *tracker) { /* First movement after timeout needs special handling. * @@ -247,7 +258,7 @@ calculate_velocity_after_timeout(struct pointer_tracker *tracker) * for really slow movements but provides much more useful initial * movement in normal use-cases (pause, move, pause, move) */ - return calculate_tracker_velocity(tracker, + return calculate_tracker_velocity(accel, tracker, tracker->time + MOTION_TIMEOUT); } @@ -282,11 +293,11 @@ calculate_velocity(struct pointer_accelerator *accel, uint64_t time) /* Stop if too far away in time */ if (time - tracker->time > MOTION_TIMEOUT) { if (offset == 1) - result = calculate_velocity_after_timeout(tracker); + result = calculate_velocity_after_timeout(accel, tracker); break; } - velocity = calculate_tracker_velocity(tracker, time); + velocity = calculate_tracker_velocity(accel, tracker, time); /* Stop if direction changed */ dir &= tracker->dir; -- 2.13.0 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH libinput 2/2] evdev-mt-touchpad: Enable timestamp smoothing support for bluetooth touchpads
Bluetooth wreaks havoc with the timestamp of the input events coming from the touchpad, enable timestamp smoothing support to counter this. Signed-off-by: Hans de Goede --- src/evdev-mt-touchpad.c | 4 +++- src/filter.c| 6 +- src/filter.h| 4 +++- tools/ptraccel-debug.c | 2 +- 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index 2d39e18d..6af594dd 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -2056,8 +2056,10 @@ tp_init_accel(struct tp_dispatch *tp) if (tp->device->model_flags & EVDEV_MODEL_LENOVO_X230 || tp->device->model_flags & EVDEV_MODEL_LENOVO_X220_TOUCHPAD_FW81) filter = create_pointer_accelerator_filter_lenovo_x230(tp->device->dpi); + else if (libevdev_get_id_bustype(device->evdev) == BUS_BLUETOOTH) + filter = create_pointer_accelerator_filter_touchpad(device->dpi, ms2us(50), ms2us(10)); else - filter = create_pointer_accelerator_filter_touchpad(tp->device->dpi); + filter = create_pointer_accelerator_filter_touchpad(device->dpi, 0, 0); if (!filter) return false; diff --git a/src/filter.c b/src/filter.c index 49d324eb..faf8d311 100644 --- a/src/filter.c +++ b/src/filter.c @@ -1028,7 +1028,9 @@ struct motion_filter_interface accelerator_interface_touchpad = { }; struct motion_filter * -create_pointer_accelerator_filter_touchpad(int dpi) +create_pointer_accelerator_filter_touchpad(int dpi, + uint64_t event_delta_smooth_threshold, + uint64_t event_delta_smooth_value) { struct pointer_accelerator *filter; @@ -1038,6 +1040,8 @@ create_pointer_accelerator_filter_touchpad(int dpi) filter->base.interface = &accelerator_interface_touchpad; filter->profile = touchpad_accel_profile_linear; + filter->event_delta_smooth_threshold = event_delta_smooth_threshold; + filter->event_delta_smooth_value = event_delta_smooth_value; return &filter->base; } diff --git a/src/filter.h b/src/filter.h index e24c20d4..131f8018 100644 --- a/src/filter.h +++ b/src/filter.h @@ -114,7 +114,9 @@ struct motion_filter * create_pointer_accelerator_filter_linear_low_dpi(int dpi); struct motion_filter * -create_pointer_accelerator_filter_touchpad(int dpi); +create_pointer_accelerator_filter_touchpad(int dpi, + uint64_t event_delta_smooth_threshold, + uint64_t event_delta_smooth_value); struct motion_filter * create_pointer_accelerator_filter_lenovo_x230(int dpi); diff --git a/tools/ptraccel-debug.c b/tools/ptraccel-debug.c index acb82c69..1fc31de9 100644 --- a/tools/ptraccel-debug.c +++ b/tools/ptraccel-debug.c @@ -314,7 +314,7 @@ main(int argc, char **argv) filter = create_pointer_accelerator_filter_linear_low_dpi(dpi); profile = pointer_accel_profile_linear_low_dpi; } else if (streq(filter_type, "touchpad")) { - filter = create_pointer_accelerator_filter_touchpad(dpi); + filter = create_pointer_accelerator_filter_touchpad(dpi, 0, 0); profile = touchpad_accel_profile_linear; } else if (streq(filter_type, "x230")) { filter = create_pointer_accelerator_filter_lenovo_x230(dpi); -- 2.13.0 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH RFC libinput] filter: force touchpads to a fixed report rate
Hi, On 10-07-17 07:11, Peter Hutterer wrote: From: Hans de Goede Some devices, specifically some bluetooth touchpads generate quite unreliable timestamps for their events. The problem seems to be that (some of) these touchpads sample at aprox 90 Hz, but the bluetooth stack only communicates about every 30 ms (*) and then sends mutiple HID input reports in one batch. This results in 2-4 packets / SYNs every 30 ms. With timestamps really close together. The finger coordinate deltas in these packets change by aprox. the same amount between each packet when moving a finger at constant speed. But the time deltas are e.g. 28 ms, 1 ms, 1 ms resulting in calculate_tracker_velocity returning vastly different speeds for the 1st and 2nd packet, which in turn results in very "jerky" mouse pointer movement. *) Maybe it is waiting for a transmit time slot or some such. This commit adds support for a fixed report rate on touchpads. We take the assumption that a device with a fixed report rate will send events at that rate but those events may be delayed by the kernel or accelerated following some previous delay. Any timestamp that falls within the acceptable range (1.5 times the frequency) is set to the fixed report rate instead. This does not affect the event timestamps, it only applies to pointer acceleration. Signed-off-by: Hans de Goede Signed-off-by: Peter Hutterer --- This replaces Hans' two patches in this thread. Changes to Hans' patch: - both patches squashed together - instead of smoothing threshold/value assume a fixed report rate - save the fixed timestamp in the trackers Hans, please give this one a try. btw, what touchpad are we talking about here? This is the touchpad and the Bluetooth keyboard dock of an Asus T100CHI (with hid-asus patches which are pending upstream to put it in mt mode) It's the same principle, but instead of smoothing I figured we should just go all the way and encode the report rate, assume that's what the device will produce even if the kernel or some stack adds delays and adjust the timestamps for that report rate. There's a fixme that needs to be addressed still and obviously the 82Hz value measured on my device won't work for everyone so it needs to move into a udev property. I don't think there is much difference between our 2 techniques, as long as your code hits the: > + if (tdelta < accel->report_rate_tdelta * 1.5) > + time = last_time + tdelta; "time = last_time + tdelta;" path the result is the same. The only difference is that your code will miss that path sometimes when time stamps drift, which is unavoidable. But please give it try and let me know what you think I've just given this a try and sorry, but it is no good. The cursor goes from smooth with my original patch to smooth - jump - smooth again, doing the jump thingie about 1-2 times a second. It is slightly better then not having any patch at all, but not much. I really think my approach of simply assuming a fixed delta-t when calculating the speed as long as the measured delta-t is within certain bounds is better as it does not suffer from the time stamp drift issues your solution does. As for simply enabling this for all touchpads, that is something I considered too since other event delivery mechanisms will have some jitter too I decided to play it safe, but I do agree that it is probably a good idea to enable this for all touchpads. Also having the create_pointer_accelerator_filter_touchpad argument be a sample-rate/freq rather then a period-time is fine with me too. Regards, Hans src/evdev-mt-touchpad.c | 2 +- src/filter.c| 53 + src/filter.h| 3 ++- tools/ptraccel-debug.c | 2 +- 4 files changed, 49 insertions(+), 11 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index 0a4f4d98..b2968444 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -2140,7 +2140,7 @@ tp_init_accel(struct tp_dispatch *tp) tp->device->model_flags & EVDEV_MODEL_LENOVO_X220_TOUCHPAD_FW81) filter = create_pointer_accelerator_filter_lenovo_x230(tp->device->dpi); else - filter = create_pointer_accelerator_filter_touchpad(tp->device->dpi); + filter = create_pointer_accelerator_filter_touchpad(device->dpi, 82); if (!filter) return false; diff --git a/src/filter.c b/src/filter.c index 7c500f87..d470d633 100644 --- a/src/filter.c +++ b/src/filter.c @@ -176,6 +176,10 @@ struct pointer_accelerator { double accel; /* unitless factor */ double incline; /* incline of the function */ + /* For smoothing timestamps from devices with unreliable timing */ + unsigned int report_rate; /* in Hz */ + uint64_t report_rate_tdelta; /*
Re: [PATCH libinput] gestures: don't try to pinch for nfingers > slots
Hi, On 07/30/2017 05:20 PM, Peter Hutterer wrote: We don't know the position of the third finger on 2-slot touchpads, differing between swipe and pinch is reliable. Simply disable 3-finger pinch and always use swipe; 3fg pinch is uncommon anyway and it's better to have one of the gestures working reliably than both unreliably. Signed-off-by: Peter Hutterer I agree this is the best way to handle this: Acked-by: Hans de Goede Regards, Hans --- src/evdev-mt-touchpad-gestures.c | 7 +- test/test-gestures.c | 211 --- 2 files changed, 6 insertions(+), 212 deletions(-) diff --git a/src/evdev-mt-touchpad-gestures.c b/src/evdev-mt-touchpad-gestures.c index e3bca1f4..37d98fdc 100644 --- a/src/evdev-mt-touchpad-gestures.c +++ b/src/evdev-mt-touchpad-gestures.c @@ -334,6 +334,10 @@ tp_gesture_handle_state_unknown(struct tp_dispatch *tp, uint64_t time) if (tp->gesture.finger_count == 2) { tp_gesture_set_scroll_buildup(tp); return GESTURE_STATE_SCROLL; + /* more fingers than slots, don't bother with pinch, always +* assume swipe */ + } else if (tp->gesture.finger_count > tp->num_slots) { + return GESTURE_STATE_SWIPE; } /* for 3+ finger gestures, check if one finger is > 20mm @@ -356,7 +360,8 @@ tp_gesture_handle_state_unknown(struct tp_dispatch *tp, uint64_t time) /* If both touches are moving in the same direction assume * scroll or swipe */ - if (tp_gesture_same_directions(dir1, dir2)) { + if (tp->gesture.finger_count > tp->num_slots || + tp_gesture_same_directions(dir1, dir2)) { if (tp->gesture.finger_count == 2) { tp_gesture_set_scroll_buildup(tp); return GESTURE_STATE_SCROLL; diff --git a/test/test-gestures.c b/test/test-gestures.c index ce1012d4..e95d1a01 100644 --- a/test/test-gestures.c +++ b/test/test-gestures.c @@ -754,110 +754,6 @@ START_TEST(gestures_pinch_3fg) } END_TEST -START_TEST(gestures_pinch_3fg_btntool) -{ - struct litest_device *dev = litest_current_device(); - struct libinput *li = dev->libinput; - struct libinput_event *event; - struct libinput_event_gesture *gevent; - double dx, dy; - int cardinal = _i; /* ranged test */ - double dir_x, dir_y; - int i; - double scale, oldscale; - double angle; - int cardinals[8][2] = { - { 0, 30 }, - { 30, 30 }, - { 30, 0 }, - { 30, -30 }, - { 0, -30 }, - { -30, -30 }, - { -30, 0 }, - { -30, 30 }, - }; - - if (libevdev_get_num_slots(dev->evdev) > 2 || - !libinput_device_has_capability(dev->libinput_device, - LIBINPUT_DEVICE_CAP_GESTURE)) - return; - - dir_x = cardinals[cardinal][0]; - dir_y = cardinals[cardinal][1]; - - litest_drain_events(li); - - litest_touch_down(dev, 0, 50 + dir_x, 50 + dir_y); - litest_touch_down(dev, 1, 50 - dir_x, 50 - dir_y); - litest_event(dev, EV_KEY, BTN_TOOL_DOUBLETAP, 0); - litest_event(dev, EV_KEY, BTN_TOOL_TRIPLETAP, 1); - litest_event(dev, EV_SYN, SYN_REPORT, 0); - libinput_dispatch(li); - - for (i = 0; i < 8; i++) { - litest_push_event_frame(dev); - if (dir_x > 0.0) - dir_x -= 2; - else if (dir_x < 0.0) - dir_x += 2; - if (dir_y > 0.0) - dir_y -= 2; - else if (dir_y < 0.0) - dir_y += 2; - litest_touch_move(dev, - 0, - 50 + dir_x, - 50 + dir_y); - litest_touch_move(dev, - 1, - 50 - dir_x, - 50 - dir_y); - litest_pop_event_frame(dev); - libinput_dispatch(li); - } - - event = libinput_get_event(li); - gevent = litest_is_gesture_event(event, -LIBINPUT_EVENT_GESTURE_PINCH_BEGIN, -3); - dx = libinput_event_gesture_get_dx(gevent); - dy = libinput_event_gesture_get_dy(gevent); - scale = libinput_event_gesture_get_scale(gevent); - ck_assert(dx == 0.0); - ck_assert(dy == 0.0); - ck_assert(scale == 1.0); - - libinput_event_destroy(event); - - while ((event = libinput_get_event(li)) != NULL) { - gevent = litest_is_gesture_event(event, -
Re: [PATCH libinput] udev: add quirk for Chalkboard Electronics HID Touchscreen
Hi, On 19-09-17 18:41, Matt Porter wrote: The Chalkboard Electronics HID Touchscreen is classified as a tablet device by systemd udev because it has BTN_TOOL_PEN support. It also reports a resolution of 0 for both X and Y axes in absinfo. This causes libinput to reject it as an invalid tablet device. This quirk reclassifies the device as a touchscreen which allows it to be added as a valid input device. Signed-off-by: Matt Porter Have you considered adding special handling for this device to the kernel's hid drivers ? The hid subsys has sub drivers for device specific handling and if the device never actually has a pen, then the right fix would be to not make the kernel export this. Note I assume this is for a so called "smart" whiteboard for in classes ? If that is the case then the device may actually have a pen/stylus like device in some configuration. Some of these smart boards have what are basically empty whiteboard-marker in multiple colors and the device may report these as different styluses ... Regards, Hans --- udev/90-libinput-model-quirks.hwdb | 7 +++ 1 file changed, 7 insertions(+) diff --git a/udev/90-libinput-model-quirks.hwdb b/udev/90-libinput-model-quirks.hwdb index 72fcdca..6348f0c 100644 --- a/udev/90-libinput-model-quirks.hwdb +++ b/udev/90-libinput-model-quirks.hwdb @@ -72,6 +72,13 @@ libinput:name:*ETPS/2 Elantech Touchpad*:dmi:*svnASUSTeKCOMPUTERINC.:pnX555LAB:* LIBINPUT_MODEL_TOUCHPAD_VISIBLE_MARKER=1 ## +# Chalkboard Electronics +## +libinput:name:Chalkboard Electronics HID Touchscreen:dmi:* + ID_INPUT_TABLET=0 + ID_INPUT_TOUCHSCREEN=1 + +## # Chicony ## # Acer Hawaii Keyboard, uses Chicony VID ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] udev: add quirk for Chalkboard Electronics HID Touchscreen
Hi, On 21-09-17 16:00, Matt Porter wrote: On Thu, Sep 21, 2017 at 10:59:08AM +0200, Hans de Goede wrote: Hi, On 19-09-17 18:41, Matt Porter wrote: The Chalkboard Electronics HID Touchscreen is classified as a tablet device by systemd udev because it has BTN_TOOL_PEN support. It also reports a resolution of 0 for both X and Y axes in absinfo. This causes libinput to reject it as an invalid tablet device. This quirk reclassifies the device as a touchscreen which allows it to be added as a valid input device. Signed-off-by: Matt Porter Have you considered adding special handling for this device to the kernel's hid drivers ? The hid subsys has sub drivers for device specific handling and if the device never actually has a pen, then the right fix would be to not make the kernel export this. Note I assume this is for a so called "smart" whiteboard for in classes ? If that is the case then the device may actually have a pen/stylus like device in some configuration. Some of these smart boards have what are basically empty whiteboard-marker in multiple colors and the device may report these as different styluses ... I hadn't really considered that since this userspace fixup basically follows the pattern I see for other devices in systemd's hwdb. In this case (exporting a wrong event capability) a kernel fix seems more appropriate to me. In general we try to fix things as early in the chain as possible. hwdb quirks are more for things like: Yes this device supports semi-mt and the kernel exports that info, but the semi-mt coordinates are so unreliable that we really should just treat it as a single touch device. Anyways this is just my 2 cents, Peter what do you think, kernel fix or hwdb ? Just to clarify, these are not "smartboard" style devices. They are typical 7-14" touchscreen displays with HDMI/USB interface from https://www.chalk-elec.com/ Ok, that is good to know, really weird the export a BTN_TOOL_PEN then. Regards, Habs The updated patch for this is at https://github.com/systemd/systemd/pull/6880 based on Peter's earlier comments. It could be solved in the kernel as you suggest, however it appears that usually that approach is for very different hid devices with custom protocols (e.g. logitech-hidpp). I could suppress the pen tool and update absinfo in a hid-chalkboard.c if that's preferred. There could be an advantage here for those that aren't using systemd udev. -Matt ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: touch calibration on second screen impossible
Hi, On 25-09-17 08:08, Peter Hutterer wrote: On Sat, Sep 23, 2017 at 09:57:37AM +0200, Klaus Rudolph wrote: With X11 ( before wayland ) I can set up my touchscreen with: xinput --list find my device: "Advanced Silicon S.A. CoolTouch(TM) System" and can simply set the props I need with: xinput set-prop 'Advanced Silicon S.A. CoolTouch(TM) System' --type=float 'Coordinate Transformation Matrix' 0.533, 0, 0.467, 0, 1, 0, 0, 0, 1 But now, `xinput list` did not show my any real devices, only some mystery generic ones as this: ⎡ Virtual core pointer id=2[master pointer (3)] ⎜ ↳ Virtual core XTEST pointer id=4[slave pointer (2)] ⎜ ↳ xwayland-pointer:14 id=6[slave pointer (2)] ⎜ ↳ xwayland-relative-pointer:14 id=7[slave pointer (2)] ⎜ ↳ xwayland-touch:14id=9[slave pointer (2)] ⎣ Virtual core keyboardid=3[master keyboard (2)] ↳ Virtual core XTEST keyboard id=5[slave keyboard (3)] ↳ xwayland-keyboard:14 id=8[slave keyboard (3)] So I only see some mystery wayland pseudo devices. https://who-t.blogspot.com.au/2017/05/xinput-list-shows-xwayland-pointer.html With `libinput-list-devices` I can see my touch device, but I can not find any documentation how I can configure devices for libinput. All docs tell my that it can be done with `xinput` with is not true for wayland on fedora. Any idea? Q: How can I configure input devices on wayland in fedora 25. well, there's the LIBINPUT_CALIBRATION_MATRIX but it's not actually what you need here since you have multiscreen mapping issue. https://wayland.freedesktop.org/libinput/doc/latest/udev_config.html mutter should map the touchscreen to a single screen, does it not do that? This might be: https://bugzilla.gnome.org/show_bug.cgi?id=787884 Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi Yusuf, On 8/24/22 04:18, Yusuf Khan wrote: > Sorry for the necro-bump, I hadnt seen this go by No problem. > My main concern with this proposal is the phasing out of > /sys/class/backlight/. > Currently on the user(user, not userland) level its easier for me to just > modify > the file and be done with it. xbacklight doesnt tell me when its failed, > brightnessctl doesnt make assumptions about what device is what, and > other brightness setting applications ive seen are much worse than them. > Someone needs to create a userland application thats less inconvenient > than `echo`ing into /sys/class/backlight with a name that human beings can > actually remember before I stop using the sysfs, perhaps "setbrightness" > could be the binary's name? Also I dont think its wise to disable or make it > read only though Kconfig as older apps may depend on it, maybe add a > kernel param that disables the old interface so bigger distros can pressure > app makers into changing the interface? As a big draw for DDC/CI is that > many displays support it as a way to change brightness(even if you arent > doing anything special that would break the old interface) perhaps it could > be an early adopter to that kernel parameter? Right, so deprecating the /sys/class/backlight API definitely is the last step and probably is years away. As you say hiding / making it read-only should probably be a kernel-parameter at first, with maybe a Kconfig option to set the default. So the depcration would go like this: 1. Add: A kernel-parameter to allow hiding or read-only-ing the sysfs interface + Kconfig to select the default + dev_warn_once() when the old API is used 2. (much later) Drop the Kconfig option and default to hiding/read-only 3. (even later) Maybe completely remove the sysfs interface? Note the hiding vs read-only thing is to be decided. ATM I'm rather more focused on getting the new API in place then on deprecating the old one :) Anyways I fully agree that we need to do the deprecation carefully and slowly. This is likely going to take multiple years and then some ... Regards, Hans > > On Thu, Apr 7, 2022 at 10:39 AM Hans de Goede <mailto:hdego...@redhat.com>> wrote: > > As discussed already several times in the past: > Â https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/ > <https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/> > Â > https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.intel.com/ > > <https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.intel.com/> > > The current userspace API for brightness control offered by > /sys/class/backlight devices has various issues, the biggest 2 being: > > 1. There is no way to map the backlight device to a specific > Â Â display-output / panel (1) > 2. Controlling the brightness requires root-rights requiring > Â Â desktop-environments to use suid-root helpers for this. > > As already discussed on various conference's hallway tracks > and as has been proposed on the dri-devel list once before (2), > it seems that there is consensus that the best way to to solve these > 2 issues is to add support for controlling a video-output's brightness > through properties on the drm_connector. > > This RFC outlines my plan to try and actually implement this, > which has 3 phases: > > > Phase 1: Stop registering multiple /sys/class/backlight devs for a single > display > > = > > On x86 there can be multiple firmware + direct-hw-access methods > for controlling the backlight and in some cases the kernel registers > multiple backlight-devices for a single internal laptop LCD panel: > > a) i915 and nouveau unconditionally register their "native" backlight dev > Â Â even on devices where /sys/class/backlight/acpi_video0 must be used > Â Â to control the backlight, relying on userspace to prefer the "firmware" > Â Â acpi_video0 device over "native" devices. > b) amdgpu and nouveau rely on the acpi_video driver initializing before > Â Â them, which currently causes /sys/class/backlight/acpi_video0 to > usually > Â Â show up and then they register their own native backlight driver after > Â Â which the drivers/acpi/video_detect.c code unregisters the acpi_video0 > Â Â device. This means that userspace briefly sees 2 devices and the > Â Â disappearing of acpi_video0 after a brief time confuses the systemd > Â Â backlight level save/restore code, see e.g.: > Â Â https://bbs.archlinux.org/viewtopic.php?id=269920 > <https:/
Re: [RFC] drm/kms: control display brightness through drm_connector properties
Hi, On 8/25/22 23:40, Yusuf Khan wrote: > Perhaps the Kconfig modifications could be postponed to stage 2 > since for people running distros that suddenly decide to disable > /sys/class/backlight/ it may be impractical for them to recompile > their kernels and such. In step 1, the Kconfig option is just there to select the default setting of the kernel commandline parameter. So when a distro defaults that to disabling /sys/class/backlight (or making it read-only) then the user can simple override it on the kernel commandline. No re-compiling of kernels needed. > Also stage 2 should probably take ~2 decades > until it comes into being, for reference fbdev SPECIFIC drivers > were removed from fedora just recently and because of that there > were some issues with some user's systems. I understand it's much > easier to change from the /sys/class/backlight/ interface to the one > you have proposed than to change from fbdev to KMS though. Yes chances are we will be stuck with the old sysfs API for a long time to come. Note that since in some cases the backlight driver is not part of the GPU driver, but rather part of e.g. dell-laptop we will need the backlight-device abstraction in the kernel going forward regardless of what happens with /sys/class/backlight. So the cleanup resulting from removing it completely will not be that big as the backlight-device abstraction will stay it will only be the sysfs interface which disappears. As such just having a kernel cmdline parameter to hide/unhide it might be good enough. Regards, Hans > > On Thu, Aug 25, 2022 at 3:27 AM Hans de Goede <mailto:hdego...@redhat.com>> wrote: > > Hi Yusuf, > > On 8/24/22 04:18, Yusuf Khan wrote: > > Sorry for the necro-bump, I hadnt seen this go by > > No problem. > > > My main concern with this proposal is the phasing out of > /sys/class/backlight/. > > Currently on the user(user, not userland) level its easier for me to > just modify > > the file and be done with it. xbacklight doesnt tell me when its failed, > > brightnessctl doesnt make assumptions about what device is what, and > > other brightness setting applications ive seen are much worse than them. > > Someone needs to create a userland application thats less inconvenient > > than `echo`ing into /sys/class/backlight with a name that human beings > can > > actually remember before I stop using the sysfs, perhaps "setbrightness" > > could be the binary's name? Also I dont think its wise to disable or > make it > > read only though Kconfig as older apps may depend on it, maybe add a > > kernel param that disables the old interface so bigger distros can > pressure > > app makers into changing the interface? As a big draw for DDC/CI is that > > many displays support it as a way to change brightness(even if you arent > > doing anything special that would break the old interface) perhaps it > could > > be an early adopter to that kernel parameter? > > Right, so deprecating the /sys/class/backlight API definitely is the last > step and probably is years away. As you say hiding / making it read-only > should probably be a kernel-parameter at first, with maybe a Kconfig > option to set the default. So the depcration would go like this: > > 1. Add: > A kernel-parameter to allow hiding or read-only-ing the sysfs interface + > Kconfig to select the default + > dev_warn_once() when the old API is used > > 2. (much later) Drop the Kconfig option and default to hiding/read-only > > 3. (even later) Maybe completely remove the sysfs interface? > > Note the hiding vs read-only thing is to be decided. ATM I'm rather more > focused on getting the new API in place then on deprecating the old one :) > > Anyways I fully agree that we need to do the deprecation carefully and > slowly. This is likely going to take multiple years and then some ... > > Regards, > > Hans > > > > > > > On Thu, Apr 7, 2022 at 10:39 AM Hans de Goede <mailto:hdego...@redhat.com> <mailto:hdego...@redhat.com > <mailto:hdego...@redhat.com>>> wrote: > > > >Â Â Â As discussed already several times in the past: > >Â Â Â Â https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/ > <https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/> > <https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/ > <https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/>> > >Â Â Â Â > https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.in
[RFC v2] drm/kms: control display brightness through drm_connector properties
Hi all, Here is v2 of my "drm/kms: control display brightness through drm_connector properties" RFC: Changes from version 1: - Drop bl_brightness_0_is_min_brightness from list of new connector properties. - Clearly define that 0 is always min-brightness when setting the brightness through the connector properties. - Drop bl_brightness_control_method from list of new connector properties. - Phase 1 of the plan has been completed As discussed already several times in the past: https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/ https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.intel.com/ The current userspace API for brightness control offered by /sys/class/backlight devices has various issues: 1. There is no way to map the backlight device to a specific display-output / panel (1) 2. Controlling the brightness requires root-rights requiring desktop-environments to use suid-root helpers for this. 3. The meaning of 0 is not clearly defined, it can be either off, or minimum brightness at which the display is still readable (in a low light environment) 4. It's not possible to change both the gamma and the brightness in the same KMS atomic commit. You'd want to be able to reduce brightness to conserve power, and counter the effects of that by changing gamma to reach a visually similar image. And you'd want to have the changes take effect at the same time instead of reducing brightness at some frame and change gamma at some other frame. This is pretty much impossible to do via the sysfs interface. As already discussed on various conference's hallway tracks and as has been proposed on the dri-devel list once before (2), it seems that there is consensus that the best way to to solve these 2 issues is to add support for controlling a video-output's brightness through properties on the drm_connector. This RFC outlines my plan to try and actually implement this, which has 3 phases: Phase 1: Stop registering multiple /sys/class/backlight devs for a single display = On x86 there can be multiple firmware + direct-hw-access methods for controlling the backlight and in some cases the kernel registers multiple backlight-devices for a single internal laptop LCD panel. A plan to fix this was posted here: https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343...@redhat.com/ And a pull-req actually implementing this plan has been send out this week: https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1...@redhat.com/ Phase 2: Add drm_connector properties mirroring the matching backlight device = The plan is to add a drm_connector helper function, which optionally takes a pointer to the backlight device for the GPU's native backlight device, which will then mirror the backlight settings from the backlight device in a set of read/write brightness* properties on the connector. This function can then be called by GPU drivers for the drm_connector for the internal panel and it will then take care of everything. When there is no native GPU backlight device, or when it should not be used then (on x86) the helper will use the acpi_video_get_backlight_type() to determine which backlight-device should be used instead and it will find + mirror that one. Phase 3: Deprecate /sys/class/backlight uAPI Once most userspace has moved over to using the new drm_connector brightness props, a Kconfig option can be added to stop exporting the backlight-devices under /sys/class/backlight. The plan is to just disable the sysfs interface and keep the existing backlight-device internal kernel abstraction as is, since some abstraction for (non GPU native) backlight devices will be necessary regardless. It is unsure if we will ever be able to do this. For example people using non fully integrated desktop environments like e.g. sway often use custom scripts binded to hotkeys to get functionality like the brightness up/down keyboard hotkeys changing the brightness. This typically involves e.g. the xbacklight utility. Even if the xbacklight utility is ported to use kms with the new connector object brightness properties then this still will not work because changing the properties will require drm-master rights and e.g. sway will already hold those. The drm_connector brightness properties === The new uAPI for this consists of 2 properties: 1. "display brightness": rw 0-int32_max property controlling the brightness setting of the connected display. The actual maximum of this will be less then int32_max and is given in "display brightness max". Unlike the /sys/class/backlight/foo/brightness this brightness property has a clear definition for the value 0. The kernel must ensure that 0 means minimum brightness
Meeting (BOF) at Plumbers Dublin to discuss backlight brightness as connector object property RFC?
Hi All, I will be at Plumbers Dublin next week and I was wondering if anyone interested in this wants to get together for a quick discussion / birds of a feather session about this? I have just posted version 2 of the RFC: https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86...@redhat.com/T/#u If you are interested in meeting up please send me an email off-list (no need to spam the list further with this) also please let me know if there are any times which do not work for you, and/or times which have your preference. I don't have a specific room/time for this yet, but if people are interested I will try to get a room from the organization and if that does not work out I'm sure we will figure something out. One of the things which I would like to discuss is using the backlight brightness as connector object property vs external (not part of the compositor) tools to control the brightness like e.g. xbacklight, quoting from the RFC: "people using non fully integrated desktop environments like e.g. sway often use custom scripts binded to hotkeys to get functionality like the brightness up/down keyboard hotkeys changing the brightness. This typically involves e.g. the xbacklight utility. Even if the xbacklight utility is ported to use kms with the new connector object brightness properties then this still will not work because changing the properties will require drm-master rights and e.g. sway will already hold those." Note one obvious solution here would be for these use-cases to keep using the old /sys/class/backlight interface for this, with the downside that we will then be stuck to that interface for ever... Regards, Hans
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
Hi, On 9/9/22 15:39, Simon Ser wrote: > On Friday, September 9th, 2022 at 12:12, Hans de Goede > wrote: > >> Phase 3: Deprecate /sys/class/backlight uAPI >> >> >> Once most userspace has moved over to using the new drm_connector >> brightness props, a Kconfig option can be added to stop exporting >> the backlight-devices under /sys/class/backlight. The plan is to >> just disable the sysfs interface and keep the existing backlight-device >> internal kernel abstraction as is, since some abstraction for (non GPU >> native) backlight devices will be necessary regardless. >> >> It is unsure if we will ever be able to do this. For example people using >> non fully integrated desktop environments like e.g. sway often use custom >> scripts binded to hotkeys to get functionality like the brightness >> up/down keyboard hotkeys changing the brightness. This typically involves >> e.g. the xbacklight utility. >> >> Even if the xbacklight utility is ported to use kms with the new connector >> object brightness properties then this still will not work because >> changing the properties will require drm-master rights and e.g. sway will >> already hold those. > > I replied to this here in another thread [1]. > > tl;dr I think it would be fine even for Sway-like compositors. Ok, that is good to know. > (Also note the utilities used right now are not xbacklight, but > brightnessctl/light/brillo/etc [2]) Ah I thought that xbacklight got patched at one point to support the sysfs API, but I see now that instead alternative utilities have popped up. Regards, Hans > > [1]: > https://lore.kernel.org/dri-devel/bZJU9OkYWFyaLHVa4XUE4d5iBTPFXBRyPe1wMd_ztKh5VBMu-EDNGoUDpvwtFn_u9-JMvN8QmIZVS3pzMZM_hZTkTCA9gOBnCGXc5HFmsnc=@emersion.fr/ > [2]: https://github.com/swaywm/sway/wiki#xbacklight > >> The drm_connector brightness properties >> === >> >> The new uAPI for this consists of 2 properties: >> >> 1. "display brightness": rw 0-int32_max property controlling the brightness >> setting >> of the connected display. The actual maximum of this will be less then >> int32_max and is given in "display brightness max". >> >> Unlike the /sys/class/backlight/foo/brightness this brightness property >> has a clear definition for the value 0. The kernel must ensure that 0 >> means minimum brightness (so 0 should never turn the backlight off). >> If necessary the kernel must enforce a minimum value by adding >> an offset to the value seen in the property to ensure this behavior. >> >> For example if necessary the driver must clamp 0-255 to 10-255, which then >> becomes 0-245 on the brightness property, adding 10 internally to writes >> done to the brightness property. This adding of an extra offset when >> necessary must only be done on the brightness property, >> the /sys/class/backlight interface should be left unchanged to not break >> userspace which may rely on 0 = off on some systems. >> >> Note amdgpu already does something like this even for /sys/class/backlight, >> see the use of AMDGPU_DM_DEFAULT_MIN_BACKLIGHT in amdgpu. >> >> Also whenever possible the kernel must ensure that the brightness range >> is in perceived brightness, but this cannot always be guaranteed. >> >> >> 2. "display brightness max": ro 0-int32_max property giving the actual >> maximum >> of the display's brightness setting. This will report 0 when brightness >> control is not available. >> >> The value of "display brightness max" may change at runtime, either by >> a legacy drivers/platform/x86 backlight driver loading after the drm >> driver has loaded; or when plugging in a monitor which allows brightness >> control over DDC/CI. In both these cases the max value will change from 0 >> to the actual max value, indicating backlight control has become available >> on this connector. > > The kernel will need to ensure that a hotplug uevent is sent to > user-space at this point. Otherwise user-space has no way to figure out > that the prop has changed. > > Overall this all looks very solid to me! > > Simon >
Re: Meeting (BOF) at Plumbers Dublin to discuss backlight brightness as connector object property RFC?
Hi All, On 9/9/22 12:23, Hans de Goede wrote: > Hi All, > > I will be at Plumbers Dublin next week and I was wondering if > anyone interested in this wants to get together for a quick > discussion / birds of a feather session about this? > > I have just posted version 2 of the RFC: > https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86...@redhat.com/T/#u > > If you are interested in meeting up please send me > an email off-list (no need to spam the list further with this) > also please let me know if there are any times which do not > work for you, and/or times which have your preference. > > I don't have a specific room/time for this yet, but if people > are interested I will try to get a room from the organization > and if that does not work out I'm sure we will figure something > out. I have a date, time and location for this now. The BOF session is scheduled on Monday September 12th in the "Meeting 9" room: https://lpc.events/event/16/contributions/1390/ Regards, Hans
Wrong (non modified) key under Wayland when multiple events combined in single SYN_REPORT
Hi All, Juerd (in the Cc) reported the following problem to me at our local hackerspace. During a big hacker event in the Netherlands this summer (MCH) the logistics team used custom barcodes to keep track of inventory. These custom barcodes contain a # symbol. They noticed that with the USB-keyboard emulating barcodescanner they were using the # would sometimes turn into a 3, but only when running under Wayland. Juerd, we did not discuss how you were running Wayland (which compositor), I guess you were using GNOME3 when you hit this ? We reproduced the problem at the local hackerspace (with GNOME3) and the following evdev frame: E: 13.283445 0004 0004 458977 # EV_MSC / MSC_SCAN 458977 E: 13.283445 0001 002a 0001 # EV_KEY / KEY_LEFTSHIFT1 E: 13.283445 0004 0004 458775 # EV_MSC / MSC_SCAN 458775 E: 13.283445 0001 0014 # EV_KEY / KEY_T0 E: 13.283445 0004 0004 458784 # EV_MSC / MSC_SCAN 458784 E: 13.283445 0001 0004 0001 # EV_KEY / KEY_31 E: 13.283445 # SYN_REPORT (0) -- +2ms Triggers the problem. Note that the T key release from the previous char in the barcode has been added to the same evdev frame as the shift + 3 for reporting the # (the next char in the barcode). This seems to be what triggers this problem. I have attached a full evemu-record-ing of a couple of scans of the troublesome barcode. Replaying this with a terminal in GNOME3 wayland open should reproduce (I have not tried this). Note that not all barcodes in the recording trigger the bug. I'm not sure if this is a libinput or GNOME3 issue, so I've send this email to a bunch of different people + wayland-devel. Regards, Hans # EVEMU 1.3 # Kernel: 5.19.0+ # DMI: dmi:bvnLENOVO:bvrN2WET30W(1.20):bd08/26/2021:br1.20:efr1.13:svnLENOVO:pn20U90SIT19:pvrThinkPadX1CarbonGen8:rvnLENOVO:rn20U90SIT19:rvrNotDefined:cvnLENOVO:ct10:cvrNone:skuLENOVO_MT_20U9_BU_Think_FM_ThinkPadX1CarbonGen8: # Input device name: "Symbol Technologies, Inc, 2008 Symbol Bar Code Scanner" # Input device ID: bus 0x03 vendor 0x5e0 product 0x1200 version 0x110 # Supported events: # Event type 0 (EV_SYN) # Event code 0 (SYN_REPORT) # Event code 1 (SYN_CONFIG) # Event code 2 (SYN_MT_REPORT) # Event code 3 (SYN_DROPPED) # Event code 4 ((null)) # Event code 5 ((null)) # Event code 6 ((null)) # Event code 7 ((null)) # Event code 8 ((null)) # Event code 9 ((null)) # Event code 10 ((null)) # Event code 11 ((null)) # Event code 12 ((null)) # Event code 13 ((null)) # Event code 14 ((null)) # Event code 15 (SYN_MAX) # Event type 1 (EV_KEY) # Event code 1 (KEY_ESC) # Event code 2 (KEY_1) # Event code 3 (KEY_2) # Event code 4 (KEY_3) # Event code 5 (KEY_4) # Event code 6 (KEY_5) # Event code 7 (KEY_6) # Event code 8 (KEY_7) # Event code 9 (KEY_8) # Event code 10 (KEY_9) # Event code 11 (KEY_0) # Event code 12 (KEY_MINUS) # Event code 13 (KEY_EQUAL) # Event code 14 (KEY_BACKSPACE) # Event code 15 (KEY_TAB) # Event code 16 (KEY_Q) # Event code 17 (KEY_W) # Event code 18 (KEY_E) # Event code 19 (KEY_R) # Event code 20 (KEY_T) # Event code 21 (KEY_Y) # Event code 22 (KEY_U) # Event code 23 (KEY_I) # Event code 24 (KEY_O) # Event code 25 (KEY_P) # Event code 26 (KEY_LEFTBRACE) # Event code 27 (KEY_RIGHTBRACE) # Event code 28 (KEY_ENTER) # Event code 29 (KEY_LEFTCTRL) # Event code 30 (KEY_A) # Event code 31 (KEY_S) # Event code 32 (KEY_D) # Event code 33 (KEY_F) # Event code 34 (KEY_G) # Event code 35 (KEY_H) # Event code 36 (KEY_J) # Event code 37 (KEY_K) # Event code 38 (KEY_L) # Event code 39 (KEY_SEMICOLON) # Event code 40 (KEY_APOSTROPHE) # Event code 41 (KEY_GRAVE) # Event code 42 (KEY_LEFTSHIFT) # Event code 43 (KEY_BACKSLASH) # Event code 44 (KEY_Z) # Event code 45 (KEY_X) # Event code 46 (KEY_C) # Event code 47 (KEY_V) # Event code 48 (KEY_B) # Event code 49 (KEY_N) # Event code 50 (KEY_M) # Event code 51 (KEY_COMMA) # Event code 52 (KEY_DOT) # Event code 53 (KEY_SLASH) # Event code 54 (KEY_RIGHTSHIFT) # Event code 55 (KEY_KPASTERISK) # Event code 56 (KEY_LEFTALT) # Event code 57 (KEY_SPACE) # Event code 58 (KEY_CAPSLOCK) # Event code 59 (KEY_F1) # Event code 60 (KEY_F2) # Event code 61 (KEY_F3) # Event code 62 (KEY_F4) # Event code 63 (KEY_F5) # Event code 64 (KEY_F6) # Event code 65 (KEY_F7) # Event code 66 (KEY_F8) # Event code 67 (KEY_F9) # Event code 68 (KEY_F10) # Event code 69 (KEY_NUMLOCK) # Event code 70 (KEY_SCROLLLOCK) # Event code 71 (KEY_KP7) # Event code 72 (KEY_KP8) # Event code 73 (KEY_KP9) # Event code 74 (KEY_KPMINUS) # Event code 75 (KEY_KP4) # Event code 76
Re: Wrong (non modified) key under Wayland when multiple events combined in single SYN_REPORT
Hi, On 9/12/22 23:20, Peter wrote: > Hi all, > > > Op maandag 12 september 2022 om 15:14:09 +0200 schreef Juerd Waalboer > : >> Hans de Goede skribis 2022-09-12Â 7:16 (+0200): >>> During a big hacker event in the Netherlands this summer (MCH) the logistics >>> team used custom barcodes to keep track of inventory. These custom barcodes >>> contain a # symbol. >> >> In other barcodes, @ symbols. Quite possibly anything with shifted >> characters; I vaguely recall a mixed case (ascii) string where the >> uppercasing was on the wrong letter. > > Yes, that definitely also happened. > >> >>> Juerd, we did not discuss how you were running Wayland (which compositor), >>> I guess you were using GNOME3 when you hit this ? >> >> I'm not sure, as I only encountered the bug as an end user and suggested >> changing to X to work around it (which worked). I've added Peter Hazenberg >> to the CC list; he installed and maintained the computers, and is familiar >> with the bug. Peter, can you confirm that we were using GNOME 3 in both >> Wayland and X? > > Yes, we used gnome 3. It was mostly a boring default Fedora 36 Workstation > installation. > > Good to hear Hans already reproduced the issue at the mentioned hackerspace, > I assume with the exact same hardware Yes I reproduced it on my own laptop inside a terminal under GNOME3. I suspect that it reproduces on any (VTE based?) terminal running under GNOME3 Wayland when using the right barcode-scanner model and scanning specific barcodes. . Regards, Hans
Re: Wrong (non modified) key under Wayland when multiple events combined in single SYN_REPORT
Hi, On 9/13/22 12:28, Carlos Garnacho wrote: > Hi!, > > On Tue, Sep 13, 2022 at 11:36 AM Hans de Goede wrote: >> >> Hi, >> >> On 9/12/22 23:20, Peter wrote: >>> Hi all, >>> >>> >>> Op maandag 12 september 2022 om 15:14:09 +0200 schreef Juerd Waalboer >>> : >>>> Hans de Goede skribis 2022-09-12 7:16 (+0200): >>>>> During a big hacker event in the Netherlands this summer (MCH) the >>>>> logistics >>>>> team used custom barcodes to keep track of inventory. These custom >>>>> barcodes >>>>> contain a # symbol. >>>> >>>> In other barcodes, @ symbols. Quite possibly anything with shifted >>>> characters; I vaguely recall a mixed case (ascii) string where the >>>> uppercasing was on the wrong letter. >>> >>> Yes, that definitely also happened. >>> >>>> >>>>> Juerd, we did not discuss how you were running Wayland (which compositor), >>>>> I guess you were using GNOME3 when you hit this ? >>>> >>>> I'm not sure, as I only encountered the bug as an end user and suggested >>>> changing to X to work around it (which worked). I've added Peter Hazenberg >>>> to the CC list; he installed and maintained the computers, and is familiar >>>> with the bug. Peter, can you confirm that we were using GNOME 3 in both >>>> Wayland and X? >>> >>> Yes, we used gnome 3. It was mostly a boring default Fedora 36 Workstation >>> installation. >>> >>> Good to hear Hans already reproduced the issue at the mentioned >>> hackerspace, I assume with the exact same hardware >> >> Yes I reproduced it on my own laptop inside a terminal under GNOME3. I >> suspect that it reproduces on any (VTE based?) terminal running under GNOME3 >> Wayland when using the right barcode-scanner model and scanning specific >> barcodes. > > Thanks Hans/Juerd/Peter. I can reproduce this issue on GNOME Shell > with the evemu logs provided. From my multiple tries, the libinput > debug output seems pretty much consistent and in line with the evemu > output (i.e. press shift, release 't', press '3', release shift), so > there does not seem to be any issue there. At the wayland level, the > wl_keyboard.modifier events received by the client are somewhat amiss > though. > > Amusingly, running on plain Mutter (e.g. `mutter --wayland > --display-server` on a TTY) does also seem to fix the issue. The most > immediate difference I can think of is the involvement of input > methods. > > Since this does not seem to be a generic Wayland issue, feel free to > file a bug at https://gitlab.gnome.org/GNOME/gnome-shell/-/issues and > move discussion there, this might still end up in Mutter, or in IBus, > unclear yet. Thanks, issue filed: https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/5890 Regards, Hans
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
Hi, On 9/28/22 12:04, Jani Nikula wrote: > On Fri, 09 Sep 2022, Hans de Goede wrote: >> Hi all, >> >> Here is v2 of my "drm/kms: control display brightness through drm_connector >> properties" RFC: >> >> Changes from version 1: >> - Drop bl_brightness_0_is_min_brightness from list of new connector >> properties. >> - Clearly define that 0 is always min-brightness when setting the brightness >> through the connector properties. >> - Drop bl_brightness_control_method from list of new connector >> properties. >> - Phase 1 of the plan has been completed >> >> As discussed already several times in the past: >> https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/ >> >> https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.intel.com/ >> >> The current userspace API for brightness control offered by >> /sys/class/backlight devices has various issues: >> >> 1. There is no way to map the backlight device to a specific >>display-output / panel (1) >> 2. Controlling the brightness requires root-rights requiring >>desktop-environments to use suid-root helpers for this. >> 3. The meaning of 0 is not clearly defined, it can be either off, >>or minimum brightness at which the display is still readable >>(in a low light environment) >> 4. It's not possible to change both the gamma and the brightness in the >>same KMS atomic commit. You'd want to be able to reduce brightness to >>conserve power, and counter the effects of that by changing gamma to >>reach a visually similar image. And you'd want to have the changes take >>effect at the same time instead of reducing brightness at some frame and >>change gamma at some other frame. This is pretty much impossible to do >>via the sysfs interface. >> >> As already discussed on various conference's hallway tracks >> and as has been proposed on the dri-devel list once before (2), >> it seems that there is consensus that the best way to to solve these >> 2 issues is to add support for controlling a video-output's brightness >> through properties on the drm_connector. >> >> This RFC outlines my plan to try and actually implement this, >> which has 3 phases: >> >> >> Phase 1: Stop registering multiple /sys/class/backlight devs for a single >> display >> = >> >> On x86 there can be multiple firmware + direct-hw-access methods >> for controlling the backlight and in some cases the kernel registers >> multiple backlight-devices for a single internal laptop LCD panel. >> >> A plan to fix this was posted here: >> https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343...@redhat.com/ >> And a pull-req actually implementing this plan has been send out this week: >> https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1...@redhat.com/ >> >> >> Phase 2: Add drm_connector properties mirroring the matching backlight device >> = >> >> The plan is to add a drm_connector helper function, which optionally takes >> a pointer to the backlight device for the GPU's native backlight device, >> which will then mirror the backlight settings from the backlight device >> in a set of read/write brightness* properties on the connector. >> >> This function can then be called by GPU drivers for the drm_connector for >> the internal panel and it will then take care of everything. When there >> is no native GPU backlight device, or when it should not be used then >> (on x86) the helper will use the acpi_video_get_backlight_type() to >> determine which backlight-device should be used instead and it will find >> + mirror that one. >> >> >> Phase 3: Deprecate /sys/class/backlight uAPI >> >> >> Once most userspace has moved over to using the new drm_connector >> brightness props, a Kconfig option can be added to stop exporting >> the backlight-devices under /sys/class/backlight. The plan is to >> just disable the sysfs interface and keep the existing backlight-device >> internal kernel abstraction as is, since some abstraction for (non GPU >> native) backlight devices will be necessary regardless. >> >> It is unsure if we will ever be able to do this. For example people using >> non fully integrated desktop environments like e.g. sway often use cust
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
Hi, On 9/28/22 12:57, Ville Syrjälä wrote: > On Wed, Sep 28, 2022 at 01:04:01PM +0300, Jani Nikula wrote: >> On Fri, 09 Sep 2022, Hans de Goede wrote: >>> Hi all, >>> >>> Here is v2 of my "drm/kms: control display brightness through drm_connector >>> properties" RFC: >>> >>> Changes from version 1: >>> - Drop bl_brightness_0_is_min_brightness from list of new connector >>> properties. >>> - Clearly define that 0 is always min-brightness when setting the brightness >>> through the connector properties. >>> - Drop bl_brightness_control_method from list of new connector >>> properties. >>> - Phase 1 of the plan has been completed >>> >>> As discussed already several times in the past: >>> https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/ >>> >>> https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.intel.com/ >>> >>> The current userspace API for brightness control offered by >>> /sys/class/backlight devices has various issues: >>> >>> 1. There is no way to map the backlight device to a specific >>>display-output / panel (1) >>> 2. Controlling the brightness requires root-rights requiring >>>desktop-environments to use suid-root helpers for this. >>> 3. The meaning of 0 is not clearly defined, it can be either off, >>>or minimum brightness at which the display is still readable >>>(in a low light environment) >>> 4. It's not possible to change both the gamma and the brightness in the >>>same KMS atomic commit. You'd want to be able to reduce brightness to >>>conserve power, and counter the effects of that by changing gamma to >>>reach a visually similar image. And you'd want to have the changes take >>>effect at the same time instead of reducing brightness at some frame and >>>change gamma at some other frame. This is pretty much impossible to do >>>via the sysfs interface. >>> >>> As already discussed on various conference's hallway tracks >>> and as has been proposed on the dri-devel list once before (2), >>> it seems that there is consensus that the best way to to solve these >>> 2 issues is to add support for controlling a video-output's brightness >>> through properties on the drm_connector. >>> >>> This RFC outlines my plan to try and actually implement this, >>> which has 3 phases: >>> >>> >>> Phase 1: Stop registering multiple /sys/class/backlight devs for a single >>> display >>> = >>> >>> On x86 there can be multiple firmware + direct-hw-access methods >>> for controlling the backlight and in some cases the kernel registers >>> multiple backlight-devices for a single internal laptop LCD panel. >>> >>> A plan to fix this was posted here: >>> https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343...@redhat.com/ >>> And a pull-req actually implementing this plan has been send out this week: >>> https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1...@redhat.com/ >>> >>> >>> Phase 2: Add drm_connector properties mirroring the matching backlight >>> device >>> = >>> >>> The plan is to add a drm_connector helper function, which optionally takes >>> a pointer to the backlight device for the GPU's native backlight device, >>> which will then mirror the backlight settings from the backlight device >>> in a set of read/write brightness* properties on the connector. >>> >>> This function can then be called by GPU drivers for the drm_connector for >>> the internal panel and it will then take care of everything. When there >>> is no native GPU backlight device, or when it should not be used then >>> (on x86) the helper will use the acpi_video_get_backlight_type() to >>> determine which backlight-device should be used instead and it will find >>> + mirror that one. >>> >>> >>> Phase 3: Deprecate /sys/class/backlight uAPI >>> >>> >>> Once most userspace has moved over to using the new drm_connector >>> brightness props, a Kconfig option can be added to stop exporting >>> the backlight-devices under /sys/class/backl
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
Hi, On 9/30/22 18:17, Sebastian Wick wrote: > On Fri, Sep 30, 2022 at 5:27 PM Pekka Paalanen wrote: >> >> On Fri, 30 Sep 2022 17:44:17 +0300 >> Ville Syrjälä wrote: >> >>> On Fri, Sep 30, 2022 at 04:20:29PM +0200, Sebastian Wick wrote: On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen wrote: > > On Thu, 29 Sep 2022 20:06:50 +0200 > Sebastian Wick wrote: > >> If it is supposed to be a non-linear luminance curve, which one is it? >> It would be much clearer if user space can control linear luminance >> and use whatever definition of perceived brightness it wants. The >> obvious downside of it is that it requires bits to encode changes that >> users can't perceive. What about backlights which only have a few >> predefined luminance levels? How would user space differentiate >> between the continuous and discrete backlight? What about >> self-emitting displays? They usually increase the dynamic range when >> they increase in brightness because the black level doesn't rise. They >> also probably employ some tonemapping to adjust for that. What about >> the range of the backlight? What about the absolute luminance of the >> backlight? I want to know about that all in user space. >> >> I understand that most of the time the kernel doesn't have answers to >> those questions right now but the API should account for all of that. > > Hi, > > if the API accounts for all that, and the kernel doesn't know, then how > can the API not lie? If the API sometimes lies, how could we ever trust > it at all? Make it possible for the API to say "I don't know". I'd much rather have an API tell me explicitly what it does and doesn't know instead of having to guess what data I can actually rely on. For example if the kernel knows the luminance is linear on one display and doesn't know anything about the other display and it exposes them both in the same way I can not possibly write any code which relies on exact control over the luminance for either display. > > Personally I have the feeling that if we can even get to the level of > "each step in the value is a more or less perceivable change", that > would be good enough. Think of UI, e.g. hotkeys to change brightness. > You'd expect almost every press to change it a bit. The nice thing is that you can have that even if you have no further information about the brightness control and it might be good enough for some use cases but it isn't for others. > If an end user wants defined and controlled luminance, I'd suggest they > need to profile (physically measure) the response of the display at > hand. This is no different from color profiling displays, but you need > a measurement device that produces absolute measurements if absolute > control is what they want. If that's the kind of user experience you're after, good for you. I certainly want things to work out of the box which makes this just a big no-go. >>> >>> I think if we have the information to make the default behaviour >>> better then we should do that. Ie. if the firmaware gives us a >>> table to remap the values for a more linear response we should >>> make use of that by default. >> >> But that's only like 20% of what Sebastian is asking for. >> >> What's "linear"? Radiometric or perceptual? >> >> Radiometric linear control would make a terrible UX, so if the control >> is radiometric, userspace needs to remap it. That might be a good >> thing, but it's also complicated, because the relationship between >> brightness and luminance is somewhere between a power curve and >> exponential curve. You need to make sure that the userspace remapping >> works for different backlights with different luminance ranges. That's >> not obvious to me. >> >>> We can of course provide a way for the user to plug in their own >>> actually measured data later. But IMO that doesn't even have to >>> happen in the initial implementation. Just need to avoid painting >>> ourselves totally in the corner in a way that would prevent later >>> additions like that. >> >> For userspace delivering its own curve, you need to define the units. >> Absolute or relative? Radiometric or perceptual? Otherwise the >> resulting control is not portable between window systems. >> >>> I just hate the current limbo where we're somehow too afraid to >>> change the current behaviour to do the remapping by default. >>> I see no upsides in the current behaviour of just blindly >>> exposing the raw hardware register values more or less. They >>> mean absolutely nothing to any user. >> >> I never argued like that. >> >> I'm saying that what looks realistic to me is somewhere *between* >> status quo and what Sebastian is asking for. Whatever you mean by "linear >> remapping" is probably a realistic goal, because you know you have some
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
Hi, On 10/3/22 12:32, Pekka Paalanen wrote: > On Mon, 3 Oct 2022 11:44:56 +0200 > Hans de Goede wrote: > >> One example mentioned here is that laptops with Intel integrated >> graphics may have some "perceived brightness" mapping table >> in their Video BIOS Tables (VBT) it would be interesting to use >> this and to export the curve coming from that to userspace as >> extra information (including allow userspace to write it). >> Since in this case (unlike many other cases) at least this >> curve is done in the kernel I guess we can then also add a boolean >> property to disable the curve (making it linear) in case that >> is helpful for HDR scenarios. > > Hi Hans, > > what if you would export that curve to userspace and not apply it in > the kernel, aside from the min-luminance=0 offset? > > I'm not sure if that was your intention, I didn't see it clearly said. > Of course this can be only about curves that are supposed to be applied > by the OS and not applied in firmware or hardware. Call it "software > curve"? > > Let userspace apply that curve if it happens to exist. Then, if we get > some other definition replacing that curve on some hardware, the kernel > could just expose the other thing as a new thing, and the old curve API > would not be interfering. Userspace that does not understand the new > thing (and the old curve property is not populated) would still be able > to control the brightness, just not in as pleasant way. > > It would also make using a custom curve a completely userspace thing with > no need for the kernel to support overwriting it. Userspace comes in many forms, which is why my preference for "software curve" support would be for it to be applied by the kernel by default (if present in the VBT) but then with a "software curve enable" flag to allow advanced userspace to disable it and then apply a curve of userspace's own choosing itself. This allows a simple implementation for desktop-environments which are unlikely to implement all this themselves, giving everyone the benefit of the nicer behavior of the VBT provided curve while allowing advanced desktop environments (likely mainly KDE + GNOME/mutter) to do something more advanced. Either way, this is all future extensions. First lets get the basic brightness control with just brightness + brightness-max attributes in place. Regards, Hans
Re: Announce MGRX v1.5.1 with improved Wayland support
Hi Mariano, On 6/19/24 5:15 PM, Mariano wrote: > Hi everybody > > MGRX is a small 2D graphics C library derived from the GRX library. GRX was > originaly written by Csaba Biegl for DJ Delorie's DOS port of the GCC > compiler. > > MGRX supports DOS, Win32, Linux framebuffer, Linux KMS/DRM, Linux X11 and > Linux Wayland since the previous version. > > The Wayland videodriver was new in MGRX version 1.5.0 and must be considered > in beta stage. Only the i386 and x86_64 platforms are tested. > > In this version I have improved the Wayland driver: > >  - We use the XDG_decoration protocol, if available, to ask for server side > window decoration. >    it works nicely in KDE. Unfortunately Gnome doesn't support it, so we > don't have decorations there. >   I know is a controversial subject here. You can use libdecor to add client-side decorations on compositors which don't support server side decorations: https://gitlab.freedesktop.org/libdecor/libdecor This is what SDL uses to decorate SDL windows under GNOME. Regards, Hans >  - We generate now autorepeat keys, because Wayland expect the client to do > it. >  - The compose keys work, thanks to functions in libxkbcommon. >  - The clipboard interact with the Wayland clipboard, using the core > protocols. > > If someone wants to made it a try, it will be welcome. > > Website: > http://fgrim.com/mgrx/index.html > Github: > https://github.com/malfer/mgrx > > Regards > Mariano Alvarez >
Re: [PATCH libinput] tablet: add libinput_tablet_tool_is_unique()
Hi, On 12/20/2015 11:34 PM, Peter Hutterer wrote: For checking if a tablet tool can be uniquely identified by libinput. In practice this means checking for a nonzero serial number, but let's not restrict ourselves to allowing just that. Signed-off-by: Peter Hutterer Looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- doc/tablet-support.dox | 4 +++- src/libinput.c | 6 ++ src/libinput.h | 24 ++-- src/libinput.sym | 1 + test/tablet.c | 25 + 5 files changed, 53 insertions(+), 7 deletions(-) diff --git a/doc/tablet-support.dox b/doc/tablet-support.dox index ac56e47..fbe778d 100644 --- a/doc/tablet-support.dox +++ b/doc/tablet-support.dox @@ -126,7 +126,9 @@ and @ref LIBINPUT_TABLET_TOOL_TYPE_LENS tools. Some tools provide hardware information that enables libinput to uniquely identify the physical device. For example, tools compatible with the Wacom Intuos 4, Intuos 5, Intuos Pro and Cintiq series are uniquely identifiable -through a serial number. +through a serial number. libinput does not specify how a tool can be +identified uniquely, a caller should use libinput_tablet_tool_is_unique() to +check if the tool is unique. libinput creates a struct libinput_tablet_tool on the first proximity in of this tool. By default, this struct is destroyed on proximity out and diff --git a/src/libinput.c b/src/libinput.c index 093f318..082c1b0 100644 --- a/src/libinput.c +++ b/src/libinput.c @@ -1318,6 +1318,12 @@ libinput_tablet_tool_get_tool_id(struct libinput_tablet_tool *tool) return tool->tool_id; } +LIBINPUT_EXPORT int +libinput_tablet_tool_is_unique(struct libinput_tablet_tool *tool) +{ + return tool->serial != 0; +} + LIBINPUT_EXPORT uint64_t libinput_tablet_tool_get_serial(struct libinput_tablet_tool *tool) { diff --git a/src/libinput.h b/src/libinput.h index 8726dc5..cc1a083 100644 --- a/src/libinput.h +++ b/src/libinput.h @@ -144,7 +144,7 @@ enum libinput_pointer_axis_source { * Tablet events generated by such a device are bound to a specific tool * rather than coming from the device directly. Depending on the hardware it * is possible to track the same physical tool across multiple - * struct libinput_device devices. + * struct libinput_device devices, see @ref tablet-serial-numbers. */ struct libinput_tablet_tool; @@ -1955,14 +1955,26 @@ libinput_tablet_tool_has_button(struct libinput_tablet_tool *tool, /** * @ingroup event_tablet * - * Return the serial number of a tool + * Return nonzero if the physical tool can be uniquely identified by + * libinput, or nonzero otherwise. If a tool can be uniquely identified, + * keeping a reference to the tool allows tracking the tool across + * proximity out sequences and across compatible tablets. + * See @ref tablet-serial-numbers for more details. * - * @note Not all tablets report a serial number along with the type of tool - * being used. If the hardware does not provide a unique serial number, the - * serial number is always 0. + * @param tool A tablet tool + * @return 1 if the tool can be uniquely identified, 0 otherwise. + */ +int +libinput_tablet_tool_is_unique(struct libinput_tablet_tool *tool); + +/** + * @ingroup event_tablet + * + * Return the serial number of a tool. If the tool does not report a serial + * number, this function returns zero. * * @param tool The libinput tool - * @return The new tool serial triggering this event + * @return The tool serial number */ uint64_t libinput_tablet_tool_get_serial(struct libinput_tablet_tool *tool); diff --git a/src/libinput.sym b/src/libinput.sym index ddfe81d..22a8dd8 100644 --- a/src/libinput.sym +++ b/src/libinput.sym @@ -226,6 +226,7 @@ LIBINPUT_TABLET_SUPPORT { libinput_tablet_tool_has_wheel; libinput_tablet_tool_has_slider; libinput_tablet_tool_has_button; + libinput_tablet_tool_is_unique; libinput_tablet_tool_ref; libinput_tablet_tool_set_user_data; libinput_tablet_tool_unref; diff --git a/test/tablet.c b/test/tablet.c index 1afbb51..ff979a8 100644 --- a/test/tablet.c +++ b/test/tablet.c @@ -1257,6 +1257,30 @@ START_TEST(normalization) } END_TEST +START_TEST(tool_unique) +{ + struct litest_device *dev = litest_current_device(); + struct libinput *li = dev->libinput; + struct libinput_event_tablet_tool *tablet_event; + struct libinput_event *event; + struct libinput_tablet_tool *tool; + + litest_drain_events(li); + + litest_event(dev, EV_KEY, BTN_TOOL_PEN, 1); + litest_event(dev, EV_MSC, MSC_SERIAL, 1000); + litest_event(dev, EV_SYN, SYN_REPORT, 0); + + libinput_dispatch(li); + event = libinput_get_event(li); + tablet_event = litest_is_tablet_event(event, + LIBINPUT_EVENT_TABLET_TOOL_PROXIMITY); + tool = libinput_event_tablet_tool_get_too
Re: [PATCH libinput] tablet: invert tilt axes when left-handed is enabled
Hi, On 12/21/2015 02:46 AM, Peter Hutterer wrote: Signed-off-by: Peter Hutterer Looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-tablet.c | 4 test/tablet.c | 38 ++ 2 files changed, 42 insertions(+) diff --git a/src/evdev-tablet.c b/src/evdev-tablet.c index bf61411..50891e3 100644 --- a/src/evdev-tablet.c +++ b/src/evdev-tablet.c @@ -365,6 +365,8 @@ tablet_handle_tilt(struct tablet_dispatch *tablet, if (bit_is_set(tablet->changed_axes, a)) { absinfo = libevdev_get_abs_info(device->evdev, ABS_TILT_X); tablet->axes[a] = normalize_tilt(absinfo); + if (device->left_handed.enabled) + tablet->axes[a] *= -1; } tilt.x = tablet->axes[a]; @@ -372,6 +374,8 @@ tablet_handle_tilt(struct tablet_dispatch *tablet, if (bit_is_set(tablet->changed_axes, a)) { absinfo = libevdev_get_abs_info(device->evdev, ABS_TILT_Y); tablet->axes[a] = normalize_tilt(absinfo); + if (device->left_handed.enabled) + tablet->axes[a] *= -1; } tilt.y = tablet->axes[a]; diff --git a/test/tablet.c b/test/tablet.c index a55b60a..87af814 100644 --- a/test/tablet.c +++ b/test/tablet.c @@ -1018,6 +1018,43 @@ START_TEST(no_left_handed) } END_TEST +START_TEST(left_handed_tilt) +{ +#if HAVE_LIBWACOM + struct litest_device *dev = litest_current_device(); + struct libinput *li = dev->libinput; + struct libinput_event *event; + struct libinput_event_tablet_tool *tev; + enum libinput_config_status status; + struct axis_replacement axes[] = { + { ABS_DISTANCE, 10 }, + { ABS_TILT_X, 90 }, + { ABS_TILT_Y, 10 }, + { -1, -1 } + }; + double tx, ty; + + status = libinput_device_config_left_handed_set(dev->libinput_device, 1); + ck_assert_int_eq(status, LIBINPUT_CONFIG_STATUS_SUCCESS); + + litest_drain_events(li); + + litest_tablet_proximity_in(dev, 10, 10, axes); + libinput_dispatch(li); + event = libinput_get_event(li); + tev = litest_is_tablet_event(event, +LIBINPUT_EVENT_TABLET_TOOL_PROXIMITY); + tx = libinput_event_tablet_tool_get_tilt_x(tev); + ty = libinput_event_tablet_tool_get_tilt_y(tev); + + ck_assert_double_lt(tx, 0); + ck_assert_double_gt(ty, 0); + + libinput_event_destroy(event); +#endif +} +END_TEST + START_TEST(motion_event_state) { struct litest_device *dev = litest_current_device(); @@ -2852,6 +2889,7 @@ litest_setup_tests(void) litest_add("tablet:tilt", tilt_x, LITEST_TABLET|LITEST_TILT, LITEST_ANY); litest_add("tablet:tilt", tilt_y, LITEST_TABLET|LITEST_TILT, LITEST_ANY); litest_add_for_device("tablet:left_handed", left_handed, LITEST_WACOM_INTUOS); + litest_add_for_device("tablet:left_handed", left_handed_tilt, LITEST_WACOM_INTUOS); litest_add_for_device("tablet:left_handed", no_left_handed, LITEST_WACOM_CINTIQ); litest_add("tablet:normalization", normalization, LITEST_TABLET, LITEST_ANY); litest_add("tablet:pad", pad_buttons_ignored, LITEST_TABLET, LITEST_ANY); ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 1/9] tablet: dump deltas_discrete, replace with a single wheel_discrete variable
Hi, On 12/21/2015 06:56 AM, Peter Hutterer wrote: Only the wheel has a discrete value, no need to keep arrays for a single value. Signed-off-by: Peter Hutterer Entire series looks good to me and is: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-tablet.c | 6 +++--- src/libinput-private.h | 2 +- src/libinput.c | 10 -- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/evdev-tablet.c b/src/evdev-tablet.c index ffb141c..83cf433 100644 --- a/src/evdev-tablet.c +++ b/src/evdev-tablet.c @@ -337,7 +337,7 @@ tablet_check_notify_axes(struct tablet_dispatch *tablet, int a; double axes[LIBINPUT_TABLET_TOOL_AXIS_MAX + 1] = {0}; double deltas[LIBINPUT_TABLET_TOOL_AXIS_MAX + 1] = {0}; - double deltas_discrete[LIBINPUT_TABLET_TOOL_AXIS_MAX + 1] = {0}; + int wheel_discrete = 0; double oldval; struct device_coords point, old_point; const struct input_absinfo *absinfo; @@ -400,7 +400,7 @@ tablet_check_notify_axes(struct tablet_dispatch *tablet, deltas[a] = get_delta(a, tablet->axes[a], oldval); continue; } else if (a == LIBINPUT_TABLET_TOOL_AXIS_REL_WHEEL) { - deltas_discrete[a] = tablet->deltas[a]; + wheel_discrete = tablet->deltas[a]; deltas[a] = normalize_wheel(tablet, tablet->deltas[a]); axes[a] = 0; @@ -468,7 +468,7 @@ tablet_check_notify_axes(struct tablet_dispatch *tablet, tablet->changed_axes, axes, deltas, - deltas_discrete); + wheel_discrete); } } diff --git a/src/libinput-private.h b/src/libinput-private.h index 1c8d97c..b404b0a 100644 --- a/src/libinput-private.h +++ b/src/libinput-private.h @@ -482,7 +482,7 @@ tablet_notify_axis(struct libinput_device *device, unsigned char *changed_axes, double *axes, double *deltas, - double *deltas_discrete); + int wheel_discrete); void tablet_notify_proximity(struct libinput_device *device, diff --git a/src/libinput.c b/src/libinput.c index b1bced0..e5b2180 100644 --- a/src/libinput.c +++ b/src/libinput.c @@ -133,7 +133,7 @@ struct libinput_event_tablet_tool { uint64_t time; double axes[LIBINPUT_TABLET_TOOL_AXIS_MAX + 1]; double deltas[LIBINPUT_TABLET_TOOL_AXIS_MAX + 1]; - double deltas_discrete[LIBINPUT_TABLET_TOOL_AXIS_MAX + 1]; + int wheel_discrete; unsigned char changed_axes[NCHARS(LIBINPUT_TABLET_TOOL_AXIS_MAX + 1)]; struct libinput_tablet_tool *tool; enum libinput_tablet_tool_proximity_state proximity_state; @@ -1161,7 +1161,7 @@ libinput_event_tablet_tool_get_wheel_delta_discrete( LIBINPUT_EVENT_TABLET_TOOL_TIP, LIBINPUT_EVENT_TABLET_TOOL_PROXIMITY); - return event->deltas_discrete[LIBINPUT_TABLET_TOOL_AXIS_REL_WHEEL]; + return event->wheel_discrete; } LIBINPUT_EXPORT double @@ -2153,7 +2153,7 @@ tablet_notify_axis(struct libinput_device *device, unsigned char *changed_axes, double *axes, double *deltas, - double *deltas_discrete) + int wheel_discrete) { struct libinput_event_tablet_tool *axis_event; @@ -2166,6 +2166,7 @@ tablet_notify_axis(struct libinput_device *device, .tool = tool, .proximity_state = LIBINPUT_TABLET_TOOL_PROXIMITY_IN, .tip_state = tip_state, + .wheel_discrete = wheel_discrete, }; memcpy(axis_event->changed_axes, @@ -2173,9 +2174,6 @@ tablet_notify_axis(struct libinput_device *device, sizeof(axis_event->changed_axes)); memcpy(axis_event->axes, axes, sizeof(axis_event->axes)); memcpy(axis_event->deltas, deltas, sizeof(axis_event->deltas)); - memcpy(axis_event->deltas_discrete, - deltas_discrete, - sizeof(axis_event->deltas_discrete)); post_device_event(device, time, ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] evdev: only reject devices with missing MT x/y if they're MT devices
Hi, On 03-01-16 23:36, Peter Hutterer wrote: A fake MT device may have ABS_MT_POSITION_X but not Y. In this case we don't care, because we don't handle those axes anyway. http://bugs.freedesktop.org/show_bug.cgi?id=93474 Signed-off-by: Peter Hutterer --- src/evdev.c | 11 +- test/Makefile.am | 1 + test/device.c| 31 + test/litest-device-anker-mouse-kbd.c | 225 +++ test/litest.c| 2 + test/litest.h| 1 + 6 files changed, 264 insertions(+), 7 deletions(-) create mode 100644 test/litest-device-anker-mouse-kbd.c diff --git a/src/evdev.c b/src/evdev.c index 3708072..d798097 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -1800,13 +1800,12 @@ evdev_fix_android_mt(struct evdev_device *device) { struct libevdev *evdev = device->evdev; - if (libevdev_has_event_code(evdev, EV_ABS, ABS_X) || + if (libevdev_has_event_code(evdev, EV_ABS, ABS_X) && libevdev_has_event_code(evdev, EV_ABS, ABS_Y)) return; if (!libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_X) || - !libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_Y) || - evdev_is_fake_mt_device(device)) + !libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_Y)) return; libevdev_enable_event_code(evdev, EV_ABS, ABS_X, @@ -1868,7 +1867,8 @@ evdev_reject_device(struct evdev_device *device) libevdev_has_event_code(evdev, EV_REL, REL_Y)) return -1; - if (libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_X) ^ + if (!evdev_is_fake_mt_device(device) && + libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_X) ^ libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_Y)) return -1; @@ -2039,9 +2039,6 @@ evdev_configure_device(struct evdev_device *device) return -1; } - if (!evdev_is_fake_mt_device(device)) - evdev_fix_android_mt(device); - if (libevdev_has_event_code(evdev, EV_ABS, ABS_X)) { if (evdev_fix_abs_resolution(device, ABS_X, ABS_Y)) device->abs.fake_resolution = 1; This chunck and the chuncks in evdev_fix_android_mt() seem unrelated to $subject? Also this removes the last caller of evdev_fix_android_mt() ? Regards, Hans diff --git a/test/Makefile.am b/test/Makefile.am index e4ed8e5..1b3090e 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -15,6 +15,7 @@ liblitest_la_SOURCES = \ litest-int.h \ litest-device-alps-semi-mt.c \ litest-device-alps-dualpoint.c \ + litest-device-anker-mouse-kbd.c \ litest-device-asus-rog-gladius.c \ litest-device-atmel-hover.c \ litest-device-bcm5974.c \ diff --git a/test/device.c b/test/device.c index b7fa0e0..03659ac 100644 --- a/test/device.c +++ b/test/device.c @@ -1254,6 +1254,35 @@ START_TEST(device_abs_rel) } END_TEST +START_TEST(device_quirks_no_abs_mt_y) +{ + struct litest_device *dev = litest_current_device(); + struct libinput *li = dev->libinput; + struct libinput_event *event; + struct libinput_event_pointer *pev; + int code; + + litest_drain_events(li); + + litest_event(dev, EV_REL, REL_HWHEEL, 1); + litest_event(dev, EV_SYN, SYN_REPORT, 0); + libinput_dispatch(li); + + event = libinput_get_event(li); + pev = litest_is_axis_event(event, + LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL, + LIBINPUT_POINTER_AXIS_SOURCE_WHEEL); + libinput_event_destroy(libinput_event_pointer_get_base_event(pev)); + + for (code = ABS_MISC + 1; code < ABS_MAX; code++) { + litest_event(dev, EV_ABS, code, 1); + litest_event(dev, EV_SYN, SYN_REPORT, 0); + litest_assert_empty_queue(li); + } + +} +END_TEST + void litest_setup_tests(void) { @@ -1308,4 +1337,6 @@ litest_setup_tests(void) litest_add_no_device("device:invalid rel events", device_touchpad_rel); litest_add_no_device("device:invalid rel events", device_touch_rel); litest_add_no_device("device:invalid rel events", device_abs_rel); + + litest_add_for_device("device:quirks", device_quirks_no_abs_mt_y, LITEST_ANKER_MOUSE_KBD); } diff --git a/test/litest-device-anker-mouse-kbd.c b/test/litest-device-anker-mouse-kbd.c new file mode 100644 index 000..5f39928 --- /dev/null +++ b/test/litest-device-anker-mouse-kbd.c @@ -0,0 +1,225 @@ +/* + * Copyright © 2015 Red Hat, Inc. + * + * 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,
Re: [RFC libinput] Add a "switch" interface for parts of the SW_* range
Hi, On 04-01-16 02:20, Peter Hutterer wrote: This is a first draft to gather some comments, it's not hooked up to anything yet and really just to get the main intention across. Things up for comments and discussion: Unlike buttons, we only expose switches we understand, hence enum libinput_switch rather than just forwarding uint32_t devices. This enables us to be selective on which devices should be handled by libinput, e.g. libinput doesn't need to handle devices that expose SW_JACK_PHYSICAL_INSERT. The ones that matter because they are input device-related are SW_LID, SW_TABLET_MODE, SW_DOCK, and SW_MUTE_DEVICE and SW_KEYPAD_SLIDE. libinput will also handle switch events internally, e.g. disable the touchpad automatically when the lid is closed. this will be transparent to the caller, though they will receive the event too. See https://bugs.freedesktop.org/show_bug.cgi?id=86223 This feature is the main driver for the interface, the alternative would be to handle the device completely internally and let the caller open a separate fd for the any switch device. That makes some sense for the "Led Switch" device, not so much for the Wacom tablets that have SW_MUTE_DEVICE on the device itself. Should we have a separate event type for the current state of the switch when the device is added? I think sending a EVENT_SWITCH_TOGGLE burst on DEVICE_ADDED is sufficient. There will also be a libinput_device_switch_has_switch() and libinput_device_switch_get_switch_state() that do the obvious thing. comments, etc. welcome. Any typos you can safely ignore for now, this is just a first draft :) Cheers, Peter --- doc/Makefile.am| 1 + doc/switches.dox | 14 src/libinput-private.h | 6 src/libinput.c | 80 src/libinput.h | 90 ++ src/libinput.sym | 7 6 files changed, 198 insertions(+) create mode 100644 doc/switches.dox diff --git a/doc/Makefile.am b/doc/Makefile.am index fe70f6a..84eb033 100644 --- a/doc/Makefile.am +++ b/doc/Makefile.am @@ -22,6 +22,7 @@ header_files = \ $(srcdir)/reporting-bugs.dox \ $(srcdir)/scrolling.dox \ $(srcdir)/seats.dox \ + $(srcdir)/switches.dox \ $(srcdir)/t440-support.dox \ $(srcdir)/tapping.dox \ $(srcdir)/test-suite.dox \ diff --git a/doc/switches.dox b/doc/switches.dox new file mode 100644 index 000..ae070560 --- /dev/null +++ b/doc/switches.dox @@ -0,0 +1,14 @@ +/** +@page switches Switches + +libinput supports a couple of switches. Unlike button events that come in +press and release pairs, switches are usually toggled once and left at the +setting for an extended period of time. + +Only some switches are handled by libinput, see @ref libinput_switch for a +list of supported switches. Switch events are exposed to the caller, but +libinput may handle some switch events internally and enable or disable +specific features based on a switch state. + +*/ + diff --git a/src/libinput-private.h b/src/libinput-private.h index 51ac35e..91c843b 100644 --- a/src/libinput-private.h +++ b/src/libinput-private.h @@ -444,6 +444,12 @@ gesture_notify_pinch_end(struct libinput_device *device, double scale, int cancelled); +void +switch_notify_toggle(struct libinput_device *device, +uint64_t time, +enum libinput_switch sw, +enum libinput_switch_state state); + static inline uint64_t libinput_now(struct libinput *libinput) { diff --git a/src/libinput.c b/src/libinput.c index dd16ab7..120552b 100644 --- a/src/libinput.c +++ b/src/libinput.c @@ -125,6 +125,13 @@ struct libinput_event_gesture { double angle; }; +struct libinput_event_switch { + struct libinput_event base; + uint64_t time; + enum libinput_switch sw; + enum libinput_switch_state state; +}; + static void libinput_default_log_func(struct libinput *libinput, enum libinput_log_priority priority, @@ -304,6 +311,17 @@ libinput_event_get_device_notify_event(struct libinput_event *event) return (struct libinput_event_device_notify *) event; } +LIBINPUT_EXPORT struct libinput_event_switch * +libinput_event_get_switch_event(struct libinput_event *event) +{ + require_event_type(libinput_event_get_context(event), + event->type, + NULL, + LIBINPUT_EVENT_SWITCH_TOGGLE); + + return (struct libinput_event_switch *) event; +} + LIBINPUT_EXPORT uint32_t libinput_event_keyboard_get_time(struct libinput_event_keyboard *event) { @@ -884,6 +902,28 @@ libinput_event_gesture_get_angle_delta(struct libinput_event_gesture *event) return event->angle; } +LIBINPUT_EXPORT enum libinput_switch +libinput_event_switch_get_switch(struct libinput_event_swi
Re: [PATCH libinput] touchpad: fix DWT pairing for Macbook Pro 2015
Hi, On 06-01-16 07:52, Peter Hutterer wrote: From: Caibin Chen Label internal keyboards through the udev hwdb and only pair the internal (usb) Apple touchpads with those keyboards labelled as such. https://bugs.freedesktop.org/show_bug.cgi?id=93367 Co-authored-by: Peter Hutterer Signed-off-by: Peter Hutterer Patch code looks good to me: Reviewed-by: Hans de Goede I do worry a bit if this will not break dwt on older macbooks though. Maybe we need to also check the bus type in this block: > + /* For Apple touchpads, always use its internal keyboard */ > + if (vendor_tp == VENDOR_ID_APPLE) { > + return vendor_kbd == vendor_tp && > + keyboard->model_flags & > + EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD; > + } > + Since we are using the usb-vendor id of apple here, right ? Regards, Hans --- src/evdev-mt-touchpad.c | 9 + src/evdev.c | 1 + src/evdev.h | 1 + src/libinput-util.h | 1 + test/Makefile.am | 1 + test/litest-device-apple-internal-keyboard.c | 239 +++ test/litest.c| 2 + test/litest.h| 1 + test/touchpad.c | 87 -- udev/90-libinput-model-quirks.hwdb | 3 + 10 files changed, 328 insertions(+), 17 deletions(-) create mode 100644 test/litest-device-apple-internal-keyboard.c diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index d78a54b..aa123cd 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -1285,6 +1285,8 @@ tp_want_dwt(struct evdev_device *touchpad, { unsigned int bus_tp = libevdev_get_id_bustype(touchpad->evdev), bus_kbd = libevdev_get_id_bustype(keyboard->evdev); + unsigned int vendor_tp = evdev_device_get_id_vendor(touchpad); + unsigned int vendor_kbd = evdev_device_get_id_vendor(keyboard); if (tp_dwt_device_is_blacklisted(touchpad) || tp_dwt_device_is_blacklisted(keyboard)) @@ -1295,6 +1297,13 @@ tp_want_dwt(struct evdev_device *touchpad, if (bus_tp == BUS_I8042 && bus_kbd != bus_tp) return false; + /* For Apple touchpads, always use its internal keyboard */ + if (vendor_tp == VENDOR_ID_APPLE) { + return vendor_kbd == vendor_tp && + keyboard->model_flags & + EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD; + } + /* everything else we don't really know, so we have to assume they go together */ diff --git a/src/evdev.c b/src/evdev.c index d798097..bfaf2ed 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -1662,6 +1662,7 @@ evdev_read_model_flags(struct evdev_device *device) { "LIBINPUT_MODEL_SYNAPTICS_SERIAL_TOUCHPAD", EVDEV_MODEL_SYNAPTICS_SERIAL_TOUCHPAD }, { "LIBINPUT_MODEL_JUMPING_SEMI_MT", EVDEV_MODEL_JUMPING_SEMI_MT }, { "LIBINPUT_MODEL_ELANTECH_TOUCHPAD", EVDEV_MODEL_ELANTECH_TOUCHPAD }, + { "LIBINPUT_MODEL_APPLE_INTERNAL_KEYBOARD", EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD }, { NULL, EVDEV_MODEL_DEFAULT }, }; const struct model_map *m = model_map; diff --git a/src/evdev.h b/src/evdev.h index 36bf7b4..97177ec 100644 --- a/src/evdev.h +++ b/src/evdev.h @@ -108,6 +108,7 @@ enum evdev_device_model { EVDEV_MODEL_JUMPING_SEMI_MT = (1 << 10), EVDEV_MODEL_ELANTECH_TOUCHPAD = (1 << 11), EVDEV_MODEL_LENOVO_X220_TOUCHPAD_FW81 = (1 << 12), + EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD = (1 << 13), }; struct mt_slot { diff --git a/src/libinput-util.h b/src/libinput-util.h index a627e5d..25de8e5 100644 --- a/src/libinput-util.h +++ b/src/libinput-util.h @@ -38,6 +38,7 @@ #define VENDOR_ID_APPLE 0x5ac #define VENDOR_ID_WACOM 0x56a #define VENDOR_ID_SYNAPTICS_SERIAL 0x002 +#define PRODUCT_ID_APPLE_KBD_TOUCHPAD 0x273 #define PRODUCT_ID_SYNAPTICS_SERIAL 0x007 /* The HW DPI rate we normalize to before calculating pointer acceleration */ diff --git a/test/Makefile.am b/test/Makefile.am index 1b3090e..4c394bf 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -16,6 +16,7 @@ liblitest_la_SOURCES = \ litest-device-alps-semi-mt.c \ litest-device-alps-dualpoint.c \ litest-device-anker-mouse-kbd.c \ + litest-device-apple-internal-keyboard.c \ litest-device-asus-rog-gladius.c \ litest-device-atmel-hover.c \ litest-device-bcm5974.c \ diff --git a/test/litest-device-apple-internal-keyboard.c b/test/litest-device-apple-internal-keyboard.c new file mode 100644 index 000..c24403d --- /dev/null +++ b/test/litest-device-a
Re: [PATCH v2 libinput] evdev: only reject devices with missing MT x/y if they're MT devices
Hi, On 08-01-16 01:51, Peter Hutterer wrote: A fake MT device may have ABS_MT_POSITION_X but not Y. In this case we don't care, because we don't handle those axes anyway. http://bugs.freedesktop.org/show_bug.cgi?id=93474 Signed-off-by: Peter Hutterer Looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- Changes to v1: - reduce to the hunk that actually matters. sorry, not sure what happened there, some rebase gone wrong of my original approach and I didn't notice it before sending it. src/evdev.c | 3 ++- test/Makefile.am | 1 + test/device.c| 31 +++ test/litest.c| 2 ++ test/litest.h| 1 + 5 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/evdev.c b/src/evdev.c index 3708072..df5fee3 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -1868,7 +1868,8 @@ evdev_reject_device(struct evdev_device *device) libevdev_has_event_code(evdev, EV_REL, REL_Y)) return -1; - if (libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_X) ^ + if (!evdev_is_fake_mt_device(device) && + libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_X) ^ libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_Y)) return -1; diff --git a/test/Makefile.am b/test/Makefile.am index e4ed8e5..1b3090e 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -15,6 +15,7 @@ liblitest_la_SOURCES = \ litest-int.h \ litest-device-alps-semi-mt.c \ litest-device-alps-dualpoint.c \ + litest-device-anker-mouse-kbd.c \ litest-device-asus-rog-gladius.c \ litest-device-atmel-hover.c \ litest-device-bcm5974.c \ diff --git a/test/device.c b/test/device.c index b7fa0e0..03659ac 100644 --- a/test/device.c +++ b/test/device.c @@ -1254,6 +1254,35 @@ START_TEST(device_abs_rel) } END_TEST +START_TEST(device_quirks_no_abs_mt_y) +{ + struct litest_device *dev = litest_current_device(); + struct libinput *li = dev->libinput; + struct libinput_event *event; + struct libinput_event_pointer *pev; + int code; + + litest_drain_events(li); + + litest_event(dev, EV_REL, REL_HWHEEL, 1); + litest_event(dev, EV_SYN, SYN_REPORT, 0); + libinput_dispatch(li); + + event = libinput_get_event(li); + pev = litest_is_axis_event(event, + LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL, + LIBINPUT_POINTER_AXIS_SOURCE_WHEEL); + libinput_event_destroy(libinput_event_pointer_get_base_event(pev)); + + for (code = ABS_MISC + 1; code < ABS_MAX; code++) { + litest_event(dev, EV_ABS, code, 1); + litest_event(dev, EV_SYN, SYN_REPORT, 0); + litest_assert_empty_queue(li); + } + +} +END_TEST + void litest_setup_tests(void) { @@ -1308,4 +1337,6 @@ litest_setup_tests(void) litest_add_no_device("device:invalid rel events", device_touchpad_rel); litest_add_no_device("device:invalid rel events", device_touch_rel); litest_add_no_device("device:invalid rel events", device_abs_rel); + + litest_add_for_device("device:quirks", device_quirks_no_abs_mt_y, LITEST_ANKER_MOUSE_KBD); } diff --git a/test/litest.c b/test/litest.c index fc188b6..e51004e 100644 --- a/test/litest.c +++ b/test/litest.c @@ -367,6 +367,7 @@ extern struct litest_test_device litest_magicpad_device; extern struct litest_test_device litest_elantech_touchpad_device; extern struct litest_test_device litest_mouse_gladius_device; extern struct litest_test_device litest_mouse_wheel_click_angle_device; +extern struct litest_test_device litest_anker_mouse_kbd_device; struct litest_test_device* devices[] = { &litest_synaptics_clickpad_device, @@ -400,6 +401,7 @@ struct litest_test_device* devices[] = { &litest_elantech_touchpad_device, &litest_mouse_gladius_device, &litest_mouse_wheel_click_angle_device, + &litest_anker_mouse_kbd_device, NULL, }; diff --git a/test/litest.h b/test/litest.h index 1268e10..be270ee 100644 --- a/test/litest.h +++ b/test/litest.h @@ -144,6 +144,7 @@ enum litest_device_type { LITEST_ELANTECH_TOUCHPAD = -30, LITEST_MOUSE_GLADIUS = -31, LITEST_MOUSE_WHEEL_CLICK_ANGLE = -32, + LITEST_ANKER_MOUSE_KBD = -33, }; enum litest_device_feature { ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: disable MT for elantech semi-mt touchpads
Hi, On 13-01-16 05:09, Peter Hutterer wrote: When three fingers are set down on the touchpad, one finger tends to get a 0/0 coordinate, triggering palm detection in the upper left corner. Handle this like the jumping semi-mt touchpads and disable MT handling and instead just rely on the x/y axis and the BTN_TOOL_* events. https://bugs.freedesktop.org/show_bug.cgi?id=93583 This kernel patch is required: https://lkml.org/lkml/2016/1/11/171 Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede About Benjamin's patch: https://lkml.org/lkml/2016/1/11/171 Benjamin, when you pass in INPUT_MT_SEMI_MT, you can drop the: __set_bit(INPUT_PROP_SEMI_MT, dev->propbit); A few lines up, since input_mt_init_slots will do that now. More in general the elantech driver could do with some cleanup to fully use the input_mt helpers, specifically stop doing pointer emulation itself for v2 and v3 touchpads: - pass in INPUT_MT_POINTER to input_mt_init_slots and stop initializing ABS_X / ABS_Y and move input_mt_init_slots to after setting up the MT axis (this applies to v4 too) - Stop reporting BTN_TOUCH / ABS_X / ABSY in diy style instead just call input_mt_frame_sync before input_sync - Replace: input_report_key(dev, BTN_TOOL_FINGER, fingers == 1); input_report_key(dev, BTN_TOOL_DOUBLETAP, fingers == 2); input_report_key(dev, BTN_TOOL_TRIPLETAP, fingers == 3); input_report_key(dev, BTN_TOOL_QUADTAP, fingers == 4); With: input_mt_report_finger_count(dev, fingers); - And for v4 touchpads replace: input_mt_report_pointer_emulation(dev, true); With: input_mt_frame_sync(dev); Which ends up doing the same, but is the right way to use the input_mt helpers Regards, Hans --- src/evdev-mt-touchpad.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index 2de2539..f91f839 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -1496,7 +1496,8 @@ tp_init_slots(struct tp_dispatch *tp, * explanation. */ if (tp->semi_mt && - (device->model_flags & EVDEV_MODEL_JUMPING_SEMI_MT)) { + (device->model_flags & +(EVDEV_MODEL_JUMPING_SEMI_MT|EVDEV_MODEL_ELANTECH_TOUCHPAD))) { tp->num_slots = 1; tp->slot = 0; tp->has_mt = false; ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: disable MT for all semi-mt devices
Hi, On 19-01-16 01:01, Peter Hutterer wrote: Synaptics, Elantech and Alps semi-mt devices all have issues with reporting correct MT data, even the bounding box which semi-mt devices are supposed to report is wrong. Synaptics devices have massive jumps with two fingers down. Elantech devices may open slots without coordinate data. Alps devices may send 0/0 coordinates as initial slot position. All these may be addressable with specific quirks, but the actual benefit is largely restricted to better palm detection (though even with quirks this is unlikely to work) and support for pinch gestures (again, lack of coordinates makes supporting those hard anyway). Elantech: https://bugs.freedesktop.org/show_bug.cgi?id=93583 Alps: https://bugzilla.redhat.com/show_bug.cgi?id=1295073 Signed-off-by: Peter Hutterer I've spend a bit thinking about this, and ultimately I think simply completely disabling gestures on these may indeed be best. If we do this, then the src/evdev-mt-touchpad-gestures.c can be simplified further though, the following bits can be removed / simplified then : tp_gesture_get_direction() : /* * Semi-mt touchpads have somewhat inaccurate coordinates when * 2 fingers are down, so use a slightly larger threshold. * Elantech semi-mt touchpads are accurate enough though. */ if (tp->semi_mt && (tp->device->model_flags & EVDEV_MODEL_ELANTECH_TOUCHPAD) == 0) move_threshold = TP_MM_TO_DPI_NORMALIZED(4); else move_threshold = TP_MM_TO_DPI_NORMALIZED(1); I think the entire move_threshold variable can be dropped here and instead we can simply use TP_MM_TO_DPI_NORMALIZED(1) directly. tp_gesture_get_pinch_info(): if (!tp->semi_mt) *angle = atan2(normalized.y, normalized.x) * 180.0 / M_PI; else *angle = 0.0; tp_gesture_twofinger_handle_state_scroll(): /* On some semi-mt models slot 0 is more accurate, so for semi-mt * we only use slot 0. */ if (tp->semi_mt) { if (!tp->touches[0].dirty) return GESTURE_2FG_STATE_SCROLL; delta = tp_get_delta(&tp->touches[0]); } else { delta = tp_get_average_touches_delta(tp); } Still seems a petty to throw away all the work we've put into getting pinch to work semi-mt devices, otoh I guess that the amount of workarounds already in the code shows that it really is best to just drop gesture support on these. --- src/evdev-mt-touchpad-gestures.c | 6 ++ src/evdev-mt-touchpad.c | 21 + test/gestures.c | 2 +- test/litest.h| 10 -- test/touchpad-tap.c | 2 +- 5 files changed, 17 insertions(+), 24 deletions(-) diff --git a/src/evdev-mt-touchpad-gestures.c b/src/evdev-mt-touchpad-gestures.c index 80aa89f..24f6a6d 100644 --- a/src/evdev-mt-touchpad-gestures.c +++ b/src/evdev-mt-touchpad-gestures.c @@ -568,10 +568,8 @@ tp_gesture_handle_state(struct tp_dispatch *tp, uint64_t time) int tp_init_gesture(struct tp_dispatch *tp) { - if (tp->device->model_flags & EVDEV_MODEL_JUMPING_SEMI_MT) - tp->gesture.enabled = false; - else - tp->gesture.enabled = true; + /* semi-mt devices are too unreliable to do pinch gestures */ + tp->gesture.enabled = !tp->semi_mt; This does not seem like the right fix, this only disables reporting of pinch gestures, but we still end up checking if a 2fg gesture is a scroll or a pinch, always ending up in a scroll anyways since we only have 1 real touch, so just needlessly delaying the scroll. Where as if we never want to allow pinch on semi-mt we should simply jump straight to GESTURE_2FG_STATE_SCROLL if (!tp->has_mt) in tp_gesture_twofinger_handle_state_none() Then we can also revert: http://cgit.freedesktop.org/wayland/libinput/commit/?id=0d40aefe Making tp_gesture_get_active_touches() look only at real touches again as intended. Regards, Hans tp->gesture.twofinger_state = GESTURE_2FG_STATE_NONE; diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index 62087fb..7f5bbf5 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -1490,17 +1490,22 @@ tp_init_slots(struct tp_dispatch *tp, tp->semi_mt = libevdev_has_property(device->evdev, INPUT_PROP_SEMI_MT); - /* This device has a terrible resolution when two fingers are down, + /* Semi-mt devices are not reliable for true multitouch data, so we +* simply pretend they're single touch touchpads with BTN_TOOL bits. +* Synaptics: +* Terrible resolution when two fingers are down, * causing scroll jumps. The single-touch emulation ABS_X/Y is * accurate but the ABS_MT_POSITION touchpoints report the bounding -* box and that causes jumps. So we simply pretend it's a single
Re: [PATCH libinput 1/4] touchpad: don't try to unhover touches if nothing changed
Hi, On 22-01-16 03:06, Peter Hutterer wrote: If the touch hasn't updated, the distance hasn't changed so there is no need to unhover. Signed-off-by: Peter Hutterer Series looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-mt-touchpad.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index 6f834fb..62087fb 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -743,6 +743,9 @@ tp_unhover_abs_distance(struct tp_dispatch *tp, uint64_t time) for (i = 0; i < tp->ntouches; i++) { t = tp_get_touch(tp, i); + if (!t->dirty) + continue; + if (t->state == TOUCH_HOVERING) { if (t->distance == 0) { /* avoid jumps when landing a finger */ ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: disable MT for all semi-mt devices
Hi, On 22-01-16 03:06, Peter Hutterer wrote: On Wed, Jan 20, 2016 at 11:31:42AM +0100, Hans de Goede wrote: Hi, Then we can also revert: http://cgit.freedesktop.org/wayland/libinput/commit/?id=0d40aefe Making tp_gesture_get_active_touches() look only at real touches again as intended. I don't think this is correct, we still need to count fake fingers on an ST touchpad for two-finger scrolling to trigger correctly. That is because you've put the: + if (!tp->gesture.enabled) + return GESTURE_2FG_STATE_SCROLL; + Check at the end of tp_gesture_twofinger_handle_state_none(), so after the call to tp_gesture_get_active_touches(). None of the variables set in tp_gesture_twofinger_handle_state_none() are used when jumping straight to GESTURE_2FG_STATE_SCROLL, so you could do the check at the beginning of tp_gesture_twofinger_handle_state_none() (before calling tp_gesture_get_active_touches()), and then you no longer need to worry about ST touchpads in there. The problem I've with checking all touches, not just real touches in tp_gesture_get_active_touches() is that you may end up returning a fake touch as one of the 2 touches, and then use the coordinates of this fake touch to decide whether the user is doing a pinch or a 2fg scroll. IMHO it would be better to just jump straight to 2FG scrolling in this case (note currently we stay in GESTURE_2FG_STATE_NONE if tp_gesture_get_active_touches() fails). ### An unrelated thing which I noticed while reviewing your patches for this is that I'm not sure if we're handling things right in tp_get_average_touches_delta() currently we are counting dirty touches there and dividing the reported deltas by the number of dirty touches, this means that if 2fg are moving and movement gets reported from both in a single frame we report the averaged result of both moves (good), but if 2 fingers are moving, but first we get a move event for fg1 only, and then for fg2 only we now end up reporting double the move of what we would report if both moves were reported in a single frame. This is wrong, and doubly so because we are likely to get movements in separate frames when the user is trying to e.g. do a slow 2fg scroll, which we now speed up by a factor of 2. I believe that we should change to counting active_touches in tp_get_touches_delta rather then changed touches, and dividing by that when averaging. We cannot simply use tp->gesture.finger_count since that may be different from the actual number of active touches when tap+dragging. ### Yet another unrelated remark, it seems we've an unused "int i" in tp_gesture_handle_state(). Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] gestures: jump straight to swipe for 3+ finger gestures on ST touchpads
Hi, On 25-01-16 04:03, Peter Hutterer wrote: The first/second variables are only needed for pinch, so we can skip them here. Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-mt-touchpad-gestures.c | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/evdev-mt-touchpad-gestures.c b/src/evdev-mt-touchpad-gestures.c index 5aa256f..8fe0bb8 100644 --- a/src/evdev-mt-touchpad-gestures.c +++ b/src/evdev-mt-touchpad-gestures.c @@ -240,6 +240,13 @@ tp_gesture_handle_state_none(struct tp_dispatch *tp, uint64_t time) if (ntouches < 2) return GESTURE_STATE_NONE; + if (!tp->gesture.enabled) { + if (ntouches == 2) + return GESTURE_STATE_SCROLL; + else + return GESTURE_STATE_SWIPE; + } + first = touches[0]; second = touches[1]; @@ -271,8 +278,7 @@ tp_gesture_handle_state_none(struct tp_dispatch *tp, uint64_t time) if (first == second) return GESTURE_STATE_NONE; - } else if (!tp->gesture.enabled) - return GESTURE_STATE_SCROLL; + } tp->gesture.initial_time = time; first->gesture.initial = first->point; ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] gestures: average motion by active touches, not moved touches
Hi, On 25-01-16 04:04, Peter Hutterer wrote: Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-mt-touchpad-gestures.c | 16 ++-- 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/evdev-mt-touchpad-gestures.c b/src/evdev-mt-touchpad-gestures.c index 8fe0bb8..dc8d606 100644 --- a/src/evdev-mt-touchpad-gestures.c +++ b/src/evdev-mt-touchpad-gestures.c @@ -49,15 +49,19 @@ static struct normalized_coords tp_get_touches_delta(struct tp_dispatch *tp, bool average) { struct tp_touch *t; - unsigned int i, nchanged = 0; + unsigned int i, nactive = 0; struct normalized_coords normalized; struct normalized_coords delta = {0.0, 0.0}; for (i = 0; i < tp->num_slots; i++) { t = &tp->touches[i]; - if (tp_touch_active(tp, t) && t->dirty) { - nchanged++; + if (!tp_touch_active(tp, t)) + continue; + + nactive++; + + if (t->dirty) { normalized = tp_get_delta(t); delta.x += normalized.x; @@ -65,11 +69,11 @@ tp_get_touches_delta(struct tp_dispatch *tp, bool average) } } - if (!average || nchanged == 0) + if (!average || nactive == 0) return delta; - delta.x /= nchanged; - delta.y /= nchanged; + delta.x /= nactive; + delta.y /= nactive; return delta; } ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: disable MT for all semi-mt devices
Hi, On 25-01-16 04:03, Peter Hutterer wrote: On Fri, Jan 22, 2016 at 11:15:55AM +0100, Hans de Goede wrote: Hi, On 22-01-16 03:06, Peter Hutterer wrote: On Wed, Jan 20, 2016 at 11:31:42AM +0100, Hans de Goede wrote: Hi, Then we can also revert: http://cgit.freedesktop.org/wayland/libinput/commit/?id=0d40aefe Making tp_gesture_get_active_touches() look only at real touches again as intended. I don't think this is correct, we still need to count fake fingers on an ST touchpad for two-finger scrolling to trigger correctly. That is because you've put the: + if (!tp->gesture.enabled) + return GESTURE_2FG_STATE_SCROLL; + Check at the end of tp_gesture_twofinger_handle_state_none(), so after the call to tp_gesture_get_active_touches(). None of the variables set in tp_gesture_twofinger_handle_state_none() are used when jumping straight to GESTURE_2FG_STATE_SCROLL, so you could do the check at the beginning of tp_gesture_twofinger_handle_state_none() (before calling tp_gesture_get_active_touches()), and then you no longer need to worry about ST touchpads in there. Note: I've since merged the 3-finger pinch gesture branch, so merging this set on top caused a couple of conflicts and things look a bit different now. tp_gesture_twofinger_handle_state_none() was renamed and is now invoked for 3 and 4-finger states as well, so we do need a finger check to bail out. what I will add though is a direct jump to swipe for a >2 finger gesture on an ST touchpad, patch coming up. The problem I've with checking all touches, not just real touches in tp_gesture_get_active_touches() is that you may end up returning a fake touch as one of the 2 touches, and then use the coordinates of this fake touch to decide whether the user is doing a pinch or a 2fg scroll. IMHO it would be better to just jump straight to 2FG scrolling in this case (note currently we stay in GESTURE_2FG_STATE_NONE if tp_gesture_get_active_touches() fails). the touchpad code is so that fake touches get the same positional coordinates as the first touch points. So you can't ever trigger a pinch on an ST device anyway, they'll always move in the same direction. I've tried to avoid putting more semi-mt conditions in than necessary, relying on that functionality to give us the correct coordinates. but I think the patch for the above addresses that anyway, so I think we're good here. What has me worried is the case where we've a true multi-touch clickpad which tracks 2 fingers, then the first finger down could be resting in a soft-button area, and the user could be doing 2fg scrolling with his other hand. In this case the coordinates of the resting finger will be used for the 2nd touch of the 2fg scroll (the 3th finger down), so we will be looking at the resting finger vs one of the 2 scrolling fingers which makes things look like a pinch. Hence it is better to make tp_gesture_get_active_touches only look at real touches, and if it fails just play it safe and jump straight to scrolling / swiping. Just my 2 cents on this :) Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] evdev: disable the mode button on the Cyborg RAT 5
Hi, On 01/29/2016 01:48 AM, Peter Hutterer wrote: This button sends a release N, press N+1 on each press, cycling through the three event codes supported. This causes a stuck button since the current mode is never released. Long-term this better served by a set of switches that toggle accordingly, for now disable the button codes. https://bugs.freedesktop.org/show_bug.cgi?id=92127 Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev.c| 36 + src/evdev.h| 1 + test/Makefile.am | 1 + test/device.c | 33 test/litest-device-cyborg-rat-5.c | 71 ++ test/litest.c | 2 + test/litest.h | 1 + udev/90-libinput-model-quirks.hwdb | 7 udev/90-libinput-model-quirks.rules.in | 4 ++ 9 files changed, 156 insertions(+) create mode 100644 test/litest-device-cyborg-rat-5.c diff --git a/src/evdev.c b/src/evdev.c index 8f0a607..66673a8 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -1677,6 +1677,7 @@ evdev_read_model_flags(struct evdev_device *device) { "LIBINPUT_MODEL_JUMPING_SEMI_MT", EVDEV_MODEL_JUMPING_SEMI_MT }, { "LIBINPUT_MODEL_ELANTECH_TOUCHPAD", EVDEV_MODEL_ELANTECH_TOUCHPAD }, { "LIBINPUT_MODEL_APPLE_INTERNAL_KEYBOARD", EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD }, + { "LIBINPUT_MODEL_CYBORG_RAT", EVDEV_MODEL_CYBORG_RAT }, { NULL, EVDEV_MODEL_DEFAULT }, }; const struct model_map *m = model_map; @@ -2249,6 +2250,39 @@ evdev_drain_fd(int fd) } } +static inline void +evdev_pre_configure_model_quirks(struct evdev_device *device) +{ + /* The Cyborg RAT has a mode button that cycles through event codes. +* On press, we get a release for the current mode and a press for the +* next mode: +* E: 0.01 0004 0004 589833 # EV_MSC / MSC_SCAN 589833 +* E: 0.01 0001 0118 # EV_KEY / (null) 0 +* E: 0.01 0004 0004 589834 # EV_MSC / MSC_SCAN 589834 +* E: 0.01 0001 0119 0001 # EV_KEY / (null) 1 +* E: 0.01 # SYN_REPORT (0) -- +0ms +* E: 0.705000 0004 0004 589834 # EV_MSC / MSC_SCAN 589834 +* E: 0.705000 0001 0119 # EV_KEY / (null) 0 +* E: 0.705000 0004 0004 589835 # EV_MSC / MSC_SCAN 589835 +* E: 0.705000 0001 011a 0001 # EV_KEY / (null) 1 +* E: 0.705000 # SYN_REPORT (0) -- +705ms +* E: 1.496995 0004 0004 589833 # EV_MSC / MSC_SCAN 589833 +* E: 1.496995 0001 0118 0001 # EV_KEY / (null) 1 +* E: 1.496995 0004 0004 589835 # EV_MSC / MSC_SCAN 589835 +* E: 1.496995 0001 011a # EV_KEY / (null) 0 +* E: 1.496995 # SYN_REPORT (0) -- +791ms +* +* https://bugs.freedesktop.org/show_bug.cgi?id=92127 +* +* Disable the event codes to avoid stuck buttons. +*/ + if(device->model_flags & EVDEV_MODEL_CYBORG_RAT) { + libevdev_disable_event_code(device->evdev, EV_KEY, 0x118); + libevdev_disable_event_code(device->evdev, EV_KEY, 0x119); + libevdev_disable_event_code(device->evdev, EV_KEY, 0x11a); + } +} + struct evdev_device * evdev_device_create(struct libinput_seat *seat, struct udev_device *udev_device) @@ -2318,6 +2352,8 @@ evdev_device_create(struct libinput_seat *seat, matrix_init_identity(&device->abs.usermatrix); matrix_init_identity(&device->abs.default_calibration); + evdev_pre_configure_model_quirks(device); + if (evdev_configure_device(device) == -1) goto err; diff --git a/src/evdev.h b/src/evdev.h index 02b5112..8b567a8 100644 --- a/src/evdev.h +++ b/src/evdev.h @@ -110,6 +110,7 @@ enum evdev_device_model { EVDEV_MODEL_ELANTECH_TOUCHPAD = (1 << 11), EVDEV_MODEL_LENOVO_X220_TOUCHPAD_FW81 = (1 << 12), EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD = (1 << 13), + EVDEV_MODEL_CYBORG_RAT = (1 << 14), }; struct mt_slot { diff --git a/test/Makefile.am b/test/Makefile.am index 885d9c6..27a2a36 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -20,6 +20,7 @@ liblitest_la_SOURCES = \ litest-device-asus-rog-gladius.c \ litest-device-atmel-hover.c \ litest-device-bcm5974.c \ + litest-device-cyborg-rat-5.c \ litest-device-elantech-touchpad.c \ litest-device-
Re: [PATCH libinput] test: add two gesture tests for scrolling with a thumb in the btnarea
Hi, On 01/29/2016 02:21 AM, Peter Hutterer wrote: If a finger is resting in the software button area, it must not be counted towards the gesture. So a two-finger movement must be a scroll event, not a three-finger pinch. Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- test/gestures.c | 60 + 1 file changed, 60 insertions(+) diff --git a/test/gestures.c b/test/gestures.c index 3f7ee83..c814911 100644 --- a/test/gestures.c +++ b/test/gestures.c @@ -1153,6 +1153,63 @@ START_TEST(gestures_time_usec) } END_TEST +START_TEST(gestures_3fg_buttonarea_scroll) +{ + struct litest_device *dev = litest_current_device(); + struct libinput *li = dev->libinput; + + if (libevdev_get_num_slots(dev->evdev) < 3) + return; + + litest_enable_buttonareas(dev); + litest_enable_2fg_scroll(dev); + litest_drain_events(li); + + litest_touch_down(dev, 0, 40, 20); + litest_touch_down(dev, 1, 30, 20); + /* third finger in btnarea */ + litest_touch_down(dev, 2, 50, 99); + libinput_dispatch(li); + litest_touch_move_two_touches(dev, + 40, 20, + 30, 20, + 0, 40, + 4, 2); + + litest_touch_up(dev, 0); + litest_touch_up(dev, 1); + libinput_dispatch(li); + litest_assert_scroll(li, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL, 4); +} +END_TEST + +START_TEST(gestures_3fg_buttonarea_scroll_btntool) +{ + struct litest_device *dev = litest_current_device(); + struct libinput *li = dev->libinput; + + if (libevdev_get_num_slots(dev->evdev) > 2) + return; + + litest_enable_buttonareas(dev); + litest_enable_2fg_scroll(dev); + litest_drain_events(li); + + /* first finger in btnarea */ + litest_touch_down(dev, 0, 20, 99); + litest_touch_down(dev, 1, 30, 20); + litest_event(dev, EV_KEY, BTN_TOOL_DOUBLETAP, 0); + litest_event(dev, EV_KEY, BTN_TOOL_TRIPLETAP, 1); + litest_event(dev, EV_SYN, SYN_REPORT, 0); + libinput_dispatch(li); + litest_touch_move_to(dev, 1, 30, 20, 30, 70, 10, 1); + + litest_touch_up(dev, 1); + libinput_dispatch(li); + litest_assert_scroll(li, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL, 4); +} +END_TEST + void litest_setup_tests(void) { @@ -1173,5 +1230,8 @@ litest_setup_tests(void) litest_add_ranged("gestures:pinch", gestures_pinch_4fg_btntool, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH, &cardinals); litest_add_ranged("gestures:pinch", gestures_spread, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH, &cardinals); + litest_add("gestures:swipe", gestures_3fg_buttonarea_scroll, LITEST_CLICKPAD, LITEST_SINGLE_TOUCH); + litest_add("gestures:swipe", gestures_3fg_buttonarea_scroll_btntool, LITEST_CLICKPAD, LITEST_SINGLE_TOUCH); + litest_add("gesture:time", gestures_time_usec, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH); } ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: drop motion hysteresis
Hi, On 01-02-16 07:43, Peter Hutterer wrote: On Mon, Feb 01, 2016 at 12:38:13PM +1000, Peter Hutterer wrote: Some older touchpad devices jitter a fair bit when a finger is resting on the touchpad. That's why the hysteresis was introduced in the synaptics driver back in 2011. However, the default value of the hysteresis in the synaptics driver ended up being 0, even though the code looks like it's using a fraction of the touchpad diagonal. When the hysteresis code was ported to libinput it was eventually set to 0.5mm. Turns out this is still too high and tiny finger motions are either nonreactive or quite jumpy, making it hard to select small targets. Drop the hysteresis by reducing its margin to 0. I'm leaving the code in place for now because this will likely be needed for some devices. https://bugs.freedesktop.org/show_bug.cgi?id=93503 Signed-off-by: Peter Hutterer patch is withdrawn, I just got recordings from a touchpad that needs it, so I'll combine this patch with the one that keeps the behaviour for that touchpad I'm pretty sure that my alps semi-mt touchpad needs this too. Chances are all semi-mt and single touch touchpads benefit from this ... Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v2 libinput] touchpad: drop motion hysteresis by default
Hi, On 02-02-16 01:16, Peter Hutterer wrote: Some older touchpad devices jitter a fair bit when a finger is resting on the touchpad. That's why the hysteresis was introduced in the synaptics driver back in 2011. However, the default value of the hysteresis in the synaptics driver ended up being 0, even though the code looks like it's using a fraction of the touchpad diagonal. When the hysteresis code was ported to libinput it was eventually set to 0.5mm. Turns out this is still too high and tiny finger motions are either nonreactive or quite jumpy, making it hard to select small targets. Drop the default hysteresis by reducing its margin to 0, but leave it in place for those devices where we need them (e.g. the cyapa touchpads). https://bugs.freedesktop.org/show_bug.cgi?id=93503 Signed-off-by: Peter Hutterer --- Changes to v1: - add exception for the cyapa touchpads I'm purposely only including that one model because we know that it needs the hysteresis. Over time, there will be other devices that need it, but let's fix those up as we find them rather than erring on the side of including a bunch of devices that may not need it, the improvements to slow motion are too good to ignore Ok, lets say how this one plays out: Reviewed-by: Hans de Goede Regards, Hans src/evdev-mt-touchpad.c| 23 ++- src/evdev.c| 1 + src/evdev.h| 1 + udev/90-libinput-model-quirks.hwdb | 3 +++ 4 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index f249116..0f72807 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -1951,13 +1951,29 @@ tp_init_default_resolution(struct tp_dispatch *tp, return 0; } +static inline void +tp_init_hysteresis(struct tp_dispatch *tp) +{ + int res_x, res_y; + + res_x = tp->device->abs.absinfo_x->resolution; + res_y = tp->device->abs.absinfo_y->resolution; + + if (tp->device->model_flags & EVDEV_MODEL_CYAPA) { + tp->hysteresis_margin.x = res_x/2; + tp->hysteresis_margin.y = res_y/2; + } else { + tp->hysteresis_margin.x = 0; + tp->hysteresis_margin.y = 0; + } +} + static int tp_init(struct tp_dispatch *tp, struct evdev_device *device) { int width, height; double diagonal; - int res_x, res_y; tp->base.interface = &tp_interface; tp->device = device; @@ -1971,8 +1987,6 @@ tp_init(struct tp_dispatch *tp, if (tp_init_slots(tp, device) != 0) return -1; - res_x = tp->device->abs.absinfo_x->resolution; - res_y = tp->device->abs.absinfo_y->resolution; width = device->abs.dimensions.x; height = device->abs.dimensions.y; diagonal = sqrt(width*width + height*height); @@ -1981,8 +1995,7 @@ tp_init(struct tp_dispatch *tp, EV_ABS, ABS_MT_DISTANCE); - tp->hysteresis_margin.x = res_x/2; - tp->hysteresis_margin.y = res_y/2; + tp_init_hysteresis(tp); if (tp_init_accel(tp, diagonal) != 0) return -1; diff --git a/src/evdev.c b/src/evdev.c index 66673a8..473ff63 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -1678,6 +1678,7 @@ evdev_read_model_flags(struct evdev_device *device) { "LIBINPUT_MODEL_ELANTECH_TOUCHPAD", EVDEV_MODEL_ELANTECH_TOUCHPAD }, { "LIBINPUT_MODEL_APPLE_INTERNAL_KEYBOARD", EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD }, { "LIBINPUT_MODEL_CYBORG_RAT", EVDEV_MODEL_CYBORG_RAT }, + { "LIBINPUT_MODEL_CYAPA", EVDEV_MODEL_CYAPA }, { NULL, EVDEV_MODEL_DEFAULT }, }; const struct model_map *m = model_map; diff --git a/src/evdev.h b/src/evdev.h index 8b567a8..b164af8 100644 --- a/src/evdev.h +++ b/src/evdev.h @@ -111,6 +111,7 @@ enum evdev_device_model { EVDEV_MODEL_LENOVO_X220_TOUCHPAD_FW81 = (1 << 12), EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD = (1 << 13), EVDEV_MODEL_CYBORG_RAT = (1 << 14), + EVDEV_MODEL_CYAPA = (1 << 15), }; struct mt_slot { diff --git a/udev/90-libinput-model-quirks.hwdb b/udev/90-libinput-model-quirks.hwdb index fa668d6..f23a7f9 100644 --- a/udev/90-libinput-model-quirks.hwdb +++ b/udev/90-libinput-model-quirks.hwdb @@ -73,6 +73,9 @@ libinput:name:Cypress APA Trackpad (cyapa):dmi:*svn*SAMSUNG*:pn*Lumpy* libinput:name:Atmel maXTouch Touchpad:dmi:*svn*GOOGLE*:pn*Samus* LIBINPUT_MODEL_CHROMEBOOK=1 +libinput:name:Cypress APA Trackpad (cyapa):dmi:* + LIBINPUT_MODEL_CYAPA=1 + ## # LENOVO #
Re: [PATCH libinput] touchpad: if we have a serio keyboard, override any previous dwt pairing
Hi, On 04-02-16 08:23, Peter Hutterer wrote: If a USB keyboard like the YubiKey is found before the internal keyboard, it will be paired with the touchpad when it is seen. The internal keyboard is seen later but ignored because we already have a keyboard paired with the touchpad. This is obviously wrong. For now, give priority to serio keyboards, and override existing dwt pairings with the new keyboard. https://bugs.freedesktop.org/show_bug.cgi?id=93983 Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards. Hans --- src/evdev-mt-touchpad.c| 48 --- test/Makefile.am | 2 + test/litest-device-synaptics-i2c.c | 102 ++ test/litest-device-yubikey.c | 169 + test/litest.c | 4 + test/litest.h | 2 + test/touchpad.c| 103 ++ 7 files changed, 416 insertions(+), 14 deletions(-) create mode 100644 test/litest-device-synaptics-i2c.c create mode 100644 test/litest-device-yubikey.c diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index d0d1ddd..2be9d40 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -1336,6 +1336,38 @@ tp_want_dwt(struct evdev_device *touchpad, } static void +tp_dwt_pair_keyboard(struct evdev_device *touchpad, +struct evdev_device *keyboard) +{ + struct tp_dispatch *tp = (struct tp_dispatch*)touchpad->dispatch; + unsigned int bus_kbd = libevdev_get_id_bustype(keyboard->evdev); + + if (!tp_want_dwt(touchpad, keyboard)) + return; + + /* If we already have a keyboard paired, override it if the new one +* is a serio device. Otherwise keep the current one */ + if (tp->dwt.keyboard) { + if (bus_kbd != BUS_I8042) + return; + + memset(tp->dwt.key_mask, 0, sizeof(tp->dwt.key_mask)); + libinput_device_remove_event_listener(&tp->dwt.keyboard_listener); + } + + libinput_device_add_event_listener(&keyboard->base, + &tp->dwt.keyboard_listener, + tp_keyboard_event, tp); + tp->dwt.keyboard = keyboard; + tp->dwt.keyboard_active = false; + + log_debug(touchpad->base.seat->libinput, + "palm: dwt activated with %s<->%s\n", + touchpad->devname, + keyboard->devname); +} + +static void tp_interface_device_added(struct evdev_device *device, struct evdev_device *added_device) { @@ -1359,20 +1391,8 @@ tp_interface_device_added(struct evdev_device *device, tp_trackpoint_event, tp); } - if (added_device->tags & EVDEV_TAG_KEYBOARD && - tp->dwt.keyboard == NULL && - tp_want_dwt(device, added_device)) { - log_debug(tp_libinput_context(tp), - "palm: dwt activated with %s<->%s\n", - device->devname, - added_device->devname); - - libinput_device_add_event_listener(&added_device->base, - &tp->dwt.keyboard_listener, - tp_keyboard_event, tp); - tp->dwt.keyboard = added_device; - tp->dwt.keyboard_active = false; - } + if (added_device->tags & EVDEV_TAG_KEYBOARD) + tp_dwt_pair_keyboard(device, added_device); if (tp->sendevents.current_mode != LIBINPUT_CONFIG_SEND_EVENTS_DISABLED_ON_EXTERNAL_MOUSE) diff --git a/test/Makefile.am b/test/Makefile.am index 27a2a36..c7e68ef 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -38,6 +38,7 @@ liblitest_la_SOURCES = \ litest-device-qemu-usb-tablet.c \ litest-device-synaptics.c \ litest-device-synaptics-hover.c \ + litest-device-synaptics-i2c.c \ litest-device-synaptics-st.c \ litest-device-synaptics-t440.c \ litest-device-synaptics-x1-carbon-3rd.c \ @@ -53,6 +54,7 @@ liblitest_la_SOURCES = \ litest-device-wheel-only.c \ litest-device-xen-virtual-pointer.c \ litest-device-vmware-virtual-usb-mouse.c \ + litest-device-yubikey.c \ litest.c liblitest_la_LIBADD = $(top_builddir)/src/libinput-util.la liblitest_la_CFLAGS = $(AM_CFLAGS) \ diff --git a/test/litest-device-synaptics-i2c.c b/test/litest-device-synaptics-i2c.c new file mode 100644 index 000..b6b632b --- /dev/null +++ b/test/litest-device-synaptics-i2c.c @@ -0,0 +1,102 @@ +/* + * Copyright © 2015 Red Hat, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining
Re: [PATCH libinput] touchpad: while a key is held down, don't disable dwt
Hi, On 04-02-16 01:33, Peter Hutterer wrote: If a key enables dwt and is held down when the timeout expires, re-issue the timeout. There is a corner case where dwt may not work as expected: 1. key down and held down 2. dwt timer expires, dwt is re-issued 3. touch starts 4. key is released 5. dwt timer expires 6. touch now starts moving the pointer This is an effect of the smart touch detection. A touch starting after the last key press is released for pointer motion once dwt turns off again. This is what happens in the above case, the dwt timer expiring is the last virtual key press. This is a corner case and likely hard to trigger by a real user. https://bugs.freedesktop.org/show_bug.cgi?id=93984 Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards. Hans --- src/evdev-mt-touchpad.c | 18 +- src/evdev-mt-touchpad.h | 1 + src/libinput-util.h | 14 + test/touchpad.c | 142 4 files changed, 173 insertions(+), 2 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index f249116..d0d1ddd 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -1196,6 +1196,15 @@ tp_keyboard_timeout(uint64_t now, void *data) { struct tp_dispatch *tp = data; + if (long_any_bit_set(tp->dwt.key_mask, +ARRAY_LENGTH(tp->dwt.key_mask))) { + libinput_timer_set(&tp->dwt.keyboard_timer, + now + DEFAULT_KEYBOARD_ACTIVITY_TIMEOUT_2); + tp->dwt.keyboard_last_press_time = now; + log_debug(tp_libinput_context(tp), "palm: keyboard timeout refresh\n"); + return; + } + tp_tap_resume(tp, now); tp->dwt.keyboard_active = false; @@ -1240,6 +1249,7 @@ tp_keyboard_event(uint64_t time, struct libinput_event *event, void *data) struct tp_dispatch *tp = data; struct libinput_event_keyboard *kbdev; unsigned int timeout; + unsigned int key; if (!tp->dwt.dwt_enabled) return; @@ -1248,15 +1258,18 @@ tp_keyboard_event(uint64_t time, struct libinput_event *event, void *data) return; kbdev = libinput_event_get_keyboard_event(event); + key = libinput_event_keyboard_get_key(kbdev); /* Only trigger the timer on key down. */ if (libinput_event_keyboard_get_key_state(kbdev) != - LIBINPUT_KEY_STATE_PRESSED) + LIBINPUT_KEY_STATE_PRESSED) { + long_clear_bit(tp->dwt.key_mask, key); return; + } /* modifier keys don't trigger disable-while-typing so things like * ctrl+zoom or ctrl+click are possible */ - if (tp_key_ignore_for_dwt(libinput_event_keyboard_get_key(kbdev))) + if (tp_key_ignore_for_dwt(key)) return; if (!tp->dwt.keyboard_active) { @@ -1270,6 +1283,7 @@ tp_keyboard_event(uint64_t time, struct libinput_event *event, void *data) } tp->dwt.keyboard_last_press_time = time; + long_set_bit(tp->dwt.key_mask, key); libinput_timer_set(&tp->dwt.keyboard_timer, time + timeout); } diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h index 0053122..87d34b2 100644 --- a/src/evdev-mt-touchpad.h +++ b/src/evdev-mt-touchpad.h @@ -343,6 +343,7 @@ struct tp_dispatch { struct libinput_event_listener keyboard_listener; struct libinput_timer keyboard_timer; struct evdev_device *keyboard; + unsigned long key_mask[NLONGS(KEY_CNT)]; uint64_t keyboard_last_press_time; } dwt; diff --git a/src/libinput-util.h b/src/libinput-util.h index 6adbbc9..522c19c 100644 --- a/src/libinput-util.h +++ b/src/libinput-util.h @@ -25,6 +25,7 @@ #ifndef LIBINPUT_UTIL_H #define LIBINPUT_UTIL_H +#include #include #include #include @@ -171,6 +172,19 @@ long_set_bit_state(unsigned long *array, int bit, int state) long_clear_bit(array, bit); } +static inline int +long_any_bit_set(unsigned long *array, size_t size) +{ + unsigned long i; + + assert(size > 0); + + for (i = 0; i < size; i++) + if (array[i] != 0) + return 1; + return 0; +} + struct matrix { float val[3][3]; /* [row][col] */ }; diff --git a/test/touchpad.c b/test/touchpad.c index 9376cd5..0d3aa03 100644 --- a/test/touchpad.c +++ b/test/touchpad.c @@ -2438,6 +2438,145 @@ START_TEST(touchpad_dwt_key_hold) } END_TEST +START_TEST(touchpad_dwt_key_hold_timeout) +{ + struct litest_device *touchpad = litest_current_device(); + struct litest_device *keyboard; + struct libinput *li = touchpad->libinput; + + if (!has_disable_while_typing(touchpad)) + r
Re: [PATCH libinput] touchpad: fix dwt disabling while a key is still down
Hi, On 08-02-16 03:51, Peter Hutterer wrote: If dwt is disabled on the commandline, e.g. by setting an xinput property it may be disabled before the release event comes in. This caused the timer to refresh indefinitely since the key state mask was still on for that key. Always updating the key state mask (even when dwt is disabled) fixes that. If a key is held down while dwt is disabled, this can still cause a indefinite timer refresh, so in the timer func, check if dwt is enabled before refreshing the timer. https://bugs.freedesktop.org/show_bug.cgi?id=94015 Signed-off-by: Peter Hutterer Looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-mt-touchpad.c | 9 --- test/touchpad.c | 68 + 2 files changed, 73 insertions(+), 4 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index f7d5d87..1f654e4 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -1196,7 +1196,8 @@ tp_keyboard_timeout(uint64_t now, void *data) { struct tp_dispatch *tp = data; - if (long_any_bit_set(tp->dwt.key_mask, + if (tp->dwt.dwt_enabled && + long_any_bit_set(tp->dwt.key_mask, ARRAY_LENGTH(tp->dwt.key_mask))) { libinput_timer_set(&tp->dwt.keyboard_timer, now + DEFAULT_KEYBOARD_ACTIVITY_TIMEOUT_2); @@ -1251,9 +1252,6 @@ tp_keyboard_event(uint64_t time, struct libinput_event *event, void *data) unsigned int timeout; unsigned int key; - if (!tp->dwt.dwt_enabled) - return; - if (event->type != LIBINPUT_EVENT_KEYBOARD_KEY) return; @@ -1267,6 +1265,9 @@ tp_keyboard_event(uint64_t time, struct libinput_event *event, void *data) return; } + if (!tp->dwt.dwt_enabled) + return; + /* modifier keys don't trigger disable-while-typing so things like * ctrl+zoom or ctrl+click are possible */ if (tp_key_ignore_for_dwt(key)) diff --git a/test/touchpad.c b/test/touchpad.c index c0ddc53..3574edd 100644 --- a/test/touchpad.c +++ b/test/touchpad.c @@ -3114,6 +3114,72 @@ START_TEST(touchpad_dwt_disable_before_touch) } END_TEST +START_TEST(touchpad_dwt_disable_during_key_release) +{ + struct litest_device *touchpad = litest_current_device(); + struct litest_device *keyboard; + struct libinput *li = touchpad->libinput; + + if (!has_disable_while_typing(touchpad)) + return; + + enable_dwt(touchpad); + + keyboard = dwt_init_paired_keyboard(li, touchpad); + litest_disable_tap(touchpad->libinput_device); + litest_drain_events(li); + + litest_keyboard_key(keyboard, KEY_A, true); + litest_assert_only_typed_events(li, LIBINPUT_EVENT_KEYBOARD_KEY); + + disable_dwt(touchpad); + libinput_dispatch(li); + litest_keyboard_key(keyboard, KEY_A, false); + litest_assert_only_typed_events(li, LIBINPUT_EVENT_KEYBOARD_KEY); + + /* touch down during timeout, wait, should generate events */ + litest_touch_down(touchpad, 0, 50, 50); + libinput_dispatch(li); + litest_timeout_dwt_long(); + litest_touch_move_to(touchpad, 0, 70, 50, 50, 50, 10, 1); + litest_assert_only_typed_events(li, LIBINPUT_EVENT_POINTER_MOTION); + + litest_delete_device(keyboard); +} +END_TEST + +START_TEST(touchpad_dwt_disable_during_key_hold) +{ + struct litest_device *touchpad = litest_current_device(); + struct litest_device *keyboard; + struct libinput *li = touchpad->libinput; + + if (!has_disable_while_typing(touchpad)) + return; + + enable_dwt(touchpad); + + keyboard = dwt_init_paired_keyboard(li, touchpad); + litest_disable_tap(touchpad->libinput_device); + litest_drain_events(li); + + litest_keyboard_key(keyboard, KEY_A, true); + litest_assert_only_typed_events(li, LIBINPUT_EVENT_KEYBOARD_KEY); + + disable_dwt(touchpad); + libinput_dispatch(li); + + /* touch down during timeout, wait, should generate events */ + litest_touch_down(touchpad, 0, 50, 50); + libinput_dispatch(li); + litest_timeout_dwt_long(); + litest_touch_move_to(touchpad, 0, 70, 50, 50, 50, 10, 1); + litest_assert_only_typed_events(li, LIBINPUT_EVENT_POINTER_MOTION); + + litest_delete_device(keyboard); +} +END_TEST + START_TEST(touchpad_dwt_enable_during_touch) { struct litest_device *touchpad = litest_current_device(); @@ -3925,6 +3991,8 @@ litest_setup_tests(void) litest_add("touchpad:dwt", touchpad_dwt_disabled, LITEST_TOUCHPAD, LITEST_ANY); litest_add("touchpad:dwt", touchpad_dwt_disable_during_touch, LITEST_TOUCHPAD, LITEST_ANY); litest_add("touchpad:dwt", t
Pre-fosdem cross desktop wayland meetup notes
Hi All, Just like last year there was a pre-fosdem cross desktop wayland meetup on the Friday before fosdem. Unfortunately this year we did not get as diverse a group as last time. Here are some short notes in case anyone is interested what was discussed. Present were: Derek Foreman (Samsung, EFL/Enlightenment) Mike Blumenkrantz (Samsung, EFL/Enlightenment) Andrew Williams (EFL/Enlightenment) Olivier Fourdan (Red Hat, graphics team) Benjamin Tissoires (Red Hat, graphics team) Hans de Goede (Red Hat, graphics team) Discussed: -Menu position stuff, basically: https://bugzilla.gnome.org/show_bug.cgi?id=756579 Looks good from both EFL and Jonas Adahl's pov -Menu position will use a new xdg_popup type: http://lists.freedesktop.org/archives/wayland-devel/2016-January/026494.html -This patch-set also introduces a xdg_tooltip type: http://lists.freedesktop.org/archives/wayland-devel/2016-January/026496.html We discussed that this is a bad name, and it would probably be better to have xdg_popup be more generic and/or have a special version which does not need a grab, does have the xdg_popup positioning stuff, but is not called tooltip as that is too specific -Tiling window managers: -Need a way to tell windows to not generate drop-shadows -Would also be useful for maximized windows -DrawModeEnum which communicates to clients how to draw their client side window decorations, e.g. "CSD with no dropshadow": https://gist.github.com/zmike/0a5f3a3da7b86da9893c -Maybe add a value to the enum to allow a "No CSD" drawmode (like X11) which will likely be useful for the KDE people who would like to be able to run without CSD. -Took a look at last years list of dicussed items: http://lists.freedesktop.org/archives/xdg/2015-July/013527.html -Discussed global-keybindings again. Enlightenment does have feature requests for this, so just saying we don't do that may not cut it -Took a look at: https://fedoraproject.org/wiki/Wayland_features -Started discussing what we need for startup notication -Discussed notifications of progress (think download-manager), maybe extend: https://people.gnome.org/~mccann/docs/notification-spec/notification-spec-latest.html With progress fields so that a persistent notification with a progress bar can be shown while a download is in progress (such as is done in android when downloading a large app from the appstore) -How to help people understand / debug wayland problems: -Quickly turned into discussion how to get people to file good Wayland bugs / to provide the right info to help developers debug their problems -No clue -Discussed startup-notification, should pretty much be a copy and paste of X spec to wayland using propertoes on xdg_shell toplevel surface to communicate the launcher clicked time for focus stealing prevention, etc. Discussed this at Fosdem with Carlos Garnacho, he is interested in looking into this / implementing this. Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 1/2] udev: fix ALPS firmware detection
Hi, On 09-02-16 03:12, Peter Hutterer wrote: The firmware version is in id.version, not id.model which is always PSMOUSE_ALPS for ALPS devices. The various fw versions are listed in /drivers/input/mouse/alps.h and are all hex numbers. Version 8 is actually 0x800, change the match accordingly. Expected side-effect: earlier versions of ALPS touchpads now lose their (erroneous) size assignment. Signed-off-by: Peter Hutterer Both patches look good to me and are: Reviewed-by: Hans de Goede Thanks & Regards, Hans --- udev/90-libinput-model-quirks.hwdb | 4 ++-- udev/libinput-model-quirks.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/udev/90-libinput-model-quirks.hwdb b/udev/90-libinput-model-quirks.hwdb index f23a7f9..6225da1 100644 --- a/udev/90-libinput-model-quirks.hwdb +++ b/udev/90-libinput-model-quirks.hwdb @@ -22,8 +22,8 @@ libinput:name:*AlpsPS/2 ALPS DualPoint TouchPad:dmi:* libinput:name:*AlpsPS/2 ALPS GlidePoint:dmi:* LIBINPUT_MODEL_ALPS_TOUCHPAD=1 -libinput:name:*AlpsPS/2 ALPS DualPoint TouchPad:fwversion:8 -libinput:name:*AlpsPS/2 ALPS GlidePoint:fwversion:8 +libinput:name:*AlpsPS/2 ALPS DualPoint TouchPad:fwversion:800 +libinput:name:*AlpsPS/2 ALPS GlidePoint:fwversion:800 LIBINPUT_ATTR_SIZE_HINT=100x55 ## diff --git a/udev/libinput-model-quirks.c b/udev/libinput-model-quirks.c index 67115fa..c8baae7 100644 --- a/udev/libinput-model-quirks.c +++ b/udev/libinput-model-quirks.c @@ -63,9 +63,9 @@ handle_touchpad_alps(struct udev_device *device) if (sscanf(product, "%x/%x/%x/%x", &bus, &vid, &pid, &version) != 4) return; - /* ALPS' firmware version is the PID */ + /* ALPS' firmware version is the version */ if (pid) - printf("LIBINPUT_MODEL_FIRMWARE_VERSION=%d\n", pid); + printf("LIBINPUT_MODEL_FIRMWARE_VERSION=%x\n", version); } static void ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] tablet: add hwdb entries to ignore Wacom Cintiq offsets
Hi, On 16-02-16 06:47, Peter Hutterer wrote: Wacom Cintiqs and some DTK/DTU devices have a sensor larger than the underlying display. Clamp any data outside the screen area to the screen area. Signed-off-by: Peter Hutterer --- src/evdev-tablet.c | 27 + src/evdev.c| 1 + src/evdev.h| 1 + test/tablet.c | 8 udev/90-libinput-model-quirks.hwdb | 36 ++ udev/90-libinput-model-quirks.rules.in | 4 6 files changed, 65 insertions(+), 12 deletions(-) diff --git a/src/evdev-tablet.c b/src/evdev-tablet.c index 1e5c2cd..50ecf25 100644 --- a/src/evdev-tablet.c +++ b/src/evdev-tablet.c @@ -244,9 +244,9 @@ adjust_tilt(const struct input_absinfo *absinfo) } static inline int32_t -invert_axis(const struct input_absinfo *absinfo) +invert_axis(const struct input_absinfo *absinfo, int value) { - return absinfo->maximum - (absinfo->value - absinfo->minimum); + return absinfo->maximum - (value - absinfo->minimum); } static void @@ -292,6 +292,18 @@ normalize_wheel(struct tablet_dispatch *tablet, return value * device->scroll.wheel_click_angle; } +static inline int +tablet_get_ranged_value(struct evdev_device *device, + const struct input_absinfo *abs) +{ + int value = abs->value; + + if (device->model_flags & EVDEV_MODEL_WACOM_SENSOR_OFFSET) + value = max(min(value, abs->maximum), abs->minimum); + + return value; +} + static inline void tablet_handle_xy(struct tablet_dispatch *tablet, struct evdev_device *device, @@ -305,26 +317,25 @@ tablet_handle_xy(struct tablet_dispatch *tablet, if (bit_is_set(tablet->changed_axes, LIBINPUT_TABLET_TOOL_AXIS_X)) { absinfo = libevdev_get_abs_info(device->evdev, ABS_X); + value = tablet_get_ranged_value(device, absinfo); if (device->left_handed.enabled) - value = invert_axis(absinfo); - else - value = absinfo->value; + value = invert_axis(absinfo, value); if (!tablet_has_status(tablet, TABLET_TOOL_ENTERING_PROXIMITY)) delta.x = value - tablet->axes.point.x; tablet->axes.point.x = value; + } point.x = tablet->axes.point.x; This seems like an extra empty line accidentally got added. Otherwise this patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans if (bit_is_set(tablet->changed_axes, LIBINPUT_TABLET_TOOL_AXIS_Y)) { absinfo = libevdev_get_abs_info(device->evdev, ABS_Y); + value = tablet_get_ranged_value(device, absinfo); if (device->left_handed.enabled) - value = invert_axis(absinfo); - else - value = absinfo->value; + value = invert_axis(absinfo, value); if (!tablet_has_status(tablet, TABLET_TOOL_ENTERING_PROXIMITY)) diff --git a/src/evdev.c b/src/evdev.c index 51768fe..ccfd7e4 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -1680,6 +1680,7 @@ evdev_read_model_flags(struct evdev_device *device) { "LIBINPUT_MODEL_CYBORG_RAT", EVDEV_MODEL_CYBORG_RAT }, { "LIBINPUT_MODEL_CYAPA", EVDEV_MODEL_CYAPA }, { "LIBINPUT_MODEL_ALPS_RUSHMORE", EVDEV_MODEL_ALPS_RUSHMORE }, + { "LIBINPUT_MODEL_WACOM_SENSOR_OFFSET", EVDEV_MODEL_WACOM_SENSOR_OFFSET }, { NULL, EVDEV_MODEL_DEFAULT }, }; const struct model_map *m = model_map; diff --git a/src/evdev.h b/src/evdev.h index 482712b..80f1606 100644 --- a/src/evdev.h +++ b/src/evdev.h @@ -113,6 +113,7 @@ enum evdev_device_model { EVDEV_MODEL_CYBORG_RAT = (1 << 14), EVDEV_MODEL_CYAPA = (1 << 15), EVDEV_MODEL_ALPS_RUSHMORE = (1 << 16), + EVDEV_MODEL_WACOM_SENSOR_OFFSET = (1 << 17), }; struct mt_slot { diff --git a/test/tablet.c b/test/tablet.c index c5dc892..b1f7160 100644 --- a/test/tablet.c +++ b/test/tablet.c @@ -1682,12 +1682,12 @@ START_TEST(motion_outside_bounds) tablet_event = litest_is_tablet_event(event, LIBINPUT_EVENT_TABLET_TOOL_AXIS); val = libinput_event_tablet_tool_get_x(tablet_event); - ck_assert_double_lt(val, 0.0); + ck_assert_double_eq(val, 0.0); val = libinput_event_tablet_tool_get_y(tablet_event); ck_assert_double_gt(val, 0.0); val = libinput_event_tablet_tool_get_x_transformed(tablet_event, 100); - ck_assert_double_lt(val, 0.0); +
Re: [PATCH libinput] touchpad: move the tapping exclusion zone to the top edge of the button
Hi, On 17-02-16 00:29, Peter Hutterer wrote: We previously used the half-way mark of the touchpad's y axis to decide where to ignore tapping. Move this down to the top edge of the software buttons instead. Users may tap with a finger in the software button areas, on the rest of the touchpad it's unlikely that they tap within 5% of the edge. On touchpads with physical buttons or if clickfinger is enabled, the no-tapping zone extends to the bottom of the touchpad. This required splitting the tests into clickfinger, softbuttons and hardbuttons. https://bugs.freedesktop.org/show_bug.cgi?id=93947 Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-mt-touchpad.c | 11 +++- src/evdev-mt-touchpad.h | 1 - test/touchpad.c | 72 ++--- 3 files changed, 72 insertions(+), 12 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index d8b2334..78d18a4 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -538,9 +538,9 @@ tp_palm_tap_is_palm(const struct tp_dispatch *tp, const struct tp_touch *t) t->point.x < tp->palm.right_edge) return false; - /* We're inside the left/right palm edge and in the northern half of -* the touchpad - this tap is a palm */ - if (t->point.y < tp->palm.vert_center) { + /* We're inside the left/right palm edge and not in one of the +* software button areas */ + if (t->point.y < tp->buttons.bottom_area.top_edge) { log_debug(tp_libinput_context(tp), "palm: palm-tap detected\n"); return true; @@ -1824,14 +1824,12 @@ static int tp_init_palmdetect(struct tp_dispatch *tp, struct evdev_device *device) { - int width, height; + int width; tp->palm.right_edge = INT_MAX; tp->palm.left_edge = INT_MIN; - tp->palm.vert_center = INT_MIN; width = device->abs.dimensions.x; - height = device->abs.dimensions.y; /* Wacom doesn't have internal touchpads */ if (device->model_flags & EVDEV_MODEL_WACOM_TOUCHPAD) @@ -1845,7 +1843,6 @@ tp_init_palmdetect(struct tp_dispatch *tp, /* palm edges are 5% of the width on each side */ tp->palm.right_edge = device->abs.absinfo_x->maximum - width * 0.05; tp->palm.left_edge = device->abs.absinfo_x->minimum + width * 0.05; - tp->palm.vert_center = device->abs.absinfo_y->minimum + height/2; tp->palm.monitor_trackpoint = true; diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h index 87d34b2..eae327b 100644 --- a/src/evdev-mt-touchpad.h +++ b/src/evdev-mt-touchpad.h @@ -321,7 +321,6 @@ struct tp_dispatch { struct { int32_t right_edge; /* in device coordinates */ int32_t left_edge; /* in device coordinates */ - int32_t vert_center;/* in device coordinates */ bool trackpoint_active; struct libinput_event_listener trackpoint_listener; diff --git a/test/touchpad.c b/test/touchpad.c index 3574edd..be9c566 100644 --- a/test/touchpad.c +++ b/test/touchpad.c @@ -1031,7 +1031,7 @@ START_TEST(touchpad_palm_detect_no_palm_moving_into_edges) } END_TEST -START_TEST(touchpad_palm_detect_tap) +START_TEST(touchpad_palm_detect_tap_hardbuttons) { struct litest_device *dev = litest_current_device(); struct libinput *li = dev->libinput; @@ -1051,7 +1051,38 @@ START_TEST(touchpad_palm_detect_tap) litest_touch_up(dev, 0); litest_assert_empty_queue(li); - litest_touch_down(dev, 0, 5, 90); + litest_touch_down(dev, 0, 5, 99); + litest_touch_up(dev, 0); + litest_assert_empty_queue(li); + + litest_touch_down(dev, 0, 95, 99); + litest_touch_up(dev, 0); + litest_assert_empty_queue(li); +} +END_TEST + +START_TEST(touchpad_palm_detect_tap_softbuttons) +{ + struct litest_device *dev = litest_current_device(); + struct libinput *li = dev->libinput; + + if (!touchpad_has_palm_detect_size(dev)) + return; + + litest_enable_tap(dev->libinput_device); + litest_enable_buttonareas(dev); + + litest_drain_events(li); + + litest_touch_down(dev, 0, 95, 5); + litest_touch_up(dev, 0); + litest_assert_empty_queue(li); + + litest_touch_down(dev, 0, 5, 5); + litest_touch_up(dev, 0); + litest_assert_empty_queue(li); + + litest_touch_down(dev, 0, 5, 99); litest_touch_up(dev, 0); litest_assert_button_event(li, BTN_LEFT, @@ -1061,7 +1092,7 @@ START_TEST(touchpad_palm_detect_tap) LIBINPUT_BUTTON_STATE_REL
Re: [PATCH libinput] touchpad: add synaptics semi-mt devices to those needing hysteresis
Hi, On 17-02-16 01:11, Peter Hutterer wrote: https://bugs.freedesktop.org/show_bug.cgi?id=94097 Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-mt-touchpad.c | 27 +++ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index d8b2334..912a0f0 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -1991,17 +1991,28 @@ tp_init_hysteresis(struct tp_dispatch *tp) { int res_x, res_y; + if (tp->device->model_flags & EVDEV_MODEL_CYAPA) + goto want_hysteresis; + + if (tp->device->model_flags & EVDEV_MODEL_ALPS_RUSHMORE) + goto want_hysteresis; + + if (tp->semi_mt && + (tp->device->model_flags & EVDEV_MODEL_SYNAPTICS_SERIAL_TOUCHPAD)) + goto want_hysteresis; + + tp->hysteresis_margin.x = 0; + tp->hysteresis_margin.y = 0; + + return; + +want_hysteresis: res_x = tp->device->abs.absinfo_x->resolution; res_y = tp->device->abs.absinfo_y->resolution; - if (tp->device->model_flags & - (EVDEV_MODEL_CYAPA|EVDEV_MODEL_ALPS_RUSHMORE)) { - tp->hysteresis_margin.x = res_x/2; - tp->hysteresis_margin.y = res_y/2; - } else { - tp->hysteresis_margin.x = 0; - tp->hysteresis_margin.y = 0; - } + tp->hysteresis_margin.x = res_x/2; + tp->hysteresis_margin.y = res_y/2; + return; } static int ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: if the second finger is within 20x5mm, bias towards 2fg scrolling
Hi, On 11-02-16 21:56, Peter Hutterer wrote: Scrolling is much more common than a 2fg spread gesture, so if the finger position indicates that the fingers are next to each other, switch to scrolling immediately. https://bugs.freedesktop.org/show_bug.cgi?id=93504 Signed-off-by: Peter Hutterer Sorry for being a bit slow on reviewing this one, this seems to be a good solution to me (I consider false positives unlikely): Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-mt-touchpad-gestures.c | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/evdev-mt-touchpad-gestures.c b/src/evdev-mt-touchpad-gestures.c index dc8d606..a9d7fa1 100644 --- a/src/evdev-mt-touchpad-gestures.c +++ b/src/evdev-mt-touchpad-gestures.c @@ -325,8 +325,9 @@ tp_gesture_handle_state_unknown(struct tp_dispatch *tp, uint64_t time) struct tp_touch *first = tp->gesture.touches[0], *second = tp->gesture.touches[1]; int dir1, dir2; - int yres = tp->device->abs.absinfo_y->resolution; - int vert_distance; + int yres = tp->device->abs.absinfo_y->resolution, + xres = tp->device->abs.absinfo_x->resolution; + int vert_distance, horiz_distance; /* for two-finger gestures, if the fingers stay unmoving for a * while, assume (slow) scroll */ @@ -338,10 +339,16 @@ tp_gesture_handle_state_unknown(struct tp_dispatch *tp, uint64_t time) /* Else check if one finger is > 20mm below the others */ vert_distance = abs(first->point.y - second->point.y); + horiz_distance = abs(first->point.x - second->point.x); if (vert_distance > 20 * yres && tp->gesture.enabled) { tp_gesture_init_pinch(tp); return GESTURE_STATE_PINCH; + /* Else if the fingers are within 20x5mm of each other */ + } else if (vert_distance < 5 * yres && + horiz_distance < 20 * xres) { + tp_gesture_set_scroll_buildup(tp); + return GESTURE_STATE_SCROLL; } /* Else wait for both fingers to have moved */ ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: only trigger immediate pinch detection for three fingers
Hi, On 26-02-16 01:49, Peter Hutterer wrote: If the fingers are vertically apart by more than 20mm we used to assume a pinch gesture immediately. This is too close together for some users during two-finger scrolling. Since we should always bias towards scrolling, only trigger this detection for three fingers, the rest has to go through the movement detection. The reason for the pinch detection here was to differentiate from 3fg swipe gestures (83f3dbd1), hence we're still in spirit of that patch. https://bugs.freedesktop.org/show_bug.cgi?id=94264 Signed-off-by: Peter Hutterer Looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-mt-touchpad-gestures.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/evdev-mt-touchpad-gestures.c b/src/evdev-mt-touchpad-gestures.c index dc8d606..3c8f5a7 100644 --- a/src/evdev-mt-touchpad-gestures.c +++ b/src/evdev-mt-touchpad-gestures.c @@ -339,6 +339,7 @@ tp_gesture_handle_state_unknown(struct tp_dispatch *tp, uint64_t time) /* Else check if one finger is > 20mm below the others */ vert_distance = abs(first->point.y - second->point.y); if (vert_distance > 20 * yres && + tp->gesture.finger_count > 2 && tp->gesture.enabled) { tp_gesture_init_pinch(tp); return GESTURE_STATE_PINCH; ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: add quirk for the T450 and T460 generation hardware
scard that first +* event with the jump. +*/ + unsigned int nonmotion_event_count; + } quirks; }; #define tp_for_each_touch(_tp, _t) \ diff --git a/src/evdev.c b/src/evdev.c index 51768fe..a5c965d 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -1680,6 +1680,7 @@ evdev_read_model_flags(struct evdev_device *device) { "LIBINPUT_MODEL_CYBORG_RAT", EVDEV_MODEL_CYBORG_RAT }, { "LIBINPUT_MODEL_CYAPA", EVDEV_MODEL_CYAPA }, { "LIBINPUT_MODEL_ALPS_RUSHMORE", EVDEV_MODEL_ALPS_RUSHMORE }, + { "LIBINPUT_MODEL_LENOVO_T450_TOUCHPAD", EVDEV_MODEL_LENOVO_T450_TOUCHPAD }, { NULL, EVDEV_MODEL_DEFAULT }, }; const struct model_map *m = model_map; diff --git a/src/evdev.h b/src/evdev.h index 482712b..4a5d807 100644 --- a/src/evdev.h +++ b/src/evdev.h @@ -113,6 +113,7 @@ enum evdev_device_model { EVDEV_MODEL_CYBORG_RAT = (1 << 14), EVDEV_MODEL_CYAPA = (1 << 15), EVDEV_MODEL_ALPS_RUSHMORE = (1 << 16), + EVDEV_MODEL_LENOVO_T450_TOUCHPAD= (1 << 17), }; struct mt_slot { diff --git a/udev/90-libinput-model-quirks.hwdb b/udev/90-libinput-model-quirks.hwdb index eb2859e..d5978f7 100644 --- a/udev/90-libinput-model-quirks.hwdb +++ b/udev/90-libinput-model-quirks.hwdb @@ -100,6 +100,13 @@ libinput:name:Cypress APA Trackpad (cyapa):dmi:* libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnLENOVO:*:pvrThinkPadX230* LIBINPUT_MODEL_LENOVO_X230=1 +# Lenovo T450/T460 and all other Lenovos of the *50 and *60 generation, +# including the X1 Carbon 3rd gen +libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnLENOVO:*:pvrThinkPad??50*: +libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnLENOVO:*:pvrThinkPad??60*: +libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnLENOVO:*:pvrThinkPadX1Carbon3rd:* + LIBINPUT_MODEL_LENOVO_T450_TOUCHPAD=1 + ## # Synaptics ## Otherwise this patch looks good to me and is: Reviewed-by: Hans de Goede Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: fix left-handed top software trackpoint buttons
Hi, On 31-03-16 03:52, Peter Hutterer wrote: The previous code would swap the top software buttons depending on the touchpad's left-handed setting, not the trackpoint setting. Changing both devices to left-handed resulted in a double-swap, i.e. the trackpoint was always right-handed. https://bugs.freedesktop.org/show_bug.cgi?id=94733 Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-mt-touchpad-buttons.c | 23 --- test/trackpoint.c | 132 2 files changed, 148 insertions(+), 7 deletions(-) diff --git a/src/evdev-mt-touchpad-buttons.c b/src/evdev-mt-touchpad-buttons.c index 82c99c7..076eab0 100644 --- a/src/evdev-mt-touchpad-buttons.c +++ b/src/evdev-mt-touchpad-buttons.c @@ -963,6 +963,7 @@ tp_post_clickpadbutton_buttons(struct tp_dispatch *tp, uint64_t time) uint32_t current, old, button, is_top; enum libinput_button_state state; enum { AREA = 0x01, LEFT = 0x02, MIDDLE = 0x04, RIGHT = 0x08 }; + bool want_left_handed = true; current = tp->buttons.state; old = tp->buttons.old_state; @@ -1008,14 +1009,22 @@ tp_post_clickpadbutton_buttons(struct tp_dispatch *tp, uint64_t time) return 0; } - if ((area & MIDDLE) || ((area & LEFT) && (area & RIGHT))) - button = evdev_to_left_handed(tp->device, BTN_MIDDLE); - else if (area & RIGHT) - button = evdev_to_left_handed(tp->device, BTN_RIGHT); - else if (area & LEFT) - button = evdev_to_left_handed(tp->device, BTN_LEFT); - else /* main or no area (for clickfinger) is always BTN_LEFT */ + if ((area & MIDDLE) || ((area & LEFT) && (area & RIGHT))) { + button = BTN_MIDDLE; + } else if (area & RIGHT) { + button = BTN_RIGHT; + } else if (area & LEFT) { button = BTN_LEFT; + } else { /* main or no area (for clickfinger) is always BTN_LEFT */ + button = BTN_LEFT; + want_left_handed = false; + } + + if (is_top) + want_left_handed = false; + + if (want_left_handed) + button = evdev_to_left_handed(tp->device, button); tp->buttons.active = button; tp->buttons.active_is_topbutton = is_top; diff --git a/test/trackpoint.c b/test/trackpoint.c index 567fba8..5a68b19 100644 --- a/test/trackpoint.c +++ b/test/trackpoint.c @@ -150,6 +150,135 @@ START_TEST(trackpoint_scroll_source) } END_TEST +START_TEST(trackpoint_topsoftbuttons_left_handed_trackpoint) +{ + struct litest_device *touchpad = litest_current_device(); + struct litest_device *trackpoint; + struct libinput *li = touchpad->libinput; + enum libinput_config_status status; + struct libinput_event *event; + struct libinput_device *device; + + trackpoint = litest_add_device(li, LITEST_TRACKPOINT); + litest_drain_events(li); + /* touchpad right-handed, trackpoint left-handed */ + status = libinput_device_config_left_handed_set( + trackpoint->libinput_device, 1); + ck_assert_int_eq(status, LIBINPUT_CONFIG_STATUS_SUCCESS); + + litest_touch_down(touchpad, 0, 5, 5); + libinput_dispatch(li); + litest_button_click(touchpad, BTN_LEFT, true); + libinput_dispatch(li); + + event = libinput_get_event(li); + litest_is_button_event(event, + BTN_RIGHT, + LIBINPUT_BUTTON_STATE_PRESSED); + device = libinput_event_get_device(event); + ck_assert(device == trackpoint->libinput_device); + libinput_event_destroy(event); + + litest_button_click(touchpad, BTN_LEFT, false); + libinput_dispatch(li); + event = libinput_get_event(li); + litest_is_button_event(event, + BTN_RIGHT, + LIBINPUT_BUTTON_STATE_RELEASED); + device = libinput_event_get_device(event); + ck_assert(device == trackpoint->libinput_device); + libinput_event_destroy(event); + + litest_delete_device(trackpoint); +} +END_TEST + +START_TEST(trackpoint_topsoftbuttons_left_handed_touchpad) +{ + struct litest_device *touchpad = litest_current_device(); + struct litest_device *trackpoint; + struct libinput *li = touchpad->libinput; + enum libinput_config_status status; + struct libinput_event *event; + struct libinput_device *device; + + trackpoint = litest_add_device(li, LITEST_TRACKPOINT); +
Re: [PATCH libinput 4/4] touchpad: reset the motion history on significant negative pressure changes
Hi, On 04/01/2016 03:10 AM, Peter Hutterer wrote: Resetting the motion history has the side-effect of swallowing movements, we don't calculate deltas until we have 4 motion events. During a finger release, we're likely to get a large pressure change between two events, resetting the motion history prevents the cursor from jumping on release. The value of 7 found by trial-and-error, tested on the T440 and T450 hardware. The absolute value is highly variable but recordings show that the pressure changes only by 1 or 2 units during normal interaction. Higher pressure changes are during finger position changes but since those should not cause a jump anyway, we tend to win there too. Currently only enabled for negative pressure changes, let's see how we go with that. https://bugs.freedesktop.org/show_bug.cgi?id=94379 Signed-off-by: Peter Hutterer --- src/evdev-mt-touchpad.c | 4 src/evdev-mt-touchpad.h | 1 + test/litest.c | 38 +++--- test/litest.h | 7 +++ test/touchpad.c | 2 +- 5 files changed, 40 insertions(+), 12 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index e16aecb..0640974 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -335,6 +335,7 @@ tp_process_absolute(struct tp_dispatch *tp, tp_end_sequence(tp, t, time); break; case ABS_MT_PRESSURE: + t->pressure_delta = e->value - t->pressure; t->pressure = e->value; t->dirty = true; tp->queued |= TOUCHPAD_EVENT_OTHERAXIS; @@ -946,6 +947,9 @@ tp_process_state(struct tp_dispatch *tp, uint64_t time) if (!t->dirty) continue; + if (t->pressure_delta < -7) + tp_motion_history_reset(t); + Do you really want to do this on all touchpads and not just on t440 / t450 and friends ? Otherwise this series looks good and is: Reviewed-by: Hans de Goede Regards, Hans tp_thumb_detect(tp, t, time); tp_palm_detect(tp, t, time); diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h index 1f05a03..d1dae83 100644 --- a/src/evdev-mt-touchpad.h +++ b/src/evdev-mt-touchpad.h @@ -154,6 +154,7 @@ struct tp_touch { uint64_t millis; int distance; /* distance == 0 means touch */ int pressure; + int pressure_delta; struct { /* A quirk mostly used on Synaptics touchpads. In a diff --git a/test/litest.c b/test/litest.c index f679652..1d729ef 100644 --- a/test/litest.c +++ b/test/litest.c @@ -1494,23 +1494,39 @@ litest_touch_move_extended(struct litest_device *d, } void +litest_touch_move_to_extended(struct litest_device *d, + unsigned int slot, + double x_from, double y_from, + double x_to, double y_to, + struct axis_replacement *axes, + int steps, int sleep_ms) +{ + for (int i = 0; i < steps - 1; i++) { + litest_touch_move_extended(d, slot, + x_from + (x_to - x_from)/steps * i, + y_from + (y_to - y_from)/steps * i, + axes); + if (sleep_ms) { + libinput_dispatch(d->libinput); + msleep(sleep_ms); + libinput_dispatch(d->libinput); + } + } + litest_touch_move_extended(d, slot, x_to, y_to, axes); +} + +void litest_touch_move_to(struct litest_device *d, unsigned int slot, double x_from, double y_from, double x_to, double y_to, int steps, int sleep_ms) { - for (int i = 0; i < steps - 1; i++) { - litest_touch_move(d, slot, - x_from + (x_to - x_from)/steps * i, - y_from + (y_to - y_from)/steps * i); - if (sleep_ms) { - libinput_dispatch(d->libinput); - msleep(sleep_ms); - libinput_dispatch(d->libinput); - } - } - litest_touch_move(d, slot, x_to, y_to); + litest_touch_move_to_extended(d, slot, + x_from, y_from, + x_to, y_to, + NULL, + steps, sleep_ms); } static int diff --git a/test/litest.h b/test/litest.h index e854210..c218361 100644 --- a/test/litest.h +++ b/test/litest.h @@ -403,6 +403,13 @@ litest_touch_move_to(struct litest_device *d, double x_fro
Re: [PATCH libinput] touchpad: add a middle button software area
Hi, On 04-04-16 03:40, Peter Hutterer wrote: Middle button interaction is most commonly to paste and it is a single-event interaction (button press). We provided middle button in software button mode by emulating it with a two-finger press with L+R down at the same time. This is also what many touchpads are spectacularly bad at, it is very common to detect the physical button down event before the second finger registers, resulting in left or right clicks where a middle button should be triggered. Unless the fingers are resting on the touchpad for at least one scanout, the success rate for middle button emulation is only at 70% or so. This patch adds a 25%-width middle button area between the left and the right software button, everything else stays the same. To avoid immediate breakage, the middle button emulation remains but may be removed in the future. The doc is updated to only refer to the middle button area now. https://bugs.freedesktop.org/show_bug.cgi?id=94755 Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- doc/clickpad-softbuttons.dox| 14 +- doc/svg/software-buttons.svg| 30 +++--- src/evdev-mt-touchpad-buttons.c | 31 ++- src/evdev-mt-touchpad.h | 2 ++ test/touchpad-buttons.c | 30 ++ 5 files changed, 74 insertions(+), 33 deletions(-) diff --git a/doc/clickpad-softbuttons.dox b/doc/clickpad-softbuttons.dox index a4f2e44..cf4c8c9 100644 --- a/doc/clickpad-softbuttons.dox +++ b/doc/clickpad-softbuttons.dox @@ -20,16 +20,20 @@ generated by libinput and passed to the caller in response to a click. @section software_buttons Software button areas On most clickpads, this is the default behavior. The bottom of the touchpad -is split in the middle to generate left or right button events on click. The -height of the button area depends on the hardware but is usually around -10mm. +is split into three distinct areas generate left, middle or right button +events on click. The height of the button area depends on the hardware but +is usually around 10mm. Left, right and middle button events can be triggered as follows: - if a finger is in the main area or the left button area, a click generates left button events. - if a finger is in the right area, a click generates right button events. -- if there is a finger in both the left and right button area, a click - generates middle button events. +- if a finger is in the middle area, a click generates middle button events. + +The middle button is always centered on the touchpad and smaller in size +than the left or right button. The actual size is device-dependent though as +many touchpads do not have visible markings for the middle button the exact +location of the button is not visibly obvious. @image html software-buttons.svg "Left, right and middle-button click with software button areas" diff --git a/doc/svg/software-buttons.svg b/doc/svg/software-buttons.svg index 903535c..c0bc610 100644 --- a/doc/svg/software-buttons.svg +++ b/doc/svg/software-buttons.svg @@ -41,8 +41,8 @@ id="namedview4312" showgrid="false" inkscape:zoom="0.57798581" - inkscape:cx="1134.9723" - inkscape:cy="-71.183873" + inkscape:cx="842.57758" + inkscape:cy="-74.644166" inkscape:window-x="0" inkscape:window-y="27" inkscape:window-maximized="1" @@ -106,7 +106,7 @@ id="rect2858-7" style="color:#00;display:inline;overflow:visible;visibility:visible;fill:#b3b3b3;fill-opacity:1;fill-rule:nonzero;stroke:#00;stroke-width:5.19376326;stroke-miterlimit:4;stroke-dasharray:none;stroke-opacity:1;marker:none;enable-background:accumulate" /> - - - - - - - point.x > tp->buttons.bottom_area.middlebutton_left_edge; +} + +static inline bool is_inside_bottom_left_area(const struct tp_dispatch *tp, const struct tp_touch *t) { return is_inside_bottom_button_area(tp, t) && + !is_inside_bottom_middle_area(tp, t) && !is_inside_bottom_right_area(tp, t); } @@ -192,6 +203,7 @@ tp_button_none_handle_event(struct tp_dispatch *tp, { switch (event) { case BUTTON_EVENT_IN_BOTTOM_R: + case BUTTON_EVENT_IN_BOTTOM_M: case BUTTON_EVENT_IN_BOTTOM_L: tp_button_set_state(tp, t, BUTTON_STATE_BOTTOM, event); break; @@ -220,6 +232,7 @@ tp_button_area_handle_event(struct tp_dispatch *tp, { switch (event) { case BUTTON_EVENT_IN_BOTTOM_R: + case BUTTON_EVENT_IN_BOTTOM_M: case BUTTON_EVENT_IN_BOTTOM_L: case BUTTON_EVENT_IN_TOP_R: case BUTTON_E
Re: [PATCH libinput] tablet: fix the airbrush slider range
Hi, On 11-04-16 01:32, Peter Hutterer wrote: Supposed to be [-1, 1] but we only generated [0, 1] Reported-by: Carlos Garnacho Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev-tablet.c | 2 +- test/tablet.c | 10 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/evdev-tablet.c b/src/evdev-tablet.c index 9a1ac52..84563a8 100644 --- a/src/evdev-tablet.c +++ b/src/evdev-tablet.c @@ -201,7 +201,7 @@ normalize_dist_slider(const struct input_absinfo *absinfo) double range = absinfo->maximum - absinfo->minimum; double value = (absinfo->value - absinfo->minimum) / range; - return value; + return value * 2 - 1; } static inline double diff --git a/test/tablet.c b/test/tablet.c index ad6ac45..3999c3d 100644 --- a/test/tablet.c +++ b/test/tablet.c @@ -2532,7 +2532,7 @@ START_TEST(airbrush_tool) } END_TEST -START_TEST(airbrush_wheel) +START_TEST(airbrush_slider) { struct litest_device *dev = litest_current_device(); struct libinput *li = dev->libinput; @@ -2541,6 +2541,7 @@ START_TEST(airbrush_wheel) const struct input_absinfo *abs; double val; double scale; + double expected; int v; if (!libevdev_has_event_code(dev->evdev, @@ -2574,7 +2575,10 @@ START_TEST(airbrush_wheel) ck_assert(libinput_event_tablet_tool_slider_has_changed(tev)); val = libinput_event_tablet_tool_get_slider_position(tev); - ck_assert_int_eq(val, (v - abs->minimum)/scale); + expected = ((v - abs->minimum)/scale) * 2 - 1; + ck_assert_double_eq(val, expected); + ck_assert_double_ge(val, -1.0); + ck_assert_double_le(val, 1.0); libinput_event_destroy(event); litest_assert_empty_queue(li); } @@ -3655,7 +3659,7 @@ litest_setup_tests(void) litest_add("tablet:mouse", mouse_rotation, LITEST_TABLET, LITEST_ANY); litest_add("tablet:mouse", mouse_wheel, LITEST_TABLET, LITEST_WHEEL); litest_add("tablet:airbrush", airbrush_tool, LITEST_TABLET, LITEST_ANY); - litest_add("tablet:airbrush", airbrush_wheel, LITEST_TABLET, LITEST_ANY); + litest_add("tablet:airbrush", airbrush_slider, LITEST_TABLET, LITEST_ANY); litest_add("tablet:artpen", artpen_tool, LITEST_TABLET, LITEST_ANY); litest_add("tablet:artpen", artpen_rotation, LITEST_TABLET, LITEST_ANY); ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 1/4] evdev: if we have a quick scroll button release, skip middle button emulation
Hi, On 11-04-16 05:54, Peter Hutterer wrote: The only difference between evdev_pointer_notify_physical_button() and evdev_pointer_notify_button() is that the former filters out middle button emulations where applicable. Doing so effectively disables using a button for scrolling that is also used for middle button emulation. This is intentional, it is a niche use-case (and prone to timer races). OTOH some devices exist that only have two buttons on the pointing stick and require button scrolling. This use-case is given preference. https://bugs.freedesktop.org/show_bug.cgi?id=94856 Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/evdev.c b/src/evdev.c index 6bb8986..9be4a96 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -505,11 +505,11 @@ evdev_button_scroll_button(struct evdev_device *device, } else { /* If the button is released quickly enough emit the * button press/release events. */ - evdev_pointer_notify_physical_button(device, + evdev_pointer_notify_button(device, device->scroll.button_down_time, device->scroll.button, LIBINPUT_BUTTON_STATE_PRESSED); - evdev_pointer_notify_physical_button(device, time, + evdev_pointer_notify_button(device, time, device->scroll.button, LIBINPUT_BUTTON_STATE_RELEASED); } ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 2/4] evdev: check the button scrolling state instead of the hw button state
Hi, On 11-04-16 05:54, Peter Hutterer wrote: The state of button scrolling should determine whether we filter the event, not the hardware button state. Signed-off-by: Peter Hutterer --- src/evdev.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/evdev.c b/src/evdev.c index 9be4a96..e272f4b 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -273,13 +273,12 @@ evdev_post_trackpoint_scroll(struct evdev_device *device, uint64_t time) { if (device->scroll.method != LIBINPUT_CONFIG_SCROLL_ON_BUTTON_DOWN || - !hw_is_key_down(device, device->scroll.button)) + !device->scroll.button_scroll_active) return false; - if (device->scroll.button_scroll_active) - evdev_post_scroll(device, time, - LIBINPUT_POINTER_AXIS_SOURCE_CONTINUOUS, - &unaccel); + evdev_post_scroll(device, time, + LIBINPUT_POINTER_AXIS_SOURCE_CONTINUOUS, + &unaccel); return true; } This won't work. I used hw_is_key_down here deliberately. The problem with using button_scroll_active is that it does not become true immediately after the scroll button goes down, it only becomes true after DEFAULT_MIDDLE_BUTTON_SCROLL_TIMEOUT. The purpose of the check this commit changes is to ignore pointer motion events in the time between the button click and the timeout, rather then sending either pointer-motion events (undesirable if the user intends to scroll) or scroll events (undesirable if the user does not intend to scroll). With this patch you end up sending unwanted motion events during the timeout when the user intends to scroll. If you want to make this work with an emulated middle button, you are going to need a scroll.button_pressed, set that when you start the timer, clear it in the appropriate places and use that here instead of the hw_is_key_down (or something similar). Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 3/4] evdev: enable middle-button scrolling on middle-button emulation
Hi, On 11-04-16 05:54, Peter Hutterer wrote: https://bugs.freedesktop.org/show_bug.cgi?id=94856 Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- src/evdev.c| 107 - src/evdev.h| 2 +- test/pointer.c | 53 3 files changed, 114 insertions(+), 48 deletions(-) diff --git a/src/evdev.c b/src/evdev.c index e272f4b..9c73e5e 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -154,14 +154,17 @@ evdev_pointer_notify_physical_button(struct evdev_device *device, state)) return; - evdev_pointer_notify_button(device, time, button, state); + evdev_pointer_notify_button(device, + time, + (unsigned int)button, + state); } -void -evdev_pointer_notify_button(struct evdev_device *device, - uint64_t time, - int button, - enum libinput_button_state state) +static void +evdev_pointer_post_button(struct evdev_device *device, + uint64_t time, + unsigned int button, + enum libinput_button_state state) { int down_count; @@ -182,6 +185,57 @@ evdev_pointer_notify_button(struct evdev_device *device, } +static void +evdev_button_scroll_timeout(uint64_t time, void *data) +{ + struct evdev_device *device = data; + + device->scroll.button_scroll_active = true; +} + +static void +evdev_button_scroll_button(struct evdev_device *device, + uint64_t time, int is_press) +{ + if (is_press) { + libinput_timer_set(&device->scroll.timer, + time + DEFAULT_MIDDLE_BUTTON_SCROLL_TIMEOUT); + device->scroll.button_down_time = time; + } else { + libinput_timer_cancel(&device->scroll.timer); + if (device->scroll.button_scroll_active) { + evdev_stop_scroll(device, time, + LIBINPUT_POINTER_AXIS_SOURCE_CONTINUOUS); + device->scroll.button_scroll_active = false; + } else { + /* If the button is released quickly enough emit the +* button press/release events. */ + evdev_pointer_post_button(device, + device->scroll.button_down_time, + device->scroll.button, + LIBINPUT_BUTTON_STATE_PRESSED); + evdev_pointer_post_button(device, time, + device->scroll.button, + LIBINPUT_BUTTON_STATE_RELEASED); + } + } +} + +void +evdev_pointer_notify_button(struct evdev_device *device, + uint64_t time, + unsigned int button, + enum libinput_button_state state) +{ + if (device->scroll.method == LIBINPUT_CONFIG_SCROLL_ON_BUTTON_DOWN && + button == device->scroll.button) { + evdev_button_scroll_button(device, time, state); + return; + } + + evdev_pointer_post_button(device, time, button, state); +} + void evdev_device_led_update(struct evdev_device *device, enum libinput_led leds) { @@ -480,42 +534,6 @@ get_key_type(uint16_t code) } static void -evdev_button_scroll_timeout(uint64_t time, void *data) -{ - struct evdev_device *device = data; - - device->scroll.button_scroll_active = true; -} - -static void -evdev_button_scroll_button(struct evdev_device *device, - uint64_t time, int is_press) -{ - if (is_press) { - libinput_timer_set(&device->scroll.timer, - time + DEFAULT_MIDDLE_BUTTON_SCROLL_TIMEOUT); - device->scroll.button_down_time = time; - } else { - libinput_timer_cancel(&device->scroll.timer); - if (device->scroll.button_scroll_active) { - evdev_stop_scroll(device, time, - LIBINPUT_POINTER_AXIS_SOURCE_CONTINUOUS); - device->scroll.button_scroll_active = false; - } else { - /* If the button is released quickly enough emit the -* button press/release events. */ - evdev_pointer_notify_button(device, - device->scroll.button_down_time, - device->scr
Re: [PATCH libinput 4/4] test: add test to ensure MB emulation doesn't start while the MB is down
Hi, On 11-04-16 05:54, Peter Hutterer wrote: We already handle the case where we have MB emulation active and a middle button is pressed because we often don't know if we have a middle button on the device. But the other way round makes little sense, when a physical middle button is down emulation should not engage. Test for this. Signed-off-by: Peter Hutterer Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- test/pointer.c | 57 + 1 file changed, 57 insertions(+) diff --git a/test/pointer.c b/test/pointer.c index d1d2fda..d15fd29 100644 --- a/test/pointer.c +++ b/test/pointer.c @@ -1260,6 +1260,62 @@ START_TEST(middlebutton) } END_TEST +START_TEST(middlebutton_nostart_while_down) +{ + struct litest_device *device = litest_current_device(); + struct libinput *li = device->libinput; + enum libinput_config_status status; + unsigned int i; + const int btn[][4] = { + { BTN_LEFT, BTN_RIGHT, BTN_LEFT, BTN_RIGHT }, + { BTN_LEFT, BTN_RIGHT, BTN_RIGHT, BTN_LEFT }, + { BTN_RIGHT, BTN_LEFT, BTN_LEFT, BTN_RIGHT }, + { BTN_RIGHT, BTN_LEFT, BTN_RIGHT, BTN_LEFT }, + }; + + if (!libinput_device_pointer_has_button(device->libinput_device, + BTN_MIDDLE)) + return; + + disable_button_scrolling(device); + + status = libinput_device_config_middle_emulation_set_enabled( + device->libinput_device, + LIBINPUT_CONFIG_MIDDLE_EMULATION_ENABLED); + if (status == LIBINPUT_CONFIG_STATUS_UNSUPPORTED) + return; + + litest_button_click(device, BTN_MIDDLE, true); + litest_drain_events(li); + + for (i = 0; i < ARRAY_LENGTH(btn); i++) { + litest_button_click(device, btn[i][0], true); + litest_assert_button_event(li, + btn[i][0], + LIBINPUT_BUTTON_STATE_PRESSED); + litest_button_click(device, btn[i][1], true); + litest_assert_button_event(li, + btn[i][1], + LIBINPUT_BUTTON_STATE_PRESSED); + + litest_assert_empty_queue(li); + + litest_button_click(device, btn[i][2], false); + litest_assert_button_event(li, + btn[i][2], + LIBINPUT_BUTTON_STATE_RELEASED); + litest_button_click(device, btn[i][3], false); + litest_assert_button_event(li, + btn[i][3], + LIBINPUT_BUTTON_STATE_RELEASED); + litest_assert_empty_queue(li); + } + + litest_button_click(device, BTN_MIDDLE, false); + litest_drain_events(li); +} +END_TEST + START_TEST(middlebutton_timeout) { struct litest_device *device = litest_current_device(); @@ -1677,6 +1733,7 @@ litest_setup_tests(void) litest_add("pointer:accel", pointer_accel_profile_flat_motion_relative, LITEST_RELATIVE, LITEST_TOUCHPAD); litest_add("pointer:middlebutton", middlebutton, LITEST_BUTTON, LITEST_ANY); + litest_add("pointer:middlebutton", middlebutton_nostart_while_down, LITEST_BUTTON, LITEST_ANY); litest_add("pointer:middlebutton", middlebutton_timeout, LITEST_BUTTON, LITEST_ANY); litest_add("pointer:middlebutton", middlebutton_doubleclick, LITEST_BUTTON, LITEST_ANY); litest_add("pointer:middlebutton", middlebutton_middleclick, LITEST_BUTTON, LITEST_ANY); ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel