Re: [PATCH v4] drm/i915: Set brightness maximum to a fixed value 100.

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

On Wed, Nov 11, 2015 at 02:05:57PM +0200, Jani Nikula wrote:
> On Wed, 11 Nov 2015, "Shih-Yuan Lee (FourDollars)" <sylee@canonical.com> wrote:
> > Take Dell XPS 13 (2015) as an example. The vbt min 10 out of [0..255].
> > The PWM max is 937 so the corresponding PWM min is 37 (10*937/256).
> > This commit makes the sysfs brightness 1 map to the PWM brightness 37
> > and 100 map to the PWM brightness 937.
> >
> > Signed-off-by: Shih-Yuan Lee (FourDollars) <sylee@canonical.com>
> > ---
> >  drivers/gpu/drm/i915/intel_panel.c | 30 ++++++++++++++++++++++++------
> >  1 file changed, 24 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index a24df35..8652ce2 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -1207,11 +1207,12 @@ static int intel_backlight_device_register(struct intel_connector *connector)
> >  	memset(&props, 0, sizeof(props));
> >  	props.type = BACKLIGHT_RAW;
> >  
> > -	/*
> > -	 * Note: Everything should work even if the backlight device max
> > -	 * presented to the userspace is arbitrarily chosen.
> > -	 */
> > -	props.max_brightness = panel->backlight.max;
> > +	/* Setting max to fixed 100 if the range is large enough. */
> > +	if (panel->backlight.max > panel->backlight.min + 99)
> > +		props.max_brightness = 100;
> > +	else
> > +		props.max_brightness = panel->backlight.max - panel->backlight.min;
> 
> No, the intention is to have a fixed 0..100 range no ifs no buts.
> 
> > +
> >  	props.brightness = scale_hw_to_user(connector,
> >  					    panel->backlight.level,
> >  					    props.max_brightness);
> > @@ -1414,6 +1415,8 @@ 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;
> > +	u32 pwm_min;
> > +	u32 pwm_step;
> >  	int min;
> >  
> >  	WARN_ON(panel->backlight.max == 0);
> > @@ -1432,7 +1435,22 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector)
> >  	}
> >  
> >  	/* vbt value is a coefficient in range [0..255] */
> > -	return scale(min, 0, 255, 0, panel->backlight.max);
> > +	pwm_min = scale(min, 0, 255, 0, panel->backlight.max);
> > +
> > +	/* Calculate the PWM step */
> > +	if (panel->backlight.max > pwm_min + 99)
> > +		pwm_step = scale(1, 0, 99, 0, panel->backlight.max - pwm_min);
> > +	else
> > +		pwm_step = 1;
> > +
> > +	/*
> > +	 * Because sysfs brightness 0 is used to turn off the backlight, we need step
> > +	 * down a little bit to make sysfs brightness 1 match the lowest brightness.
> > +	 */
> > +	if (pwm_min >= pwm_step)
> > +		pwm_min -= pwm_step;
> > +
> > +	return pwm_min;
> 
> This is getting tricky, and you assume too much.
> 
> Currently, 0 means off if dev_priv->vbt.backlight.min_brightness == 0 or
> the panel is eDP. LVDS panels with non-zero min brightness aren't
> switched off with 0 brightness.

The following change makes sysfs brightness 0 to turn off the backlight.

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

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

 drivers/gpu/drm/i915/intel_panel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index af54356..764f928 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -986,7 +986,8 @@ 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;
+                       bool enable = bd->props.power == FB_BLANK_UNBLANK &&
+                               bd->props.brightness != 0;
                        panel->backlight_power(connector, enable);
                }
        } else {

But it also abandoned the lowest brightness level from VBT because it is used
to turn off the backlight.

> 
> Pardon my bluntness, but I'm starting to feel like you just want to get
> a patch in. Take my word for it, backlight is not where you should get
> started.
My intention is to respect the VBT minimum backlight brightness again.
Just like what you did before.

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

    drm/i915: respect the VBT minimum backlight brightness

I don't mind if you can fix this minor issue for me.
However I am interested to work with the upstream developer.
Thanks a lot for your feedback and let me know many things about the
brightness control in i915.

BTW, I wrote the brightness kernel module (ideapad-laptop) before.

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