Re: [Intel-gfx] [RFC PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness
On Tue, 20 May 2014, Jesse Barnes wrote:
> On Tue, 29 Apr 2014 23:30:49 +0300
> Jani Nikula wrote:
>
>> Historically we've exposed the full backlight PWM duty cycle range to
>> the userspace, in the name of "mechanism, not policy". However, it turns
>> out there are both panels and board designs where there is a minimum
>> duty cycle that is required for proper operation. The minimum duty cycle
>> is available in the VBT.
>>
>> The backlight class sysfs interface does not make any promises to the
>> userspace about the physical meaning of the range
>> 0..max_brightness. Specifically there is no guarantee that 0 means off;
>> indeed for acpi_backlight 0 usually is not off, but the minimum
>> acceptable value.
>>
>> Respect the minimum backlight, and expose the range acceptable to the
>> hardware as 0..max_brightness to the userspace via the backlight class
>> device; 0 means the minimum acceptable enabled value. To switch off the
>> backlight, the user must disable the encoder.
>>
>> As a side effect, make the backlight class device max brightness and
>> physical PWM modulation frequency (i.e. max duty cycle) independent.
>>
>> Signed-off-by: Jani Nikula
>
> I like the direction here... I wonder if we should always virtualize
> the max too, and just always expose 0-2047 or something.
IIUC Windows 8 assumes 101 levels, 0..100 inclusive. (Curiously I didn't
find mention what 0 means there.)
The higher the backlight modulation frequency, the less flicker, and the
higher the range we expose - but, as far as I've understood, fewer
perceivable physical brightness levels. The panel just won't follow the
signal fast enough. So we could just as well expose a smaller range, and
it shouldn't make a difference.
But I'd like that to be a follow-up later on when the dust has settled.
>> @@ -965,6 +1008,18 @@ static void intel_backlight_device_unregister(struct
>> intel_connector *connector)
>> * XXX: Query mode clock or hardware clock and program PWM modulation
>> frequency
>> * appropriately when it's 0. Use VBT and/or sane defaults.
>> */
>> +static inline u32 get_backlight_min(struct intel_connector *connector)
>> +{
>> +struct drm_device *dev = connector->base.dev;
>> +struct drm_i915_private *dev_priv = dev->dev_private;
>> +struct intel_panel *panel = &connector->panel;
>> +
>> +BUG_ON(panel->backlight.max == 0);
>> +
>> +return dev_priv->vbt.backlight.min_brightness *
>> +panel->backlight.max / 255;
>> +}
>
> Is this the user version or the hw version? If hw, why not just use
> min_brightness directly?
My understanding is that the VBT value is a coefficient
(min_brightness/255) of the hw max to get the hw value. I may be wrong
as the spec sucks. 255 as the biggest absolute hw min value sounds
awfully small to me though.
BR,
Jani.
>
> --
> Jesse Barnes, Intel Open Source Technology Center
--
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness
On Wed, 2014-06-04 at 08:04 -0700, Stéphane Marchesin wrote: > Well, for example ACPI backlight isn't the default on intel, instead > i915 implements its own backlight. ACPI backlight doesn't support as > many backlight levels as the native backlight for example. So I do > think that ACPI backlight is inferior. ACPI backlight is the default on most pre-Windows 8 i915 systems. Userspace which assumes that backlight value 0 has any specific meaning is broken userspace. Adding an additional interface that allows disabling the backlight without powering down the panel seems like a completely reasonable thing to do, though. -- Matthew Garrett ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness
On Wed, 4 Jun 2014 08:04:53 -0700 Stéphane Marchesin wrote: > > We could have dpms standby mean backlight off, everything else on, > > similarly to what you want by 0 backlight meaning off. So you could > > switch between dpms on/standby more quickly. The point in that is that > > it's the right API for doing panel power sequencing. > > > > You see, even with defining backlight 0 = off, we'd have to respect the > > panel sequencing and delays for backlight (admittedly we currently don't > > do that, which is why it's fast - and broken). > > There are multiple delays at play here, and typically the longest > panel delays have nothing to do with the backlight but they are the > delays for turning the panel on/off (usually the panel on/off delays > are much bigger, up to 500ms on some panels). These are the delays I'm > trying to avoid. > > > Doing the panel > > sequencing properly from the backlight interface gets *much* harder > > because it's all backwards in the stack then. > > If you implement some interface which does "set backlight to 0 on > platforms which support it, and do panel off on other platforms", > that's fine by me; however I don't want to regress all the other > platforms just because BYT has issues. In the current state of things, > the functionality set which is made available to user space happens to > shrink with your patch. According to the LVDS and eDP timing specs, we *ought* to be able to disable the backlight and then the PWM source and leave everything else alone, as long as we apply the appropriate delays between backlight off and PWM off, and then again when we turn them back on (PWM first, delay, then backlight on). IIRC the issue on BYT was that we saw the panel power line go down when we disabled the backlight and PWM which would obviously cause all sorts of problems. But that could have been user error on the scope, so we should confirm that before baking a full DPMS cycle into the BYT implementation or the interface in general. -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness
On Wed, Jun 4, 2014 at 2:11 AM, Jani Nikula wrote: > On Wed, 04 Jun 2014, Stéphane Marchesin wrote: >> On Tue, Jun 3, 2014 at 1:26 PM, Daniel Vetter wrote: >>> On Tue, Jun 3, 2014 at 6:40 PM, Stéphane Marchesin >>> wrote: On Tue, Apr 29, 2014 at 1:30 PM, Jani Nikula wrote: > Historically we've exposed the full backlight PWM duty cycle range to > the userspace, in the name of "mechanism, not policy". However, it turns > out there are both panels and board designs where there is a minimum > duty cycle that is required for proper operation. The minimum duty cycle > is available in the VBT. > > The backlight class sysfs interface does not make any promises to the > userspace about the physical meaning of the range > 0..max_brightness. Specifically there is no guarantee that 0 means off; > indeed for acpi_backlight 0 usually is not off, but the minimum > acceptable value. This is the part we've been relying on in Chrome OS (we assume that 0 means off). It seems to me that i915 would be the first, or one of the first drivers to violate this rule (in particular you can find a lot of backlight drivers trying hard to ensure that 0 means off in the backlight drivers directory). For context, we use backlight = 0 as a quick "turn the panel off" function where possible. This allows us to avoid the panel off delay where possible. Breaking this assumption means that we'd pay the panel off delay penalty everywhere, not just with BYT. >>> >>> Well kde is the opposite and I've had users yelling at me that they >>> can't use their system, since acpi pretty much always leaves the >>> backlight on when set to 0. And acpi kinda rules on intel platforms. >>> So I think I'll merge this one since black screens trumps a slight >>> functional degration and we didn't duct-tape over the kde assumptions >>> either. Essentially you can't assume anything about backlight values >>> besides that bigger values tends to mean brigther. Linearity, absolute >>> values and endpoints are kinda all off. >> >> It seems fairly wrong to model the backlight behavior after the only >> thing everyone agrees is broken (ACPI). > > Please don't put words into everyone's mouth. > > Talking to folks on IRC, Dave Airlie said, "I think its been disagreed > over the years to be implementation dependent." and Matthew Garrett > said, "It's currently implementation dependent. It can never mean off > under all circumstances (not all interfaces offer a way to turn it > off)." > > I don't know whether they think ACPI is broken, but clearly there's no > one true answer, and I think it's not the correct approach for userspace > to assume 0 means off. Well, for example ACPI backlight isn't the default on intel, instead i915 implements its own backlight. ACPI backlight doesn't support as many backlight levels as the native backlight for example. So I do think that ACPI backlight is inferior. > >> If you go through the backlight drivers, (at least all the ones I've >> looked at) ensure that 0 really means off. > > And systemd goes out of it's way to not switch backlight off on those > interfaces... and it's obvious they can't assume anything about the > meaning of 0 either: > > http://cgit.freedesktop.org/systemd/systemd/commit/?id=7b909d7407965c03caaba30daae7aee113627a83 This seems unrelated, they just turn the backlight to non-zero to make things visible? > >>> If we want to specifically expose an intermediate "panel off" level >>> because the full link training is too expensive we imo should do this >>> as an intermediate dpms level, e.g. dpms standby. >> >> Training isn't the problem, the panel delays are the problem here (and >> not something we can work around because it's part of the panel >> specifications which we have to follow). Using dpms wouldn't solve >> anything. > > We could have dpms standby mean backlight off, everything else on, > similarly to what you want by 0 backlight meaning off. So you could > switch between dpms on/standby more quickly. The point in that is that > it's the right API for doing panel power sequencing. > > You see, even with defining backlight 0 = off, we'd have to respect the > panel sequencing and delays for backlight (admittedly we currently don't > do that, which is why it's fast - and broken). There are multiple delays at play here, and typically the longest panel delays have nothing to do with the backlight but they are the delays for turning the panel on/off (usually the panel on/off delays are much bigger, up to 500ms on some panels). These are the delays I'm trying to avoid. > Doing the panel > sequencing properly from the backlight interface gets *much* harder > because it's all backwards in the stack then. If you implement some interface which does "set backlight to 0 on platforms which support it, and do panel off on other platforms", that's fine by me; however I don't want to regress all the other platfor
Re: [Intel-gfx] [RFC PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness
On Wed, 04 Jun 2014, Stéphane Marchesin wrote: > On Tue, Jun 3, 2014 at 1:26 PM, Daniel Vetter wrote: >> On Tue, Jun 3, 2014 at 6:40 PM, Stéphane Marchesin >> wrote: >>> On Tue, Apr 29, 2014 at 1:30 PM, Jani Nikula wrote: Historically we've exposed the full backlight PWM duty cycle range to the userspace, in the name of "mechanism, not policy". However, it turns out there are both panels and board designs where there is a minimum duty cycle that is required for proper operation. The minimum duty cycle is available in the VBT. The backlight class sysfs interface does not make any promises to the userspace about the physical meaning of the range 0..max_brightness. Specifically there is no guarantee that 0 means off; indeed for acpi_backlight 0 usually is not off, but the minimum acceptable value. >>> >>> This is the part we've been relying on in Chrome OS (we assume that 0 >>> means off). It seems to me that i915 would be the first, or one of the >>> first drivers to violate this rule (in particular you can find a lot >>> of backlight drivers trying hard to ensure that 0 means off in the >>> backlight drivers directory). >>> >>> For context, we use backlight = 0 as a quick "turn the panel off" >>> function where possible. This allows us to avoid the panel off delay >>> where possible. Breaking this assumption means that we'd pay the panel >>> off delay penalty everywhere, not just with BYT. >> >> Well kde is the opposite and I've had users yelling at me that they >> can't use their system, since acpi pretty much always leaves the >> backlight on when set to 0. And acpi kinda rules on intel platforms. >> So I think I'll merge this one since black screens trumps a slight >> functional degration and we didn't duct-tape over the kde assumptions >> either. Essentially you can't assume anything about backlight values >> besides that bigger values tends to mean brigther. Linearity, absolute >> values and endpoints are kinda all off. > > It seems fairly wrong to model the backlight behavior after the only > thing everyone agrees is broken (ACPI). Please don't put words into everyone's mouth. Talking to folks on IRC, Dave Airlie said, "I think its been disagreed over the years to be implementation dependent." and Matthew Garrett said, "It's currently implementation dependent. It can never mean off under all circumstances (not all interfaces offer a way to turn it off)." I don't know whether they think ACPI is broken, but clearly there's no one true answer, and I think it's not the correct approach for userspace to assume 0 means off. > If you go through the backlight drivers, (at least all the ones I've > looked at) ensure that 0 really means off. And systemd goes out of it's way to not switch backlight off on those interfaces... and it's obvious they can't assume anything about the meaning of 0 either: http://cgit.freedesktop.org/systemd/systemd/commit/?id=7b909d7407965c03caaba30daae7aee113627a83 >> If we want to specifically expose an intermediate "panel off" level >> because the full link training is too expensive we imo should do this >> as an intermediate dpms level, e.g. dpms standby. > > Training isn't the problem, the panel delays are the problem here (and > not something we can work around because it's part of the panel > specifications which we have to follow). Using dpms wouldn't solve > anything. We could have dpms standby mean backlight off, everything else on, similarly to what you want by 0 backlight meaning off. So you could switch between dpms on/standby more quickly. The point in that is that it's the right API for doing panel power sequencing. You see, even with defining backlight 0 = off, we'd have to respect the panel sequencing and delays for backlight (admittedly we currently don't do that, which is why it's fast - and broken). Doing the panel sequencing properly from the backlight interface gets *much* harder because it's all backwards in the stack then. BR, Jani. > > Stéphane > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness
On Tue, Jun 3, 2014 at 1:26 PM, Daniel Vetter wrote: > On Tue, Jun 3, 2014 at 6:40 PM, Stéphane Marchesin > wrote: >> On Tue, Apr 29, 2014 at 1:30 PM, Jani Nikula wrote: >>> Historically we've exposed the full backlight PWM duty cycle range to >>> the userspace, in the name of "mechanism, not policy". However, it turns >>> out there are both panels and board designs where there is a minimum >>> duty cycle that is required for proper operation. The minimum duty cycle >>> is available in the VBT. >>> >>> The backlight class sysfs interface does not make any promises to the >>> userspace about the physical meaning of the range >>> 0..max_brightness. Specifically there is no guarantee that 0 means off; >>> indeed for acpi_backlight 0 usually is not off, but the minimum >>> acceptable value. >> >> This is the part we've been relying on in Chrome OS (we assume that 0 >> means off). It seems to me that i915 would be the first, or one of the >> first drivers to violate this rule (in particular you can find a lot >> of backlight drivers trying hard to ensure that 0 means off in the >> backlight drivers directory). >> >> For context, we use backlight = 0 as a quick "turn the panel off" >> function where possible. This allows us to avoid the panel off delay >> where possible. Breaking this assumption means that we'd pay the panel >> off delay penalty everywhere, not just with BYT. > > Well kde is the opposite and I've had users yelling at me that they > can't use their system, since acpi pretty much always leaves the > backlight on when set to 0. And acpi kinda rules on intel platforms. > So I think I'll merge this one since black screens trumps a slight > functional degration and we didn't duct-tape over the kde assumptions > either. Essentially you can't assume anything about backlight values > besides that bigger values tends to mean brigther. Linearity, absolute > values and endpoints are kinda all off. It seems fairly wrong to model the backlight behavior after the only thing everyone agrees is broken (ACPI). If you go through the backlight drivers, (at least all the ones I've looked at) ensure that 0 really means off. > > If we want to specifically expose an intermediate "panel off" level > because the full link training is too expensive we imo should do this > as an intermediate dpms level, e.g. dpms standby. Training isn't the problem, the panel delays are the problem here (and not something we can work around because it's part of the panel specifications which we have to follow). Using dpms wouldn't solve anything. Stéphane ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness
On Tue, Jun 3, 2014 at 6:40 PM, Stéphane Marchesin wrote: > On Tue, Apr 29, 2014 at 1:30 PM, Jani Nikula wrote: >> Historically we've exposed the full backlight PWM duty cycle range to >> the userspace, in the name of "mechanism, not policy". However, it turns >> out there are both panels and board designs where there is a minimum >> duty cycle that is required for proper operation. The minimum duty cycle >> is available in the VBT. >> >> The backlight class sysfs interface does not make any promises to the >> userspace about the physical meaning of the range >> 0..max_brightness. Specifically there is no guarantee that 0 means off; >> indeed for acpi_backlight 0 usually is not off, but the minimum >> acceptable value. > > This is the part we've been relying on in Chrome OS (we assume that 0 > means off). It seems to me that i915 would be the first, or one of the > first drivers to violate this rule (in particular you can find a lot > of backlight drivers trying hard to ensure that 0 means off in the > backlight drivers directory). > > For context, we use backlight = 0 as a quick "turn the panel off" > function where possible. This allows us to avoid the panel off delay > where possible. Breaking this assumption means that we'd pay the panel > off delay penalty everywhere, not just with BYT. Well kde is the opposite and I've had users yelling at me that they can't use their system, since acpi pretty much always leaves the backlight on when set to 0. And acpi kinda rules on intel platforms. So I think I'll merge this one since black screens trumps a slight functional degration and we didn't duct-tape over the kde assumptions either. Essentially you can't assume anything about backlight values besides that bigger values tends to mean brigther. Linearity, absolute values and endpoints are kinda all off. If we want to specifically expose an intermediate "panel off" level because the full link training is too expensive we imo should do this as an intermediate dpms level, e.g. dpms standby. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness
On Tue, Apr 29, 2014 at 1:30 PM, Jani Nikula wrote:
> Historically we've exposed the full backlight PWM duty cycle range to
> the userspace, in the name of "mechanism, not policy". However, it turns
> out there are both panels and board designs where there is a minimum
> duty cycle that is required for proper operation. The minimum duty cycle
> is available in the VBT.
>
> The backlight class sysfs interface does not make any promises to the
> userspace about the physical meaning of the range
> 0..max_brightness. Specifically there is no guarantee that 0 means off;
> indeed for acpi_backlight 0 usually is not off, but the minimum
> acceptable value.
This is the part we've been relying on in Chrome OS (we assume that 0
means off). It seems to me that i915 would be the first, or one of the
first drivers to violate this rule (in particular you can find a lot
of backlight drivers trying hard to ensure that 0 means off in the
backlight drivers directory).
For context, we use backlight = 0 as a quick "turn the panel off"
function where possible. This allows us to avoid the panel off delay
where possible. Breaking this assumption means that we'd pay the panel
off delay penalty everywhere, not just with BYT.
Stéphane
>
> Respect the minimum backlight, and expose the range acceptable to the
> hardware as 0..max_brightness to the userspace via the backlight class
> device; 0 means the minimum acceptable enabled value. To switch off the
> backlight, the user must disable the encoder.
>
> As a side effect, make the backlight class device max brightness and
> physical PWM modulation frequency (i.e. max duty cycle) independent.
>
> Signed-off-by: Jani Nikula
>
> ---
>
> UNTESTED!
> ---
> drivers/gpu/drm/i915/i915_drv.h| 1 +
> drivers/gpu/drm/i915/intel_bios.c | 3 +-
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_panel.c | 97
> +++---
> 4 files changed, 85 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 50dfc3a1a9d1..d9dad3498b87 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1164,6 +1164,7 @@ struct intel_vbt_data {
> u16 pwm_freq_hz;
> bool present;
> bool active_low_pwm;
> + u8 min_brightness; /* min_brightness/255 of max */
> } backlight;
>
> /* MIPI DSI */
> diff --git a/drivers/gpu/drm/i915/intel_bios.c
> b/drivers/gpu/drm/i915/intel_bios.c
> index 2945f57c53ee..1a3e172029b3 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -339,11 +339,12 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv,
> struct bdb_header *bdb)
>
> dev_priv->vbt.backlight.pwm_freq_hz = entry->pwm_freq_hz;
> dev_priv->vbt.backlight.active_low_pwm = entry->active_low_pwm;
> + dev_priv->vbt.backlight.min_brightness = entry->min_brightness;
> DRM_DEBUG_KMS("VBT backlight PWM modulation frequency %u Hz, "
> "active %s, min brightness %u, level %u\n",
> dev_priv->vbt.backlight.pwm_freq_hz,
> dev_priv->vbt.backlight.active_low_pwm ? "low" : "high",
> - entry->min_brightness,
> + dev_priv->vbt.backlight.min_brightness,
> backlight_data->level[panel_type]);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index d8b540b891d1..2af74dd03e31 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -165,6 +165,7 @@ struct intel_panel {
> struct {
> bool present;
> u32 level;
> + u32 min;
> u32 max;
> bool enabled;
> bool combination_mode; /* gen 2/4 only */
> diff --git a/drivers/gpu/drm/i915/intel_panel.c
> b/drivers/gpu/drm/i915/intel_panel.c
> index 776249bab488..360ae203aacb 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -398,6 +398,30 @@ intel_panel_detect(struct drm_device *dev)
> }
> }
>
> +/* Scale user_level in range [0..user_max] to [hw_min..hw_max]. */
> +static u32 scale_user_to_hw(struct intel_connector *connector,
> + u32 user_level, u32 user_max)
> +{
> + struct intel_panel *panel = &connector->panel;
> +
> + user_level = clamp(user_level, 0U, user_max);
> +
> + return panel->backlight.min + user_level *
> + (panel->backlight.max - panel->backlight.min) / user_max;
> +}
> +
> +/* Scale hw_level in range [hw_min..hw_max] to [0..user_max]. */
> +static u32 scale_hw_to_user(struct intel_connector *connector,
> + u32 hw_level, u32 user_max)
> +{
> + struct intel_panel *panel = &connector->panel;
> +
> + hw_level = clamp(hw_leve
Re: [Intel-gfx] [RFC PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness
On Tue, 29 Apr 2014 23:30:49 +0300
Jani Nikula wrote:
> Historically we've exposed the full backlight PWM duty cycle range to
> the userspace, in the name of "mechanism, not policy". However, it turns
> out there are both panels and board designs where there is a minimum
> duty cycle that is required for proper operation. The minimum duty cycle
> is available in the VBT.
>
> The backlight class sysfs interface does not make any promises to the
> userspace about the physical meaning of the range
> 0..max_brightness. Specifically there is no guarantee that 0 means off;
> indeed for acpi_backlight 0 usually is not off, but the minimum
> acceptable value.
>
> Respect the minimum backlight, and expose the range acceptable to the
> hardware as 0..max_brightness to the userspace via the backlight class
> device; 0 means the minimum acceptable enabled value. To switch off the
> backlight, the user must disable the encoder.
>
> As a side effect, make the backlight class device max brightness and
> physical PWM modulation frequency (i.e. max duty cycle) independent.
>
> Signed-off-by: Jani Nikula
I like the direction here... I wonder if we should always virtualize
the max too, and just always expose 0-2047 or something.
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 50dfc3a1a9d1..d9dad3498b87 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1164,6 +1164,7 @@ struct intel_vbt_data {
> u16 pwm_freq_hz;
> bool present;
> bool active_low_pwm;
> + u8 min_brightness; /* min_brightness/255 of max */
> } backlight;
>
> /* MIPI DSI */
> diff --git a/drivers/gpu/drm/i915/intel_bios.c
> b/drivers/gpu/drm/i915/intel_bios.c
> index 2945f57c53ee..1a3e172029b3 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -339,11 +339,12 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv,
> struct bdb_header *bdb)
>
> dev_priv->vbt.backlight.pwm_freq_hz = entry->pwm_freq_hz;
> dev_priv->vbt.backlight.active_low_pwm = entry->active_low_pwm;
> + dev_priv->vbt.backlight.min_brightness = entry->min_brightness;
> DRM_DEBUG_KMS("VBT backlight PWM modulation frequency %u Hz, "
> "active %s, min brightness %u, level %u\n",
> dev_priv->vbt.backlight.pwm_freq_hz,
> dev_priv->vbt.backlight.active_low_pwm ? "low" : "high",
> - entry->min_brightness,
> + dev_priv->vbt.backlight.min_brightness,
> backlight_data->level[panel_type]);
> }
This should probably just be a standalone patch. You can add my r-b
for that.
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index d8b540b891d1..2af74dd03e31 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -165,6 +165,7 @@ struct intel_panel {
> struct {
> bool present;
> u32 level;
> + u32 min;
> u32 max;
> bool enabled;
> bool combination_mode; /* gen 2/4 only */
> diff --git a/drivers/gpu/drm/i915/intel_panel.c
> b/drivers/gpu/drm/i915/intel_panel.c
> index 776249bab488..360ae203aacb 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -398,6 +398,30 @@ intel_panel_detect(struct drm_device *dev)
> }
> }
>
> +/* Scale user_level in range [0..user_max] to [hw_min..hw_max]. */
> +static u32 scale_user_to_hw(struct intel_connector *connector,
> + u32 user_level, u32 user_max)
> +{
> + struct intel_panel *panel = &connector->panel;
> +
> + user_level = clamp(user_level, 0U, user_max);
> +
> + return panel->backlight.min + user_level *
> + (panel->backlight.max - panel->backlight.min) / user_max;
> +}
> +
> +/* Scale hw_level in range [hw_min..hw_max] to [0..user_max]. */
> +static u32 scale_hw_to_user(struct intel_connector *connector,
> + u32 hw_level, u32 user_max)
> +{
> + struct intel_panel *panel = &connector->panel;
> +
> + hw_level = clamp(hw_level, panel->backlight.min, panel->backlight.max);
> +
> + return (hw_level - panel->backlight.min) * user_max /
> + (panel->backlight.max - panel->backlight.min);
> +}
> +
> static u32 intel_panel_compute_brightness(struct intel_connector *connector,
> u32 val)
> {
> @@ -558,14 +582,14 @@ intel_panel_actually_set_backlight(struct
> intel_connector *connector, u32 level)
> }
>
> /* set backlight brightness to level in range [0..max] */
> -void intel_panel_set_backlight(struct intel_connector *connector, u32 level,
> -u32 max)
> +void intel_panel_set_backlight(struct intel_connector *connector,
> +
Re: [Intel-gfx] [RFC PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness
On Tue, 29 Apr 2014, Jani Nikula wrote:
> Historically we've exposed the full backlight PWM duty cycle range to
> the userspace, in the name of "mechanism, not policy". However, it turns
> out there are both panels and board designs where there is a minimum
> duty cycle that is required for proper operation. The minimum duty cycle
> is available in the VBT.
>
> The backlight class sysfs interface does not make any promises to the
> userspace about the physical meaning of the range
> 0..max_brightness. Specifically there is no guarantee that 0 means off;
> indeed for acpi_backlight 0 usually is not off, but the minimum
> acceptable value.
>
> Respect the minimum backlight, and expose the range acceptable to the
> hardware as 0..max_brightness to the userspace via the backlight class
> device; 0 means the minimum acceptable enabled value. To switch off the
> backlight, the user must disable the encoder.
>
> As a side effect, make the backlight class device max brightness and
> physical PWM modulation frequency (i.e. max duty cycle) independent.
>
> Signed-off-by: Jani Nikula
Acknowledgement that I meant to include: this is based on both Jesse's
and Ben's earlier work.
>
> ---
>
> UNTESTED!
> ---
> drivers/gpu/drm/i915/i915_drv.h| 1 +
> drivers/gpu/drm/i915/intel_bios.c | 3 +-
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_panel.c | 97
> +++---
> 4 files changed, 85 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 50dfc3a1a9d1..d9dad3498b87 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1164,6 +1164,7 @@ struct intel_vbt_data {
> u16 pwm_freq_hz;
> bool present;
> bool active_low_pwm;
> + u8 min_brightness; /* min_brightness/255 of max */
> } backlight;
>
> /* MIPI DSI */
> diff --git a/drivers/gpu/drm/i915/intel_bios.c
> b/drivers/gpu/drm/i915/intel_bios.c
> index 2945f57c53ee..1a3e172029b3 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -339,11 +339,12 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv,
> struct bdb_header *bdb)
>
> dev_priv->vbt.backlight.pwm_freq_hz = entry->pwm_freq_hz;
> dev_priv->vbt.backlight.active_low_pwm = entry->active_low_pwm;
> + dev_priv->vbt.backlight.min_brightness = entry->min_brightness;
> DRM_DEBUG_KMS("VBT backlight PWM modulation frequency %u Hz, "
> "active %s, min brightness %u, level %u\n",
> dev_priv->vbt.backlight.pwm_freq_hz,
> dev_priv->vbt.backlight.active_low_pwm ? "low" : "high",
> - entry->min_brightness,
> + dev_priv->vbt.backlight.min_brightness,
> backlight_data->level[panel_type]);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index d8b540b891d1..2af74dd03e31 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -165,6 +165,7 @@ struct intel_panel {
> struct {
> bool present;
> u32 level;
> + u32 min;
> u32 max;
> bool enabled;
> bool combination_mode; /* gen 2/4 only */
> diff --git a/drivers/gpu/drm/i915/intel_panel.c
> b/drivers/gpu/drm/i915/intel_panel.c
> index 776249bab488..360ae203aacb 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -398,6 +398,30 @@ intel_panel_detect(struct drm_device *dev)
> }
> }
>
> +/* Scale user_level in range [0..user_max] to [hw_min..hw_max]. */
> +static u32 scale_user_to_hw(struct intel_connector *connector,
> + u32 user_level, u32 user_max)
> +{
> + struct intel_panel *panel = &connector->panel;
> +
> + user_level = clamp(user_level, 0U, user_max);
> +
> + return panel->backlight.min + user_level *
> + (panel->backlight.max - panel->backlight.min) / user_max;
> +}
> +
> +/* Scale hw_level in range [hw_min..hw_max] to [0..user_max]. */
> +static u32 scale_hw_to_user(struct intel_connector *connector,
> + u32 hw_level, u32 user_max)
> +{
> + struct intel_panel *panel = &connector->panel;
> +
> + hw_level = clamp(hw_level, panel->backlight.min, panel->backlight.max);
> +
> + return (hw_level - panel->backlight.min) * user_max /
> + (panel->backlight.max - panel->backlight.min);
> +}
> +
> static u32 intel_panel_compute_brightness(struct intel_connector *connector,
> u32 val)
> {
> @@ -558,14 +582,14 @@ intel_panel_actually_set_backlight(struct
> intel_connector *connector, u32 level)
> }
>
> /* set backlight brightness to level in range [0..max] */
> -void intel_panel_set_backlight(struct int
[Intel-gfx] [RFC PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness
Historically we've exposed the full backlight PWM duty cycle range to
the userspace, in the name of "mechanism, not policy". However, it turns
out there are both panels and board designs where there is a minimum
duty cycle that is required for proper operation. The minimum duty cycle
is available in the VBT.
The backlight class sysfs interface does not make any promises to the
userspace about the physical meaning of the range
0..max_brightness. Specifically there is no guarantee that 0 means off;
indeed for acpi_backlight 0 usually is not off, but the minimum
acceptable value.
Respect the minimum backlight, and expose the range acceptable to the
hardware as 0..max_brightness to the userspace via the backlight class
device; 0 means the minimum acceptable enabled value. To switch off the
backlight, the user must disable the encoder.
As a side effect, make the backlight class device max brightness and
physical PWM modulation frequency (i.e. max duty cycle) independent.
Signed-off-by: Jani Nikula
---
UNTESTED!
---
drivers/gpu/drm/i915/i915_drv.h| 1 +
drivers/gpu/drm/i915/intel_bios.c | 3 +-
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_panel.c | 97 +++---
4 files changed, 85 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 50dfc3a1a9d1..d9dad3498b87 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1164,6 +1164,7 @@ struct intel_vbt_data {
u16 pwm_freq_hz;
bool present;
bool active_low_pwm;
+ u8 min_brightness; /* min_brightness/255 of max */
} backlight;
/* MIPI DSI */
diff --git a/drivers/gpu/drm/i915/intel_bios.c
b/drivers/gpu/drm/i915/intel_bios.c
index 2945f57c53ee..1a3e172029b3 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -339,11 +339,12 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv,
struct bdb_header *bdb)
dev_priv->vbt.backlight.pwm_freq_hz = entry->pwm_freq_hz;
dev_priv->vbt.backlight.active_low_pwm = entry->active_low_pwm;
+ dev_priv->vbt.backlight.min_brightness = entry->min_brightness;
DRM_DEBUG_KMS("VBT backlight PWM modulation frequency %u Hz, "
"active %s, min brightness %u, level %u\n",
dev_priv->vbt.backlight.pwm_freq_hz,
dev_priv->vbt.backlight.active_low_pwm ? "low" : "high",
- entry->min_brightness,
+ dev_priv->vbt.backlight.min_brightness,
backlight_data->level[panel_type]);
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d8b540b891d1..2af74dd03e31 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -165,6 +165,7 @@ struct intel_panel {
struct {
bool present;
u32 level;
+ u32 min;
u32 max;
bool enabled;
bool combination_mode; /* gen 2/4 only */
diff --git a/drivers/gpu/drm/i915/intel_panel.c
b/drivers/gpu/drm/i915/intel_panel.c
index 776249bab488..360ae203aacb 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -398,6 +398,30 @@ intel_panel_detect(struct drm_device *dev)
}
}
+/* Scale user_level in range [0..user_max] to [hw_min..hw_max]. */
+static u32 scale_user_to_hw(struct intel_connector *connector,
+ u32 user_level, u32 user_max)
+{
+ struct intel_panel *panel = &connector->panel;
+
+ user_level = clamp(user_level, 0U, user_max);
+
+ return panel->backlight.min + user_level *
+ (panel->backlight.max - panel->backlight.min) / user_max;
+}
+
+/* Scale hw_level in range [hw_min..hw_max] to [0..user_max]. */
+static u32 scale_hw_to_user(struct intel_connector *connector,
+ u32 hw_level, u32 user_max)
+{
+ struct intel_panel *panel = &connector->panel;
+
+ hw_level = clamp(hw_level, panel->backlight.min, panel->backlight.max);
+
+ return (hw_level - panel->backlight.min) * user_max /
+ (panel->backlight.max - panel->backlight.min);
+}
+
static u32 intel_panel_compute_brightness(struct intel_connector *connector,
u32 val)
{
@@ -558,14 +582,14 @@ intel_panel_actually_set_backlight(struct intel_connector
*connector, u32 level)
}
/* set backlight brightness to level in range [0..max] */
-void intel_panel_set_backlight(struct intel_connector *connector, u32 level,
- u32 max)
+void intel_panel_set_backlight(struct intel_connector *connector,
+ u32 user_level, u32 user_max)
{
struct drm_device *dev = connector->base.dev;
struct drm_i915_private *dev_priv =
