From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness Date: Sat, 28 Jun 2014 16:45:03 +0300 Message-ID: <87d2dtjggw.fsf@intel.com> References: <1403623660-1257-1-git-send-email-jani.nikula@intel.com> <1403623660-1257-2-git-send-email-jani.nikula@intel.com> <20140627085111.43b95d59@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id EF7286E1CC for ; Sat, 28 Jun 2014 06:45:11 -0700 (PDT) In-Reply-To: <20140627085111.43b95d59@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, 27 Jun 2014, Jesse Barnes wrote: > On Tue, 24 Jun 2014 18:27:40 +0300 > Jani Nikula 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 >> >> --- >> >> 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 -- Jani Nikula, Intel Open Source Technology Center