[Intel-gfx] [PATCH] drm/i915: A better maximum brightness for users.

Mon Nov 9 06:02:24 PST 2015

On Mon, Nov 9, 2015 at 8:58 PM, Jani Nikula <jani.nikula at linux.intel.com>
wrote:

> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" <sylee at canonical.com>
> wrote:
> > On Mon, Nov 9, 2015 at 6:51 PM, Jani Nikula <jani.nikula at linux.intel.com
> >
> > wrote:
> >
> >> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" <sylee at canonical.com
> >
> >> wrote:
> >> > On Mon, Nov 9, 2015 at 6:17 PM, Jani Nikula <
> jani.nikula at linux.intel.com
> >> >
> >> > wrote:
> >> >
> >> >> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" <
> sylee at canonical.com
> >> >
> >> >> wrote:
> >> >> > The PWM brightness level of Dell XPS 13 (2015) is from 10 to 937
> >> however
> >> >> > the sysfs brightness level always starts from 0 so it is better to
> use
> >> >> > 927 as the sysfs maximum brightness level and it becomes easier to
> map
> >> >> > from the PWM brightness level to the sysfs brightness level.
> >> >>
> >> >> We've been thinking we should provide a fixed range to userspace
> >> >> instead. Say, 0-100.
> >> >>
> >> >> BR,
> >> >> Jani.
> >> >>
> >> >>
> >> >>
> >> > That might not be a good idea for the backward compatibility.
> >>
> >> Please explain how you think your change is good and a fixed range 0-100
> >> is bad in terms of backward compatibility. Both just arbitrarily change
> >> the max.
> >>
> > The original sysfs brightness range is from 0 to 937. Let's define it as
> A
> > = {0, 1, 2, ..., 937}.
> > The PWM brightness range is from 10 to 937. Let's define it as B = {10,
> 11,
> > 12, ..., 937}.
> > |A| = 938, |B| = 928
> > |A| != |B|
> > It implies A and B is not an 1-1 mapping.
> >
> > My patch is not a arbitrarily change.
> > It makes A' = {0, 1, 2, ..., 927}. |A'| = 928
> > You can see |A'| = |B| so it implies A' and B is an 1-1 mapping.
> > It means any value in A' can be mapped to a different value in B and visa
> > versa.
> >
> > C = {0, 100} has the same mapping problem.
>
> Please tell me why you think this is a problem to begin with. What (user
> perceptible) problem are you trying to solve?
>
I am investigating some backlight issue that the i915.ko brightness
behavior is changed on Dell XPS 13 (2015).
Originally the lowest brightness won't turn off the backlight but the Linux
kernel after 3.19 will turn off the backlight.
Dell's engineer tells me that Windows driver also uses VBT to change the
brightness but it doesn't turn off the backlight.
I am not a dedicated kernel engineer but I have some interest to look at
this issue.
This regression is from
http://lists.freedesktop.org/archives/intel-gfx/2014-August/050642.html.

This patch is found during my investigation for that problem.

>
> I understand we could simplify (or remove) the scaling code with your
> change, but I'm reluctant to go that way when, as I said, we've been
> talking about fixing the range reported to userspace.
>
I personally think i915.ko just needs to respect the settings from VBT.
No matter how many valid levels from VBT, i915.ko just provides the same
levels in brightness sysfs.

>
> Part of the reason for going to 0-100 range would be that there are
> barely that many user distinguishable steps in the backlight level. It
> is silly to have brightness range of, say, 0-937, because you can't
> distinguish them from each other. (Perhaps counter-intuitively, the
> higher the PWM modulation frequency, the fewer user distinguishable
> levels you can actually get.)
>
I think i915.ko doesn't need to care about this problem.
In fact, the very end users only use a scroll bar to change the brightness.
Or they will use brightness up/down hotkeys to change the brightness but
the desktop environments like GNOME will make it only work for 20 levels.

However some advanced users like me may still prefer to have all valid
brightness levels.
That is why I made this patch and this is my first patch for Linux kernel
project.

Regards,
$4

>
> >> Besides, we've changed the max for some platforms before, and the ABI
> >> for backlight sysfs is you can stick a value between 0 and
> >> max_brightness to the brightness attribute. Any userspace that relies on
> >> anything else is broken.
> >>
> >> > However I saw some message as the following.
> >> > [    3.402233] [drm:parse_lfp_backlight] VBT backlight PWM modulation
> >> > frequency 200 Hz, active high, min brightness 10, level 255
> >> >
> >> >
> >> > Does it mean the brightness range is also defined in the BIOS?
> >>
> >> The minimum and the PWM modulation frequency are defined in BIOS. The
> >> modulation frequency is an attribute for the hardware; I think that was
> >> originally exposed as the max was just for implementation convenience.
> >>
> > I don't mean the minimum or the PWM modulation frequency.
> > I mean "level 255".
> > Does it mean the brightness range or something else?
>
> It probably means the suggested initial level of the backlight in some
> units, but AFAICT we don't use that for anything, and frankly I am not
> sure why we log it.
>
> BR,
> Jani.
>
>
> >
> > Regards,
> > $4
> >
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> >
> >> > Regards,
> >> > $4
> >> >
> >> >>
> >> >> >
> >> >> > Signed-off-by: Shih-Yuan Lee (FourDollars) <sylee at canonical.com>
> >> >> > ---
> >> >> >  drivers/gpu/drm/i915/intel_panel.c | 2 +-
> >> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >> >
> >> >> > diff --git a/drivers/gpu/drm/i915/intel_panel.c
> >> >> b/drivers/gpu/drm/i915/intel_panel.c
> >> >> > index a24df35..697fd4d 100644
> >> >> > --- a/drivers/gpu/drm/i915/intel_panel.c
> >> >> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> >> > @@ -1211,7 +1211,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;
> >> >> >       props.brightness = scale_hw_to_user(connector,
> >> >> >                                           panel->backlight.level,
> >> >> >                                           props.max_brightness);
> >> >>
> >> >> --
> >> >> Jani Nikula, Intel Open Source Technology Center
> >> >>
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
> >>
>
> --
> Jani Nikula, Intel Open Source Technology Center
>

-- 
Shih-Yuan Lee (FourDollars) | Software Engineer | Commercial Engineering -
PC & Core Taipei | Ubuntu Engineering and Services | Canonical
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20151109/4746f75e/attachment-0001.html>