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

[Shih-Yuan Lee (FourDollars)]

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.

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

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
>



-- 
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/464af8e9/attachment-0001.html>