Re: [PATCH] drm/i915: A better maximum brightness for users.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 09, 2015 at 10:02:24PM +0800, Shih-Yuan Lee (FourDollars) wrote:
> On Mon, Nov 9, 2015 at 8:58 PM, Jani Nikula <jani.nikula@linux.intel.com>
> wrote:
> 
> > On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" <sylee@canonical.com>
> > wrote:
> > > On Mon, Nov 9, 2015 at 6:51 PM, Jani Nikula <jani.nikula@linux.intel.com
> > >
> > > wrote:
> > >
> > >> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" <sylee@canonical.com
> > >
> > >> wrote:
> > >> > On Mon, Nov 9, 2015 at 6:17 PM, Jani Nikula <
> > jani.nikula@linux.intel.com
> > >> >
> > >> > wrote:
> > >> >
> > >> >> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" <
> > sylee@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.

BTW there's that BLCM thing in opregion that provides some kind of
non-linear remapping for backlight levels. A while back I got fed up
with the uneven brightness steps on my IVB X1 Carbon, and so I cooked
up a patch to use BLCM. It did improve the situation quite a bit.

Pushed it here in case anyone else is interested:
git://github.com/vsyrjala/linux.git blcm_backlight

> 
> 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@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

> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx