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 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. >>
Re: [RFC] drm/kms: control display brightness through drm_connector properties
On Friday, April 29th, 2022 at 10:55, Hans de Goede wrote: > I believe that we can fix the new interface, the plan is for there > to be some helper code to proxy the new connector properties to what > is still a good old backlight-device internally in the kernel,. > > This proxy-ing code could take a minimum value below which it should > not go when things are set through the properties and then if e.g. > the /sys/class/backlight interface offers range of 0-65535 and the > kms driver asks the proxying helper for a minimum of 500, show this > as 0-65035 on the property, simply adding 500 before sending the > value to the backlight-device on writes (and subtracting 500 on reads, > clamping to 0 as lowest value reported on reads). > > This way apps using the new API can never go below 500 (in this > example) and for old API users nothing changes. > > Given that Jani seems to be in favor of enforcing some minimal value > inside the i915 code going forward and also what Alex said that the > amdgpu code already enforces its own minimum if the video BIOS tables > don't provide one, it seems that there is consensus that we want 0 > to mean minimum brightness at which the screen is still somewhat > readable and that we want to enforce this at the kernel level. > > Which also means the weird hint property which I came up with won't > be necessary as we now have a clean definition of what brightness > 0 is supposed to mean (in the new API) and any cases where this is not > the case are kernel bugs and should be fixed in the kernel. Looks like a good approach to me from user-space PoV!
Re: [RFC] drm/kms: control display brightness through drm_connector properties
On Fri, 29 Apr 2022 08:59:24 + Simon Ser wrote: > On Friday, April 29th, 2022 at 10:55, Hans de Goede > wrote: > > > I believe that we can fix the new interface, the plan is for there > > to be some helper code to proxy the new connector properties to what > > is still a good old backlight-device internally in the kernel,. > > > > This proxy-ing code could take a minimum value below which it should > > not go when things are set through the properties and then if e.g. > > the /sys/class/backlight interface offers range of 0-65535 and the > > kms driver asks the proxying helper for a minimum of 500, show this > > as 0-65035 on the property, simply adding 500 before sending the > > value to the backlight-device on writes (and subtracting 500 on reads, > > clamping to 0 as lowest value reported on reads). > > > > This way apps using the new API can never go below 500 (in this > > example) and for old API users nothing changes. > > > > Given that Jani seems to be in favor of enforcing some minimal value > > inside the i915 code going forward and also what Alex said that the > > amdgpu code already enforces its own minimum if the video BIOS tables > > don't provide one, it seems that there is consensus that we want 0 > > to mean minimum brightness at which the screen is still somewhat > > readable and that we want to enforce this at the kernel level. > > > > Which also means the weird hint property which I came up with won't > > be necessary as we now have a clean definition of what brightness > > 0 is supposed to mean (in the new API) and any cases where this is not > > the case are kernel bugs and should be fixed in the kernel. > > Looks like a good approach to me from user-space PoV! Yes! Thanks, pq pgpJda7V6CfSE.pgp Description: OpenPGP digital signature
RE: [RFC] drm/kms: control display brightness through drm_connector properties
Yes we would need this. -Sameer -Original Message- From: wayland-devel On Behalf Of Pekka Paalanen Sent: Friday, April 29, 2022 2:37 PM To: Hans de Goede Cc: Jani Nikula ; Sebastian Wick ; Martin Roukala ; Christoph Grenz ; wayland ; dri-de...@lists.freedesktop.org; Daniel Vetter ; Alex Deucher ; Yusuf Khan Subject: Re: [RFC] drm/kms: control display brightness through drm_connector properties On Fri, 29 Apr 2022 08:59:24 + Simon Ser wrote: > On Friday, April 29th, 2022 at 10:55, Hans de Goede > wrote: > > > I believe that we can fix the new interface, the plan is for there > > to be some helper code to proxy the new connector properties to what > > is still a good old backlight-device internally in the kernel,. > > > > This proxy-ing code could take a minimum value below which it should > > not go when things are set through the properties and then if e.g. > > the /sys/class/backlight interface offers range of 0-65535 and the > > kms driver asks the proxying helper for a minimum of 500, show this > > as 0-65035 on the property, simply adding 500 before sending the > > value to the backlight-device on writes (and subtracting 500 on > > reads, clamping to 0 as lowest value reported on reads). > > > > This way apps using the new API can never go below 500 (in this > > example) and for old API users nothing changes. > > > > Given that Jani seems to be in favor of enforcing some minimal value > > inside the i915 code going forward and also what Alex said that the > > amdgpu code already enforces its own minimum if the video BIOS > > tables don't provide one, it seems that there is consensus that we > > want 0 to mean minimum brightness at which the screen is still > > somewhat readable and that we want to enforce this at the kernel level. > > > > Which also means the weird hint property which I came up with won't > > be necessary as we now have a clean definition of what brightness > > 0 is supposed to mean (in the new API) and any cases where this is > > not the case are kernel bugs and should be fixed in the kernel. > > Looks like a good approach to me from user-space PoV! Yes! Thanks, pq
