[Intel-gfx] [PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness
Jani Nikula
jani.nikula at intel.com
Sat Jun 28 15:45:03 CEST 2014
On Fri, 27 Jun 2014, Jesse Barnes <jesse.barnes at intel.com> wrote:
> On Tue, 24 Jun 2014 18:27:40 +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. This allows a follow-up patch to virtualize the max value
>> exposed to the userspace.
>>
>> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>>
>> ---
>>
>> This version turned out uglier than I anticipated because the requests
>> through opregion in range 0..255 already have the minimum limit bundled
>> in. Scaling that would be wrong. Ideas welcome.
>> ---
>> drivers/gpu/drm/i915/intel_drv.h | 5 +-
>> drivers/gpu/drm/i915/intel_opregion.c | 2 +-
>> drivers/gpu/drm/i915/intel_panel.c | 158 ++++++++++++++++++++++++++++++----
>> 3 files changed, 146 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 5f7c7bd94d90..2651e2ff7c05 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 */
>> @@ -948,8 +949,8 @@ void intel_pch_panel_fitting(struct intel_crtc *crtc,
>> void intel_gmch_panel_fitting(struct intel_crtc *crtc,
>> struct intel_crtc_config *pipe_config,
>> int fitting_mode);
>> -void intel_panel_set_backlight(struct intel_connector *connector, u32 level,
>> - u32 max);
>> +void intel_panel_set_backlight_acpi(struct intel_connector *connector,
>> + u32 level, u32 max);
>> int intel_panel_setup_backlight(struct drm_connector *connector);
>> void intel_panel_enable_backlight(struct intel_connector *connector);
>> void intel_panel_disable_backlight(struct intel_connector *connector);
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index 2e2c71fcc9ed..5a979b70e3cf 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -418,7 +418,7 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>> */
>> DRM_DEBUG_KMS("updating opregion backlight %d/255\n", bclp);
>> list_for_each_entry(intel_connector, &dev->mode_config.connector_list, base.head)
>> - intel_panel_set_backlight(intel_connector, bclp, 255);
>> + intel_panel_set_backlight_acpi(intel_connector, bclp, 255);
>> iowrite32(DIV_ROUND_UP(bclp * 100, 255) | ASLE_CBLV_VALID, &asle->cblv);
>>
>> drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> index 38a98570d10c..4924c5e07b07 100644
>> --- a/drivers/gpu/drm/i915/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>> @@ -398,6 +398,69 @@ intel_panel_detect(struct drm_device *dev)
>> }
>> }
>>
>> +/**
>> + * scale - scale values from one range to another
>> + *
>> + * @source_val: value in range [@source_min.. at source_max]
>> + *
>> + * Return @source_val in range [@source_min.. at source_max] scaled to range
>> + * [@target_min.. at target_max].
>> + */
>> +static uint32_t scale(uint32_t source_val,
>> + uint32_t source_min, uint32_t source_max,
>> + uint32_t target_min, uint32_t target_max)
>> +{
>> + uint64_t target_val;
>> +
>> + BUG_ON(source_min > source_max);
>> + BUG_ON(target_min > target_max);
>> +
>> + /* defensive */
>> + source_val = clamp(source_val, source_min, source_max);
>> +
>> + /* avoid overflows */
>> + target_val = (uint64_t)(source_val - source_min) *
>> + (target_max - target_min);
>> + do_div(target_val, source_max - source_min);
>> + target_val += target_min;
>> +
>> + return target_val;
>> +}
>> +
>> +/* Scale user_level in range [0..user_max] to [hw_min..hw_max]. */
>> +static inline u32 scale_user_to_hw(struct intel_connector *connector,
>> + u32 user_level, u32 user_max)
>> +{
>> + struct intel_panel *panel = &connector->panel;
>> +
>> + return scale(user_level, 0, user_max,
>> + panel->backlight.min, panel->backlight.max);
>> +}
>> +
>> +/* Scale user_level in range [0..user_max] to [0..hw_max], clamping the result
>> + * to [hw_min..hw_max]. */
>> +static inline u32 clamp_user_to_hw(struct intel_connector *connector,
>> + u32 user_level, u32 user_max)
>> +{
>> + struct intel_panel *panel = &connector->panel;
>> + u32 hw_level;
>> +
>> + hw_level = scale(user_level, 0, user_max, 0, panel->backlight.max);
>> + hw_level = clamp(hw_level, panel->backlight.min, panel->backlight.max);
>> +
>> + return hw_level;
>> +}
>
> I like the direction here, but does this mean some user values will
> potentially be no-ops? E.g. the low 10 values or something would all
> map to backlight.min depending on the min?
This patch doesn't really change the fact. For a max backlight of, say,
100000, we're bound to not have user perceivable difference between
consecutive levels. I agree we should have a fixed, limited range here.
This also depends on the backlight modulation frequency, see my earlier
message about this: http://mid.gmane.org/87iooecg1e.fsf@intel.com
> I just want to make sure every value we expose to userspace is
> meaningful, and somehow equidistant from the values next to it, so we
> have nice, smooth backlight transitions, and fades look good (on that
> front, is 256 enough? or should we have a scaled range up to 1024 or
> so?)
Probably even fewer than that is enough.
But you bring up another requirement, equidistant - do you mean in terms
of luminance? Almost certainly a linear mapping to the duty cycle is not
going to give you a linear luminance! It's possible to define a curve
for this in the acpi opregion; patches to implement this are welcome. ;)
BR,
Jani.
>
> Cc'ing Clint.
>
>> +
>> +/* Scale hw_level in range [hw_min..hw_max] to [0..user_max]. */
>> +static inline u32 scale_hw_to_user(struct intel_connector *connector,
>> + u32 hw_level, u32 user_max)
>> +{
>> + struct intel_panel *panel = &connector->panel;
>> +
>> + return scale(hw_level, panel->backlight.min, panel->backlight.max,
>> + 0, user_max);
>> +}
>> +
>> static u32 intel_panel_compute_brightness(struct intel_connector *connector,
>> u32 val)
>> {
>> @@ -557,17 +620,16 @@ intel_panel_actually_set_backlight(struct intel_connector *connector, u32 level)
>> dev_priv->display.set_backlight(connector, level);
>> }
>>
>> -/* set backlight brightness to level in range [0..max] */
>> -void intel_panel_set_backlight(struct intel_connector *connector, u32 level,
>> - u32 max)
>> +/* set backlight brightness to level in range [0..max], scaling wrt hw min */
>> +static 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;
>> - u64 n;
>>
>> if (!panel->backlight.present || pipe == INVALID_PIPE)
>> return;
>> @@ -576,18 +638,46 @@ 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;
>> - n = (u64)level * freq;
>> - do_div(n, max);
>> - level = n;
>> + hw_level = scale_user_to_hw(connector, user_level, user_max);
>> + panel->backlight.level = hw_level;
>> +
>> + if (panel->backlight.enabled)
>> + intel_panel_actually_set_backlight(connector, hw_level);
>> +
>> + spin_unlock_irqrestore(&dev_priv->backlight_lock, flags);
>> +}
>> +
>> +/* set backlight brightness to level in range [0..max], assuming hw min is
>> + * respected.
>> + */
>> +void intel_panel_set_backlight_acpi(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 hw_level;
>> + unsigned long flags;
>> +
>> + if (!panel->backlight.present || pipe == INVALID_PIPE)
>> + return;
>> +
>> + spin_lock_irqsave(&dev_priv->backlight_lock, flags);
>> +
>> + WARN_ON(panel->backlight.max == 0);
>> +
>> + hw_level = clamp_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;
>> + panel->backlight.device->props.brightness =
>> + scale_hw_to_user(connector,
>> + panel->backlight.level,
>> + panel->backlight.device->props.max_brightness);
>>
>> 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);
>> }
>> @@ -860,7 +950,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);
>> @@ -889,11 +981,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);
>> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> - 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);
>> +
>> drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> intel_runtime_pm_put(dev_priv);
>>
>> @@ -917,8 +1013,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
>> @@ -964,6 +1067,19 @@ 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 u32 get_backlight_min_vbt(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);
>> +
>> + /* vbt value is a coefficient in range [0..255] */
>> + return scale(dev_priv->vbt.backlight.min_brightness, 0, 255,
>> + 0, panel->backlight.max);
>> +}
>> +
>> static int bdw_setup_backlight(struct intel_connector *connector)
>> {
>> struct drm_device *dev = connector->base.dev;
>> @@ -979,6 +1095,8 @@ static int bdw_setup_backlight(struct intel_connector *connector)
>> if (!panel->backlight.max)
>> return -ENODEV;
>>
>> + panel->backlight.min = get_backlight_min_vbt(connector);
>> +
>> val = bdw_get_backlight(connector);
>> panel->backlight.level = intel_panel_compute_brightness(connector, val);
>>
>> @@ -1003,6 +1121,8 @@ static int pch_setup_backlight(struct intel_connector *connector)
>> if (!panel->backlight.max)
>> return -ENODEV;
>>
>> + panel->backlight.min = get_backlight_min_vbt(connector);
>> +
>> val = pch_get_backlight(connector);
>> panel->backlight.level = intel_panel_compute_brightness(connector, val);
>>
>> @@ -1035,6 +1155,8 @@ static int i9xx_setup_backlight(struct intel_connector *connector)
>> if (!panel->backlight.max)
>> return -ENODEV;
>>
>> + panel->backlight.min = get_backlight_min_vbt(connector);
>> +
>> val = i9xx_get_backlight(connector);
>> panel->backlight.level = intel_panel_compute_brightness(connector, val);
>>
>> @@ -1062,6 +1184,8 @@ static int i965_setup_backlight(struct intel_connector *connector)
>> if (!panel->backlight.max)
>> return -ENODEV;
>>
>> + panel->backlight.min = get_backlight_min_vbt(connector);
>> +
>> val = i9xx_get_backlight(connector);
>> panel->backlight.level = intel_panel_compute_brightness(connector, val);
>>
>> @@ -1099,6 +1223,8 @@ static int vlv_setup_backlight(struct intel_connector *connector)
>> if (!panel->backlight.max)
>> return -ENODEV;
>>
>> + panel->backlight.min = get_backlight_min_vbt(connector);
>> +
>> val = _vlv_get_backlight(dev, PIPE_A);
>> panel->backlight.level = intel_panel_compute_brightness(connector, val);
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list