Re: i915 backlight

Re: i915 backlight

[Rafael J. Wysocki]

On Wednesday, July 31, 2013 06:36:23 PM Borislav Petkov wrote:
> On Wed, Jul 31, 2013 at 06:22:52PM +0200, Borislav Petkov wrote:
> > Dudes,
> > 
> > has anyone already reported this (happens on Linus of today +
> > tip/master):
> 
> Oh, one more thing: I can't control the backlight anymore on this x230
> with the Fn-Fx keys and this is most probably related to that recent
> backlight revert story. If there is a new approach I can test, please
> let me know.
> 
> Btw, it worked before on this machine with "acpi_backlight=vendor" on
> the cmdline.

Does reverting efaa14c help?

Rafael

Re: i915 backlight

[Borislav Petkov]

On Wed, Jul 31, 2013 at 11:16:52PM +0200, Rafael J. Wysocki wrote:
> Does reverting efaa14c help?

Nope.

But see my other reply to Aaron.

Thanks.

Re: i915 backlight

[Aaron Lu]

On 08/01/2013 04:07 PM, Borislav Petkov wrote:
> On Wed, Jul 31, 2013 at 11:16:52PM +0200, Rafael J. Wysocki wrote:
>> Does reverting efaa14c help?
> 
> Nope.
> 
> But see my other reply to Aaron.

Assume you have specified to use intel_backlight in xorg.conf, does
booting with video.brightness_switch_enabled=0 help?

Thanks,
Aaron

Re: i915 backlight

[Josep Lladonosa]

Hello,

I am using a Lenovo Edge E530 and, with kernel 3.11.0-rc3, I had to
change to this parameter to the kernel boot:


GRUB_CMDLINE_LINUX="acpi_osi=\"!Windows 2012\""


instead of previous

GRUB_CMDLINE_LINUX="acpi_osi=Linux acpi_backlight=vendor"

to be able to change brightness. In some kernel versions before, it
worked, but with 15 levels, but in graphical system brightness bar was
not moving.


Now I have, though, 8 possible values for brightness and brightness
bar shows its correct position.

Josep

On 2 August 2013 08:00, Aaron Lu <aaron.lu@intel.com> wrote:
> On 08/01/2013 04:07 PM, Borislav Petkov wrote:
>> On Wed, Jul 31, 2013 at 11:16:52PM +0200, Rafael J. Wysocki wrote:
>>> Does reverting efaa14c help?
>>
>> Nope.
>>
>> But see my other reply to Aaron.
>
> Assume you have specified to use intel_backlight in xorg.conf, does
> booting with video.brightness_switch_enabled=0 help?
>
> Thanks,
> Aaron
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
--
Salutacions...Josep
--

Re: i915 backlight

[Aaron Lu]

On 08/02/2013 02:25 PM, Josep Lladonosa wrote:
> Hello,
> 
> I am using a Lenovo Edge E530 and, with kernel 3.11.0-rc3, I had to
> change to this parameter to the kernel boot:
> 
> 
> GRUB_CMDLINE_LINUX="acpi_osi=\"!Windows 2012\""

What if you remove the above from kernel command line, and add
video.brightness_switch_enabled=0 to kernel command line, then set the
following in xorg.conf:
$ cat /etc/X11/xorg.conf
Section "Device"
        Option     "Backlight"	"intel_backlight"
	Identifier  "Card0"
	Driver      "intel"
	BusID       "PCI:0:2:0"
EndSection

Does everything work?

If not, please test if manually change brightness level through sysfs
works:
# cd /sys/calss/backlight/intel_backlight
# echo xxx > brightness

And also test if hotkey event is sent out or not by running acpi_listen
and then press the hotkey.

Thanks,
Aaron

> 
> 
> instead of previous
> 
> GRUB_CMDLINE_LINUX="acpi_osi=Linux acpi_backlight=vendor"
> 
> to be able to change brightness. In some kernel versions before, it
> worked, but with 15 levels, but in graphical system brightness bar was
> not moving.
> 
> 
> Now I have, though, 8 possible values for brightness and brightness
> bar shows its correct position.
> 
> Josep
> 
> On 2 August 2013 08:00, Aaron Lu <aaron.lu@intel.com> wrote:
>> On 08/01/2013 04:07 PM, Borislav Petkov wrote:
>>> On Wed, Jul 31, 2013 at 11:16:52PM +0200, Rafael J. Wysocki wrote:
>>>> Does reverting efaa14c help?
>>>
>>> Nope.
>>>
>>> But see my other reply to Aaron.
>>
>> Assume you have specified to use intel_backlight in xorg.conf, does
>> booting with video.brightness_switch_enabled=0 help?
>>
>> Thanks,
>> Aaron
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 
> 

Re: i915 backlight

[Josep Lladonosa]

Hello,

Yes, it works! I get now 11 levels from all black to the brightest.

acpi_listen shows messages

Josep

On 2 August 2013 08:36, Aaron Lu <aaron.lu@intel.com> wrote:
> On 08/02/2013 02:25 PM, Josep Lladonosa wrote:
>> Hello,
>>
>> I am using a Lenovo Edge E530 and, with kernel 3.11.0-rc3, I had to
>> change to this parameter to the kernel boot:
>>
>>
>> GRUB_CMDLINE_LINUX="acpi_osi=\"!Windows 2012\""
>
> What if you remove the above from kernel command line, and add
> video.brightness_switch_enabled=0 to kernel command line, then set the
> following in xorg.conf:
> $ cat /etc/X11/xorg.conf
> Section "Device"
>         Option     "Backlight"  "intel_backlight"
>         Identifier  "Card0"
>         Driver      "intel"
>         BusID       "PCI:0:2:0"
> EndSection
>
> Does everything work?
>
> If not, please test if manually change brightness level through sysfs
> works:
> # cd /sys/calss/backlight/intel_backlight
> # echo xxx > brightness
>
> And also test if hotkey event is sent out or not by running acpi_listen
> and then press the hotkey.
>
> Thanks,
> Aaron
>
>>
>>
>> instead of previous
>>
>> GRUB_CMDLINE_LINUX="acpi_osi=Linux acpi_backlight=vendor"
>>
>> to be able to change brightness. In some kernel versions before, it
>> worked, but with 15 levels, but in graphical system brightness bar was
>> not moving.
>>
>>
>> Now I have, though, 8 possible values for brightness and brightness
>> bar shows its correct position.
>>
>> Josep
>>
>> On 2 August 2013 08:00, Aaron Lu <aaron.lu@intel.com> wrote:
>>> On 08/01/2013 04:07 PM, Borislav Petkov wrote:
>>>> On Wed, Jul 31, 2013 at 11:16:52PM +0200, Rafael J. Wysocki wrote:
>>>>> Does reverting efaa14c help?
>>>>
>>>> Nope.
>>>>
>>>> But see my other reply to Aaron.
>>>
>>> Assume you have specified to use intel_backlight in xorg.conf, does
>>> booting with video.brightness_switch_enabled=0 help?
>>>
>>> Thanks,
>>> Aaron
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>>
>>
>



-- 
--
Salutacions...Josep
--

Re: i915 backlight

[Felipe Contreras]

On Fri, Aug 2, 2013 at 1:25 AM, Josep Lladonosa <jlladono@gmail.com> wrote:
> Hello,
>
> I am using a Lenovo Edge E530 and, with kernel 3.11.0-rc3, I had to
> change to this parameter to the kernel boot:
>
>
> GRUB_CMDLINE_LINUX="acpi_osi=\"!Windows 2012\""

I think it's pretty obvious that for the time being we need to
blacklist a ton of machines so they boot without this OSI. In fact, in
might make sense to simply remove the OSI completely for all machines
(for now).

-- 
Felipe Contreras

Re: i915 backlight

[Rafael J. Wysocki]

On Friday, August 02, 2013 01:48:37 AM Felipe Contreras wrote:
> On Fri, Aug 2, 2013 at 1:25 AM, Josep Lladonosa <jlladono@gmail.com> wrote:
> > Hello,
> >
> > I am using a Lenovo Edge E530 and, with kernel 3.11.0-rc3, I had to
> > change to this parameter to the kernel boot:
> >
> >
> > GRUB_CMDLINE_LINUX="acpi_osi=\"!Windows 2012\""
> 
> I think it's pretty obvious that for the time being we need to
> blacklist a ton of machines so they boot without this OSI. In fact, in
> might make sense to simply remove the OSI completely for all machines
> (for now).

That would have made sense 6 months ago, but not today.

The reason is that you don't really know what's affected by that and I'm
pretty sure it's not only backlight.

So no, we won't do that.

We *might* blacklist machines that shipped with Windows 7, but whose BIOSes
call the Windows 8 OSI, because there's a good chance they weren't really
tested with Windows 8.

Thanks,
Rafael

Re: i915 backlight

[Felipe Contreras]

On Fri, Aug 2, 2013 at 9:03 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, August 02, 2013 01:48:37 AM Felipe Contreras wrote:
>> On Fri, Aug 2, 2013 at 1:25 AM, Josep Lladonosa <jlladono@gmail.com> wrote:
>> > Hello,
>> >
>> > I am using a Lenovo Edge E530 and, with kernel 3.11.0-rc3, I had to
>> > change to this parameter to the kernel boot:
>> >
>> >
>> > GRUB_CMDLINE_LINUX="acpi_osi=\"!Windows 2012\""
>>
>> I think it's pretty obvious that for the time being we need to
>> blacklist a ton of machines so they boot without this OSI. In fact, in
>> might make sense to simply remove the OSI completely for all machines
>> (for now).
>
> That would have made sense 6 months ago, but not today.

Today, like 6 months ago these machines remain broken, and it will be
the same tomorrow, presumably on v3.11, and at least v3.12 as well.

> The reason is that you don't really know what's affected by that and I'm
> pretty sure it's not only backlight.

I haven't heard a single comment that says acpi_osi="!Windows 2012"
breaks other things. OTOH everybody is saying it fixes the backlight
problem (if indeed it's the same problem).

Are you claiming that those users are wrong?

> So no, we won't do that.

Yeah, because that would fix the backlight problems, not tomorrow, or
several months from now, *today*. Geez, who would want that?

Here is the patch to fix the problem, *today*.

https://bugzilla.kernel.org/show_bug.cgi?id=60682

This is what we should do:

1) Improve that blacklist list
2) Fix the Intel driver issues
3) Enable your patch that uses the Intel driver instead
4) Remove that patch

Anything else is not be good for the users.

-- 
Felipe Contreras

Re: i915 backlight

[Josep Lladonosa]

Hi,

With this setup, something has happened: in xorg, when screen goes to
screensaver and after, enters into Standby mode, when I press a key,
it keeps black and, to recover screen, I have to adjust brightness
manually (by increasing), as if it didn't remember previous value to
standby mode.

This was something that before didn't happen.

Josep

On 2 August 2013 20:58, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> On Fri, Aug 2, 2013 at 9:03 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> On Friday, August 02, 2013 01:48:37 AM Felipe Contreras wrote:
>>> On Fri, Aug 2, 2013 at 1:25 AM, Josep Lladonosa <jlladono@gmail.com> wrote:
>>> > Hello,
>>> >
>>> > I am using a Lenovo Edge E530 and, with kernel 3.11.0-rc3, I had to
>>> > change to this parameter to the kernel boot:
>>> >
>>> >
>>> > GRUB_CMDLINE_LINUX="acpi_osi=\"!Windows 2012\""
>>>
>>> I think it's pretty obvious that for the time being we need to
>>> blacklist a ton of machines so they boot without this OSI. In fact, in
>>> might make sense to simply remove the OSI completely for all machines
>>> (for now).
>>
>> That would have made sense 6 months ago, but not today.
>
> Today, like 6 months ago these machines remain broken, and it will be
> the same tomorrow, presumably on v3.11, and at least v3.12 as well.
>
>> The reason is that you don't really know what's affected by that and I'm
>> pretty sure it's not only backlight.
>
> I haven't heard a single comment that says acpi_osi="!Windows 2012"
> breaks other things. OTOH everybody is saying it fixes the backlight
> problem (if indeed it's the same problem).
>
> Are you claiming that those users are wrong?
>
>> So no, we won't do that.
>
> Yeah, because that would fix the backlight problems, not tomorrow, or
> several months from now, *today*. Geez, who would want that?
>
> Here is the patch to fix the problem, *today*.
>
> https://bugzilla.kernel.org/show_bug.cgi?id=60682
>
> This is what we should do:
>
> 1) Improve that blacklist list
> 2) Fix the Intel driver issues
> 3) Enable your patch that uses the Intel driver instead
> 4) Remove that patch
>
> Anything else is not be good for the users.
>
> --
> Felipe Contreras



-- 
--
Salutacions...Josep
--

Re: i915 backlight

[Felipe Contreras]

On Fri, Aug 2, 2013 at 3:03 PM, Josep Lladonosa <jlladono@gmail.com> wrote:
> With this setup, something has happened: in xorg, when screen goes to
> screensaver and after, enters into Standby mode, when I press a key,
> it keeps black and, to recover screen, I have to adjust brightness
> manually (by increasing), as if it didn't remember previous value to
> standby mode.
>
> This was something that before didn't happen.

You mean with acpi_osi="!Windows 2012"? And when you say "before",
what do you mean?

-- 
Felipe Contreras

Re: i915 backlight

[Josep Lladonosa]

"Before" means with previous kernels that worked with

GRUB_CMDLINE_LINUX="acpi_osi=Linux acpi_backlight=vendor"

I have not checked this issue with acpi_osi="!Windows 2012".

Josep

On 2 August 2013 22:08, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> On Fri, Aug 2, 2013 at 3:03 PM, Josep Lladonosa <jlladono@gmail.com> wrote:
>> With this setup, something has happened: in xorg, when screen goes to
>> screensaver and after, enters into Standby mode, when I press a key,
>> it keeps black and, to recover screen, I have to adjust brightness
>> manually (by increasing), as if it didn't remember previous value to
>> standby mode.
>>
>> This was something that before didn't happen.
>
> You mean with acpi_osi="!Windows 2012"? And when you say "before",
> what do you mean?
>
> --
> Felipe Contreras



-- 
--
Salutacions...Josep
--

Re: i915 backlight

[Borislav Petkov]

On Fri, Aug 02, 2013 at 10:11:27PM +0200, Josep Lladonosa wrote:
> "Before" means with previous kernels that worked with
> 
> GRUB_CMDLINE_LINUX="acpi_osi=Linux acpi_backlight=vendor"
> 
> I have not checked this issue with acpi_osi="!Windows 2012".

Hey Josep,

would you please not top-post when you reply?

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: i915 backlight

[Felipe Contreras]

On Fri, Aug 2, 2013 at 3:11 PM, Josep Lladonosa <jlladono@gmail.com> wrote:
> "Before" means with previous kernels that worked with
>
> GRUB_CMDLINE_LINUX="acpi_osi=Linux acpi_backlight=vendor"

That's probably a different issue. You would need to bisect the problem.

> I have not checked this issue with acpi_osi="!Windows 2012".

Please do.

-- 
Felipe Contreras

Re: i915 backlight

[Josep Lladonosa]

On 2 August 2013 23:25, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> On Fri, Aug 2, 2013 at 3:11 PM, Josep Lladonosa <jlladono@gmail.com> wrote:
>> "Before" means with previous kernels that worked with
>>
>> GRUB_CMDLINE_LINUX="acpi_osi=Linux acpi_backlight=vendor"
>
> That's probably a different issue. You would need to bisect the problem.
>
>> I have not checked this issue with acpi_osi="!Windows 2012".
>
> Please do.

Hello, checking with acpi_osi="!Windows 2012" in the cmdline for
kernel 3.11.0-rc3 (and without the section you gave for the xorg.conf)
i get:

- brightness can be set among 16 levels.
- brightness recovers fine after a standby.
- brightness can be managed before entering xorg (during lightdm
screen, in my case. With the configuration of xorg.conf and
video.brightness_switch_enabled=0 it is not possible, only until xorg
has started).

Josep

>
> --
> Felipe Contreras



-- 
--
Salutacions...Josep
--

Re: i915 backlight

[Rafael J. Wysocki]

On Friday, August 02, 2013 01:58:55 PM Felipe Contreras wrote:
> On Fri, Aug 2, 2013 at 9:03 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Friday, August 02, 2013 01:48:37 AM Felipe Contreras wrote:
> >> On Fri, Aug 2, 2013 at 1:25 AM, Josep Lladonosa <jlladono@gmail.com> wrote:
> >> > Hello,
> >> >
> >> > I am using a Lenovo Edge E530 and, with kernel 3.11.0-rc3, I had to
> >> > change to this parameter to the kernel boot:
> >> >
> >> >
> >> > GRUB_CMDLINE_LINUX="acpi_osi=\"!Windows 2012\""
> >>
> >> I think it's pretty obvious that for the time being we need to
> >> blacklist a ton of machines so they boot without this OSI. In fact, in
> >> might make sense to simply remove the OSI completely for all machines
> >> (for now).
> >
> > That would have made sense 6 months ago, but not today.
> 
> Today, like 6 months ago these machines remain broken, and it will be
> the same tomorrow, presumably on v3.11, and at least v3.12 as well.

Can you possibly look at things from a bit broader perspective than just the
broken backlight?

[I'm talking about "simply removing the OSI completely for all machines" if
that's not clear.]

The problem is that for the last 6 months the kernel has responded to
OSI(Windows 2012) with a "yes" and now, after that time, you want it to
make a U turn and start saying "no" even though that may cause problems to
happen on other people's machines.  That's simply irresponsible.

> > The reason is that you don't really know what's affected by that and I'm
> > pretty sure it's not only backlight.
> 
> I haven't heard a single comment that says acpi_osi="!Windows 2012"
> breaks other things. OTOH everybody is saying it fixes the backlight
> problem (if indeed it's the same problem).
> 
> Are you claiming that those users are wrong?

No, they are saying what they see and they are the people having the backlight
problem in the first place.  You have no data from people for whom things work
now.

> > So no, we won't do that.
> 
> Yeah, because that would fix the backlight problems, not tomorrow, or
> several months from now, *today*. Geez, who would want that?
> 
> Here is the patch to fix the problem, *today*.

It doesn't "fix" anything.  It just creates a blacklist of systems where
acpi_osi="!Windows 2012" happens to help with the backlight control problem.

You don't even know why exactly it happens to work on those machines in the
first place and you don't know what is affected by that apart from backlight
(you can't be sure that nothing is affected in particular).

> https://bugzilla.kernel.org/show_bug.cgi?id=60682
> 
> This is what we should do:
> 
> 1) Improve that blacklist list
> 2) Fix the Intel driver issues
> 3) Enable your patch that uses the Intel driver instead
> 4) Remove that patch
> 
> Anything else is not be good for the users.

Actually, the users can easily put the acpi_osi="!Windows 2012" into the
kernel command line (which is what they have been doing already for some
time I suppose).  However, if we add the blacklist to the kernel, that will
mean we kind of give up fixing the backlight control for them (because they
won't have any incentive to test anything else then).

That said this is a controverisal matter and we evidently don't agree with
each other.  I have my reasons, you have your arguments and it doesn't look
like any of us is likely to change his mind, so why don't we do what's
normally done in such cases: Why don't we ask others?

Matthew, Aaron, Rui, what do you think about this?  Should we create an
acpi_osi="!Windows 2012" blacklist of systems where this workaround is known
to help with backlight control issues?  Is this a good idea in your opinion?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: i915 backlight

[Felipe Contreras]

On Fri, Aug 2, 2013 at 6:35 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, August 02, 2013 01:58:55 PM Felipe Contreras wrote:
>> On Fri, Aug 2, 2013 at 9:03 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Friday, August 02, 2013 01:48:37 AM Felipe Contreras wrote:

>> >> I think it's pretty obvious that for the time being we need to
>> >> blacklist a ton of machines so they boot without this OSI. In fact, in
>> >> might make sense to simply remove the OSI completely for all machines
>> >> (for now).
>> >
>> > That would have made sense 6 months ago, but not today.
>>
>> Today, like 6 months ago these machines remain broken, and it will be
>> the same tomorrow, presumably on v3.11, and at least v3.12 as well.
>
> Can you possibly look at things from a bit broader perspective than just the
> broken backlight?
>
> [I'm talking about "simply removing the OSI completely for all machines" if
> that's not clear.]
>
> The problem is that for the last 6 months the kernel has responded to
> OSI(Windows 2012) with a "yes" and now, after that time, you want it to
> make a U turn and start saying "no" even though that may cause problems to
> happen on other people's machines.  That's simply irresponsible.

The difference is we *know* *for sure* responding "Windows 2012" with
a yes causes problems, on the other hand you *think* responding with a
no, *might* cause problems.

The only reason it would make sense to remain in the current situation
is that if *knew* that switching to no would cause problems, but to my
knowledge such problems are not real, merely theoretical.

We have had proven brokedness for four releases (almost five now), if
you disable it for v3.12 and it turns out it causes even more
problems, then you enable it back again for v3.13, or even v3.12.1.
The only way to know for certain is to go ahead and try it.

But we know it's going to be all right, because v3.6 was all right.

>> > The reason is that you don't really know what's affected by that and I'm
>> > pretty sure it's not only backlight.
>>
>> I haven't heard a single comment that says acpi_osi="!Windows 2012"
>> breaks other things. OTOH everybody is saying it fixes the backlight
>> problem (if indeed it's the same problem).
>>
>> Are you claiming that those users are wrong?
>
> No, they are saying what they see and they are the people having the backlight
> problem in the first place.  You have no data from people for whom things work
> now.

The data comes from v3.6, who complained? Anyway, it's easy to find
out; just disable it for *one release*.

>> > So no, we won't do that.
>>
>> Yeah, because that would fix the backlight problems, not tomorrow, or
>> several months from now, *today*. Geez, who would want that?
>>
>> Here is the patch to fix the problem, *today*.
>
> It doesn't "fix" anything.  It just creates a blacklist of systems where
> acpi_osi="!Windows 2012" happens to help with the backlight control problem.

>From the user's point of view; right now it doesn't work, after the
patch it does. That is a fix.

> You don't even know why exactly it happens to work on those machines in the
> first place and you don't know what is affected by that apart from backlight
> (you can't be sure that nothing is affected in particular).

I have a fairly good idea of the *real* problems involved with the backlight.

The other problems you speak of are hypothetical, and yes, I don't
know if there will be more problems, but neither do you. This is an
argument from ignorance fallacy, and it's easy to solve; let's try it
for one release and see how it goes. Then we would *know*.

>> https://bugzilla.kernel.org/show_bug.cgi?id=60682
>>
>> This is what we should do:
>>
>> 1) Improve that blacklist list
>> 2) Fix the Intel driver issues
>> 3) Enable your patch that uses the Intel driver instead
>> 4) Remove that patch
>>
>> Anything else is not be good for the users.
>
> Actually, the users can easily put the acpi_osi="!Windows 2012" into the
> kernel command line (which is what they have been doing already for some
> time I suppose).

The kernel should just work out-of-the-box. You can't expect every
user out there to put all sorts of quirks into their boot command,
that's why we have quirks in the kernel in the first place.

> However, if we add the blacklist to the kernel, that will
> mean we kind of give up fixing the backlight control for them (because they
> won't have any incentive to test anything else then).

No, it doesn't mean that.

You should not break systems willingly and knowingly only to create
incentives to the developers.

When things are ready, you enable the fix again, if they break, you
disable it again. At no point in time does it make sense to retain
code that we know breaks user-experience.

> That said this is a controverisal matter and we evidently don't agree with
> each other.  I have my reasons, you have your arguments and it doesn't look
> like any of us is likely to change his mind, so why don't we do what's
> normally done in such cases: Why don't we ask others?
>
> Matthew, Aaron, Rui, what do you think about this?  Should we create an
> acpi_osi="!Windows 2012" blacklist of systems where this workaround is known
> to help with backlight control issues?  Is this a good idea in your opinion?

Mind that the suggestion is to do this *temporarily*, until there's
more certainty that the proper fix works.

-- 
Felipe Contreras

Re: i915 backlight

[Borislav Petkov]

On Fri, Aug 02, 2013 at 02:00:42PM +0800, Aaron Lu wrote:
> Assume you have specified to use intel_backlight in xorg.conf

Right, I have:

Section "Device"
        Option "Backlight"      "intel_backlight"
        Identifier  "Card0"
        Driver      "intel"
        BusID       "PCI:0:2:0"
EndSection

in there currently.

> does booting with video.brightness_switch_enabled=0 help?

Nope.

Kernel command line is like this:

[    0.000000] Command line: BOOT_IMAGE=/boot/vmlinuz-3.11.0-rc3+ root=/dev/sdb2 ro root=/dev/sdb2 ignore_loglevel log_buf_len=10M resume=/dev/sdb1 acpi_backlight=vendor video.brightness_switch_enabled=0 i915.i915_enable_rc6=4 i915.i915_enable_fbc=1 i915.lvds_downclock=1 i915.powersave=1

and acpi_backlight=vendor doesn't make any difference. It still works
over sysfs though.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: i915 backlight

[Aaron Lu]

On 08/01/2013 05:07 PM, Aaron Lu wrote:
> On 08/01/2013 04:12 PM, Borislav Petkov wrote:
>> On Thu, Aug 01, 2013 at 09:13:35AM +0800, Aaron Lu wrote:
>>> Can you please run acpi_listen and then press the Fn-Fx key, see if the
>>> events are correctly sent out?
>>
>> Like this?
>>
>> # acpi_listen
>> video/brightnessdown BRTDN 00000087 00000000
>> video/brightnessup BRTUP 00000086 00000000
>> video/brightnessdown BRTDN 00000087 00000000
>> video/brightnessup BRTUP 00000086 00000000
>> video/brightnessdown BRTDN 00000087 00000000
>> video/brightnessup BRTUP 00000086 00000000
>> video/brightnessdown BRTDN 00000087 00000000
>> video/brightnessup BRTUP 00000086 00000000
>> video/brightnessdown BRTDN 00000087 00000000
>> ^C
> 
> Yes, so the event is correctly sent out.
> 
>>
>>> From the bug page:
>>> https://bugzilla.kernel.org/show_bug.cgi?id=51231#c80
>>> I got the impression that both the acpi_video interface and the vendor
>>> interface thinkpad_screen are broken. So adding this cmdline here works
>>> suggests that either thinkpad_screen works or thinkpad vendor driver
>>> doesn't get loaded or doesn't create that interface for some reason.
>>>
>>> Alternatively, if the intel_backlight interface works(highly possible),
>>> you can use xorg.conf to specify the that backlight interface for X.
>>>
>>> Section "Device"
>>>         Option     "Backlight"	"intel_backlight"
>>> 	Identifier  "Card0"
>>> 	Driver      "intel"
>>> 	BusID       "PCI:0:2:0"
>>> EndSection
>>
>> Yeah, that didn't work *but* manually writing to both:
>>
>> /sys/class/backlight/acpi_video0/brightness
>>
>> and
>>
>> /sys/class/backlight/intel_backlight/brightness
>>
>> works.
> 
> Err...we have the event sent out on hotkey press and the interface also
> works, but still, using hotkey to adjust brightness level is broken...
> 
> I just found an old acer laptop that has similar issue(or even worse: on
> X starts, an almost black screen is shown and hotkey adjust doesn't
> work), I'll look into this.

Hi Jani & Daniel,

It turned out there is an integer overflow problem, and the below patch
fixed this problem on Acer Aspire 4732Z and thinkpad R61i.

From: Aaron Lu <aaron.lu@intel.com>
Subject: [PATCH] drm/i915: avoid brightness overflow when doing scale

Some card's max brightness level is pretty large, e.g. on Acer Aspire
4732Z, the max level is 989910. If user space set a large enough level
then the current scale done in intel_panel_set_backlight will cause an
integer overflow and the scaled level will be mistakenly small, leaving
user with an almost black screen. This patch fixes this problem.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
Since the only external user of intel_panel_set_backlight is operation
region code where the max will be a constant of 255, this patch fixes
the problem by comparing freq and max and then do things accordingly
instead of converting to 64 bits.

 drivers/gpu/drm/i915/intel_panel.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 67e2c1f..7c674f0 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -498,7 +498,10 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level, u32 max)
 	}
 
 	/* scale to hardware */
-	level = level * freq / max;
+	if (freq < max)
+		level = level * freq / max;
+	else
+		level = freq / max * level;
 
 	dev_priv->backlight.level = level;
 	if (dev_priv->backlight.device)
-- 
1.8.3.1


Hi Boris,

Since the sysfs interface works on your system, I think your problem
should be different. Can you please file a bug for this? I can provide
you with a debug patch and then see what happened. Please attach
acpidump when filing the bug.

https://bugzilla.kernel.org, ACPI/Power-Video.

Thanks,
Aaron

> 
>>
>> The ranges are different, though:
>>
>> intel_backlight/actual_brightness:1000
>> intel_backlight/bl_power:0
>> intel_backlight/brightness:1000
>> intel_backlight/max_brightness:4437
>> intel_backlight/type:raw
>>
>> acpi_video0/actual_brightness:41
>> acpi_video0/bl_power:0
>> acpi_video0/brightness:41
>> acpi_video0/max_brightness:100
>> acpi_video0/type:firmware
> 
> Yes, different interface has different brightness ranges and a value in
> one range may turn out to be the same actual brightness level of another
> value in another range.
> 
>>
>> I guess I need to write me a dirty script for now ... :-)
> 
> :-)
> 
>>
>> Thanks guys.
>>
> Thanks,
> Aaron
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: i915 backlight

[Borislav Petkov]

On Fri, Aug 02, 2013 at 09:16:03AM +0800, Aaron Lu wrote:
> Since the sysfs interface works on your system, I think your problem
> should be different. Can you please file a bug for this? I can provide
> you with a debug patch and then see what happened. Please attach
> acpidump when filing the bug.
>
> https://bugzilla.kernel.org, ACPI/Power-Video.

Done: https://bugzilla.kernel.org/show_bug.cgi?id=60680

I did acpidump by hand but it should be ok.

Thanks for looking into this Aaron! :)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: i915 backlight

[Daniel Vetter]

On Fri, Aug 02, 2013 at 09:16:03AM +0800, Aaron Lu wrote:
> Hi Jani & Daniel,
> 
> It turned out there is an integer overflow problem, and the below patch
> fixed this problem on Acer Aspire 4732Z and thinkpad R61i.
> 
> From: Aaron Lu <aaron.lu@intel.com>
> Subject: [PATCH] drm/i915: avoid brightness overflow when doing scale
> 
> Some card's max brightness level is pretty large, e.g. on Acer Aspire
> 4732Z, the max level is 989910. If user space set a large enough level
> then the current scale done in intel_panel_set_backlight will cause an
> integer overflow and the scaled level will be mistakenly small, leaving
> user with an almost black screen. This patch fixes this problem.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
> Since the only external user of intel_panel_set_backlight is operation
> region code where the max will be a constant of 255, this patch fixes
> the problem by comparing freq and max and then do things accordingly
> instead of converting to 64 bits.

Yeah, makes sense. Picked up for -fixes, thanks for the patch.
-Daniel
> 
>  drivers/gpu/drm/i915/intel_panel.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 67e2c1f..7c674f0 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -498,7 +498,10 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level, u32 max)
>  	}
>  
>  	/* scale to hardware */
> -	level = level * freq / max;
> +	if (freq < max)
> +		level = level * freq / max;
> +	else
> +		level = freq / max * level;
>  
>  	dev_priv->backlight.level = level;
>  	if (dev_priv->backlight.device)
> -- 
> 1.8.3.1
> 
> 
> Hi Boris,
> 
> Since the sysfs interface works on your system, I think your problem
> should be different. Can you please file a bug for this? I can provide
> you with a debug patch and then see what happened. Please attach
> acpidump when filing the bug.
> 
> https://bugzilla.kernel.org, ACPI/Power-Video.
> 
> Thanks,
> Aaron
> 
> > 
> >>
> >> The ranges are different, though:
> >>
> >> intel_backlight/actual_brightness:1000
> >> intel_backlight/bl_power:0
> >> intel_backlight/brightness:1000
> >> intel_backlight/max_brightness:4437
> >> intel_backlight/type:raw
> >>
> >> acpi_video0/actual_brightness:41
> >> acpi_video0/bl_power:0
> >> acpi_video0/brightness:41
> >> acpi_video0/max_brightness:100
> >> acpi_video0/type:firmware
> > 
> > Yes, different interface has different brightness ranges and a value in
> > one range may turn out to be the same actual brightness level of another
> > value in another range.
> > 
> >>
> >> I guess I need to write me a dirty script for now ... :-)
> > 
> > :-)
> > 
> >>
> >> Thanks guys.
> >>
> > Thanks,
> > Aaron
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch