Re: [RFC PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 29 Apr 2014, Jani Nikula <jani.nikula@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@intel.com>

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 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center