[Intel-gfx] [RFC PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness
Stéphane Marchesin
marcheu at chromium.org
Tue Jun 3 18:40:08 CEST 2014
On Tue, Apr 29, 2014 at 1:30 PM, Jani Nikula <jani.nikula at intel.com> 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 <jani.nikula at intel.com>
>
> ---
>
> 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 = dev->dev_private;
> struct intel_panel *panel = &connector->panel;
> enum pipe pipe = intel_get_pipe_from_connector(connector);
> - u32 freq;
> + u32 hw_level;
> unsigned long flags;
>
> if (!panel->backlight.present || pipe == INVALID_PIPE)
> @@ -575,19 +599,25 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level,
>
> WARN_ON(panel->backlight.max == 0);
>
> - /* scale to hardware max, but be careful to not overflow */
> - freq = panel->backlight.max;
> - if (freq < max)
> - level = level * freq / max;
> - else
> - level = freq / max * level;
> + hw_level = scale_user_to_hw(connector, user_level, user_max);
> + panel->backlight.level = hw_level;
>
> - panel->backlight.level = level;
> - if (panel->backlight.device)
> - panel->backlight.device->props.brightness = level;
> + if (panel->backlight.device) {
> + /*
> + * Update backlight device brightness. In most cases, the
> + * request comes from the backlight device sysfs, user_max ==
> + * props.max_brightness, and this is redundant. However, this
> + * serves to sync ACPI opregion backlight requests to the
> + * backlight device.
> + */
> + panel->backlight.device->props.brightness =
> + user_level *
> + panel->backlight.device->props.max_brightness /
> + user_max;
> + }
>
> if (panel->backlight.enabled)
> - intel_panel_actually_set_backlight(connector, level);
> + intel_panel_actually_set_backlight(connector, hw_level);
>
> spin_unlock_irqrestore(&dev_priv->backlight_lock, flags);
> }
> @@ -861,7 +891,9 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
> panel->backlight.level = panel->backlight.max;
> if (panel->backlight.device)
> panel->backlight.device->props.brightness =
> - panel->backlight.level;
> + scale_hw_to_user(connector,
> + panel->backlight.level,
> + panel->backlight.device->props.max_brightness);
> }
>
> dev_priv->display.enable_backlight(connector);
> @@ -890,11 +922,15 @@ static int intel_backlight_device_get_brightness(struct backlight_device *bd)
> struct intel_connector *connector = bl_get_data(bd);
> struct drm_device *dev = connector->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> + u32 hw_level;
> int ret;
>
> intel_runtime_pm_get(dev_priv);
> mutex_lock(&dev->mode_config.mutex);
> - ret = intel_panel_get_backlight(connector);
> +
> + hw_level = intel_panel_get_backlight(connector);
> + ret = scale_hw_to_user(connector, hw_level, bd->props.max_brightness);
> +
> mutex_unlock(&dev->mode_config.mutex);
> intel_runtime_pm_put(dev_priv);
>
> @@ -918,8 +954,15 @@ static int intel_backlight_device_register(struct intel_connector *connector)
>
> memset(&props, 0, sizeof(props));
> props.type = BACKLIGHT_RAW;
> - props.brightness = panel->backlight.level;
> +
> + /*
> + * Note: Everything should work even if the backlight device max
> + * presented to the userspace is arbitrarily chosen.
> + */
> props.max_brightness = panel->backlight.max;
> + props.brightness = scale_hw_to_user(connector,
> + panel->backlight.level,
> + props.max_brightness);
>
> /*
> * Note: using the same name independent of the connector prevents
> @@ -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;
> +}
> +
> static int bdw_setup_backlight(struct intel_connector *connector)
> {
> struct drm_device *dev = connector->base.dev;
> @@ -980,6 +1035,8 @@ static int bdw_setup_backlight(struct intel_connector *connector)
> if (!panel->backlight.max)
> return -ENODEV;
>
> + panel->backlight.min = get_backlight_min(connector);
> +
> val = bdw_get_backlight(connector);
> panel->backlight.level = intel_panel_compute_brightness(connector, val);
>
> @@ -1004,6 +1061,8 @@ static int pch_setup_backlight(struct intel_connector *connector)
> if (!panel->backlight.max)
> return -ENODEV;
>
> + panel->backlight.min = get_backlight_min(connector);
> +
> val = pch_get_backlight(connector);
> panel->backlight.level = intel_panel_compute_brightness(connector, val);
>
> @@ -1036,6 +1095,8 @@ static int i9xx_setup_backlight(struct intel_connector *connector)
> if (!panel->backlight.max)
> return -ENODEV;
>
> + panel->backlight.min = get_backlight_min(connector);
> +
> val = i9xx_get_backlight(connector);
> panel->backlight.level = intel_panel_compute_brightness(connector, val);
>
> @@ -1063,6 +1124,8 @@ static int i965_setup_backlight(struct intel_connector *connector)
> if (!panel->backlight.max)
> return -ENODEV;
>
> + panel->backlight.min = get_backlight_min(connector);
> +
> val = i9xx_get_backlight(connector);
> panel->backlight.level = intel_panel_compute_brightness(connector, val);
>
> @@ -1100,6 +1163,8 @@ static int vlv_setup_backlight(struct intel_connector *connector)
> if (!panel->backlight.max)
> return -ENODEV;
>
> + panel->backlight.min = get_backlight_min(connector);
> +
> val = _vlv_get_backlight(dev, PIPE_A);
> panel->backlight.level = intel_panel_compute_brightness(connector, val);
>
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list