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: [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: [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
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/backlight. The plan is to >
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 custom >> scripts binded to
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: 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
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: 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
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 >
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
[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
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.intel.com/ > >
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://bbs.archlinux.org/v
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 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: [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: [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 scaling where the a
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 the props alread
[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
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
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 slider? E.g. something l
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, 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 acpi_video0 after a bri
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 mess to the "right" ba
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 way to undo the brightness change and now >> the user can no
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 this is set to true then it is safe
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 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 people. > > Basically a
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 people. > > Basically a
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 Windows8 or later and the G
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 later as a follow up change. At a minimum when I code
[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: 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
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: 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, 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: 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: [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 <mpor...@konsulko.com> 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: [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 PorterHave 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] 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 <peter.hutte...@who-t.net> I agree this is the best way to handle this: Acked-by: Hans de Goede <hdego...@redhat.com> 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))
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 <hdego...@redhat.com> 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 <hdego...@redhat.com> Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> --- 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; /*
[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 <hdego...@redhat.com> --- 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 = _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 >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
[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 <hdego...@redhat.com> --- 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
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 <peter.hutte...@who-t.net> Sounds reasonable to me and the code changes look good: Reviewed-by: Hans de Goede <hdego...@redhat.com> 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
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 <peter.hutte...@who-t.net> --- 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 <hdego...@redhat.com> Regards, Hans ___ 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 <peter.hutte...@who-t.net> Patch looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> 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: 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 <peter.hutte...@who-t.net> Patch looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> 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: 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 <peter.hutte...@who-t.net> Patch looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> 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, , ); - /* 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, , ); - 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 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 <hdego...@redhat.com> Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
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 <peter.hutte...@who-t.net> Patch series looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> 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] 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 <peter.hutte...@who-t.net> 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 <hdego...@redhat.com> 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, 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. - */ + +
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 <hdgo...@redhat.com> Regards, Hans ___ 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 <peter.hutte...@who-t.net> --- Sorry, used the wrong patch, v1 was an earlier version. v2 looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> 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] Added missing button range for pad on CTH-680
Hi, On 13-02-17 07:57, Peter Hutterer wrote: From: Sakse Dalum <s...@srq.re> 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 <peter.hutte...@who-t.net> Patch looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> 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 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 <hdego...@redhat.com> 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 <peter.hutte...@who-t.net> --- 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 == 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, , ); 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.:pnX555LAB:* LIBINPUT_
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 <hdego...@redhat.com> 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 <peter.hutte...@who-t.net> --- 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 avoid mixing up the two
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 <peter.hutte...@who-t.net> Series looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> 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 = _switch_interface; dispatch->device = lid_device; libinput_device_init_event_listener(>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_devic
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 <hdego...@redhat.com> 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 <peter.hutte...@who-t.net> --- 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->nfi
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 <peter.hutte...@who-t.net> Looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> 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, , ); - mm.x = width * 0.58; + mm.x = width * 0.60; mm.y = topsize_mm; edges = evdev_device_mm_to_units(device, ); 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, ); 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 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 <peter.hutte...@who-t.net> Patch-set looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> 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, >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 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 <peter.hutte...@who-t.net> Series looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> 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] udev: mark Asus X555LAB as touchpad with visible marker
Hi, On 09-01-17 23:39, Peter Hutterer wrote: https://bugs.freedesktop.org/show_bug.cgi?id=99200 Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Patch looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- udev/90-libinput-model-quirks.hwdb | 6 ++ 1 file changed, 6 insertions(+) diff --git a/udev/90-libinput-model-quirks.hwdb b/udev/90-libinput-model-quirks.hwdb index e467e59..04bdf9a 100644 --- a/udev/90-libinput-model-quirks.hwdb +++ b/udev/90-libinput-model-quirks.hwdb @@ -42,6 +42,12 @@ libinput:mouse:input:b0005v05ACp030D* LIBINPUT_MODEL_APPLE_MAGICMOUSE=1 ## +# Asus +## +libinput:name:*ETPS/2 Elantech Touchpad*:dmi:*svnASUSTeKCOMPUTERINC.:pnX555LAB:* + LIBINPUT_MODEL_TOUCHPAD_VISIBLE_MARKER=1 + +## # Cyborg ## # Saitek Cyborg R.A.T.5 Mouse ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v2 libinput] touchpad: add a model tag to mark touchpads with visible marker
Hi, On 09-01-17 23:38, Peter Hutterer wrote: We used to mark dell touchpads this way but let's make this more generic. Nothing else used the dell touchpad model flag, so we can simply replace it. Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> --- sorry, rebase gone wrong and it skipped the test/udev directories in v1. Ah, that explains things... New version looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans src/evdev-mt-touchpad-buttons.c | 4 +--- src/evdev.c | 2 +- src/evdev.h | 2 +- test/litest-device-alps-dualpoint.c | 2 +- test/litest-device-synaptics-i2c.c | 2 +- udev/90-libinput-model-quirks.hwdb | 2 +- 6 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/evdev-mt-touchpad-buttons.c b/src/evdev-mt-touchpad-buttons.c index b59cf13..f4fe6b7 100644 --- a/src/evdev-mt-touchpad-buttons.c +++ b/src/evdev-mt-touchpad-buttons.c @@ -569,10 +569,8 @@ tp_init_softbuttons(struct tp_dispatch *tp, * * On touchpads with visible markings we reduce the size of the * middle button since users have a visual guide. -* -* All Dell touchpads appear to have a middle marker. */ - if (tp->device->model_flags & EVDEV_MODEL_DELL_TOUCHPAD) { + if (tp->device->model_flags & EVDEV_MODEL_TOUCHPAD_VISIBLE_MARKER) { mm.x = width/2 - 5; /* 10mm wide */ edges = evdev_device_mm_to_units(device, ); mb_le = edges.x; diff --git a/src/evdev.c b/src/evdev.c index c06daa6..6ab68ae 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -2201,7 +2201,7 @@ evdev_read_model_flags(struct evdev_device *device) MODEL(CYAPA), MODEL(HP_STREAM11_TOUCHPAD), MODEL(LENOVO_T450_TOUCHPAD), - MODEL(DELL_TOUCHPAD), + MODEL(TOUCHPAD_VISIBLE_MARKER), MODEL(TRACKBALL), MODEL(APPLE_MAGICMOUSE), MODEL(HP8510_TOUCHPAD), diff --git a/src/evdev.h b/src/evdev.h index c07b09f..7ad3dfd 100644 --- a/src/evdev.h +++ b/src/evdev.h @@ -116,7 +116,7 @@ enum evdev_device_model { EVDEV_MODEL_CYAPA = (1 << 15), EVDEV_MODEL_HP_STREAM11_TOUCHPAD = (1 << 16), EVDEV_MODEL_LENOVO_T450_TOUCHPAD= (1 << 17), - EVDEV_MODEL_DELL_TOUCHPAD = (1 << 18), + EVDEV_MODEL_TOUCHPAD_VISIBLE_MARKER = (1 << 18), EVDEV_MODEL_TRACKBALL = (1 << 19), EVDEV_MODEL_APPLE_MAGICMOUSE = (1 << 20), EVDEV_MODEL_HP8510_TOUCHPAD = (1 << 21), diff --git a/test/litest-device-alps-dualpoint.c b/test/litest-device-alps-dualpoint.c index 08ba006..fe8cf96 100644 --- a/test/litest-device-alps-dualpoint.c +++ b/test/litest-device-alps-dualpoint.c @@ -106,7 +106,7 @@ static const char udev_rule[] = "ENV{ID_INPUT_TOUCHPAD}==\"\", GOTO=\"touchpad_end\"\n" "\n" "ATTRS{name}==\"litest AlpsPS/2 ALPS DualPoint TouchPad\"," -"ENV{LIBINPUT_MODEL_DELL_TOUCHPAD}=\"1\"\n" +"ENV{LIBINPUT_MODEL_TOUCHPAD_VISIBLE_MARKER}=\"1\"\n" "\n" "LABEL=\"touchpad_end\""; diff --git a/test/litest-device-synaptics-i2c.c b/test/litest-device-synaptics-i2c.c index 0d83caa..3e1d5e4 100644 --- a/test/litest-device-synaptics-i2c.c +++ b/test/litest-device-synaptics-i2c.c @@ -92,7 +92,7 @@ static const char udev_rule[] = "ENV{ID_INPUT_TOUCHPAD}==\"\", GOTO=\"touchpad_end\"\n" "\n" "ATTRS{name}==\"litest DLL0704:01 06CB:76AD Touchpad\"," -"ENV{LIBINPUT_MODEL_DELL_TOUCHPAD}=\"1\"\n" +"ENV{LIBINPUT_MODEL_TOUCHPAD_VISIBLE_MARKER}=\"1\"\n" "\n" "LABEL=\"touchpad_end\""; diff --git a/udev/90-libinput-model-quirks.hwdb b/udev/90-libinput-model-quirks.hwdb index eb74f61..e467e59 100644 --- a/udev/90-libinput-model-quirks.hwdb +++ b/udev/90-libinput-model-quirks.hwdb @@ -53,7 +53,7 @@ libinput:mouse:input:b0003v06A3p0CD5* ## libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnDellInc.:* libinput:name:* Touchpad:dmi:*svnDellInc.:* - LIBINPUT_MODEL_DELL_TOUCHPAD=1 + LIBINPUT_MODEL_TOUCHPAD_VISIBLE_MARKER=1 ## # Elantech ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] test: add test for the vertical position-dependent pinch
Hi, On 21-12-16 01:13, Peter Hutterer wrote: Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> --- test/gestures.c | 55 +++ test/litest.c | 6 ++ test/litest.h | 3 +++ 3 files changed, 64 insertions(+) diff --git a/test/gestures.c b/test/gestures.c index 0b132c3..3a66afb 100644 --- a/test/gestures.c +++ b/test/gestures.c @@ -496,6 +496,58 @@ START_TEST(gestures_swipe_4fg_btntool) } END_TEST +START_TEST(gestures_pinch_vertical_positon) +{ + struct litest_device *dev = litest_current_device(); + struct libinput *li = dev->libinput; + struct libinput_event *event; + int nfingers = _i; /* ranged test */ + + if (libevdev_get_num_slots(dev->evdev) < nfingers || + !libinput_device_has_capability(dev->libinput_device, + LIBINPUT_DEVICE_CAP_GESTURE)) + return; + + litest_disable_tap(dev->libinput_device); + litest_drain_events(li); + + litest_touch_down(dev, 0, 40, 30); + litest_touch_down(dev, 1, 50, 70); + litest_touch_down(dev, 2, 60, 70); + if (nfingers > 3) + litest_touch_down(dev, 3, 70, 70); + libinput_dispatch(li); + litest_timeout_gesture_scroll(); + libinput_dispatch(li); + + /* This is actually a small swipe gesture, all three fingers moving +* down but we're checking for the code that triggers based on +* finger position. */ + litest_touch_move_to(dev, 0, 40, 30, 40, 30.5, 1, 0); + litest_touch_move_to(dev, 1, 50, 70, 50, 70.5, 1, 0); + litest_touch_move_to(dev, 2, 60, 70, 60, 70.5, 1, 0); + if (nfingers > 3) + litest_touch_move_to(dev, 3, 70, 70, 70, 70.5, 1, 0); + libinput_dispatch(li); + + event = libinput_get_event(li); + litest_is_gesture_event(event, + LIBINPUT_EVENT_GESTURE_PINCH_BEGIN, + nfingers); + libinput_event_destroy(event); + + litest_touch_move_to(dev, 0, 40, 30.5, 40, 36, 5, 0); + litest_touch_move_to(dev, 1, 50, 70.5, 50, 76, 5, 0); + litest_touch_move_to(dev, 2, 60, 70.5, 60, 76, 5, 0); + if (nfingers > 3) + litest_touch_move_to(dev, 3, 70, 70.5, 60, 76, 5, 0); + libinput_dispatch(li); + + litest_assert_only_typed_events(li, + LIBINPUT_EVENT_GESTURE_PINCH_UPDATE); +} +END_TEST + START_TEST(gestures_pinch) { struct litest_device *dev = litest_current_device(); @@ -1219,6 +1271,7 @@ litest_setup_tests_gestures(void) { /* N, NE, ... */ struct range cardinals = { 0, 8 }; + struct range fingers = { 3, 5 }; litest_add("gestures:cap", gestures_cap, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH); litest_add("gestures:cap", gestures_nocap, LITEST_ANY, LITEST_TOUCHPAD); @@ -1230,9 +1283,11 @@ litest_setup_tests_gestures(void) litest_add_ranged("gestures:pinch", gestures_pinch, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH, ); litest_add_ranged("gestures:pinch", gestures_pinch_3fg, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH, ); litest_add_ranged("gestures:pinch", gestures_pinch_3fg_btntool, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH, ); + litest_add_ranged("gestures:pinch", gestures_pinch_3fg, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH, ); Your doing the: litest_add_ranged("gestures:pinch", gestures_pinch_3fg, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH, ); Test twice now, otherwise LGTM, so with this fixed: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans litest_add_ranged("gestures:pinch", gestures_pinch_4fg, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH, ); litest_add_ranged("gestures:pinch", gestures_pinch_4fg_btntool, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH, ); litest_add_ranged("gestures:pinch", gestures_spread, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH, ); + litest_add_ranged("gestures:pinch", gestures_pinch_vertical_positon, LITEST_TOUCHPAD, LITEST_SINGLE_TOUCH, ); 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); diff --git a/test/litest.c b/test/litest.c index 40029d6..0574c0a 100644 --- a/test/litest.c +++ b/test/litest.c @@ -2964,6 +2964,12 @@ litest_timeout_gesture(void) } void +litest_timeout_gesture_scroll(void) +{ + msleep(180); +} + +void litest_timeout_trackpoint(void) { msleep(320); diff --git a/test/litest.h b/test/litest.h index 017a3d2..a707509 100644 --- a/test/litest.h +++ b/test/litest.h @@ -654,6 +654,9 @@ void litest_timeout_gesture(void); void +lite
Re: [PATCH v2 libinput] gestures: if fingers don't move, force a gesture by finger position
Hi, On 21-12-16 01:12, Peter Hutterer wrote: If the fingers rest on the touchpad without moving for a timeout, switch to pinch or swipe based on the finger position. We already switched to two-finger scrolling based on the timeout, now we also do so for 3 and 4 finger gestures. This gives us better reaction to small movements. This also fixes previously unreachable code: the test for the finger position required at least 3 fingers down but was within a condition that ensured only 2 fingers were down. This was introduced in 11917061fe320c. Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> --- Changes to v1: - better commit message/comments - dropped the finger_count > 2 check that was a dead condition before anyway LGTM now: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans src/evdev-mt-touchpad-gestures.c | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/evdev-mt-touchpad-gestures.c b/src/evdev-mt-touchpad-gestures.c index 08c179d..de3ec4f 100644 --- a/src/evdev-mt-touchpad-gestures.c +++ b/src/evdev-mt-touchpad-gestures.c @@ -331,21 +331,23 @@ tp_gesture_handle_state_unknown(struct tp_dispatch *tp, uint64_t time) int yres = tp->device->abs.absinfo_y->resolution; int vert_distance; - /* for two-finger gestures, if the fingers stay unmoving for a -* while, assume (slow) scroll */ - if (tp->gesture.finger_count == 2) { - if (time > (tp->gesture.initial_time + DEFAULT_GESTURE_2FG_SCROLL_TIMEOUT)) { + if (time > (tp->gesture.initial_time + DEFAULT_GESTURE_2FG_SCROLL_TIMEOUT)) { + /* for two-finger gestures, if the fingers stay unmoving for a +* while, assume (slow) scroll */ + if (tp->gesture.finger_count == 2) { tp_gesture_set_scroll_buildup(tp); return GESTURE_STATE_SCROLL; } - /* Else check if one finger is > 20mm below the others */ + /* for 3+ finger gestures, 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; + } else { + return GESTURE_STATE_SWIPE; } } ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] gestures: if fingers don't move, force a gesture by finger position
Hi, On 20-12-16 04:57, Peter Hutterer wrote: If the fingers rest on the touchpad without moving for a timeout, switch to pinch or swipe based on the finger position. We already did so for two-fingers switching to scrolling, now we also do so for 3 and 4 finger gestures. This gives us better reaction to small movements. Signed-off-by: Peter Hutterer--- src/evdev-mt-touchpad-gestures.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/evdev-mt-touchpad-gestures.c b/src/evdev-mt-touchpad-gestures.c index 6c568a3..e54db2a 100644 --- a/src/evdev-mt-touchpad-gestures.c +++ b/src/evdev-mt-touchpad-gestures.c @@ -331,10 +331,10 @@ tp_gesture_handle_state_unknown(struct tp_dispatch *tp, uint64_t time) int yres = tp->device->abs.absinfo_y->resolution; int vert_distance; - /* for two-finger gestures, if the fingers stay unmoving for a -* while, assume (slow) scroll */ - if (tp->gesture.finger_count == 2) { - if (time > (tp->gesture.initial_time + DEFAULT_GESTURE_2FG_SCROLL_TIMEOUT)) { + if (time > (tp->gesture.initial_time + DEFAULT_GESTURE_2FG_SCROLL_TIMEOUT)) { + /* for two-finger gestures, if the fingers stay unmoving for a +* while, assume (slow) scroll */ + if (tp->gesture.finger_count == 2) { tp_gesture_set_scroll_buildup(tp); return GESTURE_STATE_SCROLL; } @@ -346,6 +346,8 @@ tp_gesture_handle_state_unknown(struct tp_dispatch *tp, uint64_t time) tp->gesture.enabled) { tp_gesture_init_pinch(tp); return GESTURE_STATE_PINCH; + } else { + return GESTURE_STATE_SWIPE; } } This change means that if 2 fingers are down and one finger is > 20mm below the others, we will no longer immediately switch to pinch mode, as that test is now only done after the timeout. I think that that is an unintended side-effect of this patch ? Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 1/4] test: don't set LITEST_VERBOSE during make check
Hi, On 20-12-16 03:51, Peter Hutterer wrote: I've never had the log output help me identify a bug during a test run. Now that we run all tests in the same binary the verbosity just leads to a massive file that makes it hard to find the actual failure. Turn off LITEST_VERBOSE by default but leave the parsing in for cases where it may come in handy. Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Series looks good to me and is: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- test/Makefile.am | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/Makefile.am b/test/Makefile.am index 6dfc138..f43cf52 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -143,8 +143,6 @@ test_build_linker_LDADD = $(top_builddir)/src/libinput.la $(top_builddir)/src/li test_build_cxx_SOURCES = build-cxx.cc test_build_cxx_CXXFLAGS = -Wall -Wextra -Wno-unused-parameter $(AM_CXXFLAGS) -AM_TESTS_ENVIRONMENT= LITEST_VERBOSE=1; export LITEST_VERBOSE; - if HAVE_VALGRIND VALGRIND_FLAGS=--leak-check=full \ --quiet \ ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 00/13] Revamp touchpad acceleration code
Hi, On 19-12-16 06:20, Peter Hutterer wrote: This patchset is a cleanup and revamp of the touchpad acceleration code. It doesn't give us perfect acceleration, but it goes from the current rather abysimal state to one that should at least be good enough most of the time. More tweaking will come, but meanwhile santa and whatnot. Right now, we use the mouse acceleration for touchpads with a magic slowdown applied. The first 10 patches separate the touchpad code from the mouse acceleration code and switch it to use mm/s as base velocity unit (rather than the current 1000dpi-mouse-equivalency units). 11 is the main change that changes the acceleration pattern, mostly to start accelerating at a lot higher finger speed (found mostly by trial and error). See that patch for details. 12 is the doc update, 13 is a gratitous change that just seems to make it feel better, but who knows. Some pretty graphics and writeup http://who-t.blogspot.com/2016/12/libinput-touchpad-pointer-acceleration.html Branch is available here: https://github.com/whot/libinput/tree/wip/touchpad-pointer-accel-v3 but beware, it spews debug printfs like crazy. Just reset back a few commits if you're trying this branch. Series looks good to me and is: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 1/3] touchpad: convert two functions to use the device->phys helpers
Hi, On 14-12-16 08:36, Peter Hutterer wrote: Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Series looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- src/evdev-mt-touchpad.c | 24 +--- src/evdev.h | 27 +++ 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index 7bac8ec..26b65de 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -478,18 +478,19 @@ tp_process_key(struct tp_dispatch *tp, static void tp_unpin_finger(const struct tp_dispatch *tp, struct tp_touch *t) { - double xdist, ydist; + struct phys_coords mm; + struct device_coords delta; if (!t->pinned.is_pinned) return; - xdist = abs(t->point.x - t->pinned.center.x); - xdist *= tp->buttons.motion_dist.x_scale_coeff; - ydist = abs(t->point.y - t->pinned.center.y); - ydist *= tp->buttons.motion_dist.y_scale_coeff; + delta.x = abs(t->point.x - t->pinned.center.x); + delta.y = abs(t->point.y - t->pinned.center.y); + + mm = evdev_device_unit_delta_to_mm(tp->device, ); /* 1.5mm movement -> unpin */ - if (hypot(xdist, ydist) >= 1.5) { + if (hypot(mm.x, mm.y) >= 1.5) { t->pinned.is_pinned = false; return; } @@ -962,8 +963,8 @@ tp_need_motion_history_reset(struct tp_dispatch *tp) static bool tp_detect_jumps(const struct tp_dispatch *tp, struct tp_touch *t) { - struct device_coords *last; - double dx, dy; + struct device_coords *last, delta; + struct phys_coords mm; const int JUMP_THRESHOLD_MM = 20; /* We haven't seen pointer jumps on Wacom tablets yet, so exclude @@ -978,10 +979,11 @@ tp_detect_jumps(const struct tp_dispatch *tp, struct tp_touch *t) /* called before tp_motion_history_push, so offset 0 is the most * recent coordinate */ last = tp_motion_history_offset(t, 0); - dx = 1.0 * abs(t->point.x - last->x) / tp->device->abs.absinfo_x->resolution; - dy = 1.0 * abs(t->point.y - last->y) / tp->device->abs.absinfo_y->resolution; + delta.x = abs(t->point.x - last->x); + delta.y = abs(t->point.y - last->y); + mm = evdev_device_unit_delta_to_mm(tp->device, ); - return hypot(dx, dy) > JUMP_THRESHOLD_MM; + return hypot(mm.x, mm.y) > JUMP_THRESHOLD_MM; } static void diff --git a/src/evdev.h b/src/evdev.h index 071b9ec..c07b09f 100644 --- a/src/evdev.h +++ b/src/evdev.h @@ -596,6 +596,33 @@ evdev_libinput_context(const struct evdev_device *device) } /** + * Convert the pair of delta coordinates in device space to mm. + */ +static inline struct phys_coords +evdev_device_unit_delta_to_mm(const struct evdev_device* device, + const struct device_coords *units) +{ + struct phys_coords mm = { 0, 0 }; + const struct input_absinfo *absx, *absy; + + if (device->abs.absinfo_x == NULL || + device->abs.absinfo_y == NULL) { + log_bug_libinput(evdev_libinput_context(device), +"%s: is not an abs device\n", +device->devname); + return mm; + } + + absx = device->abs.absinfo_x; + absy = device->abs.absinfo_y; + + mm.x = 1.0 * units->x/absx->resolution; + mm.y = 1.0 * units->y/absy->resolution; + + return mm; +} + +/** * Convert the pair of coordinates in device space to mm. This takes the * axis min into account, i.e. a unit of min is equivalent to 0 mm. */ ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 1/2] touchpad: reduce the initial timeout for tapping after touch
Hi, On 12-12-16 06:53, Peter Hutterer wrote: This is the timeout before we decide "this is just a finger down, not a tap". Until this timeout is hit a finger's movement is filtered. To allow for a more responsive touchpad, we want that timeout as short as possible. Event analysis from several users showed that 95% of the touches are less than 100ms long. Reducing the threshold should have almost no impact on most tapping users but improves the reaction time of the pointer for normal movements. For a more details see: http://who-t.blogspot.com/2016/12/libinput-touchpad-tap-analysis.html Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Series looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- src/evdev-mt-touchpad-tap.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/evdev-mt-touchpad-tap.c b/src/evdev-mt-touchpad-tap.c index cb4abf2..b65bac4 100644 --- a/src/evdev-mt-touchpad-tap.c +++ b/src/evdev-mt-touchpad-tap.c @@ -33,6 +33,7 @@ #include "evdev-mt-touchpad.h" +#define DEFAULT_TAP_INITIAL_TIMEOUT_PERIOD ms2us(100) #define DEFAULT_TAP_TIMEOUT_PERIOD ms2us(180) #define DEFAULT_DRAG_TIMEOUT_PERIOD ms2us(300) #define DEFAULT_TAP_MOVE_THRESHOLD TP_MM_TO_DPI_NORMALIZED(3) @@ -127,6 +128,13 @@ tp_tap_notify(struct tp_dispatch *tp, } static void +tp_tap_set_initial_timer(struct tp_dispatch *tp, uint64_t time) +{ + libinput_timer_set(>tap.timer, + time + DEFAULT_TAP_INITIAL_TIMEOUT_PERIOD); +} + +static void tp_tap_set_timer(struct tp_dispatch *tp, uint64_t time) { libinput_timer_set(>tap.timer, time + DEFAULT_TAP_TIMEOUT_PERIOD); @@ -155,7 +163,7 @@ tp_tap_idle_handle_event(struct tp_dispatch *tp, case TAP_EVENT_TOUCH: tp->tap.state = TAP_STATE_TOUCH; tp->tap.first_press_time = time; - tp_tap_set_timer(tp, time); + tp_tap_set_initial_timer(tp, time); break; case TAP_EVENT_RELEASE: break; ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: sync BTN_TOOL_FINGER state on init
Hi, On 05-12-16 05:00, Peter Hutterer wrote: The Elantech touchpad on my Asus Vivobook doesn't release BTN_TOOL_FINGER on up. If the touchpad was used before libinput initializes, the kernel filters the event because its state is already set. We never receive it and keep ignoring all events until the first switch to BTN_TOOL_DOUBLETAP and back. On touchpad init sync the BTN_TOOL_FINGER state and set it accordingly. This is the only event that can be legitimately down on init. We don't care about BTN_TOUCH because ignoring an ongoing touch on init is generally a good idea and we can ignore any multifinger gesture as well. Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Patch LGTM: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- src/evdev-mt-touchpad.c | 8 test/touchpad.c | 24 2 files changed, 32 insertions(+) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index 0a261a3..762f539 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -1804,6 +1804,14 @@ tp_init_slots(struct tp_dispatch *tp, for (i = 1; i < tp->num_slots; i++) tp_sync_touch(tp, device, >touches[i], i); + /* Some touchpads don't reset BTN_TOOL_FINGER on touch up and only +* change to/from it when BTN_TOOL_DOUBLETAP is set. This causes us +* to ignore the first touches events until a two-finger gesture is +* performed. +*/ + if (libevdev_get_event_value(device->evdev, EV_KEY, BTN_TOOL_FINGER)) + tp_fake_finger_set(tp, BTN_TOOL_FINGER, 1); + return true; } diff --git a/test/touchpad.c b/test/touchpad.c index cdc261b..5b6f0a4 100644 --- a/test/touchpad.c +++ b/test/touchpad.c @@ -4365,6 +4365,29 @@ START_TEST(touchpad_slot_swap) } END_TEST +START_TEST(touchpad_finger_always_down) +{ + struct litest_device *dev = litest_current_device(); + struct libinput *li; + + /* Set BTN_TOOL_FINGER before a new context is initialized */ + litest_event(dev, EV_KEY, BTN_TOOL_FINGER, 1); + litest_event(dev, EV_SYN, SYN_REPORT, 0); + + li = litest_create_context(); + libinput_path_add_device(li, +libevdev_uinput_get_devnode(dev->uinput)); + litest_drain_events(li); + + litest_touch_down(dev, 0, 50, 50); + litest_touch_move_to(dev, 0, 50, 50, 70, 50, 10, 0); + + litest_assert_only_typed_events(li, LIBINPUT_EVENT_POINTER_MOTION); + + libinput_unref(li); +} +END_TEST + START_TEST(touchpad_time_usec) { struct litest_device *dev = litest_current_device(); @@ -4742,6 +4765,7 @@ litest_setup_tests_touchpad(void) litest_add_for_device("touchpad:bugs", touchpad_tool_tripletap_touch_count, LITEST_SYNAPTICS_TOPBUTTONPAD); litest_add_for_device("touchpad:bugs", touchpad_slot_swap, LITEST_SYNAPTICS_TOPBUTTONPAD); + litest_add_for_device("touchpad:bugs", touchpad_finger_always_down, LITEST_SYNAPTICS_TOPBUTTONPAD); litest_add("touchpad:time", touchpad_time_usec, LITEST_TOUCHPAD, LITEST_ANY); ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 1/4] Use the LIBINPUT_VERSION define, not the normal VERSION
Hi, On 01-12-16 02:37, Peter Hutterer wrote: Not that it really matters, but given we're already setting it anyway... Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Series looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- src/libinput-private.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libinput-private.h b/src/libinput-private.h index 52f129a..60685b5 100644 --- a/src/libinput-private.h +++ b/src/libinput-private.h @@ -34,11 +34,12 @@ #include "libinput.h" #include "libinput-util.h" +#include "libinput-version.h" #if LIBINPUT_VERSION_MICRO >= 90 #define HTTP_DOC_LINK "https://wayland.freedesktop.org/libinput/doc/latest/; #else -#define HTTP_DOC_LINK "https://wayland.freedesktop.org/libinput/doc/; VERSION "/" +#define HTTP_DOC_LINK "https://wayland.freedesktop.org/libinput/doc/; LIBINPUT_VERSION "/" #endif struct libinput_source; ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v2 libinput] touchpad: add a quirk for the HP Pavilion dm4
Hi, On 30-11-16 03:24, Peter Hutterer wrote: This touchpad has cursor jumps for 2-finger scrolling that also affects the single-finger emulation. So disable any multitouch bits on this device and disallow the 2-finger scroll method. This still allows for 2-finger tapping/clicking. https://bugs.freedesktop.org/show_bug.cgi?id=91135 Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> v2 LGTM: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- Changes to v1: - move the DM4 magic to tp_init_slots and pretend it's a single-touch touchpad. src/evdev-mt-touchpad.c| 12 +++- src/evdev.c| 1 + src/evdev.h| 1 + udev/90-libinput-model-quirks.hwdb | 4 4 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index 0492851..0a261a3 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -1770,8 +1770,12 @@ tp_init_slots(struct tp_dispatch *tp, * If three fingers are set down in the same frame, one slot has the * coordinates 0/0 and may not get updated for several frames. * See https://bugzilla.redhat.com/show_bug.cgi?id=1295073 +* +* The HP Pavilion DM4 touchpad has random jumps in slots, including +* for single-finger movement. See fdo bug 91135 */ - if (tp->semi_mt) { + if (tp->semi_mt || + device->model_flags & EVDEV_MODEL_HP_PAVILION_DM4_TOUCHPAD) { tp->num_slots = 1; tp->slot = 0; tp->has_mt = false; @@ -1874,6 +1878,12 @@ tp_scroll_get_methods(struct tp_dispatch *tp) { uint32_t methods = LIBINPUT_CONFIG_SCROLL_EDGE; + /* Any movement with more than one finger has random cursor +* jumps. Don't allow for 2fg scrolling on this device, see +* fdo bug 91135 */ + if (tp->device->model_flags & EVDEV_MODEL_HP_PAVILION_DM4_TOUCHPAD) + return LIBINPUT_CONFIG_SCROLL_EDGE; + if (tp->ntouches >= 2) methods |= LIBINPUT_CONFIG_SCROLL_2FG; diff --git a/src/evdev.c b/src/evdev.c index afb5e34..57670e2 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -2186,6 +2186,7 @@ evdev_read_model_flags(struct evdev_device *device) MODEL(HP8510_TOUCHPAD), MODEL(HP6910_TOUCHPAD), MODEL(HP_ZBOOK_STUDIO_G3), + MODEL(HP_PAVILION_DM4_TOUCHPAD), #undef MODEL { "ID_INPUT_TRACKBALL", EVDEV_MODEL_TRACKBALL }, { NULL, EVDEV_MODEL_DEFAULT }, diff --git a/src/evdev.h b/src/evdev.h index b5a54a2..071b9ec 100644 --- a/src/evdev.h +++ b/src/evdev.h @@ -122,6 +122,7 @@ enum evdev_device_model { EVDEV_MODEL_HP8510_TOUCHPAD = (1 << 21), EVDEV_MODEL_HP6910_TOUCHPAD = (1 << 22), EVDEV_MODEL_HP_ZBOOK_STUDIO_G3 = (1 << 23), + EVDEV_MODEL_HP_PAVILION_DM4_TOUCHPAD = (1 << 24), }; struct mt_slot { diff --git a/udev/90-libinput-model-quirks.hwdb b/udev/90-libinput-model-quirks.hwdb index 347a229..eb74f61 100644 --- a/udev/90-libinput-model-quirks.hwdb +++ b/udev/90-libinput-model-quirks.hwdb @@ -99,6 +99,10 @@ libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnHewlett-Packard:*pnHPCompaq6910 libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnHewlett-Packard:*pnHPCompaq8510w* LIBINPUT_MODEL_HP8510_TOUCHPAD=1 +# HP Pavillion dm4 +libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnHewlett-Packard:*pnHPPaviliondm4NotebookPC* + LIBINPUT_MODEL_HP_PAVILION_DM4_TOUCHPAD=1 + # HP Stream 11 libinput:name:SYN1EDE:00 06CB:7442:dmi:*svnHewlett-Packard:pnHPStreamNotebookPC11* LIBINPUT_MODEL_HP_STREAM11_TOUCHPAD=1 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 0/4] Wheel tilt scroll source
Hi, On 29-11-16 03:28, Peter Hutterer wrote: Quite a few mice have tilt-capable wheels for horizontal scrolling. We can't detect those automatically so we'll have to rely on systemd/udev for the tagging. But despite that, we should be honest about the scroll source and mark them as tilt where possible rather than pretending that they're degrees like the wheel promises to be. For backwards compat, I'm leaving the actually returned value as-is, so most callers that don't support the new scroll source should just work fine. There's no real value to return anyway despite "tilted" and returning just -1 or 1 here would likely break the world. And I like this world, I don't have another one available. Series looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] evdev: init axis range warnings for touch devices too
Hi, On 29-11-16 02:22, Peter Hutterer wrote: Move the code from the touchpad code into the more generic evdev code Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Patch looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- src/evdev-mt-touchpad.c | 76 + src/evdev-mt-touchpad.h | 5 src/evdev.c | 6 src/evdev.h | 65 ++ test/log.c | 50 5 files changed, 134 insertions(+), 68 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index 7b8514c..0492851 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -279,39 +279,6 @@ tp_get_delta(struct tp_touch *t) return tp_normalize_delta(t->tp, delta); } -static inline void -tp_check_axis_range(struct tp_dispatch *tp, - unsigned int code, - int value) -{ - int min, max; - - switch(code) { - case ABS_X: - case ABS_MT_POSITION_X: - min = tp->warning_range.min.x; - max = tp->warning_range.max.x; - break; - case ABS_Y: - case ABS_MT_POSITION_Y: - min = tp->warning_range.min.y; - max = tp->warning_range.max.y; - break; - default: - return; - } - - if (value < min || value > max) { - log_info_ratelimit(tp_libinput_context(tp), - >warning_range.range_warn_limit, - "Axis %#x value %d is outside expected range [%d, %d]\n" - "See %s/absolute_coordinate_ranges.html for details\n", - code, value, min, max, - HTTP_DOC_LINK); - - } -} - static void tp_process_absolute(struct tp_dispatch *tp, const struct input_event *e, @@ -321,14 +288,18 @@ tp_process_absolute(struct tp_dispatch *tp, switch(e->code) { case ABS_MT_POSITION_X: - tp_check_axis_range(tp, e->code, e->value); + evdev_device_check_abs_axis_range(tp->device, + e->code, + e->value); t->point.x = e->value; t->millis = time; t->dirty = true; tp->queued |= TOUCHPAD_EVENT_MOTION; break; case ABS_MT_POSITION_Y: - tp_check_axis_range(tp, e->code, e->value); + evdev_device_check_abs_axis_range(tp->device, + e->code, + e->value); t->point.y = e->value; t->millis = time; t->dirty = true; @@ -363,14 +334,18 @@ tp_process_absolute_st(struct tp_dispatch *tp, switch(e->code) { case ABS_X: - tp_check_axis_range(tp, e->code, e->value); + evdev_device_check_abs_axis_range(tp->device, + e->code, + e->value); t->point.x = e->value; t->millis = time; t->dirty = true; tp->queued |= TOUCHPAD_EVENT_MOTION; break; case ABS_Y: - tp_check_axis_range(tp, e->code, e->value); + evdev_device_check_abs_axis_range(tp->device, + e->code, + e->value); t->point.y = e->value; t->millis = time; t->dirty = true; @@ -2241,32 +2216,10 @@ tp_init_hysteresis(struct tp_dispatch *tp) return; } -static void -tp_init_range_warnings(struct tp_dispatch *tp, - struct evdev_device *device, - int width, - int height) -{ - const struct input_absinfo *x, *y; - - x = device->abs.absinfo_x; - y = device->abs.absinfo_y; - - tp->warning_range.min.x = x->minimum - 0.05 * width; - tp->warning_range.min.y = y->minimum - 0.05 * height; - tp->warning_range.max.x = x->maximum + 0.05 * width; - tp->warning_range.max.y = y->maximum + 0.05 * height; - - /* One warning every 5 min is enough */ - ratelimit_init(>warning_range.range_warn_limit, s2us(3000), 1); -} - static int tp_init(struct tp_dispatch *tp, struct evdev_device *device) { - int width, height; - tp->base.interfac
Re: [PATCH libinput] touchpad: add a quirk for the HP Pavilion dm4
Hi, On 29-11-16 04:48, Peter Hutterer wrote: On Mon, Nov 28, 2016 at 03:33:25PM +0100, Hans de Goede wrote: Hi, On 27-11-16 23:55, Peter Hutterer wrote: This touchpad has cursor jumps for 2-finger scrolling that also affects the single-finger emulation. So disable any multitouch bits on this device and disallow the 2-finger scroll method. This still allows for 2-finger tapping/clicking. https://bugs.freedesktop.org/show_bug.cgi?id=91135 Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> This sounds a lot like what we're doing for semi-mt devices, but then with completely different code-paths. AFAICT we still allow BTN_TOOL_DOUBLETAP, etc. for semi-mt, so why not use the same method. To be specific I'm talking about the "if (tp->semi_mt) { ... }" block with the large comment above it in tp_init_slots(), to me it sounds like you want to change that to: if (tp->semi_mit || (tp->device->model_flags & EVDEV_MODEL_HP_PAVILION_DM4_TOUCHPAD)) { ... } Rather then come up with a second approach to only listen to the non-mt coordinates. right, with the semi-mt we do a similar-ish thing but we still allow two-finger scrolling based on the single position + BTN_TOOL_DOUBLETAP. In this case the touchpad data is garbage as soon as two fingers are down, even the single-touch emulation is useless. So it's not quite the same category as semi-mts which can still be ok for 2fg scrolling. And since it's garbage anyway, we might as well disable all MT axes. I see, so that would require keeping this hunk of your original patch: diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index d72cb19..beb19cd 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -1909,6 +1909,12 @@ tp_scroll_get_methods(struct tp_dispatch *tp) { uint32_t methods = LIBINPUT_CONFIG_SCROLL_EDGE; + /* Any movement with more than one finger has random cursor +* jumps. Don't allow for 2fg scrolling on this device, see +* fdo bug 91135 */ + if (tp->device->model_flags & EVDEV_MODEL_HP_PAVILION_DM4_TOUCHPAD) + return LIBINPUT_CONFIG_SCROLL_EDGE; + if (tp->ntouches >= 2) methods |= LIBINPUT_CONFIG_SCROLL_2FG; But my above suggestion could still replace this hunk, which to me really feels like doing the same as the semi-mt handling but then in a more complicated way and having 2 code paths doing the same thing in a different way feels wrong: diff --git a/src/evdev.c b/src/evdev.c index fac8fcb..eb4c0d0 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -2762,6 +2763,17 @@ evdev_pre_configure_model_quirks(struct evdev_device *device) if (device->model_flags & EVDEV_MODEL_HP_STREAM11_TOUCHPAD) libevdev_enable_property(device->evdev, INPUT_PROP_BUTTONPAD); + + /* Touchpad has random jumps in slots, including for single-finger +* movement. See fdo bug 91135 */ + if (device->model_flags & EVDEV_MODEL_HP_PAVILION_DM4_TOUCHPAD) { + unsigned int code; + + for (code = ABS_MT_SLOT; code <= ABS_MT_TRACKING_ID; code++) + libevdev_disable_event_code(device->evdev, + EV_ABS, + code); + } } struct evdev_device * Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 1/7] test: add a test for safe_atoi
Hi, On 28-11-16 06:20, Peter Hutterer wrote: Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Entire series looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- test/misc.c | 42 ++ 1 file changed, 42 insertions(+) diff --git a/test/misc.c b/test/misc.c index 4afd4d0..95776f7 100644 --- a/test/misc.c +++ b/test/misc.c @@ -861,6 +861,47 @@ START_TEST(time_conversion) } END_TEST +struct atoi_test { + char *str; + bool success; + int val; +}; + +START_TEST(safe_atoi_test) +{ + struct atoi_test tests[] = { + { "10", true, 10 }, + { "20", true, 20 }, + { "-1", true, -1 }, + { "2147483647", true, 2147483647 }, + { "-2147483648", true, -2147483648 }, + { "4294967295", false, 0 }, + { "0x0", false, 0 }, + { "-10x10", false, 0 }, + { "1x-99", false, 0 }, + { "", false, 0 }, + { "abd", false, 0 }, + { "xabd", false, 0 }, + { "0xaf", false, 0 }, + { "0x0x", false, 0 }, + { "x10", false, 0 }, + { NULL, false, 0 } + }; + int v; + bool success; + + for (int i = 0; tests[i].str != NULL; i++) { + v = 0xad; + success = safe_atoi(tests[i].str, ); + ck_assert(success == tests[i].success); + if (success) + ck_assert_int_eq(v, tests[i].val); + else + ck_assert_int_eq(v, 0xad); + } +} +END_TEST + static int open_restricted_leak(const char *path, int flags, void *data) { return *(int*)data; @@ -988,6 +1029,7 @@ litest_setup_tests_misc(void) litest_add_no_device("misc:parser", wheel_click_count_parser); litest_add_no_device("misc:parser", trackpoint_accel_parser); litest_add_no_device("misc:parser", dimension_prop_parser); + litest_add_no_device("misc:parser", safe_atoi_test); litest_add_no_device("misc:time", time_conversion); litest_add_no_device("misc:fd", fd_no_event_leak); ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: add a quirk for the HP Pavilion dm4
Hi, On 27-11-16 23:55, Peter Hutterer wrote: This touchpad has cursor jumps for 2-finger scrolling that also affects the single-finger emulation. So disable any multitouch bits on this device and disallow the 2-finger scroll method. This still allows for 2-finger tapping/clicking. https://bugs.freedesktop.org/show_bug.cgi?id=91135 Signed-off-by: Peter HuttererThis sounds a lot like what we're doing for semi-mt devices, but then with completely different code-paths. AFAICT we still allow BTN_TOOL_DOUBLETAP, etc. for semi-mt, so why not use the same method. To be specific I'm talking about the "if (tp->semi_mt) { ... }" block with the large comment above it in tp_init_slots(), to me it sounds like you want to change that to: if (tp->semi_mit || (tp->device->model_flags & EVDEV_MODEL_HP_PAVILION_DM4_TOUCHPAD)) { ... } Rather then come up with a second approach to only listen to the non-mt coordinates. Regards, Hans --- src/evdev-mt-touchpad.c| 6 ++ src/evdev.c| 12 src/evdev.h| 1 + udev/90-libinput-model-quirks.hwdb | 4 4 files changed, 23 insertions(+) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index d72cb19..beb19cd 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -1909,6 +1909,12 @@ tp_scroll_get_methods(struct tp_dispatch *tp) { uint32_t methods = LIBINPUT_CONFIG_SCROLL_EDGE; + /* Any movement with more than one finger has random cursor +* jumps. Don't allow for 2fg scrolling on this device, see +* fdo bug 91135 */ + if (tp->device->model_flags & EVDEV_MODEL_HP_PAVILION_DM4_TOUCHPAD) + return LIBINPUT_CONFIG_SCROLL_EDGE; + if (tp->ntouches >= 2) methods |= LIBINPUT_CONFIG_SCROLL_2FG; diff --git a/src/evdev.c b/src/evdev.c index fac8fcb..eb4c0d0 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -2179,6 +2179,7 @@ evdev_read_model_flags(struct evdev_device *device) MODEL(APPLE_MAGICMOUSE), MODEL(HP8510_TOUCHPAD), MODEL(HP6910_TOUCHPAD), + MODEL(HP_PAVILION_DM4_TOUCHPAD), #undef MODEL { "ID_INPUT_TRACKBALL", EVDEV_MODEL_TRACKBALL }, { NULL, EVDEV_MODEL_DEFAULT }, @@ -2762,6 +2763,17 @@ evdev_pre_configure_model_quirks(struct evdev_device *device) if (device->model_flags & EVDEV_MODEL_HP_STREAM11_TOUCHPAD) libevdev_enable_property(device->evdev, INPUT_PROP_BUTTONPAD); + + /* Touchpad has random jumps in slots, including for single-finger +* movement. See fdo bug 91135 */ + if (device->model_flags & EVDEV_MODEL_HP_PAVILION_DM4_TOUCHPAD) { + unsigned int code; + + for (code = ABS_MT_SLOT; code <= ABS_MT_TRACKING_ID; code++) + libevdev_disable_event_code(device->evdev, + EV_ABS, + code); + } } struct evdev_device * diff --git a/src/evdev.h b/src/evdev.h index b811f51..9689051 100644 --- a/src/evdev.h +++ b/src/evdev.h @@ -121,6 +121,7 @@ enum evdev_device_model { EVDEV_MODEL_APPLE_MAGICMOUSE = (1 << 20), EVDEV_MODEL_HP8510_TOUCHPAD = (1 << 21), EVDEV_MODEL_HP6910_TOUCHPAD = (1 << 22), + EVDEV_MODEL_HP_PAVILION_DM4_TOUCHPAD = (1 << 23), }; struct mt_slot { diff --git a/udev/90-libinput-model-quirks.hwdb b/udev/90-libinput-model-quirks.hwdb index 4bfc0f9..d5b1d78 100644 --- a/udev/90-libinput-model-quirks.hwdb +++ b/udev/90-libinput-model-quirks.hwdb @@ -99,6 +99,10 @@ libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnHewlett-Packard:*pnHPCompaq6910 libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnHewlett-Packard:*pnHPCompaq8510w* LIBINPUT_MODEL_HP8510_TOUCHPAD=1 +# HP Pavillion dm4 +libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnHewlett-Packard:*pnHPPaviliondm4NotebookPC* + LIBINPUT_MODEL_HP_PAVILION_DM4_TOUCHPAD=1 + # HP Stream 11 libinput:name:SYN1EDE:00 06CB:7442:dmi:*svnHewlett-Packard:pnHPStreamNotebookPC11* LIBINPUT_MODEL_HP_STREAM11_TOUCHPAD=1 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: only use the last two coordinates for delta calculation
Hi, On 22-11-16 02:34, Peter Hutterer wrote: Taking the last 4 points means factoring in a coordinate that may be more than 40ms in the past - or even more when the finger moves slowly and we don't get events for a while. This makes the pointer more sluggish and slower to catch up with what the finger is actually doing. We already have the motion hysteresis as a separate item to prevent jumps (and thus adds some delay to the movement), the calculation over time doesn't provide enough benefit to justify the sluggish pointer. Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> --- I'm leaving the motion history as-is for now even though it's largely unused now. This can be fixed up later once we know this patch has the desired effect (it does here, but that could be confirmation bias) Patch looks good to me and is: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans src/evdev-mt-touchpad.c | 20 +--- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index d72cb19..7b8514c 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -262,29 +262,19 @@ tp_end_sequence(struct tp_dispatch *tp, struct tp_touch *t, uint64_t time) tp_end_touch(tp, t, time); } -static double -tp_estimate_delta(int x0, int x1, int x2, int x3) -{ - return (x0 + x1 - x2 - x3) / 4.0; -} - struct normalized_coords tp_get_delta(struct tp_touch *t) { struct device_float_coords delta; const struct normalized_coords zero = { 0.0, 0.0 }; - if (t->history.count < TOUCHPAD_MIN_SAMPLES) + if (t->history.count <= 1) return zero; - delta.x = tp_estimate_delta(tp_motion_history_offset(t, 0)->x, - tp_motion_history_offset(t, 1)->x, - tp_motion_history_offset(t, 2)->x, - tp_motion_history_offset(t, 3)->x); - delta.y = tp_estimate_delta(tp_motion_history_offset(t, 0)->y, - tp_motion_history_offset(t, 1)->y, - tp_motion_history_offset(t, 2)->y, - tp_motion_history_offset(t, 3)->y); + delta.x = tp_motion_history_offset(t, 0)->x - + tp_motion_history_offset(t, 1)->x; + delta.y = tp_motion_history_offset(t, 0)->y - + tp_motion_history_offset(t, 1)->y; return tp_normalize_delta(t->tp, delta); } ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 1/4] test: start with the first offset when moving touches
Hi, On 22-11-16 02:32, Peter Hutterer wrote: This doesn't have an effect in our current tests because the touchpad always needs 4 motion events to get moving. But for the future, it simplifies the case of "i want to move between x1/y1 and x2/y2", because it fills in only the events in between rather than re-using the touch down coordinates and thus not causing a motion on the first event. Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Series looks good to me and is: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- test/litest.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/litest.c b/test/litest.c index 94da85a..e685d61 100644 --- a/test/litest.c +++ b/test/litest.c @@ -1603,7 +1603,7 @@ litest_touch_move_to(struct litest_device *d, double x_to, double y_to, int steps, int sleep_ms) { - for (int i = 0; i < steps - 1; i++) { + for (int i = 1; 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); @@ -1699,7 +1699,7 @@ litest_touch_move_two_touches(struct litest_device *d, double dx, double dy, int steps, int sleep_ms) { - for (int i = 0; i < steps - 1; i++) { + for (int i = 1; i < steps; i++) { litest_push_event_frame(d); litest_touch_move(d, 0, x0 + dx / steps * i, y0 + dy / steps * i); ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] evdev: add a quirk for the HP Zbook Studio G3
Hi, On 20-11-16 00:41, Peter Hutterer wrote: Announces 4 slots but only sends data for the first two. This causes libinput to miss three-finger actions (we don't look at BTN_TOOL_TRIPLETAP if we have 3 or more slots). https://bugs.freedesktop.org/show_bug.cgi?id=98100 Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Patch looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- src/evdev.c| 6 ++ src/evdev.h| 1 + udev/90-libinput-model-quirks.hwdb | 4 3 files changed, 11 insertions(+) diff --git a/src/evdev.c b/src/evdev.c index fac8fcb..6a34e37 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -2179,6 +2179,7 @@ evdev_read_model_flags(struct evdev_device *device) MODEL(APPLE_MAGICMOUSE), MODEL(HP8510_TOUCHPAD), MODEL(HP6910_TOUCHPAD), + MODEL(HP_ZBOOK_STUDIO_G3), #undef MODEL { "ID_INPUT_TRACKBALL", EVDEV_MODEL_TRACKBALL }, { NULL, EVDEV_MODEL_DEFAULT }, @@ -2762,6 +2763,11 @@ evdev_pre_configure_model_quirks(struct evdev_device *device) if (device->model_flags & EVDEV_MODEL_HP_STREAM11_TOUCHPAD) libevdev_enable_property(device->evdev, INPUT_PROP_BUTTONPAD); + + /* Touchpad claims to have 4 slots but only ever sends 2 +* https://bugs.freedesktop.org/show_bug.cgi?id=98100 */ + if (device->model_flags & EVDEV_MODEL_HP_ZBOOK_STUDIO_G3) + libevdev_set_abs_maximum(device->evdev, ABS_MT_SLOT, 1); } struct evdev_device * diff --git a/src/evdev.h b/src/evdev.h index b811f51..888cc28 100644 --- a/src/evdev.h +++ b/src/evdev.h @@ -121,6 +121,7 @@ enum evdev_device_model { EVDEV_MODEL_APPLE_MAGICMOUSE = (1 << 20), EVDEV_MODEL_HP8510_TOUCHPAD = (1 << 21), EVDEV_MODEL_HP6910_TOUCHPAD = (1 << 22), + EVDEV_MODEL_HP_ZBOOK_STUDIO_G3 = (1 << 23), }; struct mt_slot { diff --git a/udev/90-libinput-model-quirks.hwdb b/udev/90-libinput-model-quirks.hwdb index 4bfc0f9..347a229 100644 --- a/udev/90-libinput-model-quirks.hwdb +++ b/udev/90-libinput-model-quirks.hwdb @@ -103,6 +103,10 @@ libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnHewlett-Packard:*pnHPCompaq8510 libinput:name:SYN1EDE:00 06CB:7442:dmi:*svnHewlett-Packard:pnHPStreamNotebookPC11* LIBINPUT_MODEL_HP_STREAM11_TOUCHPAD=1 +# HP Zbook Studio G3 +libinput:name:AlpsPS/2 ALPS GlidePoint:dmi:*svnHP:pnHPZBookStudioG3:* + LIBINPUT_MODEL_HP_ZBOOK_STUDIO_G3=1 + ## # LENOVO ## ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 1/4] configure.ac: move all AM_CONDITIONALs into one place
Hi, On 14-11-16 01:50, Peter Hutterer wrote: Makes it easier to see in one go what is conditional in the build. Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> The entire series looks good to me and is: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- configure.ac | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index 43db9bb..012ab0b 100644 --- a/configure.ac +++ b/configure.ac @@ -159,7 +159,6 @@ if test "x$build_eventgui" = "xyes"; then PKG_CHECK_MODULES(CAIRO, [cairo]) PKG_CHECK_MODULES(GTK, [glib-2.0 gtk+-3.0]) fi -AM_CONDITIONAL(BUILD_EVENTGUI, [test "x$build_eventgui" = "xyes"]) AC_ARG_ENABLE(tests, AS_HELP_STRING([--enable-tests], [Build the tests (default=auto)]), @@ -199,7 +198,6 @@ if test "x$build_tests" = "xyes"; then AC_DEFINE_UNQUOTED(ADDR2LINE, ["$ADDR2LINE"], [Path to addr2line]) fi fi -AM_CONDITIONAL(HAVE_LIBUNWIND, [test "x$HAVE_LIBUNWIND" = xyes]) AC_ARG_ENABLE(libwacom, AS_HELP_STRING([--enable-libwacom], @@ -227,12 +225,14 @@ if test "x$use_libwacom" = "xyes"; then LIBS=$OLD_LIBS CFLAGS=$OLD_CFLAGS fi + AM_CONDITIONAL(HAVE_LIBWACOM_GET_PAIRED_DEVICE, [test "x$libwacom_have_get_paired_device" == "xyes"]) - AM_CONDITIONAL(HAVE_VALGRIND, [test "x$VALGRIND" != "x"]) AM_CONDITIONAL(BUILD_TESTS, [test "x$build_tests" = "xyes"]) AM_CONDITIONAL(BUILD_DOCS, [test "x$build_documentation" = "xyes"]) +AM_CONDITIONAL(HAVE_LIBUNWIND, [test "x$HAVE_LIBUNWIND" = xyes]) +AM_CONDITIONAL(BUILD_EVENTGUI, [test "x$build_eventgui" = "xyes"]) # Used by the udev rules so we can use callouts during testing without # installing everything first. Default is the empty string so the installed ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] evdev: implement support for the MOUSE_WHEEL_CLICK_COUNT property
Hi, On 03-11-16 03:16, Peter Hutterer wrote: On Fri, Oct 28, 2016 at 12:57:38PM +0200, Hans de Goede wrote: Hi, On 28-10-16 07:08, Peter Hutterer wrote: Not all mice have a click angle with integer degrees. The new MOUSE_WHEEL_CLICK_COUNT property specifies how many clicks per full rotation, the angle can be calculated from that. See https://github.com/systemd/systemd/pull/4440 for more information CLICK_COUNT overrides CLICK_ANGLE, so we check for the former first and then fall back to the angle if need be. No changes to the user-facing API. Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> --- src/evdev.c | 51 +++--- src/libinput-private.h | 2 +- src/libinput-util.c | 32 src/libinput-util.h | 1 + test/Makefile.am | 1 + test/litest-device-mouse-wheel-click-count.c | 77 test/litest.c| 2 + test/litest.h| 1 + test/misc.c | 30 +++ test/pointer.c | 49 +++--- 10 files changed, 229 insertions(+), 17 deletions(-) create mode 100644 test/litest-device-mouse-wheel-click-count.c diff --git a/src/evdev.c b/src/evdev.c index d49b391..1c46534 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -2008,7 +2008,7 @@ evdev_device_init_pointer_acceleration(struct evdev_device *device, static inline bool evdev_read_wheel_click_prop(struct evdev_device *device, const char *prop, - int *angle) + double *angle) { int val; @@ -2032,18 +2032,53 @@ evdev_read_wheel_click_prop(struct evdev_device *device, return false; } +static inline bool +evdev_read_wheel_click_count_prop(struct evdev_device *device, + const char *prop, + double *angle) +{ + int val; + + prop = udev_device_get_property_value(device->udev_device, prop); + if (!prop) + return false; + + val = parse_mouse_wheel_click_angle_property(prop); + if (val) { + *angle = 360.0/val; + return true; + } + + log_error(evdev_libinput_context(device), + "Mouse wheel click count '%s' is present but invalid, " + "using %d degrees for angle instead instead\n", + device->devname, + DEFAULT_WHEEL_CLICK_ANGLE); + *angle = DEFAULT_WHEEL_CLICK_ANGLE; + + return false; +} + This is almost a 100% copy of evdev_read_wheel_click_prop how about giving evdev_read_wheel_click_prop an extra "bool count" argument and then doing: if (val) { if (count) *angle = 360.0 / val; else *angle = val; return true; } Then we do not need the almost identical function. I played around with this for a few days and tbh I'm not happy any incarnation that I tried. Any merging together means we have to have conditions for the error message (or a more generic and thus less useful one). And with the parsing functions themselves, yes, they're 100% the same but most of the info is contained in the comments above to explain what the property does. Merging those together is also a net loss in information. Given that they're small enough and they won't see much change over time, I'm gonna go with the current patch after all. Ok. Regards, Hans ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] evdev: add hwdb quirk for HP Compaq 6910
Hi, On 02-11-16 12:33, Peter Hutterer wrote: Same as the HP Compat 8510, it doesn't send BTN_TOOL_DOUBLETAP/TRIPLETAP. This may be a general issue with those series but they're 6 years old now, so it's questionable to spend extra effort detecting them. https://bugs.freedesktop.org/show_bug.cgi?id=98538 Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Patch LGTM: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- src/evdev.c| 7 +-- src/evdev.h| 1 + udev/90-libinput-model-quirks.hwdb | 6 +- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/evdev.c b/src/evdev.c index f423251..2412751 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -2143,6 +2143,7 @@ evdev_read_model_flags(struct evdev_device *device) MODEL(TRACKBALL), MODEL(APPLE_MAGICMOUSE), MODEL(HP8510_TOUCHPAD), + MODEL(HP6910_TOUCHPAD), #undef MODEL { "ID_INPUT_TRACKBALL", EVDEV_MODEL_TRACKBALL }, { NULL, EVDEV_MODEL_DEFAULT }, @@ -2712,9 +2713,11 @@ evdev_pre_configure_model_quirks(struct evdev_device *device) libevdev_disable_event_type(device->evdev, EV_ABS); /* Claims to have double/tripletap but doesn't actually send it -* https://bugzilla.redhat.com/show_bug.cgi?id=1351285 +* https://bugzilla.redhat.com/show_bug.cgi?id=1351285 and +* https://bugzilla.redhat.com/show_bug.cgi?id=98538 */ - if (device->model_flags & EVDEV_MODEL_HP8510_TOUCHPAD) { + if (device->model_flags & + (EVDEV_MODEL_HP8510_TOUCHPAD|EVDEV_MODEL_HP6910_TOUCHPAD)) { libevdev_disable_event_code(device->evdev, EV_KEY, BTN_TOOL_DOUBLETAP); libevdev_disable_event_code(device->evdev, EV_KEY, BTN_TOOL_TRIPLETAP); } diff --git a/src/evdev.h b/src/evdev.h index 4e28e05..b811f51 100644 --- a/src/evdev.h +++ b/src/evdev.h @@ -120,6 +120,7 @@ enum evdev_device_model { EVDEV_MODEL_TRACKBALL = (1 << 19), EVDEV_MODEL_APPLE_MAGICMOUSE = (1 << 20), EVDEV_MODEL_HP8510_TOUCHPAD = (1 << 21), + EVDEV_MODEL_HP6910_TOUCHPAD = (1 << 22), }; struct mt_slot { diff --git a/udev/90-libinput-model-quirks.hwdb b/udev/90-libinput-model-quirks.hwdb index fed28e2..4bfc0f9 100644 --- a/udev/90-libinput-model-quirks.hwdb +++ b/udev/90-libinput-model-quirks.hwdb @@ -91,7 +91,11 @@ libinput:name:Cypress APA Trackpad ?cyapa?:dmi:* # HP ## -# HP 8510w +# HP Compaq6910p +libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnHewlett-Packard:*pnHPCompaq6910p* + LIBINPUT_MODEL_HP6910_TOUCHPAD=1 + +# HP Compaq 8510w libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnHewlett-Packard:*pnHPCompaq8510w* LIBINPUT_MODEL_HP8510_TOUCHPAD=1 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v2 libinput] evdev: actually ignore joysticks
Hi, On 02-11-16 05:51, Peter Hutterer wrote: A joystick has ID_INPUT_JOYSTICK *and* ID_INPUT set, so we need to check for both. https://bugs.freedesktop.org/show_bug.cgi?id=98009 Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Patch looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- Changes to v1: - fix bug with previous version where a device with just ID_INPUT set would be detected as joystick (though in hindsight this wouldn't have been an issue as we filter those out early anyway) src/evdev.c | 2 +- test/device.c | 34 ++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/evdev.c b/src/evdev.c index d49b391..f423251 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -2468,7 +2468,7 @@ evdev_configure_device(struct evdev_device *device) /* libwacom *adds* TABLET, TOUCHPAD but leaves JOYSTICK in place, so make sure we only ignore real joystick devices */ - if ((udev_tags & EVDEV_UDEV_TAG_JOYSTICK) == udev_tags) { + if (udev_tags == (EVDEV_UDEV_TAG_INPUT|EVDEV_UDEV_TAG_JOYSTICK)) { log_info(libinput, "input device '%s', %s is a joystick, ignoring\n", device->devname, devnode); diff --git a/test/device.c b/test/device.c index 4ed12d9..f44a988 100644 --- a/test/device.c +++ b/test/device.c @@ -1058,6 +1058,39 @@ START_TEST(abs_mt_device_missing_res) } END_TEST +START_TEST(ignore_joystick) +{ + struct libinput *li; + struct libevdev_uinput *uinput; + struct libinput_device *device; + struct input_absinfo absinfo[] = { + { ABS_X, 0, 10, 0, 0, 10 }, + { ABS_Y, 0, 10, 0, 0, 10 }, + { ABS_RX, 0, 10, 0, 0, 10 }, + { ABS_RY, 0, 10, 0, 0, 10 }, + { ABS_THROTTLE, 0, 2, 0, 0, 0 }, + { ABS_RUDDER, 0, 255, 0, 0, 0 }, + { -1, -1, -1, -1, -1, -1 } + }; + + li = litest_create_context(); + litest_disable_log_handler(li); + litest_drain_events(li); + + uinput = litest_create_uinput_abs_device("joystick test device", NULL, +absinfo, +EV_KEY, BTN_TRIGGER, +EV_KEY, BTN_A, +-1); + device = libinput_path_add_device(li, + libevdev_uinput_get_devnode(uinput)); + litest_assert_ptr_null(device); + libevdev_uinput_destroy(uinput); + litest_restore_log_handler(li); + libinput_unref(li); +} +END_TEST + START_TEST(device_wheel_only) { struct litest_device *dev = litest_current_device(); @@ -1464,6 +1497,7 @@ litest_setup_tests_device(void) litest_add_ranged_no_device("device:invalid devices", abs_mt_device_no_range, _mt_range); litest_add_no_device("device:invalid devices", abs_device_missing_res); litest_add_no_device("device:invalid devices", abs_mt_device_missing_res); + litest_add_no_device("device:invalid devices", ignore_joystick); litest_add("device:wheel", device_wheel_only, LITEST_WHEEL, LITEST_RELATIVE|LITEST_ABSOLUTE|LITEST_TABLET); litest_add_no_device("device:accelerometer", device_accelerometer); ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 1/4] touchpad: split the touchpad->keyboard/trackpoint pairing helpers out
Hi, On 28-10-16 07:41, Peter Hutterer wrote: No functional changes Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Entire series LGTM: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- src/evdev-mt-touchpad.c | 24 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index 7867122..2ee0f79 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -1536,29 +1536,37 @@ tp_dwt_pair_keyboard(struct evdev_device *touchpad, } static void -tp_interface_device_added(struct evdev_device *device, - struct evdev_device *added_device) +tp_pair_trackpoint(struct evdev_device *touchpad, + struct evdev_device *trackpoint) { - struct tp_dispatch *tp = (struct tp_dispatch*)device->dispatch; - unsigned int bus_tp = libevdev_get_id_bustype(device->evdev), -bus_trp = libevdev_get_id_bustype(added_device->evdev); + struct tp_dispatch *tp = (struct tp_dispatch*)touchpad->dispatch; + unsigned int bus_tp = libevdev_get_id_bustype(touchpad->evdev), +bus_trp = libevdev_get_id_bustype(trackpoint->evdev); bool tp_is_internal, trp_is_internal; tp_is_internal = bus_tp != BUS_USB && bus_tp != BUS_BLUETOOTH; trp_is_internal = bus_trp != BUS_USB && bus_trp != BUS_BLUETOOTH; if (tp->buttons.trackpoint == NULL && - (added_device->tags & EVDEV_TAG_TRACKPOINT) && + (trackpoint->tags & EVDEV_TAG_TRACKPOINT) && tp_is_internal && trp_is_internal) { /* Don't send any pending releases to the new trackpoint */ tp->buttons.active_is_topbutton = false; - tp->buttons.trackpoint = added_device; + tp->buttons.trackpoint = trackpoint; if (tp->palm.monitor_trackpoint) - libinput_device_add_event_listener(_device->base, + libinput_device_add_event_listener(>base, >palm.trackpoint_listener, tp_trackpoint_event, tp); } +} +static void +tp_interface_device_added(struct evdev_device *device, + struct evdev_device *added_device) +{ + struct tp_dispatch *tp = (struct tp_dispatch*)device->dispatch; + + tp_pair_trackpoint(device, added_device); if (added_device->tags & EVDEV_TAG_KEYBOARD) tp_dwt_pair_keyboard(device, added_device); ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] udev: add the hwdb_parser.py test from systemd
Hi, On 28-10-16 07:33, Peter Hutterer wrote: upstream for this file lives in systemd, any changes to the actual parser should flow back there. libinput's matches are fairly simple. We have the various LIBINPUT_MODEL_ tags that just take a "1" and the two attributes that are dimensions. Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Patch LGTM: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- configure.ac | 4 + udev/90-libinput-model-quirks.hwdb | 2 +- udev/Makefile.am | 4 + udev/parse_hwdb.py | 178 + 4 files changed, 187 insertions(+), 1 deletion(-) create mode 100755 udev/parse_hwdb.py diff --git a/configure.ac b/configure.ac index 38b465d..8658247 100644 --- a/configure.ac +++ b/configure.ac @@ -46,6 +46,10 @@ AC_PROG_CC_C99 AC_PROG_CXX # Only used by build C++ test AC_PROG_GREP +# Only used for testing the hwdb +AM_PATH_PYTHON([3.0],, [:]) +AM_CONDITIONAL([HAVE_PYTHON], [test "$PYTHON" != :]) + # Initialize libtool LT_PREREQ([2.2]) LT_INIT diff --git a/udev/90-libinput-model-quirks.hwdb b/udev/90-libinput-model-quirks.hwdb index ec71c26..fed28e2 100644 --- a/udev/90-libinput-model-quirks.hwdb +++ b/udev/90-libinput-model-quirks.hwdb @@ -96,7 +96,7 @@ libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnHewlett-Packard:*pnHPCompaq8510 LIBINPUT_MODEL_HP8510_TOUCHPAD=1 # HP Stream 11 -evdev:name:SYN1EDE:00 06CB:7442:dmi:*svnHewlett-Packard:pnHPStreamNotebookPC11* +libinput:name:SYN1EDE:00 06CB:7442:dmi:*svnHewlett-Packard:pnHPStreamNotebookPC11* LIBINPUT_MODEL_HP_STREAM11_TOUCHPAD=1 ## diff --git a/udev/Makefile.am b/udev/Makefile.am index f06dc56..74103f4 100644 --- a/udev/Makefile.am +++ b/udev/Makefile.am @@ -41,3 +41,7 @@ DISTCLEANFILES = \ 80-libinput-device-groups.rules \ 90-libinput-model-quirks.rules EXTRA_DIST = 80-libinput-test-device.rules + +if HAVE_PYTHON +TESTS = parse_hwdb.py +endif diff --git a/udev/parse_hwdb.py b/udev/parse_hwdb.py new file mode 100755 index 000..99e9c59 --- /dev/null +++ b/udev/parse_hwdb.py @@ -0,0 +1,178 @@ +#!/usr/bin/python3 +# vim: set expandtab shiftwidth=4: +# -*- Mode: python; coding: utf-8; indent-tabs-mode: nil -*- */ +# +# ANY MODIFICATIONS TO THIS FILE SHOULD BE MERGED INTO THE SYSTEMD UPSTREAM +# +# This file is part of systemd. It is distributed under the MIT license, see +# below. +# +# Copyright 2016 Zbigniew Jędrzejewski-Szmek +# +# The MIT License (MIT) +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +# SOFTWARE. + +import functools +import glob +import string +import sys +import os + +try: +from pyparsing import (Word, White, Literal, ParserElement, Regex, + LineStart, LineEnd, + ZeroOrMore, OneOrMore, Combine, Or, Optional, Suppress, Group, + nums, alphanums, printables, + stringEnd, pythonStyleComment, + ParseBaseException) +except ImportError: +print('pyparsing is not available') +sys.exit(77) + +try: +from evdev.ecodes import ecodes +except ImportError: +ecodes = None +print('WARNING: evdev is not available') + +EOL = LineEnd().suppress() +EMPTYLINE = LineStart() + LineEnd() +COMMENTLINE = pythonStyleComment + EOL +INTEGER = Word(nums) +REAL = Combine((INTEGER + Optional('.' + Optional(INTEGER))) ^ ('.' + INTEGER)) +UDEV_TAG = Word(string.ascii_uppercase, alphanums + '_') + +TYPES = { + 'libinput': ('name', 'touchpad', 'mouse'), + } + +@functools.lru_cache() +def hwdb_grammar(): +ParserElement.setDefaultWhitespaceChars('') + +prefix = Or(category + ':' + Or(conn) + ':' +for category, conn in TYPES.items()) +matchline = Combine(prefix + Word(printables + ' ' + '®')) + E
Re: [PATCH libinput] evdev: implement support for the MOUSE_WHEEL_CLICK_COUNT property
Hi, On 28-10-16 07:08, Peter Hutterer wrote: Not all mice have a click angle with integer degrees. The new MOUSE_WHEEL_CLICK_COUNT property specifies how many clicks per full rotation, the angle can be calculated from that. See https://github.com/systemd/systemd/pull/4440 for more information CLICK_COUNT overrides CLICK_ANGLE, so we check for the former first and then fall back to the angle if need be. No changes to the user-facing API. Signed-off-by: Peter Hutterer--- src/evdev.c | 51 +++--- src/libinput-private.h | 2 +- src/libinput-util.c | 32 src/libinput-util.h | 1 + test/Makefile.am | 1 + test/litest-device-mouse-wheel-click-count.c | 77 test/litest.c| 2 + test/litest.h| 1 + test/misc.c | 30 +++ test/pointer.c | 49 +++--- 10 files changed, 229 insertions(+), 17 deletions(-) create mode 100644 test/litest-device-mouse-wheel-click-count.c diff --git a/src/evdev.c b/src/evdev.c index d49b391..1c46534 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -2008,7 +2008,7 @@ evdev_device_init_pointer_acceleration(struct evdev_device *device, static inline bool evdev_read_wheel_click_prop(struct evdev_device *device, const char *prop, - int *angle) + double *angle) { int val; @@ -2032,18 +2032,53 @@ evdev_read_wheel_click_prop(struct evdev_device *device, return false; } +static inline bool +evdev_read_wheel_click_count_prop(struct evdev_device *device, + const char *prop, + double *angle) +{ + int val; + + prop = udev_device_get_property_value(device->udev_device, prop); + if (!prop) + return false; + + val = parse_mouse_wheel_click_angle_property(prop); + if (val) { + *angle = 360.0/val; + return true; + } + + log_error(evdev_libinput_context(device), + "Mouse wheel click count '%s' is present but invalid, " + "using %d degrees for angle instead instead\n", + device->devname, + DEFAULT_WHEEL_CLICK_ANGLE); + *angle = DEFAULT_WHEEL_CLICK_ANGLE; + + return false; +} + This is almost a 100% copy of evdev_read_wheel_click_prop how about giving evdev_read_wheel_click_prop an extra "bool count" argument and then doing: if (val) { if (count) *angle = 360.0 / val; else *angle = val; return true; } Then we do not need the almost identical function. static inline struct wheel_angle evdev_read_wheel_click_props(struct evdev_device *device) { struct wheel_angle angles; - evdev_read_wheel_click_prop(device, - "MOUSE_WHEEL_CLICK_ANGLE", - ); - if (!evdev_read_wheel_click_prop(device, -"MOUSE_WHEEL_CLICK_ANGLE_HORIZONTAL", -)) - angles.y = angles.x; + /* CLICK_COUNT overrides CLICK_ANGLE */ + if (!evdev_read_wheel_click_count_prop(device, + "MOUSE_WHEEL_CLICK_COUNT", + )) + evdev_read_wheel_click_prop(device, + "MOUSE_WHEEL_CLICK_ANGLE", + ); + if (!evdev_read_wheel_click_count_prop(device, + "MOUSE_WHEEL_CLICK_COUNT_HORIZONTAL", + )) { + if (!evdev_read_wheel_click_prop(device, + "MOUSE_WHEEL_CLICK_ANGLE_HORIZONTAL", +)) + angles.y = angles.x; + } return angles; } diff --git a/src/libinput-private.h b/src/libinput-private.h index 28656e0..2044cdd 100644 --- a/src/libinput-private.h +++ b/src/libinput-private.h @@ -73,7 +73,7 @@ struct normalized_range_coords { /* A pair of angles in degrees */ struct wheel_angle { - int x, y; + double x, y; }; /* A pair of angles in degrees */ diff --git a/src/libinput-util.c b/src/libinput-util.c index 4b90fbb..6c051c3 100644 --- a/src/libinput-util.c +++ b/src/libinput-util.c @@ -176,6 +176,38 @@ parse_mouse_dpi_property(const char *prop) } /** + * Helper function to parse the MOUSE_WHEEL_CLICK_COUNT property from udev. + * Property is of
Re: [PATCH libinput 1/3] Mark some internal log functions as printf-style function
Hi, All 3 patches LGTM and are: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans On 24-10-16 03:45, Peter Hutterer wrote: Fixes the respective clang warnings Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> --- src/libinput.c| 6 ++ test/litest.c | 16 test/misc.c | 7 +++ tools/event-gui.c | 8 tools/shared.c| 7 +++ 5 files changed, 44 insertions(+) diff --git a/src/libinput.c b/src/libinput.c index 6958042..ec1c72a 100644 --- a/src/libinput.c +++ b/src/libinput.c @@ -163,6 +163,12 @@ static void libinput_default_log_func(struct libinput *libinput, enum libinput_log_priority priority, const char *format, va_list args) + LIBINPUT_ATTRIBUTE_PRINTF(3, 0); + +static void +libinput_default_log_func(struct libinput *libinput, + enum libinput_log_priority priority, + const char *format, va_list args) { const char *prefix; diff --git a/test/litest.c b/test/litest.c index 4c301b5..940cf79 100644 --- a/test/litest.c +++ b/test/litest.c @@ -270,6 +270,15 @@ litest_fail_condition(const char *file, const char *condition, const char *message, ...) + LIBINPUT_ATTRIBUTE_PRINTF(5, 6); + +void +litest_fail_condition(const char *file, + int line, + const char *func, + const char *condition, + const char *message, + ...) { litest_log("FAILED: %s\n", condition); @@ -761,6 +770,13 @@ litest_log_handler(struct libinput *libinput, enum libinput_log_priority pri, const char *format, va_list args) + LIBINPUT_ATTRIBUTE_PRINTF(3, 0); + +static void +litest_log_handler(struct libinput *libinput, + enum libinput_log_priority pri, + const char *format, + va_list args) { const char *priority = NULL; diff --git a/test/misc.c b/test/misc.c index 791ebc3..44c4502 100644 --- a/test/misc.c +++ b/test/misc.c @@ -852,6 +852,13 @@ simple_log_handler(struct libinput *libinput, enum libinput_log_priority priority, const char *format, va_list args) + LIBINPUT_ATTRIBUTE_PRINTF(3, 0); + +static void +simple_log_handler(struct libinput *libinput, + enum libinput_log_priority priority, + const char *format, + va_list args) { vfprintf(stderr, format, args); } diff --git a/tools/event-gui.c b/tools/event-gui.c index b67ca45..e5fb26a 100644 --- a/tools/event-gui.c +++ b/tools/event-gui.c @@ -110,6 +110,10 @@ struct window { static int error(const char *fmt, ...) + LIBINPUT_ATTRIBUTE_PRINTF(1, 2); + +static int +error(const char *fmt, ...) { va_list args; fprintf(stderr, "error: "); @@ -123,6 +127,10 @@ error(const char *fmt, ...) static void msg(const char *fmt, ...) + LIBINPUT_ATTRIBUTE_PRINTF(1, 2); + +static void +msg(const char *fmt, ...) { va_list args; printf("info: "); diff --git a/tools/shared.c b/tools/shared.c index 95655ba..f539957 100644 --- a/tools/shared.c +++ b/tools/shared.c @@ -70,6 +70,13 @@ log_handler(struct libinput *li, enum libinput_log_priority priority, const char *format, va_list args) + LIBINPUT_ATTRIBUTE_PRINTF(3, 0); + +static void +log_handler(struct libinput *li, + enum libinput_log_priority priority, + const char *format, + va_list args) { vprintf(format, args); } ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: require at least 3 events before enabling trackpoint palm detection
Hi, On 08-09-16 07:43, Peter Hutterer wrote: On Wed, Sep 07, 2016 at 10:22:22AM +1000, Peter Hutterer wrote: Some trackpoints, notably the one on the Lenovo T460s have a tendency to send the odd event even when they're not actually used. Trackpoint events trigger palm detection (see 0210f1fee193) and thus effectively disable the touchpad, causing the touchpad to appear nonresponsive. Fix this by requiring at least 3 events from a trackpoint before palm detection is enabled. For normal use it's hard enough to trigger a single event anyway so this should not affect the normal use-case. Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> follow-up: this is actually this bug here: https://bugzilla.redhat.com/show_bug.cgi?id=1364850 after looking at the wrong thing for a while I found that comment 3 shows that single event from event5 (the trackpoint node). I somehow missed the original posting of this patch, so I'm replying this mail instead. Patch LGTM: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans Cheers, Peter --- src/evdev-mt-touchpad.c | 9 - src/evdev-mt-touchpad.h | 1 + test/trackpoint.c | 26 ++ 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index 65b0abf..8f7dbf0 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -1302,6 +1302,7 @@ tp_trackpoint_timeout(uint64_t now, void *data) tp_tap_resume(tp, now); tp->palm.trackpoint_active = false; + tp->palm.trackpoint_event_count = 0; } static void @@ -1314,6 +1315,13 @@ tp_trackpoint_event(uint64_t time, struct libinput_event *event, void *data) if (event->type == LIBINPUT_EVENT_POINTER_BUTTON) return; + tp->palm.trackpoint_last_event_time = time; + tp->palm.trackpoint_event_count++; + + /* Require at least three events before enabling palm detection */ + if (tp->palm.trackpoint_event_count < 3) + return; + if (!tp->palm.trackpoint_active) { tp_edge_scroll_stop_events(tp, time); tp_gesture_cancel(tp, time); @@ -1321,7 +1329,6 @@ tp_trackpoint_event(uint64_t time, struct libinput_event *event, void *data) tp->palm.trackpoint_active = true; } - tp->palm.trackpoint_last_event_time = time; libinput_timer_set(>palm.trackpoint_timer, time + DEFAULT_TRACKPOINT_ACTIVITY_TIMEOUT); } diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h index 8a8d2db..c2edb83 100644 --- a/src/evdev-mt-touchpad.h +++ b/src/evdev-mt-touchpad.h @@ -337,6 +337,7 @@ struct tp_dispatch { struct libinput_event_listener trackpoint_listener; struct libinput_timer trackpoint_timer; uint64_t trackpoint_last_event_time; + uint32_t trackpoint_event_count; bool monitor_trackpoint; } palm; diff --git a/test/trackpoint.c b/test/trackpoint.c index b92b994..e9ba027 100644 --- a/test/trackpoint.c +++ b/test/trackpoint.c @@ -349,6 +349,31 @@ START_TEST(trackpoint_palmdetect_resume_touch) } END_TEST +START_TEST(trackpoint_palmdetect_require_min_events) +{ + struct litest_device *trackpoint = litest_current_device(); + struct litest_device *touchpad; + struct libinput *li = trackpoint->libinput; + + touchpad = litest_add_device(li, LITEST_SYNAPTICS_I2C); + litest_drain_events(li); + + /* A single event does not trigger palm detection */ + litest_event(trackpoint, EV_REL, REL_X, 1); + litest_event(trackpoint, EV_REL, REL_Y, 1); + litest_event(trackpoint, EV_SYN, SYN_REPORT, 0); + libinput_dispatch(li); + litest_drain_events(li); + + litest_touch_down(touchpad, 0, 30, 30); + litest_touch_move_to(touchpad, 0, 30, 30, 80, 80, 10, 1); + litest_touch_up(touchpad, 0); + litest_assert_only_typed_events(li, LIBINPUT_EVENT_POINTER_MOTION); + + litest_delete_device(touchpad); +} +END_TEST + void litest_setup_tests_trackpoint(void) { @@ -362,4 +387,5 @@ litest_setup_tests_trackpoint(void) litest_add("trackpoint:palmdetect", trackpoint_palmdetect, LITEST_POINTINGSTICK, LITEST_ANY); litest_add("trackpoint:palmdetect", trackpoint_palmdetect_resume_touch, LITEST_POINTINGSTICK, LITEST_ANY); + litest_add("trackpoint:palmdetect", trackpoint_palmdetect_require_min_events, LITEST_POINTINGSTICK, LITEST_ANY); } -- 2.7.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] Force the HP Stream 11 touchpad as a clickpad
Hi, On 01-09-16 03:38, Peter Hutterer wrote: INPUT_PROP_BUTTONPAD is not set on this device and RMI4 which should fix this is a bit too far into the future at this point. Hack around it. https://bugs.freedesktop.org/show_bug.cgi?id=97147 Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- src/evdev.c| 7 +++ src/evdev.h| 1 + udev/90-libinput-model-quirks.hwdb | 4 3 files changed, 12 insertions(+) diff --git a/src/evdev.c b/src/evdev.c index 399aab5..3f4a128 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -1950,6 +1950,7 @@ evdev_read_model_flags(struct evdev_device *device) MODEL(APPLE_INTERNAL_KEYBOARD), MODEL(CYBORG_RAT), MODEL(CYAPA), + MODEL(HP_STREAM11_TOUCHPAD), MODEL(LENOVO_T450_TOUCHPAD), MODEL(DELL_TOUCHPAD), MODEL(TRACKBALL), @@ -2530,6 +2531,12 @@ evdev_pre_configure_model_quirks(struct evdev_device *device) libevdev_disable_event_code(device->evdev, EV_KEY, BTN_TOOL_DOUBLETAP); libevdev_disable_event_code(device->evdev, EV_KEY, BTN_TOOL_TRIPLETAP); } + + /* Touchpad is a clickpad but INPUT_PROP_BUTTONPAD is not set, see +* fdo bug 97147. Remove when RMI4 is commonplace */ + if (device->model_flags & EVDEV_MODEL_HP_STREAM11_TOUCHPAD) + libevdev_enable_property(device->evdev, +INPUT_PROP_BUTTONPAD); } struct evdev_device * diff --git a/src/evdev.h b/src/evdev.h index 10b0e58..0d632ef 100644 --- a/src/evdev.h +++ b/src/evdev.h @@ -114,6 +114,7 @@ enum evdev_device_model { EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD = (1 << 13), EVDEV_MODEL_CYBORG_RAT = (1 << 14), EVDEV_MODEL_CYAPA = (1 << 15), + EVDEV_MODEL_HP_STREAM11_TOUCHPAD = (1 << 16), EVDEV_MODEL_LENOVO_T450_TOUCHPAD= (1 << 17), EVDEV_MODEL_DELL_TOUCHPAD = (1 << 18), EVDEV_MODEL_TRACKBALL = (1 << 19), diff --git a/udev/90-libinput-model-quirks.hwdb b/udev/90-libinput-model-quirks.hwdb index 2bccc71..33dd7ff 100644 --- a/udev/90-libinput-model-quirks.hwdb +++ b/udev/90-libinput-model-quirks.hwdb @@ -93,6 +93,10 @@ libinput:name:Cypress APA Trackpad ?cyapa?:dmi:* libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnHewlett-Packard:*pnHPCompaq8510w* LIBINPUT_MODEL_HP8510_TOUCHPAD=1 +# HP Stream 11 +evdev:name:SYN1EDE:00 06CB:7442:dmi:*svnHewlett-Packard:pnHPStreamNotebookPC11* + LIBINPUT_MODEL_HP_STREAM11_TOUCHPAD=1 + ## # LENOVO ## ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] evdev: add quirk for the HP85810 touchpad
Hi, On 31-08-16 02:27, Peter Hutterer wrote: The touchpad's says it can do two- and three-finger detection but it never sends events for it. Disable them so we treat it as pure single-finger touchpad. https://bugzilla.redhat.com/show_bug.cgi?id=1351285 Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Patch LGTM: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- src/evdev.c| 9 + src/evdev.h| 1 + udev/90-libinput-model-quirks.hwdb | 7 +++ 3 files changed, 17 insertions(+) diff --git a/src/evdev.c b/src/evdev.c index 4ec74a5..9154e96 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -1954,6 +1954,7 @@ evdev_read_model_flags(struct evdev_device *device) MODEL(DELL_TOUCHPAD), MODEL(TRACKBALL), MODEL(APPLE_MAGICMOUSE), + MODEL(HP8510_TOUCHPAD), #undef MODEL { "ID_INPUT_TRACKBALL", EVDEV_MODEL_TRACKBALL }, { NULL, EVDEV_MODEL_DEFAULT }, @@ -2521,6 +2522,14 @@ evdev_pre_configure_model_quirks(struct evdev_device *device) */ if (device->model_flags & EVDEV_MODEL_APPLE_MAGICMOUSE) libevdev_disable_event_type(device->evdev, EV_ABS); + + /* Claims to have double/tripletap but doesn't actually send it +* https://bugzilla.redhat.com/show_bug.cgi?id=1351285 +*/ + if (device->model_flags & EVDEV_MODEL_HP8510_TOUCHPAD) { + libevdev_disable_event_code(device->evdev, EV_KEY, BTN_TOOL_DOUBLETAP); + libevdev_disable_event_code(device->evdev, EV_KEY, BTN_TOOL_TRIPLETAP); + } } struct evdev_device * diff --git a/src/evdev.h b/src/evdev.h index 9564e77..10b0e58 100644 --- a/src/evdev.h +++ b/src/evdev.h @@ -118,6 +118,7 @@ enum evdev_device_model { EVDEV_MODEL_DELL_TOUCHPAD = (1 << 18), EVDEV_MODEL_TRACKBALL = (1 << 19), EVDEV_MODEL_APPLE_MAGICMOUSE = (1 << 20), + EVDEV_MODEL_HP8510_TOUCHPAD = (1 << 21), }; struct mt_slot { diff --git a/udev/90-libinput-model-quirks.hwdb b/udev/90-libinput-model-quirks.hwdb index ebce8d2..2bccc71 100644 --- a/udev/90-libinput-model-quirks.hwdb +++ b/udev/90-libinput-model-quirks.hwdb @@ -87,6 +87,13 @@ libinput:name:Cypress APA Trackpad ?cyapa?:dmi:* LIBINPUT_MODEL_CYAPA=1 ## +# HP +## +# +libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnHewlett-Packard:*pnHPCompaq8510w* + LIBINPUT_MODEL_HP8510_TOUCHPAD=1 + +## # LENOVO ## ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: always reset the motion history on finger changes
Hi, On 29-08-16 06:25, Peter Hutterer wrote: We've already been doing this for semi-mt devices and for non-clickpads but let's do it for clickpads as well. On Synaptics touchpads (PS/2 and RMI4) we see slot jumps where two slots are active, slot X ends but slot Y continues with the other slot's positional data. This causes a cursor jump on finger lift after a two-finger scrolling motion. Simply resetting the motion history fixes it. The only multi-finger interaction where a user could expect perfect fluid motion is when using a second finger to touch cone of the software button areas. Let's see if we have complaints first before we implement something more complex. https://bugs.freedesktop.org/show_bug.cgi?id=91695 Signed-off-by:Peter Hutterer <peter.hutte...@who-t.net> A nice and KISS solution, I like: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- I had a nice and more complicated patch to address the issue where touches change slots but really, this on is enough. src/evdev-mt-touchpad.c | 10 +- test/touchpad.c | 51 + 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index 3baed09..65b0abf 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -957,11 +957,11 @@ tp_need_motion_history_reset(struct tp_dispatch *tp) { bool rc = false; - /* Semi-mt finger postions may "jump" when nfingers changes. And on -* a non-clickpad the only reason to have more than one finger down -* is scrolling/gesture, so a reset just makes things sane again */ - if ((tp->semi_mt || !tp->buttons.is_clickpad) && - tp->nfingers_down != tp->old_nfingers_down) + /* Changing the numbers of fingers can cause a jump in the +* coordinates, always reset the motion history for all touches when +* that happens. +*/ + if (tp->nfingers_down != tp->old_nfingers_down) return true; /* if we're transitioning between slots and fake touches in either diff --git a/test/touchpad.c b/test/touchpad.c index ea20036..6e0d310 100644 --- a/test/touchpad.c +++ b/test/touchpad.c @@ -4315,6 +4315,56 @@ START_TEST(touchpad_tool_tripletap_touch_count) } END_TEST +START_TEST(touchpad_slot_swap) +{ + struct litest_device *dev = litest_current_device(); + struct libinput *li = dev->libinput; + struct libinput_event *event; + struct libinput_event_pointer *ptrev; + int first, second; + + /* Synaptics touchpads sometimes end the wrong touchpoint on finger +* up, causing the remaining slot to continue with the other slot's +* coordinates. +* https://bugs.freedesktop.org/show_bug.cgi?id=91352 +*/ + litest_drain_events(li); + + for (first = 0; first <= 1; first++) { + second = 1 - first; + + litest_touch_down(dev, 0, 50, 50); + libinput_dispatch(li); + litest_touch_down(dev, 1, 70, 70); + libinput_dispatch(li); + + litest_touch_move_to(dev, first, 50, 50, 50, 30, 10, 1); + litest_drain_events(li); + + /* release touch 0, continue other slot with 0's coords */ + litest_push_event_frame(dev); + litest_touch_up(dev, first); + litest_touch_move(dev, second, 50, 30.1); + litest_pop_event_frame(dev); + libinput_dispatch(li); + litest_touch_move_to(dev, second, 50, 30, 50, 11, 10, 1); + libinput_dispatch(li); + event = libinput_get_event(li); + do { + ptrev = litest_is_motion_event(event); + ck_assert_double_eq(libinput_event_pointer_get_dx(ptrev), 0.0); + ck_assert_double_lt(libinput_event_pointer_get_dy(ptrev), 1.0); + + libinput_event_destroy(event); + event = libinput_get_event(li); + } while (event); + litest_assert_empty_queue(li); + + litest_touch_up(dev, second); + } +} +END_TEST + START_TEST(touchpad_time_usec) { struct litest_device *dev = litest_current_device(); @@ -4513,6 +4563,7 @@ litest_setup_tests_touchpad(void) litest_add("touchpad:thumb", touchpad_thumb_tap_hold_2ndfg_tap, LITEST_CLICKPAD, LITEST_SINGLE_TOUCH); litest_add_for_device("touchpad:bugs", touchpad_tool_tripletap_touch_count, LITEST_SYNAPTICS_TOPBUTTONPAD); + litest_add_for_device("touchpad:bugs", touchpad_slot_swap, LITEST_SYNAPTICS_TOPBUTTONPAD); litest_add("touchpad:time", touchpad_time_usec, LITEST_TOUCHPAD, LITEST_ANY); ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: reset the edge scroll state on touch up if edge scroll is disabled
Hi, On 22-08-16 08:17, Peter Hutterer wrote: If a touch was down (and up again) before the device was switched to edge scrolling, libinput reported an error message: litest error: libinput bug: unexpected scroll event 0 in area state While edge scrolling was disabled, any new touch would be set to the area state but it was never reset on touch release. https://bugs.freedesktop.org/show_bug.cgi?id=97425 Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- src/evdev-mt-touchpad-edge-scroll.c | 3 +++ test/touchpad.c | 8 2 files changed, 11 insertions(+) diff --git a/src/evdev-mt-touchpad-edge-scroll.c b/src/evdev-mt-touchpad-edge-scroll.c index 081d2c4..1d30bca 100644 --- a/src/evdev-mt-touchpad-edge-scroll.c +++ b/src/evdev-mt-touchpad-edge-scroll.c @@ -338,6 +338,9 @@ tp_edge_scroll_handle_state(struct tp_dispatch *tp, uint64_t time) if (t->state == TOUCH_BEGIN) t->scroll.edge_state = EDGE_SCROLL_TOUCH_STATE_AREA; + else if (t->state == TOUCH_END) + t->scroll.edge_state = + EDGE_SCROLL_TOUCH_STATE_NONE; } return; } diff --git a/test/touchpad.c b/test/touchpad.c index 06bde13..ea20036 100644 --- a/test/touchpad.c +++ b/test/touchpad.c @@ -429,6 +429,10 @@ START_TEST(touchpad_edge_scroll_vert) struct litest_device *dev = litest_current_device(); struct libinput *li = dev->libinput; + litest_touch_down(dev, 0, 99, 20); + litest_touch_move_to(dev, 0, 99, 20, 99, 80, 10, 0); + litest_touch_up(dev, 0); + litest_drain_events(li); litest_enable_edge_scroll(dev); @@ -466,6 +470,10 @@ START_TEST(touchpad_edge_scroll_horiz) struct litest_device *dev = litest_current_device(); struct libinput *li = dev->libinput; + litest_touch_down(dev, 0, 99, 20); + litest_touch_move_to(dev, 0, 99, 20, 99, 80, 10, 0); + litest_touch_up(dev, 0); + if (!touchpad_has_horiz_edge_scroll_size(dev)) return; ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] touchpad: on a non-clickpad, reset the motion history on nfingers change
Hi, On 18-08-16 08:02, Peter Hutterer wrote: The only reason to have more than one finger on a non-clickpad is to tap, scroll or gesture. In all cases resetting the motion history is a good idea to avoid jumps moving from 2 to 1 finger. https://bugs.freedesktop.org/show_bug.cgi?id=91695 https://bugs.freedesktop.org/show_bug.cgi?id=97194 Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- src/evdev-mt-touchpad.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c index 6cebfa3..3baed09 100644 --- a/src/evdev-mt-touchpad.c +++ b/src/evdev-mt-touchpad.c @@ -957,8 +957,11 @@ tp_need_motion_history_reset(struct tp_dispatch *tp) { bool rc = false; - /* semi-mt finger postions may "jump" when nfingers changes */ - if (tp->semi_mt && tp->nfingers_down != tp->old_nfingers_down) + /* Semi-mt finger postions may "jump" when nfingers changes. And on +* a non-clickpad the only reason to have more than one finger down +* is scrolling/gesture, so a reset just makes things sane again */ + if ((tp->semi_mt || !tp->buttons.is_clickpad) && + tp->nfingers_down != tp->old_nfingers_down) return true; /* if we're transitioning between slots and fake touches in either ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] Read the horizontal wheel click angle property if available
Hi, On 16-08-16 07:18, Peter Hutterer wrote: The Logitech MX master has different click angles for the two wheels. https://github.com/systemd/systemd/issues/3947 Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Patch looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- src/evdev-tablet.c | 2 +- src/evdev.c | 61 +++- src/evdev.h | 2 +- src/libinput-private.h | 5 +++ test/litest-device-mouse-wheel-click-angle.c | 1 + test/pointer.c | 11 +++-- 6 files changed, 56 insertions(+), 26 deletions(-) diff --git a/src/evdev-tablet.c b/src/evdev-tablet.c index 8f24555..b02bf7a 100644 --- a/src/evdev-tablet.c +++ b/src/evdev-tablet.c @@ -334,7 +334,7 @@ normalize_wheel(struct tablet_dispatch *tablet, { struct evdev_device *device = tablet->device; - return value * device->scroll.wheel_click_angle; + return value * device->scroll.wheel_click_angle.x; } static inline void diff --git a/src/evdev.c b/src/evdev.c index e906a50..e8db238 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -805,7 +805,7 @@ fallback_process_relative(struct fallback_dispatch *dispatch, case REL_WHEEL: fallback_flush_pending_event(dispatch, device, time); wheel_degrees.y = -1 * e->value * - device->scroll.wheel_click_angle; + device->scroll.wheel_click_angle.x; discrete.y = -1 * e->value; evdev_notify_axis( device, @@ -817,7 +817,8 @@ fallback_process_relative(struct fallback_dispatch *dispatch, break; case REL_HWHEEL: fallback_flush_pending_event(dispatch, device, time); - wheel_degrees.x = e->value * device->scroll.wheel_click_angle; + wheel_degrees.x = e->value * + device->scroll.wheel_click_angle.y; discrete.x = e->value; evdev_notify_axis( device, @@ -1815,27 +1816,47 @@ evdev_device_init_pointer_acceleration(struct evdev_device *device, } } -static inline int -evdev_read_wheel_click_prop(struct evdev_device *device) +static inline bool +evdev_read_wheel_click_prop(struct evdev_device *device, + const char *prop, + int *angle) { - const char *prop; - int angle = DEFAULT_WHEEL_CLICK_ANGLE; + int val; - prop = udev_device_get_property_value(device->udev_device, - "MOUSE_WHEEL_CLICK_ANGLE"); - if (prop) { - angle = parse_mouse_wheel_click_angle_property(prop); - if (!angle) { - log_error(evdev_libinput_context(device), - "Mouse wheel click angle '%s' is present but invalid," - "using %d degrees instead\n", - device->devname, - DEFAULT_WHEEL_CLICK_ANGLE); - angle = DEFAULT_WHEEL_CLICK_ANGLE; - } + *angle = DEFAULT_WHEEL_CLICK_ANGLE; + prop = udev_device_get_property_value(device->udev_device, prop); + if (!prop) + return false; + + val = parse_mouse_wheel_click_angle_property(prop); + if (angle) { + *angle = val; + return true; } - return angle; + log_error(evdev_libinput_context(device), + "Mouse wheel click angle '%s' is present but invalid," + "using %d degrees instead\n", + device->devname, + DEFAULT_WHEEL_CLICK_ANGLE); + + return false; +} + +static inline struct wheel_angle +evdev_read_wheel_click_props(struct evdev_device *device) +{ + struct wheel_angle angles; + + evdev_read_wheel_click_prop(device, + "MOUSE_WHEEL_CLICK_ANGLE", + ); + if (!evdev_read_wheel_click_prop(device, +"MOUSE_WHEEL_CLICK_ANGLE_HORIZ", +)) + angles.y = angles.x; + + return angles; } static inline int @@ -2550,7 +2571,7 @@ evdev_device_create(struct libinput_seat *seat, device->scroll.direction_lock_threshold = 5.0; /* Default may be overridden */ device->scroll.direction = 0; device->scroll.wheel_click_angle = - evdev_read_wheel_click_prop(device); + evdev_read_wheel_click_props(device); d
Re: [PATCH libinput 1/3] Add configurable button map to tappings
Hi, On 22-07-16 04:46, Peter Hutterer wrote: The previously hardcoded button map for tapping is 1/2/3 to LRM. But the middle button is a common feature on the desktop (used for paste, most prominently) and three-finger tapping is almost impossible to do reliably on some touchpads (e.g. the T440 has a recognition rate of ~1 in 5). Left and right buttons have a prominent physical position (either softbuttons or physical buttons) so make the tap order configurable. Those that require middle buttons reliably can use the [software] buttons for left/right and 2-finger tap for a middle button. https://bugs.freedesktop.org/show_bug.cgi?id=96962 Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Series looks good to me: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- src/evdev-mt-touchpad-tap.c | 22 src/libinput-private.h | 5 +++ src/libinput.c | 36 src/libinput.h | 82 + src/libinput.sym| 6 tools/shared.c | 21 tools/shared.h | 1 + 7 files changed, 173 insertions(+) diff --git a/src/evdev-mt-touchpad-tap.c b/src/evdev-mt-touchpad-tap.c index c2b331c..3ca4f95 100644 --- a/src/evdev-mt-touchpad-tap.c +++ b/src/evdev-mt-touchpad-tap.c @@ -935,6 +935,25 @@ tp_tap_config_get_default(struct libinput_device *device) } static enum libinput_config_status +tp_tap_config_set_map(struct libinput_device *device, + enum libinput_config_tap_button_map map) +{ + return LIBINPUT_CONFIG_STATUS_UNSUPPORTED; +} + +static enum libinput_config_tap_button_map +tp_tap_config_get_map(struct libinput_device *device) +{ + return LIBINPUT_CONFIG_TAP_MAP_LRM; +} + +static enum libinput_config_tap_button_map +tp_tap_config_get_default_map(struct libinput_device *device) +{ + return LIBINPUT_CONFIG_TAP_MAP_LRM; +} + +static enum libinput_config_status tp_tap_config_set_drag_enabled(struct libinput_device *device, enum libinput_config_drag_state enabled) { @@ -1017,6 +1036,9 @@ tp_init_tap(struct tp_dispatch *tp) tp->tap.config.set_enabled = tp_tap_config_set_enabled; tp->tap.config.get_enabled = tp_tap_config_is_enabled; tp->tap.config.get_default = tp_tap_config_get_default; + tp->tap.config.set_map = tp_tap_config_set_map; + tp->tap.config.get_map = tp_tap_config_get_map; + tp->tap.config.get_default_map = tp_tap_config_get_default_map; tp->tap.config.set_drag_enabled = tp_tap_config_set_drag_enabled; tp->tap.config.get_drag_enabled = tp_tap_config_get_drag_enabled; tp->tap.config.get_default_drag_enabled = tp_tap_config_get_default_drag_enabled; diff --git a/src/libinput-private.h b/src/libinput-private.h index 472f6f3..1f27af6 100644 --- a/src/libinput-private.h +++ b/src/libinput-private.h @@ -158,6 +158,11 @@ struct libinput_device_config_tap { enum libinput_config_tap_state (*get_enabled)(struct libinput_device *device); enum libinput_config_tap_state (*get_default)(struct libinput_device *device); + enum libinput_config_status (*set_map)(struct libinput_device *device, + enum libinput_config_tap_button_map map); + enum libinput_config_tap_button_map (*get_map)(struct libinput_device *device); + enum libinput_config_tap_button_map (*get_default_map)(struct libinput_device *device); + enum libinput_config_status (*set_drag_enabled)(struct libinput_device *device, enum libinput_config_drag_state); enum libinput_config_drag_state (*get_drag_enabled)(struct libinput_device *device); diff --git a/src/libinput.c b/src/libinput.c index a8240bd..6958042 100644 --- a/src/libinput.c +++ b/src/libinput.c @@ -3352,6 +3352,42 @@ libinput_device_config_tap_get_default_enabled(struct libinput_device *device) } LIBINPUT_EXPORT enum libinput_config_status +libinput_device_config_tap_set_button_map(struct libinput_device *device, + enum libinput_config_tap_button_map map) +{ + switch (map) { + case LIBINPUT_CONFIG_TAP_MAP_LRM: + case LIBINPUT_CONFIG_TAP_MAP_LMR: + break; + default: + return LIBINPUT_CONFIG_STATUS_INVALID; + } + + if (libinput_device_config_tap_get_finger_count(device) == 0) + return LIBINPUT_CONFIG_STATUS_UNSUPPORTED; + + return device->config.tap->set_map(device, map); +} + +LIBINPUT_EXPORT enum libinput_config_tap_button_map +libinput_device_config_tap_get_button_map(struct libinput_device *device) +{ + if (libinput_device_config_tap_get_finger_count(device) == 0) + return LIBINPUT_CONFIG_TAP_MAP_LRM; + + return device->co
Re: [PATCH libinput] evdev: recognize and use ID_INPUT_TRACKBALL
Hi, On 03-08-16 23:57, Peter Hutterer wrote: We leave the old LIBINPUT_MODEL_TRACKBALL in place until we can rely on systems to have the new systemd tagging. https://github.com/systemd/systemd/pull/3872 Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> LGTM: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans --- src/evdev.c | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/evdev.c b/src/evdev.c index ed581b4..9a0b991 100644 --- a/src/evdev.c +++ b/src/evdev.c @@ -67,6 +67,7 @@ enum evdev_device_udev_tags { EVDEV_UDEV_TAG_ACCELEROMETER = (1 << 7), EVDEV_UDEV_TAG_TABLET_PAD = (1 << 8), EVDEV_UDEV_TAG_POINTINGSTICK = (1 << 9), +EVDEV_UDEV_TAG_TRACKBALL = (1 << 10), }; struct evdev_udev_tag_match { @@ -86,6 +87,7 @@ static const struct evdev_udev_tag_match evdev_udev_tag_matches[] = { {"ID_INPUT_JOYSTICK", EVDEV_UDEV_TAG_JOYSTICK}, {"ID_INPUT_ACCELEROMETER",EVDEV_UDEV_TAG_ACCELEROMETER}, {"ID_INPUT_POINTINGSTICK",EVDEV_UDEV_TAG_POINTINGSTICK}, + {"ID_INPUT_TRACKBALL",EVDEV_UDEV_TAG_TRACKBALL}, /* sentinel value */ { 0 }, @@ -1797,8 +1799,9 @@ evdev_read_model_flags(struct evdev_device *device) MODEL(DELL_TOUCHPAD), MODEL(TRACKBALL), MODEL(APPLE_MAGICMOUSE), - { NULL, EVDEV_MODEL_DEFAULT }, #undef MODEL + { "ID_INPUT_TRACKBALL", EVDEV_MODEL_TRACKBALL }, + { NULL, EVDEV_MODEL_DEFAULT }, }; const struct model_map *m = model_map; uint32_t model_flags = 0; @@ -2146,7 +2149,7 @@ evdev_configure_device(struct evdev_device *device) } log_info(libinput, -"input device '%s', %s is tagged by udev as:%s%s%s%s%s%s%s%s%s\n", +"input device '%s', %s is tagged by udev as:%s%s%s%s%s%s%s%s%s%s\n", device->devname, devnode, udev_tags & EVDEV_UDEV_TAG_KEYBOARD ? " Keyboard" : "", udev_tags & EVDEV_UDEV_TAG_MOUSE ? " Mouse" : "", @@ -2156,7 +2159,8 @@ evdev_configure_device(struct evdev_device *device) udev_tags & EVDEV_UDEV_TAG_POINTINGSTICK ? " Pointingstick" : "", udev_tags & EVDEV_UDEV_TAG_JOYSTICK ? " Joystick" : "", udev_tags & EVDEV_UDEV_TAG_ACCELEROMETER ? " Accelerometer" : "", -udev_tags & EVDEV_UDEV_TAG_TABLET_PAD ? " TabletPad" : ""); +udev_tags & EVDEV_UDEV_TAG_TABLET_PAD ? " TabletPad" : "", +udev_tags & EVDEV_UDEV_TAG_TRACKBALL ? " Trackball" : ""); if (udev_tags & EVDEV_UDEV_TAG_ACCELEROMETER) { log_info(libinput, ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput] gestures: reduce the 2fg scroll timeout to 150ms
Hi, On 04-08-16 07:07, Peter Hutterer wrote: On Wed, Aug 03, 2016 at 01:06:01PM +0200, Hans de Goede wrote: Hi, On 03-08-16 11:36, Peter Hutterer wrote: This timeout is there to switch to scrolling when the fingers rest on the touchpad unmoving and thus avoids the initial scroll threshold for slow scrolls. Since the only other gestures we support are swipe (usually a fast movement) and pinch-and-rotate (also a fast movement) we can drop the timeout down significantly and thus make the scroll feel more reactive. https://bugs.freedesktop.org/show_bug.cgi?id=93504 Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net> Sounds like a good idea to me, but as Martin already pointed out the commit subject and the actual change do not match up. whoops. this should've been 150ms everywhere. I'll change this locally, testing confirms 150ms still works as expected. Ok, with things fixed to be 150ms everywhere this patch is: Reviewed-by: Hans de Goede <hdego...@redhat.com> Regards, Hans Cheers, Peter --- src/evdev-mt-touchpad-gestures.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/evdev-mt-touchpad-gestures.c b/src/evdev-mt-touchpad-gestures.c index acfc875..c965e11 100644 --- a/src/evdev-mt-touchpad-gestures.c +++ b/src/evdev-mt-touchpad-gestures.c @@ -30,7 +30,7 @@ #include "evdev-mt-touchpad.h" #define DEFAULT_GESTURE_SWITCH_TIMEOUT ms2us(100) -#define DEFAULT_GESTURE_2FG_SCROLL_TIMEOUT ms2us(500) +#define DEFAULT_GESTURE_2FG_SCROLL_TIMEOUT ms2us(200) static inline const char* gesture_state_to_str(enum tp_gesture_state state) ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel