Re: [RFC] drm/kms: control display brightness through drm_connector properties
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 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 accide
Re: [RFC] drm/kms: control display brightness through drm_connector properties
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 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
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 GPU driver offers a GPU native backlight device use that. 4. Use the ACPI video extensions backlight device > 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. The problem here is 2. or IOW devices which don't support the ACPI video extensions, these typically (always?) also don't offer a GPU native backlight device, instead relying on the embedded-controller for backlight control using some vendor specific firmware API to talk to the EC. For the other cases there are indeed some gaps which I plan to close so that we can make sure that the backlight device will
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 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 GPU driver offers a GPU native backlight device use that. > 4. Use the ACPI video extensions backlight device > >> 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. > > The problem here is 2. or IOW devices which don't support the > ACPI video extensions, these typically (always?) also don't offer > a GPU native backlight device, instead relying on > the embedded-controller for backlight control using
Re: [RFC] drm/kms: control display brightness through drm_connector properties
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.)
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 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 GPU driver offers a GPU native backlight device use that. > 4. Use the ACPI video extensions backlight device > >> 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. > > The problem here is 2. or IOW devices which don't support the > ACPI video extensions, these typically (always?) also don't offer > a GPU native backlight device, instead relying on > the embedded-controller for backlight control using
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: Touchscreen calibration matrix changes
Persistent storage was never the problem. Issue was getting the
non-default values in use, but it has now been sorted.
-TeemuK
On Wed, Apr 6, 2022 at 7:58 AM Peter Hutterer wrote:
>
> On Wed, Apr 06, 2022 at 06:02:22AM +0300, Teemu K wrote:
> > On Tue, Apr 5, 2022 at 5:21 PM Pekka Paalanen wrote:
> > >
> > > On Tue, 29 Mar 2022 15:31:59 +0300
> > > Teemu K wrote:
> > >
> > > > Hi,
> > > >
> > > > I'm trying to get touch calibration working on Wayland/Weston for
> > > > resistive touch. I can get the calibration points and calculate
> > > > calibration matrix values, but when I call
> > > > libinput_device_config_calibration_set_matrix and then read it back it
> > > > would seem that the matrix is changed, but the actual touchscreen does
> > > > not indicate so. Ie. if I calibrate the touch clearly wrong it still
> > > > behaves like it would have been calibrated perfectly.
> > >
> > > Hi,
> > >
> > > what exactly do you mean by "when I call
> > > libinput_device_config_calibration_set_matrix", what is "I" there?
> > By "I" I mean the application.
> >
> > > > If I set the same values to udev rule like this:
> > > > ATTRS{name}=="tsc2007", ENV{LIBINPUT_CALIBRATION_MATRIX}="1.29342,
> > > > -0.0223598, -0.135092, -0.0272545, -1.11435, 1.04368"
> > > >
> > > > They get taken in use and I can read those values as default values
> > > > with libinput_device_config_calibration_get_default_matrix - function.
> > >
> > > That's right.
> > >
> > > > Is there something else that needs to be done to get the updated
> > > > matrix taken in use or is there known issue with Wayland/Weston given
> > > > that it is quite old version Wayland 1.17.0/Weston 6.0.1.
> > > > Unfortunately I'm stuck with those versions for now.
> > >
> > > I'm not sure what problem you have. The udev rule works, right?
> > Yeah, but the problem was replacing those defaults with the calibrated
> > values with that ibinput_device_config_calibration_set_matrix -
> > function.
> >
> > > Or do you mean updating the calibration matrix while Weston is already
> > > running?
> > >
> > > Did you write a new program that simply calls
> > > libinput_device_config_calibration_set_matrix() and then expect Weston
> > > to pick that up when you run it?
> > Weston is already running and I'm trying to set a new matrix in use
> > for Weston. Is that not what the
> > ibinput_device_config_calibration_set_matrix - function is meant for?
> >
> > > If so, then libinput just does not work that way. There is no persistent
> > > configuration in libinput, and any setting you make is local in the
> > > in-memory libinput data structure. The udev rules are the persistent
> > > configuration. That's why e.g. calibration_helper option exists in
> > > weston.ini, so that you could have your own way of storing the
> > > calibration in udev rules.
> > This is not the issue. I'm very well aware that there is no persistent
> > storage in libinput and I'm not trying to use it as such. I'm trying
> > to set a new matrix when Weston is already running.
>
> The issue here is less whether libinput has persistent storage, the issue is
> that you do not have access to the libinput context that the compositor uses.
> https://wayland.freedesktop.org/libinput/doc/latest/faqs.html#can-i-write-a-program-to-make-libinput-do-foo
>
> The only way around this is to use compositor-provided knobs that *the
> compositor* then converts into libinput configuration calls. Like the weston
> touch protocol that you have found now.
>
> Cheers,
> Peter
>
> >
> > But I ended up copying the implementation from weston-touch-calibrator
> > as you mentioned instead and it worked so this issue is solved.
> >
> > Thanks.
> >
> > -TeemuK
> >
> > > It looks like clients/touch-calibrator.c existed already in Weston
> > > 6.0.0. It's a ready-made calibration tool that computes
> > > LIBINPUT_CALIBRATION_MATRIX values, and uploads those to Weston. You
> > > need weston.ini option touchscreen_calibrator=true to make it work, and
> > > the calibration_helper option to save the calibration persistent. These
> > > are described in the weston.ini man page, and there is an example
> > > script at
> > > https://gitlab.freedesktop.org/wayland/weston/-/blob/main/doc/scripts/calibration-helper.bash
> > >
> > > The option touchscreen_calibrator=true exposes a Wayland extension that
> > > allows both getting raw touch coordinates and uploading a new
> > > calibration matrix to Weston. This is the only way to update the matrix
> > > without restarting Weston.
> > >
> > >
> > > Thanks,
> > > pq
Re: [RFC] drm/kms: control display brightness through drm_connector properties
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 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
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 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 tr
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 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
Re: [RFC] drm/kms: control display brightness through drm_connector properties
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" 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 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 du
