<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Nov 9, 2015 at 8:58 PM, Jani Nikula <span dir="ltr"><<a href="mailto:jani.nikula@linux.intel.com" target="_blank">jani.nikula@linux.intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" <<a href="mailto:sylee@canonical.com">sylee@canonical.com</a>> wrote:<br>
> On Mon, Nov 9, 2015 at 6:51 PM, Jani Nikula <<a href="mailto:jani.nikula@linux.intel.com">jani.nikula@linux.intel.com</a>><br>
> wrote:<br>
><br>
>> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" <<a href="mailto:sylee@canonical.com">sylee@canonical.com</a>><br>
>> wrote:<br>
>> > On Mon, Nov 9, 2015 at 6:17 PM, Jani Nikula <<a href="mailto:jani.nikula@linux.intel.com">jani.nikula@linux.intel.com</a><br>
>> ><br>
>> > wrote:<br>
>> ><br>
>> >> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" <<a href="mailto:sylee@canonical.com">sylee@canonical.com</a><br>
>> ><br>
>> >> wrote:<br>
>> >> > The PWM brightness level of Dell XPS 13 (2015) is from 10 to 937<br>
>> however<br>
>> >> > the sysfs brightness level always starts from 0 so it is better to use<br>
>> >> > 927 as the sysfs maximum brightness level and it becomes easier to map<br>
>> >> > from the PWM brightness level to the sysfs brightness level.<br>
>> >><br>
>> >> We've been thinking we should provide a fixed range to userspace<br>
>> >> instead. Say, 0-100.<br>
>> >><br>
>> >> BR,<br>
>> >> Jani.<br>
>> >><br>
>> >><br>
>> >><br>
>> > That might not be a good idea for the backward compatibility.<br>
>><br>
>> Please explain how you think your change is good and a fixed range 0-100<br>
>> is bad in terms of backward compatibility. Both just arbitrarily change<br>
>> the max.<br>
>><br>
> The original sysfs brightness range is from 0 to 937. Let's define it as A<br>
> = {0, 1, 2, ..., 937}.<br>
> The PWM brightness range is from 10 to 937. Let's define it as B = {10, 11,<br>
> 12, ..., 937}.<br>
> |A| = 938, |B| = 928<br>
> |A| != |B|<br>
> It implies A and B is not an 1-1 mapping.<br>
><br>
> My patch is not a arbitrarily change.<br>
> It makes A' = {0, 1, 2, ..., 927}. |A'| = 928<br>
> You can see |A'| = |B| so it implies A' and B is an 1-1 mapping.<br>
> It means any value in A' can be mapped to a different value in B and visa<br>
> versa.<br>
><br>
> C = {0, 100} has the same mapping problem.<br>
<br>
</div></div>Please tell me why you think this is a problem to begin with. What (user<br>
perceptible) problem are you trying to solve?<br></blockquote><div>I am investigating some backlight issue that the i915.ko brightness behavior is changed on Dell XPS 13 (2015).</div><div>Originally the lowest brightness won't turn off the backlight but the Linux kernel after 3.19 will turn off the backlight.</div><div>Dell's engineer tells me that Windows driver also uses VBT to change the brightness but it doesn't turn off the backlight.<br></div><div>I am not a dedicated kernel engineer but I have some interest to look at this issue.</div><div>This regression is from <a href="http://lists.freedesktop.org/archives/intel-gfx/2014-August/050642.html">http://lists.freedesktop.org/archives/intel-gfx/2014-August/050642.html</a>.</div><div><br></div><div><div>This patch is found during my investigation for that problem.</div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
I understand we could simplify (or remove) the scaling code with your<br>
change, but I'm reluctant to go that way when, as I said, we've been<br>
talking about fixing the range reported to userspace.<br></blockquote><div>I personally think i915.ko just needs to respect the settings from VBT.</div><div>No matter how many valid levels from VBT, i915.ko just provides the same levels in brightness sysfs.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Part of the reason for going to 0-100 range would be that there are<br>
barely that many user distinguishable steps in the backlight level. It<br>
is silly to have brightness range of, say, 0-937, because you can't<br>
distinguish them from each other. (Perhaps counter-intuitively, the<br>
higher the PWM modulation frequency, the fewer user distinguishable<br>
levels you can actually get.)<br></blockquote><div>I think i915.ko doesn't need to care about this problem.</div><div>In fact, the very end users only use a scroll bar to change the brightness.</div><div>Or they will use brightness up/down hotkeys to change the brightness but</div><div>the desktop environments like GNOME will make it only work for 20 levels.</div><div><br></div><div>However some advanced users like me may still prefer to have all valid brightness levels.<br></div><div>That is why I made this patch and this is my first patch for Linux kernel project.</div><div><br></div><div>Regards,</div><div>$4</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
>> Besides, we've changed the max for some platforms before, and the ABI<br>
>> for backlight sysfs is you can stick a value between 0 and<br>
>> max_brightness to the brightness attribute. Any userspace that relies on<br>
>> anything else is broken.<br>
>><br>
>> > However I saw some message as the following.<br>
>> > [ 3.402233] [drm:parse_lfp_backlight] VBT backlight PWM modulation<br>
>> > frequency 200 Hz, active high, min brightness 10, level 255<br>
>> ><br>
>> ><br>
>> > Does it mean the brightness range is also defined in the BIOS?<br>
>><br>
>> The minimum and the PWM modulation frequency are defined in BIOS. The<br>
>> modulation frequency is an attribute for the hardware; I think that was<br>
>> originally exposed as the max was just for implementation convenience.<br>
>><br>
> I don't mean the minimum or the PWM modulation frequency.<br>
> I mean "level 255".<br>
> Does it mean the brightness range or something else?<br>
<br>
</span>It probably means the suggested initial level of the backlight in some<br>
units, but AFAICT we don't use that for anything, and frankly I am not<br>
sure why we log it.<br>
<br>
BR,<br>
Jani.<br>
<div class=""><div class="h5"><br>
<br>
><br>
> Regards,<br>
> $4<br>
><br>
>><br>
>> BR,<br>
>> Jani.<br>
>><br>
>><br>
>> ><br>
>> > Regards,<br>
>> > $4<br>
>> ><br>
>> >><br>
>> >> ><br>
>> >> > Signed-off-by: Shih-Yuan Lee (FourDollars) <<a href="mailto:sylee@canonical.com">sylee@canonical.com</a>><br>
>> >> > ---<br>
>> >> > drivers/gpu/drm/i915/intel_panel.c | 2 +-<br>
>> >> > 1 file changed, 1 insertion(+), 1 deletion(-)<br>
>> >> ><br>
>> >> > diff --git a/drivers/gpu/drm/i915/intel_panel.c<br>
>> >> b/drivers/gpu/drm/i915/intel_panel.c<br>
>> >> > index a24df35..697fd4d 100644<br>
>> >> > --- a/drivers/gpu/drm/i915/intel_panel.c<br>
>> >> > +++ b/drivers/gpu/drm/i915/intel_panel.c<br>
>> >> > @@ -1211,7 +1211,7 @@ static int<br>
>> intel_backlight_device_register(struct<br>
>> >> intel_connector *connector)<br>
>> >> > * Note: Everything should work even if the backlight device max<br>
>> >> > * presented to the userspace is arbitrarily chosen.<br>
>> >> > */<br>
>> >> > - props.max_brightness = panel->backlight.max;<br>
>> >> > + props.max_brightness = panel->backlight.max -<br>
>> panel->backlight.min;<br>
>> >> > props.brightness = scale_hw_to_user(connector,<br>
>> >> > panel->backlight.level,<br>
>> >> > props.max_brightness);<br>
>> >><br>
>> >> --<br>
>> >> Jani Nikula, Intel Open Source Technology Center<br>
>> >><br>
>><br>
>> --<br>
>> Jani Nikula, Intel Open Source Technology Center<br>
>><br>
<br>
--<br>
Jani Nikula, Intel Open Source Technology Center<br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature">Shih-Yuan Lee (FourDollars) | Software Engineer | Commercial Engineering - PC & Core Taipei | Ubuntu Engineering and Services | Canonical</div>
</div></div>