Re: [PATCH v2] drm/i915: Respect the brightness range from VBT.

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

On Tue, 10 Nov 2015, "Shih-Yuan Lee (FourDollars)" <sylee@canonical.com> wrote:
> Taking Dell XPS 13 (2015) as an example. The lowest PWM brightness is 10
> and the highest PWM brightness is 937. Before this change, we can only
> use from 37 to 937, and 37 is used to turn off the backlight because it
> is mapped to 0 of sysfs brightness however the maximum sysfs brightness
> is still 937 so it makes some sysfs brightness values are mapped to the
> same PWM brightness values.
>
> After this change, we can use the whole PWM brightness range from 10 to
> 937, and they are mapped into the range from 0 to 927 in the sysfs
> brightness and vice versa because they are 1-1 mapping.
>
> 10 is the lowest PWM brightness. We should make users able to use it for
> the extreme power saving reason and users can still see the display.
>
> Signed-off-by: Shih-Yuan Lee (FourDollars) <sylee@canonical.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index a24df35..57bc2fe 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1154,8 +1154,7 @@ static int intel_backlight_device_update_status(struct backlight_device *bd)
>  	 */
>  	if (panel->backlight.enabled) {
>  		if (panel->backlight.power) {
> -			bool enable = bd->props.power == FB_BLANK_UNBLANK &&
> -				bd->props.brightness != 0;
> +			bool enable = bd->props.power == FB_BLANK_UNBLANK;
>  			panel->backlight.power(connector, enable);

This hunk would be a revert of

commit e6755fb78e8f20ecadf2a4080084121336624ad9
Author: Jani Nikula <jani.nikula@intel.com>
Date:   Tue Aug 12 17:11:42 2014 +0300

    drm/i915: switch off backlight for backlight class 0 brightness

>  		}
>  	} else {
> @@ -1211,7 +1210,7 @@ static int intel_backlight_device_register(struct intel_connector *connector)
>  	 * Note: Everything should work even if the backlight device max
>  	 * presented to the userspace is arbitrarily chosen.
>  	 */
> -	props.max_brightness = panel->backlight.max;
> +	props.max_brightness = panel->backlight.max - panel->backlight.min;

IMO we should consider setting max to fixed 100 here instead. That was
the plan all along when I wrote the scaling stuff and added the Note:
above in

commit 6dda730e55f412a6dfb181cae6784822ba463847
Author: Jani Nikula <jani.nikula@intel.com>
Date:   Tue Jun 24 18:27:40 2014 +0300

    drm/i915: respect the VBT minimum backlight brightness

I just chickened out of it at that time. The commit message is worth
reading in full.

>  	props.brightness = scale_hw_to_user(connector,
>  					    panel->backlight.level,
>  					    props.max_brightness);
> @@ -1429,10 +1428,11 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector)
>  	if (min != dev_priv->vbt.backlight.min_brightness) {
>  		DRM_DEBUG_KMS("clamping VBT min backlight %d/255 to %d/255\n",
>  			      dev_priv->vbt.backlight.min_brightness, min);
> +		/* vbt value is a coefficient in range [0..255] */
> +		return scale(min, 0, 255, 0, panel->backlight.max);
>  	}
>  
> -	/* vbt value is a coefficient in range [0..255] */
> -	return scale(min, 0, 255, 0, panel->backlight.max);
> +	return min;

This change would interpret the VBT minimum value as a coefficient x/255
if it's in range 0..64, and as an absolute value x if it's in range
65..255. This seems like an odd thing to do, although I admit what I did
in

commit e1c412e75754ab7b7002f3e18a2652d999c40d4b
Author: Jani Nikula <jani.nikula@intel.com>
Date:   Wed Nov 5 14:46:31 2014 +0200

    drm/i915: safeguard against too high minimum brightness

is not that much better.

The problem is, there's a spec on VBT (internal, I'm afraid), there's a
tool to generate VBT (Windows only, probably not freely available) with
some interpretation of the spec, and then there are users of the tool
that tweak the variables until they see something they like on the
machine they're developing, running some Windows version with a driver
version with some interpretation of the spec.

So no, I don't agree with any of the changes you propose.

I do acknowledge that we have a minor bug that prevents the user from
setting the brightness to the minimum acceptable by the hardware if the
minimum is non-zero, and 0 means off. Keeping the 0 = off meaning, the
fix would be to have 1 mean the minimum brightness acceptable by the
hardware... which really isn't that much different from what 1 currently
means. The bigger problem is that the userspace probably never sets
brightness to 1 except by accident.


BR,
Jani.


>  }
>  
>  static int lpt_setup_backlight(struct intel_connector *connector, enum pipe unused)

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx