[Intel-gfx] [RFC PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness
Jesse Barnes
jbarnes at virtuousgeek.org
Tue May 20 21:08:11 CEST 2014
On Tue, 29 Apr 2014 23:30:49 +0300
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.
>
> 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>
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,
> + 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;
> +}
Is this the user version or the hw version? If hw, why not just use
min_brightness directly?
--
Jesse Barnes, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list