| Message ID | 1403623660-1257-2-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 24 Jun 2014 18:27:40 +0300 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. This allows a follow-up patch to virtualize the max value > exposed to the userspace. > > Signed-off-by: Jani Nikula <jani.nikula@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..@source_max] > + * > + * Return @source_val in range [@source_min..@source_max] scaled to range > + * [@target_min..@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? 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?) 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); >
On Fri, 27 Jun 2014, Jesse Barnes <jesse.barnes@intel.com> wrote: > On Tue, 24 Jun 2014 18:27:40 +0300 > 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. This allows a follow-up patch to virtualize the max value >> exposed to the userspace. >> >> Signed-off-by: Jani Nikula <jani.nikula@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..@source_max] >> + * >> + * Return @source_val in range [@source_min..@source_max] scaled to range >> + * [@target_min..@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Sat, 28 Jun 2014 16:45:03 +0300 Jani Nikula <jani.nikula@intel.com> wrote: > >> +/* 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. ;) Ok, I guess both of these are projects for future patches. For making the brightness levels evenly spaced, yeah I guess we'd need to apply a rough approximation of a luminosity function (doesn't look like a simple exponential curve, oh well). Jesse
On Mon, 30 Jun 2014 08:05:34 -0700 Jesse Barnes <jesse.barnes@intel.com> wrote: > On Sat, 28 Jun 2014 16:45:03 +0300 > Jani Nikula <jani.nikula@intel.com> wrote: > > >> +/* 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. ;) > > Ok, I guess both of these are projects for future patches. For making > the brightness levels evenly spaced, yeah I guess we'd need to apply a > rough approximation of a luminosity function (doesn't look like a > simple exponential curve, oh well). Oh and r-b for this patch. :) Thanks,
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..@source_max] + * + * Return @source_val in range [@source_min..@source_max] scaled to range + * [@target_min..@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; +} + +/* 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);
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@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(-)