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 hdegoede@redhat.com wrote:
On Thursday, April 7th, 2022 at 17:38, Hans de Goede hdegoede@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.
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] = 65535Right, 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