On Tue, Nov 10, 2015 at 12:15:48PM +0200, Jani Nikula wrote:
> On Tue, 10 Nov 2015, "Shih-Yuan Lee (FourDollars)" <sylee@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
> 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@xxxxxxxxx>
> 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@xxxxxxxxx>
> 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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
