On Mon, 11 Apr 2022, Alex Deucher <alexdeuc...@gmail.com> wrote: > On Mon, Apr 11, 2022 at 6:18 AM Hans de Goede <hdego...@redhat.com> wrote: >> >> Hi, >> >> On 4/8/22 17:11, Alex Deucher wrote: >> > On Fri, Apr 8, 2022 at 10:56 AM Hans de Goede <hdego...@redhat.com> wrote: >> >> >> >> Hi, >> >> >> >> On 4/8/22 16:08, Alex Deucher wrote: >> >>> On Fri, Apr 8, 2022 at 4:07 AM Daniel Vetter <dan...@ffwll.ch> 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 <hdego...@redhat.com> >> >>>>> 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 >> >>>>>>> <hdego...@redhat.com> 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 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. >> >>>>> >> >>>>> Why not just make it policy that 0 is defined as minimum brightness, >> >>>>> not off, and have all drivers conform to that? >> >>>> >> >>>> Because the backlight subsystem isn't as consistent on this, and it's >> >>>> been >> >>>> an epic source of confusion since forever. >> >>>> >> >>>> What's worse, there's both userspace out there which assumes brightness >> >>>> = >> >>>> 0 is a really fast dpms off _and_ userspace that assumes that >> >>>> brightness = >> >>>> 0 is the lowest setting. Of course on different sets of machines. >> >>>> >> >>>> So yeah we're screwed. I have no idea how to get out of this. >> >>> >> >>> Yes, but this is a new API. So can't we do better? Sure the old >> >>> backlight interface is broken, but why carry around clunky workarounds >> >>> for new interfaces? >> >> >> >> Right we certainly need to define the behavior of the new API >> >> clearly, so that userspace does not misuse / misinterpret it. >> >> >> >> The intend is for brightness=0 to mean minimum brightness >> >> to still be able to see what is on the screen. But the problem >> >> is that in many cases the GPU driver directly controls a PWM >> >> output, no minimum PWM value is given in the video BIOS tables >> >> and actually setting the PWM to 0% dutycycle turns off the >> >> screen. >> > >> > Sure. So have the GPU driver map 0 to some valid minimum if that is >> > the case or might be the case. If bugs come up, we can add quirks in >> > the GPU drivers. >> >> The problem is that when 0% duty-cycle is not off, but minimum >> brightness because there is some smart backlight-controller involved >> downstream of the pwm, then of we limit it to say min 5% then we >> have just limited the range of the brightness. GNOME already does >> this in userspace now and it is already receiving bug-reports >> from users that GNOME does not allow the brightness to go as low >> as they like to have it in a dark(ish) room. >> >> And in some cases 5% is likely not enough for the backlight to >> actually turn on. So it will be wrong in one direction on some >> devices and wrong in the other direction in other devices. >> >> Which means that to satisfy everyone here we will need a ton >> of quirks, much too many to maintain in the kernel IMHO. >> >> >> >> So we can only promise a best-effort to make brightness=0 >> >> mean minimum brightness, combined with documenting that it >> >> may turn off the backlight and that userspace _must_ never >> >> depend on it turning off the backlight. >> >> >> >> Also note that setting a direct PWM output to duty-cycle 0% >> >> does not guarantee that the backlight goes off, this may be >> >> an input for a special backlight LED driver IC like the >> >> TI LP855x series which can have an internal lookup >> >> table causing it to actually output a minimum brightness >> >> when its PWM input is at 0% dutycycle. So this is a case >> >> where we just don't get enough info from the fw/hw to be able >> >> to offer the guarantees which we would like to guarantee. >> > >> > 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. > > We have similar issues with AMD platforms. Some platforms don't > populate the min value tables, but in the driver we set the minimum > safe value as the default min value when that happens. It may not > always go as low as the platform may be capable of, but at least we > have consistent behavior and it's all controlled in one place. > >> >> 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. > > So why not do it in the kernel? At least that way everyone will get > it the fixes as they happen. A big user database may or may not > happen and behavior will be inconsistent across desktop environments > until that does. I don't really see any value in having the flag. > There will be cases where the flag is wrong and will require kernel > fixes anyway (e.g., OEM switches panel due to supply chain issues and > forgets to update the vbios, etc.), so why not just define 0 as > minimum safe backlight value? If it's too low and flickers or turns > the backlight off, we quirk it. If a particular platform can go > lower, we can quirk it. If we add the flag then we need to not only > add quirks for the minimum value, but we also have to deal with quirks > for when the flag is set wrong. So now we are maintaining two quirks > instead of one.
Just chiming in, there are certainly plenty of panels and designs where 0 PWM duty cycle is physically not possible, and thus 0 brightness simply can't universally mean off. Daniel referred to a case where 0 brightness was used as fast mini dpms off, and I think it's fundamentally a broken use case. We can't guarantee to be able to support that. I think the appeal was partly in being able to do it without access to kms api, quick and dirty via sysfs. Please let's just make 0 the minimum but not off. If you want off, you do modeset, and the driver can follow panel timings etc. I think that's also something the kernel can actually guarantee, while we can't guarantee 0 is off. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
