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

["Shih-Yuan Lee (FourDollars)" <sylee@canonical.com>]

On Tue, Nov 10, 2015 at 12:15:48PM +0200, Jani Nikula wrote:
> 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.

Apparently you are not aware of GNOME has used 1 as the minimum brightness
level because 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

Please check https://bugzilla.gnome.org/show_bug.cgi?id=744278.

Thanks for your time to review my patch.
I will check other commits you mentioned.

Regards,
$4

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